Re: [GIT PATCH] USB patches for 3.9-rc1

Re: [GIT PATCH] USB patches for 3.9-rc1

[Fabio Baltieri]

On Fri, Feb 22, 2013 at 03:59:54AM -0500, Dave Jones wrote:
> On Thu, Feb 21, 2013 at 10:40:10AM -0800, Greg KH wrote:
> 
>  > USB patches for 3.9-rc1
>  > 
>  > Here's the big USB merge for 3.9-rc1
>  > 
>  > Nothing major, lots of gadget fixes, and of course, xhci stuff.
> 
> I get no USB devices recognised when I insert them any more, which
> I think is pretty major.  I suspect it has something to do with this ?
> 
>  > Lan Tianyu (12):
>  >       usb: add runtime pm support for usb port device
>  >       usb: add usb port auto power off mechanism
> 
> It looks like every port on my laptop is powered down, as I can't
> even charge devices with it.

Hi Dave,

I have the same problem (and almost the same laptop, Thinkpad T430
here), all external USB ports without power - even the always-on one
:-).

The bug seems to be ACPI related, I bisected it down to this patch:

f95988d ACPI / scan: Treat power resources in a special way

In the dmesg I have some error like these:

ACPI: Power Resource [PUBS] (on)
...
pci 0000:00:14.0: System wakeup disabled by ACPI

for the USB controllers in the broken kernel, there are some in your
dmesg too.  I'll try to come up with a fix for current mainline, but all
the acpi stuff is quite obscure to me and the patch does not revert
cleanly, maybe Rafael (in CC) has some idea!

Fabio

-- 
Fabio Baltieri

Re: [GIT PATCH] USB patches for 3.9-rc1

[Dave Jones]

On Fri, Feb 22, 2013 at 10:51:58PM +0100, Fabio Baltieri wrote:
 > On Fri, Feb 22, 2013 at 03:59:54AM -0500, Dave Jones wrote:
 > > On Thu, Feb 21, 2013 at 10:40:10AM -0800, Greg KH wrote:
 > > 
 > > It looks like every port on my laptop is powered down, as I can't
 > > even charge devices with it.
 > 
 > I have the same problem (and almost the same laptop, Thinkpad T430
 > here), all external USB ports without power - even the always-on one
 > :-).
 > 
 > The bug seems to be ACPI related, I bisected it down to this patch:
 > 
 > f95988d ACPI / scan: Treat power resources in a special way
 > 
 > In the dmesg I have some error like these:
 > 
 > ACPI: Power Resource [PUBS] (on)
 > ...
 > pci 0000:00:14.0: System wakeup disabled by ACPI
 > 
 > for the USB controllers in the broken kernel, there are some in your
 > dmesg too.  I'll try to come up with a fix for current mainline, but all
 > the acpi stuff is quite obscure to me and the patch does not revert
 > cleanly, maybe Rafael (in CC) has some idea!

Good find. I see the same thing.

[    0.930283] ACPI _OSC control for PCIe not granted, disabling ASPM
[    0.933527] pci 0000:00:14.0: System wakeup disabled by ACPI
[    0.935982] pci 0000:00:19.0: System wakeup disabled by ACPI
[    0.937898] pci 0000:00:1a.0: System wakeup disabled by ACPI
[    0.939835] pci 0000:00:1b.0: System wakeup disabled by ACPI
[    0.940700] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.EXP1._PRT]
[    0.942195] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.EXP2._PRT]
[    0.943737] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.EXP3._PRT]
[    0.944564] pci 0000:00:1c.2: System wakeup disabled by ACPI
[    0.966491] pci 0000:00:1d.0: System wakeup disabled by ACPI

I must have pulled in the acpi bits the same time as the usb pull,
because I don't recall seeing this problem before last night.

	Dave

Re: [GIT PATCH] USB patches for 3.9-rc1

[Fabio Baltieri]

On Fri, Feb 22, 2013 at 05:23:04PM -0500, Dave Jones wrote:
> On Fri, Feb 22, 2013 at 10:51:58PM +0100, Fabio Baltieri wrote:
>  > On Fri, Feb 22, 2013 at 03:59:54AM -0500, Dave Jones wrote:
>  > > On Thu, Feb 21, 2013 at 10:40:10AM -0800, Greg KH wrote:
>  > > 
>  > > It looks like every port on my laptop is powered down, as I can't
>  > > even charge devices with it.
>  > 
>  > I have the same problem (and almost the same laptop, Thinkpad T430
>  > here), all external USB ports without power - even the always-on one
>  > :-).
>  > 
>  > The bug seems to be ACPI related, I bisected it down to this patch:
>  > 
>  > f95988d ACPI / scan: Treat power resources in a special way
>  > 
>  > In the dmesg I have some error like these:
>  > 
>  > ACPI: Power Resource [PUBS] (on)
>  > ...
>  > pci 0000:00:14.0: System wakeup disabled by ACPI
>  > 
>  > for the USB controllers in the broken kernel, there are some in your
>  > dmesg too.  I'll try to come up with a fix for current mainline, but all
>  > the acpi stuff is quite obscure to me and the patch does not revert
>  > cleanly, maybe Rafael (in CC) has some idea!
> 
> Good find. I see the same thing.
> 
> [    0.930283] ACPI _OSC control for PCIe not granted, disabling ASPM
> [    0.933527] pci 0000:00:14.0: System wakeup disabled by ACPI
> [    0.935982] pci 0000:00:19.0: System wakeup disabled by ACPI
> [    0.937898] pci 0000:00:1a.0: System wakeup disabled by ACPI
> [    0.939835] pci 0000:00:1b.0: System wakeup disabled by ACPI
> [    0.940700] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.EXP1._PRT]
> [    0.942195] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.EXP2._PRT]
> [    0.943737] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.EXP3._PRT]
> [    0.944564] pci 0000:00:1c.2: System wakeup disabled by ACPI
> [    0.966491] pci 0000:00:1d.0: System wakeup disabled by ACPI
> 
> I must have pulled in the acpi bits the same time as the usb pull,
> because I don't recall seeing this problem before last night.

Definitely.

Well, this did the trick in my case:

--- >8 ---
diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
index b820528..54175a0 100644
--- a/drivers/acpi/power.c
+++ b/drivers/acpi/power.c
@@ -795,7 +795,7 @@ int acpi_add_power_resource(acpi_handle handle)
        int state, result = -ENODEV;
 
        acpi_bus_get_device(handle, &device);
-       if (device)
+       if (!device)
                return 0;
 
        resource = kzalloc(sizeof(*resource), GFP_KERNEL);
--- >8 ---

But I guess it's working as a coincidence and something else is wrong -
I'll not even try to make a patch out of it and will leave the dirty
work to the ACPI guys instead.

Fabio

-- 
Fabio Baltieri

Re: [GIT PATCH] USB patches for 3.9-rc1

[Rafael J. Wysocki]

On Saturday, February 23, 2013 01:10:55 AM Fabio Baltieri wrote:
> On Fri, Feb 22, 2013 at 05:23:04PM -0500, Dave Jones wrote:
> > On Fri, Feb 22, 2013 at 10:51:58PM +0100, Fabio Baltieri wrote:
> >  > On Fri, Feb 22, 2013 at 03:59:54AM -0500, Dave Jones wrote:
> >  > > On Thu, Feb 21, 2013 at 10:40:10AM -0800, Greg KH wrote:
> >  > > 
> >  > > It looks like every port on my laptop is powered down, as I can't
> >  > > even charge devices with it.
> >  > 
> >  > I have the same problem (and almost the same laptop, Thinkpad T430
> >  > here), all external USB ports without power - even the always-on one
> >  > :-).
> >  > 
> >  > The bug seems to be ACPI related, I bisected it down to this patch:
> >  > 
> >  > f95988d ACPI / scan: Treat power resources in a special way
> >  > 
> >  > In the dmesg I have some error like these:
> >  > 
> >  > ACPI: Power Resource [PUBS] (on)
> >  > ...
> >  > pci 0000:00:14.0: System wakeup disabled by ACPI
> >  > 
> >  > for the USB controllers in the broken kernel, there are some in your
> >  > dmesg too.  I'll try to come up with a fix for current mainline, but all
> >  > the acpi stuff is quite obscure to me and the patch does not revert
> >  > cleanly, maybe Rafael (in CC) has some idea!
> > 
> > Good find. I see the same thing.
> > 
> > [    0.930283] ACPI _OSC control for PCIe not granted, disabling ASPM
> > [    0.933527] pci 0000:00:14.0: System wakeup disabled by ACPI
> > [    0.935982] pci 0000:00:19.0: System wakeup disabled by ACPI
> > [    0.937898] pci 0000:00:1a.0: System wakeup disabled by ACPI
> > [    0.939835] pci 0000:00:1b.0: System wakeup disabled by ACPI
> > [    0.940700] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.EXP1._PRT]
> > [    0.942195] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.EXP2._PRT]
> > [    0.943737] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.EXP3._PRT]
> > [    0.944564] pci 0000:00:1c.2: System wakeup disabled by ACPI
> > [    0.966491] pci 0000:00:1d.0: System wakeup disabled by ACPI
> > 
> > I must have pulled in the acpi bits the same time as the usb pull,
> > because I don't recall seeing this problem before last night.
> 
> Definitely.
> 
> Well, this did the trick in my case:
> 
> --- >8 ---
> diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
> index b820528..54175a0 100644
> --- a/drivers/acpi/power.c
> +++ b/drivers/acpi/power.c
> @@ -795,7 +795,7 @@ int acpi_add_power_resource(acpi_handle handle)
>         int state, result = -ENODEV;
>  
>         acpi_bus_get_device(handle, &device);
> -       if (device)
> +       if (!device)
>                 return 0;
>  
>         resource = kzalloc(sizeof(*resource), GFP_KERNEL);
> --- >8 ---
> 
> But I guess it's working as a coincidence and something else is wrong -
> I'll not even try to make a patch out of it and will leave the dirty
> work to the ACPI guys instead.

