Re: /sys/module/pcie_aspm/parameters/policy not writable?

Re: /sys/module/pcie_aspm/parameters/policy not writable?

[Bjorn Helgaas]

[+cc linux-pci]

On Wed, Jul 10, 2013 at 10:21:32PM +0000, Wyborny, Carolyn wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:bhelgaas@google.com]
> [..]
> 
> > Pavel's ThinkPad X60 has two NICs: Intel 82573L and Intel PRO/Wireless
> > 3945ABG.  I'm pretty sure the problem he's reporting is with the 82573L.  Ping
> > times are bad (~100msec) when ASPM is enabled, as reported by lspci.
> > 
> > On Pavel's system, the FADT says we shouldn't enable OSPM control of ASPM
> > (ACPI_FADT_NO_ASPM is set), so we set "aspm_disabled = 1".  One effect is that
> > we don't blacklist the pre-1.1 82573L device, which I think results in it being left
> > with the BIOS configuration, which apparently has ASPM enabled.  (Pavel, could
> > you confirm the BIOS config, e.g., with "pci=earlydump"?)
> >
> > e1000e claims to disable ASPM, but because aspm_disabled is set, the driver's
> > call to pci_disable_link_state_locked() actually does nothing [1].
> 
> Yes, this is the problem we run into.  It would help if the call to pci_disable_link_state_locked() returned an error if ASPM is not disabled as requested so that drivers can then do the brute force disabling of it themselves.

I considered returning an error, but resisted because I think drivers
will just handle the error by doing the brute-force disable themselves,
and then we might as well drop the pci_disable_link_state() interface
completely.

I proposed a patch [3] a while ago that made pci_disable_link_state()
turn off ASPM unconditionally.  That would have the same effect as
returning failure and having drivers disable ASPM themselves.  But
Rafael and Matthew thought it was too risky [4] (and I think they're
probably right because it does not match the Windows behavior).

So by extension, I guess it would also be risky to return an error and
have the driver disable ASPM.

> > I experimented [2] with Windows and found that when a driver requests
> > PciASPMOptOut, Windows will not touch ASPM config if the _OSC method fails,
> > i.e., the BIOS declines to grant ASPM control to the OS.
> > However, I do not know if Windows similarly ignores PciASPMOptOut when the
> > FADT ACPI_FADT_NO_ASPM bit is set.
> > 
> > The PCI core has failed spectacularly at providing useful ASPM interfaces.  Do
> > you Intel folks have any suggestions about how to resolve this?  I assume that
> > the Windows driver for the 82573L must disable ASPM somehow, even though
> > ACPI_FADT_NO_ASPM is set.  Does it just use brute-force, as in the version of
> > __e1000e_disable_aspm() that's used when CONFIG_PCIEASPM is not set? 
> 
> My friends in our Windows development team told me that the driver doesn't try to disable  ASPM basically because we can't.  I'm not sure if the same issue presents in Windows the same way or not.

So the Windows driver *never* disables ASPM, not even with its own
register writes?  So on a machine like Pavel's, it would run with ASPM
enabled?  (I'm assuming his BIOS leaves ASPM enabled; hopefully Pavel
can confirm that.)

If the Windows driver works with ASPM enabled but the Linux driver on the
same hardware requires ASPM to be disabled, it sounds like the Linux
driver just needs to be fixed.

[3] https://lkml.kernel.org/r/20130510225257.GA10847@google.com
[4] https://lkml.kernel.org/r/20130516225535.GA27962@google.com

RE: /sys/module/pcie_aspm/parameters/policy not writable?

[Wyborny, Carolyn]

