[PATCH 1/3] usb: gadget: dfu: Use func_to_dfu

[PATCH 1/3] usb: gadget: dfu: Use func_to_dfu

[Jules Maselbas]

Replace the uses of container_of with func_to_dfu when available.

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

diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
index 7e23fa315..9d6a9d252 100644
--- a/drivers/usb/gadget/dfu.c
+++ b/drivers/usb/gadget/dfu.c
@@ -181,7 +181,7 @@ dfu_bind(struct usb_configuration *c, struct usb_function *f)
 	struct usb_descriptor_header **header;
 	struct usb_interface_descriptor *desc;
 	struct file_list_entry *fentry;
-	struct f_dfu *dfu = container_of(f, struct f_dfu, func);
+	struct f_dfu *dfu = func_to_dfu(f);
 	int i;
 	int			status;
 	struct usb_string	*us;
@@ -863,7 +863,7 @@ out:
 
 static void dfu_free_func(struct usb_function *f)
 {
-	struct f_dfu *dfu = container_of(f, struct f_dfu, func);
+	struct f_dfu *dfu = func_to_dfu(f);
 
 	free(dfu);
 }
-- 
2.17.1



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

[PATCH 2/3] 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.

---
change RFC -> v1:
 - Rework manifestation phase, copy work is directly triggered
   when entering the manifestation state
 - Rework cleanup, it is now done when either exiting the ERROR
   state or when calling dfu_abort or dfu_disable.
 - Rework error handling when reading from file

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

diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
index 9d6a9d252..95e8da82e 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,191 @@ 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);
+	struct f_dfu *dfu = dw->dfu;
+
+	if (dfu->dfu_state != DFU_STATE_dfuERROR && dfu->dfu_status == DFU_STATUS_OK)
+		dw->task(dw);
+	else
+		debug("skip work\n");
+
+	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;
+	ssize_t size, wlen = dw->len;
+	ssize_t 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_state = DFU_STATE_dfuERROR;
+			dfu->dfu_status = DFU_STATUS_errERASE;
+			return;
+		}
+	}
+
+	dfu_written += wlen;
+	ret = write(dfufd, dw->wbuf, wlen);
+	if (ret < wlen) {
+		perror("write");
+		dfu->dfu_state = DFU_STATE_dfuERROR;
+		dfu->dfu_status = DFU_STATUS_errWRITE;
+	}
+}
+
+static void dfu_do_read(struct dfu_work *dw)
+{
+	struct f_dfu *dfu = dw->dfu;
+	struct usb_composite_dev *cdev = dfu->func.config->cdev;
+	ssize_t size, rlen = dw->len;
+
+	debug("do read\n");
+
+	size = read(dfufd, dfu->dnreq->buf, rlen);
+	dfu->dnreq->length = size;
+	if (size < 0) {
+		perror("read");
+		dfu->dnreq->length = 0;
+		dfu->dfu_state = DFU_STATE_dfuERROR;
+		dfu->dfu_status = DFU_STATUS_errFILE;
+	} else if (size < rlen) {
+		/* this is the last chunk, go to IDLE and close file */
+		dfu_cleanup(dfu);
+	}
+
+	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_state = DFU_STATE_dfuERROR;
+		dfu->dfu_status = DFU_STATUS_errFILE;
+		return;
+	}
+
+	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;
+	}
+}
+
+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_state = DFU_STATE_dfuERROR;
+		dfu->dfu_status = DFU_STATUS_errFILE;
+	}
+}
+
+static void dfu_do_close(struct dfu_work *dw)
+{
+	struct stat s;
+
+	debug("do close\n");
+
+	if (dfufd > 0) {
+		close(dfufd);
+		dfufd = -EINVAL;
+	}
+
+	if (!stat(DFU_TEMPFILE, &s))
+		unlink(DFU_TEMPFILE);
+}
+
+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_state = DFU_STATE_dfuERROR;
+		dfu->dfu_status = DFU_STATUS_errERASE;
+		return;
+	}
+
+	ret = erase(fd, ERASE_SIZE_ALL, 0);
+	close(fd);
+	if (ret && ret != -ENOSYS) {
+		perror("erase");
+		dfu->dfu_state = DFU_STATE_dfuERROR;
+		dfu->dfu_status = DFU_STATUS_errERASE;
+		return;
+	}
+
+	ret = copy_file(DFU_TEMPFILE, dfu_file_entry->filename, 0);
+	if (ret) {
+		printf("copy file failed\n");
+		dfu->dfu_state = DFU_STATE_dfuERROR;
+		dfu->dfu_status = DFU_STATUS_errWRITE;
+		return;
+	}
+
+	dfu->dfu_state = DFU_STATE_dfuIDLE;
+	dfu_do_close(dw);
+}
 
 static int
 dfu_bind(struct usb_configuration *c, struct usb_function *f)