Well, this patch effectively disables the handling of power resources on your
machine entirely.  The effect of which is probably that the power resources
can't be turned off for the USB controllers, so they don't go into D3cold.

And the bisection found a commit that restores the handling of power resources
for you, which is not entirely surprising, but the root cause is somewhere else
most likely.

The new sysfs interface for power resources control may be helpful here.  You
need to use the Linus' current for it to work, though.

Can you please do

$ find /sys/devices/LNXSYSTM:00/ -name power_state -print -exec cat {} \;

$ find /sys/devices/LNXSYSTM:00/ -name real_power_state -print -exec cat {} \;

$ find /sys/devices/LNXSYSTM:00/ -name power_resources_D\* -print -exec ls {} \;

$ find /sys/devices/LNXSYSTM:00/ -name resource_in_use -print -exec cat {} \;

and send the output?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [GIT PATCH] USB patches for 3.9-rc1

[Fabio Baltieri]

On Sat, Feb 23, 2013 at 01:35:26AM +0100, Rafael J. Wysocki wrote:
> On Saturday, February 23, 2013 01:10:55 AM Fabio Baltieri wrote:
> > Well, this did the trick in my case:
> > 
> > --- >8 ---
> > diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
> > index b820528..54175a0 100644
> > --- a/drivers/acpi/power.c
> > +++ b/drivers/acpi/power.c
> > @@ -795,7 +795,7 @@ int acpi_add_power_resource(acpi_handle handle)
> >         int state, result = -ENODEV;
> >  
> >         acpi_bus_get_device(handle, &device);
> > -       if (device)
> > +       if (!device)
> >                 return 0;
> >  
> >         resource = kzalloc(sizeof(*resource), GFP_KERNEL);
> > --- >8 ---
> > 
> > But I guess it's working as a coincidence and something else is wrong -
> > I'll not even try to make a patch out of it and will leave the dirty
> > work to the ACPI guys instead.
> 
> Well, this patch effectively disables the handling of power resources on your
> machine entirely.  The effect of which is probably that the power resources
> can't be turned off for the USB controllers, so they don't go into D3cold.

Ok, as I wrote, I suspected this was just shutting off something and not
solving the real problem.

So, I'll try to recap all the threads here:

> And the bisection found a commit that restores the handling of power resources
> for you, which is not entirely surprising, but the root cause is somewhere else
> most likely.

Indeed.

> The new sysfs interface for power resources control may be helpful here.  You
> need to use the Linus' current for it to work, though.
> 
> Can you please do
> 
> $ find /sys/devices/LNXSYSTM:00/ -name power_state -print -exec cat {} \;
> $ find /sys/devices/LNXSYSTM:00/ -name real_power_state -print -exec cat {} \;
> $ find /sys/devices/LNXSYSTM:00/ -name power_resources_D\* -print -exec ls {} \;
> $ find /sys/devices/LNXSYSTM:00/ -name resource_in_use -print -exec cat {} \;
> and send the output?

With the acpi_add_power_resource disabled all power_state read "D0",
other attributes are not generated.

With a plain kernel that's the output:

$ find /sys/devices/LNXSYSTM:00/ -name power_state -print -exec cat {} \;
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:09/LNXVIDEO:01/power_state
D0
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/power_state
D0
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/power_state
D0
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/power_state
D0
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/power_state
D0

$ find /sys/devices/LNXSYSTM:00/ -name real_power_state -print -exec cat {} \;
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/real_power_state
D3cold
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/real_power_state
D3cold
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/real_power_state
D3cold

$ find /sys/devices/LNXSYSTM:00/ -name power_resources_D\* -print -exec ls {} \;
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/power_resources_D0
LNXPOWER:00
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/power_resources_D1
LNXPOWER:00
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/power_resources_D2
LNXPOWER:00
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/power_resources_D0
LNXPOWER:00
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/power_resources_D1
LNXPOWER:00
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/power_resources_D2
LNXPOWER:00
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/power_resources_D0
LNXPOWER:00
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/power_resources_D1
LNXPOWER:00
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/power_resources_D2
LNXPOWER:00

$ find /sys/devices/LNXSYSTM:00/ -name resource_in_use -print -exec cat {} \;
/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:08/PNP0C09:00/LNXPOWER:00/resource_in_use
0

> Can you please check if the problem is still there in the master
> branch of the linux-pm.git tree alone?

Not sure if this was for me or Dave, anyway linux-pm.git master
currently points as:

10baf04 Merge branch 'release' of git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux

and the problem is still there.

> May I see the bisection log, BTW?

Sure, here it is:

git bisect start
# bad: [8793422fd9ac5037f5047f80473007301df3689f] Merge tag 'pm+acpi-3.9-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
git bisect bad 8793422fd9ac5037f5047f80473007301df3689f
# good: [19f949f52599ba7c3f67a5897ac6be14bfcb1200] Linux 3.8
git bisect good 19f949f52599ba7c3f67a5897ac6be14bfcb1200
# good: [8909ff652ddfc83ecdf450f96629c25489d88f77] Merge tag 'regulator-3.9' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator
git bisect good 8909ff652ddfc83ecdf450f96629c25489d88f77
# good: [3aad3f03b2b6d2d977b985c49274cdb78a1593e5] Merge tag 'spi-for-linus' of git://git.secretlab.ca/git/linux
git bisect good 3aad3f03b2b6d2d977b985c49274cdb78a1593e5
# bad: [e8f71df723339b6d3861886f58c245812d1994f8] Merge branch 'acpi-cleanup'
git bisect bad e8f71df723339b6d3861886f58c245812d1994f8
# bad: [701190fd7419f6757c19cdc6473830c79debb3ae] clk: x86: add support for Lynxpoint LPSS clocks
git bisect bad 701190fd7419f6757c19cdc6473830c79debb3ae
# good: [3e5621a750e2cfb26748c34acbb67c691845494a] ACPICA: Update ACPICA initialization messages.
git bisect good 3e5621a750e2cfb26748c34acbb67c691845494a
# bad: [115c9ad854bd4c0f58ebcb967ec1b0a1c4e4b2d3] ACPI: remove unused acpi_op_bind and acpi_op_unbind
git bisect bad 115c9ad854bd4c0f58ebcb967ec1b0a1c4e4b2d3
# good: [e3863094c6f9b2f980d6e7a5cad6b4d03a4dd579] ACPI: Drop the second argument of acpi_bus_scan()
git bisect good e3863094c6f9b2f980d6e7a5cad6b4d03a4dd579
# good: [38a9a67a281eeebcd7cccf87f0e371f58ae625e3] ACPI / PCI: Move the _PRT setup and cleanup code to pci-acpi.c
git bisect good 38a9a67a281eeebcd7cccf87f0e371f58ae625e3
# good: [e0ebda2ee12c261fb2f2d7abf21489b93d9caa4e] ACPI: Remove unused struct acpi_pci_root.id member
git bisect good e0ebda2ee12c261fb2f2d7abf21489b93d9caa4e
# bad: [abe99210e0f624cea39f1dc375ba818b201c0d7f] ACPI / scan: Fix check of device_attach() return value.
git bisect bad abe99210e0f624cea39f1dc375ba818b201c0d7f
# bad: [f95988de06ea62ef5bd861f06e9ef56cea405ed1] ACPI / scan: Treat power resources in a special way
git bisect bad f95988de06ea62ef5bd861f06e9ef56cea405ed1

> Can you please send a dmesg boot log and the output of acpidump from the
> affected machine?

dmesg:
http://paste.ubuntu.com/5556864/

acpidump:
http://paste.ubuntu.com/5556867/

Thanks,
Fabio

-- 
Fabio Baltieri

Re: [GIT PATCH] USB patches for 3.9-rc1

[Rafael J. Wysocki]

On Saturday, February 23, 2013 02:44:27 AM Fabio Baltieri wrote:
> On Sat, Feb 23, 2013 at 01:35:26AM +0100, Rafael J. Wysocki wrote:
> > On Saturday, February 23, 2013 01:10:55 AM Fabio Baltieri wrote:
> > > Well, this did the trick in my case:
> > > 
> > > --- >8 ---
> > > diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
> > > index b820528..54175a0 100644
> > > --- a/drivers/acpi/power.c
> > > +++ b/drivers/acpi/power.c
> > > @@ -795,7 +795,7 @@ int acpi_add_power_resource(acpi_handle handle)
> > >         int state, result = -ENODEV;
> > >  
> > >         acpi_bus_get_device(handle, &device);
> > > -       if (device)
> > > +       if (!device)
> > >                 return 0;
> > >  
> > >         resource = kzalloc(sizeof(*resource), GFP_KERNEL);
> > > --- >8 ---
> > > 
> > > But I guess it's working as a coincidence and something else is wrong -
> > > I'll not even try to make a patch out of it and will leave the dirty
> > > work to the ACPI guys instead.
> > 
> > Well, this patch effectively disables the handling of power resources on your
> > machine entirely.  The effect of which is probably that the power resources
> > can't be turned off for the USB controllers, so they don't go into D3cold.
> 
> Ok, as I wrote, I suspected this was just shutting off something and not
> solving the real problem.
> 
> So, I'll try to recap all the threads here:
> 
> > And the bisection found a commit that restores the handling of power resources
> > for you, which is not entirely surprising, but the root cause is somewhere else
> > most likely.
> 
> Indeed.
> 
> > The new sysfs interface for power resources control may be helpful here.  You
> > need to use the Linus' current for it to work, though.
> > 
> > Can you please do
> > 
> > $ find /sys/devices/LNXSYSTM:00/ -name power_state -print -exec cat {} \;
> > $ find /sys/devices/LNXSYSTM:00/ -name real_power_state -print -exec cat {} \;
> > $ find /sys/devices/LNXSYSTM:00/ -name power_resources_D\* -print -exec ls {} \;
> > $ find /sys/devices/LNXSYSTM:00/ -name resource_in_use -print -exec cat {} \;
> > and send the output?
> 
> With the acpi_add_power_resource disabled all power_state read "D0",
> other attributes are not generated.

