[PATCH v3 00/13] driver core / pmdomain: Add support for fined grained sync_state

[PATCH v3 00/13] driver core / pmdomain: Add support for fined grained sync_state

[Ulf Hansson]

Changes in v3:
	- Dropped tested-by tags from Geert, due to changes in patch2.
	- Addressed the comments from Danilo and Geert, more information in the
	changelog for each patch.

Assuming this is accepted, the best merge strategy would be for Danilo to pick
up the driver core patches (1 to 4) and host a branch that I can pull into my
pmdomain tree so I can take the rest of the series. Danilo, please let me know
what you prefer.

Since the introduction [1] of the common sync_state support for pmdomains
(genpd), we have encountered a lot of various interesting problems. In most
cases the new behaviour of genpd triggered some weird platform specific bugs.

That said, in LPC in Tokyo me and Saravana hosted a session to walk through the
remaining limitations that we have found for genpd's sync state support. In
particular, we discussed the problems we have for the so-called onecell power
domain providers, where a single provider typically provides multiple
independent power domains, all with their own set of consumers.

Note that, onecell power domain providers are very common. It's being used by
many SoCs/platforms/technologies. To name a few:
SCMI, Qualcomm, NXP, Mediatek, Renesas, TI, etc.

Anyway, in these cases, the generic sync_state mechanism with fw_devlink isn't
fine grained enough, as we end up waiting for all consumers for all power
domains before the ->sync_callback gets called for the supplier/provider. In
other words, we may end up keeping unused power domains powered-on, for no good
reasons.

The series intends to fix this problem. Please have a look at the commit
messages for more details and help review/test!

Kind regards
Ulf Hansson

[1]
https://lore.kernel.org/all/20250701114733.636510-1-ulf.hansson@linaro.org/


Ulf Hansson (13):
  Revert "driver core: move dev_has_sync_state() to drivers/base/base.h"
  driver core: Enable suppliers to implement fine grained sync_state
    support
  driver core: Add documentation for dev_set_drv_sync_state()
  driver core: Add dev_set_drv_queue_sync_state()
  pmdomain: core: Move genpd_get_from_provider()
  pmdomain: core: Add initial fine grained sync_state support
  pmdomain: core: Extend fine grained sync_state to more onecell
    providers
  pmdomain: core: Export a common function for ->queue_sync_state()
  pmdomain: renesas: rcar-gen4-sysc: Drop GENPD_FLAG_NO_STAY_ON
  pmdomain: renesas: rcar-sysc: Drop GENPD_FLAG_NO_STAY_ON
  pmdomain: renesas: rmobile-sysc: Drop GENPD_FLAG_NO_STAY_ON
  pmdomain: core: Avoid an unnecessary power off at sync_state
  pmdomain: core: Add a couple of debug prints for sync_state

 drivers/base/base.h                       |  22 ++-
 drivers/base/core.c                       |  77 ++++++--
 drivers/base/driver.c                     |   7 +
 drivers/pmdomain/core.c                   | 227 ++++++++++++++++++----
 drivers/pmdomain/renesas/rcar-gen4-sysc.c |   1 -
 drivers/pmdomain/renesas/rcar-sysc.c      |   1 -
 drivers/pmdomain/renesas/rmobile-sysc.c   |   3 +-
 include/linux/device.h                    |  44 +++++
 include/linux/device/driver.h             |   7 +
 include/linux/pm_domain.h                 |   3 +
 10 files changed, 318 insertions(+), 74 deletions(-)

-- 
2.43.0

[PATCH v3 01/13] Revert "driver core: move dev_has_sync_state() to drivers/base/base.h"

[Ulf Hansson]

It turns out that the dev_has_sync_state() is needed outside the driver
core. A subsequent change to the pmdomain subsystem starts making use of
it.

Fixes: 9db268212e0d ("driver core: move dev_has_sync_state() to drivers/base/base.h")
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v3:
	- New patch.

---
 drivers/base/base.h    | 14 --------------
 include/linux/device.h | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 0ed1e278b957..30b416588617 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -188,20 +188,6 @@ static inline int driver_match_device(const struct device_driver *drv,
 	return drv->bus->match ? drv->bus->match(dev, drv) : 1;
 }
 
-static inline bool dev_has_sync_state(struct device *dev)
-{
-	struct device_driver *drv;
-
-	if (!dev)
-		return false;
-	drv = READ_ONCE(dev->driver);
-	if (drv && drv->sync_state)
-		return true;
-	if (dev->bus && dev->bus->sync_state)
-		return true;
-	return false;
-}
-
 static inline void dev_sync_state(struct device *dev)
 {
 	if (dev->bus->sync_state)
diff --git a/include/linux/device.h b/include/linux/device.h
index d54c86d77764..56a96e41d2c9 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1061,6 +1061,20 @@ static inline void device_lock_assert(struct device *dev)
 	lockdep_assert_held(&dev->mutex);
 }
 
+static inline bool dev_has_sync_state(struct device *dev)
+{
+	struct device_driver *drv;
+
+	if (!dev)
+		return false;
+	drv = READ_ONCE(dev->driver);
+	if (drv && drv->sync_state)
+		return true;
+	if (dev->bus && dev->bus->sync_state)
+		return true;
+	return false;
+}
+
 static inline int dev_set_drv_sync_state(struct device *dev,
 					 void (*fn)(struct device *dev))
 {
-- 
2.43.0

[PATCH v3 02/13] driver core: Enable suppliers to implement fine grained sync_state support

[Ulf Hansson]

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

---
 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)
+{
+	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.
  *
  * 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);
 
 	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.
  * @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

[PATCH v3 03/13] driver core: Add documentation for dev_set_drv_sync_state()

[Ulf Hansson]

As dev_set_drv_sync_state() is an exported function, let's add some
documentation of it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v3:
	- New patch.

---
 include/linux/device.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 6848b0a2c2d9..209feea8050e 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1077,6 +1077,14 @@ static inline bool dev_has_sync_state(struct device *dev)
 	return false;
 }
 
