Received-SPF: pass (google.com: domain of amsfield22@gmail.com designates 2607:f8b0:400e:c03::234 as permitted sender) client-ip=2607:f8b0:400e:c03::234;
Date: Tue, 16 Feb 2016 12:58:57 -0800
From: Alison Schofield <amsfield22@gmail.com>
To: Julia Lawall <julia.lawall@lip6.fr>
Cc: outreachy-kernel@googlegroups.com
Subject: Re: [Outreachy kernel] [PATCH v2] staging: vt6656: move local var
 init into declaration
Message-ID: <20160216205854.GA3156@d830.WORKGROUP>
References: <20160212070014.GA13288@d830.WORKGROUP>
 <1455302683-5119-1-git-send-email-amsfield22@gmail.com>
 <alpine.DEB.2.10.1602121954390.2510@hadrien>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.DEB.2.10.1602121954390.2510@hadrien>
User-Agent: Mutt/1.5.23 (2014-03-12)

On Fri, Feb 12, 2016 at 07:58:05PM +0100, Julia Lawall wrote:
> 
> 
> On Fri, 12 Feb 2016, Alison Schofield wrote:
> 
> > Improve readability by initializing local variables at declaration.
> >
> > In one instance, vnt_start_interrupt_urb_complete(), also use that
> > local variable in subsequent switch.
> >
> > Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> > ---

.............snip

> >
> > diff --git a/drivers/staging/vt6656/usbpipe.c b/drivers/staging/vt6656/usbpipe.c
> > index bec5baf..f27eb9a 100644
> > --- a/drivers/staging/vt6656/usbpipe.c
> > +++ b/drivers/staging/vt6656/usbpipe.c
> > @@ -101,9 +101,9 @@ void vnt_control_in_u8(struct vnt_private *priv, u8 reg, u8 reg_off, u8 *data)
> >  static void vnt_start_interrupt_urb_complete(struct urb *urb)
> >  {
> >  	struct vnt_private *priv = urb->context;
> > -	int status;
> > +	int status = urb->status;
> >
> > -	switch (urb->status) {
> > +	switch (status) {
> >  	case 0:
> >  	case -ETIMEDOUT:
> >  		break;
> > @@ -116,8 +116,6 @@ static void vnt_start_interrupt_urb_complete(struct urb *urb)
> >  		break;
> >  	}
> >
> > -	status = urb->status;
> > -
> >  	if (status != STATUS_SUCCESS) {
> 
> Not related to this patch, but this test is very unlikeable.
> SUCCESS_STATUS is defined in the following (vt6656/device.h)
> 
> enum {
>         STATUS_SUCCESS = 0,
>         STATUS_FAILURE,
>         STATUS_RESOURCES,
>         STATUS_PENDING,
> };
> 
> This is not at all the same kind of value as -ETIMEDOUT, etc.  The urb
> status is something that is not set in this driver as well.  I guess is it
> set in the USB library, where STATUS_SUCCESS is not known.  So status
> should not be compared against this value.
> 
> julia
>
I see that a "successful" urb status is simply 0 and the failures
are standard E* statuses.  This is evident directly above in the
switch, and I confirmed it in drivers/usb code samples.

The STATUS_SUCCESS is used correctly elsewhere for NDIS status per it's
definition. 

I'll send a patch for this.

When I do a subsequent patch like this, should I send it as a reply
here in this email thread, or post a new message? I'm not sure if it
would get buried in here since Greg has already taken the first patch?

Thanks,
alison


.................snip