[PATCH v3 0/9] driver core: class: Fix bug and code improvements for class APIs

[PATCH v3 0/9] driver core: class: Fix bug and code improvements for class APIs

[Zijun Hu]

This patch series is to fix bugs and improve codes regarding various
driver core device iterating APIs

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Changes in v3:
- Correct commit message, add fix tag, and correct pr_crit() message for 1st patch
- Add more patches regarding driver core device iterating APIs.
- Link to v2: https://lore.kernel.org/r/20241112-class_fix-v2-0-73d198d0a0d5@quicinc.com

Changes in v2:
- Remove both fix and stable tag for patch 1/3
- drop patch 3/3
- Link to v1: https://lore.kernel.org/r/20241105-class_fix-v1-0-80866f9994a5@quicinc.com

---
Zijun Hu (9):
      driver core: class: Fix wild pointer dereferences in API class_dev_iter_next()
      blk-cgroup: Fix class @block_class's subsystem refcount leakage
      driver core: bus: Move true expression out of if condition in API bus_find_device()
      driver core: Move true expression out of if condition in API driver_find_device()
      driver core: Move true expression out of if condition in API device_find_child()
      driver core: Rename declaration parameter name for API device_find_child() cluster
      driver core: Correct parameter check for API device_for_each_child_reverse_from()
      driver core: Correct API device_for_each_child_reverse_from() prototype
      driver core: Introduce device_iter_t for device iterating APIs

 block/blk-cgroup.c            |  1 +
 drivers/base/bus.c            |  9 ++++++---
 drivers/base/class.c          | 11 +++++++++--
 drivers/base/core.c           | 17 ++++++++++-------
 drivers/base/driver.c         |  9 ++++++---
 drivers/cxl/core/hdm.c        |  2 +-
 drivers/cxl/core/region.c     |  2 +-
 include/linux/device.h        | 14 +++++++-------
 include/linux/device/bus.h    |  7 +++++--
 include/linux/device/class.h  |  4 ++--
 include/linux/device/driver.h |  2 +-
 11 files changed, 49 insertions(+), 29 deletions(-)
---
base-commit: cdd30ebb1b9f36159d66f088b61aee264e649d7a
change-id: 20241104-class_fix-f176bd9eba22
prerequisite-change-id: 20241201-const_dfc_done-aaec71e3bbea:v4
prerequisite-patch-id: 536aa56c0d055f644a1f71ab5c88b7cac9510162
prerequisite-patch-id: 39b0cf088c72853d9ce60c9e633ad2070a0278a8
prerequisite-patch-id: 60b22c42b67ad56a3d2a7b80a30ad588cbe740ec
prerequisite-patch-id: 119a167d7248481987b5e015db0e4fdb0d6edab8
prerequisite-patch-id: 133248083f3d3c57beb16473c2a4c62b3abc5fd0
prerequisite-patch-id: 4cda541f55165650bfa69fb19cbe0524eff0cb85
prerequisite-patch-id: 2b4193c6ea6370c07e6b66de04be89fb09448f54
prerequisite-patch-id: 73c675db18330c89fd8ca4790914d1d486ce0db8
prerequisite-patch-id: 88c50fc851fd7077797fd4e63fb12966b1b601bd
prerequisite-patch-id: 47b93916c1b5fb809d7c99aeaa05c729b1af01c5
prerequisite-patch-id: 52ffb42b5aae69cae708332e0ddc7016139999f1

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

[PATCH v3 1/9] driver core: class: Fix wild pointer dereferences in API class_dev_iter_next()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

There are a potential wild pointer dereferences issue regarding APIs
class_dev_iter_(init|next|exit)(), as explained by below typical usage:

// All members of @iter are wild pointers.
struct class_dev_iter iter;

// class_dev_iter_init(@iter, @class, ...) checks parameter @class for
// potential class_to_subsys() error, and it returns void type and does
// not initialize its output parameter @iter, so caller can not detect
// the error and continues to invoke class_dev_iter_next(@iter) even if
// @iter still contains wild pointers.
class_dev_iter_init(&iter, ...);

// Dereference these wild pointers in @iter here once suffer the error.
while (dev = class_dev_iter_next(&iter)) { ... };

// Also dereference these wild pointers here.
class_dev_iter_exit(&iter);

Actually, all callers of these APIs have such usage pattern in kernel tree.
Fix by:
- Initialize output parameter @iter by memset() in class_dev_iter_init()
  and give callers prompt by pr_crit() for the error.
- Check if @iter is valid in class_dev_iter_next().

Fixes: 7b884b7f24b4 ("driver core: class.c: convert to only use class_to_subsys")
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>

---
Alternative fix solutions ever thought about:

1) Use BUG_ON(!sp) instead of error return in class_dev_iter_init().
2) Change class_dev_iter_init()'s type to int, lots of jobs to do.

This issue is APIs themself issues, and regardless of how various API
users use them, and silent wild pointer dereferences are not what API
users expect for the error absolutely.
---
 drivers/base/class.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/base/class.c b/drivers/base/class.c