+/**
+ * dev_set_drv_sync_state - Assign the ->sync_state callback for a device.
+ * @dev: The device.
+ * @fn: The callback.
+ *
+ * This function dynamically tries to assign the driver's ->sync_state()
+ * callback for the corresponding @dev.
+ */
 static inline int dev_set_drv_sync_state(struct device *dev,
 					 void (*fn)(struct device *dev))
 {
-- 
2.43.0

[PATCH v3 04/13] driver core: Add dev_set_drv_queue_sync_state()

[Ulf Hansson]

Similar to the dev_set_drv_sync_state() helper, let's add another one to
allow subsystem level code to set the ->queue_sync_state() callback for a
driver that has not already set it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v3:
	- Added documentation.

---
 include/linux/device.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 209feea8050e..e32f85e8d80c 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1097,6 +1097,26 @@ static inline int dev_set_drv_sync_state(struct device *dev,
 	return 0;
 }
 
+/**
+ * dev_set_drv_sync_state - Assign the ->queue_sync_state callback for a device.
+ * @dev: The device.
+ * @fn: The callback.
+ *
+ * This function dynamically tries to assign the driver's ->queue_sync_state()
+ * callback for the corresponding @dev.
+ */
+static inline int dev_set_drv_queue_sync_state(struct device *dev,
+					       void (*fn)(struct device *dev))
+{
+	if (!dev || !dev->driver)
+		return 0;
+	if (dev->driver->queue_sync_state && dev->driver->queue_sync_state != fn)
+		return -EBUSY;
+	if (!dev->driver->queue_sync_state)
+		dev->driver->queue_sync_state = fn;
+	return 0;
+}
+
 static inline void dev_set_removable(struct device *dev,
 				     enum device_removable removable)
 {
-- 
2.43.0

[PATCH v3 05/13] pmdomain: core: Move genpd_get_from_provider()

[Ulf Hansson]

To prepare for subsequent changes and to avoid an unnecessary function
declaration, let's move genpd_get_from_provider() a bit earlier in the
code.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v3:
	- No change.

---
 drivers/pmdomain/core.c | 70 ++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index 4d32fc676aaf..ad57846f02a3 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -2664,6 +2664,41 @@ static bool genpd_present(const struct generic_pm_domain *genpd)
 	return ret;
 }
 
+/**
+ * genpd_get_from_provider() - Look-up PM domain
+ * @genpdspec: OF phandle args to use for look-up
+ *
+ * Looks for a PM domain provider under the node specified by @genpdspec and if
+ * found, uses xlate function of the provider to map phandle args to a PM
+ * domain.
+ *
+ * Returns a valid pointer to struct generic_pm_domain on success or ERR_PTR()
+ * on failure.
+ */
+static struct generic_pm_domain *genpd_get_from_provider(
+					const struct of_phandle_args *genpdspec)
+{
+	struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
+	struct of_genpd_provider *provider;
+
+	if (!genpdspec)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&of_genpd_mutex);
+
+	/* Check if we have such a provider in our array */
+	list_for_each_entry(provider, &of_genpd_providers, link) {
+		if (provider->node == genpdspec->np)
+			genpd = provider->xlate(genpdspec, provider->data);
+		if (!IS_ERR(genpd))
+			break;
+	}
+
+	mutex_unlock(&of_genpd_mutex);
+
+	return genpd;
+}
+
 static void genpd_sync_state(struct device *dev)
 {
 	return of_genpd_sync_state(dev->of_node);
@@ -2889,41 +2924,6 @@ void of_genpd_del_provider(struct device_node *np)
 }
 EXPORT_SYMBOL_GPL(of_genpd_del_provider);
 
