[PATCH 0/2] staging: vt6656: change function from always returning 0 to void

[PATCH 0/2] staging: vt6656: change function from always returning 0 to void

[John B. Wyatt IV]

Change vnt_radio_power_on from always returning 0 to void.

The first patch in this series was originally submitted as a 
standalone patch. Greg Kroah-Hartman <gregkh@linuxfoundation.org> 
suggested more changes to be made into a patchset.

John B. Wyatt IV (2):
  staging: vt6656: remove unneeded variable: ret
  staging: vt6656: change unused int return value to void

 drivers/staging/vt6656/card.c     | 9 ++-------
 drivers/staging/vt6656/card.h     | 2 +-
 drivers/staging/vt6656/main_usb.c | 4 +---
 3 files changed, 4 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH 0/2] staging: vt6656: change function from always returning 0 to void

[John B. Wyatt IV]

Change vnt_radio_power_on from always returning 0 to void.

The first patch in this series was originally submitted as a 
standalone patch. Greg Kroah-Hartman <gregkh@linuxfoundation.org> 
suggested more changes to be made into a patchset.

John B. Wyatt IV (2):
  staging: vt6656: remove unneeded variable: ret
  staging: vt6656: change unused int return value to void

 drivers/staging/vt6656/card.c     | 9 ++-------
 drivers/staging/vt6656/card.h     | 2 +-
 drivers/staging/vt6656/main_usb.c | 4 +---
 3 files changed, 4 insertions(+), 11 deletions(-)

-- 
2.25.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 1/2] staging: vt6656: remove unneeded variable: ret

[John B. Wyatt IV]

Remove unneeded variable ret; replace with 0 for the return value.

Update function documentation (comment) on the return status as
suggested by Julia Lawall <julia.lawall@inria.fr>.

Issue reported by coccinelle (coccicheck).

Signed-off-by: John B. Wyatt IV <jbwyatt4@gmail.com>
---
 drivers/staging/vt6656/card.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/vt6656/card.c b/drivers/staging/vt6656/card.c
index dc3ab10eb630..05b57a2489a0 100644
--- a/drivers/staging/vt6656/card.c
+++ b/drivers/staging/vt6656/card.c
@@ -716,13 +716,11 @@ int vnt_radio_power_off(struct vnt_private *priv)
  *  Out:
  *      none
  *
- * Return Value: true if success; otherwise false
+ * Return Value: 0
  *
  */
 int vnt_radio_power_on(struct vnt_private *priv)
 {
-	int ret = 0;
-
 	vnt_exit_deep_sleep(priv);
 
 	vnt_mac_reg_bits_on(priv, MAC_REG_HOSTCR, HOSTCR_RXON);
@@ -741,7 +739,7 @@ int vnt_radio_power_on(struct vnt_private *priv)
 
 	vnt_mac_reg_bits_off(priv, MAC_REG_GPIOCTL1, GPIO3_INTMD);
 
-	return ret;
+	return 0;
 }
 
 void vnt_set_bss_mode(struct vnt_private *priv)
-- 
2.25.1

[PATCH 1/2] staging: vt6656: remove unneeded variable: ret

[John B. Wyatt IV]

Remove unneeded variable ret; replace with 0 for the return value.

Update function documentation (comment) on the return status as
suggested by Julia Lawall <julia.lawall@inria.fr>.

Issue reported by coccinelle (coccicheck).

Signed-off-by: John B. Wyatt IV <jbwyatt4@gmail.com>
---
 drivers/staging/vt6656/card.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/vt6656/card.c b/drivers/staging/vt6656/card.c
index dc3ab10eb630..05b57a2489a0 100644
--- a/drivers/staging/vt6656/card.c
+++ b/drivers/staging/vt6656/card.c
@@ -716,13 +716,11 @@ int vnt_radio_power_off(struct vnt_private *priv)
  *  Out:
  *      none
  *
- * Return Value: true if success; otherwise false
+ * Return Value: 0
  *
  */
 int vnt_radio_power_on(struct vnt_private *priv)
 {
-	int ret = 0;
-
 	vnt_exit_deep_sleep(priv);
 
 	vnt_mac_reg_bits_on(priv, MAC_REG_HOSTCR, HOSTCR_RXON);
@@ -741,7 +739,7 @@ int vnt_radio_power_on(struct vnt_private *priv)
 
 	vnt_mac_reg_bits_off(priv, MAC_REG_GPIOCTL1, GPIO3_INTMD);
 
-	return ret;
+	return 0;
 }
 
 void vnt_set_bss_mode(struct vnt_private *priv)
