[PATCH v2 1/2] staging: rtl8712: Fix style issues in rtl871x_io.c

[PATCH v2 1/2] staging: rtl8712: Fix style issues in rtl871x_io.c

[Manisha Singh]

Remove multiple assignments from a line

CHECK: multiple assignments should be avoided
+	pintf_priv = pintf_hdl->pintfpriv = kmalloc(sizeof(struct intf_priv),

Refactor the _init_intf_hdl() function to avoid multiple
assignments in a single statement. This change improves code readability
and adheres to kernel coding style guidelines.

Signed-off-by: Manisha Singh <masingh.linux@gmail.com>
---
Changes Since V1:
	Broke the patch into 2 different fixes

 drivers/staging/rtl8712/rtl871x_io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_io.c b/drivers/staging/rtl8712/rtl871x_io.c
index 6789a4c98564..6311ac15c581 100644
--- a/drivers/staging/rtl8712/rtl871x_io.c
+++ b/drivers/staging/rtl8712/rtl871x_io.c
@@ -48,10 +48,10 @@ static uint _init_intf_hdl(struct _adapter *padapter,
 	set_intf_funs = &(r8712_usb_set_intf_funs);
 	set_intf_ops = &r8712_usb_set_intf_ops;
 	init_intf_priv = &r8712_usb_init_intf_priv;
-	pintf_priv = pintf_hdl->pintfpriv = kmalloc(sizeof(struct intf_priv),
-						    GFP_ATOMIC);
+	pintf_priv = kmalloc(sizeof(struct intf_priv), GFP_ATOMIC);
 	if (!pintf_priv)
 		goto _init_intf_hdl_fail;
+	pintf_hdl->pintfpriv = pintf_priv;
 	pintf_hdl->adapter = (u8 *)padapter;
 	set_intf_option(&pintf_hdl->intf_option);
 	set_intf_funs(pintf_hdl);
-- 
2.43.0

[PATCH v2 2/2] staging: rtl8712: Calculate size from pointer

[Manisha Singh]

Calculate the size from the pointer instead of struct

CHECK: Prefer kmalloc(sizeof(*pintf_hdl->pintfpriv)...) over
kmalloc(sizeof(struct intf_priv)...)

Signed-off-by: Manisha Singh <masingh.linux@gmail.com>
---
 drivers/staging/rtl8712/rtl871x_io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8712/rtl871x_io.c b/drivers/staging/rtl8712/rtl871x_io.c
index 6311ac15c581..8ad4647dad2f 100644
--- a/drivers/staging/rtl8712/rtl871x_io.c
+++ b/drivers/staging/rtl8712/rtl871x_io.c
@@ -48,7 +48,7 @@ static uint _init_intf_hdl(struct _adapter *padapter,
 	set_intf_funs = &(r8712_usb_set_intf_funs);
 	set_intf_ops = &r8712_usb_set_intf_ops;
 	init_intf_priv = &r8712_usb_init_intf_priv;
-	pintf_priv = kmalloc(sizeof(struct intf_priv), GFP_ATOMIC);
+	pintf_priv = kmalloc(sizeof(*pintf_priv), GFP_ATOMIC);
 	if (!pintf_priv)
 		goto _init_intf_hdl_fail;
 	pintf_hdl->pintfpriv = pintf_priv;
-- 
2.43.0

Re: [PATCH v2 2/2] staging: rtl8712: Calculate size from pointer

[Philipp Hortmann]

On 8/28/24 22:45, Manisha Singh wrote:
> Calculate the size from the pointer instead of struct
> 
Hi Manisha,

the first line of the description is good. Please just add the why (to 
adhere to kernel coding style?) and the "." to make it a full sentence.

Omit the next two lines.
> CHECK: Prefer kmalloc(sizeof(*pintf_hdl->pintfpriv)...) over
> kmalloc(sizeof(struct intf_priv)...)
> 

Thanks for your support.

Bye Philipp

> Signed-off-by: Manisha Singh <masingh.linux@gmail.com>
> ---
>   drivers/staging/rtl8712/rtl871x_io.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/rtl8712/rtl871x_io.c b/drivers/staging/rtl8712/rtl871x_io.c
> index 6311ac15c581..8ad4647dad2f 100644
> --- a/drivers/staging/rtl8712/rtl871x_io.c
> +++ b/drivers/staging/rtl8712/rtl871x_io.c
> @@ -48,7 +48,7 @@ static uint _init_intf_hdl(struct _adapter *padapter,
>   	set_intf_funs = &(r8712_usb_set_intf_funs);
>   	set_intf_ops = &r8712_usb_set_intf_ops;
>   	init_intf_priv = &r8712_usb_init_intf_priv;
> -	pintf_priv = kmalloc(sizeof(struct intf_priv), GFP_ATOMIC);
> +	pintf_priv = kmalloc(sizeof(*pintf_priv), GFP_ATOMIC);
>   	if (!pintf_priv)
>   		goto _init_intf_hdl_fail;
>   	pintf_hdl->pintfpriv = pintf_priv;

Re: [PATCH v2 2/2] staging: rtl8712: Calculate size from pointer

[Manisha Singh]

On 8/29/24 02:45, Philipp Hortmann wrote:
> On 8/28/24 22:45, Manisha Singh wrote:
>> Calculate the size from the pointer instead of struct
>>
> Hi Manisha,
>
> the first line of the description is good. Please just add the why (to 
> adhere to kernel coding style?) and the "." to make it a full sentence.
>
> Omit the next two lines.
>> CHECK: Prefer kmalloc(sizeof(*pintf_hdl->pintfpriv)...) over
>> kmalloc(sizeof(struct intf_priv)...)
>>
>
> Thanks for your support.
>
> Bye Philipp
>
Sure, will do that.

Thanks for reviewing.

Manisha

Re: [PATCH v2 1/2] staging: rtl8712: Fix style issues in rtl871x_io.c

[Philipp Hortmann]

On 8/28/24 22:45, Manisha Singh wrote:
> Remove multiple assignments from a line
> 
> CHECK: multiple assignments should be avoided
> +	pintf_priv = pintf_hdl->pintfpriv = kmalloc(sizeof(struct intf_priv),
> 

Hi Manisha,
please remove the 4 upper lines of the description. They are not required.
> Refactor the _init_intf_hdl() function to avoid multiple
> assignments in a single statement. This change improves code readability
> and adheres to kernel coding style guidelines.
> 
> Signed-off-by: Manisha Singh <masingh.linux@gmail.com>
> ---
> Changes Since V1:
> 	Broke the patch into 2 different fixes
> 
>   drivers/staging/rtl8712/rtl871x_io.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8712/rtl871x_io.c b/drivers/staging/rtl8712/rtl871x_io.c
> index 6789a4c98564..6311ac15c581 100644
> --- a/drivers/staging/rtl8712/rtl871x_io.c
> +++ b/drivers/staging/rtl8712/rtl871x_io.c
> @@ -48,10 +48,10 @@ static uint _init_intf_hdl(struct _adapter *padapter,
>   	set_intf_funs = &(r8712_usb_set_intf_funs);
>   	set_intf_ops = &r8712_usb_set_intf_ops;
>   	init_intf_priv = &r8712_usb_init_intf_priv;
> -	pintf_priv = pintf_hdl->pintfpriv = kmalloc(sizeof(struct intf_priv),
> -						    GFP_ATOMIC);
> +	pintf_priv = kmalloc(sizeof(struct intf_priv), GFP_ATOMIC);
>   	if (!pintf_priv)
>   		goto _init_intf_hdl_fail;

By pushing the below statement after the "if (!pintf_priv)" you change 
the logic. Is this really wanted? Why do you think it is better? I would 
avoid this and it would be a separate patch anyhow.

> +	pintf_hdl->pintfpriv = pintf_priv;

Thanks for your support.

Bye Philipp

>   	pintf_hdl->adapter = (u8 *)padapter;
>   	set_intf_option(&pintf_hdl->intf_option);
>   	set_intf_funs(pintf_hdl);

Re: [PATCH v2 1/2] staging: rtl8712: Fix style issues in rtl871x_io.c

[Manisha Singh]

On 8/29/24 02:38, Philipp Hortmann wrote:
> On 8/28/24 22:45, Manisha Singh wrote:
>> Remove multiple assignments from a line
>>
>> CHECK: multiple assignments should be avoided
>> +    pintf_priv = pintf_hdl->pintfpriv = kmalloc(sizeof(struct 
>> intf_priv),
>>
>
> Hi Manisha,
> please remove the 4 upper lines of the description. They are not 
> required.
Sure.
>> Refactor the _init_intf_hdl() function to avoid multiple
>> assignments in a single statement. This change improves code readability
>> and adheres to kernel coding style guidelines.
>>
>> Signed-off-by: Manisha Singh <masingh.linux@gmail.com>
>> ---
>> Changes Since V1:
>>     Broke the patch into 2 different fixes
>>
>>   drivers/staging/rtl8712/rtl871x_io.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8712/rtl871x_io.c 
>> b/drivers/staging/rtl8712/rtl871x_io.c
>> index 6789a4c98564..6311ac15c581 100644
>> --- a/drivers/staging/rtl8712/rtl871x_io.c
>> +++ b/drivers/staging/rtl8712/rtl871x_io.c
>> @@ -48,10 +48,10 @@ static uint _init_intf_hdl(struct _adapter 
>> *padapter,
>>       set_intf_funs = &(r8712_usb_set_intf_funs);
>>       set_intf_ops = &r8712_usb_set_intf_ops;
>>       init_intf_priv = &r8712_usb_init_intf_priv;
>> -    pintf_priv = pintf_hdl->pintfpriv = kmalloc(sizeof(struct 
>> intf_priv),
>> -                            GFP_ATOMIC);
>> +    pintf_priv = kmalloc(sizeof(struct intf_priv), GFP_ATOMIC);
>>       if (!pintf_priv)
>>           goto _init_intf_hdl_fail;
>
> By pushing the below statement after the "if (!pintf_priv)" you change 
> the logic. Is this really wanted? Why do you think it is better? I 
> would avoid this and it would be a separate patch anyhow.
>
>> +    pintf_hdl->pintfpriv = pintf_priv;
>
> Thanks for your support.
>
> Bye Philipp

Yeah, I shouldn't have changed the logic. I don't have hardware to test 
it, Let's move the assignment before the condition.

Thanks,
Manisha


>
>>       pintf_hdl->adapter = (u8 *)padapter;
>>       set_intf_option(&pintf_hdl->intf_option);
>>       set_intf_funs(pintf_hdl);
>

Re: [PATCH v2 1/2] staging: rtl8712: Fix style issues in rtl871x_io.c

[Dan Carpenter]

On Wed, Aug 28, 2024 at 11:08:31PM +0200, Philipp Hortmann wrote:
> > diff --git a/drivers/staging/rtl8712/rtl871x_io.c b/drivers/staging/rtl8712/rtl871x_io.c
> > index 6789a4c98564..6311ac15c581 100644
> > --- a/drivers/staging/rtl8712/rtl871x_io.c
> > +++ b/drivers/staging/rtl8712/rtl871x_io.c
> > @@ -48,10 +48,10 @@ static uint _init_intf_hdl(struct _adapter *padapter,
> >   	set_intf_funs = &(r8712_usb_set_intf_funs);
> >   	set_intf_ops = &r8712_usb_set_intf_ops;
> >   	init_intf_priv = &r8712_usb_init_intf_priv;
> > -	pintf_priv = pintf_hdl->pintfpriv = kmalloc(sizeof(struct intf_priv),
> > -						    GFP_ATOMIC);
> > +	pintf_priv = kmalloc(sizeof(struct intf_priv), GFP_ATOMIC);
> >   	if (!pintf_priv)
> >   		goto _init_intf_hdl_fail;
> 
> By pushing the below statement after the "if (!pintf_priv)" you change the
> logic. Is this really wanted? Why do you think it is better? I would avoid
> this and it would be a separate patch anyhow.
> 
> > +	pintf_hdl->pintfpriv = pintf_priv;

I liked moving it.  And I feel like it should be done in this patch, not as a
separate patch.  But it should have some justification as you say.  The commit
message could say something like:

    Checkpatch complains that we should avoid multiple assignments on the same
    line for readability purposes.  Generally, we would allocate, check and then
    assign.  It doesn't matter what is assigned to "pintf_hdl->pintfpriv" on the
    error path.  For example, on subsequent error paths "pintf_hdl->pintfpriv"
    is a freed pointer.  So this code is okay as-is and it's also okay to move
    the pintf_hdl->pintfpriv = pintf_priv assignment after the NULL check.

(Notice how I sold this as one thing, moving the "pintf_hdl->pintfpriv"
assignment, not silencing checkpatch and then moving it.  Notice how I avoided
using the word "also".)

regards,
dan carpenter

Re: [PATCH v2 1/2] staging: rtl8712: Fix style issues in rtl871x_io.c

[Philipp Hortmann]

On 8/29/24 11:36, Dan Carpenter wrote:
> On Wed, Aug 28, 2024 at 11:08:31PM +0200, Philipp Hortmann wrote:
>>> diff --git a/drivers/staging/rtl8712/rtl871x_io.c b/drivers/staging/rtl8712/rtl871x_io.c
>>> index 6789a4c98564..6311ac15c581 100644
>>> --- a/drivers/staging/rtl8712/rtl871x_io.c
>>> +++ b/drivers/staging/rtl8712/rtl871x_io.c
>>> @@ -48,10 +48,10 @@ static uint _init_intf_hdl(struct _adapter *padapter,
>>>    	set_intf_funs = &(r8712_usb_set_intf_funs);
>>>    	set_intf_ops = &r8712_usb_set_intf_ops;
>>>    	init_intf_priv = &r8712_usb_init_intf_priv;
>>> -	pintf_priv = pintf_hdl->pintfpriv = kmalloc(sizeof(struct intf_priv),
>>> -						    GFP_ATOMIC);
>>> +	pintf_priv = kmalloc(sizeof(struct intf_priv), GFP_ATOMIC);
>>>    	if (!pintf_priv)
>>>    		goto _init_intf_hdl_fail;
>>
>> By pushing the below statement after the "if (!pintf_priv)" you change the
>> logic. Is this really wanted? Why do you think it is better? I would avoid
>> this and it would be a separate patch anyhow.
>>
>>> +	pintf_hdl->pintfpriv = pintf_priv;
> 
> I liked moving it.  And I feel like it should be done in this patch, not as a
> separate patch.  But it should have some justification as you say.  The commit
> message could say something like:
> 
>      Checkpatch complains that we should avoid multiple assignments on the same
>      line for readability purposes.  Generally, we would allocate, check and then
>      assign.  It doesn't matter what is assigned to "pintf_hdl->pintfpriv" on the
>      error path.  For example, on subsequent error paths "pintf_hdl->pintfpriv"
>      is a freed pointer.  So this code is okay as-is and it's also okay to move
>      the pintf_hdl->pintfpriv = pintf_priv assignment after the NULL check.
> 
> (Notice how I sold this as one thing, moving the "pintf_hdl->pintfpriv"
> assignment, not silencing checkpatch and then moving it.  Notice how I avoided
> using the word "also".)
> 
> regards,
> dan carpenter
> 

Hi Manisha,

I like to give you an order of importance.
Greg is the maintainer. You need to fullfill his requirements as it is 
his decision to accept or reject your patches.

Dan is a very experienced developer. You are well advised to listen to him.

I am only a part time Hobbyist. I try to support Greg and Dan by 
responding to patches. Typically they will correct me when I am wrong.

Bye Philipp

Re: [PATCH v2 1/2] staging: rtl8712: Fix style issues in rtl871x_io.c

[Dan Carpenter]

No no.  The v3 is also fine.  Let's not stress too much about minor things.

regards,
dan carpenter

Re: [PATCH v2 1/2] staging: rtl8712: Fix style issues in rtl871x_io.c

[Manisha Singh]

Hi Dan and Phillip,

Thank you so much for your guidance; I really appreciate it, especially 
as i am new to kernel development. I will make sure to consider all your 
suggestions in the future.

I apologize for the delayed response.

Thanks again,
Manisha

On 8/30/24 01:04, Dan Carpenter wrote:
> No no.  The v3 is also fine.  Let's not stress too much about minor things.
>
> regards,
> dan carpenter
>