From: Luis R. Rodriguez on
On Tue, Jun 22, 2010 at 11:44:26AM -0700, Matthew Garrett wrote:
> On Tue, Jun 22, 2010 at 11:28:20AM -0700, Luis R. Rodriguez wrote:
> > On Tue, Jun 22, 2010 at 10:50 AM, Matthew Garrett <mjg59(a)srcf.ucam.org> wrote:
> > > People who use "force" deserve whatever they get,
> >
> > Heh, this whole patch and thread was started because Jussi tested
> > ath5k with pcie_aspm=force (on a pre PCIE 1.1 device (?)) . I have
> > been trying to explain all along why this is a terrible idea to the
> > point we should probably just remove that code from the kernel. Hence
> > my side rants and explanations to justify my reasonings.
>
> Well, there's two things here. If you use force then you might get
> inappropriate ASPM. But if your BIOS enables ASPM on an old device, then
> booting *without* CONFIG_PCIE_ASPM will leave it turned on, and booting
> *with* CONFIG_PCIE_ASPM will turn it off. The Kconfig description is
> confusing - reality is that CONFIG_PCIE_ASPM enables logic that allows
> the kernel to modify the BIOS default, and disabling it makes the
> assumption that your BIOS did something sensible.

Agreed, given that you also mentioned you would put it under embeeded
how about something like this:

diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index b8b494b..9c76b70 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -31,14 +31,29 @@ source "drivers/pci/pcie/aer/Kconfig"
# PCI Express ASPM
#
config PCIEASPM
- bool "PCI Express ASPM support(Experimental)"
- depends on PCI && EXPERIMENTAL && PCIEPORTBUS
- default n
+ bool "PCI Express ASPM sanity check support" if EMBEDDED
+ depends on PCI && PCIEPORTBUS
+ default y
help
- This enables PCI Express ASPM (Active State Power Management) and
- Clock Power Management. ASPM supports state L0/L0s/L1.
+ This enables some sanity checks for PCI Express ASPM.
+ ASPM supports the states L0/L0s/L1. The sanity checks are to
+ disable ASPM if:
+
+ a) the device is pre-1.1
+ b) the firmware has the FADT flag set to tell you not to
+ c) the firmware doesn't grant control via _OSC
+
+ Without this option your BIOS's defaults will be respected
+ and while although this should always be correct it typically
+ is not. If your ASPM settings are incorrect you may experience
+ odd hangs which are hard to debug. These sanity checks should
+ help avoid these odd hangs by only enabling ASPM if we are
+ sure we can enable it.
+
+ For more information you can refer to this documentation:
+
+ http://wireless.kernel.org/en/users/Documentation/ASPM

- When in doubt, say N.
config PCIEASPM_DEBUG
bool "Debug PCI Express ASPM"
depends on PCIEASPM

> > Where is "powersave"?
> >
> > I do see a "powersave" but its an ASPM policy string and it implies
> > you want to enable L1 and L0s when the device's LinkCap supports it,
> > see pcie_config_aspm_link() and its users. So in other words powersave
> > seems to imply you are using pcie_aspm=force, no?
>
> No. pcie_aspm=force will enable ASPM even if (a) the device is pre-1.1,
> (b) the firmware has the FADT flag set to tell you not to and (c) the
> firmware doesn't grant control via _OSC. The powersave policy will
> enable ASPM even if the BIOS didn't, but only if something else doesn't
> tell us not to.

So if any of the above (a) (b) or (c) are true powersave will keep
it disabled? Is pcie_aspm=forcepowersave this a new option with your
patches?

