* [PATCH] staging: vt6656: remove unused variable
@ 2019-05-16 9:31 Quentin Deslandes
2019-05-16 9:39 ` Greg Kroah-Hartman
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Quentin Deslandes @ 2019-05-16 9:31 UTC (permalink / raw)
To: kernel-janitors
Cc: Quentin Deslandes, Forest Bond, Greg Kroah-Hartman, devel, linux-kernel
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH] staging: vt6656: remove unused variable 2019-05-16 9:31 [PATCH] staging: vt6656: remove unused variable Quentin Deslandes @ 2019-05-16 9:39 ` Greg Kroah-Hartman 2019-05-16 9:50 ` Quentin Deslandes 2019-05-16 11:22 ` [PATCH] staging: vt6656: returns error code on vnt_int_start_interrupt fail Quentin Deslandes ` (2 subsequent siblings) 3 siblings, 1 reply; 12+ messages in thread From: Greg Kroah-Hartman @ 2019-05-16 9:39 UTC (permalink / raw) To: Quentin Deslandes; +Cc: kernel-janitors, devel, Forest Bond, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: vt6656: remove unused variable 2019-05-16 9:39 ` Greg Kroah-Hartman @ 2019-05-16 9:50 ` Quentin Deslandes 2019-05-16 10:19 ` Greg Kroah-Hartman 0 siblings, 1 reply; 12+ messages in thread From: Quentin Deslandes @ 2019-05-16 9:50 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: kernel-janitors, devel, Forest Bond, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: vt6656: remove unused variable 2019-05-16 9:50 ` Quentin Deslandes @ 2019-05-16 10:19 ` Greg Kroah-Hartman 2019-05-16 10:27 ` Quentin Deslandes 0 siblings, 1 reply; 12+ messages in thread From: Greg Kroah-Hartman @ 2019-05-16 10:19 UTC (permalink / raw) To: Quentin Deslandes; +Cc: kernel-janitors, devel, Forest Bond, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: vt6656: remove unused variable 2019-05-16 10:19 ` Greg Kroah-Hartman @ 2019-05-16 10:27 ` Quentin Deslandes 0 siblings, 0 replies; 12+ messages in thread From: Quentin Deslandes @ 2019-05-16 10:27 UTC (permalink / raw) To: Greg Kroah-Hartman; +Cc: kernel-janitors, devel, Forest Bond, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] staging: vt6656: returns error code on vnt_int_start_interrupt fail 2019-05-16 9:31 [PATCH] staging: vt6656: remove unused variable Quentin Deslandes 2019-05-16 9:39 ` Greg Kroah-Hartman @ 2019-05-16 11:22 ` Quentin Deslandes 2019-05-16 11:57 ` [PATCH v2] " Quentin Deslandes 2019-05-17 7:53 ` [PATCH v3] " Quentin Deslandes 3 siblings, 0 replies; 12+ messages in thread From: Quentin Deslandes @ 2019-05-16 11:22 UTC (permalink / raw) To: kernel-janitors Cc: Quentin Deslandes, Forest Bond, Greg Kroah-Hartman, Mukesh Ojha, Ojaswin Mujoo, devel, linux-kernel 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2] staging: vt6656: returns error code on vnt_int_start_interrupt fail 2019-05-16 9:31 [PATCH] staging: vt6656: remove unused variable Quentin Deslandes 2019-05-16 9:39 ` Greg Kroah-Hartman 2019-05-16 11:22 ` [PATCH] staging: vt6656: returns error code on vnt_int_start_interrupt fail Quentin Deslandes @ 2019-05-16 11:57 ` Quentin Deslandes 2019-05-17 7:31 ` Greg Kroah-Hartman 2019-05-17 7:53 ` [PATCH v3] " Quentin Deslandes 3 siblings, 1 reply; 12+ messages in thread From: Quentin Deslandes @ 2019-05-16 11:57 UTC (permalink / raw) To: kernel-janitors Cc: Quentin Deslandes, Forest Bond, Greg Kroah-Hartman, Mukesh Ojha, Ojaswin Mujoo, devel, linux-kernel 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] staging: vt6656: returns error code on vnt_int_start_interrupt fail 2019-05-16 11:57 ` [PATCH v2] " Quentin Deslandes @ 2019-05-17 7:31 ` Greg Kroah-Hartman 0 siblings, 0 replies; 12+ messages in thread From: Greg Kroah-Hartman @ 2019-05-17 7:31 UTC (permalink / raw) To: Quentin Deslandes Cc: kernel-janitors, devel, Mukesh Ojha, linux-kernel, Forest Bond, Ojaswin Mujoo 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3] staging: vt6656: returns error code on vnt_int_start_interrupt fail 2019-05-16 9:31 [PATCH] staging: vt6656: remove unused variable Quentin Deslandes ` (2 preceding siblings ...) 2019-05-16 11:57 ` [PATCH v2] " Quentin Deslandes @ 2019-05-17 7:53 ` Quentin Deslandes 2019-05-17 9:17 ` Greg Kroah-Hartman 3 siblings, 1 reply; 12+ messages in thread From: Quentin Deslandes @ 2019-05-17 7:53 UTC (permalink / raw) To: kernel-janitors Cc: Quentin Deslandes, Forest Bond, Greg Kroah-Hartman, Ojaswin Mujoo, Mukesh Ojha, devel, linux-kernel 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3] staging: vt6656: returns error code on vnt_int_start_interrupt fail 2019-05-17 7:53 ` [PATCH v3] " Quentin Deslandes @ 2019-05-17 9:17 ` Greg Kroah-Hartman 2019-05-17 13:15 ` Quentin Deslandes 0 siblings, 1 reply; 12+ messages in thread From: Greg Kroah-Hartman @ 2019-05-17 9:17 UTC (permalink / raw) To: Quentin Deslandes Cc: kernel-janitors, devel, Mukesh Ojha, linux-kernel, Forest Bond, Ojaswin Mujoo 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] staging: vt6656: returns error code on vnt_int_start_interrupt fail 2019-05-17 9:17 ` Greg Kroah-Hartman @ 2019-05-17 13:15 ` Quentin Deslandes 2019-05-17 13:29 ` Greg Kroah-Hartman 0 siblings, 1 reply; 12+ messages in thread From: Quentin Deslandes @ 2019-05-17 13:15 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: kernel-janitors, devel, Mukesh Ojha, linux-kernel, Forest Bond, Ojaswin Mujoo 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] staging: vt6656: returns error code on vnt_int_start_interrupt fail 2019-05-17 13:15 ` Quentin Deslandes @ 2019-05-17 13:29 ` Greg Kroah-Hartman 0 siblings, 0 replies; 12+ messages in thread From: Greg Kroah-Hartman @ 2019-05-17 13:29 UTC (permalink / raw) To: Quentin Deslandes Cc: kernel-janitors, devel, Mukesh Ojha, linux-kernel, Forest Bond, Ojaswin Mujoo 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-05-17 13:29 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-16 9:31 [PATCH] staging: vt6656: remove unused variable Quentin Deslandes 2019-05-16 9:39 ` Greg Kroah-Hartman 2019-05-16 9:50 ` Quentin Deslandes 2019-05-16 10:19 ` Greg Kroah-Hartman 2019-05-16 10:27 ` Quentin Deslandes 2019-05-16 11:22 ` [PATCH] staging: vt6656: returns error code on vnt_int_start_interrupt fail Quentin Deslandes 2019-05-16 11:57 ` [PATCH v2] " Quentin Deslandes 2019-05-17 7:31 ` Greg Kroah-Hartman 2019-05-17 7:53 ` [PATCH v3] " Quentin Deslandes 2019-05-17 9:17 ` Greg Kroah-Hartman 2019-05-17 13:15 ` Quentin Deslandes 2019-05-17 13:29 ` Greg Kroah-Hartman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).