lkml.org 
[lkml]   [2013]   [Nov]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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



\
 
 \ /
  Last update: 2013-11-05 17:21    [W:0.061 / U:0.500 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site