-/**
- * genpd_get_from_provider() - Look-up PM domain
- * @genpdspec: OF phandle args to use for look-up
- *
- * Looks for a PM domain provider under the node specified by @genpdspec and if
- * found, uses xlate function of the provider to map phandle args to a PM
- * domain.
- *
- * Returns a valid pointer to struct generic_pm_domain on success or ERR_PTR()
- * on failure.
- */
-static struct generic_pm_domain *genpd_get_from_provider(
-					const struct of_phandle_args *genpdspec)
-{
-	struct generic_pm_domain *genpd = ERR_PTR(-ENOENT);
-	struct of_genpd_provider *provider;
-
-	if (!genpdspec)
-		return ERR_PTR(-EINVAL);
-
-	mutex_lock(&of_genpd_mutex);
-
-	/* Check if we have such a provider in our array */
-	list_for_each_entry(provider, &of_genpd_providers, link) {
-		if (provider->node == genpdspec->np)
-			genpd = provider->xlate(genpdspec, provider->data);
-		if (!IS_ERR(genpd))
-			break;
-	}
-
-	mutex_unlock(&of_genpd_mutex);
-
-	return genpd;
-}
-
 /**
  * of_genpd_add_device() - Add a device to an I/O PM domain
  * @genpdspec: OF phandle args to use for look-up PM domain
-- 
2.43.0

[PATCH v3 06/13] pmdomain: core: Add initial fine grained sync_state support

[Ulf Hansson]

A onecell (#power-domain-cells = <1 or 2>; in DT) power domain provider
typically provides multiple independent power domains, each with their own
corresponding consumers. In these cases we have to wait for all consumers
for all the provided power domains before the ->sync_state() callback gets
called for the supplier.

In a first step to improve this, let's implement support for fine grained
sync_state support a per genpd basis by using the ->queue_sync_state()
callback. To take step by step, let's initially limit the improvement to
the internal genpd provider driver and to its corresponding genpd devices
for onecell providers.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v3:
	- Addressed some cosmetic comments from Geert.

---
 drivers/pmdomain/core.c   | 124 ++++++++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h |   1 +
 2 files changed, 125 insertions(+)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index ad57846f02a3..c01a9a96e5c2 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -2699,6 +2699,119 @@ static struct generic_pm_domain *genpd_get_from_provider(
 	return genpd;
 }
 
+static bool genpd_should_wait_for_consumer(struct device_node *np)
+{
+	struct generic_pm_domain *genpd;
+	bool should_wait = false;
+
+	mutex_lock(&gpd_list_lock);
+	list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
+		if (genpd->provider == of_fwnode_handle(np)) {
+			genpd_lock(genpd);
+
+			/* Clear the previous state before reevaluating. */
+			genpd->wait_for_consumer = false;
+
+			/*
+			 * Unless there is at least one genpd for the provider
+			 * that is being kept powered-on, we don't have to care
+			 * about waiting for consumers.
+			 */
+			if (genpd->stay_on)
+				should_wait = true;
+
+			genpd_unlock(genpd);
+		}
+	}
+	mutex_unlock(&gpd_list_lock);
+
+	return should_wait;
+}
+
+static void genpd_parse_for_consumer(struct device_node *sup,
+				     struct device_node *con)
+{
+	struct generic_pm_domain *genpd;
+
+	for (unsigned int i = 0; ; i++) {
+		struct of_phandle_args pd_args;
+
+		if (of_parse_phandle_with_args(con, "power-domains",
+					       "#power-domain-cells",
+					       i, &pd_args))
+			break;
+
+		/*
+		 * The phandle must correspond to the supplier's genpd provider
+		 * to be relevant else let's move to the next index.
+		 */
+		if (sup != pd_args.np) {
+			of_node_put(pd_args.np);
+			continue;
+		}
+
+		mutex_lock(&gpd_list_lock);
+		genpd = genpd_get_from_provider(&pd_args);
+		if (!IS_ERR(genpd)) {
+			genpd_lock(genpd);
+			genpd->wait_for_consumer = true;
+			genpd_unlock(genpd);
+		}
+		mutex_unlock(&gpd_list_lock);
+
+		of_node_put(pd_args.np);
+	}
+}
+
+static void _genpd_queue_sync_state(struct device_node *np)
+{
+	struct generic_pm_domain *genpd;
+
+	mutex_lock(&gpd_list_lock);
+	list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
+		if (genpd->provider == of_fwnode_handle(np)) {
+			genpd_lock(genpd);
+			if (genpd->stay_on && !genpd->wait_for_consumer) {
+				genpd->stay_on = false;
+				genpd_queue_power_off_work(genpd);
+			}
+			genpd_unlock(genpd);
+		}
+	}
+	mutex_unlock(&gpd_list_lock);
+}
+
+static void genpd_queue_sync_state(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct device_link *link;
+
+	if (!genpd_should_wait_for_consumer(np))
+		return;
+
+	list_for_each_entry(link, &dev->links.consumers, s_node) {
+		struct device *consumer = link->consumer;
+
+		if (!device_link_test(link, DL_FLAG_MANAGED))
+			continue;
+
+		if (link->status == DL_STATE_ACTIVE)
+			continue;
+
+		if (!consumer->of_node)
+			continue;
+
+		/*
+		 * A consumer device has not been probed yet. Let's parse its
+		 * device node for the power-domains property, to find out the
+		 * genpds it may belong to and then prevent sync state for them.
+		 */
+		genpd_parse_for_consumer(np, consumer->of_node);
+	}
+
+	_genpd_queue_sync_state(np);
+}
+
 static void genpd_sync_state(struct device *dev)
 {
 	return of_genpd_sync_state(dev->of_node);
@@ -3531,6 +3644,16 @@ static int genpd_provider_probe(struct device *dev)
 	return 0;
 }
 
+static void genpd_provider_queue_sync_state(struct device *dev)
+{
+	struct generic_pm_domain *genpd = container_of(dev, struct generic_pm_domain, dev);
+
+	if (genpd->sync_state != GENPD_SYNC_STATE_ONECELL)
+		return;
+
+	genpd_queue_sync_state(dev);
+}
+
 static void genpd_provider_sync_state(struct device *dev)
 {
 	struct generic_pm_domain *genpd = container_of(dev, struct generic_pm_domain, dev);
@@ -3559,6 +3682,7 @@ static struct device_driver genpd_provider_drv = {
 	.name = "genpd_provider",
 	.bus = &genpd_provider_bus_type,
 	.probe = genpd_provider_probe,
+	.queue_sync_state = genpd_provider_queue_sync_state,
 	.sync_state = genpd_provider_sync_state,
 	.suppress_bind_attrs = true,
 };
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index b299dc0128d6..7aa49721cde5 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -215,6 +215,7 @@ struct generic_pm_domain {
 	cpumask_var_t cpus;		/* A cpumask of the attached CPUs */
 	bool synced_poweroff;		/* A consumer needs a synced poweroff */
 	bool stay_on;			/* Stay powered-on during boot. */
+	bool wait_for_consumer;		/* Consumers awaits to be probed. */
 	enum genpd_sync_state sync_state; /* How sync_state is managed. */
 	int (*power_off)(struct generic_pm_domain *domain);
 	int (*power_on)(struct generic_pm_domain *domain);
-- 
2.43.0

[PATCH v3 07/13] pmdomain: core: Extend fine grained sync_state to more onecell providers

[Ulf Hansson]

A onecell power domain provider driver that we can assign a common
->sync_state() callback for, should be able to benefit from the improved
fine grained sync_state support in genpd. Therefore, let's also assign the
->queue_sync_state() callback for these types of provider drivers.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v3:
	- No changes.

---
 drivers/pmdomain/core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index c01a9a96e5c2..b2b5cc30c7fc 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -2917,10 +2917,12 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 
 	fwnode = of_fwnode_handle(np);
 	dev = get_dev_from_fwnode(fwnode);
-	if (!dev)
+	if (!dev) {
 		sync_state = true;
-	else
+	} else if (!dev_has_sync_state(dev)) {
 		dev_set_drv_sync_state(dev, genpd_sync_state);
+		dev_set_drv_queue_sync_state(dev, genpd_queue_sync_state);
+	}
 
 	put_device(dev);
 
-- 
2.43.0

[PATCH v3 08/13] pmdomain: core: Export a common function for ->queue_sync_state()

[Ulf Hansson]

Along with of_genpd_sync_state() that genpd provider drivers may use to
manage sync_state, let's add and export of_genpd_queue_sync_state() for
those that may need it. It's expected that the genpd provider driver
assigns it's own ->queue_sync_state() callback and invoke the new helper
from there.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v3:
	- No changes.

---
 drivers/pmdomain/core.c   | 14 +++++++++-----
 include/linux/pm_domain.h |  2 ++
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index b2b5cc30c7fc..b3ec3dec3f10 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -2763,7 +2763,7 @@ static void genpd_parse_for_consumer(struct device_node *sup,
 	}
 }
 
-static void _genpd_queue_sync_state(struct device_node *np)
+static void genpd_queue_sync_state(struct device_node *np)
 {
 	struct generic_pm_domain *genpd;
 
@@ -2781,11 +2781,14 @@ static void _genpd_queue_sync_state(struct device_node *np)
 	mutex_unlock(&gpd_list_lock);
 }
 
-static void genpd_queue_sync_state(struct device *dev)
+void of_genpd_queue_sync_state(struct device *dev)
 {
 	struct device_node *np = dev->of_node;
 	struct device_link *link;
 
+	if (!np)
+		return;
+
 	if (!genpd_should_wait_for_consumer(np))
 		return;
 
@@ -2809,8 +2812,9 @@ static void genpd_queue_sync_state(struct device *dev)
 		genpd_parse_for_consumer(np, consumer->of_node);
 	}
 
-	_genpd_queue_sync_state(np);
+	genpd_queue_sync_state(np);
 }
