Re: [PATCH v1 2/2] driver core: Make state_synced device attribute writeable

[Doug Anderson]

Hi,

On Thu, Feb 23, 2023 at 11:05 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> If the file is written to and sync_state() hasn't been called for the
> device yet, then call sync_state() for the device independent of the
> state of its consumers.
>
> This is useful for supplier devices that have one or more consumers that
> don't have a driver but the consumers are in a state that don't use the
> resources supplied by the supplier device.
>
> This gives finer grained control than using the
> fw_devlink.sync_state=timeout kernel commandline parameter.
>
> Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> ---
>  .../ABI/testing/sysfs-devices-state_synced     |  5 +++++
>  drivers/base/base.h                            |  8 ++++++++
>  drivers/base/core.c                            |  5 +----
>  drivers/base/dd.c                              | 18 +++++++++++++++++-
>  4 files changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-state_synced b/Documentation/ABI/testing/sysfs-devices-state_synced
> index 0c922d7d02fc..cc4090c9df75 100644
> --- a/Documentation/ABI/testing/sysfs-devices-state_synced
> +++ b/Documentation/ABI/testing/sysfs-devices-state_synced
> @@ -21,4 +21,9 @@ Description:
>                 at the time the kernel starts are not affected or limited in
>                 any way by sync_state() callbacks.
>
> +               Writing anything to this file will force a call to the device's
> +               sync_state() function if it hasn't been called already. The
> +               sync_state() call happens is independent of the state of the
> +               consumer devices.

Please don't just accept anything written. It doesn't take much to
check that the user wrote some known value here and then if we ever
have a reason to allow something else we don't have to break old ABIs.
Maybe "-1"?


> +
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 6fcd71803d35..b055eba1ec30 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -164,6 +164,14 @@ static inline int driver_match_device(struct device_driver *drv,
>         return drv->bus->match ? drv->bus->match(dev, drv) : 1;
>  }
>
> +static inline void dev_sync_state(struct device *dev)

IMO don't force inline. The compiler is probably smarter than you. I
could even believe that it might be more optimal for this rarely
called function to be _not_ inline if it kept the kernel smaller. I
guess that means moving it out of the header...

> +{
> +       if (dev->bus->sync_state)
> +               dev->bus->sync_state(dev);
> +       else if (dev->driver && dev->driver->sync_state)
> +               dev->driver->sync_state(dev);
> +}
> +
>  extern int driver_add_groups(struct device_driver *drv,
>                              const struct attribute_group **groups);
>  extern void driver_remove_groups(struct device_driver *drv,
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 929ec218f180..60bb3551977b 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1215,10 +1215,7 @@ static void device_links_flush_sync_list(struct list_head *list,
>                 if (dev != dont_lock_dev)
>                         device_lock(dev);
>
> -               if (dev->bus->sync_state)
> -                       dev->bus->sync_state(dev);
> -               else if (dev->driver && dev->driver->sync_state)
> -                       dev->driver->sync_state(dev);
> +               dev_sync_state(dev);
>
>                 if (dev != dont_lock_dev)
>                         device_unlock(dev);
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 84f07e0050dd..17b51573f794 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -510,6 +510,22 @@ EXPORT_SYMBOL_GPL(device_bind_driver);
>  static atomic_t probe_count = ATOMIC_INIT(0);
>  static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
>
> +static ssize_t state_synced_store(struct device *dev,
> +                                 struct device_attribute *attr,
> +                                 const char *buf, size_t count)
> +{
> +       device_lock(dev);
> +       if (!dev->state_synced) {
> +               dev->state_synced = true;
> +               dev_sync_state(dev);
> +       } else {
> +               count = -EINVAL;

count is of type "size_t", not "ssize_t". -EINVAL is signed.