staging: rtl8712: Fix style issues in rtl871x_io.c
PATCH v2 1/2 staging: rtl8712: Fix style issues in rtl871x_io.c
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.
---
Changes Since V1:
Broke the patch into 2 different fixes
drivers/staging/rtl8712/rtl871x_io.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/rtl8712/rtl871x_io.c +2 -2
@@ -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);
Signature · 1 line
2.43.0
PATCH v2 2/2 staging: rtl8712: Calculate size from pointer
Calculate the size from the pointer instead of struct
CHECK: Prefer kmalloc(sizeof(*pintf_hdl->pintfpriv)...) over kmalloc(sizeof(struct intf_priv)...)
--- drivers/staging/rtl8712/rtl871x_io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/rtl8712/rtl871x_io.c +1 -1
@@ -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;
Signature · 1 line
2.43.0
Re: [PATCH v2 2/2] staging: rtl8712: Calculate size from pointer
Quoted 2 lines · Manisha Singh
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.
Quoted 3 lines
CHECK: Prefer kmalloc(sizeof(*pintf_hdl->pintfpriv)...) over
kmalloc(sizeof(struct intf_priv)...)
Thanks for your support.
Bye Philipp
Quoted 18 lines
Signed-off-by Manisha Singh <[email protected]>--- drivers/staging/rtl8712/rtl871x_io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/staging/rtl8712/rtl871x_io.c +1
-1@@ -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
Quoted 7 lines · Philipp Hortmann
Quoted 2 lines · Manisha Singh
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.
Quoted 9 lines
Omit the next two lines.
Quoted 3 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
Review
Quoted 5 lines · Manisha Singh
Remove multiple assignments from a line
CHECK: multiple assignments should be avoided
(quoted diff) +1
-0+ 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.
Quoted 17 lines
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 <[email protected]>---
Changes Since V1:
Broke the patch into 2 different fixesdrivers/staging/rtl8712/rtl871x_io.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/staging/rtl8712/rtl871x_io.c +0
-0
drivers/staging/rtl8712/rtl871x_io.c static uint _init_intf_hdl(struct _adapter *padapter, · line 53
Quoted 8 lines
drivers/staging/rtl8712/rtl871x_io.c +1
-2@@ -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.
drivers/staging/rtl8712/rtl871x_io.c static uint _init_intf_hdl(struct _adapter *padapter, · line 54
Quoted 1 line
drivers/staging/rtl8712/rtl871x_io.c +1
-0+ pintf_hdl->pintfpriv = pintf_priv;
Thanks for your support.
Bye Philipp
Quoted 3 lines
drivers/staging/rtl8712/rtl871x_io.c +0
-0pintf_hdl->adapter = (u8 *)padapter;set_intf_option (&pintf_hdl->intf_option);set_intf_funs (pintf_hdl);
Quoted 10 lines · Philipp Hortmann
Quoted 6 lines · Manisha Singh
Remove multiple assignments from a line
CHECK: multiple assignments should be avoided
+pintf_priv=pintf_hdl->pintfpriv= kmalloc(sizeof(structintf_priv),Hi Manisha,
please remove the 4 upper lines of the description. They are not required.
Sure.
Quoted 30 lines
Quoted 17 lines
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 <[email protected]>---
Changes Since V1:
Broke the patch into 2 different fixes
drivers/staging/rtl8712/rtl871x_io.c| 4 ++--1 file changed, 2 insertions(+), 2 deletions(-)drivers/staging/rtl8712/rtl871x_io.c +0
-0drivers/staging/rtl8712/rtl871x_io.c static uint _init_intf_hdl(struct _adapter · line 55
Quoted 10 lines
drivers/staging/rtl8712/rtl871x_io.c +1
-2@@ -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.
Quoted 5 lines
drivers/staging/rtl8712/rtl871x_io.c static uint _init_intf_hdl(struct _adapter *padapter, · line 54
Quoted 1 line
drivers/staging/rtl8712/rtl871x_io.c +1
-0+ 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
Quoted 5 lines
Quoted 4 lines
drivers/staging/rtl8712/rtl871x_io.c +0
-0pintf_hdl->adapter = (u8 *)padapter;
set_intf_option(&pintf_hdl->intf_option);
set_intf_funs(pintf_hdl);
Quoted 18 lines · Philipp Hortmann
drivers/staging/rtl8712/rtl871x_io.c static uint _init_intf_hdl(struct _adapter *padapter, · line 53
Quoted 13 lines
drivers/staging/rtl8712/rtl871x_io.c +1
-2@@ -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.
Quoted 1 line
drivers/staging/rtl8712/rtl871x_io.c static uint _init_intf_hdl(struct _adapter *padapter, · line 54
Quoted 1 line
drivers/staging/rtl8712/rtl871x_io.c +1
-0+ 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
Quoted 32 lines · Dan Carpenter
Quoted 18 lines · Philipp Hortmann
drivers/staging/rtl8712/rtl871x_io.c static uint _init_intf_hdl(struct _adapter *padapter, · line 53
Quoted 13 lines
drivers/staging/rtl8712/rtl871x_io.c +1
-2@@ -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.
Quoted 1 line
drivers/staging/rtl8712/rtl871x_io.c static uint _init_intf_hdl(struct _adapter *padapter, · line 54
Quoted 1 line
drivers/staging/rtl8712/rtl871x_io.c +1
-0+ 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 thepintf_hdl->pintfpriv=pintf_privassignment after the NULL check.
Quoted 7 lines
(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
No no. The v3 is also fine. Let's not stress too much about minor things.
regards,
dan carpenter
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
Quoted 5 lines · Dan Carpenter
No no. The v3 is also fine. Let's not stress too much about minor things.
regards,
dan carpenter