Messages in this thread |  | | Date | Tue, 5 Nov 2013 10:38:29 -0500 (EST) | From | Alan Stern <> | Subject | Re: [PATCH v4 3/4] usb: ffs: check quirk to pad epout buf size when not aligned to maxpacketsize |
| |
On Tue, 5 Nov 2013, David Cohen wrote:
> >> + /* > >> + * Controller requires buffer size to be aligned to > >> + * maxpacketsize of an out endpoint. > >> + */ > >> + if (gadget->quirk_ep_out_aligned_size && read) { > >> + /* > >> + * We pass 'orig_len' to usp_ep_align_maxpacketsize() > >> + * due to we're in a loop and 'len' may have been > >> + * changed. > >> + */ > >> + len = usb_ep_align_maxpacketsize(ep->ep, orig_len); > >> + if (data && len > data_len) { > >> + kfree(data); > >> + data = NULL; > >> + data_len = 0; > >> + } > >> + } > > > > Since the value of orig_len never changes, there's no point calling > > usb_ep_align_maxpacketsize() inside the loop. You should call it only > > once, before the loop starts. Once you do that, you won't need > > orig_len at all. > > orig_len doesn't change but ep->ep does. If USB specs say max packet > size won't change even if ep does, than we can call it from outside the > loop.
I'm not too familiar with this driver. It looks like the only way ep->ep can change is if the endpoint gets enabled while you're sitting inside the wait_event_interruptible() call.
In fact, the whole structure of that loop looks peculiar. Why not acquire the mutex first and then do everything else?
Does it even make sense for ep to change? Would this change be visible to the host? What if the host changes the alternate setting while this loop is running -- does it make sense for the userspace program to start a read or write under one altsetting but then have the read/write take place under a different altsetting?
Alan Stern
|  |