[PATCH] staging: vt6656: remove unused variable

[PATCH] staging: vt6656: remove unused variable

[Quentin Deslandes]

Rml4ZWQgJ3NldCBidXQgbm90IHVzZWQnIHdhcm5pbmcgbWVzc2FnZSBvbiBhIHN0YXR1cyB2YXJp
YWJsZS4gVGhlDQpjYWxsZWQgZnVuY3Rpb24gcmV0dXJuaW5nIHRoZSBzdGF0dXMgY29kZSAndm50
X3N0YXJ0X2ludGVycnVwdF91cmIoKScNCmNsZWFuIHVwIGFmdGVyIGl0c2VsZiBhbmQgdGhlIGNh
bGxlciBmdW5jdGlvbg0KJ3ZudF9pbnRfc3RhcnRfaW50ZXJydXB0KCknIGRvZXMgbm90IHJldHVy
bnMgYW55IHZhbHVlLg0KDQpTaWduZWQtb2ZmLWJ5OiBRdWVudGluIERlc2xhbmRlcyA8cXVlbnRp
bi5kZXNsYW5kZXNAaXRkZXYuY28udWs+DQotLS0NCiBkcml2ZXJzL3N0YWdpbmcvdnQ2NjU2L2lu
dC5jIHwgMyArLS0NCiAxIGZpbGUgY2hhbmdlZCwgMSBpbnNlcnRpb24oKyksIDIgZGVsZXRpb25z
KC0pDQoNCmRpZmYgLS1naXQgYS9kcml2ZXJzL3N0YWdpbmcvdnQ2NjU2L2ludC5jIGIvZHJpdmVy
cy9zdGFnaW5nL3Z0NjY1Ni9pbnQuYw0KaW5kZXggNTA0NDI0YjE5ZmNmLi5hYzMwY2U3MmRiNWEg
MTAwNjQ0DQotLS0gYS9kcml2ZXJzL3N0YWdpbmcvdnQ2NjU2L2ludC5jDQorKysgYi9kcml2ZXJz
L3N0YWdpbmcvdnQ2NjU2L2ludC5jDQpAQCAtNDIsMTMgKzQyLDEyIEBAIHN0YXRpYyBjb25zdCB1
OCBmYWxsYmFja19yYXRlMVs1XVs1XSA9IHsNCiB2b2lkIHZudF9pbnRfc3RhcnRfaW50ZXJydXB0
KHN0cnVjdCB2bnRfcHJpdmF0ZSAqcHJpdikNCiB7DQogCXVuc2lnbmVkIGxvbmcgZmxhZ3M7DQot
CWludCBzdGF0dXM7DQogDQogCWRldl9kYmcoJnByaXYtPnVzYi0+ZGV2LCAiLS0tLT5JbnRlcnJ1
cHQgUG9sbGluZyBUaHJlYWRcbiIpOw0KIA0KIAlzcGluX2xvY2tfaXJxc2F2ZSgmcHJpdi0+bG9j
aywgZmxhZ3MpOw0KIA0KLQlzdGF0dXMgPSB2bnRfc3RhcnRfaW50ZXJydXB0X3VyYihwcml2KTsN
CisJdm50X3N0YXJ0X2ludGVycnVwdF91cmIocHJpdik7DQogDQogCXNwaW5fdW5sb2NrX2lycXJl
c3RvcmUoJnByaXYtPmxvY2ssIGZsYWdzKTsNCiB9DQotLSANCjIuMTcuMQ0KDQo

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]