-- 
2.25.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 2/2] staging: vt6656: change unused int return value to void

[John B. Wyatt IV]

Change unused int function return value to void from previous patch.

Update function documentation to remove mention of return value.

Remove if statement check of the only usage of function in the
kernel. Replace with calling the function.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: John B. Wyatt IV <jbwyatt4@gmail.com>
---
 drivers/staging/vt6656/card.c     | 7 ++-----
 drivers/staging/vt6656/card.h     | 2 +-
 drivers/staging/vt6656/main_usb.c | 4 +---
 3 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/vt6656/card.c b/drivers/staging/vt6656/card.c
index 05b57a2489a0..4be7fca32796 100644
--- a/drivers/staging/vt6656/card.c
+++ b/drivers/staging/vt6656/card.c
@@ -715,11 +715,8 @@ int vnt_radio_power_off(struct vnt_private *priv)
  *      priv         - The adapter to be turned on
  *  Out:
  *      none
- *
- * Return Value: 0
- *
  */
-int vnt_radio_power_on(struct vnt_private *priv)
+void vnt_radio_power_on(struct vnt_private *priv)
 {
 	vnt_exit_deep_sleep(priv);
 
@@ -739,7 +736,7 @@ int vnt_radio_power_on(struct vnt_private *priv)
 
 	vnt_mac_reg_bits_off(priv, MAC_REG_GPIOCTL1, GPIO3_INTMD);
 
-	return 0;
+	return;
 }
 
 void vnt_set_bss_mode(struct vnt_private *priv)
diff --git a/drivers/staging/vt6656/card.h b/drivers/staging/vt6656/card.h
index 75cd340c0cce..fcab6b086e71 100644
--- a/drivers/staging/vt6656/card.h
+++ b/drivers/staging/vt6656/card.h
@@ -40,7 +40,7 @@ void vnt_update_next_tbtt(struct vnt_private *priv, u64 tsf,
 u64 vnt_get_next_tbtt(u64 tsf, u16 beacon_interval);
 u64 vnt_get_tsf_offset(u8 rx_rate, u64 tsf1, u64 tsf2);
 int vnt_radio_power_off(struct vnt_private *priv);
-int vnt_radio_power_on(struct vnt_private *priv);
+void vnt_radio_power_on(struct vnt_private *priv);
 u8 vnt_get_pkt_type(struct vnt_private *priv);
 void vnt_set_bss_mode(struct vnt_private *priv);
 
diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
index 8e7269c87ea9..8214427f5ee3 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -370,9 +370,7 @@ static int vnt_init_registers(struct vnt_private *priv)
 	if (ret)
 		goto end;
 
-	ret = vnt_radio_power_on(priv);
-	if (ret)
-		goto end;
+	vnt_radio_power_on(priv);
 
 	dev_dbg(&priv->usb->dev, "<----INIbInitAdapter Exit\n");
 
-- 
2.25.1

[PATCH 2/2] staging: vt6656: change unused int return value to void

[John B. Wyatt IV]

Change unused int function return value to void from previous patch.

Update function documentation to remove mention of return value.

Remove if statement check of the only usage of function in the
kernel. Replace with calling the function.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: John B. Wyatt IV <jbwyatt4@gmail.com>
---
 drivers/staging/vt6656/card.c     | 7 ++-----
 drivers/staging/vt6656/card.h     | 2 +-
 drivers/staging/vt6656/main_usb.c | 4 +---
 3 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/vt6656/card.c b/drivers/staging/vt6656/card.c
index 05b57a2489a0..4be7fca32796 100644
--- a/drivers/staging/vt6656/card.c
+++ b/drivers/staging/vt6656/card.c
@@ -715,11 +715,8 @@ int vnt_radio_power_off(struct vnt_private *priv)
  *      priv         - The adapter to be turned on
  *  Out:
  *      none
- *
- * Return Value: 0
- *
  */
-int vnt_radio_power_on(struct vnt_private *priv)
+void vnt_radio_power_on(struct vnt_private *priv)
 {
 	vnt_exit_deep_sleep(priv);
 
@@ -739,7 +736,7 @@ int vnt_radio_power_on(struct vnt_private *priv)
 
 	vnt_mac_reg_bits_off(priv, MAC_REG_GPIOCTL1, GPIO3_INTMD);
 
-	return 0;
+	return;
 }
 
 void vnt_set_bss_mode(struct vnt_private *priv)
diff --git a/drivers/staging/vt6656/card.h b/drivers/staging/vt6656/card.h
index 75cd340c0cce..fcab6b086e71 100644
--- a/drivers/staging/vt6656/card.h
+++ b/drivers/staging/vt6656/card.h
@@ -40,7 +40,7 @@ void vnt_update_next_tbtt(struct vnt_private *priv, u64 tsf,
 u64 vnt_get_next_tbtt(u64 tsf, u16 beacon_interval);
 u64 vnt_get_tsf_offset(u8 rx_rate, u64 tsf1, u64 tsf2);
 int vnt_radio_power_off(struct vnt_private *priv);
-int vnt_radio_power_on(struct vnt_private *priv);
+void vnt_radio_power_on(struct vnt_private *priv);
 u8 vnt_get_pkt_type(struct vnt_private *priv);
 void vnt_set_bss_mode(struct vnt_private *priv);
 
diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
index 8e7269c87ea9..8214427f5ee3 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -370,9 +370,7 @@ static int vnt_init_registers(struct vnt_private *priv)
 	if (ret)
 		goto end;
 
-	ret = vnt_radio_power_on(priv);
-	if (ret)
-		goto end;
+	vnt_radio_power_on(priv);
 
 	dev_dbg(&priv->usb->dev, "<----INIbInitAdapter Exit\n");
 
-- 
2.25.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: vt6656: remove unneeded variable: ret

[Quentin Deslandes]

On 03/23/20 23:45:44, John B. Wyatt IV wrote:
>  	vnt_mac_reg_bits_off(priv, MAC_REG_GPIOCTL1, GPIO3_INTMD);

This function, and all the functions called in vnt_radio_power_on() returns
a value, why don't you catch it and act accordingly (forward error code
for example) instead of silencing it?

Thanks,
Quentin

Re: [PATCH 1/2] staging: vt6656: remove unneeded variable: ret

[Quentin Deslandes]

On 03/23/20 23:45:44, John B. Wyatt IV wrote:
>  	vnt_mac_reg_bits_off(priv, MAC_REG_GPIOCTL1, GPIO3_INTMD);

This function, and all the functions called in vnt_radio_power_on() returns
a value, why don't you catch it and act accordingly (forward error code
for example) instead of silencing it?

