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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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