Messages in this thread Patch in this message | | | Date | Mon, 11 Nov 2013 15:21:58 -0800 | From | David Cohen <> | Subject | Re: [PATCH v5 3/5] usb: gadget: f_fs: remove loop from I/O function |
| |
On 11/11/2013 12:16 PM, David Cohen wrote: > From: Michal Nazarewicz <mina86@mina86.com> > > When endpoint changes (due to it being disabled or alt setting changed), > mimic the action as if the change happened after the request has been > queued, instead of retrying with the new endpoint. > > Signed-off-by: Michal Nazarewicz <mina86@mina86.com> > Cc: David Cohen <david.a.cohen@linux.intel.com> > --- > drivers/usb/gadget/f_fs.c | 94 +++++++++++++++++++++-------------------------- > 1 file changed, 41 insertions(+), 53 deletions(-) > > diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c > index 75e4b7846a8d..f1563c47e0c2 100644 > --- a/drivers/usb/gadget/f_fs.c > +++ b/drivers/usb/gadget/f_fs.c > @@ -756,74 +756,61 @@ static ssize_t ffs_epfile_io(struct file *file, > { > struct ffs_epfile *epfile = file->private_data; > struct ffs_ep *ep; > - char *data = NULL; > ssize_t ret; > + char *data; > int halt; > > - goto first_try; > - do { > - spin_unlock_irq(&epfile->ffs->eps_lock); > - mutex_unlock(&epfile->mutex); > + /* Are we still active? */ > + if (WARN_ON(epfile->ffs->state != FFS_ACTIVE)) { > + ret = -ENODEV; > + goto error; > + } > > -first_try: > - /* Are we still active? */ > - if (WARN_ON(epfile->ffs->state != FFS_ACTIVE)) { > - ret = -ENODEV; > + /* Wait for endpoint to be enabled */ > + ep = epfile->ep; > + if (!ep) { > + if (file->f_flags & O_NONBLOCK) { > + ret = -EAGAIN; > goto error; > } > > - /* Wait for endpoint to be enabled */ > - ep = epfile->ep; > - if (!ep) { > - if (file->f_flags & O_NONBLOCK) { > - ret = -EAGAIN; > - goto error; > - } > - > - if (wait_event_interruptible(epfile->wait, > - (ep = epfile->ep))) { > - ret = -EINTR; > - goto error; > - } > - } > - > - /* Do we halt? */ > - halt = !read == !epfile->in; > - if (halt && epfile->isoc) { > - ret = -EINVAL; > + if (wait_event_interruptible(epfile->wait, (ep = epfile->ep))) { > + ret = -EINTR; > goto error; > } > + } > > - /* Allocate & copy */ > - if (!halt && !data) { > - data = kzalloc(len, GFP_KERNEL); > - if (unlikely(!data)) > - return -ENOMEM; > + /* Do we halt? */ > + halt = !read == !epfile->in;
One curiosity here. This patch prints the following warning:
In file included from (...)/drivers/usb/gadget/g_ffs.c:55:0: (...)/drivers/usb/gadget/f_fs.c: In function 'ffs_epfile_io.isra.18': (...)/drivers/usb/gadget/f_fs.c:837:15: warning: 'data_len' may be used uninitialized in this function [-Wmaybe-uninitialized]
But if we do this dummy change on it:
@@ -782,7 +782,10 @@ static ssize_t ffs_epfile_io(struct file *file, }
/* Do we halt? */ - halt = !read == !epfile->in; + if (!read == !epfile->in) + halt = 1; + else + halt = 0; if (halt && epfile->isoc) { ret = -EINVAL; goto error;
The warning goes away. This false-positive warning comes out of this gcc version: $ i686-linux-android-gcc --version i686-linux-android-gcc (GCC) 4.7
Br, David Cohen
> + if (halt && epfile->isoc) { > + ret = -EINVAL; > + goto error; > + } > > - if (!read && > - unlikely(__copy_from_user(data, buf, len))) { > - ret = -EFAULT; > - goto error; > - } > - } > + /* Allocate & copy */ > + if (!halt) { > + data = kmalloc(len, GFP_KERNEL); > + if (unlikely(!data)) > + return -ENOMEM; > > - /* We will be using request */ > - ret = ffs_mutex_lock(&epfile->mutex, > - file->f_flags & O_NONBLOCK); > - if (unlikely(ret)) > + if (!read && unlikely(copy_from_user(data, buf, len))) { > + ret = -EFAULT; > goto error; > + } > + } > > - /* > - * We're called from user space, we can use _irq rather then > - * _irqsave > - */ > - spin_lock_irq(&epfile->ffs->eps_lock); > + /* We will be using request */ > + ret = ffs_mutex_lock(&epfile->mutex, file->f_flags & O_NONBLOCK); > + if (unlikely(ret)) > + goto error; > + spin_lock_irq(&epfile->ffs->eps_lock); > > - /* > - * While we were acquiring mutex endpoint got disabled > - * or changed? > - */ > - } while (unlikely(epfile->ep != ep)); > + /* While we were acquiring mutex endpoint got disabled or changed. */ > + if (epfile->ep != ep) { > + ret = -ESHUTDOWN; > + spin_unlock_irq(&epfile->ffs->eps_lock); > + goto error_unlock; > + } > > /* Halt */ > if (unlikely(halt)) { > @@ -858,6 +845,7 @@ first_try: > } > } > > +error_unlock: > mutex_unlock(&epfile->mutex); > error: > kfree(data); >
| |