@@ -223,6 +410,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 +469,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);
@@ -320,47 +513,50 @@ static int dfu_status(struct usb_function *f, const struct usb_ctrlrequest *ctrl
 
 static void dfu_cleanup(struct f_dfu *dfu)
 {
-	struct stat s;
+	struct dfu_work *dw;
+
+	debug("dfu cleanup\n");
 
 	memset(&dfu_mtdinfo, 0, sizeof(dfu_mtdinfo));
 	dfu_written = 0;
 	dfu_erased = 0;
 	prog_erase = 0;
 
-	if (dfufd > 0) {
-		close(dfufd);
-		dfufd = -EINVAL;
-	}
+	dfu->dfu_state = DFU_STATE_dfuIDLE;
+	dfu->dfu_status = DFU_STATUS_OK;
 
-	if (!stat(DFU_TEMPFILE, &s))
-		unlink(DFU_TEMPFILE);
+	dw = xzalloc(sizeof(*dw));
+	dw->dfu = dfu;
+	dw->task = dfu_do_close;
+	wq_queue_work(&dfu->wq, &dw->work);
 }
 
 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;
+
+	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);
+	wq_queue_work(&dfu->wq, &dw->work);
+}
 
-	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;
-		}
-	}
+static int handle_manifest(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
+{
+	struct f_dfu		*dfu = func_to_dfu(f);
+	struct dfu_work *dw;
 
-	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);
+	if (dfu_file_entry->flags & FILE_LIST_FLAG_SAFE) {
+		dw = xzalloc(sizeof(*dw));
+		dw->dfu = dfu;
+		dw->task = dfu_do_copy;
+		wq_queue_work(&dfu->wq, &dw->work);
 	}
+
+	return 0;
 }
 
 static int handle_dnload(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
@@ -370,12 +566,8 @@ 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);
-		}
+		handle_manifest(f, ctrl);
+		dfu->dfu_state = DFU_STATE_dfuMANIFEST;
 		return 0;
 	}
 
@@ -386,53 +578,6 @@ static int handle_dnload(struct usb_function *f, const struct usb_ctrlrequest *c
 	return 0;
 }
 
-static int handle_manifest(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
-{
-	struct f_dfu		*dfu = func_to_dfu(f);
-	int ret;
-
-	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;
-		}
-	}
-
-	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,28 +585,22 @@ 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);
+	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;
 }
 
 static void dfu_abort(struct f_dfu *dfu)
 {
-	dfu->dfu_state = DFU_STATE_dfuIDLE;
-	dfu->dfu_status = DFU_STATUS_OK;
+	wq_cancel_work(&dfu->wq);
 
 	dfu_cleanup(dfu);
 }
@@ -474,7 +613,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 +640,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 +655,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:
@@ -606,6 +727,7 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 		}
 		break;
 	case DFU_STATE_dfuERROR:
+		wq_cancel_work(&dfu->wq);
 		switch (ctrl->bRequest) {
 		case USB_REQ_DFU_GETSTATUS:
 			value = dfu_status(f, ctrl);
@@ -647,10 +769,7 @@ 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;
-		}
+		dfu->dfu_state = DFU_STATE_dfuMANIFEST_SYNC;
 		switch (ctrl->bRequest) {
 		case USB_REQ_DFU_GETSTATUS:
 			value = dfu_status(f, ctrl);
@@ -692,9 +811,7 @@ static void dfu_disable(struct usb_function *f)
 {
 	struct f_dfu		*dfu = func_to_dfu(f);
 
-	dfu->dfu_state = DFU_STATE_dfuIDLE;
-
-	dfu_cleanup(dfu);
+	dfu_abort(dfu);
 }
 
 #define STRING_MANUFACTURER_IDX		0
-- 
2.17.1



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

[PATCH 3/3] usb: gadget: dfu: Replace printf with pr_err

[Jules Maselbas]

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

diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
index 95e8da82e..a28e7c9e8 100644
--- a/drivers/usb/gadget/dfu.c
+++ b/drivers/usb/gadget/dfu.c
@@ -351,7 +351,7 @@ static void dfu_do_copy(struct dfu_work *dw)
 
 	ret = copy_file(DFU_TEMPFILE, dfu_file_entry->filename, 0);
 	if (ret) {
-		printf("copy file failed\n");
+		pr_err("dfu: copy file failed\n");
 		dfu->dfu_state = DFU_STATE_dfuERROR;
 		dfu->dfu_status = DFU_STATUS_errWRITE;
 		return;
@@ -396,7 +396,7 @@ dfu_bind(struct usb_configuration *c, struct usb_function *f)
 
 	dfu->dnreq = usb_ep_alloc_request(c->cdev->gadget->ep0);
 	if (!dfu->dnreq) {
-		printf("usb_ep_alloc_request failed\n");
+		pr_err("dfu: usb_ep_alloc_request failed\n");
 		status = -ENOMEM;
 		goto out;
 	}
@@ -445,7 +445,7 @@ dfu_bind(struct usb_configuration *c, struct usb_function *f)
 
 	i = 0;
 	file_list_for_each_entry(dfu_files, fentry) {
-		printf("dfu: register alt%d(%s) with device %s\n",
+		pr_err("dfu: register alt%d(%s) with device %s\n",
 				i, fentry->name, fentry->filename);
 		i++;
 	}
-- 
2.17.1



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

Re: [PATCH 1/3] usb: gadget: dfu: Use func_to_dfu

[Sascha Hauer]

On Mon, Feb 08, 2021 at 05:34:56PM +0100, Jules Maselbas wrote:
> Replace the uses of container_of with func_to_dfu when available.
> 
> Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
> ---
>  drivers/usb/gadget/dfu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied, thanks

Sascha

> 
> diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
> index 7e23fa315..9d6a9d252 100644
> --- a/drivers/usb/gadget/dfu.c
> +++ b/drivers/usb/gadget/dfu.c
> @@ -181,7 +181,7 @@ dfu_bind(struct usb_configuration *c, struct usb_function *f)
>  	struct usb_descriptor_header **header;
>  	struct usb_interface_descriptor *desc;
>  	struct file_list_entry *fentry;
> -	struct f_dfu *dfu = container_of(f, struct f_dfu, func);
> +	struct f_dfu *dfu = func_to_dfu(f);
>  	int i;
>  	int			status;
>  	struct usb_string	*us;
> @@ -863,7 +863,7 @@ out:
>  
>  static void dfu_free_func(struct usb_function *f)
>  {
> -	struct f_dfu *dfu = container_of(f, struct f_dfu, func);
> +	struct f_dfu *dfu = func_to_dfu(f);
>  
>  	free(dfu);
>  }
> -- 
> 2.17.1
> 
> 
> 

-- 
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: [PATCH 3/3] usb: gadget: dfu: Replace printf with pr_err

[Ahmad Fatoum]

Hello Jules,

On 08.02.21 17:34, Jules Maselbas wrote:
> Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
> ---
>  drivers/usb/gadget/dfu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
> index 95e8da82e..a28e7c9e8 100644
> --- a/drivers/usb/gadget/dfu.c
> +++ b/drivers/usb/gadget/dfu.c
> @@ -351,7 +351,7 @@ static void dfu_do_copy(struct dfu_work *dw)
>  
>  	ret = copy_file(DFU_TEMPFILE, dfu_file_entry->filename, 0);
>  	if (ret) {
> -		printf("copy file failed\n");
> +		pr_err("dfu: copy file failed\n");

Like with Linux, you could define pr_fmt(fmt) "dfu: " fmt before including
any headers and that prefix will be applied for all pr_ in that file.

>  		dfu->dfu_state = DFU_STATE_dfuERROR;
>  		dfu->dfu_status = DFU_STATUS_errWRITE;
>  		return;
> @@ -396,7 +396,7 @@ dfu_bind(struct usb_configuration *c, struct usb_function *f)
>  
>  	dfu->dnreq = usb_ep_alloc_request(c->cdev->gadget->ep0);
>  	if (!dfu->dnreq) {
> -		printf("usb_ep_alloc_request failed\n");
> +		pr_err("dfu: usb_ep_alloc_request failed\n");
>  		status = -ENOMEM;
>  		goto out;
>  	}
> @@ -445,7 +445,7 @@ dfu_bind(struct usb_configuration *c, struct usb_function *f)
>  
>  	i = 0;
>  	file_list_for_each_entry(dfu_files, fentry) {
> -		printf("dfu: register alt%d(%s) with device %s\n",
> +		pr_err("dfu: register alt%d(%s) with device %s\n",
>  				i, fentry->name, fentry->filename);
>  		i++;
>  	}
> 

-- 
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

[PATCH v2 1/2] usb: gadget: dfu: Rework print messages

[Jules Maselbas]

Replace printf with pr_err and debug with pr_debug.
Defines "dfu :" as a prefix for formatted prints.

---
v1 -> v2:
 - add `pr_fmt(fmt) "dfu: " fmt`
 - change debug call to pr_debug
 - remove "dfu: " in print messages

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

diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
index 9d6a9d252..fba4ad782 100644
--- a/drivers/usb/gadget/dfu.c
+++ b/drivers/usb/gadget/dfu.c
@@ -32,6 +32,7 @@
  *   checking?
  * - make 'dnstate' attached to 'struct usb_device_instance'
  */
+#define pr_fmt(fmt) "dfu: " fmt
 
 #include <dma.h>
 #include <asm/byteorder.h>
@@ -209,7 +210,7 @@ dfu_bind(struct usb_configuration *c, struct usb_function *f)
 
 	dfu->dnreq = usb_ep_alloc_request(c->cdev->gadget->ep0);
 	if (!dfu->dnreq) {
-		printf("usb_ep_alloc_request failed\n");
+		pr_err("dfu: usb_ep_alloc_request failed\n");
 		status = -ENOMEM;
 		goto out;
 	}
@@ -254,7 +255,7 @@ dfu_bind(struct usb_configuration *c, struct usb_function *f)
 
 	i = 0;
 	file_list_for_each_entry(dfu_files, fentry) {
-		printf("dfu: register alt%d(%s) with device %s\n",
+		pr_err("register alt%d(%s) with device %s\n",
 				i, fentry->name, fentry->filename);
 		i++;
 	}
@@ -418,7 +419,7 @@ static int handle_manifest(struct usb_function *f, const struct usb_ctrlrequest
 
 		ret = copy_file(DFU_TEMPFILE, dfu_file_entry->filename, 0);
 		if (ret) {
-			printf("copy file failed\n");
+			pr_err("copy file failed\n");
 			ret = -EINVAL;
 			goto out;
 		}
@@ -500,7 +501,7 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 				value = -EINVAL;
 				goto out;
 			}
-			debug("dfu: starting download to %s\n", dfu_file_entry->filename);
+			pr_debug("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 {
@@ -529,7 +530,7 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 			return 0;
 		case USB_REQ_DFU_UPLOAD:
 			dfu->dfu_state = DFU_STATE_dfuUPLOAD_IDLE;
-			debug("dfu: starting upload from %s\n", dfu_file_entry->filename);
+			pr_debug("starting upload from %s\n", dfu_file_entry->filename);
 			if (!(dfu_file_entry->flags & FILE_LIST_FLAG_READBACK)) {
 				dfu->dfu_state = DFU_STATE_dfuERROR;
 				goto out;
-- 
2.17.1



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

[PATCH v2 2/2] 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.

---
change RFC -> v1:
 - Rework manifestation phase, copy work is directly triggered
   when entering the manifestation state
 - Rework cleanup, it is now done when either exiting the ERROR
   state or when calling dfu_abort or dfu_disable.
 - Rework error handling when reading from file
change v1 -> v2:
 - use pr_debug instead of debug

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

diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
index fba4ad782..82c9dc030 100644
--- a/drivers/usb/gadget/dfu.c
+++ b/drivers/usb/gadget/dfu.c
@@ -55,6 +55,7 @@
 #include <fs.h>
 #include <ioctl.h>
 #include <linux/mtd/mtd-abi.h>
+#include <work.h>
 
 #define USB_DT_DFU			0x21
 
@@ -154,6 +155,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)
@@ -174,6 +176,191 @@ 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);
+	struct f_dfu *dfu = dw->dfu;
+
+	if (dfu->dfu_state != DFU_STATE_dfuERROR && dfu->dfu_status == DFU_STATUS_OK)
+		dw->task(dw);
+	else
+		pr_debug("skip work\n");
+
+	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;
+	ssize_t size, wlen = dw->len;
+	ssize_t ret;
+
+	pr_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_state = DFU_STATE_dfuERROR;
+			dfu->dfu_status = DFU_STATUS_errERASE;
+			return;
+		}
+	}
+
+	dfu_written += wlen;
+	ret = write(dfufd, dw->wbuf, wlen);
+	if (ret < wlen) {
+		perror("write");
+		dfu->dfu_state = DFU_STATE_dfuERROR;
+		dfu->dfu_status = DFU_STATUS_errWRITE;
+	}
+}
+
+static void dfu_do_read(struct dfu_work *dw)
+{
+	struct f_dfu *dfu = dw->dfu;
+	struct usb_composite_dev *cdev = dfu->func.config->cdev;
+	ssize_t size, rlen = dw->len;
+
+	pr_debug("do read\n");
+
+	size = read(dfufd, dfu->dnreq->buf, rlen);
+	dfu->dnreq->length = size;
+	if (size < 0) {
+		perror("read");
+		dfu->dnreq->length = 0;
+		dfu->dfu_state = DFU_STATE_dfuERROR;
+		dfu->dfu_status = DFU_STATUS_errFILE;
+	} else if (size < rlen) {
+		/* this is the last chunk, go to IDLE and close file */
+		dfu_cleanup(dfu);
+	}
+
+	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;
+
+	pr_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_state = DFU_STATE_dfuERROR;
+		dfu->dfu_status = DFU_STATUS_errFILE;
+		return;
+	}
+
+	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;
+	}
+}
+
+static void dfu_do_open_upload(struct dfu_work *dw)
+{
+	struct f_dfu *dfu = dw->dfu;
+
+	pr_debug("do open upload\n");
+
+	dfufd = open(dfu_file_entry->filename, O_RDONLY);
+	if (dfufd < 0) {
+		perror("open");
+		dfu->dfu_state = DFU_STATE_dfuERROR;
+		dfu->dfu_status = DFU_STATUS_errFILE;
+	}
+}
+
+static void dfu_do_close(struct dfu_work *dw)
+{
+	struct stat s;
+
+	pr_debug("do close\n");
+
+	if (dfufd > 0) {
+		close(dfufd);
+		dfufd = -EINVAL;
+	}
+
+	if (!stat(DFU_TEMPFILE, &s))
+		unlink(DFU_TEMPFILE);
+}
+
+static void dfu_do_copy(struct dfu_work *dw)
+{
+	struct f_dfu *dfu = dw->dfu;
+	unsigned flags = O_WRONLY;
+	int ret, fd;
+
+	pr_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_state = DFU_STATE_dfuERROR;
+		dfu->dfu_status = DFU_STATUS_errERASE;
+		return;
+	}
+
+	ret = erase(fd, ERASE_SIZE_ALL, 0);
+	close(fd);
+	if (ret && ret != -ENOSYS) {
+		perror("erase");
+		dfu->dfu_state = DFU_STATE_dfuERROR;
+		dfu->dfu_status = DFU_STATUS_errERASE;
+		return;
+	}
+
+	ret = copy_file(DFU_TEMPFILE, dfu_file_entry->filename, 0);
+	if (ret) {
+		pr_err("copy file failed\n");
+		dfu->dfu_state = DFU_STATE_dfuERROR;
+		dfu->dfu_status = DFU_STATUS_errWRITE;
+		return;
+	}
+
+	dfu->dfu_state = DFU_STATE_dfuIDLE;
+	dfu_do_close(dw);
+}
 
 static int
 dfu_bind(struct usb_configuration *c, struct usb_function *f)
