staging: vt6656: move local var init closer to usage

PATCH In review Alison Schofield 3 participants 8 messages Feb 12, 2016 – Feb 17, 2016 Staging

lore

Share & tools

lore

Alison Schofield

author

2016-02-12 07:00 UTC #0 lore raw

PATCH 1/1 staging: vt6656: move local var init closer to usage

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 <[email protected]>

---
 drivers/staging/vt6656/usbpipe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/vt6656/usbpipe.c +1 -1

index bec5baf..3648d16 100644
@@ -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),

Signature · 1 line

2.1.4

Julia Lawall

Acked-by on v1 1/1

2016-02-12 07:14 UTC #1 lore raw

Quoted 4 lines · 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 <[email protected]>

Acked-by Julia Lawall <[email protected]>

Quoted 32 lines

---
 drivers/staging/vt6656/usbpipe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/vt6656/usbpipe.c +1 -1

index bec5baf..3648d16 100644
@@ -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),

Signature · 8 lines

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 [email protected].
To post to this group, send email to [email protected].
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.

Tejun Heo

2016-02-12 17:23 UTC #2 lore raw

In reply to Alison Schofield

Hello, Alison.

Quoted 16 lines · 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 <[email protected]>
---
 drivers/staging/vt6656/usbpipe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/vt6656/usbpipe.c +0 -0
index bec5baf..3648d16 100644
@@ -209,12 +209,12 @@ int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb)
 	int status = 0;
 	struct urb *urb;
 

drivers/staging/vt6656/usbpipe.c int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb) · line 220

Quoted 10 lines

drivers/staging/vt6656/usbpipe.c +1 -1

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

Signature · 1 line

tejun

Alison Schofield

author

2016-02-12 18:16 UTC #3 lore raw

Quoted 32 lines · Tejun Heo

Hello, Alison.
Quoted 16 lines · 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 <[email protected]>
---
 drivers/staging/vt6656/usbpipe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/vt6656/usbpipe.c +0 -0
index bec5baf..3648d16 100644
@@ -209,12 +209,12 @@ int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb)
 	int status = 0;
 	struct urb *urb;
 
drivers/staging/vt6656/usbpipe.c int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb) · line 220
Quoted 10 lines
drivers/staging/vt6656/usbpipe.c +1 -1
-	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

Quoted 10 lines

	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.
Signature · 1 line
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

Alison Schofield

author

2016-02-12 18:44 UTC #4 lore raw

PATCH v2 1/1 staging: vt6656: move local var init into declaration

In reply to 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 <[email protected]>

---
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(-)

drivers/staging/vt6656/usbpipe.c +4 -9

index bec5baf..f27eb9a 100644
@@ -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),

Signature · 1 line

2.1.4

Julia Lawall

2016-02-12 18:58 UTC #5 lore raw

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

Quoted 33 lines · 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 <[email protected]>
---
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(-)
drivers/staging/vt6656/usbpipe.c +2 -2
index bec5baf..f27eb9a 100644
@@ -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;

drivers/staging/vt6656/usbpipe.c static void vnt_start_interrupt_urb_complete(struct urb *urb) · line 119

Quoted 6 lines

drivers/staging/vt6656/usbpipe.c +0 -2

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

Quoted 40 lines

drivers/staging/vt6656/usbpipe.c +2 -6
 		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 [email protected]. To post to this group, send email to [email protected].
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.

Alison Schofield

author

2016-02-16 20:59 UTC #6 lore raw

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

Quoted 11 lines · Julia Lawall

Quoted 7 lines · 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 <[email protected]>
---

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

Quoted 35 lines

Quoted 18 lines
drivers/staging/vt6656/usbpipe.c +2 -2
index bec5baf..f27eb9a 100644
@@ -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;
drivers/staging/vt6656/usbpipe.c static void vnt_start_interrupt_urb_complete(struct urb *urb) · line 119
Quoted 6 lines
drivers/staging/vt6656/usbpipe.c +0 -2
@@ -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,
};

Quoted 7 lines

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

Julia Lawall

2016-02-17 00:58 UTC #7 lore raw

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

Quoted 61 lines · Alison Schofield

Quoted 11 lines · Julia Lawall
Quoted 7 lines · 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 <[email protected]>
---
.............snip
Quoted 35 lines
Quoted 18 lines
drivers/staging/vt6656/usbpipe.c +2 -2
index bec5baf..f27eb9a 100644
@@ -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;
drivers/staging/vt6656/usbpipe.c static void vnt_start_interrupt_urb_complete(struct urb *urb) · line 119
Quoted 6 lines
drivers/staging/vt6656/usbpipe.c +0 -2
@@ -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,
};
Quoted 7 lines
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.

Quoted 8 lines

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