+EXPORT_SYMBOL_GPL(of_genpd_queue_sync_state);
 
 static void genpd_sync_state(struct device *dev)
 {
@@ -2921,7 +2925,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 		sync_state = true;
 	} else if (!dev_has_sync_state(dev)) {
 		dev_set_drv_sync_state(dev, genpd_sync_state);
-		dev_set_drv_queue_sync_state(dev, genpd_queue_sync_state);
+		dev_set_drv_queue_sync_state(dev, of_genpd_queue_sync_state);
 	}
 
 	put_device(dev);
@@ -3653,7 +3657,7 @@ static void genpd_provider_queue_sync_state(struct device *dev)
 	if (genpd->sync_state != GENPD_SYNC_STATE_ONECELL)
 		return;
 
-	genpd_queue_sync_state(dev);
+	of_genpd_queue_sync_state(dev);
 }
 
 static void genpd_provider_sync_state(struct device *dev)
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 7aa49721cde5..d428dd805c46 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -467,6 +467,7 @@ int of_genpd_remove_subdomain(const struct of_phandle_args *parent_spec,
 struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
 int of_genpd_parse_idle_states(struct device_node *dn,
 			       struct genpd_power_state **states, int *n);
+void of_genpd_queue_sync_state(struct device *dev);
 void of_genpd_sync_state(struct device_node *np);
 
 int genpd_dev_pm_attach(struct device *dev);
@@ -513,6 +514,7 @@ static inline int of_genpd_parse_idle_states(struct device_node *dn,
 	return -ENODEV;
 }
 
+static inline void of_genpd_queue_sync_state(struct device *dev) {}
 static inline void of_genpd_sync_state(struct device_node *np) {}
 
 static inline int genpd_dev_pm_attach(struct device *dev)
-- 
2.43.0

[PATCH v3 09/13] pmdomain: renesas: rcar-gen4-sysc: Drop GENPD_FLAG_NO_STAY_ON

[Ulf Hansson]

Due to the new fine grained sync_state support for onecell genpd provider
drivers, we should no longer need use the legacy behaviour. Therefore,
let's drop GENPD_FLAG_NO_STAY_ON.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v3:
	- No changes.

---
 drivers/pmdomain/renesas/rcar-gen4-sysc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pmdomain/renesas/rcar-gen4-sysc.c b/drivers/pmdomain/renesas/rcar-gen4-sysc.c
index 0c6c639a91d0..81b154da725f 100644
--- a/drivers/pmdomain/renesas/rcar-gen4-sysc.c
+++ b/drivers/pmdomain/renesas/rcar-gen4-sysc.c
@@ -251,7 +251,6 @@ static int __init rcar_gen4_sysc_pd_setup(struct rcar_gen4_sysc_pd *pd)
 		genpd->detach_dev = cpg_mssr_detach_dev;
 	}
 
-	genpd->flags |= GENPD_FLAG_NO_STAY_ON;
 	genpd->power_off = rcar_gen4_sysc_pd_power_off;
 	genpd->power_on = rcar_gen4_sysc_pd_power_on;
 
-- 
2.43.0

[PATCH v3 10/13] pmdomain: renesas: rcar-sysc: Drop GENPD_FLAG_NO_STAY_ON

[Ulf Hansson]

Due to the new fine grained sync_state support for onecell genpd provider
drivers, we should no longer need use the legacy behaviour. Therefore,
let's drop GENPD_FLAG_NO_STAY_ON.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v3:
	- No changes.

---
 drivers/pmdomain/renesas/rcar-sysc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pmdomain/renesas/rcar-sysc.c b/drivers/pmdomain/renesas/rcar-sysc.c
index bd7bb9cbd9da..e4608c657629 100644
--- a/drivers/pmdomain/renesas/rcar-sysc.c
+++ b/drivers/pmdomain/renesas/rcar-sysc.c
@@ -241,7 +241,6 @@ static int __init rcar_sysc_pd_setup(struct rcar_sysc_pd *pd)
 		}
 	}
 
-	genpd->flags |= GENPD_FLAG_NO_STAY_ON;
 	genpd->power_off = rcar_sysc_pd_power_off;
 	genpd->power_on = rcar_sysc_pd_power_on;
 
