Re: [PATCH v2 1/5] ACPI: Enable driver and firmware hints to control power at probe time

[Sakari Ailus]

Hi Greg,

Thanks for the comments.

On Mon, Aug 26, 2019 at 10:46:34AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 26, 2019 at 11:31:08AM +0300, Sakari Ailus wrote:
> > Allow drivers and firmware tell ACPI that there's no need to power on a
> > device for probe. This requires both a hint from the firmware as well as
> > an indication from a driver to leave the device off.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/acpi/device_pm.c | 15 +++++++++++++--
> >  include/linux/device.h   |  7 +++++++
> >  2 files changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > index f616b16c1f0be..adcdf78ce4de8 100644
> > --- a/drivers/acpi/device_pm.c
> > +++ b/drivers/acpi/device_pm.c
> > @@ -1276,7 +1276,12 @@ static void acpi_dev_pm_detach(struct device *dev, bool power_off)
> >  	if (adev && dev->pm_domain == &acpi_general_pm_domain) {
> >  		dev_pm_domain_set(dev, NULL);
> >  		acpi_remove_pm_notifier(adev);
> > -		if (power_off) {
> > +		if (power_off
> > +#ifdef CONFIG_PM
> > +		    && !(dev->driver->probe_low_power &&
> > +			 device_property_present(dev, "probe-low-power"))
> > +#endif
> 
> As proof of the "only a bus-specific thing", why is probe_low_power even
> needed?  Why not just always trigger off of this crazy device_property?
> That makes the driver changes less.

That's an option, too, but firmware having this property for a device the
driver of which doesn't expect it will fail to power on the device for
probe. This leaves some room for unexpected failures that admittedly are
easy to fix, but could be harder to debug.

> 
> Also, is this #ifdef really needed?

I thought it was but it seems if CONFIG_PM is disabled,
dev_pm_domain_attach() has a nop implementation. So I agree it is not.

-- 
Regards,

Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx