[PATCH v1] driver core: Add state_synced sysfs file for devices that support it

[PATCH v1] driver core: Add state_synced sysfs file for devices that support it

[Saravana Kannan]

This can be used to check if a device supports sync_state() callbacks
and therefore keeps resources left on by the bootloader enabled till all
its consumers have probed.

This can also be used to check if sync_state() has been called for a
device or whether it is still trying to keep resources enabled because
they were left enabled by the bootloader and all its consumers haven't
probed yet.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 .../ABI/testing/sysfs-devices-state_synced    | 24 +++++++++++++++++++
 drivers/base/dd.c                             | 16 +++++++++++++
 2 files changed, 40 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-state_synced

diff --git a/Documentation/ABI/testing/sysfs-devices-state_synced b/Documentation/ABI/testing/sysfs-devices-state_synced
new file mode 100644
index 000000000000..0c922d7d02fc
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-state_synced
@@ -0,0 +1,24 @@
+What:		/sys/devices/.../state_synced
+Date:		May 2020
+Contact:	Saravana Kannan <saravanak@google.com>
+Description:
+		The /sys/devices/.../state_synced attribute is only present for
+		devices whose bus types or driver provides the .sync_state()
+		callback. The number read from it (0 or 1) reflects the value
+		of the device's 'state_synced' field. A value of 0 means the
+		.sync_state() callback hasn't been called yet. A value of 1
+		means the .sync_state() callback has been called.
+
+		Generally, if a device has sync_state() support and has some of
+		the resources it provides enabled at the time the kernel starts
+		(Eg: enabled by hardware reset or bootloader or anything that
+		run before the kernel starts), then it'll keep those resources
+		enabled and in a state that's compatible with the state they
+		were in at the start of the kernel. The device will stop doing
+		this only when the sync_state() callback has been called --
+		which happens only when all its consumer devices are registered
+		and have probed successfully. Resources that were left disabled
+		at the time the kernel starts are not affected or limited in
+		any way by sync_state() callbacks.
+
+
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 48ca81cb8ebc..72599436ae84 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -458,6 +458,13 @@ static void driver_deferred_probe_add_trigger(struct device *dev,
 		driver_deferred_probe_trigger();
 }
 
+static ssize_t state_synced_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%u\n", dev->state_synced);
+}
+static DEVICE_ATTR_RO(state_synced);
+
 static int really_probe(struct device *dev, struct device_driver *drv)
 {
 	int ret = -EPROBE_DEFER;
@@ -531,9 +538,16 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 		goto dev_groups_failed;
 	}
 
+	if (dev_has_sync_state(dev) &&
+	    device_create_file(dev, &dev_attr_state_synced)) {
+		dev_err(dev, "state_synced sysfs add failed\n");
+		goto dev_sysfs_state_synced_failed;
+	}
+
 	if (test_remove) {
 		test_remove = false;
 
+		device_remove_file(dev, &dev_attr_state_synced);
 		device_remove_groups(dev, drv->dev_groups);
 
 		if (dev->bus->remove)
@@ -563,6 +577,8 @@ static int really_probe(struct device *dev, struct device_driver *drv)
 		 drv->bus->name, __func__, dev_name(dev), drv->name);
 	goto done;
 
+dev_sysfs_state_synced_failed:
+	device_remove_groups(dev, drv->dev_groups);
 dev_groups_failed:
 	if (dev->bus->remove)
 		dev->bus->remove(dev);
-- 
2.26.2.645.ge9eca65c58-goog

Re: [PATCH v1] driver core: Add state_synced sysfs file for devices that support it

[Greg Kroah-Hartman]

On Tue, May 12, 2020 at 06:34:15PM -0700, Saravana Kannan wrote:
> This can be used to check if a device supports sync_state() callbacks
> and therefore keeps resources left on by the bootloader enabled till all
> its consumers have probed.
> 
> This can also be used to check if sync_state() has been called for a
> device or whether it is still trying to keep resources enabled because
> they were left enabled by the bootloader and all its consumers haven't
> probed yet.
> 
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  .../ABI/testing/sysfs-devices-state_synced    | 24 +++++++++++++++++++
>  drivers/base/dd.c                             | 16 +++++++++++++
>  2 files changed, 40 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-devices-state_synced
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-state_synced b/Documentation/ABI/testing/sysfs-devices-state_synced
> new file mode 100644
> index 000000000000..0c922d7d02fc
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-devices-state_synced
> @@ -0,0 +1,24 @@
> +What:		/sys/devices/.../state_synced
> +Date:		May 2020
> +Contact:	Saravana Kannan <saravanak@google.com>
> +Description:
> +		The /sys/devices/.../state_synced attribute is only present for
> +		devices whose bus types or driver provides the .sync_state()
> +		callback. The number read from it (0 or 1) reflects the value
> +		of the device's 'state_synced' field. A value of 0 means the
> +		.sync_state() callback hasn't been called yet. A value of 1
> +		means the .sync_state() callback has been called.
> +
> +		Generally, if a device has sync_state() support and has some of
> +		the resources it provides enabled at the time the kernel starts
> +		(Eg: enabled by hardware reset or bootloader or anything that
> +		run before the kernel starts), then it'll keep those resources
> +		enabled and in a state that's compatible with the state they
> +		were in at the start of the kernel. The device will stop doing
> +		this only when the sync_state() callback has been called --
> +		which happens only when all its consumer devices are registered
> +		and have probed successfully. Resources that were left disabled
> +		at the time the kernel starts are not affected or limited in
> +		any way by sync_state() callbacks.
> +
> +
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 48ca81cb8ebc..72599436ae84 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -458,6 +458,13 @@ static void driver_deferred_probe_add_trigger(struct device *dev,
>  		driver_deferred_probe_trigger();
>  }
>  
> +static ssize_t state_synced_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%u\n", dev->state_synced);
> +}
> +static DEVICE_ATTR_RO(state_synced);
> +
>  static int really_probe(struct device *dev, struct device_driver *drv)
>  {
>  	int ret = -EPROBE_DEFER;
> @@ -531,9 +538,16 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  		goto dev_groups_failed;
>  	}
>  
> +	if (dev_has_sync_state(dev) &&
> +	    device_create_file(dev, &dev_attr_state_synced)) {
> +		dev_err(dev, "state_synced sysfs add failed\n");
> +		goto dev_sysfs_state_synced_failed;
> +	}

Why not add this to the groups above this and only enable it if needed
at runtime?

The is_visible() callback should be what you need to use here.  That
will save you lots of housekeeping as well as properly remove the
attribute when the device is removed from the system (which you didn't
explicitly do in this patch...)

thanks,

greg k-h

Re: [PATCH v1] driver core: Add state_synced sysfs file for devices that support it

[Saravana Kannan]

