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

[Felipe Balbi]

Hi,

Nathan Chancellor <natechancellor@xxxxxxxxx> writes:
> 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 :/

hmm, I don't have that patch in my inbox. Could you resend it?

-- 
balbi

Attachment: signature.asc
Description: PGP signature