diff mbox series

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

Message ID 20230511131441.45704-15-ilpo.jarvinen@linux.intel.com
State New
Headers
Series PCI: Improve LNKCTL & LNKCTL2 concurrency control |

Commit Message

Ilpo Järvinen May 11, 2023, 1:14 p.m. UTC 
  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(-)

Comments

Heiner Kallweit May 11, 2023, 7:49 p.m. UTC | #1 
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)
Ilpo Järvinen May 11, 2023, 8 p.m. UTC | #2 
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.
Lukas Wunner May 11, 2023, 8:02 p.m. UTC | #3 
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
Lukas Wunner May 11, 2023, 8:10 p.m. UTC | #4 
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/
Heiner Kallweit May 11, 2023, 8:11 p.m. UTC | #5 
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!
Heiner Kallweit May 11, 2023, 8:17 p.m. UTC | #6 
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
diff mbox series

Patch

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)