[PATCH v1] devres: Initialize a uninitialized struct member

[PATCH v1] devres: Initialize a uninitialized struct member

[Zijun Hu]

Use memset() after kmalloc() a struct devres_group to initialize
potential uninitialized members such as @color within kernel API
devres_open_group() as alloc_dr() does.

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

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 3df0025d12aa..ba3e4603cd77 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -558,6 +558,10 @@ void * devres_open_group(struct device *dev, void *id, gfp_t gfp)
 	if (unlikely(!grp))
 		return NULL;
 
+	/* No need to clear memory twice */
+	if (!(gfp & __GFP_ZERO))
+		memset(grp, 0, sizeof(*grp));
+
 	grp->node[0].release = &group_open_release;
 	grp->node[1].release = &group_close_release;
 	INIT_LIST_HEAD(&grp->node[0].entry);
-- 
2.7.4

Re: [PATCH v1] devres: Initialize a uninitialized struct member

[Greg KH]

On Mon, Jun 17, 2024 at 09:09:25PM +0800, Zijun Hu wrote:
> Use memset() after kmalloc() a struct devres_group to initialize
> potential uninitialized members such as @color within kernel API
> devres_open_group() as alloc_dr() does.
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/base/devres.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index 3df0025d12aa..ba3e4603cd77 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -558,6 +558,10 @@ void * devres_open_group(struct device *dev, void *id, gfp_t gfp)
>  	if (unlikely(!grp))
>  		return NULL;
>  
> +	/* No need to clear memory twice */
> +	if (!(gfp & __GFP_ZERO))
> +		memset(grp, 0, sizeof(*grp));

Is this an actual bugfix (i.e. do we have uninitialized fields today?)

If so, what commit does this fix?

thanks,

greg k-h

Re: [PATCH v1] devres: Initialize a uninitialized struct member

[quic_zijuhu]

On 6/18/2024 2:14 AM, Greg KH wrote:
> On Mon, Jun 17, 2024 at 09:09:25PM +0800, Zijun Hu wrote:
>> Use memset() after kmalloc() a struct devres_group to initialize
>> potential uninitialized members such as @color within kernel API
>> devres_open_group() as alloc_dr() does.
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/base/devres.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
>> index 3df0025d12aa..ba3e4603cd77 100644
>> --- a/drivers/base/devres.c
>> +++ b/drivers/base/devres.c
>> @@ -558,6 +558,10 @@ void * devres_open_group(struct device *dev, void *id, gfp_t gfp)
>>  	if (unlikely(!grp))
>>  		return NULL;
>>  
>> +	/* No need to clear memory twice */
>> +	if (!(gfp & __GFP_ZERO))
>> +		memset(grp, 0, sizeof(*grp));
> 
> Is this an actual bugfix (i.e. do we have uninitialized fields today?)
> 
no, maybe take it as code optimization instead of bugfix.
yes, field grp->color is NOT initialized.
> If so, what commit does this fix?
> 
> thanks,
> 
> greg k-h

[PATCH] devres: Simple code optimization

[Zijun Hu]

Initialize an uninitialized struct member for devres_open_group()
and simplify devm_percpu_match() implementation.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
This change is intend to replace below one:
https://lore.kernel.org/lkml/1718629765-32720-1-git-send-email-quic_zijuhu@quicinc.com/#t

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

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 3df0025d12aa..5b1d498e83ab 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]);
@@ -1172,9 +1173,9 @@ 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);
+	void __percpu *ptr = *(void __percpu **)data;
 
