On 29 August 2017 at 17:27, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote: > On Tuesday, August 29, 2017 4:56:48 PM CEST Ulf Hansson wrote: >> This change enables the ACPI PM domain to cope with drivers that deploys >> the runtime PM centric path for system sleep. > > [cut] > >> @@ -1052,11 +1066,20 @@ EXPORT_SYMBOL_GPL(acpi_subsys_complete); >> * @dev: Device to handle. >> * >> * Follow PCI and resume devices suspended at run time before running their >> - * system suspend callbacks. >> + * system suspend callbacks. However, try to avoid it in case the runtime PM >> + * centric path is used for the device and then trust the driver to do the >> + * right thing. >> */ >> int acpi_subsys_suspend(struct device *dev) >> { >> - pm_runtime_resume(dev); >> + struct acpi_device *adev = ACPI_COMPANION(dev); >> + >> + if (!adev) >> + return 0; >> + >> + if (!dev_pm_is_rpm_sleep(dev) || acpi_dev_needs_resume(dev, adev)) >> + pm_runtime_resume(dev); >> + >> return pm_generic_suspend(dev); >> } >> EXPORT_SYMBOL_GPL(acpi_subsys_suspend); > > Well, I tried to avoid calling acpi_dev_needs_resume() for multiple times > and that's why I added the update_state thing. > > Moreover, the is_rpm_sleep flag here has to mean not only that > direct_complete should not be used with the device, but also that its driver > is fine with not resuming it. Let me try to explain this better. I realize the changelog is misleading around this particular section! Huh, apologize for that! First, patch1 makes the PM core treat the is_rpm_sleep flag as the direct_complete isn't allowed for the device. For that reason, when the is_rpm_sleep is set, there is no point calling acpi_dev_needs_resume() from acpi_subsys_prepare(), but instead that can be deferred to acpi_subsys_suspend() - because it doesn't matter if acpi_subsys_prepare() returns 0 or 1, in either case the acpi_subsys_suspend() will be called. That's really what goes on here. The end result is the same. If the acpi_dev_needs_resume() thinks that the device needs to be runtime resumed, pm_runtime_resume() is called for the device in acpi_subsys_suspend(). So, this has nothing to do with whether the driver "is fine with not resuming it" thing. > > IMO it is not a good idea to use one flag for these two different things at the > same time at all. Yeah, I guess my upper comment addresses your immediate concern here? However, there is one other thing the is_rpm_flag means. That is that the driver has informed the ACPI PM domain, to trust the driver to deal with system sleep, via re-using the runtime PM callbacks. So the flag does still have two meanings, but that we can change - of course. > > Thanks, > Rafael > Kind regards Uffe -- 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