-- 
2.43.0

[PATCH v3 11/13] pmdomain: renesas: rmobile-sysc: Drop GENPD_FLAG_NO_STAY_ON

[Ulf Hansson]

Rmobile-sysc is not a onecell provider and didn't really needed
the GENPD_FLAG_NO_STAY_ON flag in the first place. Let's drop it.

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v3:
	- No changes.

---
 drivers/pmdomain/renesas/rmobile-sysc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pmdomain/renesas/rmobile-sysc.c b/drivers/pmdomain/renesas/rmobile-sysc.c
index 93103ff33d6e..e36f5d763c91 100644
--- a/drivers/pmdomain/renesas/rmobile-sysc.c
+++ b/drivers/pmdomain/renesas/rmobile-sysc.c
@@ -100,8 +100,7 @@ static void rmobile_init_pm_domain(struct rmobile_pm_domain *rmobile_pd)
 	struct generic_pm_domain *genpd = &rmobile_pd->genpd;
 	struct dev_power_governor *gov = rmobile_pd->gov;
 
-	genpd->flags |= GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP |
-		GENPD_FLAG_NO_STAY_ON;
+	genpd->flags |= GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP;
 	genpd->attach_dev = cpg_mstp_attach_dev;
 	genpd->detach_dev = cpg_mstp_detach_dev;
 
-- 
2.43.0

[PATCH v3 12/13] pmdomain: core: Avoid an unnecessary power off at sync_state

[Ulf Hansson]

If the PM domain was not powered on during initialization, genpd will not
prevent it from being powered off during boot. In these cases there is no
need to call genpd_power_off() at sync_state, hence let's avoid it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v3:
	- New patch.

---
 drivers/pmdomain/core.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index b3ec3dec3f10..b5d90ddfd698 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -3636,8 +3636,10 @@ void of_genpd_sync_state(struct device_node *np)
 	list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
 		if (genpd->provider == of_fwnode_handle(np)) {
 			genpd_lock(genpd);
-			genpd->stay_on = false;
-			genpd_power_off(genpd, false, 0);
+			if (genpd->stay_on) {
+				genpd->stay_on = false;
+				genpd_power_off(genpd, false, 0);
+			}
 			genpd_unlock(genpd);
 		}
 	}
@@ -3674,8 +3676,10 @@ static void genpd_provider_sync_state(struct device *dev)
 
 	case GENPD_SYNC_STATE_SIMPLE:
 		genpd_lock(genpd);
-		genpd->stay_on = false;
-		genpd_power_off(genpd, false, 0);
+		if (genpd->stay_on) {
+			genpd->stay_on = false;
+			genpd_power_off(genpd, false, 0);
+		}
 		genpd_unlock(genpd);
 		break;
 
-- 
2.43.0

[PATCH v3 13/13] pmdomain: core: Add a couple of debug prints for sync_state

[Ulf Hansson]

To inform the developer of what is happening during sync_state transitions
in genpd, let's add a couple of debug prints for it.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v3:
	- New patch.

---
 drivers/pmdomain/core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
index b5d90ddfd698..3d3e0f38815b 100644
--- a/drivers/pmdomain/core.c
+++ b/drivers/pmdomain/core.c
@@ -2756,6 +2756,9 @@ static void genpd_parse_for_consumer(struct device_node *sup,
 			genpd_lock(genpd);
 			genpd->wait_for_consumer = true;
 			genpd_unlock(genpd);
+
+			pr_debug("%s: wait for consumer %pOF\n",
+				 dev_name(&genpd->dev), con);
 		}
 		mutex_unlock(&gpd_list_lock);
 
@@ -2772,6 +2775,9 @@ static void genpd_queue_sync_state(struct device_node *np)
 		if (genpd->provider == of_fwnode_handle(np)) {
 			genpd_lock(genpd);
 			if (genpd->stay_on && !genpd->wait_for_consumer) {
+				pr_debug("%s: queuing sync state\n",
+					 dev_name(&genpd->dev));
+
 				genpd->stay_on = false;
 				genpd_queue_power_off_work(genpd);
 			}
@@ -3637,6 +3643,9 @@ void of_genpd_sync_state(struct device_node *np)
 		if (genpd->provider == of_fwnode_handle(np)) {
 			genpd_lock(genpd);
 			if (genpd->stay_on) {
+				pr_debug("%s: sync state\n",
+					 dev_name(&genpd->dev));
+
 				genpd->stay_on = false;
 				genpd_power_off(genpd, false, 0);
 			}
@@ -3677,6 +3686,8 @@ static void genpd_provider_sync_state(struct device *dev)
 	case GENPD_SYNC_STATE_SIMPLE:
 		genpd_lock(genpd);
 		if (genpd->stay_on) {
+			pr_debug("%s: sync state\n", dev_name(&genpd->dev));
+
 			genpd->stay_on = false;
 			genpd_power_off(genpd, false, 0);
 		}
-- 
2.43.0

Re: [PATCH v3 01/13] Revert "driver core: move dev_has_sync_state() to drivers/base/base.h"

[Danilo Krummrich]

On Fri May 8, 2026 at 2:38 PM CEST, Ulf Hansson wrote:
> It turns out that the dev_has_sync_state() is needed outside the driver
> core. A subsequent change to the pmdomain subsystem starts making use of
> it.
>
> Fixes: 9db268212e0d ("driver core: move dev_has_sync_state() to drivers/base/base.h")

Hm...the commit was correct, it's just that you now want to use it outside of
the driver-core code, so I'd drop this tag.

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Re: [PATCH v3 02/13] driver core: Enable suppliers to implement fine grained sync_state support

[Danilo Krummrich]

On Fri May 8, 2026 at 2:38 PM CEST, Ulf Hansson wrote:
> 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)
> +{
> +	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)
> +{

Let's add device_lock_assert(dev), as this function requires it to be held.

> +	if (dev->driver && dev->driver->queue_sync_state)
> +		dev->driver->queue_sync_state(dev);
> +}

<snip>

> @@ -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);

