Re: [PATCH v5 3/5] usb: gadget: f_fs: remove loop from I/O function

[David Cohen]

On 11/11/2013 12:16 PM, David Cohen wrote:
> From: Michal Nazarewicz <mina86@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Cc: David Cohen <david.a.cohen@xxxxxxxxxxxxxxx>
> ---
>   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);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/