From: Michal Nazarewicz <mina86@mina86.com>
To: John Keeping <john@metanate.com>, linux-usb@vger.kernel.org
Cc: Felipe Balbi <balbi@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org, John Keeping <john@metanate.com>
Subject: Re: [PATCH v2] usb: gadget: ffs: handle I/O completion in-order
In-Reply-To: <20170912092440.18864-1-john@metanate.com>
Organization: http://mina86.com/
References: <20170912092440.18864-1-john@metanate.com>
Face: iVBORw0KGgoAAAANSUhEUgAAADAAAAAwBAMAAAClLOS0AAAAJFBMVEWbfGlUPDDHgE57V0jUupKjgIObY0PLrom9mH4dFRK4gmjPs41MxjOgAAACP0lEQVQ4T23Sv2vbQBQHcBk1xE6WyALX107VUEgmn6+ouUwpEQQ6uRjttkWP4CkBg2M0BQLBdPFZYPsyFYo7qEtKDQ7on+t7+nF2Ux8ahD587717OmNYrOvycHsZ+o2r051wHTHysAvGb8ygvgu4QWT0sCmkgZCIEnlV2X8BtyraazFGDuxhmKSQJMlwHQ7v5MHSNxmz78rfElwAa3ieVD9e+hBhjaPDDG6NgFo2f4wBMNIo5YmRtF0RyDgFjJjlMIWbnuM4x9MMfABGTlN4qgIQB4A1DEyA1BHWtfeWNUMwiVJKoqh97KrkOO+qzgluVYLvFCUKAX73nONeBr7BGMdM6Sg0kuep03VywLaIzRiVr+GAzKlpQIsAFnWAG2e6DT5WmWDiudZMIc6hYrMOmeMQK9WX0B+/RfjzL9DI7Y9/Iayn29Ci0r2i4f9gMimMSZLCDMalgQGU5hnUtqAN0OGvEmO1Wnl0C0wWSCEHnuHBqmygxdxA8oWXwbipoc1EoNR9DqOpBpOJrnr0criQab9ZT4LL+wI+K7GBQH30CrhUruilgP9DRTrhVWZCiAyILP+wiuLeCKGTD6r/nc8LOJcAwR6IBTUs+7CASw3QFZ0MdA2PI3zNziH4ZKVhXCRMBjeZ1DWMekKwDCASwExy+NQ86TaykaDAFHO4aP48y4fIcDM5yOG8GcTLbOyp8A8azjJI93JFd1EA6yN8sSxMQJWoABqniRZVykYgRXErzrdqExAoUrRb0xfRp8p2A/4XmfilTtkDZ4cAAAAASUVORK5CYII=
OpenPGP: id=AC1F5F5CD41888F8CC8458582060401250751FF4; url=http://mina86.com/mina86.pub
Date: Tue, 12 Sep 2017 12:11:36 +0200
Message-ID: <xa1t1snci5uv.fsf@mina86.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Sender: linux-kernel-owner@vger.kernel.org
Content-Transfer-Encoding: 8bit
Content-Length: 4148
Lines: 112

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»