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 <j...@metanate.com>
Acked-by: Michal Nazarewicz <min...@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»