UmV0dXJucyBlcnJvciBjb2RlIGZyb20gJ3ZudF9pbnRfc3RhcnRfaW50ZXJydXB0KCknIHNvIHRo
ZSBkZXZpY2UncyBwcml2YXRlDQpidWZmZXJzIHdpbGwgYmUgY29ycmVjdGx5IGZyZWVkIGFuZCAn
c3RydWN0IGllZWU4MDIxMV9odycgc3RhcnQgZnVuY3Rpb24NCndpbGwgcmV0dXJuIGFuIGVycm9y
IGNvZGUuDQoNClNpZ25lZC1vZmYtYnk6IFF1ZW50aW4gRGVzbGFuZGVzIDxxdWVudGluLmRlc2xh
bmRlc0BpdGRldi5jby51az4NCi0tLQ0KdjI6IGluc3RlYWQgb2YgcmVtb3Zpbmcgc3RhdHVzIHZh
cmlhYmxlLCByZXR1cm5zIGl0cyB2YWx1ZSB0byBjYWxsZXIgYW5kDQogICAgaGFuZGxlIGVycm9y
IGluIGNhbGxlci4NCg0KIGRyaXZlcnMvc3RhZ2luZy92dDY2NTYvaW50LmMgICAgICB8ICA0ICsr
Ky0NCiBkcml2ZXJzL3N0YWdpbmcvdnQ2NjU2L2ludC5oICAgICAgfCAgMiArLQ0KIGRyaXZlcnMv
c3RhZ2luZy92dDY2NTYvbWFpbl91c2IuYyB8IDEyICsrKysrKysrKy0tLQ0KIDMgZmlsZXMgY2hh
bmdlZCwgMTMgaW5zZXJ0aW9ucygrKSwgNSBkZWxldGlvbnMoLSkNCg0KZGlmZiAtLWdpdCBhL2Ry
aXZlcnMvc3RhZ2luZy92dDY2NTYvaW50LmMgYi9kcml2ZXJzL3N0YWdpbmcvdnQ2NjU2L2ludC5j
DQppbmRleCA1MDQ0MjRiMTlmY2YuLmYzZWUyMTk4ZTFiMyAxMDA2NDQNCi0tLSBhL2RyaXZlcnMv
c3RhZ2luZy92dDY2NTYvaW50LmMNCisrKyBiL2RyaXZlcnMvc3RhZ2luZy92dDY2NTYvaW50LmMN
CkBAIC0zOSw3ICszOSw3IEBAIHN0YXRpYyBjb25zdCB1OCBmYWxsYmFja19yYXRlMVs1XVs1XSA9
IHsNCiAJe1JBVEVfNTRNLCBSQVRFXzU0TSwgUkFURV8zNk0sIFJBVEVfMThNLCBSQVRFXzE4TX0N
CiB9Ow0KIA0KLXZvaWQgdm50X2ludF9zdGFydF9pbnRlcnJ1cHQoc3RydWN0IHZudF9wcml2YXRl
ICpwcml2KQ0KK2ludCB2bnRfaW50X3N0YXJ0X2ludGVycnVwdChzdHJ1Y3Qgdm50X3ByaXZhdGUg
KnByaXYpDQogew0KIAl1bnNpZ25lZCBsb25nIGZsYWdzOw0KIAlpbnQgc3RhdHVzOw0KQEAgLTUx
LDYgKzUxLDggQEAgdm9pZCB2bnRfaW50X3N0YXJ0X2ludGVycnVwdChzdHJ1Y3Qgdm50X3ByaXZh
dGUgKnByaXYpDQogCXN0YXR1cyA9IHZudF9zdGFydF9pbnRlcnJ1cHRfdXJiKHByaXYpOw0KIA0K
IAlzcGluX3VubG9ja19pcnFyZXN0b3JlKCZwcml2LT5sb2NrLCBmbGFncyk7DQorDQorCXJldHVy
biBzdGF0dXM7DQogfQ0KIA0KIHN0YXRpYyBpbnQgdm50X2ludF9yZXBvcnRfcmF0ZShzdHJ1Y3Qg
dm50X3ByaXZhdGUgKnByaXYsIHU4IHBrdF9ubywgdTggdHNyKQ0KZGlmZiAtLWdpdCBhL2RyaXZl
cnMvc3RhZ2luZy92dDY2NTYvaW50LmggYi9kcml2ZXJzL3N0YWdpbmcvdnQ2NjU2L2ludC5oDQpp
bmRleCA5ODdjNDU0ZTk5ZTkuLjhhNmQ2MDU2OWNlYiAxMDA2NDQNCi0tLSBhL2RyaXZlcnMvc3Rh
Z2luZy92dDY2NTYvaW50LmgNCisrKyBiL2RyaXZlcnMvc3RhZ2luZy92dDY2NTYvaW50LmgNCkBA
IC00MSw3ICs0MSw3IEBAIHN0cnVjdCB2bnRfaW50ZXJydXB0X2RhdGEgew0KIAl1OCBzd1syXTsN
CiB9IF9fcGFja2VkOw0KIA0KLXZvaWQgdm50X2ludF9zdGFydF9pbnRlcnJ1cHQoc3RydWN0IHZu
dF9wcml2YXRlICpwcml2KTsNCitpbnQgdm50X2ludF9zdGFydF9pbnRlcnJ1cHQoc3RydWN0IHZu
dF9wcml2YXRlICpwcml2KTsNCiB2b2lkIHZudF9pbnRfcHJvY2Vzc19kYXRhKHN0cnVjdCB2bnRf
cHJpdmF0ZSAqcHJpdik7DQogDQogI2VuZGlmIC8qIF9fSU5UX0hfXyAqLw0KZGlmZiAtLWdpdCBh
L2RyaXZlcnMvc3RhZ2luZy92dDY2NTYvbWFpbl91c2IuYyBiL2RyaXZlcnMvc3RhZ2luZy92dDY2
NTYvbWFpbl91c2IuYw0KaW5kZXggY2NhZmNjMmM4N2FjLi43MWUxMGI5YWUyNTMgMTAwNjQ0DQot
LS0gYS9kcml2ZXJzL3N0YWdpbmcvdnQ2NjU2L21haW5fdXNiLmMNCisrKyBiL2RyaXZlcnMvc3Rh
Z2luZy92dDY2NTYvbWFpbl91c2IuYw0KQEAgLTQ4Myw2ICs0ODMsNyBAQCBzdGF0aWMgdm9pZCB2
bnRfdHhfODAyMTEoc3RydWN0IGllZWU4MDIxMV9odyAqaHcsDQogDQogc3RhdGljIGludCB2bnRf
c3RhcnQoc3RydWN0IGllZWU4MDIxMV9odyAqaHcpDQogew0KKwlpbnQgZXJyID0gMDsNCiAJc3Ry
dWN0IHZudF9wcml2YXRlICpwcml2ID0gaHctPnByaXY7DQogDQogCXByaXYtPnJ4X2J1Zl9zeiA9
IE1BWF9UT1RBTF9TSVpFX1dJVEhfQUxMX0hFQURFUlM7DQpAQCAtNDk2LDE1ICs0OTcsMjAgQEAg
c3RhdGljIGludCB2bnRfc3RhcnQoc3RydWN0IGllZWU4MDIxMV9odyAqaHcpDQogDQogCWlmICh2
bnRfaW5pdF9yZWdpc3RlcnMocHJpdikgPT0gZmFsc2UpIHsNCiAJCWRldl9kYmcoJnByaXYtPnVz
Yi0+ZGV2LCAiIGluaXQgcmVnaXN0ZXIgZmFpbFxuIik7DQorCQllcnIgPSAtRU5PTUVNOw0KIAkJ
Z290byBmcmVlX2FsbDsNCiAJfQ0KIA0KLQlpZiAodm50X2tleV9pbml0X3RhYmxlKHByaXYpKQ0K
KwlpZiAodm50X2tleV9pbml0X3RhYmxlKHByaXYpKSB7DQorCQllcnIgPSAtRU5PTUVNOw0KIAkJ
Z290byBmcmVlX2FsbDsNCisJfQ0KIA0KIAlwcml2LT5pbnRfaW50ZXJ2YWwgPSAxOyAgLyogYklu
dGVydmFsIGlzIHNldCB0byAxICovDQogDQotCXZudF9pbnRfc3RhcnRfaW50ZXJydXB0KHByaXYp
Ow0KKwllcnIgPSB2bnRfaW50X3N0YXJ0X2ludGVycnVwdChwcml2KTsNCisJaWYgKGVycikNCisJ
CWdvdG8gZnJlZV9hbGw7DQogDQogCWllZWU4MDIxMV93YWtlX3F1ZXVlcyhodyk7DQogDQpAQCAt
NTE4LDcgKzUyNCw3IEBAIHN0YXRpYyBpbnQgdm50X3N0YXJ0KHN0cnVjdCBpZWVlODAyMTFfaHcg
Kmh3KQ0KIAl1c2Jfa2lsbF91cmIocHJpdi0+aW50ZXJydXB0X3VyYik7DQogCXVzYl9mcmVlX3Vy
Yihwcml2LT5pbnRlcnJ1cHRfdXJiKTsNCiANCi0JcmV0dXJuIC1FTk9NRU07DQorCXJldHVybiBl
cnI7DQogfQ0KIA0KIHN0YXRpYyB2b2lkIHZudF9zdG9wKHN0cnVjdCBpZWVlODAyMTFfaHcgKmh3
KQ0KLS0gDQoyLjE3LjENCg0K

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