Yup.  That's how it should be.

> With a plain kernel that's the output:
> 
> $ find /sys/devices/LNXSYSTM:00/ -name power_state -print -exec cat {} \;
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:09/LNXVIDEO:01/power_state
> D0
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/power_state
> D0
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/power_state
> D0
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/power_state
> D0
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/power_state
> D0
> 
> $ find /sys/devices/LNXSYSTM:00/ -name real_power_state -print -exec cat {} \;
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/real_power_state
> D3cold
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/real_power_state
> D3cold
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/real_power_state
> D3cold

This is obviously wrong.  We expect the devices to be in D0, while they really
are in D3cold.  Let's look deeper.

> $ find /sys/devices/LNXSYSTM:00/ -name power_resources_D\* -print -exec ls {} \;
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/power_resources_D0
> LNXPOWER:00
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/power_resources_D1
> LNXPOWER:00
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/power_resources_D2
> LNXPOWER:00
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/power_resources_D0
> LNXPOWER:00
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/power_resources_D1
> LNXPOWER:00
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/power_resources_D2
> LNXPOWER:00
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/power_resources_D0
> LNXPOWER:00
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/power_resources_D1
> LNXPOWER:00
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/power_resources_D2
> LNXPOWER:00

PNP0A08 is the PCI root bridge and device:29, device:34, device:1f are the USB
controllers.  All of them have ACPI power states D0, D1, D2 and D3cold, where
D0-D2 depend on the same power resource, so in fact they all are the same state
(what sense does this make?).

I suspect that the power resource being shared (and it being shared in such a
broken way) has to do something with the breakage.

> $ find /sys/devices/LNXSYSTM:00/ -name resource_in_use -print -exec cat {} \;
> /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:08/PNP0C09:00/LNXPOWER:00/resource_in_use
> 0

There's one power resource in the system and it's usage count is 0, so it is
"off" (which is consistent with the real power states of the USB controllers).

Now, the boot log shows that the power resource was "on" when it was found,
so the initial states of the USB controllers should have been D0.

However, the DSDT source shows that the very same power resource that the D0-D2
power states of the devices depend on is listed for them as a wakeup power
resource by _PRW.  [Is that even valid?  It doesn't seem to make sense anyway.]
Thus when pci_acpi_setup() does acpi_pci_sleep_wake(pci_dev, false), which
calls acpi_pm_device_sleep_wake(&dev->dev, false), eventually
acpi_disable_wakeup_device_power() will be called for the device.  This leads
to calling acpi_power_off_list() for wakeup resources and that list includes
our single power resource, so its refcount will be dropped and it will be
turned off silently without updating the current power state of the device.

So first, the commit that broke things for you was probably
d2e5f0c (ACPI / PCI: Rework the setup and cleanup of device wakeup) which moved
the wakeup initialization, but didn't show up in the bisection, because the
power resources handling didn't work at that point.  And if I'm not mistaken,
it only broke things because we've never assumed that a wakeup power resource
may be the same as a power resource the device's power states depend on (which
we should).

Now, how to fix this is an interesting problem.

After some consideration I got the appended patch, but I'm really tired now
and couldn't really test it, so caveat emptor.  I'll look at it once again
tomorrow and see if it still makes sense to me then.

> > Can you please check if the problem is still there in the master
> > branch of the linux-pm.git tree alone?
> 
> Not sure if this was for me or Dave, anyway linux-pm.git master
> currently points as:
> 
> 10baf04 Merge branch 'release' of git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux
> 
> and the problem is still there.

OK, thanks for verifying this.

> > May I see the bisection log, BTW?
> 
> Sure, here it is:
> 
> git bisect start
> # bad: [8793422fd9ac5037f5047f80473007301df3689f] Merge tag 'pm+acpi-3.9-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
> git bisect bad 8793422fd9ac5037f5047f80473007301df3689f
> # good: [19f949f52599ba7c3f67a5897ac6be14bfcb1200] Linux 3.8
> git bisect good 19f949f52599ba7c3f67a5897ac6be14bfcb1200
> # good: [8909ff652ddfc83ecdf450f96629c25489d88f77] Merge tag 'regulator-3.9' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator
> git bisect good 8909ff652ddfc83ecdf450f96629c25489d88f77
> # good: [3aad3f03b2b6d2d977b985c49274cdb78a1593e5] Merge tag 'spi-for-linus' of git://git.secretlab.ca/git/linux
> git bisect good 3aad3f03b2b6d2d977b985c49274cdb78a1593e5
> # bad: [e8f71df723339b6d3861886f58c245812d1994f8] Merge branch 'acpi-cleanup'
> git bisect bad e8f71df723339b6d3861886f58c245812d1994f8
> # bad: [701190fd7419f6757c19cdc6473830c79debb3ae] clk: x86: add support for Lynxpoint LPSS clocks
> git bisect bad 701190fd7419f6757c19cdc6473830c79debb3ae
> # good: [3e5621a750e2cfb26748c34acbb67c691845494a] ACPICA: Update ACPICA initialization messages.
> git bisect good 3e5621a750e2cfb26748c34acbb67c691845494a
> # bad: [115c9ad854bd4c0f58ebcb967ec1b0a1c4e4b2d3] ACPI: remove unused acpi_op_bind and acpi_op_unbind
> git bisect bad 115c9ad854bd4c0f58ebcb967ec1b0a1c4e4b2d3
> # good: [e3863094c6f9b2f980d6e7a5cad6b4d03a4dd579] ACPI: Drop the second argument of acpi_bus_scan()
> git bisect good e3863094c6f9b2f980d6e7a5cad6b4d03a4dd579
> # good: [38a9a67a281eeebcd7cccf87f0e371f58ae625e3] ACPI / PCI: Move the _PRT setup and cleanup code to pci-acpi.c
> git bisect good 38a9a67a281eeebcd7cccf87f0e371f58ae625e3
> # good: [e0ebda2ee12c261fb2f2d7abf21489b93d9caa4e] ACPI: Remove unused struct acpi_pci_root.id member
> git bisect good e0ebda2ee12c261fb2f2d7abf21489b93d9caa4e
> # bad: [abe99210e0f624cea39f1dc375ba818b201c0d7f] ACPI / scan: Fix check of device_attach() return value.
> git bisect bad abe99210e0f624cea39f1dc375ba818b201c0d7f
> # bad: [f95988de06ea62ef5bd861f06e9ef56cea405ed1] ACPI / scan: Treat power resources in a special way
> git bisect bad f95988de06ea62ef5bd861f06e9ef56cea405ed1

Thanks.  I had hoped it would give me a hint, but it didn't after all.

> > Can you please send a dmesg boot log and the output of acpidump from the
> > affected machine?
> 
> dmesg:
> http://paste.ubuntu.com/5556864/
> 
> acpidump:
> http://paste.ubuntu.com/5556867/

Thanks a lot for the great info, it all helped.

Thanks,
Rafael


---
 drivers/acpi/internal.h |    2 
 drivers/acpi/power.c    |  106 ++++++++++++++++++++++++++++++++++++------------
 drivers/acpi/scan.c     |    2 
 3 files changed, 82 insertions(+), 28 deletions(-)

Index: test/drivers/acpi/internal.h
===================================================================
--- test.orig/drivers/acpi/internal.h
+++ test/drivers/acpi/internal.h
@@ -84,7 +84,7 @@ int acpi_extract_power_resources(union a
 				 struct list_head *list);
 int acpi_add_power_resource(acpi_handle handle);
 void acpi_power_add_remove_device(struct acpi_device *adev, bool add);
-int acpi_power_min_system_level(struct list_head *list);
+int acpi_power_wakeup_list_init(struct list_head *list);
 int acpi_device_sleep_wake(struct acpi_device *dev,
                            int enable, int sleep_state, int dev_state);
 int acpi_power_get_inferred_state(struct acpi_device *device, int *state);
Index: test/drivers/acpi/power.c
===================================================================
--- test.orig/drivers/acpi/power.c
+++ test/drivers/acpi/power.c
@@ -73,6 +73,7 @@ struct acpi_power_resource {
 	u32 system_level;
 	u32 order;
 	unsigned int ref_count;
+	bool wakeup_enabled;
 	struct mutex resource_lock;
 };
 
@@ -272,11 +273,9 @@ static int __acpi_power_on(struct acpi_p
 	return 0;
 }
 