Hm...this is called without the device_links_write_lock() held, so this looks
like this could this race with the list_del_init() from
device_links_driver_cleanup() and create an infinite loop.

(The other list iterator above might have the same issue.)

> +
> +		if (dev != dont_lock_dev)

This is pre-existing, but I think it would be good to add a device_lock_assert()
call for this as well.

> +			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);
> +	}
>  }

Re: [PATCH v3 02/13] driver core: Enable suppliers to implement fine grained sync_state support

[Saravana Kannan]

On Fri, May 8, 2026 at 5:39 AM Ulf Hansson <ulf.hansson@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@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.

> ---
>  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()

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.

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

>
>         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()".

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
>

Re: [PATCH v3 06/13] pmdomain: core: Add initial fine grained sync_state support

[Saravana Kannan]

On Fri, May 8, 2026 at 5:39 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> A onecell (#power-domain-cells = <1 or 2>; in DT) power domain provider
> typically provides multiple independent power domains, each with their own
> corresponding consumers. In these cases we have to wait for all consumers
> for all the provided power domains before the ->sync_state() callback gets
> called for the supplier.
>
> In a first step to improve this, let's implement support for fine grained
> sync_state support a per genpd basis by using the ->queue_sync_state()
> callback. To take step by step, let's initially limit the improvement to
> the internal genpd provider driver and to its corresponding genpd devices
> for onecell providers.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes in v3:
>         - Addressed some cosmetic comments from Geert.
>
> ---
>  drivers/pmdomain/core.c   | 124 ++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h |   1 +
>  2 files changed, 125 insertions(+)
>
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index ad57846f02a3..c01a9a96e5c2 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -2699,6 +2699,119 @@ static struct generic_pm_domain *genpd_get_from_provider(
>         return genpd;
>  }
>
> +static bool genpd_should_wait_for_consumer(struct device_node *np)
> +{
> +       struct generic_pm_domain *genpd;
> +       bool should_wait = false;
> +
> +       mutex_lock(&gpd_list_lock);
> +       list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
> +               if (genpd->provider == of_fwnode_handle(np)) {
> +                       genpd_lock(genpd);
> +
> +                       /* Clear the previous state before reevaluating. */
> +                       genpd->wait_for_consumer = false;
> +
> +                       /*
> +                        * Unless there is at least one genpd for the provider
> +                        * that is being kept powered-on, we don't have to care
> +                        * about waiting for consumers.
> +                        */
> +                       if (genpd->stay_on)
> +                               should_wait = true;
> +
> +                       genpd_unlock(genpd);
> +               }
> +       }
> +       mutex_unlock(&gpd_list_lock);

I think I understand the intent of this function, but haven't dug into
genpd code enough to comment on this yet. I'll come back to this
later.

> +
> +       return should_wait;
> +}
> +
> +static void genpd_parse_for_consumer(struct device_node *sup,
> +                                    struct device_node *con)
> +{
> +       struct generic_pm_domain *genpd;
> +
> +       for (unsigned int i = 0; ; i++) {
> +               struct of_phandle_args pd_args;
> +
> +               if (of_parse_phandle_with_args(con, "power-domains",
> +                                              "#power-domain-cells",
> +                                              i, &pd_args))
> +                       break;

Why not use a while or a do while() instead of this infinite for loop
with a break?

> +
> +               /*
> +                * The phandle must correspond to the supplier's genpd provider
> +                * to be relevant else let's move to the next index.
> +                */
> +               if (sup != pd_args.np) {
> +                       of_node_put(pd_args.np);
> +                       continue;
> +               }
> +
> +               mutex_lock(&gpd_list_lock);
> +               genpd = genpd_get_from_provider(&pd_args);
> +               if (!IS_ERR(genpd)) {
> +                       genpd_lock(genpd);
> +                       genpd->wait_for_consumer = true;
> +                       genpd_unlock(genpd);
> +               }
> +               mutex_unlock(&gpd_list_lock);
> +
> +               of_node_put(pd_args.np);
> +       }
> +}
> +
> +static void _genpd_queue_sync_state(struct device_node *np)
> +{
> +       struct generic_pm_domain *genpd;
> +
> +       mutex_lock(&gpd_list_lock);
> +       list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
> +               if (genpd->provider == of_fwnode_handle(np)) {
> +                       genpd_lock(genpd);
> +                       if (genpd->stay_on && !genpd->wait_for_consumer) {
> +                               genpd->stay_on = false;
> +                               genpd_queue_power_off_work(genpd);
> +                       }
> +                       genpd_unlock(genpd);
> +               }
> +       }
> +       mutex_unlock(&gpd_list_lock);
> +}
> +
> +static void genpd_queue_sync_state(struct device *dev)
> +{
> +       struct device_node *np = dev->of_node;
> +       struct device_link *link;
> +
> +       if (!genpd_should_wait_for_consumer(np))
> +               return;
> +
> +       list_for_each_entry(link, &dev->links.consumers, s_node) {

Couple of issues:
1. I don't want the frameworks to be so deeply aware of driver core
internals. I want the driver core maintainers to be able to change the
devlink implementation without having to worry about going and fixing
all the frameworks. So, please add a for_each_consumer_dev(supplier,
callback) and for_each_supplier_dev(consumer, callback) helper
functions.

2. This doesn't ignore "SYNC_STATE_ONLY" links and that's going to
confuse the consumer count/check you might do or at the least waste
parsing those.

3. **Device** links are not the complete list of consumers because
they can only link consumer **devices** once the consumer **device**
is created.

4. What you really need is a for_each_consumer_fwnode(supplier,
callback) that first loops through all the consumer device links and
calls the callback() on their fwnode and then the same function needs
to loop through all the fwnode links and then pass those consumer
fwnodes to the callback. And inside that callback you can do whatever
you want.

5. You might want to add a for_each_inactive_consumer(supplier,
callback) too to simplify your need for checking if a fwnode has a
device and then checking if it's probed.

Thanks,
Saravana


> +               struct device *consumer = link->consumer;
> +
> +               if (!device_link_test(link, DL_FLAG_MANAGED))
> +                       continue;
> +
> +               if (link->status == DL_STATE_ACTIVE)
> +                       continue;
> +
> +               if (!consumer->of_node)
> +                       continue;
> +
> +               /*
> +                * A consumer device has not been probed yet. Let's parse its
> +                * device node for the power-domains property, to find out the
> +                * genpds it may belong to and then prevent sync state for them.
> +                */
> +               genpd_parse_for_consumer(np, consumer->of_node);
> +       }
> +
> +       _genpd_queue_sync_state(np);
> +}
> +
>  static void genpd_sync_state(struct device *dev)
>  {
>         return of_genpd_sync_state(dev->of_node);
> @@ -3531,6 +3644,16 @@ static int genpd_provider_probe(struct device *dev)
>         return 0;
>  }
>
> +static void genpd_provider_queue_sync_state(struct device *dev)
> +{
> +       struct generic_pm_domain *genpd = container_of(dev, struct generic_pm_domain, dev);
> +
> +       if (genpd->sync_state != GENPD_SYNC_STATE_ONECELL)
> +               return;
> +
> +       genpd_queue_sync_state(dev);
> +}
> +
>  static void genpd_provider_sync_state(struct device *dev)
>  {
>         struct generic_pm_domain *genpd = container_of(dev, struct generic_pm_domain, dev);
> @@ -3559,6 +3682,7 @@ static struct device_driver genpd_provider_drv = {
>         .name = "genpd_provider",
>         .bus = &genpd_provider_bus_type,
>         .probe = genpd_provider_probe,
> +       .queue_sync_state = genpd_provider_queue_sync_state,
>         .sync_state = genpd_provider_sync_state,
>         .suppress_bind_attrs = true,
>  };
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index b299dc0128d6..7aa49721cde5 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -215,6 +215,7 @@ struct generic_pm_domain {
>         cpumask_var_t cpus;             /* A cpumask of the attached CPUs */
>         bool synced_poweroff;           /* A consumer needs a synced poweroff */
>         bool stay_on;                   /* Stay powered-on during boot. */
> +       bool wait_for_consumer;         /* Consumers awaits to be probed. */
>         enum genpd_sync_state sync_state; /* How sync_state is managed. */
>         int (*power_off)(struct generic_pm_domain *domain);
>         int (*power_on)(struct generic_pm_domain *domain);
> --
> 2.43.0
>

Re: [PATCH v3 02/13] driver core: Enable suppliers to implement fine grained sync_state support

[Ulf Hansson]

On Fri, 8 May 2026 at 20:23, Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Fri May 8, 2026 at 2:38 PM CEST, Ulf Hansson wrote:
> > 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)
> > +{
> > +     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)
> > +{
>
> Let's add device_lock_assert(dev), as this function requires it to be held.
>
> > +     if (dev->driver && dev->driver->queue_sync_state)
> > +             dev->driver->queue_sync_state(dev);
> > +}
>
> <snip>
>
> > @@ -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);
>
> Hm...this is called without the device_links_write_lock() held, so this looks
> like this could this race with the list_del_init() from
> device_links_driver_cleanup() and create an infinite loop.
>
> (The other list iterator above might have the same issue.)

