[RFC PATCH] usb: gadget: dfu: Wrap fs operation in workqueue

[RFC PATCH] usb: gadget: dfu: Wrap fs operation in workqueue

[Jules Maselbas]

File system operation shouldn't be executed in a poller. Use
a workqueue to delay filesystem operation to command context.

This is an RFC, extra work must be done to properly handle error
cases and dfu cleanup.

Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
---
 drivers/usb/gadget/dfu.c | 321 ++++++++++++++++++++++++++-------------
 1 file changed, 216 insertions(+), 105 deletions(-)

diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
index 9d6a9d252..75abd1576 100644
--- a/drivers/usb/gadget/dfu.c
+++ b/drivers/usb/gadget/dfu.c
@@ -54,6 +54,7 @@
 #include <fs.h>
 #include <ioctl.h>
 #include <linux/mtd/mtd-abi.h>
+#include <work.h>
 
 #define USB_DT_DFU			0x21
 
@@ -153,6 +154,7 @@ struct f_dfu {
 	u8	dfu_state;
 	u8	dfu_status;
 	struct usb_request		*dnreq;
+	struct work_queue wq;
 };
 
 static inline struct f_dfu *func_to_dfu(struct usb_function *f)
@@ -173,6 +175,178 @@ static struct usb_gadget_strings *dfu_strings[] = {
 };
 
 static void dn_complete(struct usb_ep *ep, struct usb_request *req);
+static void up_complete(struct usb_ep *ep, struct usb_request *req);
+static void dfu_cleanup(struct f_dfu *dfu);
+
+struct dfu_work {
+	struct work_struct work;
+	struct f_dfu *dfu;
+	void (*task)(struct dfu_work *dw);
+	size_t len;
+	uint8_t *rbuf;
+	uint8_t wbuf[CONFIG_USBD_DFU_XFER_SIZE];
+};
+
+static void dfu_do_work(struct work_struct *w)
+{
+	struct dfu_work *dw = container_of(w, struct dfu_work, work);
+
+	/* TODO: find a better way to skip tasks when the dfu gadget
+	 * has encounter an error and dfu_cleanup has been called */
+	if (dw->task && dw->dfu->dfu_status == DFU_STATUS_OK)
+		dw->task(dw);
+
+	free(dw);
+}
+
+static void dfu_work_cancel(struct work_struct *w)
+{
+	struct dfu_work *dw = container_of(w, struct dfu_work, work);
+
+	free(dw);
+}
+
+static void dfu_do_write(struct dfu_work *dw)
+{
+	struct f_dfu *dfu = dw->dfu;
+	size_t size, wlen = dw->len;
+	int ret;
+
+	debug("do write\n");
+
+	if (prog_erase && (dfu_written + wlen) > dfu_erased) {
+		size = roundup(wlen, dfu_mtdinfo.erasesize);
+		ret = erase(dfufd, size, dfu_erased);
+		dfu_erased += size;
+		if (ret && ret != -ENOSYS) {
+			perror("erase");
+			dfu->dfu_status = DFU_STATUS_errERASE;
+			dfu_cleanup(dfu);
+			return;
+		}
+	}
+
+	dfu_written += wlen;
+	ret = write(dfufd, dw->wbuf, wlen);
+	if (ret < (int)wlen) {
+		perror("write");
+		dfu->dfu_status = DFU_STATUS_errWRITE;
+		dfu_cleanup(dfu);
+	}
+}
+
+static void dfu_do_read(struct dfu_work *dw)
+{
+	struct f_dfu *dfu = dw->dfu;
+	struct usb_composite_dev *cdev = dfu->func.config->cdev;
+	size_t size, rlen = dw->len;
+
+	debug("do read\n");
+
+	size = read(dfufd, dfu->dnreq->buf, rlen);
+	dfu->dnreq->length = size;
+	if (size < (int)rlen) {
+		perror("read");
+		dfu_cleanup(dfu);
+		dfu->dfu_state = DFU_STATE_dfuIDLE;
+	}
+
+	dfu->dnreq->complete = up_complete;
+	usb_ep_queue(cdev->gadget->ep0, dfu->dnreq);
+}
+
+static void dfu_do_open_dnload(struct dfu_work *dw)
+{
+	struct f_dfu *dfu = dw->dfu;
+	int ret;
+
+	debug("do open dnload\n");
+
+	if (dfu_file_entry->flags & FILE_LIST_FLAG_SAFE) {
+		dfufd = open(DFU_TEMPFILE, O_WRONLY | O_CREAT);
+	} else {
+		unsigned flags = O_WRONLY;
+
+		if (dfu_file_entry->flags & FILE_LIST_FLAG_CREATE)
+			flags |= O_CREAT | O_TRUNC;
+
+		dfufd = open(dfu_file_entry->filename, flags);
+	}
+
+	if (dfufd < 0) {
+		perror("open");
+		dfu->dfu_status = DFU_STATUS_errFILE;
+		goto out;
+	}
+
+	if (!(dfu_file_entry->flags & FILE_LIST_FLAG_SAFE)) {
+		ret = ioctl(dfufd, MEMGETINFO, &dfu_mtdinfo);
+		if (!ret) /* file is on a mtd device */
+			prog_erase = 1;
+	}
+
+	return;
+out:
+	dfu->dfu_state = DFU_STATE_dfuERROR;
+	dfu_cleanup(dfu);
+}
+
+static void dfu_do_open_upload(struct dfu_work *dw)
+{
+	struct f_dfu *dfu = dw->dfu;
+
+	debug("do open upload\n");
+
+	dfufd = open(dfu_file_entry->filename, O_RDONLY);
+	if (dfufd < 0) {
+		perror("open");
+		dfu->dfu_status = DFU_STATUS_errFILE;
+		dfu->dfu_state = DFU_STATE_dfuERROR;
+		dfu_cleanup(dfu);
+	}
+}
+
+static void dfu_do_copy(struct dfu_work *dw)
+{
+	struct f_dfu *dfu = dw->dfu;
+	unsigned flags = O_WRONLY;
+	int ret, fd;
+
+	debug("do copy\n");
+
+	if (dfu_file_entry->flags & FILE_LIST_FLAG_CREATE)
+		flags |= O_CREAT | O_TRUNC;
+
+	fd = open(dfu_file_entry->filename, flags);
+	if (fd < 0) {
+		perror("open");
+		dfu->dfu_status = DFU_STATUS_errERASE;
+		goto out;
+	}
+
+	ret = erase(fd, ERASE_SIZE_ALL, 0);
+	close(fd);
+	if (ret && ret != -ENOSYS) {
+		perror("erase");
+		dfu->dfu_status = DFU_STATUS_errERASE;
+		goto out;
+	}
+
+	ret = copy_file(DFU_TEMPFILE, dfu_file_entry->filename, 0);
+	if (ret) {
+		dfu->dfu_status = DFU_STATUS_errWRITE;
+		printf("copy file failed\n");
+		goto out;
+	}
+
+	dfu->dfu_state = DFU_STATE_dfuIDLE;
+	dfu_cleanup(dfu);
+
+	return;
+out:
+	dfu->dfu_state = DFU_STATE_dfuERROR;
+	dfu_cleanup(dfu);
+}
 
 static int
 dfu_bind(struct usb_configuration *c, struct usb_function *f)
