Google
  Web www.spinics.net

Re: [PATCH] USB: add support for SuperH USBF

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


David Brownell wrote:
> 
> I won't sign off on this until some of the obvious locking bugs
> get fixed.  Example:
> 
>> +static void transfer_complete(struct sh_udc_ep *ep,
>> +			      struct sh_udc_request *req,
>> +			      int status)
>> +{
>> +	int restart = 0;
>> +
>> +	list_del_init(&req->queue);
>> +	if (ep->sh_udc->gadget.speed == USB_SPEED_UNKNOWN)
> 
> And status is zero ... don't clobber other fault codes
> with ESHUTDOWN.

I will modify it to check it before calling this function.

>> +		req->req.status = -ESHUTDOWN;
>> +	else
>> +		req->req.status = status;
>> +
>> +	if (!list_empty(&ep->queue))
>> +		restart = 1;
>> +
>> +	if (likely(req->req.complete))
>> +		req->req.complete(&ep->ep, &req->req);
> 
> First, it's a serious bug if there's no complete(); don't bother
> even checking that.

This check was unnecessary...

> Second, that's a locking bug ... since this is at least sometimes
> called with the spinlock held, it should be dropped before complete()
> is called then reclaimed immediately afterwards.  (Reason:  it's routine
> for completion callbacks to submit new requests, which grab the lock.)
> 
> Third, several callers didn't even hold that spinlock ... which is a
> different class of problems.  Notice the list_del_init() modifies
> data structures that lock must protect.  The IRQ handler doesn't
> grab the spinlock; it should do so, and drop it around outcalls to
> the setup() callback.
> 
> I get the impression this hasn't undergone much testing with
> lockdep enabled... if SH doesn't yet support that, it should
> have a manual lock audit as part of its code review.  (I did
> only part of one here.  Basic rule:  know what data the lock
> protects, and make sure all access to that data is done while
> holding that lock.)

I tried to enable lockdep. The development environment for USBF
could boot Linux :-)  And it detected a problem of spinlock.
I will fix this problem.

>> +
>> +	if (restart) {
>> +		req = list_entry(ep->queue.next, struct sh_udc_request, queue);
>> +		if (ep->desc)
>> +			start_packet(ep, req);
>> +	}
>> +}
> 
> 
> Also:
> 
>> +                       INIT_LIST_HEAD(&sh_udc->ep[i].ep.ep_list);
>> +                       list_add_tail(&sh_udc->ep[i].ep.ep_list,
>> +                                     &sh_udc->gadget.ep_list);
> 
> You don't need the INIT_LIST_HEAD, since list_add_tail() clobbers
> those fields right away.

I understood it.

> Other random points:
> 
>  - The start_ep_to_ep_addr() function would be more efficiently
>    done by having a byte in the EP structure; there's plenty of
>    padding available next to the "busy" bit.

I understood it. I will try to modify this function.

>  - Most of the inline functions in the header file should be in
>    the C file instead.  And for the IRQ logic, that's a bit much
>    to force as an inline ... just declare it static then let the
>    compiler decide if inlining is worthwhile.
> 
>  - In several places you declare arrays on the stack, which are
>    used as constant lookup tables.  Don't put those on the stack,
>    as it takes space and time to initialize them.  Just declare
>    them as "static const".

I understood it.

>  - The "udc_enable" and "udc_disable" methods are misnamed; they
>    enable and disable endpoints, not the whole UDC.  It's unwise
>    to do no error checking at all ... current gadget drivers don't
>    have bugs there, new (or custom) ones easily could.  (Examples:
>    trying to set up ep0 as a bulk endpoint; getting direction or
>    size wrong on one of the fixed configuration endpoints; enabling
>    an endpoint that's already enabled; disabling one that's not
>    been enabled; no locking around ep->desc modification.)
> 
>  - Actually *ALL* endpoint methods have that misnaming problem.

I will change the function name into "sh_udc_ep_enable".
And I will add error checking in enable method.

>  - If this driver isn't ready to support isochronous transfers,
>    then don't expose those ISO-only endpoints ...

I will remove ISO-only endpoints.

>  - Don't bother with a NOP stub for fetching fifo status, it's
>    not needed.

I will delete this method.

> That's just a brief review.  I figure that once the locking bugs
> are no longer so obvious, a more careful review would make sense.

I will try to debug the spinlock bug.

Thanks,
Yoshihiro Shimoda

> - Dave
> 


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-devel@xxxxxxxxxxxxxxxxxxxxx
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

[Home]     [Video for Linux]     [Photo]     [Yosemite Forum]     [Yosemite Photos]    [Video Projectors]     [PDAs]     [Hacking TiVo]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Devices]     [Big List of Linux Books]     [Free Dating]

  Powered by Linux