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). > > 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. Thanks, Rafael _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
