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

[David Brownell <david-b@xxxxxxxxxxx>]

On Thursday 10 January 2008, Yoshihiro Shimoda wrote:
> add support for SuperH USBF(USB peripheral controller).
> 
> supported CPUs are:
>  - SH7720
>  - SH7721
>  - SH7763
> 
> Signed-off-by: Yoshihiro Shimoda <shimoda.yoshihiro@xxxxxxxxxxx>

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.


> +		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.

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.)


> +
> +	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.


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.

 - 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".

 - 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.

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

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

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.

- 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