> > > Fedora's defaulted to that for a while now - we've hit
> > > issues with aacraid, but that's pretty much it in terms of cases where
> > > the heuristics don't work. Maxim's problems wouldn't be triggered
> > > because CONFIG_PCIE_ASPM disables it on pre-1.1 devices regardless of
> > > the BIOS setup.
> >
> > I don't expect all distributions to have CONFIG_PCIE_ASPM enabled, in
> > fact I was unaware of this sanity check being included as part of
> > CONFIG_PCIE_ASPM, I recommend we consider just enabling the sanity
> > check all the time. The fact that CONFIG_PCIE_ASPM is even an option
> > seems confusing to me given that apart from this sanity check the only
> > other thing that I see useful in it is the forcing of ASPM settings
> > and as I noted I think pcie_aspm=force is pretty dangerous.
>
> You're right, it shouldn't be an option. It's vital for making sure that
> ASPM is disabled when it should be. I'd be happy with pcie_aspm=force
> tainting the kernel.

:) !

> > > With the patch I've just sent, they should also all be used for Linux as
> > > well.
> >
> > Oh nice! It'll be part of 2.6.36?
>
> As long as Jesse picks it up.

Nice.

> > > If the same problems would appear under Windows then it's not a problem
> > > that I'm hugely concerned about as yet
> >
> > Yes, these issues would also be part of Windows. But should also note
> > this also means for those people working on their own BIOSes it means
> > you also have to take these things into more serious consideration.
>
> There's a standardised mechanism for BIOS authors to tell us not to
> touch their ASPM configuration, and people that ignore that really do
> deserve to have things break.

Oh, I was more concerned about open bios hackers. If a device requires
PCI host controller tweaks should *we* be doing those tweaks and sanity
checks oursevles upstream or should we rely on the open-bios guys to
do it accordingly in their code?

> > Me neither, ASPM should just work with default settings, which is why
> > I also do not like that the sanity check on the PCIE 1.1 spec is done
> > through CONFIG_PCIE_ASPM, it makes no sense given that ASPM *will*
> > work even if you do not have CONFIG_PCIE_ASPM but the device has
> > functional ASPM.
>
> I agree. I'll send a patch that moves it under CONFIG_EMBEDDED and
> defaults to on.

:D

> > I do think we should be depending on userspace to do development
> > testing and forcing ASPM on, because the only other alternative is
> > pcie_aspm=force and as noted this is just not a good idea unless you
> > *seriously* know what you are doing.
>
> If you set the powersave policy and ASPM doesn't get enabled, then
> that's because we've got a really strong belief that ASPM shouldn't be
> enabled. Is your concern just that pcie_aspm=force is too easy for users
> to get at?

Yes! I think people are starting to use it like to magically save
more power without taking into consideration the implications. This is
why I was suggesting maybe we nuke that option all together. Does it
*really* give us any benefit? The only benefit I see is if we *are*
100% sure our system should work with all our root complexes and
endpoints having ASPM enabled. That I do not see happening until
a few years from now.

Maybe we should have another type of module parameter type, a
I_know_what_Im_doing_module_parameter and taint whenever any of
those are on, not just pcie_aspm=force ? :)

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Luis R. Rodriguez on
On Tue, Jun 22, 2010 at 12:31 PM, Johannes Stezenbach <js(a)sig21.net> wrote:
> On Tue, Jun 22, 2010 at 07:44:26PM +0100, Matthew Garrett wrote:
>> On Tue, Jun 22, 2010 at 11:28:20AM -0700, Luis R. Rodriguez wrote:
>> >
>> > Heh, this whole patch and thread was started because Jussi tested
>> > ath5k with  pcie_aspm=force (on a pre PCIE 1.1 device (?)) . I have
>> > been trying to explain all along why this is a terrible idea to the
>> > point we should probably just remove that code from the kernel. Hence
>> > my side rants and explanations to justify my reasonings.
>>
>> Well, there's two things here. If you use force then you might get
>> inappropriate ASPM. But if your BIOS enables ASPM on an old device, then
>> booting *without* CONFIG_PCIE_ASPM will leave it turned on, and booting
>> *with* CONFIG_PCIE_ASPM will turn it off. The Kconfig description is
>> confusing - reality is that CONFIG_PCIE_ASPM enables logic that allows
>> the kernel to modify the BIOS default, and disabling it makes the
>> assumption that your BIOS did something sensible.
>
> Does CONFIG_PCIEASPM provide a way for the user to modifiy
> the settings at runtime?