@@ -224,6 +411,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)
@@ -279,6 +470,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);
@@ -321,47 +514,50 @@ static int dfu_status(struct usb_function *f, const struct usb_ctrlrequest *ctrl
 
 static void dfu_cleanup(struct f_dfu *dfu)
 {
-	struct stat s;
+	struct dfu_work *dw;
+
+	pr_debug("dfu cleanup\n");
 
 	memset(&dfu_mtdinfo, 0, sizeof(dfu_mtdinfo));
 	dfu_written = 0;
 	dfu_erased = 0;
 	prog_erase = 0;
 
-	if (dfufd > 0) {
-		close(dfufd);
-		dfufd = -EINVAL;
-	}
+	dfu->dfu_state = DFU_STATE_dfuIDLE;
+	dfu->dfu_status = DFU_STATUS_OK;
 
-	if (!stat(DFU_TEMPFILE, &s))
-		unlink(DFU_TEMPFILE);
+	dw = xzalloc(sizeof(*dw));
+	dw->dfu = dfu;
+	dw->task = dfu_do_close;
+	wq_queue_work(&dfu->wq, &dw->work);
 }
 
 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;
+
+	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);
+	wq_queue_work(&dfu->wq, &dw->work);
+}
 
-	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;
-		}
-	}
+static int handle_manifest(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
+{
+	struct f_dfu		*dfu = func_to_dfu(f);
+	struct dfu_work *dw;
 
-	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);
+	if (dfu_file_entry->flags & FILE_LIST_FLAG_SAFE) {
+		dw = xzalloc(sizeof(*dw));
+		dw->dfu = dfu;
+		dw->task = dfu_do_copy;
+		wq_queue_work(&dfu->wq, &dw->work);
 	}
+
+	return 0;
 }
 
 static int handle_dnload(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
@@ -371,12 +567,8 @@ 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);
-		}
+		handle_manifest(f, ctrl);
+		dfu->dfu_state = DFU_STATE_dfuMANIFEST;
 		return 0;
 	}
 