On Wed, May 13, 2020 at 1:22 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, May 12, 2020 at 06:34:15PM -0700, Saravana Kannan wrote:
> > This can be used to check if a device supports sync_state() callbacks
> > and therefore keeps resources left on by the bootloader enabled till all
> > its consumers have probed.
> >
> > This can also be used to check if sync_state() has been called for a
> > device or whether it is still trying to keep resources enabled because
> > they were left enabled by the bootloader and all its consumers haven't
> > probed yet.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  .../ABI/testing/sysfs-devices-state_synced    | 24 +++++++++++++++++++
> >  drivers/base/dd.c                             | 16 +++++++++++++
> >  2 files changed, 40 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-devices-state_synced
> >
> > diff --git a/Documentation/ABI/testing/sysfs-devices-state_synced b/Documentation/ABI/testing/sysfs-devices-state_synced
> > new file mode 100644
> > index 000000000000..0c922d7d02fc
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-devices-state_synced
> > @@ -0,0 +1,24 @@
> > +What:                /sys/devices/.../state_synced
> > +Date:                May 2020
> > +Contact:     Saravana Kannan <saravanak@google.com>
> > +Description:
> > +             The /sys/devices/.../state_synced attribute is only present for
> > +             devices whose bus types or driver provides the .sync_state()
> > +             callback. The number read from it (0 or 1) reflects the value
> > +             of the device's 'state_synced' field. A value of 0 means the
> > +             .sync_state() callback hasn't been called yet. A value of 1
> > +             means the .sync_state() callback has been called.
> > +
> > +             Generally, if a device has sync_state() support and has some of
> > +             the resources it provides enabled at the time the kernel starts
> > +             (Eg: enabled by hardware reset or bootloader or anything that
> > +             run before the kernel starts), then it'll keep those resources
> > +             enabled and in a state that's compatible with the state they
> > +             were in at the start of the kernel. The device will stop doing
> > +             this only when the sync_state() callback has been called --
> > +             which happens only when all its consumer devices are registered
> > +             and have probed successfully. Resources that were left disabled
> > +             at the time the kernel starts are not affected or limited in
> > +             any way by sync_state() callbacks.
> > +
> > +
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index 48ca81cb8ebc..72599436ae84 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -458,6 +458,13 @@ static void driver_deferred_probe_add_trigger(struct device *dev,
> >               driver_deferred_probe_trigger();
> >  }
> >
> > +static ssize_t state_synced_show(struct device *dev,
> > +                              struct device_attribute *attr, char *buf)
> > +{
> > +     return sprintf(buf, "%u\n", dev->state_synced);
> > +}
> > +static DEVICE_ATTR_RO(state_synced);
> > +
> >  static int really_probe(struct device *dev, struct device_driver *drv)
> >  {
> >       int ret = -EPROBE_DEFER;
> > @@ -531,9 +538,16 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> >               goto dev_groups_failed;
> >       }
> >
> > +     if (dev_has_sync_state(dev) &&
> > +         device_create_file(dev, &dev_attr_state_synced)) {
> > +             dev_err(dev, "state_synced sysfs add failed\n");
> > +             goto dev_sysfs_state_synced_failed;
> > +     }
>
> Why not add this to the groups above this and only enable it if needed
> at runtime?

Those groups above seem to be driver specific groups. Looking at the
code, some drivers seem to be setting them. Also, this attribute can
only be decided after a driver has successfully bound to the device
because dev_has_sync_state() has to check the bus and the driver for
sync_state() support.

> The is_visible() callback should be what you need to use here.

If this is an attribute specific property, it might work. I'll take a look.

>  That
> will save you lots of housekeeping as well as properly remove the
> attribute when the device is removed from the system (which you didn't
> explicitly do in this patch...)

I had a nagging feeling I was missing something. Duh! I'll make sure I
do proper clean up in v2.

Thanks for the review.

-Saravana

Re: [PATCH v1] driver core: Add state_synced sysfs file for devices that support it

[Saravana Kannan]

