Received-SPF: pass (google.com: domain of amsfield22@gmail.com designates 2607:f8b0:400e:c00::232 as permitted sender) client-ip=2607:f8b0:400e:c00::232;
Date: Thu, 11 Feb 2016 11:46:47 -0800
From: Alison Schofield <amsfield22@gmail.com>
To: Julia Lawall <julia.lawall@lip6.fr>
Cc: outreachy-kernel@googlegroups.com
Subject: Re: [Outreachy kernel] [PATCH] staging: vt6656: replace explicit
 NULL comparison with ! operator
Message-ID: <20160211194646.GA3257@d830.WORKGROUP>
References: <20160211070636.GA16318@d830.WORKGROUP>
 <alpine.DEB.2.10.1602110912590.2508@hadrien>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.DEB.2.10.1602110912590.2508@hadrien>
User-Agent: Mutt/1.5.23 (2014-03-12)

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