Re: [PATCH v2 3/9] PCI/ASPM: Use RMW accessors for changing LNKCTL

From: Rafael J. Wysocki
Date: Wed May 17 2023 - 07:34:10 EST

On Wed, May 17, 2023 at 12:53 PM Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
>
> Don't assume that the device is fully under the control of ASPM and use
> RMW capability accessors which do proper locking to avoid losing
> concurrent updates to the register values.
>
> If configuration fails in pcie_aspm_configure_common_clock(), the
> function attempts to restore the old PCI_EXP_LNKCTL_CCC settings. Store
> only the old PCI_EXP_LNKCTL_CCC bit for the relevant devices rather
> than the content of the whole LNKCTL registers. It aligns better with
> how pcie_lnkctl_clear_and_set() expects its parameter and makes the
> code more obvious to understand.
>
> Fixes: 4ec73791a64b ("PCI: Work around Pericom PCIe-to-PCI bridge Retrain Link erratum")
> Fixes: 86fa6a344209 ("PCI: Factor out pcie_retrain_link() function")
> Fixes: 2a42d9dba784 ("PCIe: ASPM: Break out of endless loop waiting for PCI config bits to switch")
> Fixes: 7d715a6c1ae5 ("PCI: add PCI Express ASPM support")
> Suggested-by: Lukas Wunner <lukas@xxxxxxxxx>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx

Acked-by: Rafael J. Wysocki <rafael@xxxxxxxxxx>

> ---
> drivers/pci/pcie/aspm.c | 39 ++++++++++++++++-----------------------
> 1 file changed, 16 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index dde1ef13d0d1..426fb0bd8e3a 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -213,7 +213,6 @@ static bool pcie_wait_for_retrain(struct pci_dev *pdev)
> static bool pcie_retrain_link(struct pcie_link_state *link)
> {
> struct pci_dev *parent = link->pdev;
> - u16 reg16;
>
> /*
> * Ensure the updated LNKCTL parameters are used during link
> @@ -224,17 +223,14 @@ static bool pcie_retrain_link(struct pcie_link_state *link)
> if (!pcie_wait_for_retrain(parent))
> return false;
>
> - pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &reg16);
> - reg16 |= PCI_EXP_LNKCTL_RL;
> - pcie_capability_write_word(parent, PCI_EXP_LNKCTL, reg16);
> + pcie_capability_set_word(parent, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_RL);
> if (parent->clear_retrain_link) {
> /*
> * Due to an erratum in some devices the Retrain Link bit
> * needs to be cleared again manually to allow the link
> * training to succeed.
> */
> - reg16 &= ~PCI_EXP_LNKCTL_RL;
> - pcie_capability_write_word(parent, PCI_EXP_LNKCTL, reg16);
> + pcie_capability_clear_word(parent, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_RL);
> }
>
> return pcie_wait_for_retrain(parent);
> @@ -248,7 +244,7 @@ static bool pcie_retrain_link(struct pcie_link_state *link)
> static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
> {
> int same_clock = 1;
> - u16 reg16, parent_reg, child_reg[8];
> + u16 reg16, parent_old_ccc, child_old_ccc[8];
> struct pci_dev *child, *parent = link->pdev;
> struct pci_bus *linkbus = parent->subordinate;
> /*
> @@ -270,6 +266,7 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
>
> /* Port might be already in common clock mode */
> pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &reg16);
> + parent_old_ccc = reg16 & PCI_EXP_LNKCTL_CCC;
> if (same_clock && (reg16 & PCI_EXP_LNKCTL_CCC)) {
> bool consistent = true;
>
> @@ -289,22 +286,16 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
> /* Configure downstream component, all functions */
> list_for_each_entry(child, &linkbus->devices, bus_list) {
> pcie_capability_read_word(child, PCI_EXP_LNKCTL, &reg16);
> - child_reg[PCI_FUNC(child->devfn)] = reg16;
> - if (same_clock)
> - reg16 |= PCI_EXP_LNKCTL_CCC;
> - else
> - reg16 &= ~PCI_EXP_LNKCTL_CCC;
> - pcie_capability_write_word(child, PCI_EXP_LNKCTL, reg16);
> + child_old_ccc[PCI_FUNC(child->devfn)] = reg16 & PCI_EXP_LNKCTL_CCC;
> + pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
> + PCI_EXP_LNKCTL_CCC,
> + same_clock ? PCI_EXP_LNKCTL_CCC : 0);
> }
>
> /* Configure upstream component */
> - pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &reg16);
> - parent_reg = reg16;
> - if (same_clock)
> - reg16 |= PCI_EXP_LNKCTL_CCC;
> - else
> - reg16 &= ~PCI_EXP_LNKCTL_CCC;
> - pcie_capability_write_word(parent, PCI_EXP_LNKCTL, reg16);
> + pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
> + PCI_EXP_LNKCTL_CCC,
> + same_clock ? PCI_EXP_LNKCTL_CCC : 0);
>
> if (pcie_retrain_link(link))
> return;
> @@ -312,9 +303,11 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
> /* Training failed. Restore common clock configurations */
> pci_err(parent, "ASPM: Could not configure common clock\n");
> list_for_each_entry(child, &linkbus->devices, bus_list)
> - pcie_capability_write_word(child, PCI_EXP_LNKCTL,
> - child_reg[PCI_FUNC(child->devfn)]);
> - pcie_capability_write_word(parent, PCI_EXP_LNKCTL, parent_reg);
> + pcie_capability_clear_and_set_word(child, PCI_EXP_LNKCTL,
> + PCI_EXP_LNKCTL_CCC,
> + child_old_ccc[PCI_FUNC(child->devfn)]);
> + pcie_capability_clear_and_set_word(parent, PCI_EXP_LNKCTL,
> + PCI_EXP_LNKCTL_CCC, parent_old_ccc);
> }
>
> /* Convert L0s latency encoding to ns */
> --
> 2.30.2
>