-static int acpi_power_on(struct acpi_power_resource *resource)
+static int acpi_power_on_unlocked(struct acpi_power_resource *resource)
 {
-	int result = 0;;
-
-	mutex_lock(&resource->resource_lock);
+	int result = 0;
 
 	if (resource->ref_count++) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
@@ -293,9 +292,16 @@ static int acpi_power_on(struct acpi_pow
 				schedule_work(&dep->work);
 		}
 	}
+	return result;
+}
 
-	mutex_unlock(&resource->resource_lock);
+static int acpi_power_on(struct acpi_power_resource *resource)
+{
+	int result;
 
+	mutex_lock(&resource->resource_lock);
+	result = acpi_power_on_unlocked(resource);
+	mutex_unlock(&resource->resource_lock);
 	return result;
 }
 
@@ -313,17 +319,15 @@ static int __acpi_power_off(struct acpi_
 	return 0;
 }
 
-static int acpi_power_off(struct acpi_power_resource *resource)
+static int acpi_power_off_unlocked(struct acpi_power_resource *resource)
 {
 	int result = 0;
 
-	mutex_lock(&resource->resource_lock);
-
 	if (!resource->ref_count) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "Power resource [%s] already off",
 				  resource->name));
-		goto unlock;
+		return 0;
 	}
 
 	if (--resource->ref_count) {
@@ -335,10 +339,16 @@ static int acpi_power_off(struct acpi_po
 		if (result)
 			resource->ref_count++;
 	}
+	return result;
+}
 
- unlock:
-	mutex_unlock(&resource->resource_lock);
+static int acpi_power_off(struct acpi_power_resource *resource)
+{
+	int result;
 
+	mutex_lock(&resource->resource_lock);
+	result = acpi_power_off_unlocked(resource);
+	mutex_unlock(&resource->resource_lock);
 	return result;
 }
 
@@ -521,16 +531,29 @@ void acpi_power_add_remove_device(struct
 	}
 }
 
-int acpi_power_min_system_level(struct list_head *list)
+int acpi_power_wakeup_list_init(struct list_head *list)
 {
 	struct acpi_power_resource_entry *entry;
 	int system_level = 5;
 
 	list_for_each_entry(entry, list, node) {
 		struct acpi_power_resource *resource = entry->resource;
+		acpi_handle handle = resource->device.handle;
+		int result;
+		int state;
+
+		mutex_lock(&resource->resource_lock);
+
+		result = acpi_power_get_state(handle, &state);
+		if (!result && state == ACPI_POWER_RESOURCE_STATE_ON) {
+			resource->ref_count++;
+			resource->wakeup_enabled = true;
+		}
 
 		if (system_level > resource->system_level)
 			system_level = resource->system_level;
+
+		mutex_unlock(&resource->resource_lock);
 	}
 	return system_level;
 }
