Latest working kernel version: none Earliest failing kernel version: probably all versions Hardware Environment: ThinkPad T43 Software Environment: Linux 2.6.28 mainline Problem Description: Most ThinkPads have a Power Resource ("PUBS") which is used to control power to the (internal?) USB ports. This power resource is correctly detected by Linux and one can use sysfs to enable/disable it. So far, so good. ACPI 3.0b, section 3.4.2 specifies that when no devices are using a power resource, it should be turned OFF by the OS. So, if no USB HCI driver is loaded, the power resource should be turned off. This doesn't happen in Linux. The BIOS turns the power resource ON at boot, and it remains untouched until power down. The kernel doesn't attempt to track the usage of power resources, and just leaves them alone, instead of deactivating them when not in use.
Hi, Henrique Will you please attach the output of acpidump? Thanks.
It is a bit more basic. The resources are not being attached to all devices properly, and because of that, someone decided to break things further to avoid shutting down devices for no good reason... At first glance, it appears that devices already in D0 (because of the BIOS for example) will never get a chance to have their power resources attached and properly set up. We are not to trust anything. We should do the "attach power resources and bring them ON" and "detach power resources and, if not referenced anymore, turn them off" dance to ALL devices with an ACPI node, regardless of the state they were handled to us.
Created attachment 20326 [details] acpidump of ThinkPad T43 model 2687 (BIOS 1Y), v 1.29
When the underlying issue gets fixed, commit ff24ba74b6d3befbfbafa142582211b5a6095d45 has to be reverted, as it is bogus. The reason the USB ports on *any* ThinkPad would be disabled is that nothing is holding a reference to the "PUBS" power resource. Not EHCI, not UHCI, nothing... so obviously, it gets disabled the first time resource state was properly checked. When the whole problem is fixed, thinkpads will power down the USB ports if neither EHCI or UHCI are loaded through the PUBS power resource. And they will power it up when either driver gets loaded.
Well, I have a patchset here that seems to fix it. But as far as I can test, nothing turns the resources off later (might be USB braindamage, I don't know if other PCI device drivers do the proper thing and set devices to D3 when unloaded). Anyway, I am worried that we would get dangling pointers when, for example, modular ehci-hcd or uhci-hcd are removed on thinkpads (where they are linked to a power resource), since they do NOT power down the device on removal... so, the power resource still keeps the reference to the now rmmod'ed ehci-hcd device. Right now we don't trigger this because power resources for already-at-D0 devices were just plain not being registered in the first place.
Hi, Henrique Will you please attach the patch set for this issue? Thanks.
Sure about it? It will leak pointers like crazy until the rest of the code is fixed... What I did was just a revert for ff24ba74b6d3befbfbafa142582211b5a6095d45 and a simple patch to always call the power resource setup, even if the device was already at D0. The revert is trivial (but must be done by hand because the code changed a bit in that area). The other part of it is just conceptual. The complete fix would require: 1. A set of proper hooks for enable/disable device, that are **ALWAYS** called, regardless of initial power state, and regardless of whether the underlying code has been fixed to place devices back at D3 or not. Without these hooks, we either fail to attach resources, or (MUCH WORSE) leak resources and dangling pointers when devices are freed. 2. Using these hooks for power resource attach/detach. 3. Checking for any power resources that are enabled, but that have no users at ACPI start (as well as power resources that are disabled, but DO have users) at late suspend, early resume, and maybe other windows of opportunity.
> What I did was just a revert for ff24ba74b6d3befbfbafa142582211b5a6095d45 and > a > simple patch to always call the power resource setup, even if the device was > already at D0. The revert is trivial (but must be done by hand because the > code changed a bit in that area). The other part of it is just conceptual. Please attach it so that we can work on this issue together. > > The complete fix would require: > > 1. A set of proper hooks for enable/disable device, that are **ALWAYS** > called, Now there exists such a hook for PCI device(acpi_pci_set_power_state). When the PCI is switched to D0/D3, the corresponding power resource will be turned on/off if there exists the power resource. But there is no hook function for PNP device. If the power resource is also used by the PNP i8042 device, we can't turn on/off the power resource in course of suspend/resume. > regardless of initial power state, and regardless of whether the underlying > code has been fixed to place devices back at D3 or not. > > Without these hooks, we either fail to attach resources, or (MUCH WORSE) leak > resources and dangling pointers when devices are freed. > > 2. Using these hooks for power resource attach/detach. > > 3. Checking for any power resources that are enabled, but that have no users > at > ACPI start (as well as power resources that are disabled, but DO have users) > at > late suspend, early resume, and maybe other windows of opportunity. > For the ACPI device(eg: Fan), the power resource will be turned on/off according to the Fan state. And this is already realized in the current driver. In the course of boot phase the FAN device will be turned on/off according to the thermal zone temperature. In such case the power resource will be controlled correctly. For the PCI device: Only when the PCI state is changed between D0/D3, the hook function will be called and then the power resource can be controlled correctly. How about the following idea? a. the power resource will be put in off state in course of scanning ACPI device b. When the ACPI device is put into D0 state, the corresponding power resource will be turned on. Of course when it is put into the D3 state, the power resource will be turned off. c. For the PCI device: When the driver is loaded for the PCI device, the hook function is called to set the correct PCI power state.(For example: it can be called in the function of pci_enable_device. In fact it is already called in the current function of pci_enable_device). But the problem is for the PNP device.
Created attachment 20662 [details] Attach power resources for already-enabled PCI devices Careful. After this patch is applied, the kernel will have dangling pointers on module removal if the power resource is not properly unregistered when a module is unloaded.
Created attachment 20663 [details] Revert band-aid commit When power resources are properly attached, the PUBS device on thinkpads won't be disabled anymore on resume. Whether the power resource in question is going to be properly *DISABLED* at suspend when no wakeup devices are registered on the USB bus, is a complete different question...
There you have the partial fix patches. We still need a much proper connection between power management and ACPI power resources... enable power resources when devices are enabled, disable power resources when devices are disabled, disable power resources when all devices that need them are suspended (and THIS needs to be fully aware of wake devices), etc...
There is a patch in the ACPI ML, to fix PNPACPI device power: Message-ID: <49D101DA.50201@gmx.net> Date: Mon, 30 Mar 2009 19:31:06 +0200 From: Witold Szczeponik <Witold.Szczeponik@gmx.net> Subject: Enable PNPACPI _PSx Support, v3 Will attach, just for completeness.
Created attachment 20756 [details] Sets the power of PnP ACPI devices to D0 when they are activated and to D3 when they are disabled Patch from Witold Szczeponik <Witold.Szczeponik@gmx.net>
Created attachment 21206 [details] [Patch 1/2]: Put the power resource into OFF state before scanning ACPI device tree
Created attachment 21207 [details] [Patch 2/2]: Set the Pnp device into D0 state in case of adding "pnpacpi=off"
Hi, Henrique Will you please try the attached patch set and see whether the issue still exists? Patch 1: Put the power resource into OFF state before scanning ACPI device tree. Only when the ACPI device is switched to D0 state, it will be put into _ON state. For example: for PCI device. When enabling the PCI device, the hook function of acpi_pci_set_power_state will be called and the power resource can be enabled. Patch 2: Set the PNP device into D0 state in case of adding "pnpacpi=off" thanks.
We shouldn't go turning ACPI power states to OFF if the BIOS handled the device already turned ON (with the power source enabled as well). Turning resources off when we are not sure they're unused is bound to cause problems, it will disturb devices like boot-media on USB, etc. It is a _VERY_ Bad Idea. Instead, we should go over the entire list, attach all power resources according to the *current* state of the device. Lots of devices will _already_ be at D0 state, and we must attach the power resources to them before we start turning them off. We must not turn a power resource off until we can be sure that there are NO devices using it. So, we'd need something like my patch (but covering pnp as well), to attach the resources to devices already in D0. Then, we must find opportune times to do a scan and turn off any unattached resources. These are: when we disable any device, during suspend and resume, and after we are sure we attached resources to every device already enabled (even to those without drivers, as modules might be loaded later). The above is not very difficult, but it is just half the job. What is troublesome is the fact that we will never disable *all* devices we enable (because there are buggy PCI crap that we cannot resurrect back to D0 when we place it in D3, etc). And the current power resource code takes pointers (in the linked lists) back to the devices. We must change this, because the modules must still be allowed to unload, and in that case, the power resource must not still hold a reference to an area of memory that is now invalid. We will have to change the power resources to use reference counting or something else. Drivers that do not disable devices on unload will cause the power resource to never turn off, but that's alright - the device is still on, anyway.
Thanks for the response. Maybe what you said sounds reasonable. But how can we check whether the ACPI power resource is used? By traversing the power resource list? In fact before we don't set the device state(D0/D3), we can't build the relationship between device and power resource. In such case we can't know which device will refer the power resource. According to the spec the power resource can be shared by multiple devices. When the device is in D0 state, the corresponding power resource *should" be in ON state. If all the devices using power resource are already in D3 state, the power resource won't be referred. In such case it should be put into OFF state. If we don't load the device driver for one device, can we consider that the power resource is not required by the this? which state can be regarded as the correct state? D0 or D3? If the D0 is considered as the correct state, the power resource should be in ON state. If the D3 is considered as the correct state, the power resource is not required by the device. In such case the power resource had better in OFF state. Now for the ACPI fan device. It will be checked whether it should be turned on/off. If it should be turned off, the power resource referred by the fan device will be put in OFF state. For the PCI device: When the PCI device is enabled, the acpi_pci_set_power_state hook function will be called. And the power resource is required, it will be put in ON state. When the PCI device is disabled, it will derefer the power resource. When there is no reference for the power resource, it will be put in OFF state.(This is done by the last device that using power resource). Maybe I don't understand the meaning of this bug. This purpose of this bug is that the acpi power resource should be disabled when "unused". But it is not clear how the acpi power resource is not used. That is to say, which case can be regarded that the power resource is not used. Thanks.
Well, nowadays, this report is about the power resources not working, and not being easily fixed. It is about more than they not being disabled :( First, we were not registering all power resources that we should as "referenced by an active device". This is PROBLEM-1. My patch fixes this, but it exposes other problems. We are not turning devices off, and therefore we are not releasing power resources. This is PROBLEM-2. We can't always turn devices off, so the fix for this is not simply to always turn devices off. We are not detaching power resources when a module is unloaded (because of PROBLEM-2). This leaves dangling pointers in linked lists, and is PROBLEM-3. We can't fix PROBLEM-1 without fixing PROBLEM-3, because it will trigger PROBLEM-3 in every ThinkPad produced in the last five years. We are not checking for power resources that are to be turned off before sleep. This is PROBLEM-4, and fixed by a revert of ff24ba74b6d3befbfbafa142582211b5a6095d45, but we can only do this after we fix PROBLEM-1 (or we will regress on ThinkPads). We are not doing opportune scans to find power resources that could be turned off. This is PROBLEM-5, and just like PROBLEM-4, it can only be fixed after we fix PROBLEM-1 (or we will regress on ThinkPads). So, PROBLEM-2 is the key. It needs to be fixed first. I don't know how we can fix it.
ping Yakui...
Hi, Henrique Sorry for the late response. Thanks for pointing the issue. In fact the issue can be fixed by my patch that the power resource is turned off in the boot phase. And then it is turned on/off by changing the device state. For example: when we loading the driver for PCI device, the pci-acpi callback function will be called to turn on/off the power resource. But the side effect is the power resource is turned off if the BIOS handled the device already turned ON (with the power source enabled as well). Can we try to keep the device power original state in the boot phase and only disable the power resources that are used by any device? That means that the acpi device will still be in D0 state if it is set to D0 state by BIOS even when no driver is loaded for the corresponding PCI device/PNP device. In such case we don't change the state of the power resource related with this ACPI device. How about the following flowchart? (This will be done in the function of acpi_power_init). a. Loading the ACPI bus driver for the power resources, b. get the current power state for every device and set the current device power state again by calling the function of acpi_bus_set_power. That means that we will set it to D0 state if the device is in D0-state. This is to build the relationship between the power resource and ACPI device. (In fact when we set the device power state, the corresponding power resource will be enabled/disabled. And the reference count for the power resource will be updated.) c. enumerate the power resource list and disable the power resource that is not used by any ACPI device.(The reference count is zero) Is the above Ok? Hi, Henrique Do you agree with the above proposal? thanks.
I agree with the proposal as a good fix for PROBLEM-1, and it will be trivial to fix some of the other problems later. HOWEVER You do need to fix PROBLEM-2 as well (see comment #19), in the sense that we MUST unregister devices from the power resources (because of the linked list). Otherwise, you will have references to freed memory in the linked lists on the power resources, which is unacceptable. Example: USB EHCI and UHCI in most ThinkPads (you can use thinkpads to reproduce the bug I describe): Firmware has a single power resource, "PUBS", that is related to ACPI devices for the single EHCI and for the four UHCI devices. 1. Your patch will notice that the power resource is already in D0, and register a link between PUBS and EHCI. It will also do the same for UHCI. This is correct, and good. 2. User rmmods EHCI or UHCI: the 2.6.29 would NOT unregister the link between PUBS and EHCI/UHCI... (didn't check if you already fixed it in 2.6.30). 3. Something in the kernel tries to use the linked list of devices related to a power-resource: OOPS (if you are lucky), memory corruption (if you're unlucky), etc. So, we need to _ALWAYS_ unregister the devices from the power resources. This need to be fixed first.
(In reply to comment #22) > I agree with the proposal as a good fix for PROBLEM-1, and it will be trivial > to fix some of the other problems later. > > HOWEVER > > You do need to fix PROBLEM-2 as well (see comment #19), in the sense that we > MUST unregister devices from the power resources (because of the linked > list). > Otherwise, you will have references to freed memory in the linked lists on > the > power resources, which is unacceptable. > > Example: USB EHCI and UHCI in most ThinkPads (you can use thinkpads to > reproduce the bug I describe): > > Firmware has a single power resource, "PUBS", that is related to ACPI devices > for the single EHCI and for the four UHCI devices. > > 1. Your patch will notice that the power resource is already in D0, and > register a link between PUBS and EHCI. It will also do the same for UHCI. > This is correct, and good. > > 2. User rmmods EHCI or UHCI: the 2.6.29 would NOT unregister the link between > PUBS and EHCI/UHCI... (didn't check if you already fixed it in 2.6.30). If the PCI device state is set to D3 when unloading the driver, the link between the device and power resource will be freed. But it seems that the PCI device state is not touched when unloading most pci driver. In such case the link between the device and power resource can't be freed. In fact this issue is related with which state the device is in when there is no device driver. D0 / D3? Or the device state should not be touched when there is no device driver. Thanks. > > 3. Something in the kernel tries to use the linked list of devices related to > a > power-resource: OOPS (if you are lucky), memory corruption (if you're > unlucky), etc. > > So, we need to _ALWAYS_ unregister the devices from the power resources. > This > need to be fixed first.
No, it is not related. There are many defective PCI devices which cause severe problems when they are placed in D3 (the most common is that they cannot be resurrected back to D0 until the system power cycles). There are also devices which the kernel doesn't know how to initialize, so it can't place them on D3, because it doesn't know how to resurrect them later (some GPUs, for example). These devices sometimes are already in D0, but I can't say if they are always already in D0. But many devices that work well can be set to D0 by the BIOS, so this wouldn't help at all, anyway. Due to the D3 problem of some devices, PCI will *NOT* place any devices in D3 right now (which is bad). I hope they will fix it soon, so that only broken or problematic devices will not be put in D3. To me, it looks like splitting "put device in D3" from "unlink power resources" is the only way to fix this. Maybe use an API for the disable device call, that lets one to say "DEVICE_REMAIN_IN_D0", so that PCI can always call it? Either that, or you must change the way power resources keep track of devices, so that they can notice when a device has gone away.
(In reply to comment #24) > No, it is not related. There are many defective PCI devices which cause > severe > problems when they are placed in D3 (the most common is that they cannot be > resurrected back to D0 until the system power cycles). There are also > devices > which the kernel doesn't know how to initialize, so it can't place them on > D3, > because it doesn't know how to resurrect them later (some GPUs, for example). > > These devices sometimes are already in D0, but I can't say if they are always > already in D0. But many devices that work well can be set to D0 by the BIOS, > so this wouldn't help at all, anyway. > > Due to the D3 problem of some devices, PCI will *NOT* place any devices in D3 > right now (which is bad). I hope they will fix it soon, so that only broken > or > problematic devices will not be put in D3. If the PCI device can't be put in PCI_D3HOT/PCI_D3cold when unloading device driver, how can we put the corresponding ACPI device into D3 state? Maybe when we put the corresponding ACPI device into D3 state, the power resource is turned off, which causes that the PCI device can't work well. > > To me, it looks like splitting "put device in D3" from "unlink power > resources" > is the only way to fix this. Maybe use an API for the disable device call, > that lets one to say "DEVICE_REMAIN_IN_D0", so that PCI can always call it? > > Either that, or you must change the way power resources keep track of > devices, > so that they can notice when a device has gone away.
(In reply to comment #25) > (In reply to comment #24) > > Due to the D3 problem of some devices, PCI will *NOT* place any devices in > D3 > > right now (which is bad). I hope they will fix it soon, so that only > broken or > > problematic devices will not be put in D3. > If the PCI device can't be put in PCI_D3HOT/PCI_D3cold when unloading device > driver, how can we put the corresponding ACPI device into D3 state? > Maybe when we put the corresponding ACPI device into D3 state, the power > resource is turned off, which causes that the PCI device can't work well. Indeed, we will (unfortunately) have devices that are _never_ put in ACPI D3, but the module (driver) for these devices _will_ be removed (rmmod). So, as far as I can see, ACPI power resource handling MUST: 1. release the power resource from the device before the device is removed, even if it is not put it in D3 (you can set a marker to never turn off the power resource for devices that are removed without being put in D3). OR 2. stop using pointers to devices in the power resources, so that it doesn't crash the kernel after a rmmod... I think (1) is best. Do you have any other ideas for solving this problem?
(In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #24) > > > Due to the D3 problem of some devices, PCI will *NOT* place any devices > in D3 > > > right now (which is bad). I hope they will fix it soon, so that only > broken or > > > problematic devices will not be put in D3. > > If the PCI device can't be put in PCI_D3HOT/PCI_D3cold when unloading > device > > driver, how can we put the corresponding ACPI device into D3 state? > > Maybe when we put the corresponding ACPI device into D3 state, the power > > resource is turned off, which causes that the PCI device can't work well. > > Indeed, we will (unfortunately) have devices that are _never_ put in ACPI D3, > but the module (driver) for these devices _will_ be removed (rmmod). > > So, as far as I can see, ACPI power resource handling MUST: > > 1. release the power resource from the device before the device is removed, > even if it is not put it in D3 (you can set a marker to never turn off the > power resource for devices that are removed without being put in D3). The main purpose of releasing the reference between power resource and device is to turn off the power resource when it is not used by any device. what is the meaning of setting the marker to never turn off the power resource? > > OR > > 2. stop using pointers to devices in the power resources, so that it doesn't > crash the kernel after a rmmod... This is not an issue. The acpi device is not removed after a rmmod. So it won't crash the kernel even when we access the corresponding ACPI device. At the same time the power resource is used internally by the ACPI device. It is not meaningful to get by which device it is used. Instead at most case we will get the power resources for one ACPI device. In fact IMO the purpose of this bug is to disable the power resource when it is not used by any device. This includes the following two cases: a. the power resource is never used by any device. b. the Device power state is in D3. We had better disable the power resource used by it. If the device power state is in D0, we can't turn off the power resource even when there is no device driver for the corresponding PCI device. > > I think (1) is best. Do you have any other ideas for solving this problem?
Yakui, any update on this?
Hi, Henrique Rafael has already pushed a bunch of patches related with power resources. Can the issue mentioned in this bug be fixed by these patches? Thanks. Yakui
It got rid of the list to (potentially freed) memory, so it fixed PROBLEM-3 for sure. Which is good, that means the other problems can be fixed independently as long as PROBLEM-1 is fixed (otherwise, it is dangerous to have any code that could turn a non-fan power resource off). I can't figure out whether it fixes PROBLEM-1 or not just from a quick look, and likewise I am not sure whether it fixes PROBLEMS-4 and -5. And PROBLEM-2 depends on the PCI subsystem, which I know little of. Unfortunately, I can't risk testing a non-stable new kernel on my T43 right now, and these fixes were not backported to any stable/longterm kernels yet.
Hi, Henrique, I think the problem still exists, right? I'll look at this issue as Yakui no longer works on ACPI any more.
The current code either keeps the device in the state set by BIOS during boot, or reset it to D0. And IMO, this is the right way to go because it is hard to say which devices are unused. For example, the system boots with Fan running, if the fan driver is not built, is it proper to turn off the fan by ACPI core code? IMO, the answer is no. Thus I will close this bug report.