On Wed, 2010-12-15 at 04:24 +0800, Rafael J. Wysocki wrote: > On Tuesday, December 14, 2010, ykzhao wrote: > > On Tue, 2010-12-14 at 05:25 +0800, Rafael J. Wysocki wrote: > > > On Monday, December 13, 2010, ykzhao wrote: > > > > On Sun, 2010-12-12 at 06:39 +0800, Rafael J. Wysocki wrote: > > > > > Hi Len, > > > > > > > > > > The following three patches seem to have been dropped from your 'test' branch. > > > > > > > > > > If that happened by accident, please reapply. Otherwise, please let me know > > > > > what's wrong with the patches so that I can fix them. > > > > > > > > > > [1/3] - Make fujitsu_laptop use acpi_bus_update_power() instead of > > > > > acpi_bus_get_power() which is unsafe. > > > > > > > > ïIt seems that the function of acpi_bus_update_power not only obtains > > > > the current power state, but also set the corresponding power state. > > > > Right? > > > > > > Yes, it does. > > > > > > > If the device reports the bogus power state, maybe we will set the > > > > incorrect power state for the corresponding device when using the > > > > function of acpi_bus_update_power instead of acpi_bus_get_power. > > > > > > Please actually look at acpi_bus_get_power() (being removed by [2/3]) and note > > > that it _also_ modifies device->power.state (it doesn't return the state, actually), > > > so if the returned state is really bogus, we'll have a mismatch between > > > device->power.state and the real state of the device. This cannot be good. > > > > The device->power.state is also updated in the function of > > acpi_bus_get_power. But it won't try to call the _PSC or _ON/OFF method > > to really change the corresponding power state. It only reports the > > corresponding power state. > > > > > In the case of acpi_bus_update_power() we at least _try_ to keep the two things > > > in sync. > > > > Yes. I agree that the acpi_bus_update_power can always assure that the > > two things are in sync state. But ïsome systems will report the bogus > > power state although we already set another power state(For example: > > Maybe it reports that it is in D3 state because of bogus BIOS code). In > > such case if we try to turn off the corresponding power resource by > > using _PSC/_ON/_OFF method, maybe we will cut off the power supply for > > these devices. > > If you put a device into D0 and __acpi_bus_get_power() sees it in D3, you never > know why that is so. Either the device refused to change the state, in which > case the reported value is valid, or it reports a wrong value, but there's no > way to tell which the case really is. Assuming that the reported value is > possibly wrong, we mishandle the case when it's actually correct. Assuming > that the reported value is correct we may end up putting the device into the > state corresponding to the reported value, which effectively prevents it from > going into D0. In this case, though, our knowledge of the device state is > consistent with the reported state, which I think is better. > > Anyway, IMO the only place where acpi_bus_update_power() may possibly cause > problems to happen on broken hardware is fan_get_cur_state(), but I'll only > change that if anyone reports a problem with it. The other usage cases are (a) > initialization and (b) resume where we have no real choice but to actually > trust the state reported ACPI control methods (because we don't know what > it is in the first place). Agree. Maybe the matter is the broken BIOS/hardware. > > > Note, this is _essentially_ important for power resources (if > > > acpi_bus_get_power() is used, the refcounts are _guaranteed_ not to be in sync > > > with device->power.state in some situations). > > > > > > > In such case maybe the device can't work well. > > > > > > > > The bogus power state is reported for some devices on some laptops. For > > > > example: > > > > http://bugzilla.kernel.org/show_bug.cgi?id=8049 > > > > http://bugzilla.kernel.org/show_bug.cgi?id=11000 > > > > > > These bugs are about acpi_bus_set_power() doing the acpi_bus_get_power() > > > before setting the state, which is wrong and is being removed by my previous > > > patches (now in the Len's tree). > > > > It seems that another point is missed in previous patch. > > Before the reworking patch of power resource, the force_power_state > > flag is used when setting the corresponding power state for some ACPI > > devices(For example: Fan). This flag will still force to call the > > _PSC/_ON/_OFF method even when it is already the same as the target > > state. Maybe this is to workaround the BIOS issue that the power state > > is not reported correctly first time. > > > > Not sure whether my above understanding is reasonable. > > The force_power_state was _only_ a workaround for the case when > acpi_bus_set_power() called acpi_bus_get_power() and apparently received > a wrong value from it on some systems (two of them had been identified, one was > blacklisted in the end). However, after my changes acpi_bus_set_power() > doesn't call acpi_bus_get_power() (or any equivalent) any more, so this > workaround isn't necessary any more either. The flag of force_power_state will force it to do the transition of power state even when the target state is already the same as the current power state. Without this flag, it will skip the power transition if the two states are consistent. This is useful under the following cases: (The FAN device is used as an example) 1. The initial state is reported as D0 state (But we don't know whether the FAN device is really turned on) 2. If we expect to turn on the corresponding FAN device for the first time(put it into D0 state), it will skip the power transition as the check of "state == device->power.state". In such case it will be better that the power transition is forced in order to assure that the FAN device is turned on by calling the _PSC/_ON object. Thanks. Yakui > > Thanks, > Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
