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 <[email protected]> > --- > 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 [email protected] http://lists.infradead.org/mailman/listinfo/barebox