On Wed, May 13, 2020 at 1:34 AM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, May 13, 2020 at 1:22 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, May 12, 2020 at 06:34:15PM -0700, Saravana Kannan wrote:
> > > This can be used to check if a device supports sync_state() callbacks
> > > and therefore keeps resources left on by the bootloader enabled till all
> > > its consumers have probed.
> > >
> > > This can also be used to check if sync_state() has been called for a
> > > device or whether it is still trying to keep resources enabled because
> > > they were left enabled by the bootloader and all its consumers haven't
> > > probed yet.
> > >
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > ---
> > >  .../ABI/testing/sysfs-devices-state_synced    | 24 +++++++++++++++++++
> > >  drivers/base/dd.c                             | 16 +++++++++++++
> > >  2 files changed, 40 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-devices-state_synced
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-devices-state_synced b/Documentation/ABI/testing/sysfs-devices-state_synced
> > > new file mode 100644
> > > index 000000000000..0c922d7d02fc
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-devices-state_synced
> > > @@ -0,0 +1,24 @@
> > > +What:                /sys/devices/.../state_synced
> > > +Date:                May 2020
> > > +Contact:     Saravana Kannan <saravanak@google.com>
> > > +Description:
> > > +             The /sys/devices/.../state_synced attribute is only present for
> > > +             devices whose bus types or driver provides the .sync_state()
> > > +             callback. The number read from it (0 or 1) reflects the value
> > > +             of the device's 'state_synced' field. A value of 0 means the
> > > +             .sync_state() callback hasn't been called yet. A value of 1
> > > +             means the .sync_state() callback has been called.
> > > +
> > > +             Generally, if a device has sync_state() support and has some of
> > > +             the resources it provides enabled at the time the kernel starts
> > > +             (Eg: enabled by hardware reset or bootloader or anything that
> > > +             run before the kernel starts), then it'll keep those resources
> > > +             enabled and in a state that's compatible with the state they
> > > +             were in at the start of the kernel. The device will stop doing
> > > +             this only when the sync_state() callback has been called --
> > > +             which happens only when all its consumer devices are registered
> > > +             and have probed successfully. Resources that were left disabled
> > > +             at the time the kernel starts are not affected or limited in
> > > +             any way by sync_state() callbacks.
> > > +
> > > +
> > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > index 48ca81cb8ebc..72599436ae84 100644
> > > --- a/drivers/base/dd.c
> > > +++ b/drivers/base/dd.c
> > > @@ -458,6 +458,13 @@ static void driver_deferred_probe_add_trigger(struct device *dev,
> > >               driver_deferred_probe_trigger();
> > >  }
> > >
> > > +static ssize_t state_synced_show(struct device *dev,
> > > +                              struct device_attribute *attr, char *buf)
> > > +{
> > > +     return sprintf(buf, "%u\n", dev->state_synced);
> > > +}
> > > +static DEVICE_ATTR_RO(state_synced);
> > > +
> > >  static int really_probe(struct device *dev, struct device_driver *drv)
> > >  {
> > >       int ret = -EPROBE_DEFER;
> > > @@ -531,9 +538,16 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> > >               goto dev_groups_failed;
> > >       }
> > >
> > > +     if (dev_has_sync_state(dev) &&
> > > +         device_create_file(dev, &dev_attr_state_synced)) {
> > > +             dev_err(dev, "state_synced sysfs add failed\n");
> > > +             goto dev_sysfs_state_synced_failed;
> > > +     }
> >
> > Why not add this to the groups above this and only enable it if needed
> > at runtime?
>
> Those groups above seem to be driver specific groups. Looking at the
> code, some drivers seem to be setting them. Also, this attribute can
> only be decided after a driver has successfully bound to the device
> because dev_has_sync_state() has to check the bus and the driver for
> sync_state() support.
>
> > The is_visible() callback should be what you need to use here.
>
> If this is an attribute specific property, it might work. I'll take a look.

I took a look at is_visible(). It only makes sense for a group of
attributes that are exposed in a sub directory. But state_synced is a
top level property IMO. What I'm doing is similar to the "online"
attribute that's exposed for a device.

And even if I create a group with one lone attribute, I'll still have
to add it after the device has probed or have to call
sysfs_update_groups() after the device is probed to update the
visibility. Seems quite messy and unnecessarily complicated.

I'd like to keep this an attribute and not create a group for that
reason. Does that sound okay to you? I'll obviously still do the clean
up correctly.

-Saravana

Re: [PATCH v1] driver core: Add state_synced sysfs file for devices that support it

[Greg Kroah-Hartman]

