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

[Saravana Kannan]

On Tue, Feb 28, 2023 at 2:33 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> 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"?

Fine. I'll make it "1" to be consistent with the read behavior.

>
>
> > +
> >
> > 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...

I'm following the style of every other function in the .h file.

>
> > +{
> > +       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.

Heh... I took the time to make sure it was signed... but looks like I
accidentally followed the other typdefs. Also the -EINVAL actually
worked when I tried writing
to a file that already had "1". I guess since the return value is
signed, it worked out. But, I'll fix it.



-Saravana