From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753949Ab3KKXRs (ORCPT ); Mon, 11 Nov 2013 18:17:48 -0500 Received: from mga02.intel.com ([134.134.136.20]:39261 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753598Ab3KKXRa (ORCPT ); Mon, 11 Nov 2013 18:17:30 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.93,535,1378882800"; d="scan'208";a="433508962" Message-ID: <52816696.1000806@linux.intel.com> Date: Mon, 11 Nov 2013 15:21:58 -0800 From: David Cohen User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20131005 Icedove/17.0.9 MIME-Version: 1.0 To: David Cohen CC: balbi@ti.com, gregkh@linuxfoundation.org, stern@rowland.harvard.edu, mina86@mina86.com, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 3/5] usb: gadget: f_fs: remove loop from I/O function References: <1384201011-11114-1-git-send-email-david.a.cohen@linux.intel.com> <1384201011-11114-4-git-send-email-david.a.cohen@linux.intel.com> In-Reply-To: <1384201011-11114-4-git-send-email-david.a.cohen@linux.intel.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/11/2013 12:16 PM, David Cohen wrote: > From: Michal Nazarewicz > > 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 > Cc: David Cohen > --- > 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); >