Thanks,
Quentin
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 0/2] staging: vt6656: change function from always returning 0 to void

[Dan Carpenter]

Fold these two patches together so its just one patch.

regards,
dan carpenter

Re: [PATCH 0/2] staging: vt6656: change function from always returning 0 to void

[Dan Carpenter]

Fold these two patches together so its just one patch.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: vt6656: remove unneeded variable: ret

[John Wyatt]

On Tue, 2020-03-24 at 10:03 +0000, Quentin Deslandes wrote:
> On 03/23/20 23:45:44, John B. Wyatt IV wrote:
> >  	vnt_mac_reg_bits_off(priv, MAC_REG_GPIOCTL1, GPIO3_INTMD);
> 
> This function, and all the functions called in vnt_radio_power_on()
> returns
> a value, why don't you catch it and act accordingly (forward error
> code

Hi Quentin,

I do not know what these functions do.

There is no function documentation for:
vnt_exit_deep_sleep
vnt_mac_reg_bits_on
vnt_mac_reg_bits_off

I am a new kernel developer intern with the Outreachy program. I am
trying to fix a style issue reported by Coccinelle. I do not have that
much experience with writing drivers yet.

I have CC'ed the developers listed by git blame + the outreachy
mentors.

(Rithvik Patibandla's name shows up for one of the functions, but the
email is not listed by get_maintainer.)

Please advise.

Re: [PATCH 1/2] staging: vt6656: remove unneeded variable: ret

[Quentin Deslandes]

On 03/24/20 17:32:01, John Wyatt wrote:
> I do not know what these functions do.
> 
> There is no function documentation for:
> vnt_exit_deep_sleep
> vnt_mac_reg_bits_on
> vnt_mac_reg_bits_off

I understand, however discarding the return value of functions that could
fail is not the best thing to do. Whatever those 3 functions are doing,
you should rely on their return code, and if one of those fail,
vnt_radio_power_on() should fail too.

> I am a new kernel developer intern with the Outreachy program. I am
> trying to fix a style issue reported by Coccinelle. I do not have that
> much experience with writing drivers yet.

Don't worry, I'm not that experienced either. If my request is out of
scope for an Outreachy mentee, a more experienced contributor will pop
in the discussion.

Thanks,
Quentin

Re: [PATCH 1/2] staging: vt6656: remove unneeded variable: ret

[John Wyatt]

