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

[Rafael J. Wysocki]

On Wed, Aug 28, 2019 at 11:57 AM Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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. :-)