[PATCH] staging: vt6656: move local var init closer to usage

[PATCH] staging: vt6656: move local var init closer to usage

[Alison Schofield]

Move init of struct urb to after sk_buf is verified otherwise
the init is wasted if we exit early.

Signed-off-by: Alison Schofield <amsfield22@gmail.com>
---
 drivers/staging/vt6656/usbpipe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vt6656/usbpipe.c b/drivers/staging/vt6656/usbpipe.c
index bec5baf..3648d16 100644
--- a/drivers/staging/vt6656/usbpipe.c
+++ b/drivers/staging/vt6656/usbpipe.c
@@ -209,12 +209,12 @@ int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb)
 	int status = 0;
 	struct urb *urb;
 
-	urb = rcb->urb;
 	if (!rcb->skb) {
 		dev_dbg(&priv->usb->dev, "rcb->skb is null\n");
 		return status;
 	}
 
+	urb = rcb->urb;
 	usb_fill_bulk_urb(urb,
 			  priv->usb,
 			  usb_rcvbulkpipe(priv->usb, 2),
-- 
2.1.4

Re: [Outreachy kernel] [PATCH] staging: vt6656: move local var init closer to usage

[Julia Lawall]

On Thu, 11 Feb 2016, Alison Schofield wrote:

> Move init of struct urb to after sk_buf is verified otherwise
> the init is wasted if we exit early.
>
> Signed-off-by: Alison Schofield <amsfield22@gmail.com>

Acked-by: Julia Lawall <julia.lawall@lip6.fr>

> ---
>  drivers/staging/vt6656/usbpipe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vt6656/usbpipe.c b/drivers/staging/vt6656/usbpipe.c
> index bec5baf..3648d16 100644
> --- a/drivers/staging/vt6656/usbpipe.c
> +++ b/drivers/staging/vt6656/usbpipe.c
> @@ -209,12 +209,12 @@ int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb)
>  	int status = 0;
>  	struct urb *urb;
>  
> -	urb = rcb->urb;
>  	if (!rcb->skb) {
>  		dev_dbg(&priv->usb->dev, "rcb->skb is null\n");
>  		return status;
>  	}
>  
> +	urb = rcb->urb;
>  	usb_fill_bulk_urb(urb,
>  			  priv->usb,
>  			  usb_rcvbulkpipe(priv->usb, 2),
> -- 
> 2.1.4
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20160212070014.GA13288%40d830.WORKGROUP.
> For more options, visit https://groups.google.com/d/optout.
> 

Re: [Outreachy kernel] [PATCH] staging: vt6656: move local var init closer to usage

[Tejun Heo]

Hello, Alison.

On Thu, Feb 11, 2016 at 11:00:22PM -0800, Alison Schofield wrote:
> Move init of struct urb to after sk_buf is verified otherwise
> the init is wasted if we exit early.
> 
> Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> ---
>  drivers/staging/vt6656/usbpipe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vt6656/usbpipe.c b/drivers/staging/vt6656/usbpipe.c
> index bec5baf..3648d16 100644
> --- a/drivers/staging/vt6656/usbpipe.c
> +++ b/drivers/staging/vt6656/usbpipe.c
> @@ -209,12 +209,12 @@ int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb)
>  	int status = 0;
>  	struct urb *urb;
>  
> -	urb = rcb->urb;
>  	if (!rcb->skb) {
>  		dev_dbg(&priv->usb->dev, "rcb->skb is null\n");
>  		return status;
>  	}
>  
> +	urb = rcb->urb;
>  	usb_fill_bulk_urb(urb,
>  			  priv->usb,
>  			  usb_rcvbulkpipe(priv->usb, 2),

I'd actually go the other way and do

	struct urb *urb = rcb->urb;

Compilers can easily optimize these orderings.  The focus should be on
brevity and readability instead of micro optimizations which don't
really yield anything.

Thanks.

-- 
tejun

Re: [Outreachy kernel] [PATCH] staging: vt6656: move local var init closer to usage

[Alison Schofield]

On Fri, Feb 12, 2016 at 12:23:17PM -0500, Tejun Heo wrote:
> Hello, Alison.
> 
> On Thu, Feb 11, 2016 at 11:00:22PM -0800, Alison Schofield wrote:
> > Move init of struct urb to after sk_buf is verified otherwise
> > the init is wasted if we exit early.
> > 
> > Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> > ---
> >  drivers/staging/vt6656/usbpipe.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/vt6656/usbpipe.c b/drivers/staging/vt6656/usbpipe.c
> > index bec5baf..3648d16 100644
> > --- a/drivers/staging/vt6656/usbpipe.c
> > +++ b/drivers/staging/vt6656/usbpipe.c
> > @@ -209,12 +209,12 @@ int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb)
> >  	int status = 0;
> >  	struct urb *urb;
> >  
> > -	urb = rcb->urb;
> >  	if (!rcb->skb) {
> >  		dev_dbg(&priv->usb->dev, "rcb->skb is null\n");
> >  		return status;
> >  	}
> >  
> > +	urb = rcb->urb;
> >  	usb_fill_bulk_urb(urb,
> >  			  priv->usb,
> >  			  usb_rcvbulkpipe(priv->usb, 2),
> 
> I'd actually go the other way and do
> 
> 	struct urb *urb = rcb->urb;
> 
> Compilers can easily optimize these orderings.  The focus should be on
> brevity and readability instead of micro optimizations which don't
> really yield anything.
> 
> Thanks.
> 
> -- 
> tejun
This was suggested to me, but I chose to mimic what another
funtion was doing in same file.  I'll change both instances
for brevity & readabiity! 
Thanks,
alison

[PATCH v2] staging: vt6656: move local var init into declaration

[Alison Schofield]

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>
---
Changes in v2:
- moved local variable initialization to declaration,
  rather than after first exit.
- cleaned up 2 additional instances in same file
- modified commit message and changelog to match
 
 drivers/staging/vt6656/usbpipe.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

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) {
 		priv->int_buf.in_use = false;
 
@@ -207,9 +205,8 @@ static void vnt_submit_rx_urb_complete(struct urb *urb)
 int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb)
 {
 	int status = 0;
-	struct urb *urb;
+	struct urb *urb = rcb->urb;
 
-	urb = rcb->urb;
 	if (!rcb->skb) {
 		dev_dbg(&priv->usb->dev, "rcb->skb is null\n");
 		return status;
@@ -269,15 +266,13 @@ int vnt_tx_context(struct vnt_private *priv,
 		   struct vnt_usb_send_context *context)
 {
 	int status;
-	struct urb *urb;
+	struct urb *urb = context->urb;
 
 	if (test_bit(DEVICE_FLAGS_DISCONNECTED, &priv->flags)) {
 		context->in_use = false;
 		return STATUS_RESOURCES;
 	}
 
-	urb = context->urb;
-
 	usb_fill_bulk_urb(urb,
 			  priv->usb,
 			  usb_sndbulkpipe(priv->usb, 3),
-- 
2.1.4

Re: [Outreachy kernel] [PATCH v2] staging: vt6656: move local var init into declaration

[Julia Lawall]

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>
> ---
> Changes in v2:
> - moved local variable initialization to declaration,
>   rather than after first exit.
> - cleaned up 2 additional instances in same file
> - modified commit message and changelog to match
>
>  drivers/staging/vt6656/usbpipe.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
>
> 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

>  		priv->int_buf.in_use = false;
>
> @@ -207,9 +205,8 @@ static void vnt_submit_rx_urb_complete(struct urb *urb)
>  int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb)
>  {
>  	int status = 0;
> -	struct urb *urb;
> +	struct urb *urb = rcb->urb;
>
> -	urb = rcb->urb;
>  	if (!rcb->skb) {
>  		dev_dbg(&priv->usb->dev, "rcb->skb is null\n");
>  		return status;
> @@ -269,15 +266,13 @@ int vnt_tx_context(struct vnt_private *priv,
>  		   struct vnt_usb_send_context *context)
>  {
>  	int status;
> -	struct urb *urb;
> +	struct urb *urb = context->urb;
>
>  	if (test_bit(DEVICE_FLAGS_DISCONNECTED, &priv->flags)) {
>  		context->in_use = false;
>  		return STATUS_RESOURCES;
>  	}
>
> -	urb = context->urb;
> -
>  	usb_fill_bulk_urb(urb,
>  			  priv->usb,
>  			  usb_sndbulkpipe(priv->usb, 3),
> --
> 2.1.4
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1455302683-5119-1-git-send-email-amsfield22%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH v2] staging: vt6656: move local var init into declaration

[Alison Schofield]

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

Re: [Outreachy kernel] [PATCH v2] staging: vt6656: move local var init into declaration

[Julia Lawall]

On Tue, 16 Feb 2016, Alison Schofield wrote:

> 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?

If Greg already took the first patch, it could be better to start a new
thread.

julia