[PATCH] staging: vt6656: replace explicit NULL comparison with ! operator

[PATCH] staging: vt6656: replace explicit NULL comparison with ! operator

[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 <amsfield22@gmail.com>
---
 drivers/staging/vt6656/main_usb.c | 10 +++++-----
 drivers/staging/vt6656/usbpipe.c  |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
index a2f23ae..05f86ff 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -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;
 	}
diff --git a/drivers/staging/vt6656/usbpipe.c b/drivers/staging/vt6656/usbpipe.c
index 351a99f..bec5baf 100644
--- a/drivers/staging/vt6656/usbpipe.c
+++ b/drivers/staging/vt6656/usbpipe.c
@@ -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

Re: [Outreachy kernel] [PATCH] staging: vt6656: replace explicit NULL comparison with ! operator

[Julia Lawall]

On Wed, 10 Feb 2016, Alison Schofield wrote:

> 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 <amsfield22@gmail.com>
> ---
>  drivers/staging/vt6656/main_usb.c | 10 +++++-----
>  drivers/staging/vt6656/usbpipe.c  |  2 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> index a2f23ae..05f86ff 100644
> --- a/drivers/staging/vt6656/main_usb.c
> +++ b/drivers/staging/vt6656/main_usb.c
> @@ -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);

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.

> -	if (priv->int_buf.data_buf == NULL) {
> +	if (!priv->int_buf.data_buf) {
>  		usb_free_urb(priv->interrupt_urb);
>  		goto free_rx_tx;
>  	}
> diff --git a/drivers/staging/vt6656/usbpipe.c b/drivers/staging/vt6656/usbpipe.c
> index 351a99f..bec5baf 100644
> --- a/drivers/staging/vt6656/usbpipe.c
> +++ b/drivers/staging/vt6656/usbpipe.c
> @@ -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

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

Re: [Outreachy kernel] [PATCH] staging: vt6656: replace explicit NULL comparison with ! operator

[Julia Lawall]

On Wed, 10 Feb 2016, Alison Schofield wrote:

> 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 <amsfield22@gmail.com>

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

> ---
>  drivers/staging/vt6656/main_usb.c | 10 +++++-----
>  drivers/staging/vt6656/usbpipe.c  |  2 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> index a2f23ae..05f86ff 100644
> --- a/drivers/staging/vt6656/main_usb.c
> +++ b/drivers/staging/vt6656/main_usb.c
> @@ -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;
>  	}
> diff --git a/drivers/staging/vt6656/usbpipe.c b/drivers/staging/vt6656/usbpipe.c
> index 351a99f..bec5baf 100644
> --- a/drivers/staging/vt6656/usbpipe.c
> +++ b/drivers/staging/vt6656/usbpipe.c
> @@ -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 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/20160211070636.GA16318%40d830.WORKGROUP.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH] staging: vt6656: replace explicit NULL comparison with ! operator

[Alison Schofield]

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

------ snip ------------
> 
> 
> > @@ -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);
> 
> 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.

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

> > diff --git a/drivers/staging/vt6656/usbpipe.c b/drivers/staging/vt6656/usbpipe.c
> > index 351a99f..bec5baf 100644
> > --- a/drivers/staging/vt6656/usbpipe.c
> > +++ b/drivers/staging/vt6656/usbpipe.c
> > @@ -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

 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


 

Re: [Outreachy kernel] [PATCH] staging: vt6656: replace explicit NULL comparison with ! operator

[Julia Lawall]

On Thu, 11 Feb 2016, Alison Schofield wrote:

> On Thu, Feb 11, 2016 at 09:16:25AM +0100, Julia Lawall wrote:
> 
> ------ snip ------------
> > 
> > 
> > > @@ -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);
> > 
> > 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.
> 
> 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.

OK

> more below...
> 
> ............snip.............
> 
> > > diff --git a/drivers/staging/vt6656/usbpipe.c b/drivers/staging/vt6656/usbpipe.c
> > > index 351a99f..bec5baf 100644
> > > --- a/drivers/staging/vt6656/usbpipe.c
> > > +++ b/drivers/staging/vt6656/usbpipe.c
> > > @@ -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
> 
>  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?	

That's fine.

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