index 582b5a02a5c410113326601fe00eb6d7231f988f..d57f277978dc9033fba3484b4620bcf884a4029f 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -323,8 +323,12 @@ void class_dev_iter_init(struct class_dev_iter *iter, const struct class *class,
 	struct subsys_private *sp = class_to_subsys(class);
 	struct klist_node *start_knode = NULL;
 
-	if (!sp)
+	memset(iter, 0, sizeof(*iter));
+	if (!sp) {
+		pr_crit("%s: class %p was not registered yet\n",
+			__func__, class);
 		return;
+	}
 
 	if (start)
 		start_knode = &start->p->knode_class;
@@ -351,6 +355,9 @@ struct device *class_dev_iter_next(struct class_dev_iter *iter)
 	struct klist_node *knode;
 	struct device *dev;
 
+	if (!iter->sp)
+		return NULL;
+
 	while (1) {
 		knode = klist_next(&iter->ki);
 		if (!knode)

-- 
2.34.1

Re: [PATCH v3 1/9] driver core: class: Fix wild pointer dereferences in API class_dev_iter_next()

[Jonathan Cameron]

On Thu, 12 Dec 2024 21:38:37 +0800
Zijun Hu <zijun_hu@icloud.com> wrote:

> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> There are a potential wild pointer dereferences issue regarding APIs
> class_dev_iter_(init|next|exit)(), as explained by below typical usage:
> 
> // All members of @iter are wild pointers.
> struct class_dev_iter iter;
> 
> // class_dev_iter_init(@iter, @class, ...) checks parameter @class for
> // potential class_to_subsys() error, and it returns void type and does
> // not initialize its output parameter @iter, so caller can not detect
> // the error and continues to invoke class_dev_iter_next(@iter) even if
> // @iter still contains wild pointers.
> class_dev_iter_init(&iter, ...);
> 
> // Dereference these wild pointers in @iter here once suffer the error.
> while (dev = class_dev_iter_next(&iter)) { ... };
> 
> // Also dereference these wild pointers here.
> class_dev_iter_exit(&iter);
> 
> Actually, all callers of these APIs have such usage pattern in kernel tree.
> Fix by:
> - Initialize output parameter @iter by memset() in class_dev_iter_init()
>   and give callers prompt by pr_crit() for the error.
> - Check if @iter is valid in class_dev_iter_next().
> 
> Fixes: 7b884b7f24b4 ("driver core: class.c: convert to only use class_to_subsys")
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>

This looks fine in general, but over to the core device model folk for which
element they think should be used as the sentinel and whether zeroing the
whole thing makes sense or just the one being used as a flag, or even setting
it to an error pointer.

> 
> ---
> Alternative fix solutions ever thought about:
> 
> 1) Use BUG_ON(!sp) instead of error return in class_dev_iter_init().
> 2) Change class_dev_iter_init()'s type to int, lots of jobs to do.
> 
> This issue is APIs themself issues, and regardless of how various API
> users use them, and silent wild pointer dereferences are not what API
> users expect for the error absolutely.
> ---
>  drivers/base/class.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/class.c b/drivers/base/class.c
> index 582b5a02a5c410113326601fe00eb6d7231f988f..d57f277978dc9033fba3484b4620bcf884a4029f 100644
> --- a/drivers/base/class.c
> +++ b/drivers/base/class.c
> @@ -323,8 +323,12 @@ void class_dev_iter_init(struct class_dev_iter *iter, const struct class *class,
>  	struct subsys_private *sp = class_to_subsys(class);
>  	struct klist_node *start_knode = NULL;
>  
> -	if (!sp)
> +	memset(iter, 0, sizeof(*iter));
> +	if (!sp) {
> +		pr_crit("%s: class %p was not registered yet\n",
> +			__func__, class);
>  		return;
> +	}
>  
>  	if (start)
>  		start_knode = &start->p->knode_class;
> @@ -351,6 +355,9 @@ struct device *class_dev_iter_next(struct class_dev_iter *iter)
>  	struct klist_node *knode;
>  	struct device *dev;
>  
> +	if (!iter->sp)
> +		return NULL;
> +
>  	while (1) {
>  		knode = klist_next(&iter->ki);
>  		if (!knode)
> 

Re: [PATCH v3 1/9] driver core: class: Fix wild pointer dereferences in API class_dev_iter_next()

[Zijun Hu]

On 2024/12/16 23:36, Jonathan Cameron wrote:
>> Actually, all callers of these APIs have such usage pattern in kernel tree.
>> Fix by:
>> - Initialize output parameter @iter by memset() in class_dev_iter_init()
>>   and give callers prompt by pr_crit() for the error.
>> - Check if @iter is valid in class_dev_iter_next().
>>
>> Fixes: 7b884b7f24b4 ("driver core: class.c: convert to only use class_to_subsys")
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> This looks fine in general, but over to the core device model folk for which
> element they think should be used as the sentinel and whether zeroing the
> whole thing makes sense or just the one being used as a flag, or even setting
> it to an error pointer.

thank you Jonathan for code review.

i actually ever thought about below change and finally dropped it.

-       if (!sp)
+       if (!sp) {
+               iter->sp = NULL;
                return;
+       }


For such APIs, they return void, and does not any output parameter to
feedback operation results.
it may be good practice to always reset its output parameters firstly.

[PATCH v3 2/9] blk-cgroup: Fix class @block_class's subsystem refcount leakage

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

blkcg_fill_root_iostats() iterates over @block_class's devices by
class_dev_iter_(init|next)(), but does not end iterating with
class_dev_iter_exit(), so causes the class's subsystem refcount leakage.

Fix by ending the iterating with class_dev_iter_exit().

Fixes: ef45fe470e1e ("blk-cgroup: show global disk stats in root cgroup io.stat")
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 block/blk-cgroup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index e68c725cf8d975f53703ecf6e6c50594076204c8..fb9858efdfe9443704cb9a239def0e08addf2518 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1138,6 +1138,7 @@ static void blkcg_fill_root_iostats(void)
 		blkg_iostat_set(&blkg->iostat.cur, &tmp);
 		u64_stats_update_end_irqrestore(&blkg->iostat.sync, flags);
 	}
+	class_dev_iter_exit(&iter);
 }
 
 static void blkcg_print_one_stat(struct blkcg_gq *blkg, struct seq_file *s)

