diff mbox series

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

Message ID 20230517105235.29176-4-ilpo.jarvinen@linux.intel.com
State New
Headers
Series PCI: Improve PCIe Capability RMW concurrency control |

Commit Message

Ilpo Järvinen May 17, 2023, 10:52 a.m. UTC 
  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@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/pci/pcie/aspm.c | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

Comments

Rafael J. Wysocki May 17, 2023, 11:33 a.m. UTC | #1 
On Wed, May 17, 2023 at 12:53 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> 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@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Cc: stable@vger.kernel.org

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

> ---
>  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
>
Lukas Wunner June 16, 2023, 7:10 p.m. UTC | #2 
On Wed, May 17, 2023 at 01:52:29PM +0300, Ilpo Järvinen 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.
[...]
> @@ -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) {

This and several other RMW operations in drivers/pci/pcie/aspm.c
are touched by commit b1689799772a ("PCI/ASPM: Use distinct local
vars in pcie_retrain_link()") which got applied to pci/enumeration
this week:

https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=enumeration&id=b1689799772a6f4180f918b0ff66e264a3db9796

As a result the $SUBJECT_PATCH no longer applies cleanly and needs
to be respun.

Thanks,

Lukas
Ilpo Järvinen June 19, 2023, 2:45 p.m. UTC | #3 
On Fri, 16 Jun 2023, Lukas Wunner wrote:

> On Wed, May 17, 2023 at 01:52:29PM +0300, Ilpo Järvinen 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.
> [...]
> > @@ -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) {
> 
> This and several other RMW operations in drivers/pci/pcie/aspm.c
> are touched by commit b1689799772a ("PCI/ASPM: Use distinct local
> vars in pcie_retrain_link()") which got applied to pci/enumeration
> this week:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=enumeration&id=b1689799772a6f4180f918b0ff66e264a3db9796
> 
> As a result the $SUBJECT_PATCH no longer applies cleanly and needs
> to be respun.

Okay but I'm a bit lost which commit/head in pci repo I should now base 
this series because there's a conflict between pci/aspm and 
pci/enumeration which is not resolved in the repo because pci/enumeration 
hasn't advanced into pci/next yet. Any suggestion?
Bjorn Helgaas June 19, 2023, 3:09 p.m. UTC | #4 
On Mon, Jun 19, 2023 at 05:45:06PM +0300, Ilpo Järvinen wrote:
> On Fri, 16 Jun 2023, Lukas Wunner wrote:
> > On Wed, May 17, 2023 at 01:52:29PM +0300, Ilpo Järvinen 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.
> > [...]
> > > @@ -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) {
> > 
> > This and several other RMW operations in drivers/pci/pcie/aspm.c
> > are touched by commit b1689799772a ("PCI/ASPM: Use distinct local
> > vars in pcie_retrain_link()") which got applied to pci/enumeration
> > this week:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=enumeration&id=b1689799772a6f4180f918b0ff66e264a3db9796
> > 
> > As a result the $SUBJECT_PATCH no longer applies cleanly and needs
> > to be respun.
> 
> Okay but I'm a bit lost which commit/head in pci repo I should now base 
> this series because there's a conflict between pci/aspm and 
> pci/enumeration which is not resolved in the repo because pci/enumeration 
> hasn't advanced into pci/next yet. Any suggestion?

Generally speaking I prefer patches based on the PCI "main" branch
(usually -rc1) because I base topic branches on that.  If there are
conflicts with other pending material, it's great if you can mention
them, but I can resolve them when applying, so no need to repost just
for that.

Bjorn
Ilpo Järvinen June 19, 2023, 4:06 p.m. UTC | #5 
On Mon, 19 Jun 2023, Bjorn Helgaas wrote:

> On Mon, Jun 19, 2023 at 05:45:06PM +0300, Ilpo Järvinen wrote:
> > On Fri, 16 Jun 2023, Lukas Wunner wrote:
> > > On Wed, May 17, 2023 at 01:52:29PM +0300, Ilpo Järvinen 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.
> > > [...]
> > > > @@ -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) {
> > > 
> > > This and several other RMW operations in drivers/pci/pcie/aspm.c
> > > are touched by commit b1689799772a ("PCI/ASPM: Use distinct local
> > > vars in pcie_retrain_link()") which got applied to pci/enumeration
> > > this week:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=enumeration&id=b1689799772a6f4180f918b0ff66e264a3db9796
> > > 
> > > As a result the $SUBJECT_PATCH no longer applies cleanly and needs
> > > to be respun.
> > 
> > Okay but I'm a bit lost which commit/head in pci repo I should now base 
> > this series because there's a conflict between pci/aspm and 
> > pci/enumeration which is not resolved in the repo because pci/enumeration 
> > hasn't advanced into pci/next yet. Any suggestion?
> 
> Generally speaking I prefer patches based on the PCI "main" branch
> (usually -rc1) because I base topic branches on that.  If there are
> conflicts with other pending material, it's great if you can mention
> them, but I can resolve them when applying, so no need to repost just
> for that.

Just to confirm I'm understanding this correctly... Am I supposed to send
a patch whose changelog description and diff do not agree?? (The diff 
will still modify pcie/aspm.c if it's main-based and the description 
refers to something more generic as per Lukas' request since the code got 
moved into pci.c in the enumeration branch).
diff mbox series

Patch

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 */