@@ -387,53 +579,6 @@ static int handle_dnload(struct usb_function *f, const struct usb_ctrlrequest *c
 	return 0;
 }
 
-static int handle_manifest(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
-{
-	struct f_dfu		*dfu = func_to_dfu(f);
-	int ret;
-
-	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) {
-			pr_err("copy file failed\n");
-			ret = -EINVAL;
-			goto out;
-		}
-	}
-
-	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)
 {
 }
@@ -441,28 +586,22 @@ 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);
+	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;
 }
 
 static void dfu_abort(struct f_dfu *dfu)
 {
-	dfu->dfu_state = DFU_STATE_dfuIDLE;
-	dfu->dfu_status = DFU_STATUS_OK;
+	wq_cancel_work(&dfu->wq);
 
 	dfu_cleanup(dfu);
 }
@@ -475,7 +614,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) {
@@ -502,28 +641,10 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 				goto out;
 			}
 			pr_debug("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;
@@ -535,12 +656,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:
@@ -607,6 +728,7 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 		}
 		break;
 	case DFU_STATE_dfuERROR:
+		wq_cancel_work(&dfu->wq);
 		switch (ctrl->bRequest) {
 		case USB_REQ_DFU_GETSTATUS:
 			value = dfu_status(f, ctrl);
@@ -648,10 +770,7 @@ 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;
-		}
+		dfu->dfu_state = DFU_STATE_dfuMANIFEST_SYNC;
 		switch (ctrl->bRequest) {
 		case USB_REQ_DFU_GETSTATUS:
 			value = dfu_status(f, ctrl);
@@ -693,9 +812,7 @@ static void dfu_disable(struct usb_function *f)
 {
 	struct f_dfu		*dfu = func_to_dfu(f);
 
-	dfu->dfu_state = DFU_STATE_dfuIDLE;
-
-	dfu_cleanup(dfu);
+	dfu_abort(dfu);
 }
 
 #define STRING_MANUFACTURER_IDX		0
-- 
2.17.1



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

Re: [PATCH v2 1/2] usb: gadget: dfu: Rework print messages

[Bartosz Bilas]

Hello Jules,

On 15.02.2021 15:18, Jules Maselbas wrote:
> Replace printf with pr_err and debug with pr_debug.
> Defines "dfu :" as a prefix for formatted prints.
>
> ---
> v1 -> v2:
>   - add `pr_fmt(fmt) "dfu: " fmt`
>   - change debug call to pr_debug
>   - remove "dfu: " in print messages
>
> Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
> ---
>   drivers/usb/gadget/dfu.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
> index 9d6a9d252..fba4ad782 100644
> --- a/drivers/usb/gadget/dfu.c
> +++ b/drivers/usb/gadget/dfu.c
> @@ -32,6 +32,7 @@
>    *   checking?
>    * - make 'dnstate' attached to 'struct usb_device_instance'
>    */
> +#define pr_fmt(fmt) "dfu: " fmt
>   
>   #include <dma.h>
>   #include <asm/byteorder.h>
> @@ -209,7 +210,7 @@ dfu_bind(struct usb_configuration *c, struct usb_function *f)
>   
>   	dfu->dnreq = usb_ep_alloc_request(c->cdev->gadget->ep0);
>   	if (!dfu->dnreq) {
> -		printf("usb_ep_alloc_request failed\n");
> +		pr_err("dfu: usb_ep_alloc_request failed\n");

you have missed this one ^^^^^


Best
Bartek
>   		status = -ENOMEM;
>   		goto out;
>   	}
> @@ -254,7 +255,7 @@ dfu_bind(struct usb_configuration *c, struct usb_function *f)
>   
>   	i = 0;
>   	file_list_for_each_entry(dfu_files, fentry) {
> -		printf("dfu: register alt%d(%s) with device %s\n",
> +		pr_err("register alt%d(%s) with device %s\n",
>   				i, fentry->name, fentry->filename);
>   		i++;
>   	}
> @@ -418,7 +419,7 @@ static int handle_manifest(struct usb_function *f, const struct usb_ctrlrequest
>   
>   		ret = copy_file(DFU_TEMPFILE, dfu_file_entry->filename, 0);
>   		if (ret) {
> -			printf("copy file failed\n");
> +			pr_err("copy file failed\n");
>   			ret = -EINVAL;
>   			goto out;
>   		}
> @@ -500,7 +501,7 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
>   				value = -EINVAL;
>   				goto out;
>   			}
> -			debug("dfu: starting download to %s\n", dfu_file_entry->filename);
> +			pr_debug("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 {
> @@ -529,7 +530,7 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
>   			return 0;
>   		case USB_REQ_DFU_UPLOAD:
>   			dfu->dfu_state = DFU_STATE_dfuUPLOAD_IDLE;
> -			debug("dfu: starting upload from %s\n", dfu_file_entry->filename);
> +			pr_debug("starting upload from %s\n", dfu_file_entry->filename);
>   			if (!(dfu_file_entry->flags & FILE_LIST_FLAG_READBACK)) {
>   				dfu->dfu_state = DFU_STATE_dfuERROR;
>   				goto out;

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

Re: [PATCH v2 1/2] usb: gadget: dfu: Rework print messages

[Sascha Hauer]

On Tue, Feb 16, 2021 at 10:19:00AM +0100, Bartosz Bilas wrote:
> Hello Jules,
> 
> On 15.02.2021 15:18, Jules Maselbas wrote:
> > Replace printf with pr_err and debug with pr_debug.
> > Defines "dfu :" as a prefix for formatted prints.
> > 
> > ---
> > v1 -> v2:
> >   - add `pr_fmt(fmt) "dfu: " fmt`
> >   - change debug call to pr_debug
> >   - remove "dfu: " in print messages
> > 
> > Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
> > ---
> >   drivers/usb/gadget/dfu.c | 11 ++++++-----
> >   1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
> > index 9d6a9d252..fba4ad782 100644
> > --- a/drivers/usb/gadget/dfu.c
> > +++ b/drivers/usb/gadget/dfu.c
> > @@ -32,6 +32,7 @@
> >    *   checking?
> >    * - make 'dnstate' attached to 'struct usb_device_instance'
> >    */
> > +#define pr_fmt(fmt) "dfu: " fmt
> >   #include <dma.h>
> >   #include <asm/byteorder.h>
> > @@ -209,7 +210,7 @@ dfu_bind(struct usb_configuration *c, struct usb_function *f)
> >   	dfu->dnreq = usb_ep_alloc_request(c->cdev->gadget->ep0);
> >   	if (!dfu->dnreq) {
> > -		printf("usb_ep_alloc_request failed\n");
> > +		pr_err("dfu: usb_ep_alloc_request failed\n");
> 
> you have missed this one ^^^^^

Fixed while applying, thanks

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

Re: [PATCH v2 1/2] usb: gadget: dfu: Rework print messages

[Jules Maselbas]

Hi Bartek,

On Tue, Feb 16, 2021 at 10:19:00AM +0100, Bartosz Bilas wrote:
> > @@ -209,7 +210,7 @@ dfu_bind(struct usb_configuration *c, struct usb_function *f)
> >   	dfu->dnreq = usb_ep_alloc_request(c->cdev->gadget->ep0);
> >   	if (!dfu->dnreq) {
> > -		printf("usb_ep_alloc_request failed\n");
> > +		pr_err("dfu: usb_ep_alloc_request failed\n");
> 
> you have missed this one ^^^^^
> 
Thank you for catching this :]

Best


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

Re: [PATCH v2 1/2] usb: gadget: dfu: Rework print messages

[Jules Maselbas]

Hi Sascha,

On Tue, Feb 16, 2021 at 10:27:29AM +0100, Sascha Hauer wrote:
> > > @@ -209,7 +210,7 @@ dfu_bind(struct usb_configuration *c, struct usb_function *f)
> > >   	dfu->dnreq = usb_ep_alloc_request(c->cdev->gadget->ep0);
> > >   	if (!dfu->dnreq) {
> > > -		printf("usb_ep_alloc_request failed\n");
> > > +		pr_err("dfu: usb_ep_alloc_request failed\n");
> > 
> > you have missed this one ^^^^^
> 
> Fixed while applying, thanks
Thanks !


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

[PATCH 1/2] fixup! usb: gadget: dfu: Wrap fs operation in workqueue

[Jules Maselbas]

When not in mode "safe", do_close was never called.

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

diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
index 82c9dc030..3234f25b3 100644
--- a/drivers/usb/gadget/dfu.c
+++ b/drivers/usb/gadget/dfu.c
@@ -359,7 +359,6 @@ static void dfu_do_copy(struct dfu_work *dw)
 	}
 
 	dfu->dfu_state = DFU_STATE_dfuIDLE;
-	dfu_do_close(dw);
 }
 
 static int
@@ -557,6 +556,11 @@ static int handle_manifest(struct usb_function *f, const struct usb_ctrlrequest
 		wq_queue_work(&dfu->wq, &dw->work);
 	}
 
+	dw = xzalloc(sizeof(*dw));
+	dw->dfu = dfu;
+	dw->task = dfu_do_close;
+	wq_queue_work(&dfu->wq, &dw->work);
+
 	return 0;
 }
 
-- 
2.17.1



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

[PATCH 2/2] fixup! usb: gadget: dfu: Wrap fs operation in workqueue

[Jules Maselbas]

Simplify the state handle, the idle state will only be entered
from do_close.

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

diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
index 3234f25b3..1dc659953 100644
--- a/drivers/usb/gadget/dfu.c
+++ b/drivers/usb/gadget/dfu.c
@@ -320,6 +320,8 @@ static void dfu_do_close(struct dfu_work *dw)
 
 	if (!stat(DFU_TEMPFILE, &s))
 		unlink(DFU_TEMPFILE);
+
+	dw->dfu->dfu_state = DFU_STATE_dfuIDLE;
 }
 
 static void dfu_do_copy(struct dfu_work *dw)
@@ -756,11 +758,8 @@ static int dfu_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 	case DFU_STATE_dfuMANIFEST_SYNC:
 		switch (ctrl->bRequest) {
 		case USB_REQ_DFU_GETSTATUS:
+			dfu->dfu_state = DFU_STATE_dfuMANIFEST;
 			value = dfu_status(f, ctrl);
-			if (dfu_file_entry->flags & FILE_LIST_FLAG_SAFE)
-				dfu->dfu_state = DFU_STATE_dfuMANIFEST;
-			else
-				dfu->dfu_state = DFU_STATE_dfuIDLE;
 			value = min(value, w_length);
 			break;
 		case USB_REQ_DFU_GETSTATE:
-- 
2.17.1



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

Re: [PATCH 1/2] fixup! usb: gadget: dfu: Wrap fs operation in workqueue

[Sascha Hauer]

On Mon, Mar 01, 2021 at 10:30:39AM +0100, Jules Maselbas wrote:
> When not in mode "safe", do_close was never called.
> 
> Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
> ---
>  drivers/usb/gadget/dfu.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

I have rewritten the commit messages while applying. fixup! is no longer
possible because the patch is already in master.

Applied, thanks

Sascha


> 
> diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
> index 82c9dc030..3234f25b3 100644
> --- a/drivers/usb/gadget/dfu.c
> +++ b/drivers/usb/gadget/dfu.c
> @@ -359,7 +359,6 @@ static void dfu_do_copy(struct dfu_work *dw)
>  	}
>  
>  	dfu->dfu_state = DFU_STATE_dfuIDLE;
> -	dfu_do_close(dw);
>  }
>  
>  static int
> @@ -557,6 +556,11 @@ static int handle_manifest(struct usb_function *f, const struct usb_ctrlrequest
>  		wq_queue_work(&dfu->wq, &dw->work);
>  	}
>  
> +	dw = xzalloc(sizeof(*dw));
> +	dw->dfu = dfu;
> +	dw->task = dfu_do_close;
> +	wq_queue_work(&dfu->wq, &dw->work);
> +
>  	return 0;
>  }
>  
> -- 
> 2.17.1
> 
> 
> 