-- 
2.34.1

Re: [PATCH v3 2/9] blk-cgroup: Fix class @block_class's subsystem refcount leakage

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 206 bytes --]

On Thu, Dec 12, 2024 at 09:38:38PM GMT, Zijun Hu <zijun_hu@icloud.com> wrote:
>  block/blk-cgroup.c | 1 +
>  1 file changed, 1 insertion(+)

Well caught,
Reviewed-by: Michal Koutný <mkoutny@suse.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v3 3/9] driver core: bus: Move true expression out of if condition in API bus_find_device()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

For bus_find_device(), get_device() in the if condition always returns
true, move it to if body to make the API's logic more clearer.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/base/bus.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 657c93c38b0dc2a2247e5f482fadd3a9376a58e8..73a56f376d3a05962ce0931a2fe8b4d8839157f2 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -402,9 +402,12 @@ struct device *bus_find_device(const struct bus_type *bus,
 
 	klist_iter_init_node(&sp->klist_devices, &i,
 			     (start ? &start->p->knode_bus : NULL));
-	while ((dev = next_device(&i)))
-		if (match(dev, data) && get_device(dev))
+	while ((dev = next_device(&i))) {
+		if (match(dev, data)) {
+			get_device(dev);
 			break;
+		}
+	}
 	klist_iter_exit(&i);
 	subsys_put(sp);
 	return dev;

-- 
2.34.1

Re: [PATCH v3 3/9] driver core: bus: Move true expression out of if condition in API bus_find_device()

[Jonathan Cameron]

On Thu, 12 Dec 2024 21:38:39 +0800
Zijun Hu <zijun_hu@icloud.com> wrote:

> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> For bus_find_device(), get_device() in the if condition always returns
> true, move it to if body to make the API's logic more clearer.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
I agree this is easier to read and the reasoning is sound.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/base/bus.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 657c93c38b0dc2a2247e5f482fadd3a9376a58e8..73a56f376d3a05962ce0931a2fe8b4d8839157f2 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -402,9 +402,12 @@ struct device *bus_find_device(const struct bus_type *bus,
>  
>  	klist_iter_init_node(&sp->klist_devices, &i,
>  			     (start ? &start->p->knode_bus : NULL));
> -	while ((dev = next_device(&i)))
> -		if (match(dev, data) && get_device(dev))
> +	while ((dev = next_device(&i))) {
> +		if (match(dev, data)) {
> +			get_device(dev);
>  			break;
> +		}
> +	}
>  	klist_iter_exit(&i);
>  	subsys_put(sp);
>  	return dev;
> 

[PATCH v3 4/9] driver core: Move true expression out of if condition in API driver_find_device()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

For driver_find_device(), get_device() in the if condition always returns
true, move it to if body to make the API's logic more clearer.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/base/driver.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index b4eb5b89c4ee7bc35458fc75730b16a6d1e804d3..6f033a741aa7ce6138d1c61e49e72b2a3eb85e06 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -160,9 +160,12 @@ struct device *driver_find_device(const struct device_driver *drv,
 
 	klist_iter_init_node(&drv->p->klist_devices, &i,
 			     (start ? &start->p->knode_driver : NULL));
-	while ((dev = next_device(&i)))
-		if (match(dev, data) && get_device(dev))
+	while ((dev = next_device(&i))) {
+		if (match(dev, data)) {
+			get_device(dev);
 			break;
+		}
+	}
 	klist_iter_exit(&i);
 	return dev;
 }

-- 
2.34.1

Re: [PATCH v3 4/9] driver core: Move true expression out of if condition in API driver_find_device()

[Jonathan Cameron]

On Thu, 12 Dec 2024 21:38:40 +0800
Zijun Hu <zijun_hu@icloud.com> wrote:

> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> For driver_find_device(), get_device() in the if condition always returns
> true, move it to if body to make the API's logic more clearer.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Though I think it would have been fine to have all these similar patches
as a single patch.

> ---
>  drivers/base/driver.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index b4eb5b89c4ee7bc35458fc75730b16a6d1e804d3..6f033a741aa7ce6138d1c61e49e72b2a3eb85e06 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -160,9 +160,12 @@ struct device *driver_find_device(const struct device_driver *drv,
>  
>  	klist_iter_init_node(&drv->p->klist_devices, &i,
>  			     (start ? &start->p->knode_driver : NULL));
> -	while ((dev = next_device(&i)))
> -		if (match(dev, data) && get_device(dev))
> +	while ((dev = next_device(&i))) {
> +		if (match(dev, data)) {
> +			get_device(dev);
>  			break;
> +		}
> +	}
>  	klist_iter_exit(&i);
>  	return dev;
>  }
> 

Re: [PATCH v3 4/9] driver core: Move true expression out of if condition in API driver_find_device()

[Fan Ni]

On Thu, Dec 12, 2024 at 09:38:40PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> For driver_find_device(), get_device() in the if condition always returns
> true, move it to if body to make the API's logic more clearer.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---

This patch and the next patch can be squished into one patch.

Reviewed-by: Fan Ni <fan.ni@samsung.com>

