Re: [PATCH v2] usb: gadget: ffs: handle I/O completion in-order

[Michal Nazarewicz <mina86@mina86.com>]

On Tue, Sep 12 2017, John Keeping wrote:
> By submitting completed transfers to the system workqueue there is no
> guarantee that completion events will be queued up in the correct order,
> as in multi-processor systems there is a thread running for each
> processor and the work items are not bound to a particular core.
>
> This means that several completions are in the queue at the same time,
> they may be processed in parallel and complete out of order, resulting
> in data appearing corrupt when read by userspace.
>
> Create a single-threaded workqueue for FunctionFS so that data completed
> requests is passed to userspace in the order in which they complete.
>
> Signed-off-by: John Keeping <john@metanate.com>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> ---
> I originally sent a version of this patch back in July [1] without any
> response.  Since then, I've improved the commit message and switched
> from create_singlethread_workqueue() to alloc_ordered_workqueue(), so
> I've marked this as v2.
>
> [1] https://patchwork.kernel.org/patch/9838441/
>
>  drivers/usb/gadget/function/f_fs.c | 17 +++++++++++++----
>  drivers/usb/gadget/function/u_fs.h |  1 +
>  2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 9990944a7245..8b342587f8ad 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -46,7 +46,8 @@
>  static void ffs_data_get(struct ffs_data *ffs);
>  static void ffs_data_put(struct ffs_data *ffs);
>  /* Creates new ffs_data object. */
> -static struct ffs_data *__must_check ffs_data_new(void) __attribute__((malloc));
> +static struct ffs_data *__must_check ffs_data_new(const char *dev_name)
> +	__attribute__((malloc));
>  
>  /* Opened counter handling. */
>  static void ffs_data_opened(struct ffs_data *ffs);
> @@ -780,11 +781,12 @@ static void ffs_epfile_async_io_complete(struct usb_ep *_ep,
>  					 struct usb_request *req)
>  {
>  	struct ffs_io_data *io_data = req->context;
> +	struct ffs_data *ffs = io_data->ffs;
>  
>  	ENTER();
>  
>  	INIT_WORK(&io_data->work, ffs_user_copy_worker);
> -	schedule_work(&io_data->work);
> +	queue_work(ffs->io_completion_wq, &io_data->work);
>  }
>  
>  static void __ffs_epfile_read_buffer_free(struct ffs_epfile *epfile)
> @@ -1500,7 +1502,7 @@ ffs_fs_mount(struct file_system_type *t, int flags,
>  	if (unlikely(ret < 0))
>  		return ERR_PTR(ret);
>  
> -	ffs = ffs_data_new();
> +	ffs = ffs_data_new(dev_name);
>  	if (unlikely(!ffs))
>  		return ERR_PTR(-ENOMEM);
>  	ffs->file_perms = data.perms;
> @@ -1610,6 +1612,7 @@ static void ffs_data_put(struct ffs_data *ffs)
>  		BUG_ON(waitqueue_active(&ffs->ev.waitq) ||
>  		       waitqueue_active(&ffs->ep0req_completion.wait) ||
>  		       waitqueue_active(&ffs->wait));
> +		destroy_workqueue(ffs->io_completion_wq);
>  		kfree(ffs->dev_name);
>  		kfree(ffs);
>  	}
> @@ -1642,7 +1645,7 @@ static void ffs_data_closed(struct ffs_data *ffs)
>  	ffs_data_put(ffs);
>  }
>  
> -static struct ffs_data *ffs_data_new(void)
> +static struct ffs_data *ffs_data_new(const char *dev_name)
>  {
>  	struct ffs_data *ffs = kzalloc(sizeof *ffs, GFP_KERNEL);
>  	if (unlikely(!ffs))
> @@ -1650,6 +1653,12 @@ static struct ffs_data *ffs_data_new(void)
>  
>  	ENTER();
>  
> +	ffs->io_completion_wq = alloc_ordered_workqueue("%s", 0, dev_name);
> +	if (!ffs->io_completion_wq) {
> +		kfree(ffs);
> +		return NULL;
> +	}
> +
>  	refcount_set(&ffs->ref, 1);
>  	atomic_set(&ffs->opened, 0);
>  	ffs->state = FFS_READ_DESCRIPTORS;
> diff --git a/drivers/usb/gadget/function/u_fs.h b/drivers/usb/gadget/function/u_fs.h
> index 540f1c48c1a8..79f70ebf85dc 100644
> --- a/drivers/usb/gadget/function/u_fs.h
> +++ b/drivers/usb/gadget/function/u_fs.h
> @@ -279,6 +279,7 @@ struct ffs_data {
>  	}				file_perms;
>  
>  	struct eventfd_ctx *ffs_eventfd;
> +	struct workqueue_struct *io_completion_wq;
>  	bool no_disconnect;
>  	struct work_struct reset_work;

-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»