[PATCH] staging: vt6656: remove unused variable

[PATCH] staging: vt6656: remove unused variable

[Quentin Deslandes]

Fixed 'set but not used' warning message on a status variable. The
called function returning the status code 'vnt_start_interrupt_urb()'
clean up after itself and the caller function
'vnt_int_start_interrupt()' does not returns any value.

Signed-off-by: Quentin Deslandes <quentin.deslandes@itdev.co.uk>
---
 drivers/staging/vt6656/int.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/vt6656/int.c b/drivers/staging/vt6656/int.c
index 504424b19fcf..ac30ce72db5a 100644
--- a/drivers/staging/vt6656/int.c
+++ b/drivers/staging/vt6656/int.c
@@ -42,13 +42,12 @@ static const u8 fallback_rate1[5][5] = {
 void vnt_int_start_interrupt(struct vnt_private *priv)
 {
 	unsigned long flags;
-	int status;
 
 	dev_dbg(&priv->usb->dev, "---->Interrupt Polling Thread\n");
 
 	spin_lock_irqsave(&priv->lock, flags);
 
-	status = vnt_start_interrupt_urb(priv);
+	vnt_start_interrupt_urb(priv);
 
 	spin_unlock_irqrestore(&priv->lock, flags);
 }
-- 
2.17.1

Re: [PATCH] staging: vt6656: remove unused variable

[Greg Kroah-Hartman]

On Thu, May 16, 2019 at 09:31:05AM +0000, Quentin Deslandes wrote:
> Fixed 'set but not used' warning message on a status variable. The
> called function returning the status code 'vnt_start_interrupt_urb()'
> clean up after itself and the caller function
> 'vnt_int_start_interrupt()' does not returns any value.
> 
> Signed-off-by: Quentin Deslandes <quentin.deslandes@itdev.co.uk>
> ---
>  drivers/staging/vt6656/int.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/vt6656/int.c b/drivers/staging/vt6656/int.c
> index 504424b19fcf..ac30ce72db5a 100644
> --- a/drivers/staging/vt6656/int.c
> +++ b/drivers/staging/vt6656/int.c
> @@ -42,13 +42,12 @@ static const u8 fallback_rate1[5][5] = {
>  void vnt_int_start_interrupt(struct vnt_private *priv)
>  {
>  	unsigned long flags;
> -	int status;
>  
>  	dev_dbg(&priv->usb->dev, "---->Interrupt Polling Thread\n");
>  
>  	spin_lock_irqsave(&priv->lock, flags);
>  
> -	status = vnt_start_interrupt_urb(priv);
> +	vnt_start_interrupt_urb(priv);

Shouldn't you fix this by erroring out if this fails?  Why ignore the
errors?

thanks,

greg k-h

Re: [PATCH] staging: vt6656: remove unused variable

[Quentin Deslandes]

On Thu, May 16, 2019 at 11:39:51AM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 16, 2019 at 09:31:05AM +0000, Quentin Deslandes wrote:
> > Fixed 'set but not used' warning message on a status variable. The
> > called function returning the status code 'vnt_start_interrupt_urb()'
> > clean up after itself and the caller function
> > 'vnt_int_start_interrupt()' does not returns any value.
> > 
> > Signed-off-by: Quentin Deslandes <quentin.deslandes@itdev.co.uk>
> > ---
> >  drivers/staging/vt6656/int.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/vt6656/int.c b/drivers/staging/vt6656/int.c
> > index 504424b19fcf..ac30ce72db5a 100644
> > --- a/drivers/staging/vt6656/int.c
> > +++ b/drivers/staging/vt6656/int.c
> > @@ -42,13 +42,12 @@ static const u8 fallback_rate1[5][5] = {
> >  void vnt_int_start_interrupt(struct vnt_private *priv)
> >  {
> >  	unsigned long flags;
> > -	int status;
> >  
> >  	dev_dbg(&priv->usb->dev, "---->Interrupt Polling Thread\n");
> >  
> >  	spin_lock_irqsave(&priv->lock, flags);
> >  
> > -	status = vnt_start_interrupt_urb(priv);
> > +	vnt_start_interrupt_urb(priv);
> 
> Shouldn't you fix this by erroring out if this fails?  Why ignore the
> errors?
> 
> thanks,
> 
> greg k-h

I could, however 'vnt_start_interrupt_urb()' already call 'dev_dbg()' if
it fails. Nothing is done after this debug call except returning an
error code.

'vnt_int_start_interrupt()' should, IMHO, return a status code, but the
original developer may have good reasons not to do so.

Thank you,
Quentin

Re: [PATCH] staging: vt6656: remove unused variable

[Greg Kroah-Hartman]

On Thu, May 16, 2019 at 09:50:38AM +0000, Quentin Deslandes wrote:
> On Thu, May 16, 2019 at 11:39:51AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, May 16, 2019 at 09:31:05AM +0000, Quentin Deslandes wrote:
> > > Fixed 'set but not used' warning message on a status variable. The
> > > called function returning the status code 'vnt_start_interrupt_urb()'
> > > clean up after itself and the caller function
> > > 'vnt_int_start_interrupt()' does not returns any value.
> > > 
> > > Signed-off-by: Quentin Deslandes <quentin.deslandes@itdev.co.uk>
> > > ---
> > >  drivers/staging/vt6656/int.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/staging/vt6656/int.c b/drivers/staging/vt6656/int.c
> > > index 504424b19fcf..ac30ce72db5a 100644
> > > --- a/drivers/staging/vt6656/int.c
> > > +++ b/drivers/staging/vt6656/int.c
> > > @@ -42,13 +42,12 @@ static const u8 fallback_rate1[5][5] = {
> > >  void vnt_int_start_interrupt(struct vnt_private *priv)
> > >  {
> > >  	unsigned long flags;
> > > -	int status;
> > >  
> > >  	dev_dbg(&priv->usb->dev, "---->Interrupt Polling Thread\n");
> > >  
> > >  	spin_lock_irqsave(&priv->lock, flags);
> > >  
> > > -	status = vnt_start_interrupt_urb(priv);
> > > +	vnt_start_interrupt_urb(priv);
> > 
> > Shouldn't you fix this by erroring out if this fails?  Why ignore the
> > errors?
> > 
> > thanks,
> > 
> > greg k-h
> 
> I could, however 'vnt_start_interrupt_urb()' already call 'dev_dbg()' if
> it fails. Nothing is done after this debug call except returning an
> error code.

Returning an error code is fine for that function.  But then you have to
do something with that error.

> 'vnt_int_start_interrupt()' should, IMHO, return a status code, but the
> original developer may have good reasons not to do so.

I think that is the real problem that needs to be fixed here, don't
paper over the issue by ignoring the return value.

thanks,

greg k-h

Re: [PATCH] staging: vt6656: remove unused variable

[Quentin Deslandes]

On Thu, May 16, 2019 at 12:19:53PM +0200, Greg Kroah-Hartman wrote:
> On Thu, May 16, 2019 at 09:50:38AM +0000, Quentin Deslandes wrote:
> > On Thu, May 16, 2019 at 11:39:51AM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, May 16, 2019 at 09:31:05AM +0000, Quentin Deslandes wrote:
> > > > Fixed 'set but not used' warning message on a status variable. The
> > > > called function returning the status code 'vnt_start_interrupt_urb()'
> > > > clean up after itself and the caller function
> > > > 'vnt_int_start_interrupt()' does not returns any value.
> > > > 
> > > > Signed-off-by: Quentin Deslandes <quentin.deslandes@itdev.co.uk>
> > > > ---
> > > >  drivers/staging/vt6656/int.c | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/vt6656/int.c b/drivers/staging/vt6656/int.c
> > > > index 504424b19fcf..ac30ce72db5a 100644
> > > > --- a/drivers/staging/vt6656/int.c
> > > > +++ b/drivers/staging/vt6656/int.c
> > > > @@ -42,13 +42,12 @@ static const u8 fallback_rate1[5][5] = {
> > > >  void vnt_int_start_interrupt(struct vnt_private *priv)
> > > >  {
> > > >  	unsigned long flags;
> > > > -	int status;
> > > >  
> > > >  	dev_dbg(&priv->usb->dev, "---->Interrupt Polling Thread\n");
> > > >  
> > > >  	spin_lock_irqsave(&priv->lock, flags);
> > > >  
> > > > -	status = vnt_start_interrupt_urb(priv);
> > > > +	vnt_start_interrupt_urb(priv);
> > > 
> > > Shouldn't you fix this by erroring out if this fails?  Why ignore the
> > > errors?
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > I could, however 'vnt_start_interrupt_urb()' already call 'dev_dbg()' if
> > it fails. Nothing is done after this debug call except returning an
> > error code.
> 
> Returning an error code is fine for that function.  But then you have to
> do something with that error.
> 
> > 'vnt_int_start_interrupt()' should, IMHO, return a status code, but the
> > original developer may have good reasons not to do so.
> 
> I think that is the real problem that needs to be fixed here, don't
> paper over the issue by ignoring the return value.
> 
> thanks,
> 
> greg k-h

Thus I'll return an error value to handle this in the caller function
and then send a v2.

Thank you for your help,
Quentin

[PATCH] staging: vt6656: returns error code on vnt_int_start_interrupt fail

[Quentin Deslandes]

Returns error code from 'vnt_int_start_interrupt()' so the device's private
buffers will be correctly freed and 'struct ieee80211_hw' start function
will return an error code.

Signed-off-by: Quentin Deslandes <quentin.deslandes@itdev.co.uk>
---
v2: instead of removing status variable, returns its value to caller and
    handle error in caller.

 drivers/staging/vt6656/int.c      |  4 +++-
 drivers/staging/vt6656/int.h      |  2 +-
 drivers/staging/vt6656/main_usb.c | 12 +++++++++---
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/vt6656/int.c b/drivers/staging/vt6656/int.c
index 504424b19fcf..f3ee2198e1b3 100644
--- a/drivers/staging/vt6656/int.c
+++ b/drivers/staging/vt6656/int.c
@@ -39,7 +39,7 @@ static const u8 fallback_rate1[5][5] = {
 	{RATE_54M, RATE_54M, RATE_36M, RATE_18M, RATE_18M}
 };
 
-void vnt_int_start_interrupt(struct vnt_private *priv)
+int vnt_int_start_interrupt(struct vnt_private *priv)
 {
 	unsigned long flags;
 	int status;
@@ -51,6 +51,8 @@ void vnt_int_start_interrupt(struct vnt_private *priv)
 	status = vnt_start_interrupt_urb(priv);
 
 	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return status;
 }
 
 static int vnt_int_report_rate(struct vnt_private *priv, u8 pkt_no, u8 tsr)
diff --git a/drivers/staging/vt6656/int.h b/drivers/staging/vt6656/int.h
index 987c454e99e9..8a6d60569ceb 100644
--- a/drivers/staging/vt6656/int.h
+++ b/drivers/staging/vt6656/int.h
@@ -41,7 +41,7 @@ struct vnt_interrupt_data {
 	u8 sw[2];
 } __packed;
 
-void vnt_int_start_interrupt(struct vnt_private *priv);
+int vnt_int_start_interrupt(struct vnt_private *priv);
 void vnt_int_process_data(struct vnt_private *priv);
 
 #endif /* __INT_H__ */
diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
index ccafcc2c87ac..71e10b9ae253 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -483,6 +483,7 @@ static void vnt_tx_80211(struct ieee80211_hw *hw,
 
 static int vnt_start(struct ieee80211_hw *hw)
 {
+	int err = 0;
 	struct vnt_private *priv = hw->priv;
 
 	priv->rx_buf_sz = MAX_TOTAL_SIZE_WITH_ALL_HEADERS;
@@ -496,15 +497,20 @@ static int vnt_start(struct ieee80211_hw *hw)
 
 	if (vnt_init_registers(priv) == false) {
 		dev_dbg(&priv->usb->dev, " init register fail\n");
+		err = -ENOMEM;
 		goto free_all;
 	}
 
-	if (vnt_key_init_table(priv))
+	if (vnt_key_init_table(priv)) {
+		err = -ENOMEM;
 		goto free_all;
+	}
 
 	priv->int_interval = 1;  /* bInterval is set to 1 */
 
-	vnt_int_start_interrupt(priv);
+	err = vnt_int_start_interrupt(priv);
+	if (err)
+		goto free_all;
 
 	ieee80211_wake_queues(hw);
 
@@ -518,7 +524,7 @@ static int vnt_start(struct ieee80211_hw *hw)
 	usb_kill_urb(priv->interrupt_urb);
 	usb_free_urb(priv->interrupt_urb);
 
-	return -ENOMEM;
+	return err;
 }
 
 static void vnt_stop(struct ieee80211_hw *hw)
-- 
2.17.1

[PATCH v2] staging: vt6656: returns error code on vnt_int_start_interrupt fail

[Quentin Deslandes]

Returns error code from 'vnt_int_start_interrupt()' so the device's private
buffers will be correctly freed and 'struct ieee80211_hw' start function
will return an error code.

Signed-off-by: Quentin Deslandes <quentin.deslandes@itdev.co.uk>
---
 drivers/staging/vt6656/int.c      |  4 +++-
 drivers/staging/vt6656/int.h      |  2 +-
 drivers/staging/vt6656/main_usb.c | 12 +++++++++---
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/vt6656/int.c b/drivers/staging/vt6656/int.c
index 504424b19fcf..f3ee2198e1b3 100644
--- a/drivers/staging/vt6656/int.c
+++ b/drivers/staging/vt6656/int.c
@@ -39,7 +39,7 @@ static const u8 fallback_rate1[5][5] = {
 	{RATE_54M, RATE_54M, RATE_36M, RATE_18M, RATE_18M}
 };
 
-void vnt_int_start_interrupt(struct vnt_private *priv)
+int vnt_int_start_interrupt(struct vnt_private *priv)
 {
 	unsigned long flags;
 	int status;
@@ -51,6 +51,8 @@ void vnt_int_start_interrupt(struct vnt_private *priv)
 	status = vnt_start_interrupt_urb(priv);
 
 	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return status;
 }
 
 static int vnt_int_report_rate(struct vnt_private *priv, u8 pkt_no, u8 tsr)
diff --git a/drivers/staging/vt6656/int.h b/drivers/staging/vt6656/int.h
index 987c454e99e9..8a6d60569ceb 100644
--- a/drivers/staging/vt6656/int.h
+++ b/drivers/staging/vt6656/int.h
@@ -41,7 +41,7 @@ struct vnt_interrupt_data {
 	u8 sw[2];
 } __packed;
 
-void vnt_int_start_interrupt(struct vnt_private *priv);
+int vnt_int_start_interrupt(struct vnt_private *priv);
 void vnt_int_process_data(struct vnt_private *priv);
 
 #endif /* __INT_H__ */
diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
index ccafcc2c87ac..71e10b9ae253 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -483,6 +483,7 @@ static void vnt_tx_80211(struct ieee80211_hw *hw,
 
 static int vnt_start(struct ieee80211_hw *hw)
 {
+	int err = 0;
 	struct vnt_private *priv = hw->priv;
 
 	priv->rx_buf_sz = MAX_TOTAL_SIZE_WITH_ALL_HEADERS;
@@ -496,15 +497,20 @@ static int vnt_start(struct ieee80211_hw *hw)
 
 	if (vnt_init_registers(priv) == false) {
 		dev_dbg(&priv->usb->dev, " init register fail\n");
+		err = -ENOMEM;
 		goto free_all;
 	}
 
-	if (vnt_key_init_table(priv))
+	if (vnt_key_init_table(priv)) {
+		err = -ENOMEM;
 		goto free_all;
+	}
 
 	priv->int_interval = 1;  /* bInterval is set to 1 */
 
-	vnt_int_start_interrupt(priv);
+	err = vnt_int_start_interrupt(priv);
+	if (err)
+		goto free_all;
 
 	ieee80211_wake_queues(hw);
 
@@ -518,7 +524,7 @@ static int vnt_start(struct ieee80211_hw *hw)
 	usb_kill_urb(priv->interrupt_urb);
 	usb_free_urb(priv->interrupt_urb);
 
-	return -ENOMEM;
+	return err;
 }
 
 static void vnt_stop(struct ieee80211_hw *hw)
-- 
2.17.1

Re: [PATCH v2] staging: vt6656: returns error code on vnt_int_start_interrupt fail

[Greg Kroah-Hartman]

On Thu, May 16, 2019 at 11:57:15AM +0000, Quentin Deslandes wrote:
> Returns error code from 'vnt_int_start_interrupt()' so the device's private
> buffers will be correctly freed and 'struct ieee80211_hw' start function
> will return an error code.
> 
> Signed-off-by: Quentin Deslandes <quentin.deslandes@itdev.co.uk>
> ---
>  drivers/staging/vt6656/int.c      |  4 +++-
>  drivers/staging/vt6656/int.h      |  2 +-
>  drivers/staging/vt6656/main_usb.c | 12 +++++++++---
>  3 files changed, 13 insertions(+), 5 deletions(-)

What changed from v1?  Always put that below the --- line.

Please fix up and resend a v3.

thanks,

greg k-h

[PATCH v3] staging: vt6656: returns error code on vnt_int_start_interrupt fail

[Quentin Deslandes]

Returns error code from 'vnt_int_start_interrupt()' so the device's private
buffers will be correctly freed and 'struct ieee80211_hw' start function
will return an error code.

Signed-off-by: Quentin Deslandes <quentin.deslandes@itdev.co.uk>
---
v2: returns 'status' value to caller instead of removing it.
v3: add patch version details. Thanks to Greg K-H for his help.

 drivers/staging/vt6656/int.c      |  4 +++-
 drivers/staging/vt6656/int.h      |  2 +-
 drivers/staging/vt6656/main_usb.c | 12 +++++++++---
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/vt6656/int.c b/drivers/staging/vt6656/int.c
index 504424b19fcf..f3ee2198e1b3 100644
--- a/drivers/staging/vt6656/int.c
+++ b/drivers/staging/vt6656/int.c
@@ -39,7 +39,7 @@ static const u8 fallback_rate1[5][5] = {
 	{RATE_54M, RATE_54M, RATE_36M, RATE_18M, RATE_18M}
 };
 
-void vnt_int_start_interrupt(struct vnt_private *priv)
+int vnt_int_start_interrupt(struct vnt_private *priv)
 {
 	unsigned long flags;
 	int status;
@@ -51,6 +51,8 @@ void vnt_int_start_interrupt(struct vnt_private *priv)
 	status = vnt_start_interrupt_urb(priv);
 
 	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return status;
 }
 
 static int vnt_int_report_rate(struct vnt_private *priv, u8 pkt_no, u8 tsr)
diff --git a/drivers/staging/vt6656/int.h b/drivers/staging/vt6656/int.h
index 987c454e99e9..8a6d60569ceb 100644
--- a/drivers/staging/vt6656/int.h
+++ b/drivers/staging/vt6656/int.h
@@ -41,7 +41,7 @@ struct vnt_interrupt_data {
 	u8 sw[2];
 } __packed;
 
-void vnt_int_start_interrupt(struct vnt_private *priv);
+int vnt_int_start_interrupt(struct vnt_private *priv);
 void vnt_int_process_data(struct vnt_private *priv);
 
 #endif /* __INT_H__ */
diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
index ccafcc2c87ac..71e10b9ae253 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -483,6 +483,7 @@ static void vnt_tx_80211(struct ieee80211_hw *hw,
 
 static int vnt_start(struct ieee80211_hw *hw)
 {
+	int err = 0;
 	struct vnt_private *priv = hw->priv;
 
 	priv->rx_buf_sz = MAX_TOTAL_SIZE_WITH_ALL_HEADERS;
@@ -496,15 +497,20 @@ static int vnt_start(struct ieee80211_hw *hw)
 
 	if (vnt_init_registers(priv) == false) {
 		dev_dbg(&priv->usb->dev, " init register fail\n");
+		err = -ENOMEM;
 		goto free_all;
 	}
 
-	if (vnt_key_init_table(priv))
+	if (vnt_key_init_table(priv)) {
+		err = -ENOMEM;
 		goto free_all;
+	}
 
 	priv->int_interval = 1;  /* bInterval is set to 1 */
 
-	vnt_int_start_interrupt(priv);
+	err = vnt_int_start_interrupt(priv);
+	if (err)
+		goto free_all;
 
 	ieee80211_wake_queues(hw);
 
@@ -518,7 +524,7 @@ static int vnt_start(struct ieee80211_hw *hw)
 	usb_kill_urb(priv->interrupt_urb);
 	usb_free_urb(priv->interrupt_urb);
 
-	return -ENOMEM;
+	return err;
 }
 
 static void vnt_stop(struct ieee80211_hw *hw)
-- 
2.17.1

Re: [PATCH v3] staging: vt6656: returns error code on vnt_int_start_interrupt fail

[Greg Kroah-Hartman]

On Fri, May 17, 2019 at 07:53:49AM +0000, Quentin Deslandes wrote:
> Returns error code from 'vnt_int_start_interrupt()' so the device's private
> buffers will be correctly freed and 'struct ieee80211_hw' start function
> will return an error code.
> 
> Signed-off-by: Quentin Deslandes <quentin.deslandes@itdev.co.uk>
> ---
> v2: returns 'status' value to caller instead of removing it.
> v3: add patch version details. Thanks to Greg K-H for his help.

Looking better!

But a few minor things below:

> 
>  drivers/staging/vt6656/int.c      |  4 +++-
>  drivers/staging/vt6656/int.h      |  2 +-
>  drivers/staging/vt6656/main_usb.c | 12 +++++++++---
>  3 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/vt6656/int.c b/drivers/staging/vt6656/int.c
> index 504424b19fcf..f3ee2198e1b3 100644
> --- a/drivers/staging/vt6656/int.c
> +++ b/drivers/staging/vt6656/int.c
> @@ -39,7 +39,7 @@ static const u8 fallback_rate1[5][5] = {
>  	{RATE_54M, RATE_54M, RATE_36M, RATE_18M, RATE_18M}
>  };
>  
> -void vnt_int_start_interrupt(struct vnt_private *priv)
> +int vnt_int_start_interrupt(struct vnt_private *priv)
>  {
>  	unsigned long flags;
>  	int status;
> @@ -51,6 +51,8 @@ void vnt_int_start_interrupt(struct vnt_private *priv)
>  	status = vnt_start_interrupt_urb(priv);
>  
>  	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	return status;
>  }
>  
>  static int vnt_int_report_rate(struct vnt_private *priv, u8 pkt_no, u8 tsr)
> diff --git a/drivers/staging/vt6656/int.h b/drivers/staging/vt6656/int.h
> index 987c454e99e9..8a6d60569ceb 100644
> --- a/drivers/staging/vt6656/int.h
> +++ b/drivers/staging/vt6656/int.h
> @@ -41,7 +41,7 @@ struct vnt_interrupt_data {
>  	u8 sw[2];
>  } __packed;
>  
> -void vnt_int_start_interrupt(struct vnt_private *priv);
> +int vnt_int_start_interrupt(struct vnt_private *priv);
>  void vnt_int_process_data(struct vnt_private *priv);
>  
>  #endif /* __INT_H__ */
> diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> index ccafcc2c87ac..71e10b9ae253 100644
> --- a/drivers/staging/vt6656/main_usb.c
> +++ b/drivers/staging/vt6656/main_usb.c
> @@ -483,6 +483,7 @@ static void vnt_tx_80211(struct ieee80211_hw *hw,
>  
>  static int vnt_start(struct ieee80211_hw *hw)
>  {
> +	int err = 0;
>  	struct vnt_private *priv = hw->priv;
>  
>  	priv->rx_buf_sz = MAX_TOTAL_SIZE_WITH_ALL_HEADERS;
> @@ -496,15 +497,20 @@ static int vnt_start(struct ieee80211_hw *hw)
>  
>  	if (vnt_init_registers(priv) == false) {
>  		dev_dbg(&priv->usb->dev, " init register fail\n");
> +		err = -ENOMEM;

Why ENOMEM?  vnt_init_registers() should return a proper error code,
based on what went wrong, not true/false.  So fix that up first, and
then you can do this patch.

See, your one tiny coding style fix is turning into real cleanups, nice!

>  		goto free_all;
>  	}
>  
> -	if (vnt_key_init_table(priv))
> +	if (vnt_key_init_table(priv)) {
> +		err = -ENOMEM;

Same here, vnt_key_init_table() should return a real error value, and
then just return that here.

>  		goto free_all;
> +	}
>  
>  	priv->int_interval = 1;  /* bInterval is set to 1 */
>  
> -	vnt_int_start_interrupt(priv);
> +	err = vnt_int_start_interrupt(priv);
> +	if (err)
> +		goto free_all;

Like this, that is the correct thing.

So, this is now going to be a patch series, fixing up those two
functions (and any functions they call possibly), and then this can be
the last patch of the series.

thanks,

greg k-h

Re: [PATCH v3] staging: vt6656: returns error code on vnt_int_start_interrupt fail

[Quentin Deslandes]

On Fri, May 17, 2019 at 11:17:23AM +0200, Greg Kroah-Hartman wrote:
> On Fri, May 17, 2019 at 07:53:49AM +0000, Quentin Deslandes wrote:
> > Returns error code from 'vnt_int_start_interrupt()' so the device's private
> > buffers will be correctly freed and 'struct ieee80211_hw' start function
> > will return an error code.
> > 
> > Signed-off-by: Quentin Deslandes <quentin.deslandes@itdev.co.uk>
> > ---
> > v2: returns 'status' value to caller instead of removing it.
> > v3: add patch version details. Thanks to Greg K-H for his help.
> 
> Looking better!
> 
> But a few minor things below:
> 
> > 
> >  drivers/staging/vt6656/int.c      |  4 +++-
> >  drivers/staging/vt6656/int.h      |  2 +-
> >  drivers/staging/vt6656/main_usb.c | 12 +++++++++---
> >  3 files changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/vt6656/int.c b/drivers/staging/vt6656/int.c
> > index 504424b19fcf..f3ee2198e1b3 100644
> > --- a/drivers/staging/vt6656/int.c
> > +++ b/drivers/staging/vt6656/int.c
> > @@ -39,7 +39,7 @@ static const u8 fallback_rate1[5][5] = {
> >  	{RATE_54M, RATE_54M, RATE_36M, RATE_18M, RATE_18M}
> >  };
> >  
> > -void vnt_int_start_interrupt(struct vnt_private *priv)
> > +int vnt_int_start_interrupt(struct vnt_private *priv)
> >  {
> >  	unsigned long flags;
> >  	int status;
> > @@ -51,6 +51,8 @@ void vnt_int_start_interrupt(struct vnt_private *priv)
> >  	status = vnt_start_interrupt_urb(priv);
> >  
> >  	spin_unlock_irqrestore(&priv->lock, flags);
> > +
> > +	return status;
> >  }
> >  
> >  static int vnt_int_report_rate(struct vnt_private *priv, u8 pkt_no, u8 tsr)
> > diff --git a/drivers/staging/vt6656/int.h b/drivers/staging/vt6656/int.h
> > index 987c454e99e9..8a6d60569ceb 100644
> > --- a/drivers/staging/vt6656/int.h
> > +++ b/drivers/staging/vt6656/int.h
> > @@ -41,7 +41,7 @@ struct vnt_interrupt_data {
> >  	u8 sw[2];
> >  } __packed;
> >  
> > -void vnt_int_start_interrupt(struct vnt_private *priv);
> > +int vnt_int_start_interrupt(struct vnt_private *priv);
> >  void vnt_int_process_data(struct vnt_private *priv);
> >  
> >  #endif /* __INT_H__ */
> > diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> > index ccafcc2c87ac..71e10b9ae253 100644
> > --- a/drivers/staging/vt6656/main_usb.c
> > +++ b/drivers/staging/vt6656/main_usb.c
> > @@ -483,6 +483,7 @@ static void vnt_tx_80211(struct ieee80211_hw *hw,
> >  
> >  static int vnt_start(struct ieee80211_hw *hw)
> >  {
> > +	int err = 0;
> >  	struct vnt_private *priv = hw->priv;
> >  
> >  	priv->rx_buf_sz = MAX_TOTAL_SIZE_WITH_ALL_HEADERS;
> > @@ -496,15 +497,20 @@ static int vnt_start(struct ieee80211_hw *hw)
> >  
> >  	if (vnt_init_registers(priv) == false) {
> >  		dev_dbg(&priv->usb->dev, " init register fail\n");
> > +		err = -ENOMEM;
> 
> Why ENOMEM?  vnt_init_registers() should return a proper error code,
> based on what went wrong, not true/false.  So fix that up first, and
> then you can do this patch.
> 
> See, your one tiny coding style fix is turning into real cleanups, nice!
> 
> >  		goto free_all;
> >  	}
> >  
> > -	if (vnt_key_init_table(priv))
> > +	if (vnt_key_init_table(priv)) {
> > +		err = -ENOMEM;
> 
> Same here, vnt_key_init_table() should return a real error value, and
> then just return that here.
> 
> >  		goto free_all;
> > +	}
> >  
> >  	priv->int_interval = 1;  /* bInterval is set to 1 */
> >  
> > -	vnt_int_start_interrupt(priv);
> > +	err = vnt_int_start_interrupt(priv);
> > +	if (err)
> > +		goto free_all;
> 
> Like this, that is the correct thing.
> 
> So, this is now going to be a patch series, fixing up those two
> functions (and any functions they call possibly), and then this can be
> the last patch of the series.
> 
> thanks,
> 
> greg k-h

Thank you for your help, this is getting really exciting! However, I had
a look at these function (vnt_init_registers() and vnt_key_init_table())
and some questions popped in my mind.

If I understand correctly, your request is to fix these function so they
can return an error code instead of just failing, as I did with
vnt_int_start_interrupt() in the third patch, which is also the most
logical behaviour.

So, vnt_init_registers() is a big function (~240 lines), which should
return a proper error code. For this, all the function called in
vnt_init_registers() should also return a proper error code, right?

What about functions called that does not return any value, but their only
action is to call a function that return a status code? As I learn with this
patch, discarding error values is not a acceptable behaviour. Why would we write
functions returning an error code solely to discard it? So such function should
be changed too?

I listed up to 22 function that need to be updated in order to correctly
propagate errors up to vnt_start() so it could "nicely" fail and here is
the last problem: regarding this fair amount of changes, how to ensure
the device will work as well as before? I don't have this device at home
or at work and it doesn't seems easy to find.

Thank you,
Quentin

Re: [PATCH v3] staging: vt6656: returns error code on vnt_int_start_interrupt fail

[Greg Kroah-Hartman]

On Fri, May 17, 2019 at 01:15:43PM +0000, Quentin Deslandes wrote:
> On Fri, May 17, 2019 at 11:17:23AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, May 17, 2019 at 07:53:49AM +0000, Quentin Deslandes wrote:
> > > Returns error code from 'vnt_int_start_interrupt()' so the device's private
> > > buffers will be correctly freed and 'struct ieee80211_hw' start function
> > > will return an error code.
> > > 
> > > Signed-off-by: Quentin Deslandes <quentin.deslandes@itdev.co.uk>
> > > ---
> > > v2: returns 'status' value to caller instead of removing it.
> > > v3: add patch version details. Thanks to Greg K-H for his help.
> > 
> > Looking better!
> > 
> > But a few minor things below:
> > 
> > > 
> > >  drivers/staging/vt6656/int.c      |  4 +++-
> > >  drivers/staging/vt6656/int.h      |  2 +-
> > >  drivers/staging/vt6656/main_usb.c | 12 +++++++++---
> > >  3 files changed, 13 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/staging/vt6656/int.c b/drivers/staging/vt6656/int.c
> > > index 504424b19fcf..f3ee2198e1b3 100644
> > > --- a/drivers/staging/vt6656/int.c
> > > +++ b/drivers/staging/vt6656/int.c
> > > @@ -39,7 +39,7 @@ static const u8 fallback_rate1[5][5] = {
> > >  	{RATE_54M, RATE_54M, RATE_36M, RATE_18M, RATE_18M}
> > >  };
> > >  
> > > -void vnt_int_start_interrupt(struct vnt_private *priv)
> > > +int vnt_int_start_interrupt(struct vnt_private *priv)
> > >  {
> > >  	unsigned long flags;
> > >  	int status;
> > > @@ -51,6 +51,8 @@ void vnt_int_start_interrupt(struct vnt_private *priv)
> > >  	status = vnt_start_interrupt_urb(priv);
> > >  
> > >  	spin_unlock_irqrestore(&priv->lock, flags);
> > > +
> > > +	return status;
> > >  }
> > >  
> > >  static int vnt_int_report_rate(struct vnt_private *priv, u8 pkt_no, u8 tsr)
> > > diff --git a/drivers/staging/vt6656/int.h b/drivers/staging/vt6656/int.h
> > > index 987c454e99e9..8a6d60569ceb 100644
> > > --- a/drivers/staging/vt6656/int.h
> > > +++ b/drivers/staging/vt6656/int.h
> > > @@ -41,7 +41,7 @@ struct vnt_interrupt_data {
> > >  	u8 sw[2];
> > >  } __packed;
> > >  
> > > -void vnt_int_start_interrupt(struct vnt_private *priv);
> > > +int vnt_int_start_interrupt(struct vnt_private *priv);
> > >  void vnt_int_process_data(struct vnt_private *priv);
> > >  
> > >  #endif /* __INT_H__ */
> > > diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> > > index ccafcc2c87ac..71e10b9ae253 100644
> > > --- a/drivers/staging/vt6656/main_usb.c
> > > +++ b/drivers/staging/vt6656/main_usb.c
> > > @@ -483,6 +483,7 @@ static void vnt_tx_80211(struct ieee80211_hw *hw,
> > >  
> > >  static int vnt_start(struct ieee80211_hw *hw)
> > >  {
> > > +	int err = 0;
> > >  	struct vnt_private *priv = hw->priv;
> > >  
> > >  	priv->rx_buf_sz = MAX_TOTAL_SIZE_WITH_ALL_HEADERS;
> > > @@ -496,15 +497,20 @@ static int vnt_start(struct ieee80211_hw *hw)
> > >  
> > >  	if (vnt_init_registers(priv) == false) {
> > >  		dev_dbg(&priv->usb->dev, " init register fail\n");
> > > +		err = -ENOMEM;
> > 
> > Why ENOMEM?  vnt_init_registers() should return a proper error code,
> > based on what went wrong, not true/false.  So fix that up first, and
> > then you can do this patch.
> > 
> > See, your one tiny coding style fix is turning into real cleanups, nice!
> > 
> > >  		goto free_all;
> > >  	}
> > >  
> > > -	if (vnt_key_init_table(priv))
> > > +	if (vnt_key_init_table(priv)) {
> > > +		err = -ENOMEM;
> > 
> > Same here, vnt_key_init_table() should return a real error value, and
> > then just return that here.
> > 
> > >  		goto free_all;
> > > +	}
> > >  
> > >  	priv->int_interval = 1;  /* bInterval is set to 1 */
> > >  
> > > -	vnt_int_start_interrupt(priv);
> > > +	err = vnt_int_start_interrupt(priv);
> > > +	if (err)
> > > +		goto free_all;
> > 
> > Like this, that is the correct thing.
> > 
> > So, this is now going to be a patch series, fixing up those two
> > functions (and any functions they call possibly), and then this can be
> > the last patch of the series.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Thank you for your help, this is getting really exciting! However, I had
> a look at these function (vnt_init_registers() and vnt_key_init_table())
> and some questions popped in my mind.
> 
> If I understand correctly, your request is to fix these function so they
> can return an error code instead of just failing, as I did with
> vnt_int_start_interrupt() in the third patch, which is also the most
> logical behaviour.

Yes, that is correct.

> So, vnt_init_registers() is a big function (~240 lines), which should
> return a proper error code. For this, all the function called in
> vnt_init_registers() should also return a proper error code, right?

Correct.

> What about functions called that does not return any value, but their only
> action is to call a function that return a status code? As I learn with this
> patch, discarding error values is not a acceptable behaviour. Why would we write
> functions returning an error code solely to discard it? So such function should
> be changed too?

Yes, those functions need to be changed too.

> I listed up to 22 function that need to be updated in order to correctly
> propagate errors up to vnt_start() so it could "nicely" fail and here is
> the last problem: regarding this fair amount of changes, how to ensure
> the device will work as well as before? I don't have this device at home
> or at work and it doesn't seems easy to find.

Start small, at the "root" of the call chain, and work backwards.  You
aren't changing any logic, only passing the errors, if there are any,
back on up.

Now if the driver was relying on those functions to fail, that's a bug
in the driver, and someone who has the hardware will notice and send us
an email saying that the patches broke something.  But that's a few
months out, don't worry about that now :)

thanks,

greg k-h