Message ID | 20180612095759.6828-1-kai.heng.feng@canonical.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
Series | [1/2] r8169: Don't disable ASPM in the driver | expand |
On 12.06.2018 11:57, Kai-Heng Feng wrote: > Enable or disable ASPM should be done in PCI core instead of in the > device driver. > > Commit ba04c7c93bbc ("r8169: disable ASPM") uses > pci_disable_link_state() to disable ASPM. This is incorrect, if the > device really needs to disable ASPM, we should use a quirk in PCI core > to prevent the PCI core from setting ASPM altogether. > I wouldn't call using pci_disable_link_state() in a driver incorrect (as it works), there is just a better way which is more in line with the PCI subsystem architecture. > Let's remove pci_disable_link_state() for now. Use PCI core quirks if > any regression happens. > The vendor driver disables ASPM unconditionally for chip version 25 (there it's METHOD_9), so I think ASPM support is broken in this chip version. I'll cook a PCI quirk. > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> Please note that netdev is closed currently. Once 4.18-RC1 is out it will be re-opened. Then please re-submit properly annotating PATCH with "net-next" (I've forgotten this often enough myself). > --- > v2: > - Remove module parameter. > - Remove pci_disable_link_state(). > > drivers/net/ethernet/realtek/r8169.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c > index 75dfac0248f4..9b55ce513a36 100644 > --- a/drivers/net/ethernet/realtek/r8169.c > +++ b/drivers/net/ethernet/realtek/r8169.c > @@ -25,7 +25,6 @@ > #include <linux/dma-mapping.h> > #include <linux/pm_runtime.h> > #include <linux/firmware.h> > -#include <linux/pci-aspm.h> > #include <linux/prefetch.h> > #include <linux/ipv6.h> > #include <net/ip6_checksum.h> > @@ -7647,10 +7646,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > mii->reg_num_mask = 0x1f; > mii->supports_gmii = cfg->has_gmii; > > - /* 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 | > - PCIE_LINK_STATE_CLKPM); > > /* enable device (incl. PCI PM wakeup and hotplug setup) */ > rc = pcim_enable_device(pdev); >
Hi Heiner, > On Jun 13, 2018, at 3:30 AM, Heiner Kallweit <hkallweit1@gmail.com> wrote: > > On 12.06.2018 11:57, Kai-Heng Feng wrote: >> Enable or disable ASPM should be done in PCI core instead of in the >> device driver. >> >> Commit ba04c7c93bbc ("r8169: disable ASPM") uses >> pci_disable_link_state() to disable ASPM. This is incorrect, if the >> device really needs to disable ASPM, we should use a quirk in PCI core >> to prevent the PCI core from setting ASPM altogether. > I wouldn't call using pci_disable_link_state() in a driver incorrect > (as it works), there is just a better way which is more in line with > the PCI subsystem architecture. Ok, I'll amend the commit log in next version. > >> Let's remove pci_disable_link_state() for now. Use PCI core quirks if >> any regression happens. > The vendor driver disables ASPM unconditionally for chip version 25 > (there it's METHOD_9), so I think ASPM support is broken in this chip > version. I'll cook a PCI quirk. I actually asked Ryankao about this. He said that variant is more then a decades old and he can't find why it doesn't support ASPM. Since METHOD_9 might be a platform issue instead, my intention was to enable ASPM for all variants. If users hit any issue, then we can introduce new PCI quirks. > >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > Please note that netdev is closed currently. Once 4.18-RC1 is out it > will be re-opened. Then please re-submit properly annotating PATCH > with "net-next" (I've forgotten this often enough myself). Will do for next version. Thanks! Kai-Heng > >> --- >> v2: >> - Remove module parameter. >> - Remove pci_disable_link_state(). >> >> drivers/net/ethernet/realtek/r8169.c | 5 ----- >> 1 file changed, 5 deletions(-) >> >> diff --git a/drivers/net/ethernet/realtek/r8169.c >> b/drivers/net/ethernet/realtek/r8169.c >> index 75dfac0248f4..9b55ce513a36 100644 >> --- a/drivers/net/ethernet/realtek/r8169.c >> +++ b/drivers/net/ethernet/realtek/r8169.c >> @@ -25,7 +25,6 @@ >> #include <linux/dma-mapping.h> >> #include <linux/pm_runtime.h> >> #include <linux/firmware.h> >> -#include <linux/pci-aspm.h> >> #include <linux/prefetch.h> >> #include <linux/ipv6.h> >> #include <net/ip6_checksum.h> >> @@ -7647,10 +7646,6 @@ static int rtl_init_one(struct pci_dev *pdev, >> const struct pci_device_id *ent) >> mii->reg_num_mask = 0x1f; >> mii->supports_gmii = cfg->has_gmii; >> >> - /* 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 | >> - PCIE_LINK_STATE_CLKPM); >> >> /* enable device (incl. PCI PM wakeup and hotplug setup) */ >> rc = pcim_enable_device(pdev);
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c index 75dfac0248f4..9b55ce513a36 100644 --- a/drivers/net/ethernet/realtek/r8169.c +++ b/drivers/net/ethernet/realtek/r8169.c @@ -25,7 +25,6 @@ #include <linux/dma-mapping.h> #include <linux/pm_runtime.h> #include <linux/firmware.h> -#include <linux/pci-aspm.h> #include <linux/prefetch.h> #include <linux/ipv6.h> #include <net/ip6_checksum.h> @@ -7647,10 +7646,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) mii->reg_num_mask = 0x1f; mii->supports_gmii = cfg->has_gmii; - /* 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 | - PCIE_LINK_STATE_CLKPM); /* enable device (incl. PCI PM wakeup and hotplug setup) */ rc = pcim_enable_device(pdev);
Enable or disable ASPM should be done in PCI core instead of in the device driver. Commit ba04c7c93bbc ("r8169: disable ASPM") uses pci_disable_link_state() to disable ASPM. This is incorrect, if the device really needs to disable ASPM, we should use a quirk in PCI core to prevent the PCI core from setting ASPM altogether. Let's remove pci_disable_link_state() for now. Use PCI core quirks if any regression happens. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- v2: - Remove module parameter. - Remove pci_disable_link_state(). drivers/net/ethernet/realtek/r8169.c | 5 ----- 1 file changed, 5 deletions(-)