On Wed, 2020-03-25 at 09:15 +0000, Quentin Deslandes wrote:
> On 03/24/20 17:32:01, John Wyatt wrote:
> > I do not know what these functions do.
> > 
> > There is no function documentation for:
> > vnt_exit_deep_sleep
> > vnt_mac_reg_bits_on
> > vnt_mac_reg_bits_off
> 
> I understand, however discarding the return value of functions that
> could
> fail is not the best thing to do. Whatever those 3 functions are
> doing,
> you should rely on their return code, and if one of those fail,
> vnt_radio_power_on() should fail too.
> 
> > I am a new kernel developer intern with the Outreachy program. I am
> > trying to fix a style issue reported by Coccinelle. I do not have
> > that
> > much experience with writing drivers yet.
> 
> Don't worry, I'm not that experienced either. If my request is out of
> scope for an Outreachy mentee, a more experienced contributor will
> pop
> in the discussion.

To any of the Outreachy mentors please advise on what to do.

I was asked by Greg Kroah-Hartman to make this patch set, but Quentin
Deslandes asked to return undocumented return codes for this patch to
solve this coccinelle issue.

Thank you,
John

Re: [Outreachy kernel] Re: [PATCH 1/2] staging: vt6656: remove unneeded variable: ret

[Stefano Brivio]

Hi John,

On Thu, 26 Mar 2020 16:26:13 -0700
John Wyatt <jbwyatt4@gmail.com> wrote:

> On Wed, 2020-03-25 at 09:15 +0000, Quentin Deslandes wrote:
> > On 03/24/20 17:32:01, John Wyatt wrote:  
> > > I do not know what these functions do.
> > > 
> > > There is no function documentation for:
> > > vnt_exit_deep_sleep
> > > vnt_mac_reg_bits_on
> > > vnt_mac_reg_bits_off  
> > 
> > I understand, however discarding the return value of functions that
> > could
> > fail is not the best thing to do. Whatever those 3 functions are
> > doing,
> > you should rely on their return code, and if one of those fail,
> > vnt_radio_power_on() should fail too.
> >   
> > > I am a new kernel developer intern with the Outreachy program. I am
> > > trying to fix a style issue reported by Coccinelle. I do not have
> > > that
> > > much experience with writing drivers yet.  
> > 
> > Don't worry, I'm not that experienced either. If my request is out of
> > scope for an Outreachy mentee, a more experienced contributor will
> > pop
> > in the discussion.  
> 
> To any of the Outreachy mentors please advise on what to do.
> 
> I was asked by Greg Kroah-Hartman to make this patch set, but Quentin
> Deslandes asked to return undocumented return codes for this patch to
> solve this coccinelle issue.

Strictly speaking, those are documented: C code (especially in kernel)
is supposed to be self-documenting, and if you follow the call path
(example for vnt_mac_reg_bits_off()) you'll find out that, eventually,
you might get -EINVAL (not going to actually happen), -ENOMEM, -EIO
(the only likely one, on communication error).

If you forward those return codes as Quentin suggested, those are
handled properly by the callers, so I would say that's the way to go.

-- 
Stefano

Re: [PATCH 1/2] staging: vt6656: remove unneeded variable: ret

[Greg Kroah-Hartman]

On Thu, Mar 26, 2020 at 04:26:13PM -0700, John Wyatt wrote:
> On Wed, 2020-03-25 at 09:15 +0000, Quentin Deslandes wrote:
> > On 03/24/20 17:32:01, John Wyatt wrote:
> > > I do not know what these functions do.
> > > 
> > > There is no function documentation for:
> > > vnt_exit_deep_sleep
> > > vnt_mac_reg_bits_on
> > > vnt_mac_reg_bits_off
> > 
> > I understand, however discarding the return value of functions that
> > could
> > fail is not the best thing to do. Whatever those 3 functions are
> > doing,
> > you should rely on their return code, and if one of those fail,
> > vnt_radio_power_on() should fail too.
> > 
> > > I am a new kernel developer intern with the Outreachy program. I am
> > > trying to fix a style issue reported by Coccinelle. I do not have
> > > that
> > > much experience with writing drivers yet.
> > 
> > Don't worry, I'm not that experienced either. If my request is out of
> > scope for an Outreachy mentee, a more experienced contributor will
> > pop
> > in the discussion.
> 
> To any of the Outreachy mentors please advise on what to do.
> 
> I was asked by Greg Kroah-Hartman to make this patch set, but Quentin
> Deslandes asked to return undocumented return codes for this patch to
> solve this coccinelle issue.