[Quentin Deslandes]

UmV0dXJucyBlcnJvciBjb2RlIGZyb20gJ3ZudF9pbnRfc3RhcnRfaW50ZXJydXB0KCknIHNvIHRo
ZSBkZXZpY2UncyBwcml2YXRlDQpidWZmZXJzIHdpbGwgYmUgY29ycmVjdGx5IGZyZWVkIGFuZCAn
c3RydWN0IGllZWU4MDIxMV9odycgc3RhcnQgZnVuY3Rpb24NCndpbGwgcmV0dXJuIGFuIGVycm9y
IGNvZGUuDQoNClNpZ25lZC1vZmYtYnk6IFF1ZW50aW4gRGVzbGFuZGVzIDxxdWVudGluLmRlc2xh
bmRlc0BpdGRldi5jby51az4NCi0tLQ0KIGRyaXZlcnMvc3RhZ2luZy92dDY2NTYvaW50LmMgICAg
ICB8ICA0ICsrKy0NCiBkcml2ZXJzL3N0YWdpbmcvdnQ2NjU2L2ludC5oICAgICAgfCAgMiArLQ0K
IGRyaXZlcnMvc3RhZ2luZy92dDY2NTYvbWFpbl91c2IuYyB8IDEyICsrKysrKysrKy0tLQ0KIDMg
ZmlsZXMgY2hhbmdlZCwgMTMgaW5zZXJ0aW9ucygrKSwgNSBkZWxldGlvbnMoLSkNCg0KZGlmZiAt
LWdpdCBhL2RyaXZlcnMvc3RhZ2luZy92dDY2NTYvaW50LmMgYi9kcml2ZXJzL3N0YWdpbmcvdnQ2
NjU2L2ludC5jDQppbmRleCA1MDQ0MjRiMTlmY2YuLmYzZWUyMTk4ZTFiMyAxMDA2NDQNCi0tLSBh
L2RyaXZlcnMvc3RhZ2luZy92dDY2NTYvaW50LmMNCisrKyBiL2RyaXZlcnMvc3RhZ2luZy92dDY2
NTYvaW50LmMNCkBAIC0zOSw3ICszOSw3IEBAIHN0YXRpYyBjb25zdCB1OCBmYWxsYmFja19yYXRl
MVs1XVs1XSA9IHsNCiAJe1JBVEVfNTRNLCBSQVRFXzU0TSwgUkFURV8zNk0sIFJBVEVfMThNLCBS
QVRFXzE4TX0NCiB9Ow0KIA0KLXZvaWQgdm50X2ludF9zdGFydF9pbnRlcnJ1cHQoc3RydWN0IHZu
dF9wcml2YXRlICpwcml2KQ0KK2ludCB2bnRfaW50X3N0YXJ0X2ludGVycnVwdChzdHJ1Y3Qgdm50
X3ByaXZhdGUgKnByaXYpDQogew0KIAl1bnNpZ25lZCBsb25nIGZsYWdzOw0KIAlpbnQgc3RhdHVz
Ow0KQEAgLTUxLDYgKzUxLDggQEAgdm9pZCB2bnRfaW50X3N0YXJ0X2ludGVycnVwdChzdHJ1Y3Qg
dm50X3ByaXZhdGUgKnByaXYpDQogCXN0YXR1cyA9IHZudF9zdGFydF9pbnRlcnJ1cHRfdXJiKHBy
aXYpOw0KIA0KIAlzcGluX3VubG9ja19pcnFyZXN0b3JlKCZwcml2LT5sb2NrLCBmbGFncyk7DQor
DQorCXJldHVybiBzdGF0dXM7DQogfQ0KIA0KIHN0YXRpYyBpbnQgdm50X2ludF9yZXBvcnRfcmF0
ZShzdHJ1Y3Qgdm50X3ByaXZhdGUgKnByaXYsIHU4IHBrdF9ubywgdTggdHNyKQ0KZGlmZiAtLWdp
dCBhL2RyaXZlcnMvc3RhZ2luZy92dDY2NTYvaW50LmggYi9kcml2ZXJzL3N0YWdpbmcvdnQ2NjU2
L2ludC5oDQppbmRleCA5ODdjNDU0ZTk5ZTkuLjhhNmQ2MDU2OWNlYiAxMDA2NDQNCi0tLSBhL2Ry
aXZlcnMvc3RhZ2luZy92dDY2NTYvaW50LmgNCisrKyBiL2RyaXZlcnMvc3RhZ2luZy92dDY2NTYv
aW50LmgNCkBAIC00MSw3ICs0MSw3IEBAIHN0cnVjdCB2bnRfaW50ZXJydXB0X2RhdGEgew0KIAl1
OCBzd1syXTsNCiB9IF9fcGFja2VkOw0KIA0KLXZvaWQgdm50X2ludF9zdGFydF9pbnRlcnJ1cHQo
c3RydWN0IHZudF9wcml2YXRlICpwcml2KTsNCitpbnQgdm50X2ludF9zdGFydF9pbnRlcnJ1cHQo
c3RydWN0IHZudF9wcml2YXRlICpwcml2KTsNCiB2b2lkIHZudF9pbnRfcHJvY2Vzc19kYXRhKHN0
cnVjdCB2bnRfcHJpdmF0ZSAqcHJpdik7DQogDQogI2VuZGlmIC8qIF9fSU5UX0hfXyAqLw0KZGlm
ZiAtLWdpdCBhL2RyaXZlcnMvc3RhZ2luZy92dDY2NTYvbWFpbl91c2IuYyBiL2RyaXZlcnMvc3Rh
Z2luZy92dDY2NTYvbWFpbl91c2IuYw0KaW5kZXggY2NhZmNjMmM4N2FjLi43MWUxMGI5YWUyNTMg
MTAwNjQ0DQotLS0gYS9kcml2ZXJzL3N0YWdpbmcvdnQ2NjU2L21haW5fdXNiLmMNCisrKyBiL2Ry
aXZlcnMvc3RhZ2luZy92dDY2NTYvbWFpbl91c2IuYw0KQEAgLTQ4Myw2ICs0ODMsNyBAQCBzdGF0
aWMgdm9pZCB2bnRfdHhfODAyMTEoc3RydWN0IGllZWU4MDIxMV9odyAqaHcsDQogDQogc3RhdGlj
IGludCB2bnRfc3RhcnQoc3RydWN0IGllZWU4MDIxMV9odyAqaHcpDQogew0KKwlpbnQgZXJyID0g
MDsNCiAJc3RydWN0IHZudF9wcml2YXRlICpwcml2ID0gaHctPnByaXY7DQogDQogCXByaXYtPnJ4
X2J1Zl9zeiA9IE1BWF9UT1RBTF9TSVpFX1dJVEhfQUxMX0hFQURFUlM7DQpAQCAtNDk2LDE1ICs0
OTcsMjAgQEAgc3RhdGljIGludCB2bnRfc3RhcnQoc3RydWN0IGllZWU4MDIxMV9odyAqaHcpDQog
DQogCWlmICh2bnRfaW5pdF9yZWdpc3RlcnMocHJpdikgPT0gZmFsc2UpIHsNCiAJCWRldl9kYmco
JnByaXYtPnVzYi0+ZGV2LCAiIGluaXQgcmVnaXN0ZXIgZmFpbFxuIik7DQorCQllcnIgPSAtRU5P
TUVNOw0KIAkJZ290byBmcmVlX2FsbDsNCiAJfQ0KIA0KLQlpZiAodm50X2tleV9pbml0X3RhYmxl
KHByaXYpKQ0KKwlpZiAodm50X2tleV9pbml0X3RhYmxlKHByaXYpKSB7DQorCQllcnIgPSAtRU5P
TUVNOw0KIAkJZ290byBmcmVlX2FsbDsNCisJfQ0KIA0KIAlwcml2LT5pbnRfaW50ZXJ2YWwgPSAx
OyAgLyogYkludGVydmFsIGlzIHNldCB0byAxICovDQogDQotCXZudF9pbnRfc3RhcnRfaW50ZXJy
dXB0KHByaXYpOw0KKwllcnIgPSB2bnRfaW50X3N0YXJ0X2ludGVycnVwdChwcml2KTsNCisJaWYg
KGVycikNCisJCWdvdG8gZnJlZV9hbGw7DQogDQogCWllZWU4MDIxMV93YWtlX3F1ZXVlcyhodyk7
DQogDQpAQCAtNTE4LDcgKzUyNCw3IEBAIHN0YXRpYyBpbnQgdm50X3N0YXJ0KHN0cnVjdCBpZWVl
ODAyMTFfaHcgKmh3KQ0KIAl1c2Jfa2lsbF91cmIocHJpdi0+aW50ZXJydXB0X3VyYik7DQogCXVz
Yl9mcmVlX3VyYihwcml2LT5pbnRlcnJ1cHRfdXJiKTsNCiANCi0JcmV0dXJuIC1FTk9NRU07DQor
CXJldHVybiBlcnI7DQogfQ0KIA0KIHN0YXRpYyB2b2lkIHZudF9zdG9wKHN0cnVjdCBpZWVlODAy
MTFfaHcgKmh3KQ0KLS0gDQoyLjE3LjENCg0K

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]

