[PATCH v1 0/5] Collection of my bug fixes and code optimizations for devres

[PATCH v1 0/5] Collection of my bug fixes and code optimizations for devres

[Zijun Hu]

This patch series is to collect all of my recent patches for devres. These
patches were sent separately for different bug fixes or code optimizations
but none of them have been merged yet. Therefore, I would like to collect
them together in order to manage them more easily.

Zijun Hu (5):
  devres: Fix devm_krealloc() wasting memory
  devres: Fix memory leakage caused by driver API devm_free_percpu()
  devres: Initialize an uninitialized struct member
  devres: Simplify devm_percpu_match() implementation
  devres: Correct code style for functions that return a pointer type

 drivers/base/devres.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)


base-commit: 6b521fc111a2ad5ead39776960d3d2d289ce0722
-- 
2.34.1

[PATCH v1 1/5] devres: Fix devm_krealloc() wasting memory

[Zijun Hu]

Driver API devm_krealloc() calls alloc_dr() with wrong argument
@total_new_size, so causes more memory to be allocated than required
fix this memory waste by using @new_size as the argument for alloc_dr().

Fixes: f82485722e5d ("devres: provide devm_krealloc()")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Previous discussion link:
https://lore.kernel.org/all/1718531655-29761-1-git-send-email-quic_zijuhu@quicinc.com/

Changes since the original one:
 - Correct tile and commit message
 - Add inline comments and stable tag

 drivers/base/devres.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 3df0025d12aa..ff2247eec43c 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -896,9 +896,12 @@ void *devm_krealloc(struct device *dev, void *ptr, size_t new_size, gfp_t gfp)
 	/*
 	 * Otherwise: allocate new, larger chunk. We need to allocate before
 	 * taking the lock as most probably the caller uses GFP_KERNEL.
+	 * alloc_dr() will call check_dr_size() to reserve extra memory
+	 * for struct devres automatically, so size @new_size user request
+	 * is delivered to it directly as devm_kmalloc() does.
 	 */
 	new_dr = alloc_dr(devm_kmalloc_release,
-			  total_new_size, gfp, dev_to_node(dev));
+			  new_size, gfp, dev_to_node(dev));
 	if (!new_dr)
 		return NULL;
 
-- 
2.34.1

[PATCH v1 2/5] devres: Fix memory leakage caused by driver API devm_free_percpu()

[Zijun Hu]

It will cause memory leakage when use driver API devm_free_percpu()
to free memory allocated by devm_alloc_percpu(), fixed by using
devres_release() instead of devres_destroy() within devm_free_percpu().