@@ -223,6 +397,10 @@ dfu_bind(struct usb_configuration *c, struct usb_function *f)
 		goto out;
 	}
 
+	dfu->wq.fn = dfu_do_work;
+	dfu->wq.cancel = dfu_work_cancel;
+	wq_register(&dfu->wq);
+
 	/* allocate instance-specific interface IDs, and patch descriptors */
 	status = usb_interface_id(c, f);
 	if (status < 0)
@@ -278,6 +456,8 @@ dfu_unbind(struct usb_configuration *c, struct usb_function *f)
 	dfu_file_entry = NULL;
 	dfudetach = 0;
 
+	wq_unregister(&dfu->wq);
+
 	usb_free_all_descriptors(f);
 
 	dma_free(dfu->dnreq->buf);
@@ -327,6 +507,9 @@ static void dfu_cleanup(struct f_dfu *dfu)
 	dfu_erased = 0;
 	prog_erase = 0;
 
+	/* TODO: Right now, close and stat operation can be called
+	 * in a poller, in dfu_abort and dfu_disable. */
+
 	if (dfufd > 0) {
 		close(dfufd);
 		dfufd = -EINVAL;
@@ -339,28 +522,15 @@ static void dfu_cleanup(struct f_dfu *dfu)
 static void dn_complete(struct usb_ep *ep, struct usb_request *req)
 {
 	struct f_dfu		*dfu = req->context;
-	loff_t size;
-	int ret;
+	struct dfu_work *dw;
 
-	if (prog_erase && (dfu_written + req->length) > dfu_erased) {
-		size = roundup(req->length, dfu_mtdinfo.erasesize);
-		ret = erase(dfufd, size, dfu_erased);
-		dfu_erased += size;
-		if (ret && ret != -ENOSYS) {
-			perror("erase");
-			dfu->dfu_status = DFU_STATUS_errERASE;
-			dfu_cleanup(dfu);
-			return;
-		}
-	}
+	dw = xzalloc(sizeof(*dw));
+	dw->dfu = dfu;
+	dw->task = dfu_do_write;
+	dw->len = min_t(unsigned int, req->length, CONFIG_USBD_DFU_XFER_SIZE);
+	memcpy(dw->wbuf, req->buf, dw->len);
 
-	dfu_written += req->length;
-	ret = write(dfufd, req->buf, req->length);
-	if (ret < (int)req->length) {
-		perror("write");
-		dfu->dfu_status = DFU_STATUS_errWRITE;
-		dfu_cleanup(dfu);
-	}
+	wq_queue_work(&dfu->wq, &dw->work);
 }
 
 static int handle_dnload(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
@@ -370,12 +540,7 @@ static int handle_dnload(struct usb_function *f, const struct usb_ctrlrequest *c
 	u16			w_length = le16_to_cpu(ctrl->wLength);
 
 	if (w_length == 0) {
-		if (dfu_file_entry->flags & FILE_LIST_FLAG_SAFE) {
-			dfu->dfu_state = DFU_STATE_dfuMANIFEST;
-		} else {
-			dfu->dfu_state = DFU_STATE_dfuIDLE;
-			dfu_cleanup(dfu);
-		}
+		dfu->dfu_state = DFU_STATE_dfuMANIFEST_SYNC;
 		return 0;
 	}
 
@@ -389,48 +554,18 @@ static int handle_dnload(struct usb_function *f, const struct usb_ctrlrequest *c
 static int handle_manifest(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 {
 	struct f_dfu		*dfu = func_to_dfu(f);
-	int ret;
+	struct dfu_work *dw;
 
 	dfu->dfu_state = DFU_STATE_dfuIDLE;
 
 	if (dfu_file_entry->flags & FILE_LIST_FLAG_SAFE) {
-		int fd;
-		unsigned flags = O_WRONLY;
-
-		if (dfu_file_entry->flags & FILE_LIST_FLAG_CREATE)
-			flags |= O_CREAT | O_TRUNC;
-
-		fd = open(dfu_file_entry->filename, flags);
-		if (fd < 0) {
-			perror("open");
-			dfu->dfu_status = DFU_STATUS_errERASE;
-			ret = -EINVAL;
-			goto out;
-		}
-
-		ret = erase(fd, ERASE_SIZE_ALL, 0);
-		close(fd);
-		if (ret && ret != -ENOSYS) {
-			dfu->dfu_status = DFU_STATUS_errERASE;
-			perror("erase");
-			goto out;
-		}
-
-		ret = copy_file(DFU_TEMPFILE, dfu_file_entry->filename, 0);
-		if (ret) {
-			printf("copy file failed\n");
-			ret = -EINVAL;
-			goto out;
-		}
+		dw = xzalloc(sizeof(*dw));
+		dw->dfu = dfu;
+		dw->task = dfu_do_copy;
+		wq_queue_work(&dfu->wq, &dw->work);
 	}
 
 	return 0;
-
-out:
-	dfu->dfu_status = DFU_STATUS_errWRITE;
-	dfu->dfu_state = DFU_STATE_dfuERROR;
-	dfu_cleanup(dfu);
-	return ret;
 }
 
 static void up_complete(struct usb_ep *ep, struct usb_request *req)
@@ -440,20 +575,17 @@ static void up_complete(struct usb_ep *ep, struct usb_request *req)
 static int handle_upload(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 {
 	struct f_dfu		*dfu = func_to_dfu(f);
-	struct usb_composite_dev *cdev = f->config->cdev;
+	struct dfu_work *dw;
 	u16			w_length = le16_to_cpu(ctrl->wLength);
-	int len;
-
-	len = read(dfufd, dfu->dnreq->buf, w_length);
-
-	dfu->dnreq->length = len;
-	if (len < w_length) {
-		dfu_cleanup(dfu);
-		dfu->dfu_state = DFU_STATE_dfuIDLE;
-	}
 
-	dfu->dnreq->complete = up_complete;
-	usb_ep_queue(cdev->gadget->ep0, dfu->dnreq);
+	/* RFC: I didn't found a better way to queue the usb response other
+	 * than making dfu_do_read call usb_ep_queue after reading from file */
+	dw = xzalloc(sizeof(*dw));
+	dw->dfu = dfu;
+	dw->task = dfu_do_read;
+	dw->len = w_length;
+	dw->rbuf = dfu->dnreq->buf;
+	wq_queue_work(&dfu->wq, &dw->work);
 
 	return 0;
 }
@@ -474,7 +606,7 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 	int			value = -EOPNOTSUPP;
 	int			w_length = le16_to_cpu(ctrl->wLength);
 	int			w_value = le16_to_cpu(ctrl->wValue);
-	int ret;
+	struct dfu_work *dw;
 
 	if (ctrl->bRequestType == USB_DIR_IN && ctrl->bRequest == USB_REQ_GET_DESCRIPTOR
 			&& (w_value >> 8) == 0x21) {
@@ -501,28 +633,10 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 				goto out;
 			}
 			debug("dfu: starting download to %s\n", dfu_file_entry->filename);
-			if (dfu_file_entry->flags & FILE_LIST_FLAG_SAFE) {
-				dfufd = open(DFU_TEMPFILE, O_WRONLY | O_CREAT);
-			} else {
-				unsigned flags = O_WRONLY;
-
-				if (dfu_file_entry->flags & FILE_LIST_FLAG_CREATE)
-					flags |= O_CREAT | O_TRUNC;
-
-				dfufd = open(dfu_file_entry->filename, flags);
-			}
-
-			if (dfufd < 0) {
-				dfu->dfu_state = DFU_STATE_dfuERROR;
-				perror("open");
-				goto out;
-			}
-
-			if (!(dfu_file_entry->flags & FILE_LIST_FLAG_SAFE)) {
-				ret = ioctl(dfufd, MEMGETINFO, &dfu_mtdinfo);
-				if (!ret) /* file is on a mtd device */
-					prog_erase = 1;
-			}
+			dw = xzalloc(sizeof(*dw));
+			dw->dfu = dfu;
+			dw->task = dfu_do_open_dnload;
+			wq_queue_work(&dfu->wq, &dw->work);
 
 			value = handle_dnload(f, ctrl);
 			dfu->dfu_state = DFU_STATE_dfuDNLOAD_IDLE;
@@ -534,12 +648,12 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 				dfu->dfu_state = DFU_STATE_dfuERROR;
 				goto out;
 			}
-			dfufd = open(dfu_file_entry->filename, O_RDONLY);
-			if (dfufd < 0) {
-				dfu->dfu_state = DFU_STATE_dfuERROR;
-				perror("open");
-				goto out;
-			}
+
+			dw = xzalloc(sizeof(*dw));
+			dw->dfu = dfu;
+			dw->task = dfu_do_open_upload;
+			wq_queue_work(&dfu->wq, &dw->work);
+
 			handle_upload(f, ctrl);
 			return 0;
 		case USB_REQ_DFU_ABORT:
@@ -648,9 +762,6 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 		break;
 	case DFU_STATE_dfuMANIFEST:
 		value = handle_manifest(f, ctrl);
-		if (dfu->dfu_state != DFU_STATE_dfuIDLE) {
-			return 0;
-		}
 		switch (ctrl->bRequest) {
 		case USB_REQ_DFU_GETSTATUS:
 			value = dfu_status(f, ctrl);
-- 
2.17.1



_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC PATCH] usb: gadget: dfu: Wrap fs operation in workqueue

[Ahmad Fatoum]

Hello Jules,

On 27.01.21 17:49, Jules Maselbas wrote:
> File system operation shouldn't be executed in a poller. Use
> a workqueue to delay filesystem operation to command context.
> 
> This is an RFC, extra work must be done to properly handle error
> cases and dfu cleanup.

I erroneously thought the poller is within the DFU bits. I wonder what
side-effect moving the whole USB gadget polling into a workqueue would
have. In that case, we wouldn't need to any changes for DFU itself.

Jules, Sascha, thoughts?

Cheers,
Ahmad

> 
> Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
> ---
>  drivers/usb/gadget/dfu.c | 321 ++++++++++++++++++++++++++-------------
>  1 file changed, 216 insertions(+), 105 deletions(-)
> 
> diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
> index 9d6a9d252..75abd1576 100644
> --- a/drivers/usb/gadget/dfu.c
> +++ b/drivers/usb/gadget/dfu.c
> @@ -54,6 +54,7 @@
>  #include <fs.h>
>  #include <ioctl.h>
>  #include <linux/mtd/mtd-abi.h>
> +#include <work.h>
>  
>  #define USB_DT_DFU			0x21
>  
> @@ -153,6 +154,7 @@ struct f_dfu {
>  	u8	dfu_state;
>  	u8	dfu_status;
>  	struct usb_request		*dnreq;
> +	struct work_queue wq;
>  };
>  
>  static inline struct f_dfu *func_to_dfu(struct usb_function *f)
> @@ -173,6 +175,178 @@ static struct usb_gadget_strings *dfu_strings[] = {
>  };
>  
>  static void dn_complete(struct usb_ep *ep, struct usb_request *req);
> +static void up_complete(struct usb_ep *ep, struct usb_request *req);
> +static void dfu_cleanup(struct f_dfu *dfu);
> +
> +struct dfu_work {
> +	struct work_struct work;
> +	struct f_dfu *dfu;
> +	void (*task)(struct dfu_work *dw);
> +	size_t len;
> +	uint8_t *rbuf;
> +	uint8_t wbuf[CONFIG_USBD_DFU_XFER_SIZE];
> +};
> +
> +static void dfu_do_work(struct work_struct *w)
> +{
> +	struct dfu_work *dw = container_of(w, struct dfu_work, work);
> +
> +	/* TODO: find a better way to skip tasks when the dfu gadget
> +	 * has encounter an error and dfu_cleanup has been called */
> +	if (dw->task && dw->dfu->dfu_status == DFU_STATUS_OK)
> +		dw->task(dw);
> +
> +	free(dw);
> +}
> +
> +static void dfu_work_cancel(struct work_struct *w)
> +{
> +	struct dfu_work *dw = container_of(w, struct dfu_work, work);
> +
> +	free(dw);
> +}
> +
> +static void dfu_do_write(struct dfu_work *dw)
> +{
> +	struct f_dfu *dfu = dw->dfu;
> +	size_t size, wlen = dw->len;
> +	int ret;
> +
> +	debug("do write\n");
> +
> +	if (prog_erase && (dfu_written + wlen) > dfu_erased) {
> +		size = roundup(wlen, dfu_mtdinfo.erasesize);
> +		ret = erase(dfufd, size, dfu_erased);
> +		dfu_erased += size;
> +		if (ret && ret != -ENOSYS) {
> +			perror("erase");
> +			dfu->dfu_status = DFU_STATUS_errERASE;
> +			dfu_cleanup(dfu);
> +			return;
> +		}
> +	}
> +
> +	dfu_written += wlen;
> +	ret = write(dfufd, dw->wbuf, wlen);
> +	if (ret < (int)wlen) {
> +		perror("write");
> +		dfu->dfu_status = DFU_STATUS_errWRITE;
> +		dfu_cleanup(dfu);
> +	}
> +}
> +
> +static void dfu_do_read(struct dfu_work *dw)
> +{
> +	struct f_dfu *dfu = dw->dfu;
> +	struct usb_composite_dev *cdev = dfu->func.config->cdev;
> +	size_t size, rlen = dw->len;
> +
> +	debug("do read\n");
> +
> +	size = read(dfufd, dfu->dnreq->buf, rlen);
> +	dfu->dnreq->length = size;
> +	if (size < (int)rlen) {
> +		perror("read");
> +		dfu_cleanup(dfu);
> +		dfu->dfu_state = DFU_STATE_dfuIDLE;
> +	}
> +
> +	dfu->dnreq->complete = up_complete;
> +	usb_ep_queue(cdev->gadget->ep0, dfu->dnreq);
> +}
> +
> +static void dfu_do_open_dnload(struct dfu_work *dw)
> +{
> +	struct f_dfu *dfu = dw->dfu;
> +	int ret;
> +
> +	debug("do open dnload\n");
> +
> +	if (dfu_file_entry->flags & FILE_LIST_FLAG_SAFE) {
> +		dfufd = open(DFU_TEMPFILE, O_WRONLY | O_CREAT);
> +	} else {
> +		unsigned flags = O_WRONLY;
> +
> +		if (dfu_file_entry->flags & FILE_LIST_FLAG_CREATE)
> +			flags |= O_CREAT | O_TRUNC;
> +
> +		dfufd = open(dfu_file_entry->filename, flags);
> +	}
> +
> +	if (dfufd < 0) {
> +		perror("open");
> +		dfu->dfu_status = DFU_STATUS_errFILE;
> +		goto out;
> +	}
> +
> +	if (!(dfu_file_entry->flags & FILE_LIST_FLAG_SAFE)) {
> +		ret = ioctl(dfufd, MEMGETINFO, &dfu_mtdinfo);
> +		if (!ret) /* file is on a mtd device */
> +			prog_erase = 1;
> +	}
> +
> +	return;
> +out:
> +	dfu->dfu_state = DFU_STATE_dfuERROR;
> +	dfu_cleanup(dfu);
> +}
> +
> +static void dfu_do_open_upload(struct dfu_work *dw)
> +{
> +	struct f_dfu *dfu = dw->dfu;
> +
> +	debug("do open upload\n");
> +
> +	dfufd = open(dfu_file_entry->filename, O_RDONLY);
> +	if (dfufd < 0) {
> +		perror("open");
> +		dfu->dfu_status = DFU_STATUS_errFILE;
> +		dfu->dfu_state = DFU_STATE_dfuERROR;
> +		dfu_cleanup(dfu);
> +	}
> +}
> +
> +static void dfu_do_copy(struct dfu_work *dw)
> +{
> +	struct f_dfu *dfu = dw->dfu;
> +	unsigned flags = O_WRONLY;
> +	int ret, fd;
> +
> +	debug("do copy\n");
> +
> +	if (dfu_file_entry->flags & FILE_LIST_FLAG_CREATE)
> +		flags |= O_CREAT | O_TRUNC;
> +
> +	fd = open(dfu_file_entry->filename, flags);
> +	if (fd < 0) {
> +		perror("open");
> +		dfu->dfu_status = DFU_STATUS_errERASE;
> +		goto out;
> +	}
> +
> +	ret = erase(fd, ERASE_SIZE_ALL, 0);
> +	close(fd);
> +	if (ret && ret != -ENOSYS) {
> +		perror("erase");
> +		dfu->dfu_status = DFU_STATUS_errERASE;
> +		goto out;
> +	}
> +
> +	ret = copy_file(DFU_TEMPFILE, dfu_file_entry->filename, 0);
> +	if (ret) {
> +		dfu->dfu_status = DFU_STATUS_errWRITE;
> +		printf("copy file failed\n");
> +		goto out;
> +	}
> +
> +	dfu->dfu_state = DFU_STATE_dfuIDLE;
> +	dfu_cleanup(dfu);
> +
> +	return;
> +out:
> +	dfu->dfu_state = DFU_STATE_dfuERROR;
> +	dfu_cleanup(dfu);
> +}
>  
>  static int
>  dfu_bind(struct usb_configuration *c, struct usb_function *f)
> @@ -223,6 +397,10 @@ dfu_bind(struct usb_configuration *c, struct usb_function *f)
>  		goto out;
>  	}
>  
> +	dfu->wq.fn = dfu_do_work;
> +	dfu->wq.cancel = dfu_work_cancel;
> +	wq_register(&dfu->wq);
> +
>  	/* allocate instance-specific interface IDs, and patch descriptors */
>  	status = usb_interface_id(c, f);
>  	if (status < 0)
> @@ -278,6 +456,8 @@ dfu_unbind(struct usb_configuration *c, struct usb_function *f)
>  	dfu_file_entry = NULL;
>  	dfudetach = 0;
>  
> +	wq_unregister(&dfu->wq);
> +
>  	usb_free_all_descriptors(f);
>  
>  	dma_free(dfu->dnreq->buf);
> @@ -327,6 +507,9 @@ static void dfu_cleanup(struct f_dfu *dfu)
>  	dfu_erased = 0;
>  	prog_erase = 0;
>  
> +	/* TODO: Right now, close and stat operation can be called
> +	 * in a poller, in dfu_abort and dfu_disable. */
> +
>  	if (dfufd > 0) {
>  		close(dfufd);
>  		dfufd = -EINVAL;
> @@ -339,28 +522,15 @@ static void dfu_cleanup(struct f_dfu *dfu)
>  static void dn_complete(struct usb_ep *ep, struct usb_request *req)
>  {
>  	struct f_dfu		*dfu = req->context;
> -	loff_t size;
> -	int ret;
> +	struct dfu_work *dw;
>  
> -	if (prog_erase && (dfu_written + req->length) > dfu_erased) {
> -		size = roundup(req->length, dfu_mtdinfo.erasesize);
> -		ret = erase(dfufd, size, dfu_erased);
> -		dfu_erased += size;
> -		if (ret && ret != -ENOSYS) {
> -			perror("erase");
> -			dfu->dfu_status = DFU_STATUS_errERASE;
> -			dfu_cleanup(dfu);
> -			return;
> -		}
> -	}
> +	dw = xzalloc(sizeof(*dw));
> +	dw->dfu = dfu;
> +	dw->task = dfu_do_write;
> +	dw->len = min_t(unsigned int, req->length, CONFIG_USBD_DFU_XFER_SIZE);
> +	memcpy(dw->wbuf, req->buf, dw->len);
>  
> -	dfu_written += req->length;
> -	ret = write(dfufd, req->buf, req->length);
> -	if (ret < (int)req->length) {
> -		perror("write");
> -		dfu->dfu_status = DFU_STATUS_errWRITE;
> -		dfu_cleanup(dfu);
> -	}
> +	wq_queue_work(&dfu->wq, &dw->work);
>  }
>  
>  static int handle_dnload(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
> @@ -370,12 +540,7 @@ static int handle_dnload(struct usb_function *f, const struct usb_ctrlrequest *c
>  	u16			w_length = le16_to_cpu(ctrl->wLength);
>  
>  	if (w_length == 0) {
> -		if (dfu_file_entry->flags & FILE_LIST_FLAG_SAFE) {
> -			dfu->dfu_state = DFU_STATE_dfuMANIFEST;
> -		} else {
> -			dfu->dfu_state = DFU_STATE_dfuIDLE;
> -			dfu_cleanup(dfu);
> -		}
> +		dfu->dfu_state = DFU_STATE_dfuMANIFEST_SYNC;
>  		return 0;
>  	}
>  
> @@ -389,48 +554,18 @@ static int handle_dnload(struct usb_function *f, const struct usb_ctrlrequest *c
>  static int handle_manifest(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
>  {
>  	struct f_dfu		*dfu = func_to_dfu(f);
> -	int ret;
> +	struct dfu_work *dw;
>  
>  	dfu->dfu_state = DFU_STATE_dfuIDLE;
>  
>  	if (dfu_file_entry->flags & FILE_LIST_FLAG_SAFE) {
> -		int fd;
> -		unsigned flags = O_WRONLY;
> -
> -		if (dfu_file_entry->flags & FILE_LIST_FLAG_CREATE)
> -			flags |= O_CREAT | O_TRUNC;
> -
> -		fd = open(dfu_file_entry->filename, flags);
> -		if (fd < 0) {
> -			perror("open");
> -			dfu->dfu_status = DFU_STATUS_errERASE;
> -			ret = -EINVAL;
> -			goto out;
> -		}
> -
> -		ret = erase(fd, ERASE_SIZE_ALL, 0);
> -		close(fd);
> -		if (ret && ret != -ENOSYS) {
> -			dfu->dfu_status = DFU_STATUS_errERASE;
> -			perror("erase");
> -			goto out;
> -		}
> -
> -		ret = copy_file(DFU_TEMPFILE, dfu_file_entry->filename, 0);
> -		if (ret) {
> -			printf("copy file failed\n");
> -			ret = -EINVAL;
> -			goto out;
> -		}
> +		dw = xzalloc(sizeof(*dw));
> +		dw->dfu = dfu;
> +		dw->task = dfu_do_copy;
> +		wq_queue_work(&dfu->wq, &dw->work);
>  	}
>  
>  	return 0;
> -
> -out:
> -	dfu->dfu_status = DFU_STATUS_errWRITE;
> -	dfu->dfu_state = DFU_STATE_dfuERROR;
> -	dfu_cleanup(dfu);
> -	return ret;
>  }
>  
>  static void up_complete(struct usb_ep *ep, struct usb_request *req)
> @@ -440,20 +575,17 @@ static void up_complete(struct usb_ep *ep, struct usb_request *req)
>  static int handle_upload(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
>  {
>  	struct f_dfu		*dfu = func_to_dfu(f);
> -	struct usb_composite_dev *cdev = f->config->cdev;
> +	struct dfu_work *dw;
>  	u16			w_length = le16_to_cpu(ctrl->wLength);
> -	int len;
> -
> -	len = read(dfufd, dfu->dnreq->buf, w_length);
> -
> -	dfu->dnreq->length = len;
> -	if (len < w_length) {
> -		dfu_cleanup(dfu);
> -		dfu->dfu_state = DFU_STATE_dfuIDLE;
> -	}
>  
> -	dfu->dnreq->complete = up_complete;
> -	usb_ep_queue(cdev->gadget->ep0, dfu->dnreq);
> +	/* RFC: I didn't found a better way to queue the usb response other
> +	 * than making dfu_do_read call usb_ep_queue after reading from file */
> +	dw = xzalloc(sizeof(*dw));
> +	dw->dfu = dfu;
> +	dw->task = dfu_do_read;
> +	dw->len = w_length;
> +	dw->rbuf = dfu->dnreq->buf;
> +	wq_queue_work(&dfu->wq, &dw->work);
>  
>  	return 0;
>  }
> @@ -474,7 +606,7 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
>  	int			value = -EOPNOTSUPP;
>  	int			w_length = le16_to_cpu(ctrl->wLength);
>  	int			w_value = le16_to_cpu(ctrl->wValue);
> -	int ret;
> +	struct dfu_work *dw;
>  
>  	if (ctrl->bRequestType == USB_DIR_IN && ctrl->bRequest == USB_REQ_GET_DESCRIPTOR
>  			&& (w_value >> 8) == 0x21) {
> @@ -501,28 +633,10 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
>  				goto out;
>  			}
>  			debug("dfu: starting download to %s\n", dfu_file_entry->filename);
> -			if (dfu_file_entry->flags & FILE_LIST_FLAG_SAFE) {
> -				dfufd = open(DFU_TEMPFILE, O_WRONLY | O_CREAT);
> -			} else {
> -				unsigned flags = O_WRONLY;
> -
> -				if (dfu_file_entry->flags & FILE_LIST_FLAG_CREATE)
> -					flags |= O_CREAT | O_TRUNC;
> -
> -				dfufd = open(dfu_file_entry->filename, flags);
> -			}
> -
> -			if (dfufd < 0) {
> -				dfu->dfu_state = DFU_STATE_dfuERROR;
> -				perror("open");
> -				goto out;
> -			}
> -
> -			if (!(dfu_file_entry->flags & FILE_LIST_FLAG_SAFE)) {
> -				ret = ioctl(dfufd, MEMGETINFO, &dfu_mtdinfo);
> -				if (!ret) /* file is on a mtd device */
> -					prog_erase = 1;
> -			}
> +			dw = xzalloc(sizeof(*dw));
> +			dw->dfu = dfu;
> +			dw->task = dfu_do_open_dnload;
> +			wq_queue_work(&dfu->wq, &dw->work);
>  
>  			value = handle_dnload(f, ctrl);
>  			dfu->dfu_state = DFU_STATE_dfuDNLOAD_IDLE;
> @@ -534,12 +648,12 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
>  				dfu->dfu_state = DFU_STATE_dfuERROR;
>  				goto out;
>  			}
> -			dfufd = open(dfu_file_entry->filename, O_RDONLY);
> -			if (dfufd < 0) {
> -				dfu->dfu_state = DFU_STATE_dfuERROR;
> -				perror("open");
> -				goto out;
> -			}
> +
> +			dw = xzalloc(sizeof(*dw));
> +			dw->dfu = dfu;
> +			dw->task = dfu_do_open_upload;
> +			wq_queue_work(&dfu->wq, &dw->work);
> +
>  			handle_upload(f, ctrl);
>  			return 0;
>  		case USB_REQ_DFU_ABORT:
> @@ -648,9 +762,6 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
>  		break;
>  	case DFU_STATE_dfuMANIFEST:
>  		value = handle_manifest(f, ctrl);
> -		if (dfu->dfu_state != DFU_STATE_dfuIDLE) {
> -			return 0;
> -		}
>  		switch (ctrl->bRequest) {
>  		case USB_REQ_DFU_GETSTATUS:
>  			value = dfu_status(f, ctrl);
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC PATCH] usb: gadget: dfu: Wrap fs operation in workqueue

[Jules Maselbas]

Hi Ahmad,

On Fri, Jan 29, 2021 at 10:51:18AM +0100, Ahmad Fatoum wrote:
> Hello Jules,
> 
> On 27.01.21 17:49, Jules Maselbas wrote:
> > File system operation shouldn't be executed in a poller. Use
> > a workqueue to delay filesystem operation to command context.
> > 
> > This is an RFC, extra work must be done to properly handle error
> > cases and dfu cleanup.
> 
> I erroneously thought the poller is within the DFU bits. I wonder what
> side-effect moving the whole USB gadget polling into a workqueue would
> have. In that case, we wouldn't need to any changes for DFU itself.
> 
If I understood correctly you're suggesting to wrap the entire dfu
gadget inside a poller. I have not tried this and it might work.
However wrapping each fs operation allow the dfu gadget to respond
to GET_STATUS queries will erase/write operation are on-going.

Jules


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC PATCH] usb: gadget: dfu: Wrap fs operation in workqueue

[Ahmad Fatoum]

Hello Jules,

On 29.01.21 11:51, Jules Maselbas wrote:
> Hi Ahmad,
> 
> On Fri, Jan 29, 2021 at 10:51:18AM +0100, Ahmad Fatoum wrote:
>> Hello Jules,
>>
>> On 27.01.21 17:49, Jules Maselbas wrote:
>>> File system operation shouldn't be executed in a poller. Use
>>> a workqueue to delay filesystem operation to command context.
>>>
>>> This is an RFC, extra work must be done to properly handle error
>>> cases and dfu cleanup.
>>
>> I erroneously thought the poller is within the DFU bits. I wonder what
>> side-effect moving the whole USB gadget polling into a workqueue would
>> have. In that case, we wouldn't need to any changes for DFU itself.
>>
> If I understood correctly you're suggesting to wrap the entire dfu
> gadget inside a poller. I have not tried this and it might work.
> However wrapping each fs operation allow the dfu gadget to respond
> to GET_STATUS queries will erase/write operation are on-going.

To allow running in the "background", the UDC calls into the gadget code
in a poller. I was wondering if replacing that poller with a workqueue
would just make the problem go away, not for DFU. It now know it wouldn't:
Work queues work in shell context and doing console stuff in shell context
(via ttyACM gadget) is not a wise choice. So please dismiss my suggestion ^^.

> 
> Jules
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC PATCH] usb: gadget: dfu: Wrap fs operation in workqueue

[Sascha Hauer]

On Fri, Jan 29, 2021 at 10:51:18AM +0100, Ahmad Fatoum wrote:
> Hello Jules,
> 
> On 27.01.21 17:49, Jules Maselbas wrote:
> > File system operation shouldn't be executed in a poller. Use
> > a workqueue to delay filesystem operation to command context.
> > 
> > This is an RFC, extra work must be done to properly handle error
> > cases and dfu cleanup.
> 
> I erroneously thought the poller is within the DFU bits. I wonder what
> side-effect moving the whole USB gadget polling into a workqueue would
> have. In that case, we wouldn't need to any changes for DFU itself.

We have serial gadget support. One of the side effects likely would be
that the console no longer properly works.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox