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@???
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
(text/plain)