On Wed, May 13, 2020 at 11:49:57AM -0700, Saravana Kannan wrote:
> On Wed, May 13, 2020 at 1:34 AM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Wed, May 13, 2020 at 1:22 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, May 12, 2020 at 06:34:15PM -0700, Saravana Kannan wrote:
> > > > This can be used to check if a device supports sync_state() callbacks
> > > > and therefore keeps resources left on by the bootloader enabled till all
> > > > its consumers have probed.
> > > >
> > > > This can also be used to check if sync_state() has been called for a
> > > > device or whether it is still trying to keep resources enabled because
> > > > they were left enabled by the bootloader and all its consumers haven't
> > > > probed yet.
> > > >
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > > > ---
> > > >  .../ABI/testing/sysfs-devices-state_synced    | 24 +++++++++++++++++++
> > > >  drivers/base/dd.c                             | 16 +++++++++++++
> > > >  2 files changed, 40 insertions(+)
> > > >  create mode 100644 Documentation/ABI/testing/sysfs-devices-state_synced
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-devices-state_synced b/Documentation/ABI/testing/sysfs-devices-state_synced
> > > > new file mode 100644
> > > > index 000000000000..0c922d7d02fc
> > > > --- /dev/null
> > > > +++ b/Documentation/ABI/testing/sysfs-devices-state_synced
> > > > @@ -0,0 +1,24 @@
> > > > +What:                /sys/devices/.../state_synced
> > > > +Date:                May 2020
> > > > +Contact:     Saravana Kannan <saravanak@google.com>
> > > > +Description:
> > > > +             The /sys/devices/.../state_synced attribute is only present for
> > > > +             devices whose bus types or driver provides the .sync_state()
> > > > +             callback. The number read from it (0 or 1) reflects the value
> > > > +             of the device's 'state_synced' field. A value of 0 means the
> > > > +             .sync_state() callback hasn't been called yet. A value of 1
> > > > +             means the .sync_state() callback has been called.
> > > > +
> > > > +             Generally, if a device has sync_state() support and has some of
> > > > +             the resources it provides enabled at the time the kernel starts
> > > > +             (Eg: enabled by hardware reset or bootloader or anything that
> > > > +             run before the kernel starts), then it'll keep those resources
> > > > +             enabled and in a state that's compatible with the state they
> > > > +             were in at the start of the kernel. The device will stop doing
> > > > +             this only when the sync_state() callback has been called --
> > > > +             which happens only when all its consumer devices are registered
> > > > +             and have probed successfully. Resources that were left disabled
> > > > +             at the time the kernel starts are not affected or limited in
> > > > +             any way by sync_state() callbacks.
> > > > +
> > > > +
> > > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > > > index 48ca81cb8ebc..72599436ae84 100644
> > > > --- a/drivers/base/dd.c
> > > > +++ b/drivers/base/dd.c
> > > > @@ -458,6 +458,13 @@ static void driver_deferred_probe_add_trigger(struct device *dev,
> > > >               driver_deferred_probe_trigger();
> > > >  }
> > > >
> > > > +static ssize_t state_synced_show(struct device *dev,
> > > > +                              struct device_attribute *attr, char *buf)
> > > > +{
> > > > +     return sprintf(buf, "%u\n", dev->state_synced);
> > > > +}
> > > > +static DEVICE_ATTR_RO(state_synced);
> > > > +
> > > >  static int really_probe(struct device *dev, struct device_driver *drv)
> > > >  {
> > > >       int ret = -EPROBE_DEFER;
> > > > @@ -531,9 +538,16 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> > > >               goto dev_groups_failed;
> > > >       }
> > > >
> > > > +     if (dev_has_sync_state(dev) &&
> > > > +         device_create_file(dev, &dev_attr_state_synced)) {
> > > > +             dev_err(dev, "state_synced sysfs add failed\n");
> > > > +             goto dev_sysfs_state_synced_failed;
> > > > +     }
> > >
> > > Why not add this to the groups above this and only enable it if needed
> > > at runtime?
> >
> > Those groups above seem to be driver specific groups. Looking at the
> > code, some drivers seem to be setting them. Also, this attribute can
> > only be decided after a driver has successfully bound to the device
> > because dev_has_sync_state() has to check the bus and the driver for
> > sync_state() support.
> >
> > > The is_visible() callback should be what you need to use here.
> >
> > If this is an attribute specific property, it might work. I'll take a look.
> 
> I took a look at is_visible(). It only makes sense for a group of
> attributes that are exposed in a sub directory. But state_synced is a
> top level property IMO. What I'm doing is similar to the "online"
> attribute that's exposed for a device.

No, no need for a subdirectory for an attribute group.  If you don't
have a name for the group, the files will be placed in the device's
directory.

> And even if I create a group with one lone attribute, I'll still have
> to add it after the device has probed or have to call
> sysfs_update_groups() after the device is probed to update the
> visibility. Seems quite messy and unnecessarily complicated.

Ah, ok, that is messier.

> I'd like to keep this an attribute and not create a group for that
> reason. Does that sound okay to you? I'll obviously still do the clean
> up correctly.

Ok, that's fine, we can change it later if we figure it out.

thanks,

greg k-h