* [PATCH net-next 0/2] PCI: let pci_disable_link_state propagate errors
@ 2019-06-18 21:12 Heiner Kallweit
2019-06-18 21:13 ` [PATCH net-next 1/2] " Heiner Kallweit
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Heiner Kallweit @ 2019-06-18 21:12 UTC (permalink / raw)
To: Bjorn Helgaas, Realtek linux nic maintainers, David Miller
Cc: linux-pci, netdev
Drivers like r8169 rely on pci_disable_link_state() having disabled
certain ASPM link states. If OS can't control ASPM then
pci_disable_link_state() turns into a no-op w/o informing the caller.
The driver therefore may falsely assume the respective ASPM link
states are disabled. Let pci_disable_link_state() propagate errors
to the caller, enabling the caller to react accordingly.
I'd propose to let this series go through the netdev tree if the PCI
core extension is acked by the PCI people.
Heiner Kallweit (2):
PCI: let pci_disable_link_state propagate errors
r8169: don't activate ASPM in chip if OS can't control ASPM
drivers/net/ethernet/realtek/r8169_main.c | 8 ++++++--
drivers/pci/pcie/aspm.c | 20 +++++++++++---------
include/linux/pci-aspm.h | 7 ++++---
3 files changed, 21 insertions(+), 14 deletions(-)
--
2.22.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH net-next 1/2] PCI: let pci_disable_link_state propagate errors 2019-06-18 21:12 [PATCH net-next 0/2] PCI: let pci_disable_link_state propagate errors Heiner Kallweit @ 2019-06-18 21:13 ` Heiner Kallweit 2019-06-19 21:32 ` Bjorn Helgaas 2019-06-18 21:14 ` [PATCH net-next 2/2] r8169: don't activate ASPM in chip if OS can't control ASPM Heiner Kallweit ` (2 subsequent siblings) 3 siblings, 1 reply; 6+ messages in thread From: Heiner Kallweit @ 2019-06-18 21:13 UTC (permalink / raw) To: Bjorn Helgaas, Realtek linux nic maintainers, David Miller Cc: linux-pci, netdev Drivers may rely on pci_disable_link_state() having disabled certain ASPM link states. If OS can't control ASPM then pci_disable_link_state() turns into a no-op w/o informing the caller. The driver therefore may falsely assume the respective ASPM link states are disabled. Let pci_disable_link_state() propagate errors to the caller, enabling the caller to react accordingly. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/pci/pcie/aspm.c | 20 +++++++++++--------- include/linux/pci-aspm.h | 7 ++++--- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index fd4cb7508..e44af7f4d 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -1062,18 +1062,18 @@ void pcie_aspm_powersave_config_link(struct pci_dev *pdev) up_read(&pci_bus_sem); } -static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem) +static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem) { struct pci_dev *parent = pdev->bus->self; struct pcie_link_state *link; if (!pci_is_pcie(pdev)) - return; + return 0; if (pdev->has_secondary_link) parent = pdev; if (!parent || !parent->link_state) - return; + return -EINVAL; /* * A driver requested that ASPM be disabled on this device, but @@ -1085,7 +1085,7 @@ static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem) */ if (aspm_disabled) { pci_warn(pdev, "can't disable ASPM; OS doesn't have ASPM control\n"); - return; + return -EPERM; } if (sem) @@ -1105,11 +1105,13 @@ static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem) mutex_unlock(&aspm_lock); if (sem) up_read(&pci_bus_sem); + + return 0; } -void pci_disable_link_state_locked(struct pci_dev *pdev, int state) +int pci_disable_link_state_locked(struct pci_dev *pdev, int state) { - __pci_disable_link_state(pdev, state, false); + return __pci_disable_link_state(pdev, state, false); } EXPORT_SYMBOL(pci_disable_link_state_locked); @@ -1117,14 +1119,14 @@ EXPORT_SYMBOL(pci_disable_link_state_locked); * pci_disable_link_state - Disable device's link state, so the link will * never enter specific states. Note that if the BIOS didn't grant ASPM * control to the OS, this does nothing because we can't touch the LNKCTL - * register. + * register. Returns 0 or a negative errno. * * @pdev: PCI device * @state: ASPM link state to disable */ -void pci_disable_link_state(struct pci_dev *pdev, int state) +int pci_disable_link_state(struct pci_dev *pdev, int state) { - __pci_disable_link_state(pdev, state, true); + return __pci_disable_link_state(pdev, state, true); } EXPORT_SYMBOL(pci_disable_link_state); diff --git a/include/linux/pci-aspm.h b/include/linux/pci-aspm.h index df28af5ce..67064145d 100644 --- a/include/linux/pci-aspm.h +++ b/include/linux/pci-aspm.h @@ -24,11 +24,12 @@ #define PCIE_LINK_STATE_CLKPM 4 #ifdef CONFIG_PCIEASPM -void pci_disable_link_state(struct pci_dev *pdev, int state); -void pci_disable_link_state_locked(struct pci_dev *pdev, int state); +int pci_disable_link_state(struct pci_dev *pdev, int state); +int pci_disable_link_state_locked(struct pci_dev *pdev, int state); void pcie_no_aspm(void); #else -static inline void pci_disable_link_state(struct pci_dev *pdev, int state) { } +static inline int pci_disable_link_state(struct pci_dev *pdev, int state) +{ return 0; } static inline void pcie_no_aspm(void) { } #endif -- 2.22.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 1/2] PCI: let pci_disable_link_state propagate errors 2019-06-18 21:13 ` [PATCH net-next 1/2] " Heiner Kallweit @ 2019-06-19 21:32 ` Bjorn Helgaas 0 siblings, 0 replies; 6+ messages in thread From: Bjorn Helgaas @ 2019-06-19 21:32 UTC (permalink / raw) To: Heiner Kallweit Cc: Realtek linux nic maintainers, David Miller, linux-pci, netdev On Tue, Jun 18, 2019 at 11:13:48PM +0200, Heiner Kallweit wrote: > Drivers may rely on pci_disable_link_state() having disabled certain > ASPM link states. If OS can't control ASPM then pci_disable_link_state() > turns into a no-op w/o informing the caller. The driver therefore may > falsely assume the respective ASPM link states are disabled. > Let pci_disable_link_state() propagate errors to the caller, enabling > the caller to react accordingly. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Acked-by: Bjorn Helgaas <bhelgaas@google.com> Thanks, I think this makes good sense. > --- > drivers/pci/pcie/aspm.c | 20 +++++++++++--------- > include/linux/pci-aspm.h | 7 ++++--- > 2 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index fd4cb7508..e44af7f4d 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -1062,18 +1062,18 @@ void pcie_aspm_powersave_config_link(struct pci_dev *pdev) > up_read(&pci_bus_sem); > } > > -static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem) > +static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem) > { > struct pci_dev *parent = pdev->bus->self; > struct pcie_link_state *link; > > if (!pci_is_pcie(pdev)) > - return; > + return 0; > > if (pdev->has_secondary_link) > parent = pdev; > if (!parent || !parent->link_state) > - return; > + return -EINVAL; > > /* > * A driver requested that ASPM be disabled on this device, but > @@ -1085,7 +1085,7 @@ static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem) > */ > if (aspm_disabled) { > pci_warn(pdev, "can't disable ASPM; OS doesn't have ASPM control\n"); > - return; > + return -EPERM; > } > > if (sem) > @@ -1105,11 +1105,13 @@ static void __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem) > mutex_unlock(&aspm_lock); > if (sem) > up_read(&pci_bus_sem); > + > + return 0; > } > > -void pci_disable_link_state_locked(struct pci_dev *pdev, int state) > +int pci_disable_link_state_locked(struct pci_dev *pdev, int state) > { > - __pci_disable_link_state(pdev, state, false); > + return __pci_disable_link_state(pdev, state, false); > } > EXPORT_SYMBOL(pci_disable_link_state_locked); > > @@ -1117,14 +1119,14 @@ EXPORT_SYMBOL(pci_disable_link_state_locked); > * pci_disable_link_state - Disable device's link state, so the link will > * never enter specific states. Note that if the BIOS didn't grant ASPM > * control to the OS, this does nothing because we can't touch the LNKCTL > - * register. > + * register. Returns 0 or a negative errno. > * > * @pdev: PCI device > * @state: ASPM link state to disable > */ > -void pci_disable_link_state(struct pci_dev *pdev, int state) > +int pci_disable_link_state(struct pci_dev *pdev, int state) > { > - __pci_disable_link_state(pdev, state, true); > + return __pci_disable_link_state(pdev, state, true); > } > EXPORT_SYMBOL(pci_disable_link_state); > > diff --git a/include/linux/pci-aspm.h b/include/linux/pci-aspm.h > index df28af5ce..67064145d 100644 > --- a/include/linux/pci-aspm.h > +++ b/include/linux/pci-aspm.h > @@ -24,11 +24,12 @@ > #define PCIE_LINK_STATE_CLKPM 4 > > #ifdef CONFIG_PCIEASPM > -void pci_disable_link_state(struct pci_dev *pdev, int state); > -void pci_disable_link_state_locked(struct pci_dev *pdev, int state); > +int pci_disable_link_state(struct pci_dev *pdev, int state); > +int pci_disable_link_state_locked(struct pci_dev *pdev, int state); > void pcie_no_aspm(void); > #else > -static inline void pci_disable_link_state(struct pci_dev *pdev, int state) { } > +static inline int pci_disable_link_state(struct pci_dev *pdev, int state) > +{ return 0; } > static inline void pcie_no_aspm(void) { } > #endif > > -- > 2.22.0 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net-next 2/2] r8169: don't activate ASPM in chip if OS can't control ASPM 2019-06-18 21:12 [PATCH net-next 0/2] PCI: let pci_disable_link_state propagate errors Heiner Kallweit 2019-06-18 21:13 ` [PATCH net-next 1/2] " Heiner Kallweit @ 2019-06-18 21:14 ` Heiner Kallweit 2019-06-19 21:26 ` [PATCH net-next 0/2] PCI: let pci_disable_link_state propagate errors David Miller 2019-06-22 2:06 ` David Miller 3 siblings, 0 replies; 6+ messages in thread From: Heiner Kallweit @ 2019-06-18 21:14 UTC (permalink / raw) To: Bjorn Helgaas, Realtek linux nic maintainers, David Miller Cc: linux-pci, netdev Certain chip version / board combinations have massive problems if ASPM is active. If BIOS enables ASPM and doesn't let OS control it, then we may have a problem with the current code. Therefore check the return code of pci_disable_link_state() and don't enable ASPM in the network chip if OS can't control ASPM. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/ethernet/realtek/r8169_main.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 2e2a74aa0..48b8a90f7 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -652,6 +652,7 @@ struct rtl8169_private { unsigned irq_enabled:1; unsigned supports_gmii:1; + unsigned aspm_manageable:1; dma_addr_t counters_phys_addr; struct rtl8169_counters *counters; struct rtl8169_tc_offsets tc_offset; @@ -4286,7 +4287,8 @@ static void rtl_pcie_state_l2l3_disable(struct rtl8169_private *tp) static void rtl_hw_aspm_clkreq_enable(struct rtl8169_private *tp, bool enable) { - if (enable) { + /* Don't enable ASPM in the chip if OS can't control ASPM */ + if (enable && tp->aspm_manageable) { RTL_W8(tp, Config5, RTL_R8(tp, Config5) | ASPM_en); RTL_W8(tp, Config2, RTL_R8(tp, Config2) | ClkReqEn); } else { @@ -6678,7 +6680,9 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) /* Disable ASPM completely as that cause random device stop working * problems as well as full system hangs for some PCIe devices users. */ - pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | PCIE_LINK_STATE_L1); + rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L0S | + PCIE_LINK_STATE_L1); + tp->aspm_manageable = !rc; /* enable device (incl. PCI PM wakeup and hotplug setup) */ rc = pcim_enable_device(pdev); -- 2.22.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 0/2] PCI: let pci_disable_link_state propagate errors 2019-06-18 21:12 [PATCH net-next 0/2] PCI: let pci_disable_link_state propagate errors Heiner Kallweit 2019-06-18 21:13 ` [PATCH net-next 1/2] " Heiner Kallweit 2019-06-18 21:14 ` [PATCH net-next 2/2] r8169: don't activate ASPM in chip if OS can't control ASPM Heiner Kallweit @ 2019-06-19 21:26 ` David Miller 2019-06-22 2:06 ` David Miller 3 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2019-06-19 21:26 UTC (permalink / raw) To: hkallweit1; +Cc: bhelgaas, nic_swsd, linux-pci, netdev From: Heiner Kallweit <hkallweit1@gmail.com> Date: Tue, 18 Jun 2019 23:12:56 +0200 > Drivers like r8169 rely on pci_disable_link_state() having disabled > certain ASPM link states. If OS can't control ASPM then > pci_disable_link_state() turns into a no-op w/o informing the caller. > The driver therefore may falsely assume the respective ASPM link > states are disabled. Let pci_disable_link_state() propagate errors > to the caller, enabling the caller to react accordingly. > > I'd propose to let this series go through the netdev tree if the PCI > core extension is acked by the PCI people. Bjorn et al., please look at patch #1 and ACK/NACK Thank you. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next 0/2] PCI: let pci_disable_link_state propagate errors 2019-06-18 21:12 [PATCH net-next 0/2] PCI: let pci_disable_link_state propagate errors Heiner Kallweit ` (2 preceding siblings ...) 2019-06-19 21:26 ` [PATCH net-next 0/2] PCI: let pci_disable_link_state propagate errors David Miller @ 2019-06-22 2:06 ` David Miller 3 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2019-06-22 2:06 UTC (permalink / raw) To: hkallweit1; +Cc: bhelgaas, nic_swsd, linux-pci, netdev From: Heiner Kallweit <hkallweit1@gmail.com> Date: Tue, 18 Jun 2019 23:12:56 +0200 > Drivers like r8169 rely on pci_disable_link_state() having disabled > certain ASPM link states. If OS can't control ASPM then > pci_disable_link_state() turns into a no-op w/o informing the caller. > The driver therefore may falsely assume the respective ASPM link > states are disabled. Let pci_disable_link_state() propagate errors > to the caller, enabling the caller to react accordingly. > > I'd propose to let this series go through the netdev tree if the PCI > core extension is acked by the PCI people. Series applied, thanks Heiner. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-22 2:06 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-18 21:12 [PATCH net-next 0/2] PCI: let pci_disable_link_state propagate errors Heiner Kallweit 2019-06-18 21:13 ` [PATCH net-next 1/2] " Heiner Kallweit 2019-06-19 21:32 ` Bjorn Helgaas 2019-06-18 21:14 ` [PATCH net-next 2/2] r8169: don't activate ASPM in chip if OS can't control ASPM Heiner Kallweit 2019-06-19 21:26 ` [PATCH net-next 0/2] PCI: let pci_disable_link_state propagate errors David Miller 2019-06-22 2:06 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).