>  drivers/base/driver.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
> index b4eb5b89c4ee7bc35458fc75730b16a6d1e804d3..6f033a741aa7ce6138d1c61e49e72b2a3eb85e06 100644
> --- a/drivers/base/driver.c
> +++ b/drivers/base/driver.c
> @@ -160,9 +160,12 @@ struct device *driver_find_device(const struct device_driver *drv,
>  
>  	klist_iter_init_node(&drv->p->klist_devices, &i,
>  			     (start ? &start->p->knode_driver : NULL));
> -	while ((dev = next_device(&i)))
> -		if (match(dev, data) && get_device(dev))
> +	while ((dev = next_device(&i))) {
> +		if (match(dev, data)) {
> +			get_device(dev);
>  			break;
> +		}
> +	}
>  	klist_iter_exit(&i);
>  	return dev;
>  }
> 
> -- 
> 2.34.1
> 

-- 
Fan Ni

Re: [PATCH v3 4/9] driver core: Move true expression out of if condition in API driver_find_device()

[Zijun Hu]

On 2024/12/17 02:01, Fan Ni wrote:
> On Thu, Dec 12, 2024 at 09:38:40PM +0800, Zijun Hu wrote:
>> From: Zijun Hu <quic_zijuhu@quicinc.com>
>>
>> For driver_find_device(), get_device() in the if condition always returns
>> true, move it to if body to make the API's logic more clearer.
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
> 
> This patch and the next patch can be squished into one patch.
> 
> Reviewed-by: Fan Ni <fan.ni@samsung.com>
> 

thank you Fan and Jonathan for code review.

good suggestion.
will squish the three similar changes into one in v4. (^^)(^^)

>>  drivers/base/driver.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/driver.c b/drivers/base/driver.c
>> index b4eb5b89c4ee7bc35458fc75730b16a6d1e804d3..6f033a741aa7ce6138d1c61e49e72b2a3eb85e06 100644
>> --- a/drivers/base/driver.c
>> +++ b/drivers/base/driver.c
>> @@ -160,9 +160,12 @@ struct device *driver_find_device(const struct device_driver *drv,
>>  
>>  	klist_iter_init_node(&drv->p->klist_devices, &i,
>>  			     (start ? &start->p->knode_driver : NULL));
>> -	while ((dev = next_device(&i)))
>> -		if (match(dev, data) && get_device(dev))
>> +	while ((dev = next_device(&i))) {
>> +		if (match(dev, data)) {
>> +			get_device(dev);
>>  			break;
>> +		}
>> +	}
>>  	klist_iter_exit(&i);
>>  	return dev;
>>  }
>>
>> -- 
>> 2.34.1
>>
> 

[PATCH v3 5/9] driver core: Move true expression out of if condition in API device_find_child()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

For device_find_child(), get_device() in the if condition always returns
true, move it to if body to make the API's logic more clearer.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/base/core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 8bdbc9e657e832a063542391426f570ccb5c18b9..69bb6bf4bd12395226ee3c99e2f63d15c7e342a5 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4089,9 +4089,12 @@ struct device *device_find_child(struct device *parent, const void *data,
 		return NULL;
 
 	klist_iter_init(&parent->p->klist_children, &i);
-	while ((child = next_device(&i)))
-		if (match(child, data) && get_device(child))
+	while ((child = next_device(&i))) {
+		if (match(child, data)) {
+			get_device(child);
 			break;
+		}
+	}
 	klist_iter_exit(&i);
 	return child;
 }

-- 
2.34.1

Re: [PATCH v3 5/9] driver core: Move true expression out of if condition in API device_find_child()

[Jonathan Cameron]

On Thu, 12 Dec 2024 21:38:41 +0800
Zijun Hu <zijun_hu@icloud.com> wrote:

> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> For device_find_child(), get_device() in the if condition always returns
> true, move it to if body to make the API's logic more clearer.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
I'd squash with previous 2.  They are all the same change in the same
area of the kernel.

Jonathan

> ---
>  drivers/base/core.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 8bdbc9e657e832a063542391426f570ccb5c18b9..69bb6bf4bd12395226ee3c99e2f63d15c7e342a5 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4089,9 +4089,12 @@ struct device *device_find_child(struct device *parent, const void *data,
>  		return NULL;
>  
>  	klist_iter_init(&parent->p->klist_children, &i);
> -	while ((child = next_device(&i)))
> -		if (match(child, data) && get_device(child))
> +	while ((child = next_device(&i))) {
> +		if (match(child, data)) {
> +			get_device(child);
>  			break;
> +		}
> +	}
>  	klist_iter_exit(&i);
>  	return child;
>  }
> 

Re: [PATCH v3 5/9] driver core: Move true expression out of if condition in API device_find_child()

[Fan Ni]

On Thu, Dec 12, 2024 at 09:38:41PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> For device_find_child(), get_device() in the if condition always returns
> true, move it to if body to make the API's logic more clearer.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>

Reviewed-by: Fan Ni <fan.ni@samsung.com>

> ---
>  drivers/base/core.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 8bdbc9e657e832a063542391426f570ccb5c18b9..69bb6bf4bd12395226ee3c99e2f63d15c7e342a5 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4089,9 +4089,12 @@ struct device *device_find_child(struct device *parent, const void *data,
>  		return NULL;
>  
>  	klist_iter_init(&parent->p->klist_children, &i);
> -	while ((child = next_device(&i)))
> -		if (match(child, data) && get_device(child))
> +	while ((child = next_device(&i))) {
> +		if (match(child, data)) {
> +			get_device(child);
>  			break;
> +		}
> +	}
>  	klist_iter_exit(&i);
>  	return child;
>  }
> 
> -- 
> 2.34.1
> 

-- 
Fan Ni

[PATCH v3 6/9] driver core: Rename declaration parameter name for API device_find_child() cluster

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

