lkml.org 
[lkml]   [2017]   [Sep]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v2] usb: gadget: ffs: handle I/O completion in-order
Date
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»

\
 
 \ /
  Last update: 2017-09-12 12:12    [W:0.044 / U:0.884 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site