Re: [PATCH v7 2/3] net/9p/usbg: Add new usb gadget function transport

[Michael Grzeschik <mgr@xxxxxxxxxxxxxx>]

Hi Oliver,

Thanks for your feedback!

Based on your feedback I just send v9:

https://lore.kernel.org/r/20240116-ml-topic-u9p-v9-0-93d73f47b76b@xxxxxxxxxxxxxx


On Mon, Jul 22, 2024 at 10:49:49AM +0200, Oliver Neukum wrote:
> 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.

Right.

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

Right.

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

I refactored this.

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

Yeah, not much to explain here. It is just worthless.
Also I was not thinking through how to handle an errornous transfer
to the upper vfs layer if some tx/rx path wath broken.

I now have fixed this by not calling any enqueue from the complete
handlers but am using the wait_for_complete functions to directly
expect finished transfers and response to them. This makes error
handling much easier and is also easier on the eye to read and
understand what is actually going on. It also solves most of
the request locking issues I had to begin with.

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

I fixed that.

> > +
> > +	usb9pfs_transmit(usb9pfs);
>
> This can fail. What happens then?


This won't fail here anymore, due to the change I explained above.

> > +
> > +	return;
> > +}
> > +
>
>
> [..]
>
> > +static int p9_usbg_cancel(struct p9_client *client, struct p9_req_t *req)
>
> This ought to be boolean

It can't for now since it is an 9p callback, which is currently
expecting int.

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

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

Attachment: signature.asc
Description: PGP signature