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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
