[PATCH] PCI Express ASPM support should default to 'No'

[PATCH] PCI Express ASPM support should default to 'No'

[Jesper Juhl]

Running 'make oldconfig' I just noticed that PCIEASPM defaults to 
'y' in Kconfig even though the feature is both experimental and the 
help text recommends that if you are unsure you say 'n'.
It seems to me that this really should default to 'n', not 'y' at the 
moment.
The following patch makes that change. Please consider applying.


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index 25b04fb..5a0c6ad 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -33,7 +33,7 @@ source "drivers/pci/pcie/aer/Kconfig"
 config PCIEASPM
 	bool "PCI Express ASPM support(Experimental)"
 	depends on PCI && EXPERIMENTAL && PCIEPORTBUS
-	default y
+	default n
 	help
 	  This enables PCI Express ASPM (Active State Power Management) and
 	  Clock Power Management. ASPM supports state L0/L0s/L1.

Re: [PATCH] PCI Express ASPM support should default to 'No'

[Jesse Barnes]

On Tuesday, April 22, 2008 3:28 pm Jesper Juhl wrote:
> Running 'make oldconfig' I just noticed that PCIEASPM defaults to
> 'y' in Kconfig even though the feature is both experimental and the
> help text recommends that if you are unsure you say 'n'.
> It seems to me that this really should default to 'n', not 'y' at the
> moment.
> The following patch makes that change. Please consider applying.

Seem reasonable, Shaohua?  Please cc linux-pci on PCI patches though...

Thanks,
Jesse

Re: [PATCH] PCI Express ASPM support should default to 'No'

[Shaohua Li]

On Tue, 2008-04-22 at 17:12 -0700, Jesse Barnes wrote:
> On Tuesday, April 22, 2008 3:28 pm Jesper Juhl wrote:
> > Running 'make oldconfig' I just noticed that PCIEASPM defaults to
> > 'y' in Kconfig even though the feature is both experimental and the
> > help text recommends that if you are unsure you say 'n'.
> > It seems to me that this really should default to 'n', not 'y' at the
> > moment.
> > The following patch makes that change. Please consider applying.
> 
> Seem reasonable, Shaohua?  Please cc linux-pci on PCI patches
> though...
Ok, I'm fine with the patch. Though by default, the policy is to use
BIOS setting, that is if BIOS disables ASPM, we don't enable it too.

Thanks,
Shaohua

Re: [PATCH] PCI Express ASPM support should default to 'No'

[Jesse Barnes]

On Tuesday, April 22, 2008 6:05 pm Shaohua Li wrote:
> On Tue, 2008-04-22 at 17:12 -0700, Jesse Barnes wrote:
> > On Tuesday, April 22, 2008 3:28 pm Jesper Juhl wrote:
> > > Running 'make oldconfig' I just noticed that PCIEASPM defaults to
> > > 'y' in Kconfig even though the feature is both experimental and the
> > > help text recommends that if you are unsure you say 'n'.
> > > It seems to me that this really should default to 'n', not 'y' at the
> > > moment.
> > > The following patch makes that change. Please consider applying.
> >
> > Seem reasonable, Shaohua?  Please cc linux-pci on PCI patches
> > though...
>
> Ok, I'm fine with the patch. Though by default, the policy is to use
> BIOS setting, that is if BIOS disables ASPM, we don't enable it too.

Ok, pushed.  Thanks Jesper & Shaohua.

Jesse

Re: [PATCH] PCI Express ASPM support should default to 'No'

[Andi Kleen]

Shaohua Li <shaohua.li@intel.com> writes:

> On Tue, 2008-04-22 at 17:12 -0700, Jesse Barnes wrote:
>> On Tuesday, April 22, 2008 3:28 pm Jesper Juhl wrote:
>> > Running 'make oldconfig' I just noticed that PCIEASPM defaults to
>> > 'y' in Kconfig even though the feature is both experimental and the
>> > help text recommends that if you are unsure you say 'n'.
>> > It seems to me that this really should default to 'n', not 'y' at the
>> > moment.
>> > The following patch makes that change. Please consider applying.
>> 
>> Seem reasonable, Shaohua?  Please cc linux-pci on PCI patches
>> though...
> Ok, I'm fine with the patch. Though by default, the policy is to use
> BIOS setting, that is if BIOS disables ASPM, we don't enable it too.

Once the feature is considered stable it would be nice to make it default y
again. I think any power saving should be on by default (unless serious
issues are known), not off.

-Andi

Re: [PATCH] PCI Express ASPM support should default to 'No'

[Shaohua Li]