UmV0dXJucyBlcnJvciBjb2RlIGZyb20gJ3ZudF9pbnRfc3RhcnRfaW50ZXJydXB0KCknIHNvIHRo
ZSBkZXZpY2UncyBwcml2YXRlDQpidWZmZXJzIHdpbGwgYmUgY29ycmVjdGx5IGZyZWVkIGFuZCAn
c3RydWN0IGllZWU4MDIxMV9odycgc3RhcnQgZnVuY3Rpb24NCndpbGwgcmV0dXJuIGFuIGVycm9y
IGNvZGUuDQoNClNpZ25lZC1vZmYtYnk6IFF1ZW50aW4gRGVzbGFuZGVzIDxxdWVudGluLmRlc2xh
bmRlc0BpdGRldi5jby51az4NCi0tLQ0KdjI6IHJldHVybnMgJ3N0YXR1cycgdmFsdWUgdG8gY2Fs
bGVyIGluc3RlYWQgb2YgcmVtb3ZpbmcgaXQuDQp2MzogYWRkIHBhdGNoIHZlcnNpb24gZGV0YWls
cy4gVGhhbmtzIHRvIEdyZWcgSy1IIGZvciBoaXMgaGVscC4NCg0KIGRyaXZlcnMvc3RhZ2luZy92
dDY2NTYvaW50LmMgICAgICB8ICA0ICsrKy0NCiBkcml2ZXJzL3N0YWdpbmcvdnQ2NjU2L2ludC5o
ICAgICAgfCAgMiArLQ0KIGRyaXZlcnMvc3RhZ2luZy92dDY2NTYvbWFpbl91c2IuYyB8IDEyICsr
KysrKysrKy0tLQ0KIDMgZmlsZXMgY2hhbmdlZCwgMTMgaW5zZXJ0aW9ucygrKSwgNSBkZWxldGlv
bnMoLSkNCg0KZGlmZiAtLWdpdCBhL2RyaXZlcnMvc3RhZ2luZy92dDY2NTYvaW50LmMgYi9kcml2
ZXJzL3N0YWdpbmcvdnQ2NjU2L2ludC5jDQppbmRleCA1MDQ0MjRiMTlmY2YuLmYzZWUyMTk4ZTFi
MyAxMDA2NDQNCi0tLSBhL2RyaXZlcnMvc3RhZ2luZy92dDY2NTYvaW50LmMNCisrKyBiL2RyaXZl
cnMvc3RhZ2luZy92dDY2NTYvaW50LmMNCkBAIC0zOSw3ICszOSw3IEBAIHN0YXRpYyBjb25zdCB1
OCBmYWxsYmFja19yYXRlMVs1XVs1XSA9IHsNCiAJe1JBVEVfNTRNLCBSQVRFXzU0TSwgUkFURV8z
Nk0sIFJBVEVfMThNLCBSQVRFXzE4TX0NCiB9Ow0KIA0KLXZvaWQgdm50X2ludF9zdGFydF9pbnRl
cnJ1cHQoc3RydWN0IHZudF9wcml2YXRlICpwcml2KQ0KK2ludCB2bnRfaW50X3N0YXJ0X2ludGVy
cnVwdChzdHJ1Y3Qgdm50X3ByaXZhdGUgKnByaXYpDQogew0KIAl1bnNpZ25lZCBsb25nIGZsYWdz
Ow0KIAlpbnQgc3RhdHVzOw0KQEAgLTUxLDYgKzUxLDggQEAgdm9pZCB2bnRfaW50X3N0YXJ0X2lu
dGVycnVwdChzdHJ1Y3Qgdm50X3ByaXZhdGUgKnByaXYpDQogCXN0YXR1cyA9IHZudF9zdGFydF9p
bnRlcnJ1cHRfdXJiKHByaXYpOw0KIA0KIAlzcGluX3VubG9ja19pcnFyZXN0b3JlKCZwcml2LT5s
b2NrLCBmbGFncyk7DQorDQorCXJldHVybiBzdGF0dXM7DQogfQ0KIA0KIHN0YXRpYyBpbnQgdm50
X2ludF9yZXBvcnRfcmF0ZShzdHJ1Y3Qgdm50X3ByaXZhdGUgKnByaXYsIHU4IHBrdF9ubywgdTgg
dHNyKQ0KZGlmZiAtLWdpdCBhL2RyaXZlcnMvc3RhZ2luZy92dDY2NTYvaW50LmggYi9kcml2ZXJz
L3N0YWdpbmcvdnQ2NjU2L2ludC5oDQppbmRleCA5ODdjNDU0ZTk5ZTkuLjhhNmQ2MDU2OWNlYiAx
MDA2NDQNCi0tLSBhL2RyaXZlcnMvc3RhZ2luZy92dDY2NTYvaW50LmgNCisrKyBiL2RyaXZlcnMv
c3RhZ2luZy92dDY2NTYvaW50LmgNCkBAIC00MSw3ICs0MSw3IEBAIHN0cnVjdCB2bnRfaW50ZXJy
dXB0X2RhdGEgew0KIAl1OCBzd1syXTsNCiB9IF9fcGFja2VkOw0KIA0KLXZvaWQgdm50X2ludF9z
dGFydF9pbnRlcnJ1cHQoc3RydWN0IHZudF9wcml2YXRlICpwcml2KTsNCitpbnQgdm50X2ludF9z
dGFydF9pbnRlcnJ1cHQoc3RydWN0IHZudF9wcml2YXRlICpwcml2KTsNCiB2b2lkIHZudF9pbnRf
cHJvY2Vzc19kYXRhKHN0cnVjdCB2bnRfcHJpdmF0ZSAqcHJpdik7DQogDQogI2VuZGlmIC8qIF9f
SU5UX0hfXyAqLw0KZGlmZiAtLWdpdCBhL2RyaXZlcnMvc3RhZ2luZy92dDY2NTYvbWFpbl91c2Iu
YyBiL2RyaXZlcnMvc3RhZ2luZy92dDY2NTYvbWFpbl91c2IuYw0KaW5kZXggY2NhZmNjMmM4N2Fj
Li43MWUxMGI5YWUyNTMgMTAwNjQ0DQotLS0gYS9kcml2ZXJzL3N0YWdpbmcvdnQ2NjU2L21haW5f
dXNiLmMNCisrKyBiL2RyaXZlcnMvc3RhZ2luZy92dDY2NTYvbWFpbl91c2IuYw0KQEAgLTQ4Myw2
ICs0ODMsNyBAQCBzdGF0aWMgdm9pZCB2bnRfdHhfODAyMTEoc3RydWN0IGllZWU4MDIxMV9odyAq
aHcsDQogDQogc3RhdGljIGludCB2bnRfc3RhcnQoc3RydWN0IGllZWU4MDIxMV9odyAqaHcpDQog
ew0KKwlpbnQgZXJyID0gMDsNCiAJc3RydWN0IHZudF9wcml2YXRlICpwcml2ID0gaHctPnByaXY7
DQogDQogCXByaXYtPnJ4X2J1Zl9zeiA9IE1BWF9UT1RBTF9TSVpFX1dJVEhfQUxMX0hFQURFUlM7
DQpAQCAtNDk2LDE1ICs0OTcsMjAgQEAgc3RhdGljIGludCB2bnRfc3RhcnQoc3RydWN0IGllZWU4
MDIxMV9odyAqaHcpDQogDQogCWlmICh2bnRfaW5pdF9yZWdpc3RlcnMocHJpdikgPT0gZmFsc2Up
IHsNCiAJCWRldl9kYmcoJnByaXYtPnVzYi0+ZGV2LCAiIGluaXQgcmVnaXN0ZXIgZmFpbFxuIik7
DQorCQllcnIgPSAtRU5PTUVNOw0KIAkJZ290byBmcmVlX2FsbDsNCiAJfQ0KIA0KLQlpZiAodm50
X2tleV9pbml0X3RhYmxlKHByaXYpKQ0KKwlpZiAodm50X2tleV9pbml0X3RhYmxlKHByaXYpKSB7
DQorCQllcnIgPSAtRU5PTUVNOw0KIAkJZ290byBmcmVlX2FsbDsNCisJfQ0KIA0KIAlwcml2LT5p
bnRfaW50ZXJ2YWwgPSAxOyAgLyogYkludGVydmFsIGlzIHNldCB0byAxICovDQogDQotCXZudF9p
bnRfc3RhcnRfaW50ZXJydXB0KHByaXYpOw0KKwllcnIgPSB2bnRfaW50X3N0YXJ0X2ludGVycnVw
dChwcml2KTsNCisJaWYgKGVycikNCisJCWdvdG8gZnJlZV9hbGw7DQogDQogCWllZWU4MDIxMV93
YWtlX3F1ZXVlcyhodyk7DQogDQpAQCAtNTE4LDcgKzUyNCw3IEBAIHN0YXRpYyBpbnQgdm50X3N0
YXJ0KHN0cnVjdCBpZWVlODAyMTFfaHcgKmh3KQ0KIAl1c2Jfa2lsbF91cmIocHJpdi0+aW50ZXJy
dXB0X3VyYik7DQogCXVzYl9mcmVlX3VyYihwcml2LT5pbnRlcnJ1cHRfdXJiKTsNCiANCi0JcmV0
dXJuIC1FTk9NRU07DQorCXJldHVybiBlcnI7DQogfQ0KIA0KIHN0YXRpYyB2b2lkIHZudF9zdG9w
KHN0cnVjdCBpZWVlODAyMTFfaHcgKmh3KQ0KLS0gDQoyLjE3LjENCg0K

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