What do you mean by "undocumented return codes"?  Just pick an error
value that you think matches what went wrong and try that.  Worst thing
is we review the patch and tell you to pick a different one :)

thanks,

greg k-h

Re: [Outreachy kernel] Re: [PATCH 1/2] staging: vt6656: remove unneeded variable: ret

[John Wyatt]

On Fri, 2020-03-27 at 04:52 +0100, Stefano Brivio wrote:
> Hi John,
> 
> On Thu, 26 Mar 2020 16:26:13 -0700
> John Wyatt <jbwyatt4@gmail.com> wrote:
> 
> > On Wed, 2020-03-25 at 09:15 +0000, Quentin Deslandes wrote:
> > > On 03/24/20 17:32:01, John Wyatt wrote:  
> > > > I do not know what these functions do.
> > > > 
> > > > There is no function documentation for:
> > > > vnt_exit_deep_sleep
> > > > vnt_mac_reg_bits_on
> > > > vnt_mac_reg_bits_off  
> > > 
> > > I understand, however discarding the return value of functions
> > > that
> > > could
> > > fail is not the best thing to do. Whatever those 3 functions are
> > > doing,
> > > you should rely on their return code, and if one of those fail,
> > > vnt_radio_power_on() should fail too.
> > >   
> > > > I am a new kernel developer intern with the Outreachy program.
> > > > I am
> > > > trying to fix a style issue reported by Coccinelle. I do not
> > > > have
> > > > that
> > > > much experience with writing drivers yet.  
> > > 
> > > Don't worry, I'm not that experienced either. If my request is
> > > out of
> > > scope for an Outreachy mentee, a more experienced contributor
> > > will
> > > pop
> > > in the discussion.  
> > 
> > To any of the Outreachy mentors please advise on what to do.
> > 
> > I was asked by Greg Kroah-Hartman to make this patch set, but
> > Quentin
> > Deslandes asked to return undocumented return codes for this patch
> > to
> > solve this coccinelle issue.
> 
> Strictly speaking, those are documented: C code (especially in
> kernel)
> is supposed to be self-documenting, and if you follow the call path
> (example for vnt_mac_reg_bits_off()) you'll find out that,
> eventually,
> you might get -EINVAL (not going to actually happen), -ENOMEM, -EIO
> (the only likely one, on communication error).
> 
> If you forward those return codes as Quentin suggested, those are
> handled properly by the callers, so I would say that's the way to go.
> 

Understood, will do.

Thank you Stefano. :)

Edit: forgot to CC mailing list.

Re: [PATCH 1/2] staging: vt6656: remove unneeded variable: ret

[John Wyatt]

On Fri, 2020-03-27 at 07:34 +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 26, 2020 at 04:26:13PM -0700, John Wyatt wrote:
> > On Wed, 2020-03-25 at 09:15 +0000, Quentin Deslandes wrote:
> > > On 03/24/20 17:32:01, John Wyatt wrote:
> > > > I do not know what these functions do.
> > > > 
> > > > There is no function documentation for:
> > > > vnt_exit_deep_sleep
> > > > vnt_mac_reg_bits_on
> > > > vnt_mac_reg_bits_off
> > > 
> > > I understand, however discarding the return value of functions
> > > that
> > > could
> > > fail is not the best thing to do. Whatever those 3 functions are
> > > doing,
> > > you should rely on their return code, and if one of those fail,
> > > vnt_radio_power_on() should fail too.
> > > 
> > > > I am a new kernel developer intern with the Outreachy program.
> > > > I am
> > > > trying to fix a style issue reported by Coccinelle. I do not
> > > > have
> > > > that
> > > > much experience with writing drivers yet.
> > > 
> > > Don't worry, I'm not that experienced either. If my request is
> > > out of
> > > scope for an Outreachy mentee, a more experienced contributor
> > > will
> > > pop
> > > in the discussion.
> > 
> > To any of the Outreachy mentors please advise on what to do.
> > 
> > I was asked by Greg Kroah-Hartman to make this patch set, but
> > Quentin
> > Deslandes asked to return undocumented return codes for this patch
> > to
> > solve this coccinelle issue.
> 
> What do you mean by "undocumented return codes"?

No function comments like the one that I originally changed in the
first patch.

>   Just pick an error
> value that you think matches what went wrong and try that.  

I guess I thought it was more complicated than that. Stefano's
explanation helped me understand it.

> Worst thing
> is we review the patch and tell you to pick a different one :)
> 

Ideally I would like to minimize that. :)