For APIs:
device_find_child()
device_for_each_child()
device_for_each_child_reverse()

Their declaration has parameter name 'dev', but their defination
changes the name to 'parent'.

Rename declaration name to defination 'parent' to make both have
the same name.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 include/linux/device.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/device.h b/include/linux/device.h
index 0e0bc9bfe0d15a8734bf3d34106300f4df6b5364..a9d928398895b062094b94f2c188cbe9951d7ac1 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1074,14 +1074,14 @@ void device_del(struct device *dev);
 
 DEFINE_FREE(device_del, struct device *, if (_T) device_del(_T))
 
-int device_for_each_child(struct device *dev, void *data,
+int device_for_each_child(struct device *parent, void *data,
 			  int (*fn)(struct device *dev, void *data));
-int device_for_each_child_reverse(struct device *dev, void *data,
+int device_for_each_child_reverse(struct device *parent, void *data,
 				  int (*fn)(struct device *dev, void *data));
 int device_for_each_child_reverse_from(struct device *parent,
 				       struct device *from, const void *data,
 				       int (*fn)(struct device *, const void *));
-struct device *device_find_child(struct device *dev, const void *data,
+struct device *device_find_child(struct device *parent, const void *data,
 				 device_match_t match);
 struct device *device_find_child_by_name(struct device *parent,
 					 const char *name);

-- 
2.34.1

Re: [PATCH v3 6/9] driver core: Rename declaration parameter name for API device_find_child() cluster

[Jonathan Cameron]

On Thu, 12 Dec 2024 21:38:42 +0800
Zijun Hu <zijun_hu@icloud.com> wrote:

> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> For APIs:
> device_find_child()
> device_for_each_child()
> device_for_each_child_reverse()
> 
> Their declaration has parameter name 'dev', but their defination
> changes the name to 'parent'.
> 
> Rename declaration name to defination 'parent' to make both have
> the same name.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v3 6/9] driver core: Rename declaration parameter name for API device_find_child() cluster

[Fan Ni]

On Thu, Dec 12, 2024 at 09:38:42PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> For APIs:
> device_find_child()
> device_for_each_child()
> device_for_each_child_reverse()
> 
> Their declaration has parameter name 'dev', but their defination
> changes the name to 'parent'.
> 
> Rename declaration name to defination 'parent' to make both have
> the same name.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>

Reviewed-by: Fan Ni <fan.ni@samsung.com>

> ---
>  include/linux/device.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 0e0bc9bfe0d15a8734bf3d34106300f4df6b5364..a9d928398895b062094b94f2c188cbe9951d7ac1 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1074,14 +1074,14 @@ void device_del(struct device *dev);
>  
>  DEFINE_FREE(device_del, struct device *, if (_T) device_del(_T))
>  
> -int device_for_each_child(struct device *dev, void *data,
> +int device_for_each_child(struct device *parent, void *data,
>  			  int (*fn)(struct device *dev, void *data));
> -int device_for_each_child_reverse(struct device *dev, void *data,
> +int device_for_each_child_reverse(struct device *parent, void *data,
>  				  int (*fn)(struct device *dev, void *data));
>  int device_for_each_child_reverse_from(struct device *parent,
>  				       struct device *from, const void *data,
>  				       int (*fn)(struct device *, const void *));
> -struct device *device_find_child(struct device *dev, const void *data,
> +struct device *device_find_child(struct device *parent, const void *data,
>  				 device_match_t match);
>  struct device *device_find_child_by_name(struct device *parent,
>  					 const char *name);
> 
> -- 
> 2.34.1
> 

-- 
Fan Ni

[PATCH v3 7/9] driver core: Correct parameter check for API device_for_each_child_reverse_from()

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

device_for_each_child_reverse_from() checks (!parent->p) for its
parameter @parent, and that is not consistent with other APIs of
its cluster as shown below:

device_for_each_child_reverse_from() // check (!parent->p)
device_for_each_child_reverse()      // check (!parent || !parent->p)
device_for_each_child()              // same above
device_find_child()                  // same above

Correct the API's parameter @parent check by (!parent || !parent->p).

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/base/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 69bb6bf4bd12395226ee3c99e2f63d15c7e342a5..34fb13f914b3db47e6a047fdabf3c9b18ecc08cc 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4050,7 +4050,7 @@ int device_for_each_child_reverse_from(struct device *parent,
 	struct device *child;
 	int error = 0;
 
-	if (!parent->p)
+	if (!parent || !parent->p)
 		return 0;
 
 	klist_iter_init_node(&parent->p->klist_children, &i,

-- 
2.34.1

Re: [PATCH v3 7/9] driver core: Correct parameter check for API device_for_each_child_reverse_from()

[Jonathan Cameron]

On Thu, 12 Dec 2024 21:38:43 +0800
Zijun Hu <zijun_hu@icloud.com> wrote:

> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> device_for_each_child_reverse_from() checks (!parent->p) for its
> parameter @parent, and that is not consistent with other APIs of
> its cluster as shown below:
> 
> device_for_each_child_reverse_from() // check (!parent->p)
> device_for_each_child_reverse()      // check (!parent || !parent->p)
> device_for_each_child()              // same above
> device_find_child()                  // same above
> 
> Correct the API's parameter @parent check by (!parent || !parent->p).
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
Given that 'from' implies continuation of an iteration I can see why
it might not ever be relevant to check parent.  It's harmless, but to
my mind unnecessary.

Jonathan

> ---
>  drivers/base/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 69bb6bf4bd12395226ee3c99e2f63d15c7e342a5..34fb13f914b3db47e6a047fdabf3c9b18ecc08cc 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4050,7 +4050,7 @@ int device_for_each_child_reverse_from(struct device *parent,
>  	struct device *child;
>  	int error = 0;
>  
> -	if (!parent->p)
> +	if (!parent || !parent->p)
>  		return 0;
>  
>  	klist_iter_init_node(&parent->p->klist_children, &i,
> 

Re: [PATCH v3 7/9] driver core: Correct parameter check for API device_for_each_child_reverse_from()

[Zijun Hu]

On 2024/12/16 23:23, Jonathan Cameron wrote:
>> Correct the API's parameter @parent check by (!parent || !parent->p).
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> Given that 'from' implies continuation of an iteration I can see why
> it might not ever be relevant to check parent.  It's harmless, but to
> my mind unnecessary.

the extra check !parent has no overhead and make this check consistent
with others in the cluster. (^^)

[PATCH v3 8/9] driver core: Correct API device_for_each_child_reverse_from() prototype

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

For API device_for_each_child_reverse_from(..., const void *data,
		int (*fn)(struct device *dev, const void *data))

- Type of @data is const pointer, and means caller's data @*data is not
  allowed to be modified, but that usually is not proper for such non
  finding device iterating API.

- Types for both @data and @fn are not consistent with all other
  for_each device iterating APIs device_for_each_child(_reverse)(),
  bus_for_each_dev() and (driver|class)_for_each_device().

Correct its prototype by removing const from parameter types, then adapt
for various existing usages.

An dedicated typedef device_iter_t will be introduced as @fn() type for
various for_each device interating APIs later.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/base/core.c       | 4 ++--
 drivers/cxl/core/hdm.c    | 2 +-
 drivers/cxl/core/region.c | 2 +-
 include/linux/device.h    | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 34fb13f914b3db47e6a047fdabf3c9b18ecc08cc..7be9c732bec1b060a477b362f555c3e87c8c7b91 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -4043,8 +4043,8 @@ EXPORT_SYMBOL_GPL(device_for_each_child_reverse);
  * device_for_each_child_reverse_from();
  */
 int device_for_each_child_reverse_from(struct device *parent,
-				       struct device *from, const void *data,
-				       int (*fn)(struct device *, const void *))
+				       struct device *from, void *data,
+				       int (*fn)(struct device *, void *))
 {
 	struct klist_iter i;
 	struct device *child;
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 28edd5822486851912393f066478252b20abc19d..50e6a45b30ba260c066a0781b9ed3a2f6f3462d7 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -703,7 +703,7 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
 	return 0;
 }
 
-static int commit_reap(struct device *dev, const void *data)
+static int commit_reap(struct device *dev, void *data)
 {
 	struct cxl_port *port = to_cxl_port(dev->parent);
 	struct cxl_decoder *cxld;
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index bfecd71040c2f4373645380b4c31327d8b42d095..a4cff4c266e7a7dd6a3feb1513bf14b7d3f9afa2 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -778,7 +778,7 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
 	return rc;
 }
 
-static int check_commit_order(struct device *dev, const void *data)
+static int check_commit_order(struct device *dev, void *data)
 {
 	struct cxl_decoder *cxld = to_cxl_decoder(dev);
 
diff --git a/include/linux/device.h b/include/linux/device.h
index a9d928398895b062094b94f2c188cbe9951d7ac1..025bac08fca7b2513acb1fbcb666be1dc64f03d1 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1079,8 +1079,8 @@ int device_for_each_child(struct device *parent, void *data,
 int device_for_each_child_reverse(struct device *parent, void *data,
 				  int (*fn)(struct device *dev, void *data));
 int device_for_each_child_reverse_from(struct device *parent,
-				       struct device *from, const void *data,
-				       int (*fn)(struct device *, const void *));
+				       struct device *from, void *data,
+				       int (*fn)(struct device *, void *));
 struct device *device_find_child(struct device *parent, const void *data,
 				 device_match_t match);
 struct device *device_find_child_by_name(struct device *parent,

-- 
2.34.1

Re: [PATCH v3 8/9] driver core: Correct API device_for_each_child_reverse_from() prototype

[Jonathan Cameron]

On Thu, 12 Dec 2024 21:38:44 +0800
Zijun Hu <zijun_hu@icloud.com> wrote:

> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> For API device_for_each_child_reverse_from(..., const void *data,
> 		int (*fn)(struct device *dev, const void *data))
> 
> - Type of @data is const pointer, and means caller's data @*data is not
>   allowed to be modified, but that usually is not proper for such non
>   finding device iterating API.
> 
> - Types for both @data and @fn are not consistent with all other
>   for_each device iterating APIs device_for_each_child(_reverse)(),
>   bus_for_each_dev() and (driver|class)_for_each_device().
> 
> Correct its prototype by removing const from parameter types, then adapt
> for various existing usages.
> 
> An dedicated typedef device_iter_t will be introduced as @fn() type for
> various for_each device interating APIs later.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
This inconsistency is indeed weird.  So this changes seems reasonable to me.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Dan, this was your function. There is nothing about the const
in the original commit.  Did you intend it to be different from
device_for_each_child_reverse() etc?

Jonathan

> ---
>  drivers/base/core.c       | 4 ++--
>  drivers/cxl/core/hdm.c    | 2 +-
>  drivers/cxl/core/region.c | 2 +-
>  include/linux/device.h    | 4 ++--
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 34fb13f914b3db47e6a047fdabf3c9b18ecc08cc..7be9c732bec1b060a477b362f555c3e87c8c7b91 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4043,8 +4043,8 @@ EXPORT_SYMBOL_GPL(device_for_each_child_reverse);
>   * device_for_each_child_reverse_from();
>   */
>  int device_for_each_child_reverse_from(struct device *parent,
> -				       struct device *from, const void *data,
> -				       int (*fn)(struct device *, const void *))
> +				       struct device *from, void *data,
> +				       int (*fn)(struct device *, void *))
>  {
>  	struct klist_iter i;
>  	struct device *child;
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 28edd5822486851912393f066478252b20abc19d..50e6a45b30ba260c066a0781b9ed3a2f6f3462d7 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -703,7 +703,7 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
>  	return 0;
>  }
>  
> -static int commit_reap(struct device *dev, const void *data)
> +static int commit_reap(struct device *dev, void *data)
>  {
>  	struct cxl_port *port = to_cxl_port(dev->parent);
>  	struct cxl_decoder *cxld;
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index bfecd71040c2f4373645380b4c31327d8b42d095..a4cff4c266e7a7dd6a3feb1513bf14b7d3f9afa2 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -778,7 +778,7 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
>  	return rc;
>  }
>  
> -static int check_commit_order(struct device *dev, const void *data)
> +static int check_commit_order(struct device *dev, void *data)
>  {
>  	struct cxl_decoder *cxld = to_cxl_decoder(dev);
>  
> diff --git a/include/linux/device.h b/include/linux/device.h
> index a9d928398895b062094b94f2c188cbe9951d7ac1..025bac08fca7b2513acb1fbcb666be1dc64f03d1 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1079,8 +1079,8 @@ int device_for_each_child(struct device *parent, void *data,
>  int device_for_each_child_reverse(struct device *parent, void *data,
>  				  int (*fn)(struct device *dev, void *data));
>  int device_for_each_child_reverse_from(struct device *parent,
> -				       struct device *from, const void *data,
> -				       int (*fn)(struct device *, const void *));
> +				       struct device *from, void *data,
> +				       int (*fn)(struct device *, void *));
>  struct device *device_find_child(struct device *parent, const void *data,
>  				 device_match_t match);
>  struct device *device_find_child_by_name(struct device *parent,
> 

[PATCH v3 9/9] driver core: Introduce device_iter_t for device iterating APIs

[Zijun Hu]

From: Zijun Hu <quic_zijuhu@quicinc.com>

There are several for_each APIs which has parameter with type below:
int (*fn)(struct device *dev, void *data)
They iterate over various device lists and call @fn() for each device
with caller provided data @*data, and they usually need to modify @*data.

Give the type an dedicated typedef with advantages shown below:
typedef int (*device_iter_t)(struct device *dev, void *data)

- Shorter API declarations and definitions
- Prevent further for_each APIs from using bad parameter type

So introduce device_iter_t and apply it to various existing APIs below:
bus_for_each_dev()
(class|driver)_for_each_device()
device_for_each_child(_reverse|_reverse_from)().

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/base/bus.c            | 2 +-
 drivers/base/class.c          | 2 +-
 drivers/base/core.c           | 6 +++---
 drivers/base/driver.c         | 2 +-
 include/linux/device.h        | 6 +++---
 include/linux/device/bus.h    | 7 +++++--
 include/linux/device/class.h  | 4 ++--
 include/linux/device/driver.h | 2 +-
 8 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 73a56f376d3a05962ce0931a2fe8b4d8839157f2..6b9e65a42cd2e12046ddabf2d8dfb209d513f7da 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -354,7 +354,7 @@ static struct device *next_device(struct klist_iter *i)
  * count in the supplied callback.
  */
 int bus_for_each_dev(const struct bus_type *bus, struct device *start,
-		     void *data, int (*fn)(struct device *, void *))
+		     void *data, device_iter_t fn)
 {
 	struct subsys_private *sp = bus_to_subsys(bus);
 	struct klist_iter i;
diff --git a/drivers/base/class.c b/drivers/base/class.c
index d57f277978dc9033fba3484b4620bcf884a4029f..70ee6a7ba5a3746b5a182c6101b5c085b424d01d 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -402,7 +402,7 @@ EXPORT_SYMBOL_GPL(class_dev_iter_exit);
  * code.  There's no locking restriction.
  */
 int class_for_each_device(const struct class *class, const struct device *start,
-			  void *data, int (*fn)(struct device *, void *))
+			  void *data, device_iter_t fn)
 {
 	struct subsys_private *sp = class_to_subsys(class);
 	struct class_dev_iter iter;
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 7be9c732bec1b060a477b362f555c3e87c8c7b91..930e43a970952b20cd1c71856bdef889698f51b4 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3980,7 +3980,7 @@ const char *device_get_devnode(const struct device *dev,
  * other than 0, we break out and return that value.
  */
 int device_for_each_child(struct device *parent, void *data,
-			  int (*fn)(struct device *dev, void *data))
+			  device_iter_t fn)
 {
 	struct klist_iter i;
 	struct device *child;
@@ -4010,7 +4010,7 @@ EXPORT_SYMBOL_GPL(device_for_each_child);
  * other than 0, we break out and return that value.
  */
 int device_for_each_child_reverse(struct device *parent, void *data,
-				  int (*fn)(struct device *dev, void *data))
+				  device_iter_t fn)
 {
 	struct klist_iter i;
 	struct device *child;
@@ -4044,7 +4044,7 @@ EXPORT_SYMBOL_GPL(device_for_each_child_reverse);
  */
 int device_for_each_child_reverse_from(struct device *parent,
 				       struct device *from, void *data,
-				       int (*fn)(struct device *, void *))
+				       device_iter_t fn)
 {
 	struct klist_iter i;
 	struct device *child;
diff --git a/drivers/base/driver.c b/drivers/base/driver.c
index 6f033a741aa7ce6138d1c61e49e72b2a3eb85e06..8ab010ddf709a2b173cfd0c18610a122e58a2f4c 100644
--- a/drivers/base/driver.c
+++ b/drivers/base/driver.c
@@ -115,7 +115,7 @@ EXPORT_SYMBOL_GPL(driver_set_override);
  * Iterate over the @drv's list of devices calling @fn for each one.
  */
 int driver_for_each_device(struct device_driver *drv, struct device *start,
-			   void *data, int (*fn)(struct device *, void *))
+			   void *data, device_iter_t fn)
 {
 	struct klist_iter i;
 	struct device *dev;
diff --git a/include/linux/device.h b/include/linux/device.h
index 025bac08fca7b2513acb1fbcb666be1dc64f03d1..36d1a1607712f5a6b0668ac02a6cf6b2d0651a2d 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1075,12 +1075,12 @@ void device_del(struct device *dev);
 DEFINE_FREE(device_del, struct device *, if (_T) device_del(_T))
 
 int device_for_each_child(struct device *parent, void *data,
-			  int (*fn)(struct device *dev, void *data));
+			  device_iter_t fn);
 int device_for_each_child_reverse(struct device *parent, void *data,
-				  int (*fn)(struct device *dev, void *data));
+				  device_iter_t fn);
 int device_for_each_child_reverse_from(struct device *parent,
 				       struct device *from, void *data,
-				       int (*fn)(struct device *, void *));
+				       device_iter_t fn);
 struct device *device_find_child(struct device *parent, const void *data,
 				 device_match_t match);
 struct device *device_find_child_by_name(struct device *parent,
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
index bc3fd74bb763e6d2d862859bd2ec3f0d443f2d7a..3d3517da41a141aeadc8f219a44fd360cfd931ff 100644
--- a/include/linux/device/bus.h
+++ b/include/linux/device/bus.h
@@ -139,9 +139,12 @@ int device_match_acpi_dev(struct device *dev, const void *adev);
 int device_match_acpi_handle(struct device *dev, const void *handle);
 int device_match_any(struct device *dev, const void *unused);
 
+/* Device iterating function type for various driver core for_each APIs */
+typedef int (*device_iter_t)(struct device *dev, void *data);
+
 /* iterator helpers for buses */
-int bus_for_each_dev(const struct bus_type *bus, struct device *start, void *data,
-		     int (*fn)(struct device *dev, void *data));
+int bus_for_each_dev(const struct bus_type *bus, struct device *start,
+		     void *data, device_iter_t fn);
 struct device *bus_find_device(const struct bus_type *bus, struct device *start,
 			       const void *data, device_match_t match);
 /**
diff --git a/include/linux/device/class.h b/include/linux/device/class.h
index 518c9c83d64bdf85bb5607cea6ea640884ec3460..aa67d473681612f24a4569bf82fe5f91237d5a50 100644
--- a/include/linux/device/class.h
+++ b/include/linux/device/class.h
@@ -92,8 +92,8 @@ void class_dev_iter_init(struct class_dev_iter *iter, const struct class *class,
 struct device *class_dev_iter_next(struct class_dev_iter *iter);
 void class_dev_iter_exit(struct class_dev_iter *iter);
 
-int class_for_each_device(const struct class *class, const struct device *start, void *data,
-			  int (*fn)(struct device *dev, void *data));
+int class_for_each_device(const struct class *class, const struct device *start,
+			  void *data, device_iter_t fn);
 struct device *class_find_device(const struct class *class, const struct device *start,
 				 const void *data, device_match_t match);
 
diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
index 5c04b8e3833b995f9fd4d65b8732b3dfce2eba7e..cd8e0f0a634be9ea63ff22e89d66ada3b1a9eaf2 100644
--- a/include/linux/device/driver.h
+++ b/include/linux/device/driver.h
@@ -154,7 +154,7 @@ void driver_remove_file(const struct device_driver *driver,
 int driver_set_override(struct device *dev, const char **override,
 			const char *s, size_t len);
 int __must_check driver_for_each_device(struct device_driver *drv, struct device *start,
-					void *data, int (*fn)(struct device *dev, void *));
+					void *data, device_iter_t fn);
 struct device *driver_find_device(const struct device_driver *drv,
 				  struct device *start, const void *data,
 				  device_match_t match);

-- 
2.34.1

Re: [PATCH v3 9/9] driver core: Introduce device_iter_t for device iterating APIs

[Jonathan Cameron]

On Thu, 12 Dec 2024 21:38:45 +0800
Zijun Hu <zijun_hu@icloud.com> wrote:

> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> There are several for_each APIs which has parameter with type below:
> int (*fn)(struct device *dev, void *data)
> They iterate over various device lists and call @fn() for each device
> with caller provided data @*data, and they usually need to modify @*data.
> 
> Give the type an dedicated typedef with advantages shown below:
> typedef int (*device_iter_t)(struct device *dev, void *data)
> 
> - Shorter API declarations and definitions
> - Prevent further for_each APIs from using bad parameter type
> 
> So introduce device_iter_t and apply it to various existing APIs below:
> bus_for_each_dev()
> (class|driver)_for_each_device()
> device_for_each_child(_reverse|_reverse_from)().
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>

I'd normally argue this wasn't worth the effort, but given you tidied
up one inconsistent case in this series, fair enough as far as I'm concerned.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>