On Thu, 2008-04-24 at 09:49 +0200, Andi Kleen wrote:
> Shaohua Li <shaohua.li@intel.com> writes:
> 
> > On Tue, 2008-04-22 at 17:12 -0700, Jesse Barnes wrote:
> >> On Tuesday, April 22, 2008 3:28 pm Jesper Juhl wrote:
> >> > Running 'make oldconfig' I just noticed that PCIEASPM defaults to
> >> > 'y' in Kconfig even though the feature is both experimental and the
> >> > help text recommends that if you are unsure you say 'n'.
> >> > It seems to me that this really should default to 'n', not 'y' at the
> >> > moment.
> >> > The following patch makes that change. Please consider applying.
> >> 
> >> Seem reasonable, Shaohua?  Please cc linux-pci on PCI patches
> >> though...
> > Ok, I'm fine with the patch. Though by default, the policy is to use
> > BIOS setting, that is if BIOS disables ASPM, we don't enable it too.
> 
> Once the feature is considered stable it would be nice to make it
> default y
> again. I think any power saving should be on by default (unless
> serious
> issues are known), not off.
yes, we could do it in next release.

Thanks,
Shaohua

Re: [PATCH] PCI Express ASPM support should default to 'No'

[Jesse Barnes]

On Thursday, April 24, 2008 1:02 am Shaohua Li wrote:
> On Thu, 2008-04-24 at 09:49 +0200, Andi Kleen wrote:
> > Shaohua Li <shaohua.li@intel.com> writes:
> > > Ok, I'm fine with the patch. Though by default, the policy is to use
> > > BIOS setting, that is if BIOS disables ASPM, we don't enable it too.
> >
> > Once the feature is considered stable it would be nice to make it
> > default y
> > again. I think any power saving should be on by default (unless
> > serious
> > issues are known), not off.
>
> yes, we could do it in next release.

When we move to 'default y' we should also update the Kconfig text, removing 
the experimental tag and changing the advice.

And yeah, I'd like to save power by default too, but do you think this has 
seen enough test coverage to enable it by default?  Do we know of any 
platforms where ASPM causes problems because the BIOS enabled it and we tried 
to use it?


Thanks,
Jesse

Re: [PATCH] PCI Express ASPM support should default to 'No'

[Shaohua Li]

On Thu, 2008-04-24 at 08:33 -0700, Jesse Barnes wrote:
> On Thursday, April 24, 2008 1:02 am Shaohua Li wrote:
> > On Thu, 2008-04-24 at 09:49 +0200, Andi Kleen wrote:
> > > Shaohua Li <shaohua.li@intel.com> writes:
> > > > Ok, I'm fine with the patch. Though by default, the policy is to use
> > > > BIOS setting, that is if BIOS disables ASPM, we don't enable it too.
> > >
> > > Once the feature is considered stable it would be nice to make it
> > > default y
> > > again. I think any power saving should be on by default (unless
> > > serious
> > > issues are known), not off.
> >
> > yes, we could do it in next release.
> 
> When we move to 'default y' we should also update the Kconfig text, removing 
> the experimental tag and changing the advice.
> 
> And yeah, I'd like to save power by default too, but do you think this has 
> seen enough test coverage to enable it by default?  Do we know of any 
> platforms where ASPM causes problems because the BIOS enabled it and we tried 
> to use it?
Some chipset or device don't implement ASPM well. It's said some e1000
card has the bug and maybe other devices. ASPM patch provided API to let
driver disable ASPM too, hopefully driver can start using it. The
default policy is using BIOS setting, so only when BIOS uses it we will
try to use it. At this point, we should be safe unless BIOS is broken.

Thanks,
Shaohua

Re: [PATCH] PCI Express ASPM support should default to 'No'

[Jesper Juhl]

2008/4/24 Andi Kleen <andi@firstfloor.org>:
>
> Shaohua Li <shaohua.li@intel.com> writes:
>
>  > On Tue, 2008-04-22 at 17:12 -0700, Jesse Barnes wrote:
>  >> On Tuesday, April 22, 2008 3:28 pm Jesper Juhl wrote:
>  >> > Running 'make oldconfig' I just noticed that PCIEASPM defaults to
>  >> > 'y' in Kconfig even though the feature is both experimental and the
>  >> > help text recommends that if you are unsure you say 'n'.
>  >> > It seems to me that this really should default to 'n', not 'y' at the
>  >> > moment.
>  >> > The following patch makes that change. Please consider applying.
>  >>
>  >> Seem reasonable, Shaohua?  Please cc linux-pci on PCI patches
>  >> though...
>  > Ok, I'm fine with the patch. Though by default, the policy is to use
>  > BIOS setting, that is if BIOS disables ASPM, we don't enable it too.
>
>  Once the feature is considered stable it would be nice to make it default y
>  again. I think any power saving should be on by default (unless serious
>  issues are known), not off.
>
I agree, but as long as it's marked EXPERIMENTAL and there's even a
warning in the help text telling users to select 'n' if they are
unsure I think it makes sense to make 'n' the default.
Once it's been tested some more, is no longer experimental, then we
can change it back to 'y' easily.

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html