Fixes: ff86aae3b411 ("devres: add devm_alloc_percpu()")
Cc: stable@vger.kernel.org
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Previous discussion link:
https://lore.kernel.org/lkml/1718804281-1796-1-git-send-email-quic_zijuhu@quicinc.com/

 drivers/base/devres.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index ff2247eec43c..8d709dbd4e0c 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -1225,7 +1225,11 @@ EXPORT_SYMBOL_GPL(__devm_alloc_percpu);
  */
 void devm_free_percpu(struct device *dev, void __percpu *pdata)
 {
-	WARN_ON(devres_destroy(dev, devm_percpu_release, devm_percpu_match,
+	/*
+	 * Use devres_release() to prevent memory leakage as
+	 * devm_free_pages() does.
+	 */
+	WARN_ON(devres_release(dev, devm_percpu_release, devm_percpu_match,
 			       (__force void *)pdata));
 }
 EXPORT_SYMBOL_GPL(devm_free_percpu);
-- 
2.34.1

[PATCH v1 3/5] devres: Initialize an uninitialized struct member

[Zijun Hu]

Initialize an uninitialized struct member for driver API
devres_open_group().

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Previous discussion link:
https://lore.kernel.org/lkml/1718629765-32720-1-git-send-email-quic_zijuhu@quicinc.com/

Changes since the original one:
 - Simplify fix by use = instead of memset() and commit message

 drivers/base/devres.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 8d709dbd4e0c..e9b0d94aeabd 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -567,6 +567,7 @@ void * devres_open_group(struct device *dev, void *id, gfp_t gfp)
 	grp->id = grp;
 	if (id)
 		grp->id = id;
+	grp->color = 0;
 
 	spin_lock_irqsave(&dev->devres_lock, flags);
 	add_dr(dev, &grp->node[0]);
-- 
2.34.1

[PATCH v1 4/5] devres: Simplify devm_percpu_match() implementation

[Zijun Hu]

Simplify devm_percpu_match() implementation by removing redundant
conversions.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
Previous discussion link:
https://lore.kernel.org/lkml/1719496036-24642-1-git-send-email-quic_zijuhu@quicinc.com/

Changes since the original one:
 - Select the simplier solution

 drivers/base/devres.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index e9b0d94aeabd..2ad6dacb3472 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -1176,9 +1176,8 @@ static void devm_percpu_release(struct device *dev, void *pdata)
 
 static int devm_percpu_match(struct device *dev, void *data, void *p)
 {
-	struct devres *devr = container_of(data, struct devres, data);
-
-	return *(void **)devr->data == p;
+	/* @data is already and must be (void *)devr->data */
+	return *(void **)data == p;
 }
 
 /**
-- 
2.34.1

[PATCH v1 5/5] devres: Correct code style for functions that return a pointer type

[Zijun Hu]

Correct code style for several functions that return a pointer type.

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

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 2ad6dacb3472..be82d2d71544 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -85,7 +85,7 @@ static void group_close_release(struct device *dev, void *res)
 	/* noop */
 }
 
-static struct devres_group * node_to_group(struct devres_node *node)
+static struct devres_group *node_to_group(struct devres_node *node)
 {
 	if (node->release == &group_open_release)
 		return container_of(node, struct devres_group, node[0]);
@@ -107,8 +107,8 @@ static bool check_dr_size(size_t size, size_t *tot_size)
 	return true;
 }
 
-static __always_inline struct devres * alloc_dr(dr_release_t release,
-						size_t size, gfp_t gfp, int nid)
+static __always_inline struct devres *alloc_dr(dr_release_t release,
+					       size_t size, gfp_t gfp, int nid)
 {
 	size_t tot_size;
 	struct devres *dr;
@@ -283,8 +283,8 @@ static struct devres *find_dr(struct device *dev, dr_release_t release,
  * RETURNS:
  * Pointer to found devres, NULL if not found.
  */
-void * devres_find(struct device *dev, dr_release_t release,
-		   dr_match_t match, void *match_data)
+void *devres_find(struct device *dev, dr_release_t release,
+		  dr_match_t match, void *match_data)
 {
 	struct devres *dr;
 	unsigned long flags;
@@ -313,8 +313,8 @@ EXPORT_SYMBOL_GPL(devres_find);
  * RETURNS:
  * Pointer to found or added devres.
  */
-void * devres_get(struct device *dev, void *new_res,
-		  dr_match_t match, void *match_data)
+void *devres_get(struct device *dev, void *new_res,
+		 dr_match_t match, void *match_data)
 {
 	struct devres *new_dr = container_of(new_res, struct devres, data);
 	struct devres *dr;
@@ -349,8 +349,8 @@ EXPORT_SYMBOL_GPL(devres_get);
  * RETURNS:
  * Pointer to removed devres on success, NULL if not found.
  */
-void * devres_remove(struct device *dev, dr_release_t release,
-		     dr_match_t match, void *match_data)
+void *devres_remove(struct device *dev, dr_release_t release,
+		    dr_match_t match, void *match_data)
 {
 	struct devres *dr;
 	unsigned long flags;
@@ -549,7 +549,7 @@ int devres_release_all(struct device *dev)
  * RETURNS:
  * ID of the new group, NULL on failure.
  */
-void * devres_open_group(struct device *dev, void *id, gfp_t gfp)
+void *devres_open_group(struct device *dev, void *id, gfp_t gfp)
 {
 	struct devres_group *grp;
 	unsigned long flags;
@@ -577,7 +577,7 @@ void * devres_open_group(struct device *dev, void *id, gfp_t gfp)
 EXPORT_SYMBOL_GPL(devres_open_group);
 
 /* Find devres group with ID @id.  If @id is NULL, look for the latest. */
-static struct devres_group * find_group(struct device *dev, void *id)
+static struct devres_group *find_group(struct device *dev, void *id)
 {
 	struct devres_node *node;
 
-- 
2.34.1

Re: [PATCH v1 4/5] devres: Simplify devm_percpu_match() implementation

[Greg KH]

On Tue, Jul 02, 2024 at 10:51:53PM +0800, Zijun Hu wrote:
> Simplify devm_percpu_match() implementation by removing redundant
> conversions.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
> Previous discussion link:
> https://lore.kernel.org/lkml/1719496036-24642-1-git-send-email-quic_zijuhu@quicinc.com/
> 
> Changes since the original one:
>  - Select the simplier solution
> 
>  drivers/base/devres.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index e9b0d94aeabd..2ad6dacb3472 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -1176,9 +1176,8 @@ static void devm_percpu_release(struct device *dev, void *pdata)
>  
>  static int devm_percpu_match(struct device *dev, void *data, void *p)
>  {
> -	struct devres *devr = container_of(data, struct devres, data);
> -
> -	return *(void **)devr->data == p;
> +	/* @data is already and must be (void *)devr->data */
> +	return *(void **)data == p;

The compiler output should be identical here, right?  And container_of()
enforces the placement so I'd prefer the original here as a comment
isn't going to enforce anything :)

thanks,

greg k-h

Re: [PATCH v1 4/5] devres: Simplify devm_percpu_match() implementation

[quic_zijuhu]

On 7/4/2024 6:34 PM, Greg KH wrote:
> On Tue, Jul 02, 2024 at 10:51:53PM +0800, Zijun Hu wrote:
>> Simplify devm_percpu_match() implementation by removing redundant
>> conversions.
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>> Previous discussion link:
>> https://lore.kernel.org/lkml/1719496036-24642-1-git-send-email-quic_zijuhu@quicinc.com/
>>
>> Changes since the original one:
>>  - Select the simplier solution
>>
>>  drivers/base/devres.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
>> index e9b0d94aeabd..2ad6dacb3472 100644
>> --- a/drivers/base/devres.c
>> +++ b/drivers/base/devres.c
>> @@ -1176,9 +1176,8 @@ static void devm_percpu_release(struct device *dev, void *pdata)
>>  
>>  static int devm_percpu_match(struct device *dev, void *data, void *p)
>>  {
>> -	struct devres *devr = container_of(data, struct devres, data);
>> -
>> -	return *(void **)devr->data == p;
>> +	/* @data is already and must be (void *)devr->data */
>> +	return *(void **)data == p;
> 
> The compiler output should be identical here, right?  And container_of()
yes, you are right.
> enforces the placement so I'd prefer the original here as a comment
> isn't going to enforce anything :)
> 
i would like to show 2 different points related to the comments below:

1) the comments is applicable for *ALL* kinds of devres match()s, and is
*NOT* specific to devm_percpu_match() and also does *NOT* enforce anything.

may i remove the comments?

2) the original implementation is *NOT* normative as explained below:
include/linux/device.h:
typedef int (*dr_match_t)(struct device *dev, void *res, void *match_data);
void *devres_find(struct device *dev, dr_release_t release,
		  dr_match_t match, void *match_data);

devres API users maybe need to write their match() functions, for
example, user of API devres_find(), but struct devres is a devres
internal implementation defined within devres.c and is *NOT* exposed to
API user by device.h, so API user should not use struct devres when
implement their devres match() normally.
but original devm_percpu_match() uses the struct devres.

> thanks,
> 
> greg k-h

Re: [PATCH v1 4/5] devres: Simplify devm_percpu_match() implementation

[quic_zijuhu]

On 7/4/2024 9:17 PM, quic_zijuhu wrote:
> On 7/4/2024 6:34 PM, Greg KH wrote:
>> On Tue, Jul 02, 2024 at 10:51:53PM +0800, Zijun Hu wrote:
>>> Simplify devm_percpu_match() implementation by removing redundant
>>> conversions.
>>>
>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>> ---
>>> Previous discussion link:
>>> https://lore.kernel.org/lkml/1719496036-24642-1-git-send-email-quic_zijuhu@quicinc.com/
>>>
>>> Changes since the original one:
>>>  - Select the simplier solution
>>>
>>>  drivers/base/devres.c | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
>>> index e9b0d94aeabd..2ad6dacb3472 100644
>>> --- a/drivers/base/devres.c
>>> +++ b/drivers/base/devres.c
>>> @@ -1176,9 +1176,8 @@ static void devm_percpu_release(struct device *dev, void *pdata)
>>>  
>>>  static int devm_percpu_match(struct device *dev, void *data, void *p)
>>>  {
>>> -	struct devres *devr = container_of(data, struct devres, data);
>>> -
>>> -	return *(void **)devr->data == p;
>>> +	/* @data is already and must be (void *)devr->data */
>>> +	return *(void **)data == p;
>>
>> The compiler output should be identical here, right?  And container_of()
> yes, you are right.
>> enforces the placement so I'd prefer the original here as a comment
>> isn't going to enforce anything :)
>>
> i would like to show 2 different points related to the comments below:
> 
> 1) the comments is applicable for *ALL* kinds of devres match()s, and is
> *NOT* specific to devm_percpu_match() and also does *NOT* enforce anything.
> 
> may i remove the comments?
> 
> 2) the original implementation is *NOT* normative as explained below:
> include/linux/device.h:
> typedef int (*dr_match_t)(struct device *dev, void *res, void *match_data);
> void *devres_find(struct device *dev, dr_release_t release,
> 		  dr_match_t match, void *match_data);
> 
> devres API users maybe need to write their match() functions, for
> example, user of API devres_find(), but struct devres is a devres
> internal implementation defined within devres.c and is *NOT* exposed to
> API user by device.h, so API user should not use struct devres when
> implement their devres match() normally.
> but original devm_percpu_match() uses the struct devres.
> 

thanks for your code review.
may i know which of below options is your final preference?

A) remain current kernel design, namely, no changes.

B) changes shown by this [PATCH v1 4/5]

C) below changes we ever discussed by below link
https://lore.kernel.org/all/1719496036-24642-1-git-send-email-quic_zijuhu@quicinc.com/

 static int devm_percpu_match(struct device *dev, void *data, void *p)
 {
-	struct devres *devr = container_of(data, struct devres, data);
+	void __percpu *ptr = *(void __percpu **)data;

-	return *(void **)devr->data == p;
+	return ptr == (void __percpu *)p;
 }

>> thanks,
>>
>> greg k-h
>