> -----Original Message-----
> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
> Sent: Wednesday, July 10, 2013 3:57 PM
> To: Wyborny, Carolyn
> Cc: Pavel Machek; Greg KH; kernel list; Joe Lawrence; Myron Stowe; Kirsher,
> Jeffrey T; Brandeburg, Jesse; Allan, Bruce W; Skidmore, Donald C; Rose, Gregory
> V; Waskiewicz Jr, Peter P; Duyck, Alexander H; Ronciak, John; Dave, Tushar N;
> e1000-devel@lists.sourceforge.net; linux-pci@vger.kernel.org
> Subject: Re: /sys/module/pcie_aspm/parameters/policy not writable?
> 
> [+cc linux-pci]
> 
> On Wed, Jul 10, 2013 at 10:21:32PM +0000, Wyborny, Carolyn wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas [mailto:bhelgaas@google.com]
> > [..]
> >
> > > Pavel's ThinkPad X60 has two NICs: Intel 82573L and Intel
> > > PRO/Wireless 3945ABG.  I'm pretty sure the problem he's reporting is
> > > with the 82573L.  Ping times are bad (~100msec) when ASPM is enabled, as
> reported by lspci.
> > >
> > > On Pavel's system, the FADT says we shouldn't enable OSPM control of
> > > ASPM (ACPI_FADT_NO_ASPM is set), so we set "aspm_disabled = 1".  One
> > > effect is that we don't blacklist the pre-1.1 82573L device, which I
> > > think results in it being left with the BIOS configuration, which
> > > apparently has ASPM enabled.  (Pavel, could you confirm the BIOS
> > > config, e.g., with "pci=earlydump"?)
> > >
> > > e1000e claims to disable ASPM, but because aspm_disabled is set, the
> > > driver's call to pci_disable_link_state_locked() actually does nothing [1].
> >
> > Yes, this is the problem we run into.  It would help if the call to
> pci_disable_link_state_locked() returned an error if ASPM is not disabled as
> requested so that drivers can then do the brute force disabling of it themselves.
> 
> I considered returning an error, but resisted because I think drivers will just
> handle the error by doing the brute-force disable themselves, and then we might
> as well drop the pci_disable_link_state() interface completely.
> 
> I proposed a patch [3] a while ago that made pci_disable_link_state() turn off
> ASPM unconditionally.  That would have the same effect as returning failure and
> having drivers disable ASPM themselves.  But Rafael and Matthew thought it was
> too risky [4] (and I think they're probably right because it does not match the
> Windows behavior).
> 
> So by extension, I guess it would also be risky to return an error and have the
> driver disable ASPM.
> 
> > > I experimented [2] with Windows and found that when a driver
> > > requests PciASPMOptOut, Windows will not touch ASPM config if the
> > > _OSC method fails, i.e., the BIOS declines to grant ASPM control to the OS.
> > > However, I do not know if Windows similarly ignores PciASPMOptOut
> > > when the FADT ACPI_FADT_NO_ASPM bit is set.
> > >
> > > The PCI core has failed spectacularly at providing useful ASPM
> > > interfaces.  Do you Intel folks have any suggestions about how to
> > > resolve this?  I assume that the Windows driver for the 82573L must
> > > disable ASPM somehow, even though ACPI_FADT_NO_ASPM is set.  Does it
> > > just use brute-force, as in the version of
> > > __e1000e_disable_aspm() that's used when CONFIG_PCIEASPM is not set?
> >
> > My friends in our Windows development team told me that the driver doesn't
> try to disable  ASPM basically because we can't.  I'm not sure if the same issue
> presents in Windows the same way or not.
> 
> So the Windows driver *never* disables ASPM, not even with its own register
> writes?  So on a machine like Pavel's, it would run with ASPM enabled?  (I'm
> assuming his BIOS leaves ASPM enabled; hopefully Pavel can confirm that.
> 
> If the Windows driver works with ASPM enabled but the Linux driver on the same
> hardware requires ASPM to be disabled, it sounds like the Linux driver just needs
> to be fixed.

So, to clarify, Windows *does* have a problem with these parts if ASPM cannot be disabled.  We tell users to disable ASPM  with these parts.  There are systems that have BIOS that do not truly disable ASPM even if the user tries, even with Windows and the symptoms are as bad as Linux, there's no big difference there.  The difference is that Windows doesn't interact with the BIOS very much and the Windows driver cannot access PCIconfig space as we can with Linux.  

I would argue for the error message so that drivers are not brute-forcing the change unless they have to.  Today, a solution would be for us to just skip the pcie_disable_link_state call altogether, since we can't guarantee it will work  and just brute force it no matter what and perhaps we should consider doing this.  But I think not having the error message would make it more likely to skip the call altogether.  Can you explain more why you think proving an error message or doing the force disabling in the function,if unable to complete the disabling, is risky?

Thanks,

Carolyn
  
> 
> [3] https://lkml.kernel.org/r/20130510225257.GA10847@google.com
> [4] https://lkml.kernel.org/r/20130516225535.GA27962@google.com

Re: /sys/module/pcie_aspm/parameters/policy not writable?

[Pavel Machek]

Hi!

> > > Pavel's ThinkPad X60 has two NICs: Intel 82573L and Intel PRO/Wireless
> > > 3945ABG.  I'm pretty sure the problem he's reporting is with the 82573L.  Ping
> > > times are bad (~100msec) when ASPM is enabled, as reported by lspci.
> > > 
> > > On Pavel's system, the FADT says we shouldn't enable OSPM control of ASPM
> > > (ACPI_FADT_NO_ASPM is set), so we set "aspm_disabled = 1".  One effect is that
> > > we don't blacklist the pre-1.1 82573L device, which I think results in it being left
> > > with the BIOS configuration, which apparently has ASPM enabled.  (Pavel, could
> > > you confirm the BIOS config, e.g., with "pci=earlydump"?)
> > >
> > > e1000e claims to disable ASPM, but because aspm_disabled is set, the driver's
> > > call to pci_disable_link_state_locked() actually does nothing [1].
> > 
> > Yes, this is the problem we run into.  It would help if the call to pci_disable_link_state_locked() returned an error if ASPM is not disabled as requested so that drivers can then do the brute force disabling of it themselves.
> 
> I considered returning an error, but resisted because I think drivers
> will just handle the error by doing the brute-force disable themselves,
> and then we might as well drop the pci_disable_link_state() interface
> completely.

What is that? Yes, if function does not perform what it was asked to
do, it should return an error. Anything else is antisocial.

If drivers will attempt to do something _more_ antisocial (like doing
PCI bit banging) we can surely catch it during review.

[Actually, the functions seem to have "force" parameter. Should we
just let e1000e/iwlwifi set the force? They _know_ hardware will not
work properly with the ASPM.] 

> I proposed a patch [3] a while ago that made pci_disable_link_state()
> turn off ASPM unconditionally.  That would have the same effect as
> returning failure and having drivers disable ASPM themselves.  But
> Rafael and Matthew thought it was too risky [4] (and I think they're
> probably right because it does not match the Windows behavior).

Well, I believe "patch might be risky" is trumped by "this ethernet
card is not usable". 

> So by extension, I guess it would also be risky to return an error and
> have the driver disable ASPM.

It does not seem like clean conclusion to me.

Doing force-disabling on selected hardware that needs it (x60+e1000e)
is certainly less risky than changing behaviour for all the 1000
notebooks out there.

So yes, you should be returning error.

At the very least, e1000e and iwlwifi could warn the user loudly and
maybe even disable the interface.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html