PCI: Improve LNKCTL & LNKCTL2 concurrency control

[PATCH 14/17] r8169: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

Posted by Ilpo Järvinen 2 years, 9 months ago

Don't assume that only the driver would be accessing LNKCTL. ASPM
policy changes can trigger write to LNKCTL outside of driver's control.

Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
losing concurrent updates to the register value.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index a7e376e7e689..c0294a833681 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -2686,14 +2686,12 @@ static void __rtl_ephy_init(struct rtl8169_private *tp,
 
 static void rtl_disable_clock_request(struct rtl8169_private *tp)
 {
-	pcie_capability_clear_word(tp->pci_dev, PCI_EXP_LNKCTL,
-				   PCI_EXP_LNKCTL_CLKREQ_EN);
+	pcie_lnkctl_clear_and_set(tp->pci_dev, PCI_EXP_LNKCTL_CLKREQ_EN, 0);
 }
 
 static void rtl_enable_clock_request(struct rtl8169_private *tp)
 {
-	pcie_capability_set_word(tp->pci_dev, PCI_EXP_LNKCTL,
-				 PCI_EXP_LNKCTL_CLKREQ_EN);
+	pcie_lnkctl_clear_and_set(tp->pci_dev, 0, PCI_EXP_LNKCTL_CLKREQ_EN);
 }
 
 static void rtl_pcie_state_l2l3_disable(struct rtl8169_private *tp)
-- 
2.30.2

Re: [PATCH 14/17] r8169: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

Posted by Heiner Kallweit 2 years, 9 months ago

On 11.05.2023 15:14, Ilpo Järvinen wrote:
> Don't assume that only the driver would be accessing LNKCTL. ASPM
> policy changes can trigger write to LNKCTL outside of driver's control.
> 
> Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
> losing concurrent updates to the register value.
> 

Wouldn't it be more appropriate to add proper locking to the
underlying pcie_capability_clear_and_set_word()?


> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index a7e376e7e689..c0294a833681 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -2686,14 +2686,12 @@ static void __rtl_ephy_init(struct rtl8169_private *tp,
>  
>  static void rtl_disable_clock_request(struct rtl8169_private *tp)
>  {
> -	pcie_capability_clear_word(tp->pci_dev, PCI_EXP_LNKCTL,
> -				   PCI_EXP_LNKCTL_CLKREQ_EN);
> +	pcie_lnkctl_clear_and_set(tp->pci_dev, PCI_EXP_LNKCTL_CLKREQ_EN, 0);
>  }
>  
>  static void rtl_enable_clock_request(struct rtl8169_private *tp)
>  {
> -	pcie_capability_set_word(tp->pci_dev, PCI_EXP_LNKCTL,
> -				 PCI_EXP_LNKCTL_CLKREQ_EN);
> +	pcie_lnkctl_clear_and_set(tp->pci_dev, 0, PCI_EXP_LNKCTL_CLKREQ_EN);
>  }
>  
>  static void rtl_pcie_state_l2l3_disable(struct rtl8169_private *tp)

Re: [PATCH 14/17] r8169: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

Posted by Lukas Wunner 2 years, 9 months ago

On Thu, May 11, 2023 at 09:49:52PM +0200, Heiner Kallweit wrote:
> On 11.05.2023 15:14, Ilpo Järvinen wrote:
> > Don't assume that only the driver would be accessing LNKCTL. ASPM
> > policy changes can trigger write to LNKCTL outside of driver's control.
> > 
> > Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
> > losing concurrent updates to the register value.
> 
> Wouldn't it be more appropriate to add proper locking to the
> underlying pcie_capability_clear_and_set_word()?

PCI config space accessors such as this one are also used in hot paths
(e.g. interrupt handlers).  They should be kept lean (and lockless)
by default.  We only need locking for specific PCIe Extended Capabilities
which are concurrently accessed by PCI core code and drivers.

Thanks,

Lukas

Re: [PATCH 14/17] r8169: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

Posted by Heiner Kallweit 2 years, 9 months ago

On 11.05.2023 22:02, Lukas Wunner wrote:
> On Thu, May 11, 2023 at 09:49:52PM +0200, Heiner Kallweit wrote:
>> On 11.05.2023 15:14, Ilpo Järvinen wrote:
>>> Don't assume that only the driver would be accessing LNKCTL. ASPM
>>> policy changes can trigger write to LNKCTL outside of driver's control.
>>>
>>> Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
>>> losing concurrent updates to the register value.
>>
>> Wouldn't it be more appropriate to add proper locking to the
>> underlying pcie_capability_clear_and_set_word()?
> 
> PCI config space accessors such as this one are also used in hot paths
> (e.g. interrupt handlers).  They should be kept lean (and lockless)

I *think* in case the system uses threaded interrupts you may need locking
also in interrupt handlers.

> by default.  We only need locking for specific PCIe Extended Capabilities
> which are concurrently accessed by PCI core code and drivers.
> 
> Thanks,
> 
> Lukas

Re: [PATCH 14/17] r8169: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

Posted by Ilpo Järvinen 2 years, 9 months ago

On Thu, 11 May 2023, Heiner Kallweit wrote:

> On 11.05.2023 15:14, Ilpo Järvinen wrote:
> > Don't assume that only the driver would be accessing LNKCTL. ASPM
> > policy changes can trigger write to LNKCTL outside of driver's control.
> > 
> > Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
> > losing concurrent updates to the register value.
> > 
> 
> Wouldn't it be more appropriate to add proper locking to the
> underlying pcie_capability_clear_and_set_word()?

As per discussion for the other patch, that's where this series is now 
aiming to in v2.

-- 
 i.

Re: [PATCH 14/17] r8169: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

Posted by Lukas Wunner 2 years, 9 months ago

On Thu, May 11, 2023 at 11:00:02PM +0300, Ilpo Järvinen wrote:
> On Thu, 11 May 2023, Heiner Kallweit wrote:
> > On 11.05.2023 15:14, Ilpo Järvinen wrote:
> > > Don't assume that only the driver would be accessing LNKCTL. ASPM
> > > policy changes can trigger write to LNKCTL outside of driver's control.
> > > 
> > > Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
> > > losing concurrent updates to the register value.
> > > 
> > 
> > Wouldn't it be more appropriate to add proper locking to the
> > underlying pcie_capability_clear_and_set_word()?
> 
> As per discussion for the other patch, that's where this series is now 
> aiming to in v2.

That discussion wasn't cc'ed to Heiner.  For reference, this is the
message Ilpo is referring to:

https://lore.kernel.org/linux-pci/ZF1AjOKDVlbNFJPK@bhelgaas/

Re: [PATCH 14/17] r8169: Use pcie_lnkctl_clear_and_set() for changing LNKCTL

Posted by Heiner Kallweit 2 years, 9 months ago

On 11.05.2023 22:10, Lukas Wunner wrote:
> On Thu, May 11, 2023 at 11:00:02PM +0300, Ilpo Järvinen wrote:
>> On Thu, 11 May 2023, Heiner Kallweit wrote:
>>> On 11.05.2023 15:14, Ilpo Järvinen wrote:
>>>> Don't assume that only the driver would be accessing LNKCTL. ASPM
>>>> policy changes can trigger write to LNKCTL outside of driver's control.
>>>>
>>>> Use pcie_lnkctl_clear_and_set() which does proper locking to avoid
>>>> losing concurrent updates to the register value.
>>>>
>>>
>>> Wouldn't it be more appropriate to add proper locking to the
>>> underlying pcie_capability_clear_and_set_word()?
>>
>> As per discussion for the other patch, that's where this series is now 
>> aiming to in v2.
> 
> That discussion wasn't cc'ed to Heiner.  For reference, this is the
> message Ilpo is referring to:
> 
> https://lore.kernel.org/linux-pci/ZF1AjOKDVlbNFJPK@bhelgaas/

Thanks for the link!