staging: vt6656: replace explicit NULL comparison with ! operator

PATCH In review Alison Schofield 2 participants 5 messages Feb 11, 2016 Staging

lore

Share & tools

lore

Alison Schofield

author

2016-02-11 07:06 UTC #0 lore raw

PATCH 1/1 staging: vt6656: replace explicit NULL comparison with ! operator

Replace explicit NULL comparison with ! operator to simplify code.

Found with Coccinelle:
@@
expression e;
statement s0, s1;
@@

if (
(
+ !
e
- == NULL
|| ...
)
) s0 else s1

Signed-off-by Alison Schofield <[email protected]>

---
 drivers/staging/vt6656/main_usb.c | 10 +++++-----
 drivers/staging/vt6656/usbpipe.c  |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

drivers/staging/vt6656/main_usb.c +5 -5

index a2f23ae..05f86ff 100644
@@ -431,7 +431,7 @@ static bool vnt_alloc_bufs(struct vnt_private *priv)
 	for (ii = 0; ii < priv->num_tx_context; ii++) {
 		tx_context = kmalloc(sizeof(struct vnt_usb_send_context),
 								GFP_KERNEL);
-		if (tx_context == NULL)
+		if (!tx_context)
 			goto free_tx;
 
 		priv->tx_context[ii] = tx_context;

@@ -462,13 +462,13 @@ static bool vnt_alloc_bufs(struct vnt_private *priv)
 
 		/* allocate URBs */
 		rcb->urb = usb_alloc_urb(0, GFP_ATOMIC);
-		if (rcb->urb == NULL) {
+		if (!rcb->urb) {
 			dev_err(&priv->usb->dev, "Failed to alloc rx urb\n");
 			goto free_rx_tx;
 		}
 
 		rcb->skb = dev_alloc_skb(priv->rx_buf_sz);
-		if (rcb->skb == NULL)
+		if (!rcb->skb)
 			goto free_rx_tx;
 
 		rcb->in_use = false;

@@ -479,13 +479,13 @@ static bool vnt_alloc_bufs(struct vnt_private *priv)
 	}
 
 	priv->interrupt_urb = usb_alloc_urb(0, GFP_ATOMIC);
-	if (priv->interrupt_urb == NULL) {
+	if (!priv->interrupt_urb) {
 		dev_err(&priv->usb->dev, "Failed to alloc int urb\n");
 		goto free_rx_tx;
 	}
 
 	priv->int_buf.data_buf = kmalloc(MAX_INTERRUPT_SIZE, GFP_KERNEL);
-	if (priv->int_buf.data_buf == NULL) {
+	if (!priv->int_buf.data_buf) {
 		usb_free_urb(priv->interrupt_urb);
 		goto free_rx_tx;
 	}

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

index 351a99f..bec5baf 100644
@@ -210,7 +210,7 @@ int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb)
 	struct urb *urb;
 
 	urb = rcb->urb;
-	if (rcb->skb == NULL) {
+	if (!rcb->skb) {
 		dev_dbg(&priv->usb->dev, "rcb->skb is null\n");
 		return status;
 	}

Signature · 1 line

2.1.4

Julia Lawall

2016-02-11 08:16 UTC #1 lore raw

Quoted 53 lines · Alison Schofield

Replace explicit NULL comparison with ! operator to simplify code.

Found with Coccinelle:
@@
expression e;
statement s0, s1;
@@

if (
(
+ !
e
- == NULL
|| ...
)
) s0 else s1

Signed-off-by Alison Schofield <[email protected]>

---
 drivers/staging/vt6656/main_usb.c | 10 +++++-----
 drivers/staging/vt6656/usbpipe.c  |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

drivers/staging/vt6656/main_usb.c +3 -3

index a2f23ae..05f86ff 100644
@@ -431,7 +431,7 @@ static bool vnt_alloc_bufs(struct vnt_private *priv)
 	for (ii = 0; ii < priv->num_tx_context; ii++) {
 		tx_context = kmalloc(sizeof(struct vnt_usb_send_context),
 								GFP_KERNEL);
-		if (tx_context == NULL)
+		if (!tx_context)
 			goto free_tx;
 
 		priv->tx_context[ii] = tx_context;

@@ -462,13 +462,13 @@ static bool vnt_alloc_bufs(struct vnt_private *priv)
 
 		/* allocate URBs */
 		rcb->urb = usb_alloc_urb(0, GFP_ATOMIC);
-		if (rcb->urb == NULL) {
+		if (!rcb->urb) {
 			dev_err(&priv->usb->dev, "Failed to alloc rx urb\n");
 			goto free_rx_tx;
 		}
 
 		rcb->skb = dev_alloc_skb(priv->rx_buf_sz);
-		if (rcb->skb == NULL)
+		if (!rcb->skb)
 			goto free_rx_tx;
 
 		rcb->in_use = false;

drivers/staging/vt6656/main_usb.c static bool vnt_alloc_bufs(struct vnt_private *priv) · line 487

Quoted 10 lines

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

@@ -479,13 +479,13 @@ static bool vnt_alloc_bufs(struct vnt_private *priv)
 	}
 
 	priv->interrupt_urb = usb_alloc_urb(0, GFP_ATOMIC);
-	if (priv->interrupt_urb == NULL) {
+	if (!priv->interrupt_urb) {
 		dev_err(&priv->usb->dev, "Failed to alloc int urb\n");
 		goto free_rx_tx;
 	}
 
 	priv->int_buf.data_buf = kmalloc(MAX_INTERRUPT_SIZE, GFP_KERNEL);

Nothing to do with your change, but there seems to be an inconsistncy here. Why is one allocation done with GFP_KERNEL and the other done with GFP_ATOMIC?

GFP_ATOMIC is needed when the allocation is done with interrupts turned off, because otherwise kmalloc may block, in case of insufficient memory, and that could lead to a deadlock.

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

Quoted 13 lines

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

-	if (priv->int_buf.data_buf == NULL) {
+	if (!priv->int_buf.data_buf) {
 		usb_free_urb(priv->interrupt_urb);
 		goto free_rx_tx;
 	}

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

index 351a99f..bec5baf 100644
@@ -210,7 +210,7 @@ int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb)
 	struct urb *urb;
 
 	urb = rcb->urb;

Another issue is here. It's a bit strange to initialize a local variable, and then decide that its value is invalid and then leave the function. The initialization could be moved up to the declaration (same degree of potential uselessness, but at least one less line), or moved after the test.

julia

Quoted 15 lines

drivers/staging/vt6656/usbpipe.c +1 -2
-	if (rcb->skb == NULL) {
+	if (!rcb->skb) {
 		dev_dbg(&priv->usb->dev, "rcb->skb is null\n");
 		return status;
 	}
--
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/20160211070636.GA16318%40d830.WORKGROUP.
For more options, visit https://groups.google.com/d/optout.

Alison Schofield

author

2016-02-11 19:46 UTC #2 lore raw

On Thu, Feb 11, 2016 at 09:16:25AM +0100, Julia Lawall wrote:

------ snip ------------

Quoted 34 lines

Quoted 19 lines

drivers/staging/vt6656/main_usb.c +2 -2

@@ -462,13 +462,13 @@ static bool vnt_alloc_bufs(struct vnt_private *priv)
 
 		/* allocate URBs */
 		rcb->urb = usb_alloc_urb(0, GFP_ATOMIC);
-		if (rcb->urb == NULL) {
+		if (!rcb->urb) {
 			dev_err(&priv->usb->dev, "Failed to alloc rx urb\n");
 			goto free_rx_tx;
 		}
 
 		rcb->skb = dev_alloc_skb(priv->rx_buf_sz);
-		if (rcb->skb == NULL)
+		if (!rcb->skb)
 			goto free_rx_tx;
 
 		rcb->in_use = false;

drivers/staging/vt6656/main_usb.c static bool vnt_alloc_bufs(struct vnt_private *priv) · line 487

Quoted 10 lines

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

@@ -479,13 +479,13 @@ static bool vnt_alloc_bufs(struct vnt_private *priv)
 	}
 
 	priv->interrupt_urb = usb_alloc_urb(0, GFP_ATOMIC);
-	if (priv->interrupt_urb == NULL) {
+	if (!priv->interrupt_urb) {
 		dev_err(&priv->usb->dev, "Failed to alloc int urb\n");
 		goto free_rx_tx;
 	}
 
 	priv->int_buf.data_buf = kmalloc(MAX_INTERRUPT_SIZE, GFP_KERNEL);

Nothing to do with your change, but there seems to be an inconsistncy here. Why is one allocation done with GFP_KERNEL and the other done with GFP_ATOMIC?

Quoted 3 lines

GFP_ATOMIC is needed when the allocation is done with interrupts turned off, because otherwise kmalloc may block, in case of insufficient memory, and that could lead to a deadlock.

I see the inconsistent usage of GFP_KERNEL/ATOMIC.

vnt_alloc_bufs() can and should be consistent in usage of mem flags. ie. the rules for usb_alloc_urb() are same as for kmalloc(). In this case we are running in process context, are not in intr handler, nor holding any spinlock. I will submit a new patch for this.

Within vt6656 I also found that usbpipes.c also uses GFP_ATOMIC while managing usb urbs, but that is within interrupt context and while holding spinlock. Not touching.

more below...

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

Quoted 15 lines

drivers/staging/vt6656/usbpipe.c int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb) · line 212
Quoted 8 lines
drivers/staging/vt6656/usbpipe.c +0 -0
index 351a99f..bec5baf 100644
@@ -210,7 +210,7 @@ int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb)
 	struct urb *urb;
 
 	urb = rcb->urb;
Another issue is here. It's a bit strange to initialize a local variable, and then decide that its value is invalid and then leave the function. The initialization could be moved up to the declaration (same degree of potential uselessness, but at least one less line), or moved after the test.

Quoted 1 line

julia

I can mv the init of urb to after the test of rcb->skb because no need to init it if we're exiting before using it.

So:

	int status = 0;
	struct urb *urb;

	urb = rcb->urb;   <<< MOVE THIS
	if (!rcb->skb) {
		dev_dbg(&priv->usb->dev, "rcb->skb is null\n");
		return status;
	}
	 
	urb = rcb->urb;   <<< TO HERE

Yes?

(I may hold onto this one a bit to include w other cleanup in same driver.)

alison

Julia Lawall

2016-02-11 19:56 UTC #3 lore raw

Quoted 43 lines · Alison Schofield

On Thu, Feb 11, 2016 at 09:16:25AM +0100, Julia Lawall wrote:
------ snip ------------
Quoted 34 lines
Quoted 19 lines
drivers/staging/vt6656/main_usb.c +2 -2
@@ -462,13 +462,13 @@ static bool vnt_alloc_bufs(struct vnt_private *priv)
 
 		/* allocate URBs */
 		rcb->urb = usb_alloc_urb(0, GFP_ATOMIC);
-		if (rcb->urb == NULL) {
+		if (!rcb->urb) {
 			dev_err(&priv->usb->dev, "Failed to alloc rx urb\n");
 			goto free_rx_tx;
 		}
 
 		rcb->skb = dev_alloc_skb(priv->rx_buf_sz);
-		if (rcb->skb == NULL)
+		if (!rcb->skb)
 			goto free_rx_tx;
 
 		rcb->in_use = false;
drivers/staging/vt6656/main_usb.c static bool vnt_alloc_bufs(struct vnt_private *priv) · line 487
Quoted 10 lines
drivers/staging/vt6656/main_usb.c +1 -1
@@ -479,13 +479,13 @@ static bool vnt_alloc_bufs(struct vnt_private *priv)
 	}
 
 	priv->interrupt_urb = usb_alloc_urb(0, GFP_ATOMIC);
-	if (priv->interrupt_urb == NULL) {
+	if (!priv->interrupt_urb) {
 		dev_err(&priv->usb->dev, "Failed to alloc int urb\n");
 		goto free_rx_tx;
 	}
 
 	priv->int_buf.data_buf = kmalloc(MAX_INTERRUPT_SIZE, GFP_KERNEL);
Nothing to do with your change, but there seems to be an inconsistncy here. Why is one allocation done with GFP_KERNEL and the other done with GFP_ATOMIC?
Quoted 3 lines
GFP_ATOMIC is needed when the allocation is done with interrupts turned off, because otherwise kmalloc may block, in case of insufficient memory, and that could lead to a deadlock.
I see the inconsistent usage of GFP_KERNEL/ATOMIC.

Quoted 8 lines

vnt_alloc_bufs() can and should be consistent in usage of mem flags. ie. the rules for usb_alloc_urb() are same as for kmalloc(). In this case we are running in process context, are not in intr handler, nor holding any spinlock. I will submit a new patch for this.
Within vt6656 I also found that usbpipes.c also uses GFP_ATOMIC while managing usb urbs, but that is within interrupt context and while holding spinlock. Not touching.

Quoted 36 lines

more below...
............snip.............
Quoted 15 lines
drivers/staging/vt6656/usbpipe.c int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb) · line 212
Quoted 8 lines
drivers/staging/vt6656/usbpipe.c +0 -0
index 351a99f..bec5baf 100644
@@ -210,7 +210,7 @@ int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb)
 	struct urb *urb;
 
 	urb = rcb->urb;
Another issue is here. It's a bit strange to initialize a local variable, and then decide that its value is invalid and then leave the function. The initialization could be moved up to the declaration (same degree of potential uselessness, but at least one less line), or moved after the test.
Quoted 1 line
julia
I can mv the init of urb to after the test of rcb->skb because no need to init it if we're exiting before using it.
So:
	int status = 0;
	struct urb *urb;

	urb = rcb->urb;   <<< MOVE THIS
	if (!rcb->skb) {
		dev_dbg(&priv->usb->dev, "rcb->skb is null\n");
		return status;
	}
	 
	urb = rcb->urb;   <<< TO HERE

Quoted 1 line

Yes?

That's fine.

Quoted 2 lines

(I may hold onto this one a bit to include w other cleanup in same driver.)

Sure. It's a minor issue in this case.

julia

Julia Lawall

Acked-by on v1 1/1

2016-02-11 08:17 UTC #4 lore raw

In reply to Alison Schofield

Quoted 17 lines · Alison Schofield

Replace explicit NULL comparison with ! operator to simplify code.
Found with Coccinelle:
@@
expression e;
statement s0, s1;
@@
if (
(
+ !
e
- == NULL
|| ...
)
) s0 else s1

Quoted 1 line

Signed-off-by Alison Schofield <[email protected]>

Acked-by Julia Lawall <[email protected]>

Quoted 73 lines

---
 drivers/staging/vt6656/main_usb.c | 10 +++++-----
 drivers/staging/vt6656/usbpipe.c  |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

drivers/staging/vt6656/main_usb.c +5 -5

index a2f23ae..05f86ff 100644
@@ -431,7 +431,7 @@ static bool vnt_alloc_bufs(struct vnt_private *priv)
 	for (ii = 0; ii < priv->num_tx_context; ii++) {
 		tx_context = kmalloc(sizeof(struct vnt_usb_send_context),
 								GFP_KERNEL);
-		if (tx_context == NULL)
+		if (!tx_context)
 			goto free_tx;
 
 		priv->tx_context[ii] = tx_context;

@@ -462,13 +462,13 @@ static bool vnt_alloc_bufs(struct vnt_private *priv)
 
 		/* allocate URBs */
 		rcb->urb = usb_alloc_urb(0, GFP_ATOMIC);
-		if (rcb->urb == NULL) {
+		if (!rcb->urb) {
 			dev_err(&priv->usb->dev, "Failed to alloc rx urb\n");
 			goto free_rx_tx;
 		}
 
 		rcb->skb = dev_alloc_skb(priv->rx_buf_sz);
-		if (rcb->skb == NULL)
+		if (!rcb->skb)
 			goto free_rx_tx;
 
 		rcb->in_use = false;

@@ -479,13 +479,13 @@ static bool vnt_alloc_bufs(struct vnt_private *priv)
 	}
 
 	priv->interrupt_urb = usb_alloc_urb(0, GFP_ATOMIC);
-	if (priv->interrupt_urb == NULL) {
+	if (!priv->interrupt_urb) {
 		dev_err(&priv->usb->dev, "Failed to alloc int urb\n");
 		goto free_rx_tx;
 	}
 
 	priv->int_buf.data_buf = kmalloc(MAX_INTERRUPT_SIZE, GFP_KERNEL);
-	if (priv->int_buf.data_buf == NULL) {
+	if (!priv->int_buf.data_buf) {
 		usb_free_urb(priv->interrupt_urb);
 		goto free_rx_tx;
 	}

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

index 351a99f..bec5baf 100644
@@ -210,7 +210,7 @@ int vnt_submit_rx_urb(struct vnt_private *priv, struct vnt_rcb *rcb)
 	struct urb *urb;
 
 	urb = rcb->urb;
-	if (rcb->skb == NULL) {
+	if (!rcb->skb) {
 		dev_dbg(&priv->usb->dev, "rcb->skb is null\n");
 		return status;
 	}
--

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/20160211070636.GA16318%40d830.WORKGROUP.
For more options, visit https://groups.google.com/d/optout.