On 22.07.24 00:08, Michael Grzeschik wrote:
+ +static int usb9pfs_queue_tx(struct f_usb9pfs *usb9pfs, struct usb_request *req, + gfp_t gfp_flags) +{ + struct usb_composite_dev *cdev = usb9pfs->function.config->cdev; + int ret = -ENOMEM;
No need. This will be overwritten.
+ + if (!(usb9pfs->p9_tx_req->tc.size % usb9pfs->in_ep->maxpacket)) + req->zero = 1; + + req->buf = usb9pfs->p9_tx_req->tc.sdata; + req->length = usb9pfs->p9_tx_req->tc.size; + + dev_dbg(&cdev->gadget->dev, "%s usb9pfs send --> %d/%d, zero: %d\n", + usb9pfs->in_ep->name, req->actual, req->length, req->zero); + + ret = usb_ep_queue(usb9pfs->in_ep, req, gfp_flags); + + dev_dbg(&cdev->gadget->dev, "tx submit --> %d\n", ret); + + return ret; +} + +static int usb9pfs_queue_rx(struct f_usb9pfs *usb9pfs, struct usb_request *req, + gfp_t gfp_flags) +{ + struct usb_composite_dev *cdev = usb9pfs->function.config->cdev; + int ret = -ENOMEM;
Overwritten in literally the next statement.
+ ret = usb_ep_queue(usb9pfs->out_ep, req, gfp_flags); + + dev_dbg(&cdev->gadget->dev, "rx submit --> %d\n", ret); + + return ret; +} + +static int usb9pfs_transmit(struct f_usb9pfs *usb9pfs) +{ + struct p9_req_t *p9_req = NULL; + unsigned long flags; + int ret = 0; + + spin_lock_irqsave(&usb9pfs->lock, flags); + if (usb9pfs->p9_tx_req) { + spin_unlock_irqrestore(&usb9pfs->lock, flags); + return -EBUSY; + } + + p9_req = list_first_entry_or_null(&usb9pfs->tx_req_list, + struct p9_req_t, req_list); + if (!p9_req) { + spin_unlock_irqrestore(&usb9pfs->lock, flags); + return -ENOENT; + } + + list_del(&p9_req->req_list);
You have deleted it from the list
+ usb9pfs->p9_tx_req = p9_req; + + p9_req_get(usb9pfs->p9_tx_req); + + ret = usb9pfs_queue_tx(usb9pfs, usb9pfs->in_req, GFP_ATOMIC);
This means that if this function returns an error, the deletion from the list may or may not have happened.
+ spin_unlock_irqrestore(&usb9pfs->lock, flags); + + return ret; +} + +static void usb9pfs_tx_complete(struct usb_ep *ep, struct usb_request *req) +{ + struct f_usb9pfs *usb9pfs = ep->driver_data; + struct usb_composite_dev *cdev = usb9pfs->function.config->cdev; + int ret = 0; + + if (req->status) { + dev_err(&cdev->gadget->dev, "%s usb9pfs complete --> %d, %d/%d\n", + ep->name, req->status, req->actual, req->length); + return; + } + + /* reset zero packages */ + req->zero = 0; + + dev_dbg(&cdev->gadget->dev, "%s usb9pfs complete --> %d, %d/%d\n", + ep->name, req->status, req->actual, req->length); + + WRITE_ONCE(usb9pfs->p9_tx_req->status, REQ_STATUS_SENT); + + p9_req_put(usb9pfs->client, usb9pfs->p9_tx_req); + + ret = usb9pfs_queue_rx(usb9pfs, usb9pfs->out_req, GFP_ATOMIC); + if (ret) + return;
Ehhh ? Could you explain the error handling here?
+ + return; +} + +static struct p9_req_t *usb9pfs_rx_header(struct f_usb9pfs *usb9pfs, void *buf) +{ + struct p9_req_t *p9_rx_req; + struct p9_fcall rc; + int ret; + + /* start by reading header */ + rc.sdata = buf; + rc.offset = 0; + rc.capacity = P9_HDRSZ; + rc.size = P9_HDRSZ; + + p9_debug(P9_DEBUG_TRANS, "mux %p got %zu bytes\n", usb9pfs, + rc.capacity - rc.offset); + + ret = p9_parse_header(&rc, &rc.size, NULL, NULL, 0); + if (ret) { + p9_debug(P9_DEBUG_ERROR, + "error parsing header: %d\n", ret); + return NULL; + } + + p9_debug(P9_DEBUG_TRANS, + "mux %p pkt: size: %d bytes tag: %d\n", + usb9pfs, rc.size, rc.tag); + + p9_rx_req = p9_tag_lookup(usb9pfs->client, rc.tag); + if (!p9_rx_req || p9_rx_req->status != REQ_STATUS_SENT) { + p9_debug(P9_DEBUG_ERROR, "Unexpected packet tag %d\n", rc.tag); + return NULL; + } + + if (rc.size > p9_rx_req->rc.capacity) { + p9_debug(P9_DEBUG_ERROR, + "requested packet size too big: %d for tag %d with capacity %zd\n", + rc.size, rc.tag, p9_rx_req->rc.capacity); + p9_req_put(usb9pfs->client, p9_rx_req); + return NULL; + } + + if (!p9_rx_req->rc.sdata) { + p9_debug(P9_DEBUG_ERROR, + "No recv fcall for tag %d (req %p), disconnecting!\n", + rc.tag, p9_rx_req); + p9_req_put(usb9pfs->client, p9_rx_req); + return NULL; + } + + return p9_rx_req; +} + +static void usb9pfs_rx_complete(struct usb_ep *ep, struct usb_request *req) +{ + struct f_usb9pfs *usb9pfs = ep->driver_data; + struct usb_composite_dev *cdev = usb9pfs->function.config->cdev; + struct p9_req_t *p9_rx_req; + unsigned long flags; + + if (req->status) { + dev_err(&cdev->gadget->dev, "%s usb9pfs complete --> %d, %d/%d\n", + ep->name, req->status, req->actual, req->length); + return; + } + + p9_rx_req = usb9pfs_rx_header(usb9pfs, req->buf); + if (!p9_rx_req) + return; + + memcpy(p9_rx_req->rc.sdata, req->buf, req->actual); + + p9_rx_req->rc.size = req->actual; + + p9_client_cb(usb9pfs->client, p9_rx_req, REQ_STATUS_RCVD); + p9_req_put(usb9pfs->client, p9_rx_req); + + spin_lock_irqsave(&usb9pfs->lock, flags); + usb9pfs->p9_tx_req = NULL; + + spin_unlock_irqrestore(&usb9pfs->lock, flags);
Why can usb9pfs_tx_complete() touch this without taking the spinlock?
+ + usb9pfs_transmit(usb9pfs);
This can fail. What happens then?
+ + return; +} +
[..]
+static int p9_usbg_cancel(struct p9_client *client, struct p9_req_t *req)
This ought to be boolean
+{ + struct f_usb9pfs *usb9pfs = client->trans; + unsigned long flags; + int ret = 1; + + p9_debug(P9_DEBUG_TRANS, "client %p req %p\n", client, req); + + spin_lock_irqsave(&usb9pfs->lock, flags); + + if (req->status == REQ_STATUS_UNSENT) { + list_del(&req->req_list); + WRITE_ONCE(req->status, REQ_STATUS_FLSHD); + p9_req_put(client, req); + ret = 0; + } + spin_unlock_irqrestore(&usb9pfs->lock, flags); + + return ret; +}
Regards Oliver