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
