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



    \
     
     \ /
      Last update: 2013-11-12 00:41    [W:3.136 / U:0.604 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site