-- 
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: [PATCH 1/2] fixup! usb: gadget: dfu: Wrap fs operation in workqueue

[Jules Maselbas]

On Mon, Mar 01, 2021 at 05:03:31PM +0100, Sascha Hauer wrote:
> On Mon, Mar 01, 2021 at 10:30:39AM +0100, Jules Maselbas wrote:
> > When not in mode "safe", do_close was never called.
> > 
> > Signed-off-by: Jules Maselbas <jmaselbas@kalray.eu>
> > ---
> >  drivers/usb/gadget/dfu.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> I have rewritten the commit messages while applying. fixup! is no longer
> possible because the patch is already in master.
Ok thanks, I wasn't sure to send this as fixup or not.
I'll take a look at master next time.

> Applied, thanks
> 
> Sascha
> 
> 
> > 
> > diff --git a/drivers/usb/gadget/dfu.c b/drivers/usb/gadget/dfu.c
> > index 82c9dc030..3234f25b3 100644
> > --- a/drivers/usb/gadget/dfu.c
> > +++ b/drivers/usb/gadget/dfu.c
> > @@ -359,7 +359,6 @@ static void dfu_do_copy(struct dfu_work *dw)
> >  	}
> >  
> >  	dfu->dfu_state = DFU_STATE_dfuIDLE;
> > -	dfu_do_close(dw);
> >  }
> >  
> >  static int
> > @@ -557,6 +556,11 @@ static int handle_manifest(struct usb_function *f, const struct usb_ctrlrequest
> >  		wq_queue_work(&dfu->wq, &dw->work);
> >  	}
> >  
> > +	dw = xzalloc(sizeof(*dw));
> > +	dw->dfu = dfu;
> > +	dw->task = dfu_do_close;
> > +	wq_queue_work(&dfu->wq, &dw->work);
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.17.1
> > 
> > 
> > 
> 
> -- 
> 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