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.