@@ -610,6 +633,7 @@ int acpi_device_sleep_wake(struct acpi_d
  */
 int acpi_enable_wakeup_device_power(struct acpi_device *dev, int sleep_state)
 {
+	struct acpi_power_resource_entry *entry;
 	int err = 0;
 
 	if (!dev || !dev->wakeup.flags.valid)
@@ -620,17 +644,31 @@ int acpi_enable_wakeup_device_power(stru
 	if (dev->wakeup.prepare_count++)
 		goto out;
 
-	err = acpi_power_on_list(&dev->wakeup.resources);
-	if (err) {
-		dev_err(&dev->dev, "Cannot turn wakeup power resources on\n");
-		dev->wakeup.flags.valid = 0;
-	} else {
-		/*
-		 * Passing 3 as the third argument below means the device may be
-		 * put into arbitrary power state afterward.
-		 */
-		err = acpi_device_sleep_wake(dev, 1, sleep_state, 3);
+	list_for_each_entry(entry, &dev->wakeup.resources, node) {
+		struct acpi_power_resource *resource = entry->resource;
+
+		mutex_lock(&resource->resource_lock);
+
+		if (!resource->wakeup_enabled) {
+			err = acpi_power_on_unlocked(resource);
+			if (!err)
+				resource->wakeup_enabled = true;
+		}
+
+		mutex_unlock(&resource->resource_lock);
+
+		if (err) {
+			dev_err(&dev->dev,
+				"Cannot turn wakeup power resources on\n");
+			dev->wakeup.flags.valid = 0;
+			goto out;
+		}
 	}
+	/*
+	 * Passing 3 as the third argument below means the device may be
+	 * put into arbitrary power state afterward.
+	 */
+	err = acpi_device_sleep_wake(dev, 1, sleep_state, 3);
 	if (err)
 		dev->wakeup.prepare_count = 0;
 
@@ -647,6 +685,7 @@ int acpi_enable_wakeup_device_power(stru
  */
 int acpi_disable_wakeup_device_power(struct acpi_device *dev)
 {
+	struct acpi_power_resource_entry *entry;
 	int err = 0;
 
 	if (!dev || !dev->wakeup.flags.valid)
@@ -668,10 +707,25 @@ int acpi_disable_wakeup_device_power(str
 	if (err)
 		goto out;
 
-	err = acpi_power_off_list(&dev->wakeup.resources);
-	if (err) {
-		dev_err(&dev->dev, "Cannot turn wakeup power resources off\n");
-		dev->wakeup.flags.valid = 0;
+	list_for_each_entry(entry, &dev->wakeup.resources, node) {
+		struct acpi_power_resource *resource = entry->resource;
+
+		mutex_lock(&resource->resource_lock);
+
+		if (resource->wakeup_enabled) {
+			err = acpi_power_off_unlocked(resource);
+			if (!err)
+				resource->wakeup_enabled = false;
+		}
+
+		mutex_unlock(&resource->resource_lock);
+
+		if (err) {
+			dev_err(&dev->dev,
+				"Cannot turn wakeup power resources off\n");
+			dev->wakeup.flags.valid = 0;
+			break;
+		}
 	}
 
  out:
Index: test/drivers/acpi/scan.c
===================================================================
--- test.orig/drivers/acpi/scan.c
+++ test/drivers/acpi/scan.c
@@ -1153,7 +1153,7 @@ static int acpi_bus_extract_wakeup_devic
 	if (!list_empty(&wakeup->resources)) {
 		int sleep_state;
 
-		sleep_state = acpi_power_min_system_level(&wakeup->resources);
+		sleep_state = acpi_power_wakeup_list_init(&wakeup->resources);
 		if (sleep_state < wakeup->sleep_state) {
 			acpi_handle_warn(handle, "Overriding _PRW sleep state "
 					 "(S%d) by S%d from power resources\n",


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [GIT PATCH] USB patches for 3.9-rc1

[Fabio Baltieri]

Hello Rafael,

On Sat, Feb 23, 2013 at 05:33:39AM +0100, Rafael J. Wysocki wrote:
> On Saturday, February 23, 2013 02:44:27 AM Fabio Baltieri wrote:
> > On Sat, Feb 23, 2013 at 01:35:26AM +0100, Rafael J. Wysocki wrote:
> > > The new sysfs interface for power resources control may be helpful here.  You
> > > need to use the Linus' current for it to work, though.
> > > 
> > > Can you please do
> > > 
> > > $ find /sys/devices/LNXSYSTM:00/ -name power_state -print -exec cat {} \;
> > > $ find /sys/devices/LNXSYSTM:00/ -name real_power_state -print -exec cat {} \;
> > > $ find /sys/devices/LNXSYSTM:00/ -name power_resources_D\* -print -exec ls {} \;
> > > $ find /sys/devices/LNXSYSTM:00/ -name resource_in_use -print -exec cat {} \;
> > > and send the output?
> > 
> > With the acpi_add_power_resource disabled all power_state read "D0",
> > other attributes are not generated.
> 
> Yup.  That's how it should be.
> 
> > With a plain kernel that's the output:
> > 
> > $ find /sys/devices/LNXSYSTM:00/ -name power_state -print -exec cat {} \;
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:09/LNXVIDEO:01/power_state
> > D0
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/power_state
> > D0
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/power_state
> > D0
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/power_state
> > D0
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/power_state
> > D0
> > 
> > $ find /sys/devices/LNXSYSTM:00/ -name real_power_state -print -exec cat {} \;
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/real_power_state
> > D3cold
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/real_power_state
> > D3cold
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/real_power_state
> > D3cold
> 
> This is obviously wrong.  We expect the devices to be in D0, while they really
> are in D3cold.  Let's look deeper.
> 
> > $ find /sys/devices/LNXSYSTM:00/ -name power_resources_D\* -print -exec ls {} \;
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/power_resources_D0
> > LNXPOWER:00
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/power_resources_D1
> > LNXPOWER:00
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/power_resources_D2
> > LNXPOWER:00
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/power_resources_D0
> > LNXPOWER:00
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/power_resources_D1
> > LNXPOWER:00
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/power_resources_D2
> > LNXPOWER:00
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/power_resources_D0
> > LNXPOWER:00
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/power_resources_D1
> > LNXPOWER:00
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/power_resources_D2
> > LNXPOWER:00
> 
> PNP0A08 is the PCI root bridge and device:29, device:34, device:1f are the USB
> controllers.  All of them have ACPI power states D0, D1, D2 and D3cold, where
> D0-D2 depend on the same power resource, so in fact they all are the same state
> (what sense does this make?).
> 
> I suspect that the power resource being shared (and it being shared in such a
> broken way) has to do something with the breakage.
> 
> > $ find /sys/devices/LNXSYSTM:00/ -name resource_in_use -print -exec cat {} \;
> > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:08/PNP0C09:00/LNXPOWER:00/resource_in_use
> > 0
> 
> There's one power resource in the system and it's usage count is 0, so it is
> "off" (which is consistent with the real power states of the USB controllers).
> 
> Now, the boot log shows that the power resource was "on" when it was found,
> so the initial states of the USB controllers should have been D0.

Sounds reasonable, I see that the ports are active until the kernel
starts.

> However, the DSDT source shows that the very same power resource that the D0-D2
> power states of the devices depend on is listed for them as a wakeup power
> resource by _PRW.  [Is that even valid?  It doesn't seem to make sense anyway.]
> Thus when pci_acpi_setup() does acpi_pci_sleep_wake(pci_dev, false), which
> calls acpi_pm_device_sleep_wake(&dev->dev, false), eventually
> acpi_disable_wakeup_device_power() will be called for the device.  This leads
> to calling acpi_power_off_list() for wakeup resources and that list includes
> our single power resource, so its refcount will be dropped and it will be
> turned off silently without updating the current power state of the device.
> 
> So first, the commit that broke things for you was probably
> d2e5f0c (ACPI / PCI: Rework the setup and cleanup of device wakeup) which moved
> the wakeup initialization, but didn't show up in the bisection, because the
> power resources handling didn't work at that point.  And if I'm not mistaken,
> it only broke things because we've never assumed that a wakeup power resource
> may be the same as a power resource the device's power states depend on (which
> we should).
> 
> Now, how to fix this is an interesting problem.
> 
> After some consideration I got the appended patch, but I'm really tired now
> and couldn't really test it, so caveat emptor.  I'll look at it once again
> tomorrow and see if it still makes sense to me then.
[snip]
> ---
>  drivers/acpi/internal.h |    2 
>  drivers/acpi/power.c    |  106 ++++++++++++++++++++++++++++++++++++------------
>  drivers/acpi/scan.c     |    2 
>  3 files changed, 82 insertions(+), 28 deletions(-)

Well, this works fine on my system.  The power is back and from sysfs I
got all three real_power_state to D0 and resource_in_use to 1.

Anything else I should check?

I'll re-test again when you submit the patch formally!

Thanks,
Fabio

-- 
Fabio Baltieri

[PATCH] ACPI / PM: Take unusual configurations of power resources into account (was: Re: [GIT PATCH] USB patches for 3.9-rc1)

[Rafael J. Wysocki]

On Saturday, February 23, 2013 12:49:14 PM Fabio Baltieri wrote:
> Hello Rafael,
> 
> On Sat, Feb 23, 2013 at 05:33:39AM +0100, Rafael J. Wysocki wrote:
> > On Saturday, February 23, 2013 02:44:27 AM Fabio Baltieri wrote:
> > > On Sat, Feb 23, 2013 at 01:35:26AM +0100, Rafael J. Wysocki wrote:
> > > > The new sysfs interface for power resources control may be helpful here.  You
> > > > need to use the Linus' current for it to work, though.
> > > > 
> > > > Can you please do
> > > > 
> > > > $ find /sys/devices/LNXSYSTM:00/ -name power_state -print -exec cat {} \;
> > > > $ find /sys/devices/LNXSYSTM:00/ -name real_power_state -print -exec cat {} \;
> > > > $ find /sys/devices/LNXSYSTM:00/ -name power_resources_D\* -print -exec ls {} \;
> > > > $ find /sys/devices/LNXSYSTM:00/ -name resource_in_use -print -exec cat {} \;
> > > > and send the output?
> > > 
> > > With the acpi_add_power_resource disabled all power_state read "D0",
> > > other attributes are not generated.
> > 
> > Yup.  That's how it should be.
> > 
> > > With a plain kernel that's the output:
> > > 
> > > $ find /sys/devices/LNXSYSTM:00/ -name power_state -print -exec cat {} \;
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:09/LNXVIDEO:01/power_state
> > > D0
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/power_state
> > > D0
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/power_state
> > > D0
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/power_state
> > > D0
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/power_state
> > > D0
> > > 
> > > $ find /sys/devices/LNXSYSTM:00/ -name real_power_state -print -exec cat {} \;
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/real_power_state
> > > D3cold
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/real_power_state
> > > D3cold
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/real_power_state
> > > D3cold
> > 
> > This is obviously wrong.  We expect the devices to be in D0, while they really
> > are in D3cold.  Let's look deeper.
> > 
> > > $ find /sys/devices/LNXSYSTM:00/ -name power_resources_D\* -print -exec ls {} \;
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/power_resources_D0
> > > LNXPOWER:00
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/power_resources_D1
> > > LNXPOWER:00
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:29/power_resources_D2
> > > LNXPOWER:00
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/power_resources_D0
> > > LNXPOWER:00
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/power_resources_D1
> > > LNXPOWER:00
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:34/power_resources_D2
> > > LNXPOWER:00
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/power_resources_D0
> > > LNXPOWER:00
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/power_resources_D1
> > > LNXPOWER:00
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:1f/power_resources_D2
> > > LNXPOWER:00
> > 
> > PNP0A08 is the PCI root bridge and device:29, device:34, device:1f are the USB
> > controllers.  All of them have ACPI power states D0, D1, D2 and D3cold, where
> > D0-D2 depend on the same power resource, so in fact they all are the same state
> > (what sense does this make?).
> > 
> > I suspect that the power resource being shared (and it being shared in such a
> > broken way) has to do something with the breakage.
> > 
> > > $ find /sys/devices/LNXSYSTM:00/ -name resource_in_use -print -exec cat {} \;
> > > /sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:08/PNP0C09:00/LNXPOWER:00/resource_in_use
> > > 0
> > 
> > There's one power resource in the system and it's usage count is 0, so it is
> > "off" (which is consistent with the real power states of the USB controllers).
> > 
> > Now, the boot log shows that the power resource was "on" when it was found,
> > so the initial states of the USB controllers should have been D0.
> 
> Sounds reasonable, I see that the ports are active until the kernel
> starts.
> 
> > However, the DSDT source shows that the very same power resource that the D0-D2
> > power states of the devices depend on is listed for them as a wakeup power
> > resource by _PRW.  [Is that even valid?  It doesn't seem to make sense anyway.]
> > Thus when pci_acpi_setup() does acpi_pci_sleep_wake(pci_dev, false), which
> > calls acpi_pm_device_sleep_wake(&dev->dev, false), eventually
> > acpi_disable_wakeup_device_power() will be called for the device.  This leads
> > to calling acpi_power_off_list() for wakeup resources and that list includes
> > our single power resource, so its refcount will be dropped and it will be
> > turned off silently without updating the current power state of the device.
> > 
> > So first, the commit that broke things for you was probably
> > d2e5f0c (ACPI / PCI: Rework the setup and cleanup of device wakeup) which moved
> > the wakeup initialization, but didn't show up in the bisection, because the
> > power resources handling didn't work at that point.  And if I'm not mistaken,
> > it only broke things because we've never assumed that a wakeup power resource
> > may be the same as a power resource the device's power states depend on (which
> > we should).
> > 
> > Now, how to fix this is an interesting problem.
> > 
> > After some consideration I got the appended patch, but I'm really tired now
> > and couldn't really test it, so caveat emptor.  I'll look at it once again
> > tomorrow and see if it still makes sense to me then.
> [snip]
> > ---
> >  drivers/acpi/internal.h |    2 
> >  drivers/acpi/power.c    |  106 ++++++++++++++++++++++++++++++++++++------------
> >  drivers/acpi/scan.c     |    2 
> >  3 files changed, 82 insertions(+), 28 deletions(-)
> 
> Well, this works fine on my system.  The power is back and from sysfs I
> got all three real_power_state to D0 and resource_in_use to 1.
> 
> Anything else I should check?

No need.

> I'll re-test again when you submit the patch formally!

Thanks, it is appended.

I've only changed acpi_power_wakeup_list_init() so that it returns an error if
it cannot get the current states of wakeup power resources, which is essential.
The rest of the patch is the same as before.

Besides, it looks like it will be useful to have wakeup power resources exposed
in sysfs too, so I think I'll cut a patch for that.

Thanks,
Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: ACPI / PM: Take unusual configurations of power resources into account

Commit d2e5f0c (ACPI / PCI: Rework the setup and cleanup of device
wakeup) moved the initial disabling of system wakeup for PCI devices
into a place where it can actually work and that exposed a hidden old
issue with crap^Wunusual system designs where the same power
resources are used for both wakeup power and device power control at
run time.

Namely, say there is one power resource such that the ACPI power
state D0 of a PCI device depends on that power resource (i.e. the
device is in D0 when that power resource is "on") and it is used
as a wakeup power resource for the same device.  Then, calling
acpi_pci_sleep_wake(pci_dev, false) for the device in question will
cause the reference counter of that power resource to drop to 0,
which in turn will cause it to be turned off.  As a result, the
device will go into D3cold at that point, although it should have
stayed in D0.

As it turns out, that happens to USB controllers on some laptops
and USB becomes unusable on those machines as a result, which is
a major regression from v3.8.

To fix this problem, (1) increment the reference counters of wakup
power resources during their initialization if they are "on"
initially, (2) prevent acpi_disable_wakeup_device_power() from
decrementing the reference counters of wakeup power resources that
were not enabled for wakeup power previously, and (3) prevent
acpi_enable_wakeup_device_power() from incrementing the reference
counters of wakeup power resources that already are enabled for
wakeup power.

In addition to that, if it is impossible to determine the initial
states of wakeup power resources, avoid enabling wakeup for devices
whose wakeup power depends on those power resources.

Reported-by: Dave Jones <davej@redhat.com>
Reported-by: Fabio Baltieri <fabio.baltieri@linaro.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/internal.h |    2 
 drivers/acpi/power.c    |  112 ++++++++++++++++++++++++++++++++++++------------
 drivers/acpi/scan.c     |    9 +++
 3 files changed, 94 insertions(+), 29 deletions(-)

Index: test/drivers/acpi/internal.h
===================================================================
--- test.orig/drivers/acpi/internal.h
+++ test/drivers/acpi/internal.h
@@ -84,7 +84,7 @@ int acpi_extract_power_resources(union a
 				 struct list_head *list);
 int acpi_add_power_resource(acpi_handle handle);
 void acpi_power_add_remove_device(struct acpi_device *adev, bool add);
-int acpi_power_min_system_level(struct list_head *list);
+int acpi_power_wakeup_list_init(struct list_head *list, int *system_level);
 int acpi_device_sleep_wake(struct acpi_device *dev,
                            int enable, int sleep_state, int dev_state);
 int acpi_power_get_inferred_state(struct acpi_device *device, int *state);
Index: test/drivers/acpi/power.c
===================================================================
--- test.orig/drivers/acpi/power.c
+++ test/drivers/acpi/power.c
@@ -73,6 +73,7 @@ struct acpi_power_resource {
 	u32 system_level;
 	u32 order;
 	unsigned int ref_count;
+	bool wakeup_enabled;
 	struct mutex resource_lock;
 };
 
@@ -272,11 +273,9 @@ static int __acpi_power_on(struct acpi_p
 	return 0;
 }
 
-static int acpi_power_on(struct acpi_power_resource *resource)
+static int acpi_power_on_unlocked(struct acpi_power_resource *resource)
 {
-	int result = 0;;
-
-	mutex_lock(&resource->resource_lock);
+	int result = 0;
 
 	if (resource->ref_count++) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
@@ -293,9 +292,16 @@ static int acpi_power_on(struct acpi_pow
 				schedule_work(&dep->work);
 		}
 	}
+	return result;
+}
 
-	mutex_unlock(&resource->resource_lock);
+static int acpi_power_on(struct acpi_power_resource *resource)
+{
+	int result;
 
+	mutex_lock(&resource->resource_lock);
+	result = acpi_power_on_unlocked(resource);
+	mutex_unlock(&resource->resource_lock);
 	return result;
 }
 
@@ -313,17 +319,15 @@ static int __acpi_power_off(struct acpi_
 	return 0;
 }
 
-static int acpi_power_off(struct acpi_power_resource *resource)
+static int acpi_power_off_unlocked(struct acpi_power_resource *resource)
 {
 	int result = 0;
 
-	mutex_lock(&resource->resource_lock);
-
 	if (!resource->ref_count) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "Power resource [%s] already off",
 				  resource->name));
-		goto unlock;
+		return 0;
 	}
 
 	if (--resource->ref_count) {
@@ -335,10 +339,16 @@ static int acpi_power_off(struct acpi_po
 		if (result)
 			resource->ref_count++;
 	}
+	return result;
+}
 
- unlock:
-	mutex_unlock(&resource->resource_lock);
+static int acpi_power_off(struct acpi_power_resource *resource)
+{
+	int result;
 
+	mutex_lock(&resource->resource_lock);
+	result = acpi_power_off_unlocked(resource);
+	mutex_unlock(&resource->resource_lock);
 	return result;
 }
 
@@ -521,18 +531,35 @@ void acpi_power_add_remove_device(struct
 	}
 }
 
-int acpi_power_min_system_level(struct list_head *list)
+int acpi_power_wakeup_list_init(struct list_head *list, int *system_level_p)
 {
 	struct acpi_power_resource_entry *entry;
 	int system_level = 5;
 
 	list_for_each_entry(entry, list, node) {
 		struct acpi_power_resource *resource = entry->resource;
+		acpi_handle handle = resource->device.handle;
+		int result;
+		int state;
 
+		mutex_lock(&resource->resource_lock);
+
+		result = acpi_power_get_state(handle, &state);
+		if (result) {
+			mutex_unlock(&resource->resource_lock);
+			return result;
+		}
+		if (state == ACPI_POWER_RESOURCE_STATE_ON) {
+			resource->ref_count++;
+			resource->wakeup_enabled = true;
+		}
 		if (system_level > resource->system_level)
 			system_level = resource->system_level;
+
+		mutex_unlock(&resource->resource_lock);
 	}
-	return system_level;
+	*system_level_p = system_level;
+	return 0;
 }
 
 /* --------------------------------------------------------------------------
@@ -610,6 +637,7 @@ int acpi_device_sleep_wake(struct acpi_d
  */
 int acpi_enable_wakeup_device_power(struct acpi_device *dev, int sleep_state)
 {
+	struct acpi_power_resource_entry *entry;
 	int err = 0;
 
 	if (!dev || !dev->wakeup.flags.valid)
@@ -620,17 +648,31 @@ int acpi_enable_wakeup_device_power(stru
 	if (dev->wakeup.prepare_count++)
 		goto out;
 
-	err = acpi_power_on_list(&dev->wakeup.resources);
-	if (err) {
-		dev_err(&dev->dev, "Cannot turn wakeup power resources on\n");
-		dev->wakeup.flags.valid = 0;
-	} else {
-		/*
-		 * Passing 3 as the third argument below means the device may be
-		 * put into arbitrary power state afterward.
-		 */
-		err = acpi_device_sleep_wake(dev, 1, sleep_state, 3);
+	list_for_each_entry(entry, &dev->wakeup.resources, node) {
+		struct acpi_power_resource *resource = entry->resource;
+
+		mutex_lock(&resource->resource_lock);
+
+		if (!resource->wakeup_enabled) {
+			err = acpi_power_on_unlocked(resource);
+			if (!err)
+				resource->wakeup_enabled = true;
+		}
+
+		mutex_unlock(&resource->resource_lock);
+
+		if (err) {
+			dev_err(&dev->dev,
+				"Cannot turn wakeup power resources on\n");
+			dev->wakeup.flags.valid = 0;
+			goto out;
+		}
 	}
+	/*
+	 * Passing 3 as the third argument below means the device may be
+	 * put into arbitrary power state afterward.
+	 */
+	err = acpi_device_sleep_wake(dev, 1, sleep_state, 3);
 	if (err)
 		dev->wakeup.prepare_count = 0;
 
@@ -647,6 +689,7 @@ int acpi_enable_wakeup_device_power(stru
  */
 int acpi_disable_wakeup_device_power(struct acpi_device *dev)
 {
+	struct acpi_power_resource_entry *entry;
 	int err = 0;
 
 	if (!dev || !dev->wakeup.flags.valid)
@@ -668,10 +711,25 @@ int acpi_disable_wakeup_device_power(str
 	if (err)
 		goto out;
 
-	err = acpi_power_off_list(&dev->wakeup.resources);
-	if (err) {
-		dev_err(&dev->dev, "Cannot turn wakeup power resources off\n");
-		dev->wakeup.flags.valid = 0;
+	list_for_each_entry(entry, &dev->wakeup.resources, node) {
+		struct acpi_power_resource *resource = entry->resource;
+
+		mutex_lock(&resource->resource_lock);
+
+		if (resource->wakeup_enabled) {
+			err = acpi_power_off_unlocked(resource);
+			if (!err)
+				resource->wakeup_enabled = false;
+		}
+
+		mutex_unlock(&resource->resource_lock);
+
+		if (err) {
+			dev_err(&dev->dev,
+				"Cannot turn wakeup power resources off\n");
+			dev->wakeup.flags.valid = 0;
+			break;
+		}
 	}
 
  out:
Index: test/drivers/acpi/scan.c
===================================================================
--- test.orig/drivers/acpi/scan.c
+++ test/drivers/acpi/scan.c
@@ -1153,7 +1153,14 @@ static int acpi_bus_extract_wakeup_devic
 	if (!list_empty(&wakeup->resources)) {
 		int sleep_state;
 
-		sleep_state = acpi_power_min_system_level(&wakeup->resources);
+		err = acpi_power_wakeup_list_init(&wakeup->resources,
+						  &sleep_state);
+		if (err) {
+			acpi_handle_warn(handle, "Retrieving current states "
+					 "of wakeup power resources failed\n");
+			acpi_power_resources_list_free(&wakeup->resources);
+			goto out;
+		}
 		if (sleep_state < wakeup->sleep_state) {
 			acpi_handle_warn(handle, "Overriding _PRW sleep state "
 					 "(S%d) by S%d from power resources\n",


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] ACPI / PM: Take unusual configurations of power resources into account (was: Re: [GIT PATCH] USB patches for 3.9-rc1)

[Fabio Baltieri]

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: ACPI / PM: Take unusual configurations of power resources into account
> 
> Commit d2e5f0c (ACPI / PCI: Rework the setup and cleanup of device
> wakeup) moved the initial disabling of system wakeup for PCI devices
> into a place where it can actually work and that exposed a hidden old
> issue with crap^Wunusual system designs where the same power
> resources are used for both wakeup power and device power control at
> run time.
> 
> Namely, say there is one power resource such that the ACPI power
> state D0 of a PCI device depends on that power resource (i.e. the
> device is in D0 when that power resource is "on") and it is used
> as a wakeup power resource for the same device.  Then, calling
> acpi_pci_sleep_wake(pci_dev, false) for the device in question will
> cause the reference counter of that power resource to drop to 0,
> which in turn will cause it to be turned off.  As a result, the
> device will go into D3cold at that point, although it should have
> stayed in D0.
> 
> As it turns out, that happens to USB controllers on some laptops
> and USB becomes unusable on those machines as a result, which is
> a major regression from v3.8.
> 
> To fix this problem, (1) increment the reference counters of wakup
> power resources during their initialization if they are "on"
> initially, (2) prevent acpi_disable_wakeup_device_power() from
> decrementing the reference counters of wakeup power resources that
> were not enabled for wakeup power previously, and (3) prevent
> acpi_enable_wakeup_device_power() from incrementing the reference
> counters of wakeup power resources that already are enabled for
> wakeup power.
> 
> In addition to that, if it is impossible to determine the initial
> states of wakeup power resources, avoid enabling wakeup for devices
> whose wakeup power depends on those power resources.
> 
> Reported-by: Dave Jones <davej@redhat.com>
> Reported-by: Fabio Baltieri <fabio.baltieri@linaro.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Works just fine!

Tested-by: Fabio Baltieri <fabio.baltieri@linaro.org>

Thanks,
Fabio

-- 
Fabio Baltieri

Re: [PATCH] ACPI / PM: Take unusual configurations of power resources into account (was: Re: [GIT PATCH] USB patches for 3.9-rc1)

[Rafael J. Wysocki]

On Saturday, February 23, 2013 03:48:59 PM Fabio Baltieri wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Subject: ACPI / PM: Take unusual configurations of power resources into account
> > 
> > Commit d2e5f0c (ACPI / PCI: Rework the setup and cleanup of device
> > wakeup) moved the initial disabling of system wakeup for PCI devices
> > into a place where it can actually work and that exposed a hidden old
> > issue with crap^Wunusual system designs where the same power
> > resources are used for both wakeup power and device power control at
> > run time.
> > 
> > Namely, say there is one power resource such that the ACPI power
> > state D0 of a PCI device depends on that power resource (i.e. the
> > device is in D0 when that power resource is "on") and it is used
> > as a wakeup power resource for the same device.  Then, calling
> > acpi_pci_sleep_wake(pci_dev, false) for the device in question will
> > cause the reference counter of that power resource to drop to 0,
> > which in turn will cause it to be turned off.  As a result, the
> > device will go into D3cold at that point, although it should have
> > stayed in D0.
> > 
> > As it turns out, that happens to USB controllers on some laptops
> > and USB becomes unusable on those machines as a result, which is
> > a major regression from v3.8.
> > 
> > To fix this problem, (1) increment the reference counters of wakup
> > power resources during their initialization if they are "on"
> > initially, (2) prevent acpi_disable_wakeup_device_power() from
> > decrementing the reference counters of wakeup power resources that
> > were not enabled for wakeup power previously, and (3) prevent
> > acpi_enable_wakeup_device_power() from incrementing the reference
> > counters of wakeup power resources that already are enabled for
> > wakeup power.
> > 
> > In addition to that, if it is impossible to determine the initial
> > states of wakeup power resources, avoid enabling wakeup for devices
> > whose wakeup power depends on those power resources.
> > 
> > Reported-by: Dave Jones <davej@redhat.com>
> > Reported-by: Fabio Baltieri <fabio.baltieri@linaro.org>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Works just fine!
> 
> Tested-by: Fabio Baltieri <fabio.baltieri@linaro.org>

Thanks for verifying!

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [GIT PATCH] USB patches for 3.9-rc1

[Rafael J. Wysocki]

On Friday, February 22, 2013 05:23:04 PM Dave Jones wrote:
> On Fri, Feb 22, 2013 at 10:51:58PM +0100, Fabio Baltieri wrote:
>  > On Fri, Feb 22, 2013 at 03:59:54AM -0500, Dave Jones wrote:
>  > > On Thu, Feb 21, 2013 at 10:40:10AM -0800, Greg KH wrote:
>  > > 
>  > > It looks like every port on my laptop is powered down, as I can't
>  > > even charge devices with it.
>  > 
>  > I have the same problem (and almost the same laptop, Thinkpad T430
>  > here), all external USB ports without power - even the always-on one
>  > :-).
>  > 
>  > The bug seems to be ACPI related, I bisected it down to this patch:
>  > 
>  > f95988d ACPI / scan: Treat power resources in a special way
>  > 
>  > In the dmesg I have some error like these:
>  > 
>  > ACPI: Power Resource [PUBS] (on)
>  > ...
>  > pci 0000:00:14.0: System wakeup disabled by ACPI
>  > 
>  > for the USB controllers in the broken kernel, there are some in your
>  > dmesg too.  I'll try to come up with a fix for current mainline, but all
>  > the acpi stuff is quite obscure to me and the patch does not revert
>  > cleanly, maybe Rafael (in CC) has some idea!
> 
> Good find. I see the same thing.
> 
> [    0.930283] ACPI _OSC control for PCIe not granted, disabling ASPM
> [    0.933527] pci 0000:00:14.0: System wakeup disabled by ACPI
> [    0.935982] pci 0000:00:19.0: System wakeup disabled by ACPI
> [    0.937898] pci 0000:00:1a.0: System wakeup disabled by ACPI
> [    0.939835] pci 0000:00:1b.0: System wakeup disabled by ACPI
> [    0.940700] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.EXP1._PRT]
> [    0.942195] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.EXP2._PRT]
> [    0.943737] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.EXP3._PRT]
> [    0.944564] pci 0000:00:1c.2: System wakeup disabled by ACPI
> [    0.966491] pci 0000:00:1d.0: System wakeup disabled by ACPI
> 
> I must have pulled in the acpi bits the same time as the usb pull,
> because I don't recall seeing this problem before last night.

Can you please check if the problem is still there in the master branch of
the linux-pm.git tree alone?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [GIT PATCH] USB patches for 3.9-rc1

[Rafael J. Wysocki]

On Friday, February 22, 2013 10:51:58 PM Fabio Baltieri wrote:
> On Fri, Feb 22, 2013 at 03:59:54AM -0500, Dave Jones wrote:
> > On Thu, Feb 21, 2013 at 10:40:10AM -0800, Greg KH wrote:
> > 
> >  > USB patches for 3.9-rc1
> >  > 
> >  > Here's the big USB merge for 3.9-rc1
> >  > 
> >  > Nothing major, lots of gadget fixes, and of course, xhci stuff.
> > 
> > I get no USB devices recognised when I insert them any more, which
> > I think is pretty major.  I suspect it has something to do with this ?
> > 
> >  > Lan Tianyu (12):
> >  >       usb: add runtime pm support for usb port device
> >  >       usb: add usb port auto power off mechanism
> > 
> > It looks like every port on my laptop is powered down, as I can't
> > even charge devices with it.
> 
> Hi Dave,
> 
> I have the same problem (and almost the same laptop, Thinkpad T430
> here), all external USB ports without power - even the always-on one
> :-).
> 
> The bug seems to be ACPI related, I bisected it down to this patch:
> 
> f95988d ACPI / scan: Treat power resources in a special way
> 
> In the dmesg I have some error like these:
> 
> ACPI: Power Resource [PUBS] (on)
> ...
> pci 0000:00:14.0: System wakeup disabled by ACPI

They aren't errors in fact, just info messages.

> for the USB controllers in the broken kernel, there are some in your
> dmesg too.  I'll try to come up with a fix for current mainline, but all
> the acpi stuff is quite obscure to me and the patch does not revert
> cleanly, maybe Rafael (in CC) has some idea!

It won't revert, there's more stuff on top of it.  And it is a fix, so
reverting it is not really a good idea anyway.

I suspect that the USB controllers are put into low-power states at one
point and they don't generate wakeup events.  I'm not sure, though, why this
is related to power resources.

Can you please send a dmesg boot log and the output of acpidump from the
affected machine?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [GIT PATCH] USB patches for 3.9-rc1

[Linus Torvalds]

On Fri, Feb 22, 2013 at 4:19 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>
> It won't revert, there's more stuff on top of it.  And it is a fix, so
> reverting it is not really a good idea anyway.

Rafael, please don't *ever* write that crap again.

We revert stuff whether it "fixed" something else or not. The rule is
"NO REGRESSIONS". It doesn't matter one whit if something "fixes"
something else or not - if it breaks an old case, it gets reverted.

Seriously. Why do I even have to mention this? Why do I have to
explain this to somebody pretty much *every* f*cking merge window?

This is not a new rule.

And btw, the *reason* for that rule becoming such a hard rule was
pretty much exactly suspend/resume and ACPI. Exactly because we used
to have those infinite "let's fix one thing and break another" dances.
So you should be well acquainted with the rule, and I'm surprised to
hear that kind of utter garbage from you in particular.

There is no excuse for regressions, and "it is a fix" is actually the
_least_ valid of all reasons.

A commit that causes a regression is - by definition - not a "fix". So
please don't *ever* say something that stupid again.

Things that used to work are simply a million times more important
than things that historically didn't work.

So this had better get fixed asap, and I need to feel like people are
working on it. Otherwise we start reverting.

And no amount "but it's a fix" matters one whit. In fact, it just
makes me feel like I need to start reverting early, because the
maintainer doesn't seem to understand how serious a regression is.

                  Linus

Re: [GIT PATCH] USB patches for 3.9-rc1

[Rafael J. Wysocki]

On Friday, February 22, 2013 04:30:25 PM Linus Torvalds wrote:
> On Fri, Feb 22, 2013 at 4:19 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >
> > It won't revert, there's more stuff on top of it.  And it is a fix, so
> > reverting it is not really a good idea anyway.
> 
> Rafael, please don't *ever* write that crap again.
> 
> We revert stuff whether it "fixed" something else or not. The rule is
> "NO REGRESSIONS". It doesn't matter one whit if something "fixes"
> something else or not - if it breaks an old case, it gets reverted.
> 
> Seriously. Why do I even have to mention this? Why do I have to
> explain this to somebody pretty much *every* f*cking merge window?

Well, sorry.  I shouldn't have said that.

The problem is, though, that even if bisection turns up something, it doesn't
automatically mean that this particular commit is the one that caused the
problem to happen in the first place.

And in this particular case bisection turns up a commit that enables something
that didn't work on that particular machine for some time.  It used to work,
then it stopped working and that commit made it work again.  And the fact that
it made it work again caused something different to trigger the result of which
is the observed breakage.

I'm obviously going to fix it, because it is a serious problem, but the commit
in question is not the root cause of it in my opinion (as I wrote to Fabio in
another message).

> This is not a new rule.
> 
> And btw, the *reason* for that rule becoming such a hard rule was
> pretty much exactly suspend/resume and ACPI. Exactly because we used
> to have those infinite "let's fix one thing and break another" dances.
> So you should be well acquainted with the rule, and I'm surprised to
> hear that kind of utter garbage from you in particular.
> 
> There is no excuse for regressions, and "it is a fix" is actually the
> _least_ valid of all reasons.
> 
> A commit that causes a regression is - by definition - not a "fix". So
> please don't *ever* say something that stupid again.
> 
> Things that used to work are simply a million times more important
> than things that historically didn't work.
> 
> So this had better get fixed asap, and I need to feel like people are
> working on it. Otherwise we start reverting.
> 
> And no amount "but it's a fix" matters one whit. In fact, it just
> makes me feel like I need to start reverting early, because the
> maintainer doesn't seem to understand how serious a regression is.

OK, OK.

Please let me understand what the problem is first.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [GIT PATCH] USB patches for 3.9-rc1

[Linus Torvalds]

On Fri, Feb 22, 2013 at 4:48 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>
> The problem is, though, that even if bisection turns up something, it doesn't
> automatically mean that this particular commit is the one that caused the
> problem to happen in the first place.

No, I agree. I just react *very* strongly when somebody starts arguing
against reverting.

The fact that the commit that was bisected fixes another bug in a
previous commit does in fact just imply that that *previous* commit
should perhaps be reverted. Of course, I see that that is the first in
the whole series, so I understand the pain, but even so..

And looking at that commit that makes power resources be treated
specially, that looks completely bogus. It's not what the old code did
either, and it seems to be a total hack for the breakage that just
happens to fix that one machine for purely random reasons, as far as I
can tell. The ACPI_BUS_TYPE_POWER case always had match=1 before, no?

The thing that looks to have been dropped somewhere is that

  .acpi_op_start = !!context,

which first became

  device->add_type = context ? ACPI_BUS_ADD_START : ACPI_BUS_ADD_MATCH;

and then somehow that wasn't needed any more, so it became just an unconditional

  device->add_type = ACPI_BUS_ADD_START;

for unexplained reasons (the commit message says that the argument
"context" wasn't used, and renames it not_used to reinforce the point,
but doesn't explain why it can remove the use that was there).

That unconditional "add_type = ACPI_BUS_ADD_START" then later becomes
"flags.match_driver = true", which is where the whole match_driver
thing comes in.

Anyway, I don't see how magically it became about ACPI_BUS_TYPE_POWER.
The code is not exactly obvious, though, so whatever. But that
"context is suddenly not used" commit (e3863094c6f9: "ACPI: Drop the
second argument of acpi_bus_scan()") looks odd.

Maybe "context" was effectively always non-NULL, but it *looks* like
that series of commits is intended to have no semantic changes, and
that's the one that looked very dubious to me.

                   Linus

Re: [GIT PATCH] USB patches for 3.9-rc1

[Rafael J. Wysocki]

On Friday, February 22, 2013 05:10:43 PM Linus Torvalds wrote:
> On Fri, Feb 22, 2013 at 4:48 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >
> > The problem is, though, that even if bisection turns up something, it doesn't
> > automatically mean that this particular commit is the one that caused the
> > problem to happen in the first place.
> 
> No, I agree. I just react *very* strongly when somebody starts arguing
> against reverting.
> 
> The fact that the commit that was bisected fixes another bug in a
> previous commit does in fact just imply that that *previous* commit
> should perhaps be reverted. Of course, I see that that is the first in
> the whole series, so I understand the pain, but even so..
> 
> And looking at that commit that makes power resources be treated
> specially, that looks completely bogus. It's not what the old code did
> either, and it seems to be a total hack for the breakage that just
> happens to fix that one machine for purely random reasons, as far as I
> can tell. The ACPI_BUS_TYPE_POWER case always had match=1 before, no?

It had, when it was called from acpi_bus_add_power_resource(), but that
was not the only way power resources could be added.  That function was only
called when we found a device using power resources and we needed to add them
before adding that device (if they were not present already).

The second way we could add power resources was during the normal namespace
walk and that case was broken.

> The thing that looks to have been dropped somewhere is that
> 
>   .acpi_op_start = !!context,
> 
> which first became
> 
>   device->add_type = context ? ACPI_BUS_ADD_START : ACPI_BUS_ADD_MATCH;
> 
> and then somehow that wasn't needed any more, so it became just an unconditional
> 
>   device->add_type = ACPI_BUS_ADD_START;
> 
> for unexplained reasons (the commit message says that the argument
> "context" wasn't used, and renames it not_used to reinforce the point,
> but doesn't explain why it can remove the use that was there).
> 
> That unconditional "add_type = ACPI_BUS_ADD_START" then later becomes
> "flags.match_driver = true", which is where the whole match_driver
> thing comes in.
> 
> Anyway, I don't see how magically it became about ACPI_BUS_TYPE_POWER.

If we added a power resource with acpi_add_single_object(..., false) from
acpi_bus_check_add(), later acpi_bus_add_power_resource() saw that it
was already present and didn't add it either.  The power resource was there at
that point, but it didn't have a driver (which only would be probed in the
second pass, after all ACPI devices had been registered).  The driver was
needed, though, for the initialization of the power management of devices
depending on that power resource.

The fact that it was missing caused power management of devices depending on
that power resource not to be initialized correctly and changing their power
states didn't really work going forward.

That's why the Fabio's bisection turned up that commit, BTW (the changing
of USB controllers' power states didn't really work before it on his machine).

I put ACPI_BUS_TYPE_POWER in there, because power resources always needed
'true' in the last arg of acpi_add_single_object() and they were the only type
needing it.

> The code is not exactly obvious, though, so whatever. But that
> "context is suddenly not used" commit (e3863094c6f9: "ACPI: Drop the
> second argument of acpi_bus_scan()") looks odd.
> 
> Maybe "context" was effectively always non-NULL, but it *looks* like
> that series of commits is intended to have no semantic changes, and
> that's the one that looked very dubious to me.

While I might make a mistake in it, I seriously doubt that it is the root
cause of the problem at hand.

It surely is related to PCI power management and to the fact that PCI devices
can go into D3cold at run time now.  This isn't an old change and there were
problems with it.  The known problems were fixed, but there might be more to
uncover and it looks like we've just run into a new one. :-(

So I really need to know what's going on before deciding how to fix that.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [GIT PATCH] USB patches for 3.9-rc1

[Rafael J. Wysocki]

On Friday, February 22, 2013 10:51:58 PM Fabio Baltieri wrote:
> On Fri, Feb 22, 2013 at 03:59:54AM -0500, Dave Jones wrote:
> > On Thu, Feb 21, 2013 at 10:40:10AM -0800, Greg KH wrote:
> > 
> >  > USB patches for 3.9-rc1
> >  > 
> >  > Here's the big USB merge for 3.9-rc1
> >  > 
> >  > Nothing major, lots of gadget fixes, and of course, xhci stuff.
> > 
> > I get no USB devices recognised when I insert them any more, which
> > I think is pretty major.  I suspect it has something to do with this ?
> > 
> >  > Lan Tianyu (12):
> >  >       usb: add runtime pm support for usb port device
> >  >       usb: add usb port auto power off mechanism
> > 
> > It looks like every port on my laptop is powered down, as I can't
> > even charge devices with it.
> 
> Hi Dave,
> 
> I have the same problem (and almost the same laptop, Thinkpad T430
> here), all external USB ports without power - even the always-on one
> :-).
> 
> The bug seems to be ACPI related, I bisected it down to this patch:
> 
> f95988d ACPI / scan: Treat power resources in a special way
> 
> In the dmesg I have some error like these:
> 
> ACPI: Power Resource [PUBS] (on)
> ...
> pci 0000:00:14.0: System wakeup disabled by ACPI
> 
> for the USB controllers in the broken kernel, there are some in your
> dmesg too.  I'll try to come up with a fix for current mainline, but all
> the acpi stuff is quite obscure to me and the patch does not revert
> cleanly, maybe Rafael (in CC) has some idea!

May I see the bisection log, BTW?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.