1. Patchew
  2. linux
  3. [PATCH v1 0/2] Give more control of sync_state()
  4. View patch

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

Saravana Kannan posted 2 patches 2 years, 11 months ago
There is a newer version of this series
[PATCH v1 2/2] driver core: Make state_synced device attribute writeable
Posted by Saravana Kannan 2 years, 11 months ago 
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@google.com>
---
 .../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.
+
 
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)
+{
+	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;
+	}
+	device_unlock(dev);
+
+	return count;
+}
+
 static ssize_t state_synced_show(struct device *dev,
 				 struct device_attribute *attr, char *buf)
 {
@@ -521,7 +537,7 @@ static ssize_t state_synced_show(struct device *dev,
 
 	return sysfs_emit(buf, "%u\n", val);
 }
-static DEVICE_ATTR_RO(state_synced);
+static DEVICE_ATTR_RW(state_synced);
 
 static void device_unbind_cleanup(struct device *dev)
 {
-- 
2.39.2.637.g21b0678d19-goog
Re: [PATCH v1 2/2] driver core: Make state_synced device attribute writeable
Posted by Doug Anderson 2 years, 11 months ago 
Hi,

On Thu, Feb 23, 2023 at 11:05 PM Saravana Kannan <saravanak@google.com> 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@google.com>
> ---
>  .../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.
Re: [PATCH v1 2/2] driver core: Make state_synced device attribute writeable
Posted by Saravana Kannan 2 years, 11 months ago 
On Tue, Feb 28, 2023 at 2:33 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Feb 23, 2023 at 11:05 PM Saravana Kannan <saravanak@google.com> 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@google.com>
> > ---
> >  .../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
Re: [PATCH v1 2/2] driver core: Make state_synced device attribute writeable
Posted by Doug Anderson 2 years, 11 months ago 
Hi,

On Fri, Mar 3, 2023 at 4:53 PM Saravana Kannan <saravanak@google.com> wrote:
>
> > > --- 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.

Right, that's why I suggested moving it out of the .h file. I see
plenty of non-inline function definitions in the header file.

-Doug

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.