You can tune ASPM settings at runtime, regardless of CONFIG_PCIEASPM. See:

http://kernel.org/pub/linux/kernel/people/mcgrof/aspm/enable-aspm
http://wireless.kernel.org/en/users/Documentation/ASPM

> I have a Samsung N130 netbook which has a BIOS setting
> called "CPU Power Saving Mode".  When enabled it activates
> ASPM L1 and L0s for the ethernet chip (Realtek RTL8102e, 100Mbit)
> and the PCIE bridge (with the BIOS setting off it's just L1).
> The result is that the ethernet througput is reduced to 25Mbit/s.
> (The BIOS setting does not activa L0s for the Atheros AR9285 WLAN.)
>
> 99,9% of the time I want to enjoy the power savings,
> but occationally I have to transfer some bulk data and would
> like to switch the setting for a few minutes.
>
> Or, well, ideally I'd like to have power savings _and_ performance
> at the same time without any manual intervention.  I'm not sure
> if this is a quirk of the N130 or if ASPM L0s always causes
> performance degradation?

L0s is not going to buy you much gains, getting at least L1 will
however. L0s is just a further enhancement. I recommend you test by
enabling L1 and L0s, check how longer your battery lasts and then test
again with just L1. Then test without both L1 and L0s.

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Johannes Stezenbach on
On Tue, Jun 22, 2010 at 07:44:26PM +0100, Matthew Garrett wrote:
> On Tue, Jun 22, 2010 at 11:28:20AM -0700, Luis R. Rodriguez wrote:
> >
> > Heh, this whole patch and thread was started because Jussi tested
> > ath5k with pcie_aspm=force (on a pre PCIE 1.1 device (?)) . I have
> > been trying to explain all along why this is a terrible idea to the
> > point we should probably just remove that code from the kernel. Hence
> > my side rants and explanations to justify my reasonings.
>
> Well, there's two things here. If you use force then you might get
> inappropriate ASPM. But if your BIOS enables ASPM on an old device, then
> booting *without* CONFIG_PCIE_ASPM will leave it turned on, and booting
> *with* CONFIG_PCIE_ASPM will turn it off. The Kconfig description is
> confusing - reality is that CONFIG_PCIE_ASPM enables logic that allows
> the kernel to modify the BIOS default, and disabling it makes the
> assumption that your BIOS did something sensible.

Does CONFIG_PCIEASPM provide a way for the user to modifiy
the settings at runtime?

I have a Samsung N130 netbook which has a BIOS setting
called "CPU Power Saving Mode". When enabled it activates
ASPM L1 and L0s for the ethernet chip (Realtek RTL8102e, 100Mbit)
and the PCIE bridge (with the BIOS setting off it's just L1).
The result is that the ethernet througput is reduced to 25Mbit/s.
(The BIOS setting does not activa L0s for the Atheros AR9285 WLAN.)

99,9% of the time I want to enjoy the power savings,
but occationally I have to transfer some bulk data and would
like to switch the setting for a few minutes.

Or, well, ideally I'd like to have power savings _and_ performance
at the same time without any manual intervention. I'm not sure
if this is a quirk of the N130 or if ASPM L0s always causes
performance degradation?


Thanks
Johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Johannes Stezenbach on
On Tue, Jun 22, 2010 at 12:38:03PM -0700, Luis R. Rodriguez wrote:
> On Tue, Jun 22, 2010 at 12:37 PM, Luis R. Rodriguez <mcgrof(a)gmail.com> wrote:
> > On Tue, Jun 22, 2010 at 12:31 PM, Johannes Stezenbach <js(a)sig21.net> wrote:
> >>
> >> Does CONFIG_PCIEASPM provide a way for the user to modifiy
> >> the settings at runtime?
> >
> > You can tune ASPM settings at runtime, regardless of CONFIG_PCIEASPM. See:
> >
> > http://kernel.org/pub/linux/kernel/people/mcgrof/aspm/enable-aspm
> > http://wireless.kernel.org/en/users/Documentation/ASPM
> >
> >> I have a Samsung N130 netbook which has a BIOS setting
> >> called "CPU Power Saving Mode". �When enabled it activates
> >> ASPM L1 and L0s for the ethernet chip (Realtek RTL8102e, 100Mbit)
> >> and the PCIE bridge (with the BIOS setting off it's just L1).
> >> The result is that the ethernet througput is reduced to 25Mbit/s.
> >
> > L0s is not going to buy you much gains, getting at least L1 will
> > however. L0s is just a further enhancement. I recommend you test by
> > enabling L1 and L0s, check how longer your battery lasts and then test
> > again with just L1. Then test without both L1 and L0s.
>
> So defaults should always be sane and you should not have to play with
> this stuff, unless you're a hacker, or are testing something for
> development purposes. Tweaking ASPM settings is not something a user
> should have to worry about. Period.

OK, let me put the question another way:

If enabling ASPM comes with a performance penalty (which is not unexpected,
there is usually a tradeoff between performance and power consumption),
do you think a boot time option (pcie_aspm=) or compile time option
(CONFIG_PCIEASPM) is the right user interface?


But meanwhile I found that CONFIG_PCIEASPM has a runtime
interface, /sys/module/pcie_aspm/parameters/policy.
http://lwn.net/Articles/266585/

I have not tested it on my N130 yet.


Johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Johannes Stezenbach on
On Tue, Jun 22, 2010 at 12:37:01PM -0700, Luis R. Rodriguez wrote:
> On Tue, Jun 22, 2010 at 12:31 PM, Johannes Stezenbach <js(a)sig21.net> wrote:
> > I have a Samsung N130 netbook which has a BIOS setting
> > called "CPU Power Saving Mode". �When enabled it activates
> > ASPM L1 and L0s for the ethernet chip (Realtek RTL8102e, 100Mbit)
> > and the PCIE bridge (with the BIOS setting off it's just L1).
> > The result is that the ethernet througput is reduced to 25Mbit/s.
> > (The BIOS setting does not activa L0s for the Atheros AR9285 WLAN.)
>
> L0s is not going to buy you much gains, getting at least L1 will
> however. L0s is just a further enhancement. I recommend you test by
> enabling L1 and L0s, check how longer your battery lasts and then test
> again with just L1. Then test without both L1 and L0s.

What I did was to dump lspci output to a file, for both settings
of the BIOS option, and then diff.

00:1c.2 PCI bridge: Intel Corporation 82801G (ICH7 Family) PCI Express Port 3 (rev 02) (prog-if 00 [Normal decode])
...
LnkCap: Port #3, Speed 2.5GT/s, Width x1, ASPM L0s L1, Latency L0 <256ns, L1 <4us
ClockPM- Surprise- LLActRep+ BwNot-
- LnkCtl: ASPM L1 Enabled; RCB 64 bytes Disabled- Retrain- CommClk+
+ LnkCtl: ASPM L0s L1 Enabled; RCB 64 bytes Disabled- Retrain- CommClk+
ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive+ BWMgmt- ABWMgmt-

03:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8101E/RTL8102E PCI Express Fast Ethernet controller (rev 02)
...
LnkCap: Port #0, Speed 2.5GT/s, Width x1, ASPM L0s L1, Latency L0 <512ns, L1 <64us
ClockPM+ Surprise- LLActRep- BwNot-
- LnkCtl: ASPM L1 Enabled; RCB 64 bytes Disabled- Retrain- CommClk+
+ LnkCtl: ASPM L0s L1 Enabled; RCB 64 bytes Disabled- Retrain- CommClk+
ExtSynch- ClockPM+ AutWidDis- BWInt- AutBWInt-
LnkSta: Speed 2.5GT/s, Width x1, TrErr- Train- SlotClk+ DLActive- BWMgmt- ABWMgmt-

I suspect the performance penalty is because L0s is entered very often
even during ethernet bulk transfer, whereas L1 is only entered
when the link is idle for longer time.

The difference in battery run time due to the BIOS option is
significant (IIRC, it's been a while back), but I don't know
what else the BIOS option changes.


Johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/