-	return *(void **)devr->data == p;
+	return ptr == (void __percpu *)p;
 }
 
 /**
-- 
2.7.4

Re: [PATCH] devres: Simple code optimization

[Greg KH]

On Thu, Jun 27, 2024 at 09:47:16PM +0800, Zijun Hu wrote:
> Initialize an uninitialized struct member for devres_open_group()
> and simplify devm_percpu_match() implementation.

Huge hint, when you say "and" or "also" in a patch, it's a good idea to
split it up into different commits, right?

> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
> This change is intend to replace below one:
> https://lore.kernel.org/lkml/1718629765-32720-1-git-send-email-quic_zijuhu@quicinc.com/#t

Why?  SHouldn't this be v2 instead?

>  drivers/base/devres.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index 3df0025d12aa..5b1d498e83ab 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]);
> @@ -1172,9 +1173,9 @@ 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);
> +	void __percpu *ptr = *(void __percpu **)data;
>  
> -	return *(void **)devr->data == p;
> +	return ptr == (void __percpu *)p;

What exactly is being "optimized" here?

And where did the container_of go?  You just lost all type-safeness.

thanks,

greg k-h

Re: [PATCH] devres: Simple code optimization

[quic_zijuhu]

On 6/27/2024 9:54 PM, Greg KH wrote:
> On Thu, Jun 27, 2024 at 09:47:16PM +0800, Zijun Hu wrote:
>> Initialize an uninitialized struct member for devres_open_group()
>> and simplify devm_percpu_match() implementation.
> 
> Huge hint, when you say "and" or "also" in a patch, it's a good idea to
> split it up into different commits, right?
> 
you are right.
i would like to split this change into two changes within a patchset
even if this change is *very* simple.
>>
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>> This change is intend to replace below one:
>> https://lore.kernel.org/lkml/1718629765-32720-1-git-send-email-quic_zijuhu@quicinc.com/#t
> 
> Why?  SHouldn't this be v2 instead?
> 
this change has different title and maybe be identified as different
patch, so i send it as v1.
>>  drivers/base/devres.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
>> index 3df0025d12aa..5b1d498e83ab 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]);
>> @@ -1172,9 +1173,9 @@ 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);
>> +	void __percpu *ptr = *(void __percpu **)data;
>>  
>> -	return *(void **)devr->data == p;
>> +	return ptr == (void __percpu *)p;
> 
> What exactly is being "optimized" here?
> 
1) remove redundant container_of() and devr->data operations
   pointer parameter @data already is address of devr->data.
2) compare with right data type
    original type of @p is void __percpu * returned by
__devm_alloc_percpu().

@data is storing a pointer type void __percpu * as shown by below
statement within __devm_alloc_percpu().
*(void __percpu **)p = pcpu;

> And where did the container_of go?  You just lost all type-safeness.
> 
see above comments 1) and 2).
> thanks,
> 
> greg k-h

Re: [PATCH] devres: Simple code optimization

[Greg KH]

On Thu, Jun 27, 2024 at 10:29:43PM +0800, quic_zijuhu wrote:
> On 6/27/2024 9:54 PM, Greg KH wrote:
> > On Thu, Jun 27, 2024 at 09:47:16PM +0800, Zijun Hu wrote:
> >> Initialize an uninitialized struct member for devres_open_group()
> >> and simplify devm_percpu_match() implementation.
> > 
> > Huge hint, when you say "and" or "also" in a patch, it's a good idea to
> > split it up into different commits, right?
> > 
> you are right.
> i would like to split this change into two changes within a patchset
> even if this change is *very* simple.
> >>
> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> >> ---
> >> This change is intend to replace below one:
> >> https://lore.kernel.org/lkml/1718629765-32720-1-git-send-email-quic_zijuhu@quicinc.com/#t
> > 
> > Why?  SHouldn't this be v2 instead?
> > 
> this change has different title and maybe be identified as different
> patch, so i send it as v1.
> >>  drivers/base/devres.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> >> index 3df0025d12aa..5b1d498e83ab 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]);
> >> @@ -1172,9 +1173,9 @@ 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);
> >> +	void __percpu *ptr = *(void __percpu **)data;
> >>  
> >> -	return *(void **)devr->data == p;
> >> +	return ptr == (void __percpu *)p;
> > 
> > What exactly is being "optimized" here?
> > 
> 1) remove redundant container_of() and devr->data operations
>    pointer parameter @data already is address of devr->data.

But do we really know that ahead of time?  If so, how, just by virtue of
this being the first field?  If so, then no, keep the container_of.

> 2) compare with right data type
>     original type of @p is void __percpu * returned by
> __devm_alloc_percpu().

It's pointer math, no need for types, right?

> @data is storing a pointer type void __percpu * as shown by below
> statement within __devm_alloc_percpu().
> *(void __percpu **)p = pcpu;

Again, it's not very obvious so you better document the heck out of it
in your changelog text.

thanks,

greg k-h

Re: [PATCH] devres: Simple code optimization

[quic_zijuhu]

On 6/27/2024 10:35 PM, Greg KH wrote:
> On Thu, Jun 27, 2024 at 10:29:43PM +0800, quic_zijuhu wrote:
>> On 6/27/2024 9:54 PM, Greg KH wrote:
>>> On Thu, Jun 27, 2024 at 09:47:16PM +0800, Zijun Hu wrote:
>>>> Initialize an uninitialized struct member for devres_open_group()
>>>> and simplify devm_percpu_match() implementation.
>>>
>>> Huge hint, when you say "and" or "also" in a patch, it's a good idea to
>>> split it up into different commits, right?
>>>
>> you are right.
>> i would like to split this change into two changes within a patchset
>> even if this change is *very* simple.
>>>>
>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>> ---
>>>> This change is intend to replace below one:
>>>> https://lore.kernel.org/lkml/1718629765-32720-1-git-send-email-quic_zijuhu@quicinc.com/#t
>>>
>>> Why?  SHouldn't this be v2 instead?
>>>
>> this change has different title and maybe be identified as different
>> patch, so i send it as v1.
>>>>  drivers/base/devres.c | 5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
>>>> index 3df0025d12aa..5b1d498e83ab 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]);
>>>> @@ -1172,9 +1173,9 @@ 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);
>>>> +	void __percpu *ptr = *(void __percpu **)data;
>>>>  
>>>> -	return *(void **)devr->data == p;
>>>> +	return ptr == (void __percpu *)p;
>>>
>>> What exactly is being "optimized" here?
>>>
>> 1) remove redundant container_of() and devr->data operations
>>    pointer parameter @data already is address of devr->data.
> 
> But do we really know that ahead of time?  If so, how, just by virtue of
> this being the first field?  If so, then no, keep the container_of.
> 
yes. the 2nd parameter for match() must be devr->data by below reasons:
1) devres.c only call match() by this way match(dev, dr->data, match_data).
2) all implements of match() don't do such redundant operations to get
dr->data. such as devm_action_match()/devm_pages_match()/....
3) API user should only know address devr->data and known nothing about
devres internal struct devres. so they should not write their match() by
involving the struct.

for below match() type definition, the 1st parameter @dev have already
have fixed meaning.
typedef int (*dr_match_t)(struct device *dev, void *res, void *match_data);

suppose your 3rd question have typo error.
>> 2) compare with right data type
>>     original type of @p is void __percpu * returned by
>> __devm_alloc_percpu().
> 
> It's pointer math, no need for types, right?
> 
yes, it is more simpler for no need for types.
but it think it is more normative to compare with user original types as
this change do.
>> @data is storing a pointer type void __percpu * as shown by below
>> statement within __devm_alloc_percpu().
>> *(void __percpu **)p = pcpu;
> 
> Again, it's not very obvious so you better document the heck out of it
> in your changelog text.
> 
okay, will add comments after code review done.
> thanks,
> 
> greg k-h