| Message ID | 20230511131441.45704-15-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: Improve LNKCTL & LNKCTL2 concurrency control | expand |
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)
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.
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
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/
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!
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 --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)
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(-)