[PATCH v3 02/13] driver core: Enable suppliers to implement fine grained sync_state support
Ulf Hansson
ulf.hansson at linaro.org
Mon May 11 02:42:51 PDT 2026
On Mon, 11 May 2026 at 07:09, Saravana Kannan <saravanak at kernel.org> wrote:
>
> On Fri, May 8, 2026 at 5:39 AM Ulf Hansson <ulf.hansson at linaro.org> wrote:
> >
> > The common sync_state support isn't fine grained enough for some types of
> > suppliers, like power domains for example. Especially when a supplier
> > provides multiple independent power domains, each with their own set of
> > consumers. In these cases we need to wait for all consumers for all the
> > provided power domains before invoking the supplier's ->sync_state().
> >
> > To allow a more fine grained sync_state support to be implemented on per
> > supplier's driver basis, let's add a new optional callback. As soon as
> > there is an update worth to consider in regards to managing sync_state for
> > a supplier device, __device_links_queue_sync_state() queues the device in a
> > list, allowing the new callback to be invoked when flushing the list in
> > device_links_flush_sync_list().
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson at linaro.org>
> > ---
> >
> > Changes in v3:
> > - Re-worked the approach to use a list to queue/flush devices for
> > ->queue_sync_state(). This should make sure the device lock is being
> > held when it's needed, as pointed out by Danilo.
> >
>
> Hi Ulf,
>
> Thanks for working on this!
>
> And please bear with my slow replies.
I will try, but taking more than 2 months to reply isn't sustainable,
I think. Let's hope you can get some more bandwidth for reviews when
moving forward.
>
> > ---
> > drivers/base/base.h | 18 ++++++++
> > drivers/base/core.c | 77 ++++++++++++++++++++++++++---------
> > drivers/base/driver.c | 7 ++++
> > include/linux/device.h | 2 +
> > include/linux/device/driver.h | 7 ++++
> > 5 files changed, 91 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/base/base.h b/drivers/base/base.h
> > index 30b416588617..c8be24af92c3 100644
> > --- a/drivers/base/base.h
> > +++ b/drivers/base/base.h
> > @@ -196,6 +196,24 @@ static inline void dev_sync_state(struct device *dev)
> > dev->driver->sync_state(dev);
> > }
> >
> > +static inline bool dev_has_queue_sync_state(struct device *dev)
>
> Let's please pick a better name. This is too similar to the actual
> queue function you call __device_links_queue_sync_state() and is very
> confusing. Maybe something that has a meaning along the lines of
> "another consumer probed". So, maybe:
> * consumer_probed()
> * change_of_active_consumers()
The whole point of naming it "queue_sync_state" was exactly to refer
to __device_links_queue_sync_state(). The point is, the callback can't
be invoked unless __device_links_queue_sync_state() has been called
for the device first.
Not sure why you think that is confusing? To me, that is rather the
opposite. :-)
Before deciding on another name, note also that
__device_links_queue_sync_state() is called when resuming sync_state
from device_links_supplier_sync_state_resume() and from
device_links_driver_bound(). I am not sure "consumer_probed" a good
name that covers both of these cases; what do you think?
>
> I'm going to refer to this new callback as "consumer_probed()" in the
> rest of my replies so I can keep it straight in my head.
>
> > +{
> > + struct device_driver *drv;
> > +
> > + if (!dev)
> > + return false;
> > + drv = READ_ONCE(dev->driver);
> > + if (drv && drv->queue_sync_state)
> > + return true;
> > + return false;
> > +}
> > +
> > +static inline void dev_queue_sync_state(struct device *dev)
> > +{
> > + if (dev->driver && dev->driver->queue_sync_state)
> > + dev->driver->queue_sync_state(dev);
> > +}
> > +
> > int driver_add_groups(const struct device_driver *drv, const struct attribute_group **groups);
> > void driver_remove_groups(const struct device_driver *drv, const struct attribute_group **groups);
> > void device_driver_detach(struct device *dev);
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index d49420e066de..f1f95b3c81e5 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -1101,15 +1101,18 @@ int device_links_check_suppliers(struct device *dev)
> > /**
> > * __device_links_queue_sync_state - Queue a device for sync_state() callback
> > * @dev: Device to call sync_state() on
> > - * @list: List head to queue the @dev on
> > + * @s_list: List head for the sync_state to queue the @dev on
> > + * @q_list: List head for the queue_sync_state to queue the @dev on
> > *
> > * Queues a device for a sync_state() callback when the device links write lock
> > * isn't held. This allows the sync_state() execution flow to use device links
> > * APIs. The caller must ensure this function is called with
> > - * device_links_write_lock() held.
> > + * device_links_write_lock() held. Note, if the optional queue_sync_state()
> > + * callback has been assigned too, the device is queued for that list to allow a
> > + * more fine grained support to be implemented on per supplier basis.
>
> Why not keep this in the same format as the existing doc? Something like:
> If option list xxx is provided, queues the device for the
> consumer_probed() callback.
Okay, let me clarify this.
>
> > *
> > * This function does a get_device() to make sure the device is not freed while
> > - * on this list.
> > + * on the corresponding list.
> > *
> > * So the caller must also ensure that device_links_flush_sync_list() is called
> > * as soon as the caller releases device_links_write_lock(). This is necessary
> > @@ -1117,7 +1120,8 @@ int device_links_check_suppliers(struct device *dev)
> > * put_device() is called on this device.
> > */
> > static void __device_links_queue_sync_state(struct device *dev,
> > - struct list_head *list)
> > + struct list_head *s_list,
> > + struct list_head *q_list)
> > {
> > struct device_link *link;
> >
> > @@ -1129,8 +1133,14 @@ static void __device_links_queue_sync_state(struct device *dev,
> > list_for_each_entry(link, &dev->links.consumers, s_node) {
> > if (!device_link_test(link, DL_FLAG_MANAGED))
> > continue;
> > - if (link->status != DL_STATE_ACTIVE)
> > + if (link->status != DL_STATE_ACTIVE) {
> > + if (dev_has_queue_sync_state(dev) &&
> > + list_empty(&dev->links.queue_sync)) {
> > + get_device(dev);
> > + list_add_tail(&dev->links.queue_sync, q_list);
> > + }
> > return;
> > + }
> > }
> >
> > /*
> > @@ -1144,25 +1154,28 @@ static void __device_links_queue_sync_state(struct device *dev,
> > return;
> >
> > get_device(dev);
> > - list_add_tail(&dev->links.defer_sync, list);
> > + list_add_tail(&dev->links.defer_sync, s_list);
> > }
> >
> > /**
> > - * device_links_flush_sync_list - Call sync_state() on a list of devices
> > - * @list: List of devices to call sync_state() on
> > + * device_links_flush_sync_list - Call sync_state callbacks for the devices
> > + * @s_list: List of devices to call sync_state() on
> > + * @q_list: List of devices to call queue_sync_state() on
> > * @dont_lock_dev: Device for which lock is already held by the caller
> > *
> > - * Calls sync_state() on all the devices that have been queued for it. This
> > - * function is used in conjunction with __device_links_queue_sync_state(). The
> > - * @dont_lock_dev parameter is useful when this function is called from a
> > - * context where a device lock is already held.
> > + * Calls sync_state() and queue_sync_state() on all the devices that have been
> > + * queued for it. This function is used in conjunction with
> > + * __device_links_queue_sync_state(). The @dont_lock_dev parameter is useful
> > + * when this function is called from a context where a device lock is already
> > + * held.
> > */
> > -static void device_links_flush_sync_list(struct list_head *list,
> > +static void device_links_flush_sync_list(struct list_head *s_list,
> > + struct list_head *q_list,
> > struct device *dont_lock_dev)
> > {
> > struct device *dev, *tmp;
> >
> > - list_for_each_entry_safe(dev, tmp, list, links.defer_sync) {
> > + list_for_each_entry_safe(dev, tmp, s_list, links.defer_sync) {
> > list_del_init(&dev->links.defer_sync);
> >
> > if (dev != dont_lock_dev)
> > @@ -1175,6 +1188,25 @@ static void device_links_flush_sync_list(struct list_head *list,
> >
> > put_device(dev);
> > }
> > +
> > + if (!q_list)
> > + return;
> > +
> > + list_for_each_entry_safe(dev, tmp, q_list, links.queue_sync) {
> > + list_del_init(&dev->links.queue_sync);
> > +
> > + if (dev != dont_lock_dev)
> > + device_lock(dev);
> > +
> > + device_links_write_lock();
> > + dev_queue_sync_state(dev);
> > + device_links_write_unlock();
> > +
> > + if (dev != dont_lock_dev)
> > + device_unlock(dev);
> > +
> > + put_device(dev);
> > + }
> > }
> >
> > void device_links_supplier_sync_state_pause(void)
> > @@ -1188,6 +1220,7 @@ void device_links_supplier_sync_state_resume(void)
> > {
> > struct device *dev, *tmp;
> > LIST_HEAD(sync_list);
> > + LIST_HEAD(queue_list);
> >
> > device_links_write_lock();
> > if (!defer_sync_state_count) {
> > @@ -1204,12 +1237,12 @@ void device_links_supplier_sync_state_resume(void)
> > * sync_list because defer_sync is used for both lists.
> > */
> > list_del_init(&dev->links.defer_sync);
> > - __device_links_queue_sync_state(dev, &sync_list);
> > + __device_links_queue_sync_state(dev, &sync_list, &queue_list);
> > }
> > out:
> > device_links_write_unlock();
> >
> > - device_links_flush_sync_list(&sync_list, NULL);
> > + device_links_flush_sync_list(&sync_list, &queue_list, NULL);
> > }
> >
> > static int sync_state_resume_initcall(void)
> > @@ -1296,6 +1329,7 @@ void device_links_driver_bound(struct device *dev)
> > {
> > struct device_link *link, *ln;
> > LIST_HEAD(sync_list);
> > + LIST_HEAD(queue_list);
> >
> > /*
> > * If a device binds successfully, it's expected to have created all
> > @@ -1351,7 +1385,7 @@ void device_links_driver_bound(struct device *dev)
> > if (defer_sync_state_count)
> > __device_links_supplier_defer_sync(dev);
> > else
> > - __device_links_queue_sync_state(dev, &sync_list);
> > + __device_links_queue_sync_state(dev, &sync_list, &queue_list);
>
> This path is really meant only for situations when the device has no
> consumers. So, I don't think fine grained control is relevant.
Okay, makes sense!
>
> >
> > list_for_each_entry_safe(link, ln, &dev->links.suppliers, c_node) {
> > struct device *supplier;
> > @@ -1393,14 +1427,15 @@ void device_links_driver_bound(struct device *dev)
> > if (defer_sync_state_count)
> > __device_links_supplier_defer_sync(supplier);
> > else
> > - __device_links_queue_sync_state(supplier, &sync_list);
> > + __device_links_queue_sync_state(supplier, &sync_list,
> > + &queue_list);
> > }
> >
> > dev->links.status = DL_DEV_DRIVER_BOUND;
> >
> > device_links_write_unlock();
> >
> > - device_links_flush_sync_list(&sync_list, dev);
> > + device_links_flush_sync_list(&sync_list, &queue_list, dev);
> > }
> >
> > /**
> > @@ -1516,6 +1551,7 @@ void device_links_driver_cleanup(struct device *dev)
> > }
> >
> > list_del_init(&dev->links.defer_sync);
> > + list_del_init(&dev->links.queue_sync);
> > __device_links_no_driver(dev);
> >
> > device_links_write_unlock();
> > @@ -1808,7 +1844,7 @@ void fw_devlink_probing_done(void)
> > class_for_each_device(&devlink_class, NULL, &sync_list,
> > fw_devlink_dev_sync_state);
> > device_links_write_unlock();
> > - device_links_flush_sync_list(&sync_list, NULL);
> > + device_links_flush_sync_list(&sync_list, NULL, NULL);
> > }
> >
> > /**
> > @@ -3169,6 +3205,7 @@ void device_initialize(struct device *dev)
> > INIT_LIST_HEAD(&dev->links.consumers);
> > INIT_LIST_HEAD(&dev->links.suppliers);
> > INIT_LIST_HEAD(&dev->links.defer_sync);
> > + INIT_LIST_HEAD(&dev->links.queue_sync);
> > dev->links.status = DL_DEV_NO_DRIVER;
> > dev_assign_dma_coherent(dev, dma_default_coherent);
> > swiotlb_dev_init(dev);
> > diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> > index 8ab010ddf709..b8f4d08bbd58 100644
> > --- a/drivers/base/driver.c
> > +++ b/drivers/base/driver.c
> > @@ -239,6 +239,13 @@ int driver_register(struct device_driver *drv)
> > pr_warn("Driver '%s' needs updating - please use "
> > "bus_type methods\n", drv->name);
> >
> > + if (drv->queue_sync_state && !drv->sync_state &&
> > + !drv->bus->sync_state) {
> > + pr_err("Driver '%s' or its bus_type needs ->sync_state()",
> > + drv->name);
> > + return -EINVAL;
> > + }
> > +
> > other = driver_find(drv->name, drv->bus);
> > if (other) {
> > pr_err("Error: Driver '%s' is already registered, "
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 56a96e41d2c9..6848b0a2c2d9 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -414,12 +414,14 @@ enum device_removable {
> > * @suppliers: List of links to supplier devices.
> > * @consumers: List of links to consumer devices.
> > * @defer_sync: Hook to global list of devices that have deferred sync_state.
> > + * @defer_sync: Hook to global list of devices scheduled for queue_sync_state.
> > * @status: Driver status information.
> > */
> > struct dev_links_info {
> > struct list_head suppliers;
> > struct list_head consumers;
> > struct list_head defer_sync;
> > + struct list_head queue_sync;
> > enum dl_dev_state status;
> > };
> >
> > diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
> > index bbc67ec513ed..bc9ae1cbe03c 100644
> > --- a/include/linux/device/driver.h
> > +++ b/include/linux/device/driver.h
> > @@ -68,6 +68,12 @@ enum probe_type {
> > * be called at late_initcall_sync level. If the device has
> > * consumers that are never bound to a driver, this function
> > * will never get called until they do.
> > + * @queue_sync_state: Similar to the ->sync_state() callback, but called to
> > + * allow syncing device state to software state in a more fine
> > + * grained way. It is called when there is an updated state that
> > + * may be worth to consider for any of the consumers linked to
> > + * this device. If implemented, the ->sync_state() callback is
> > + * required too.
>
> Let's make it clear that if a device gets sync_state() callback, it
> needs to sync the state independent of whether it has received any/all
> "consumer_probed()".
Right, good point! Let me clarify that!
>
> Thanks,
> Saravana
>
>
> > * @remove: Called when the device is removed from the system to
> > * unbind a device from this driver.
> > * @shutdown: Called at shut-down time to quiesce the device.
> > @@ -110,6 +116,7 @@ struct device_driver {
> >
> > int (*probe) (struct device *dev);
> > void (*sync_state)(struct device *dev);
> > + void (*queue_sync_state)(struct device *dev);
> > int (*remove) (struct device *dev);
> > void (*shutdown) (struct device *dev);
> > int (*suspend) (struct device *dev, pm_message_t state);
> > --
> > 2.43.0
> >
Kind regards
Uffe
More information about the linux-arm-kernel
mailing list