Yes, you are right!

I was trying to follow the existing implementation for the regular
sync_state, but it certainly looks like we need a separate lock to
protect the lists.

Let me fold in a patch that fixes this for the regular ->sync_state()
code first.

>
> > +
> > +             if (dev != dont_lock_dev)
>
> This is pre-existing, but I think it would be good to add a device_lock_assert()
> call for this as well.

Makes perfect sense.

Again, I will fold in a patch that start by adding this for the
regular ->sync_state().

>
> > +                     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);
> > +     }
> >  }

Thanks for reviewing!

Kind regards
Uffe

Re: [PATCH v3 02/13] driver core: Enable suppliers to implement fine grained sync_state support

[Ulf Hansson]

On Mon, 11 May 2026 at 07:09, Saravana Kannan <saravanak@kernel.org> wrote:
>
> On Fri, May 8, 2026 at 5:39 AM Ulf Hansson <ulf.hansson@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@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

Re: [PATCH v3 06/13] pmdomain: core: Add initial fine grained sync_state support

[Ulf Hansson]

On Mon, 11 May 2026 at 07:09, Saravana Kannan <saravanak@kernel.org> wrote:
>
> On Fri, May 8, 2026 at 5:39 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > A onecell (#power-domain-cells = <1 or 2>; in DT) power domain provider
> > typically provides multiple independent power domains, each with their own
> > corresponding consumers. In these cases we have to wait for all consumers
> > for all the provided power domains before the ->sync_state() callback gets
> > called for the supplier.
> >
> > In a first step to improve this, let's implement support for fine grained
> > sync_state support a per genpd basis by using the ->queue_sync_state()
> > callback. To take step by step, let's initially limit the improvement to
> > the internal genpd provider driver and to its corresponding genpd devices
> > for onecell providers.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >
> > Changes in v3:
> >         - Addressed some cosmetic comments from Geert.
> >
> > ---
> >  drivers/pmdomain/core.c   | 124 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/pm_domain.h |   1 +
> >  2 files changed, 125 insertions(+)
> >
> > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > index ad57846f02a3..c01a9a96e5c2 100644
> > --- a/drivers/pmdomain/core.c
> > +++ b/drivers/pmdomain/core.c
> > @@ -2699,6 +2699,119 @@ static struct generic_pm_domain *genpd_get_from_provider(
> >         return genpd;
> >  }
> >
> > +static bool genpd_should_wait_for_consumer(struct device_node *np)
> > +{
> > +       struct generic_pm_domain *genpd;
> > +       bool should_wait = false;
> > +
> > +       mutex_lock(&gpd_list_lock);
> > +       list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
> > +               if (genpd->provider == of_fwnode_handle(np)) {
> > +                       genpd_lock(genpd);
> > +
> > +                       /* Clear the previous state before reevaluating. */
> > +                       genpd->wait_for_consumer = false;
> > +
> > +                       /*
> > +                        * Unless there is at least one genpd for the provider
> > +                        * that is being kept powered-on, we don't have to care
> > +                        * about waiting for consumers.
> > +                        */
> > +                       if (genpd->stay_on)
> > +                               should_wait = true;
> > +
> > +                       genpd_unlock(genpd);
> > +               }
> > +       }
> > +       mutex_unlock(&gpd_list_lock);
>
> I think I understand the intent of this function, but haven't dug into
> genpd code enough to comment on this yet. I'll come back to this
> later.
>
> > +
> > +       return should_wait;
> > +}
> > +
> > +static void genpd_parse_for_consumer(struct device_node *sup,
> > +                                    struct device_node *con)
> > +{
> > +       struct generic_pm_domain *genpd;
> > +
> > +       for (unsigned int i = 0; ; i++) {
> > +               struct of_phandle_args pd_args;
> > +
> > +               if (of_parse_phandle_with_args(con, "power-domains",
> > +                                              "#power-domain-cells",
> > +                                              i, &pd_args))
> > +                       break;
>
> Why not use a while or a do while() instead of this infinite for loop
> with a break?

I guess it's a matter of personal preference. I'm not sure the code
gets any nicer with a do/while, but if you really insist I can change
it.

>
> > +
> > +               /*
> > +                * The phandle must correspond to the supplier's genpd provider
> > +                * to be relevant else let's move to the next index.
> > +                */
> > +               if (sup != pd_args.np) {
> > +                       of_node_put(pd_args.np);
> > +                       continue;
> > +               }
> > +
> > +               mutex_lock(&gpd_list_lock);
> > +               genpd = genpd_get_from_provider(&pd_args);
> > +               if (!IS_ERR(genpd)) {
> > +                       genpd_lock(genpd);
> > +                       genpd->wait_for_consumer = true;
> > +                       genpd_unlock(genpd);
> > +               }
> > +               mutex_unlock(&gpd_list_lock);
> > +
> > +               of_node_put(pd_args.np);
> > +       }
> > +}
> > +
> > +static void _genpd_queue_sync_state(struct device_node *np)
> > +{
> > +       struct generic_pm_domain *genpd;
> > +
> > +       mutex_lock(&gpd_list_lock);
> > +       list_for_each_entry(genpd, &gpd_list, gpd_list_node) {
> > +               if (genpd->provider == of_fwnode_handle(np)) {
> > +                       genpd_lock(genpd);
> > +                       if (genpd->stay_on && !genpd->wait_for_consumer) {
> > +                               genpd->stay_on = false;
> > +                               genpd_queue_power_off_work(genpd);
> > +                       }
> > +                       genpd_unlock(genpd);
> > +               }
> > +       }
> > +       mutex_unlock(&gpd_list_lock);
> > +}
> > +
> > +static void genpd_queue_sync_state(struct device *dev)
> > +{
> > +       struct device_node *np = dev->of_node;
> > +       struct device_link *link;
> > +
> > +       if (!genpd_should_wait_for_consumer(np))
> > +               return;
> > +
> > +       list_for_each_entry(link, &dev->links.consumers, s_node) {
>
> Couple of issues:
> 1. I don't want the frameworks to be so deeply aware of driver core
> internals. I want the driver core maintainers to be able to change the
> devlink implementation without having to worry about going and fixing
> all the frameworks. So, please add a for_each_consumer_dev(supplier,
> callback) and for_each_supplier_dev(consumer, callback) helper
> functions.

I understand your concern and I like the idea. However, maybe it's
better to get this landed (the series is complicated as is) first and
then can continue to improve the code on top, with helper functions
etc.

>
> 2. This doesn't ignore "SYNC_STATE_ONLY" links and that's going to
> confuse the consumer count/check you might do or at the least waste
> parsing those.

I am not sure I understand how I should take SYNC_STATE_ONLY links
into account here.

At each call to the genpd_queue_sync_state(), we walk through all the
provided genpds for the provider. No previous state is cached to track
consumer counts.

>
> 3. **Device** links are not the complete list of consumers because
> they can only link consumer **devices** once the consumer **device**
> is created.
>
> 4. What you really need is a for_each_consumer_fwnode(supplier,
> callback) that first loops through all the consumer device links and
> calls the callback() on their fwnode and then the same function needs
> to loop through all the fwnode links and then pass those consumer
> fwnodes to the callback. And inside that callback you can do whatever
> you want.

The ->queue_sync_state() callback is invoked *after*
__device_links_queue_sync_state() has been called for the device,
which is also when the conditions for calling ->sync_state() is
checked.

If there are problems with not yet registered consumer device links,
why isn't that as problem for regular ->sync_state() in
__device_links_queue_sync_state()?

>
> 5. You might want to add a for_each_inactive_consumer(supplier,
> callback) too to simplify your need for checking if a fwnode has a
> device and then checking if it's probed.
>
> Thanks,
> Saravana
>
>
> > +               struct device *consumer = link->consumer;
> > +
> > +               if (!device_link_test(link, DL_FLAG_MANAGED))
> > +                       continue;
> > +
> > +               if (link->status == DL_STATE_ACTIVE)
> > +                       continue;
> > +
> > +               if (!consumer->of_node)
> > +                       continue;
> > +
> > +               /*
> > +                * A consumer device has not been probed yet. Let's parse its
> > +                * device node for the power-domains property, to find out the
> > +                * genpds it may belong to and then prevent sync state for them.
> > +                */
> > +               genpd_parse_for_consumer(np, consumer->of_node);
> > +       }
> > +
> > +       _genpd_queue_sync_state(np);
> > +}
> > +

[...]

Kind regards
Uffe