| Message ID | 20190826083112.8888-2-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Support running driver's probe for a device powered off | expand |
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@linux.intel.com> > --- > 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 > + ) { > /* > * If the device's PM QoS resume latency limit or flags > * have been exposed to user space, they have to be > @@ -1324,7 +1329,13 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on) > > acpi_add_pm_notifier(adev, dev, acpi_pm_notify_work_func); > dev_pm_domain_set(dev, &acpi_general_pm_domain); > - if (power_on) { > + > + if (power_on > +#ifdef CONFIG_PM > + && !(dev->driver->probe_low_power && > + device_property_present(dev, "probe-low-power")) > +#endif > + ) { > acpi_dev_pm_full_power(adev); > acpi_device_wakeup_disable(adev); > } > diff --git a/include/linux/device.h b/include/linux/device.h > index 6717adee33f01..4bc0ea4a3201a 100644 > --- a/include/linux/device.h > +++ b/include/linux/device.h > @@ -248,6 +248,12 @@ enum probe_type { > * @owner: The module owner. > * @mod_name: Used for built-in modules. > * @suppress_bind_attrs: Disables bind/unbind via sysfs. > + * @probe_low_power: The driver supports its probe function being called while > + * the device is in a low power state, independently of the > + * expected behaviour on combination of a given bus and > + * firmware interface etc. The driver is responsible for > + * powering the device on using runtime PM in such case. > + * This configuration has no effect if CONFIG_PM is disabled. > * @probe_type: Type of the probe (synchronous or asynchronous) to use. > * @of_match_table: The open firmware table. > * @acpi_match_table: The ACPI match table. > @@ -285,6 +291,7 @@ struct device_driver { > const char *mod_name; /* used for built-in modules */ > > bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ > + bool probe_low_power; Ick, no, this should be a bus-specific thing to handle such messed up hardware. Why polute this in the driver core? thanks, greg k-h
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@linux.intel.com> > --- > 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. Also, is this #ifdef really needed? thanks, greg k-h
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@linux.intel.com> > > --- > > 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.
Hi Greg, On Mon, Aug 26, 2019 at 10:43:43AM +0200, Greg Kroah-Hartman wrote: ... > > diff --git a/include/linux/device.h b/include/linux/device.h > > index 6717adee33f01..4bc0ea4a3201a 100644 > > --- a/include/linux/device.h > > +++ b/include/linux/device.h > > @@ -248,6 +248,12 @@ enum probe_type { > > * @owner: The module owner. > > * @mod_name: Used for built-in modules. > > * @suppress_bind_attrs: Disables bind/unbind via sysfs. > > + * @probe_low_power: The driver supports its probe function being called while > > + * the device is in a low power state, independently of the > > + * expected behaviour on combination of a given bus and > > + * firmware interface etc. The driver is responsible for > > + * powering the device on using runtime PM in such case. > > + * This configuration has no effect if CONFIG_PM is disabled. > > * @probe_type: Type of the probe (synchronous or asynchronous) to use. > > * @of_match_table: The open firmware table. > > * @acpi_match_table: The ACPI match table. > > @@ -285,6 +291,7 @@ struct device_driver { > > const char *mod_name; /* used for built-in modules */ > > > > bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ > > + bool probe_low_power; > > Ick, no, this should be a bus-specific thing to handle such messed up > hardware. Why polute this in the driver core? The alternative could be to make it I²C specific indeed; the vast majority of camera sensors are I²C devices these days.
On Mon, Aug 26, 2019 at 01:32:00PM +0300, Sakari Ailus wrote: > Hi Greg, > > On Mon, Aug 26, 2019 at 10:43:43AM +0200, Greg Kroah-Hartman wrote: > > ... > > > > diff --git a/include/linux/device.h b/include/linux/device.h > > > index 6717adee33f01..4bc0ea4a3201a 100644 > > > --- a/include/linux/device.h > > > +++ b/include/linux/device.h > > > @@ -248,6 +248,12 @@ enum probe_type { > > > * @owner: The module owner. > > > * @mod_name: Used for built-in modules. > > > * @suppress_bind_attrs: Disables bind/unbind via sysfs. > > > + * @probe_low_power: The driver supports its probe function being called while > > > + * the device is in a low power state, independently of the > > > + * expected behaviour on combination of a given bus and > > > + * firmware interface etc. The driver is responsible for > > > + * powering the device on using runtime PM in such case. > > > + * This configuration has no effect if CONFIG_PM is disabled. > > > * @probe_type: Type of the probe (synchronous or asynchronous) to use. > > > * @of_match_table: The open firmware table. > > > * @acpi_match_table: The ACPI match table. > > > @@ -285,6 +291,7 @@ struct device_driver { > > > const char *mod_name; /* used for built-in modules */ > > > > > > bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ > > > + bool probe_low_power; > > > > Ick, no, this should be a bus-specific thing to handle such messed up > > hardware. Why polute this in the driver core? > > The alternative could be to make it I²C specific indeed; the vast majority > of camera sensors are I²C devices these days. Why is this even needed to be a bus/device attribute at all? You are checking the firmware property in the probe function, just do the logic there as you are, what needs to be saved to the bus's logic? thanks, greg k-h
On Mon, Aug 26, 2019 at 03:34:39PM +0200, Greg Kroah-Hartman wrote: > On Mon, Aug 26, 2019 at 01:32:00PM +0300, Sakari Ailus wrote: > > Hi Greg, > > > > On Mon, Aug 26, 2019 at 10:43:43AM +0200, Greg Kroah-Hartman wrote: > > > > ... > > > > > > diff --git a/include/linux/device.h b/include/linux/device.h > > > > index 6717adee33f01..4bc0ea4a3201a 100644 > > > > --- a/include/linux/device.h > > > > +++ b/include/linux/device.h > > > > @@ -248,6 +248,12 @@ enum probe_type { > > > > * @owner: The module owner. > > > > * @mod_name: Used for built-in modules. > > > > * @suppress_bind_attrs: Disables bind/unbind via sysfs. > > > > + * @probe_low_power: The driver supports its probe function being called while > > > > + * the device is in a low power state, independently of the > > > > + * expected behaviour on combination of a given bus and > > > > + * firmware interface etc. The driver is responsible for > > > > + * powering the device on using runtime PM in such case. > > > > + * This configuration has no effect if CONFIG_PM is disabled. > > > > * @probe_type: Type of the probe (synchronous or asynchronous) to use. > > > > * @of_match_table: The open firmware table. > > > > * @acpi_match_table: The ACPI match table. > > > > @@ -285,6 +291,7 @@ struct device_driver { > > > > const char *mod_name; /* used for built-in modules */ > > > > > > > > bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ > > > > + bool probe_low_power; > > > > > > Ick, no, this should be a bus-specific thing to handle such messed up > > > hardware. Why polute this in the driver core? > > > > The alternative could be to make it I²C specific indeed; the vast majority > > of camera sensors are I²C devices these days. > > Why is this even needed to be a bus/device attribute at all? You are > checking the firmware property in the probe function, just do the logic > there as you are, what needs to be saved to the bus's logic? By the time the driver gets hold of the device, or gets to control its power state, the I²C framework has already called dev_pm_domain_attach() to power on the device. The I²C drivers on ACPI based systems expect the device to be powered on for probe.
On Mon, Aug 26, 2019 at 3:34 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Mon, Aug 26, 2019 at 01:32:00PM +0300, Sakari Ailus wrote: > > Hi Greg, > > > > On Mon, Aug 26, 2019 at 10:43:43AM +0200, Greg Kroah-Hartman wrote: > > > > ... > > > > > > diff --git a/include/linux/device.h b/include/linux/device.h > > > > index 6717adee33f01..4bc0ea4a3201a 100644 > > > > --- a/include/linux/device.h > > > > +++ b/include/linux/device.h > > > > @@ -248,6 +248,12 @@ enum probe_type { > > > > * @owner: The module owner. > > > > * @mod_name: Used for built-in modules. > > > > * @suppress_bind_attrs: Disables bind/unbind via sysfs. > > > > + * @probe_low_power: The driver supports its probe function being called while > > > > + * the device is in a low power state, independently of the > > > > + * expected behaviour on combination of a given bus and > > > > + * firmware interface etc. The driver is responsible for > > > > + * powering the device on using runtime PM in such case. > > > > + * This configuration has no effect if CONFIG_PM is disabled. > > > > * @probe_type: Type of the probe (synchronous or asynchronous) to use. > > > > * @of_match_table: The open firmware table. > > > > * @acpi_match_table: The ACPI match table. > > > > @@ -285,6 +291,7 @@ struct device_driver { > > > > const char *mod_name; /* used for built-in modules */ > > > > > > > > bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ > > > > + bool probe_low_power; > > > > > > Ick, no, this should be a bus-specific thing to handle such messed up > > > hardware. Why polute this in the driver core? > > > > The alternative could be to make it I²C specific indeed; the vast majority > > of camera sensors are I²C devices these days. > > Why is this even needed to be a bus/device attribute at all? You are > checking the firmware property in the probe function, just do the logic > there as you are, what needs to be saved to the bus's logic? The situation today is that all devices are put into D0 by the ACPI layer before driver probing since drivers generally expect devices to be in D0 when their probe routines run. If the driver is prepared to cope with devices in low-power states, though, powering them up before probing for a driver may not be necessary, but still the core (or generally the code invoking the driver probe) needs to know that the driver really is prepared for that. Hence the driver flag AFAICS. Now, in theory there may be some platform requirements regarding the power states of devices during driver probe, although admittedly it is not entirely clear to me why that would be the case) and hence the property. I would think that if the driver could cope with devices in low-power states during probe, the platform wouldn't need to worry about that.
Hi Rafael, On Wed, Aug 28, 2019 at 10:55:42AM +0200, Rafael J. Wysocki wrote: > On Mon, Aug 26, 2019 at 3:34 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Mon, Aug 26, 2019 at 01:32:00PM +0300, Sakari Ailus wrote: > > > Hi Greg, > > > > > > On Mon, Aug 26, 2019 at 10:43:43AM +0200, Greg Kroah-Hartman wrote: > > > > > > ... > > > > > > > > diff --git a/include/linux/device.h b/include/linux/device.h > > > > > index 6717adee33f01..4bc0ea4a3201a 100644 > > > > > --- a/include/linux/device.h > > > > > +++ b/include/linux/device.h > > > > > @@ -248,6 +248,12 @@ enum probe_type { > > > > > * @owner: The module owner. > > > > > * @mod_name: Used for built-in modules. > > > > > * @suppress_bind_attrs: Disables bind/unbind via sysfs. > > > > > + * @probe_low_power: The driver supports its probe function being called while > > > > > + * the device is in a low power state, independently of the > > > > > + * expected behaviour on combination of a given bus and > > > > > + * firmware interface etc. The driver is responsible for > > > > > + * powering the device on using runtime PM in such case. > > > > > + * This configuration has no effect if CONFIG_PM is disabled. > > > > > * @probe_type: Type of the probe (synchronous or asynchronous) to use. > > > > > * @of_match_table: The open firmware table. > > > > > * @acpi_match_table: The ACPI match table. > > > > > @@ -285,6 +291,7 @@ struct device_driver { > > > > > const char *mod_name; /* used for built-in modules */ > > > > > > > > > > bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ > > > > > + bool probe_low_power; > > > > > > > > Ick, no, this should be a bus-specific thing to handle such messed up > > > > hardware. Why polute this in the driver core? > > > > > > The alternative could be to make it I²C specific indeed; the vast majority > > > of camera sensors are I²C devices these days. > > > > Why is this even needed to be a bus/device attribute at all? You are > > checking the firmware property in the probe function, just do the logic > > there as you are, what needs to be saved to the bus's logic? > > The situation today is that all devices are put into D0 by the ACPI > layer before driver probing since drivers generally expect devices to > be in D0 when their probe routines run. If the driver is prepared to > cope with devices in low-power states, though, powering them up before > probing for a driver may not be necessary, but still the core (or > generally the code invoking the driver probe) needs to know that the > driver really is prepared for that. Hence the driver flag AFAICS. > > Now, in theory there may be some platform requirements regarding the > power states of devices during driver probe, although admittedly it is > not entirely clear to me why that would be the case) and hence the Please see the cover page of the set (also here): <URL:https://lkml.org/lkml/2019/8/26/175> > property. I would think that if the driver could cope with devices in > low-power states during probe, the platform wouldn't need to worry > about that. I understand this as driver deciding whether it'd power on the device during probe. That way there's no way to judge whether the device is accessible, and probe would succeed without an error, which then manifests itself on the first access of the device. Such as on the at24 EEPROM driver, the error would take place on first read of the contents, not in probe. Somebody might consider that as a driver bug.
On Wed, Aug 28, 2019 at 11:57 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Rafael, > > On Wed, Aug 28, 2019 at 10:55:42AM +0200, Rafael J. Wysocki wrote: > > On Mon, Aug 26, 2019 at 3:34 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > > > > On Mon, Aug 26, 2019 at 01:32:00PM +0300, Sakari Ailus wrote: > > > > Hi Greg, > > > > > > > > On Mon, Aug 26, 2019 at 10:43:43AM +0200, Greg Kroah-Hartman wrote: > > > > > > > > ... > > > > > > > > > > diff --git a/include/linux/device.h b/include/linux/device.h > > > > > > index 6717adee33f01..4bc0ea4a3201a 100644 > > > > > > --- a/include/linux/device.h > > > > > > +++ b/include/linux/device.h > > > > > > @@ -248,6 +248,12 @@ enum probe_type { > > > > > > * @owner: The module owner. > > > > > > * @mod_name: Used for built-in modules. > > > > > > * @suppress_bind_attrs: Disables bind/unbind via sysfs. > > > > > > + * @probe_low_power: The driver supports its probe function being called while > > > > > > + * the device is in a low power state, independently of the > > > > > > + * expected behaviour on combination of a given bus and > > > > > > + * firmware interface etc. The driver is responsible for > > > > > > + * powering the device on using runtime PM in such case. > > > > > > + * This configuration has no effect if CONFIG_PM is disabled. > > > > > > * @probe_type: Type of the probe (synchronous or asynchronous) to use. > > > > > > * @of_match_table: The open firmware table. > > > > > > * @acpi_match_table: The ACPI match table. > > > > > > @@ -285,6 +291,7 @@ struct device_driver { > > > > > > const char *mod_name; /* used for built-in modules */ > > > > > > > > > > > > bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ > > > > > > + bool probe_low_power; > > > > > > > > > > Ick, no, this should be a bus-specific thing to handle such messed up > > > > > hardware. Why polute this in the driver core? > > > > > > > > The alternative could be to make it I²C specific indeed; the vast majority > > > > of camera sensors are I²C devices these days. > > > > > > Why is this even needed to be a bus/device attribute at all? You are > > > checking the firmware property in the probe function, just do the logic > > > there as you are, what needs to be saved to the bus's logic? > > > > The situation today is that all devices are put into D0 by the ACPI > > layer before driver probing since drivers generally expect devices to > > be in D0 when their probe routines run. If the driver is prepared to > > cope with devices in low-power states, though, powering them up before > > probing for a driver may not be necessary, but still the core (or > > generally the code invoking the driver probe) needs to know that the > > driver really is prepared for that. Hence the driver flag AFAICS. > > > > Now, in theory there may be some platform requirements regarding the > > power states of devices during driver probe, although admittedly it is > > not entirely clear to me why that would be the case) and hence the > > Please see the cover page of the set (also here): > > <URL:https://lkml.org/lkml/2019/8/26/175> > > > property. I would think that if the driver could cope with devices in > > low-power states during probe, the platform wouldn't need to worry > > about that. > > I understand this as driver deciding whether it'd power on the device > during probe. > > That way there's no way to judge whether the device is accessible, and > probe would succeed without an error, which then manifests itself on the > first access of the device. OK, so the property really represents the platform preference in that respect and the presence of it by no means guarantees that there won't be any problems on the first device access. > Such as on the at24 EEPROM driver, the error > would take place on first read of the contents, not in probe. > > Somebody might consider that as a driver bug. Well, I guess you can argue that the safer thing, which therefore should be the default, is to power up the device before probing and to check whether or not it is accessible at that time. However, in some cases it may be desirable to avoid powering up the device at that point, whatever the reason, and the property provides a hint about that. Fair enough to me, but honestly I'm not sure about the example in the cover letter. :-)
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 + ) { /* * If the device's PM QoS resume latency limit or flags * have been exposed to user space, they have to be @@ -1324,7 +1329,13 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on) acpi_add_pm_notifier(adev, dev, acpi_pm_notify_work_func); dev_pm_domain_set(dev, &acpi_general_pm_domain); - if (power_on) { + + if (power_on +#ifdef CONFIG_PM + && !(dev->driver->probe_low_power && + device_property_present(dev, "probe-low-power")) +#endif + ) { acpi_dev_pm_full_power(adev); acpi_device_wakeup_disable(adev); } diff --git a/include/linux/device.h b/include/linux/device.h index 6717adee33f01..4bc0ea4a3201a 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -248,6 +248,12 @@ enum probe_type { * @owner: The module owner. * @mod_name: Used for built-in modules. * @suppress_bind_attrs: Disables bind/unbind via sysfs. + * @probe_low_power: The driver supports its probe function being called while + * the device is in a low power state, independently of the + * expected behaviour on combination of a given bus and + * firmware interface etc. The driver is responsible for + * powering the device on using runtime PM in such case. + * This configuration has no effect if CONFIG_PM is disabled. * @probe_type: Type of the probe (synchronous or asynchronous) to use. * @of_match_table: The open firmware table. * @acpi_match_table: The ACPI match table. @@ -285,6 +291,7 @@ struct device_driver { const char *mod_name; /* used for built-in modules */ bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ + bool probe_low_power; enum probe_type probe_type; const struct of_device_id *of_match_table;
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@linux.intel.com> --- drivers/acpi/device_pm.c | 15 +++++++++++++-- include/linux/device.h | 7 +++++++ 2 files changed, 20 insertions(+), 2 deletions(-)