Re: [PATCH] usb: gadget: udc: bdc: Remove unnecessary NULL checks in bdc_req_complete

[Nathan Chancellor]

On Mon, Feb 24, 2020 at 01:42:57PM -0800, Nick Desaulniers wrote:
> On Thu, Feb 20, 2020 at 8:57 PM Nathan Chancellor
> <natechancellor@xxxxxxxxx> wrote:
> >
> > I know it has been a while but ping?
> 
> Sorry! Too many bugs...barely treading water! Send help!
> 
> >
> > On Tue, Oct 22, 2019 at 05:20:15PM -0700, Nathan Chancellor wrote:
> > > When building with Clang + -Wtautological-pointer-compare:
> > >
> > > drivers/usb/gadget/udc/bdc/bdc_ep.c:543:28: warning: comparison of
> > > address of 'req->queue' equal to a null pointer is always false
> > > [-Wtautological-pointer-compare]
> > >         if (req == NULL  || &req->queue == NULL || &req->usb_req == NULL)
> > >                              ~~~~~^~~~~    ~~~~
> > > drivers/usb/gadget/udc/bdc/bdc_ep.c:543:51: warning: comparison of
> > > address of 'req->usb_req' equal to a null pointer is always false
> > > [-Wtautological-pointer-compare]
> > >         if (req == NULL  || &req->queue == NULL || &req->usb_req == NULL)
> > >                                                     ~~~~~^~~~~~~    ~~~~
> > > 2 warnings generated.
> > >
> > > As it notes, these statements will always evaluate to false so remove
> > > them.
> 
> `req` is an instance of a `struct bdc_req` defined in
> drivers/usb/gadget/udc/bdc/bdc.h as:
> 333 struct bdc_req {
> 334   struct usb_request  usb_req;
> 335   struct list_head  queue;
> 336   struct bdc_ep   *ep;
> 337   /* only one Transfer per request */
> 338   struct bd_transfer bd_xfr;
> 339   int epnum;
> 340 };
> 
> So indeed the non-pointer, struct members can never be NULL.
> 
> I think the second check that was removed should be
> `req->usb_req.complete == NULL`, since otherwise `&req->usb_req` may
> be passed to usb_gadget_giveback_request which tries to invoke the
> `complete` member as a callback.  There are numerous places in
> drivers/usb/gadget/udc/bdc/bdc_ep.c that assign `complete = NULL`.
> 
> Can the maintainers clarify?

$ sed -n 537,555p drivers/usb/gadget/udc/bdc/bdc_ep.c
/* callback to gadget layer when xfr completes */
static void bdc_req_complete(struct bdc_ep *ep, struct bdc_req *req,
						int status)
{
	struct bdc *bdc = ep->bdc;

	if (req == NULL  || &req->queue == NULL || &req->usb_req == NULL)
		return;

	dev_dbg(bdc->dev, "%s ep:%s status:%d\n", __func__, ep->name, status);
	list_del(&req->queue);
	req->usb_req.status = status;
	usb_gadget_unmap_request(&bdc->gadget, &req->usb_req, ep->dir);
	if (req->usb_req.complete) {
		spin_unlock(&bdc->lock);
		usb_gadget_giveback_request(&ep->usb_ep, &req->usb_req);
		spin_lock(&bdc->lock);
	}
}

It looks like req->usb_req.complete is checked before being passed to
usb_gadget_giveback_request. So the patch as it stands is correct,
unless those checks needed to be something else.

Felipe, could you clarify or pick up this patch if it is correct? This
is one of two warnings that I see for -Wtautological-compare and I want
it turned on for 5.7 and it'd be nice to be warning free, especially
since I sent this patch back in October :/

Cheers,
Nathan