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

[Michael Grzeschik <mgr@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 35368 bytes --]

Hi Andrzej!

Based on your feedback I rolled out v4.

https://lore.kernel.org/r/20240116-ml-topic-u9p-v4-0-722ed28b0ade@pengutronix.de

On Wed, Mar 06, 2024 at 04:18:54PM +0100, Andrzej Pietrasiewicz wrote:
>W dniu 26.02.2024 o 14:56, Michael Grzeschik pisze:
>>Add the new gadget function for 9pfs transport. This function is
>>defining an simple 9pfs transport interface that consists of one in and
>>one out endpoint. The endpoints transmit and receive the 9pfs protocol
>>payload when mounting a 9p filesystem over usb.
>
>Reading the earlier discussions on v1 and v2 I was somehow under the impression
>that what you submitted here does not contain configfs support.
>
>But once I started reading the code I realized it does.
>
>It worked for me, both as a legacy gadget and when composed with configfs.
>
>I noticed that when you stop the forwarder at the host side when 9pfs remains
>mounted at the gadget side, umount hangs at the gadget side until the forwarder
>is restarted. I know that once the host-side software dies, not much can be
>done, however, unmounting a dead filesystem seems a reasonable thing to do
>and the way usb9pfs worked for me it is not possible to unmount in this
>situation. Any ideas on improving this?

As Dominique suggested I implemented conn_cancel to ensure the
connection to be removed on the gadget side. Regarding the stopped
correspondence from the host-side, there is not much we can do.
However for the v4 I ensured that a mount -f would work out
by checking the connection status before sending anything
to the host.

With this changes I was able to mount and unmount several times
and always reuse the gadget connection by restarting the host side.

>Please find my review below.
>
>>
>>Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>
>>---
>>v2 -> v3: -
>>v1 -> v2:
>>   - move the driver to net/9p/trans_usbg.c
>>   - fixed the commit title
>>   - fix %zu usage in p9_debug for size_t type
>>   - define functions static that are only used localy
>>   - return 0 in alloc_requests
>>   - use id_alloc_max instead of ida_simple_get
>>   - call ida_free in usb9pfs_free_func
>>   - call kfree for usb9pfs->tag and usb9pfs in usb9pfs_free_func
>>   - add MODULE_ALIAS_9P to load this module automatic when mounting
>>   - improved the documentation
>>   - added depends on USB_GADGET in Kconfig
>>---
>>  Documentation/filesystems/9p.rst |  15 +
>>  net/9p/Kconfig                   |   6 +
>>  net/9p/Makefile                  |   4 +
>>  net/9p/trans_usbg.c              | 871 +++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 896 insertions(+)
>>
>>diff --git a/Documentation/filesystems/9p.rst b/Documentation/filesystems/9p.rst
>>index 1e0e0bb6fdf91..64439068a8fc5 100644
>>--- a/Documentation/filesystems/9p.rst
>>+++ b/Documentation/filesystems/9p.rst
>>@@ -53,6 +53,20 @@ mount points. Each 9P export is seen by the client as a virtio device with an
>>  associated "mount_tag" property. Available mount tags can be
>>  seen by reading /sys/bus/virtio/drivers/9pnet_virtio/virtio<n>/mount_tag files.
>>+USBG Usage
>>+==========
>>+
>>+To mount a 9p FS on a USB Host accessible via the gadget at runtime::
>>+
>>+	mount -t 9p -o trans=usbg,aname=/path/to/fs <mount_tag> /mnt/9
>>+
>>+To mount a 9p FS on a USB Host accessible via the gadget as root filesystem::
>>+
>>+	root=<mount_tag> rootfstype=9p rootflags=trans=usbg,cache=loose,uname=root,access=0,dfltuid=0,dfltgid=0,aname=/path/to/rootfs
>>+
>>+where mount_tag is the tag associated by the usb gadget transport. The
>>+pattern is usb9pfs0, usb9pfs1, ...
>
>I'd reword to use "generated" instead of "associated" which would emphasize
>that it is the usb gadget transport part that comes up with the name.
>The word "associated" suggests that maybe the name comes from elsewhere.
>
>I'm wondering if it would make sense to treat instance name, e.g.
>
>"my_9pfs" from
>
>$ mkdir /sys/kernel/config/usb_gadget/g9pfs/functions/usb9pfs.my_9pfs
>
>as device name known above as "<mount_tag>?"
>
>FunctionFS uses this strategy and IMO it makes sense to follow this pattern.
>
>BTW, isn't this being called "mount_tag" an implementation detail of 9pfs?
>When doing "mount" users usually expect a "device name" (even if not a "real"
>device).

Fair point about the mount_tag. I reworked it to be used like with ffs.

>>+
>>  Options
>>  =======
>>@@ -68,6 +82,7 @@ Options
>>  			virtio	  connect to the next virtio channel available
>>  				  (from QEMU with trans_virtio module)
>>  			rdma	  connect to a specified RDMA channel
>>+			usbg	  connect to a specified usb gadget channel
>>  			========  ============================================
>>    uname=name	user name to attempt mount as on the remote server.  The
>>diff --git a/net/9p/Kconfig b/net/9p/Kconfig
>>index 00ebce9e5a657..c3d357eb8bb37 100644
>>--- a/net/9p/Kconfig
>>+++ b/net/9p/Kconfig
>>@@ -39,6 +39,12 @@ config NET_9P_XEN
>>  	  This builds support for a transport for 9pfs between
>>  	  two Xen domains.
>>+config NET_9P_USBG
>>+	bool "9P USB Gadget Transport"
>>+	depends on USB_GADGET
>>+	help
>>+	  This builds support for a transport for 9pfs over
>>+	  usb gadget.
>>  config NET_9P_RDMA
>>  	depends on INET && INFINIBAND && INFINIBAND_ADDR_TRANS
>>diff --git a/net/9p/Makefile b/net/9p/Makefile
>>index 1df9b344c30bd..22794a451c3f7 100644
>>--- a/net/9p/Makefile
>>+++ b/net/9p/Makefile
>>@@ -4,6 +4,7 @@ obj-$(CONFIG_NET_9P_FD) += 9pnet_fd.o
>>  obj-$(CONFIG_NET_9P_XEN) += 9pnet_xen.o
>>  obj-$(CONFIG_NET_9P_VIRTIO) += 9pnet_virtio.o
>>  obj-$(CONFIG_NET_9P_RDMA) += 9pnet_rdma.o
>>+obj-$(CONFIG_NET_9P_USBG) += 9pnet_usbg.o
>>  9pnet-objs := \
>>  	mod.o \
>>@@ -23,3 +24,6 @@ obj-$(CONFIG_NET_9P_RDMA) += 9pnet_rdma.o
>>  9pnet_rdma-objs := \
>>  	trans_rdma.o \
>>+
>>+9pnet_usbg-objs := \
>>+	trans_usbg.o \
>>diff --git a/net/9p/trans_usbg.c b/net/9p/trans_usbg.c
>>new file mode 100644
>>index 0000000000000..93d9606a806d8
>>--- /dev/null
>>+++ b/net/9p/trans_usbg.c
>>@@ -0,0 +1,871 @@
>>+// SPDX-License-Identifier: GPL-2.0+
>>+/*
>>+ * trans_usbg.c - USB peripheral usb9pfs configuration driver and transport.
>>+ *
>>+ * Copyright (C) 2024 Michael Grzeschik <m.grzeschik@pengutronix.de>
>>+ */
>>+
>>+/* Gadget usb9pfs only needs two bulk endpoints, and will use the usb9pfs
>>+ * transport to mount host exported filesystem via usb gadget.
>>+ */
>>+
>>+/*     +--------------------------+    |    +--------------------------+
>>+ *     |  9PFS mounting client    |    |    |  9PFS exporting server   |
>>+ *  SW |                          |    |    |                          |
>>+ *     |   (this:trans_usbg)      |    |    |(e.g. diod or nfs-ganesha)|
>>+ *     +-------------^------------+    |    +-------------^------------+
>>+ *                   |                 |                  |
>>+ * ------------------|------------------------------------|-------------
>>+ *                   |                 |                  |
>>+ *     +-------------v------------+    |    +-------------v------------+
>>+ *     |                          |    |    |                          |
>>+ *  HW |   USB Device Controller  <--------->   USB Host Controller    |
>>+ *     |                          |    |    |                          |
>>+ *     +--------------------------+    |    +--------------------------+
>>+ */
>>+
>>+#include <linux/slab.h>
>>+#include <linux/kernel.h>
>>+#include <linux/device.h>
>>+#include <linux/module.h>
>>+#include <linux/err.h>
>>+#include <linux/usb/composite.h>
>>+#include <linux/usb/u_f.h>
>>+
>>+#include <net/9p/9p.h>
>>+#include <net/9p/client.h>
>>+#include <net/9p/transport.h>
>>+
>>+#define DEFAULT_BUFLEN        16384
>>+
>>+struct f_usb9pfs {
>>+	struct p9_client *client;
>>+
>>+	struct p9_req_t *p9_tx_req;
>>+
>>+	struct list_head tx_req_list;
>>+
>>+	/* 9p request lock for en/dequeue */
>>+	spinlock_t lock;
>>+	/* usb request lock for en/dequeue */
>>+	spinlock_t req_lock;
>>+
>>+	struct usb_request *in_req;
>>+	struct usb_request *out_req;
>>+
>>+	struct usb_ep *in_ep;
>>+	struct usb_ep *out_ep;
>>+
>>+	unsigned int buflen;
>>+
>>+	bool inuse;
>>+	char *tag;
>>+
>>+	int index;
>>+
>>+	struct usb_function function;
>>+
>>+	struct list_head function_list;
>
>usb9pfs_instance ?

Sounds good. I changed it.

>>+};
>>+
>>+static inline struct f_usb9pfs *func_to_usb9pfs(struct usb_function *f)
>>+{
>>+	return container_of(f, struct f_usb9pfs, function);
>>+}
>>+
>>+struct f_usb9pfs_opts {
>>+	struct usb_function_instance func_inst;
>>+	unsigned int buflen;
>>+
>>+	/* Read/write access to configfs attributes is handled by configfs.
>>+	 *
>>+	 * This is to protect the data from concurrent access by read/write
>>+	 * and create symlink/remove symlink.
>>+	 */
>>+	struct mutex lock;
>>+	int refcnt;
>>+};
>>+
>>+static DEFINE_IDA(usb9pfs_ida);
>>+static DEFINE_MUTEX(usb9pfs_ida_lock); /* protects access do usb9pfs_ida */
>>+
>>+static DEFINE_MUTEX(usb9pfs_lock);
>>+static struct list_head usbg_function_list;
>
>This name seems too generic to me, it creates an impression that the list
>contains all functions of USB gadgets. It really doesn't and what it
>contains is a number of 9pfs instances. What about usb9pfs_instance_list?

I will go with usb9pfs_instance_list.

>>+
>>+static int usb9pfs_tx_fill(struct f_usb9pfs *usb9pfs, struct usb_request *req,
>
>given what this function effectively does...
>
>>+			   gfp_t gfp_flags)
>>+{
>>+	struct usb_composite_dev *cdev = usb9pfs->function.config->cdev;
>>+	int retval = -ENOMEM;
>>+
>>+	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);
>>+
>>+	retval = usb_ep_queue(usb9pfs->in_ep, req, gfp_flags);
>
>... (which is usb_ep_queue), I'd rather name it e.g. usb9pfs_tx_queue.
>Or maybe usb9pfs_queue_tx to not create an impression that the function
>is a queue of some sort (because rather than being a queue it does queueing).

I changed it to usb9pfs_queue_tx.

>>+
>>+	dev_dbg(&cdev->gadget->dev, "tx submit --> %d\n", retval);
>>+
>>+	return retval;
>>+}
>
>The return value of this function is never used.

Right, I fixed that at each caller.

>
>>+
>>+static int usb9pfs_rx_fill(struct f_usb9pfs *usb9pfs, struct usb_request *req,
>
>usb9pfs_queue_rx() ?

I changed it to usb9pfs_queue_rx.

>>+			   gfp_t gfp_flags)
>>+{
>>+	struct usb_composite_dev *cdev = usb9pfs->function.config->cdev;
>>+	int retval = -ENOMEM;
>>+
>>+	retval = usb_ep_queue(usb9pfs->out_ep, req, gfp_flags);
>>+
>>+	dev_dbg(&cdev->gadget->dev, "rx submit --> %d\n", retval);
>>+
>>+	return retval;
>>+}
>
>The return value of this function is never used.

Right, I fixed that at each caller.

>>+
>>+static int usb9pfs_transmit(struct f_usb9pfs *usb9pfs)
>
>Might be worth documenting that this needs to be called with req_lock held.

I wrote a comment above the function name.

>>+{
>>+	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);
>>+
>>+	usb9pfs->p9_tx_req = p9_req;
>>+
>>+	p9_req_get(usb9pfs->p9_tx_req);
>>+
>>+	usb9pfs_tx_fill(usb9pfs, usb9pfs->in_req, GFP_ATOMIC);
>>+
>>+	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;
>>+
>>+	/* 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);
>>+
>>+	switch (req->status) {
>
>Are more cases expected here? I'm asking because for now it seems
>that "if - else" would do the job.

You are right if else is doing the job just well. I changed it.

>>+	case 0:				/* normal completion? */
>>+		spin_lock(&usb9pfs->req_lock);
>>+
>>+		WRITE_ONCE(usb9pfs->p9_tx_req->status, REQ_STATUS_SENT);
>>+
>>+		p9_req_put(usb9pfs->client, usb9pfs->p9_tx_req);
>>+
>>+		usb9pfs_rx_fill(usb9pfs, usb9pfs->out_req, GFP_ATOMIC);
>>+
>>+		spin_unlock(&usb9pfs->req_lock);
>>+
>>+		break;
>>+
>>+	default:
>>+		dev_err(&cdev->gadget->dev, "%s usb9pfs complete --> %d, %d/%d\n",
>>+			ep->name, req->status, req->actual, req->length);
>>+		usb_ep_free_request(ep == usb9pfs->in_ep ?
>>+				    usb9pfs->out_ep : usb9pfs->in_ep,
>>+				    req->context);
>>+		free_ep_req(ep, req);
>>+		return;
>
>Is this return necessary? The "default" branch does contain some statements,
>so there's no need to silence the compiler.

I removed it.

>>+	}
>>+}
>>+
>>+static struct p9_req_t *usb9pfs_rx_header(struct f_usb9pfs *usb9pfs,
>>+					  struct usb_request *req)
>
>It seems that the logic of this function does not depend on anything USB.
>It is all p9 stuff, so I'd rather accept a plain buffer instead of usb_request.

Good point. I moved it to use the plain buffer instead.

>>+{
>>+	struct p9_req_t *p9_rx_req;
>>+	struct p9_fcall	rc;
>>+	int ret;
>>+
>>+	/* start by reading header */
>>+	rc.sdata = req->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;
>>+	}
>>+
>
>I know nothing about p9 proper, but it seems that at this point
>you have p9_rx_req...
>
>>+	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);
>>+		return NULL;
>
>... which you don't "put" here, but...

Nice catch. I added p9_req_put here aswell.

>>+	}
>>+
>>+	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);
>
>... you do "put" it here. Not sure if this is ok or not ok, just asking,
>because judging by the return value the caller of this function seems unable
>to know which of the above "return" statements caused a NULL, hence it is
>unable to tell whether it (the caller) should "put" or not.
>
>>+		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;
>>+
>>+	switch (req->status) {
>>+	case 0:				/* normal completion? */
>>+		spin_lock_irqsave(&usb9pfs->req_lock, flags);
>>+		p9_rx_req = usb9pfs_rx_header(usb9pfs, req);
>>+		if (!p9_rx_req) {
>>+			spin_unlock_irqrestore(&usb9pfs->req_lock, flags);
>>+			goto free_req;
>>+		}
>>+
>>+		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);
>>+
>>+		usb9pfs->p9_tx_req = NULL;
>>+
>>+		usb9pfs_transmit(usb9pfs);
>>+
>>+		spin_unlock_irqrestore(&usb9pfs->req_lock, flags);
>>+
>>+		return;
>>+free_req:
>>+	default:
>>+		dev_err(&cdev->gadget->dev, "%s usb9pfs complete --> %d, %d/%d\n",
>>+			ep->name, req->status, req->actual, req->length);
>>+		usb_ep_free_request(ep == usb9pfs->in_ep ?
>>+				    usb9pfs->out_ep : usb9pfs->in_ep,
>>+				    req->context);
>>+		free_ep_req(ep, req);
>>+		return;
>
>I assume this return means that p9_client_cb() should not be called if
>either "free_req" or "default" cases happen?
>
>But then anything after the end of this "switch" instruction seems unreachable
>code.

Right, it was some leftover. I removed the p9_client_cb.

>>+	}
>>+
>>+	p9_client_cb(usb9pfs->client, p9_rx_req, REQ_STATUS_ERROR);
>>+}
>>+
>>+static void disable_ep(struct usb_composite_dev *cdev, struct usb_ep *ep)
>>+{
>>+	int value;
>>+
>>+	value = usb_ep_disable(ep);
>>+	if (value < 0)
>>+		dev_info(&cdev->gadget->dev, "disable %s --> %d\n", ep->name, value);
>>+}
>>+
>>+static void disable_endpoints(struct usb_composite_dev *cdev,
>>+			      struct usb_ep *in, struct usb_ep *out,
>>+			      struct usb_ep *iso_in, struct usb_ep *iso_out)
>
>iso_in and iso_out are never used
>
>>+{
>>+	disable_ep(cdev, in);
>>+	disable_ep(cdev, out);
>
>I'm not sure how useful this whole function is...
>
>>+}
>>+
>>+static void disable_usb9pfs(struct f_usb9pfs *usb9pfs)
>>+{
>>+	struct usb_composite_dev *cdev;
>>+
>>+	cdev = usb9pfs->function.config->cdev;
>>+	disable_endpoints(cdev, usb9pfs->in_ep, usb9pfs->out_ep, NULL, NULL);
>
>... why not
>	disable_ep(cdev, usb9pfs->in_ep);
>	disable_ep(cdev, usb9pfs->out_ep);
>?

I removed the function and use the disable calls directly.

>>+	dev_dbg(&cdev->gadget->dev, "%s disabled\n", usb9pfs->function.name);
>>+}
>>+
>>+static inline struct usb_request *usb9pfs_alloc_ep_req(struct usb_ep *ep, int len)
>>+{
>>+	return alloc_ep_req(ep, len);
>>+}
>
>What is the purpose of this wrapper?

Leftover from some copy paste section. I removed it.

>>+
>>+static int alloc_requests(struct usb_composite_dev *cdev,
>>+			  struct f_usb9pfs *usb9pfs)
>>+{
>>+	usb9pfs->in_req = usb_ep_alloc_request(usb9pfs->in_ep, GFP_ATOMIC);
>>+	if (!usb9pfs->in_req)
>>+		goto fail;
>>+
>>+	usb9pfs->out_req = usb9pfs_alloc_ep_req(usb9pfs->out_ep, usb9pfs->buflen);
>>+	if (!usb9pfs->out_req)
>>+		goto fail_in;
>>+
>>+	usb9pfs->in_req->complete = usb9pfs_tx_complete;
>>+	usb9pfs->out_req->complete = usb9pfs_rx_complete;
>>+
>>+	/* length will be set in complete routine */
>>+	usb9pfs->in_req->context = usb9pfs;
>>+	usb9pfs->out_req->context = usb9pfs;
>>+
>>+	return 0;
>>+
>>+fail_in:
>>+	usb_ep_free_request(usb9pfs->in_ep, usb9pfs->in_req);
>>+fail:
>>+	return 0;
>>+}
>
>This funcion returns a zero no matter what. Such a return value carries
>no information. I saw something in the changelog regarding returning 0,
>was it about this function? But then why return always 0 instead of void?

I changed it to proper return values.

>>+
>>+static int enable_endpoint(struct usb_composite_dev *cdev,
>>+			   struct f_usb9pfs *usb9pfs, struct usb_ep *ep)
>>+{
>>+	int result;
>>+
>>+	result = config_ep_by_speed(cdev->gadget, &usb9pfs->function, ep);
>>+	if (result)
>>+		goto out;
>>+
>>+	result = usb_ep_enable(ep);
>>+	if (result < 0)
>>+		goto out;
>>+
>>+	ep->driver_data = usb9pfs;
>>+
>>+	result = 0;
>>+
>>+out:
>>+	return result;
>
>I assume this is "a single point of return". You'd save 3 lines of code
>by returning "result" on the spot instead of gotos and explicitly
>returning a zero. Who knows, maybe modern compilers are smart enough
>to figure this out. Not a deal breaker for me, though.

I squashed it as you suggested.

>>+}
>>+
>>+static int
>>+enable_usb9pfs(struct usb_composite_dev *cdev, struct f_usb9pfs *usb9pfs)
>>+{
>>+	int result = 0;
>>+
>>+	result = enable_endpoint(cdev, usb9pfs, usb9pfs->in_ep);
>>+	if (result)
>>+		goto out;
>>+
>>+	result = enable_endpoint(cdev, usb9pfs, usb9pfs->out_ep);
>>+	if (result)
>>+		goto disable_in;
>>+
>>+	result = alloc_requests(cdev, usb9pfs);
>>+	if (result)
>>+		goto disable_out;
>>+
>>+	dev_dbg(&cdev->gadget->dev, "%s enabled\n", usb9pfs->function.name);
>>+	return 0;
>>+
>>+disable_out:
>>+	usb_ep_disable(usb9pfs->out_ep);
>>+disable_in:
>>+	usb_ep_disable(usb9pfs->in_ep);
>>+out:
>>+	return result;
>>+}
>>+
>>+static int p9_usbg_create(struct p9_client *client, const char *devname, char *args)
>>+{
>>+	struct usb_composite_dev *cdev;
>>+	struct f_usb9pfs *usb9pfs;
>>+	struct f_usb9pfs *tmp;
>>+	struct usb_function *f;
>>+	int ret = -ENOENT;
>>+	int found = 0;
>>+
>>+	if (!devname)
>>+		return -EINVAL;
>>+
>>+	mutex_lock(&usb9pfs_lock);
>>+	list_for_each_entry_safe(usb9pfs, tmp, &usbg_function_list, function_list) {
>>+		if (!usb9pfs->tag)
>>+			continue;
>>+		if (!strncmp(devname, usb9pfs->tag, strlen(devname))) {
>>+			if (!usb9pfs->inuse) {
>>+				usb9pfs->inuse = true;
>>+				found = 1;
>>+				break;
>>+			}
>>+			ret = -EBUSY;
>>+			break;
>>+		}
>>+	}
>>+	mutex_unlock(&usb9pfs_lock);
>>+
>>+	if (!found) {
>>+		pr_err("no channels available for device %s\n", devname);
>>+		return ret;
>>+	}
>>+
>>+	INIT_LIST_HEAD(&usb9pfs->tx_req_list);
>>+
>>+	spin_lock_init(&usb9pfs->lock);
>>+	spin_lock_init(&usb9pfs->req_lock);
>>+
>>+	f = &usb9pfs->function;
>>+	cdev = f->config->cdev;
>>+
>>+	client->trans = (void *)usb9pfs;
>>+	client->status = Connected;
>>+	usb9pfs->client = client;
>>+
>>+	client->trans_mod->maxsize = usb9pfs->buflen;
>>+
>>+	disable_usb9pfs(usb9pfs);
>>+
>>+	return enable_usb9pfs(cdev, usb9pfs);
>
>This looks a bit weird. Clearly some side effect is intended here.
>Can you document here, why and what is expected?

This is probably due to some copy paste left over. I removed the extra
disable path.

>>+}
>>+
>>+static void p9_usbg_close(struct p9_client *client)
>>+{
>>+	struct f_usb9pfs *usb9pfs = client->trans;
>>+
>>+	disable_usb9pfs(usb9pfs);
>>+}
>>+
>>+static int p9_usbg_request(struct p9_client *client, struct p9_req_t *p9_req)
>>+{
>>+	struct f_usb9pfs *usb9pfs = client->trans;
>>+	unsigned long flags;
>>+
>>+	spin_lock_irqsave(&usb9pfs->lock, flags);
>>+	list_add_tail(&p9_req->req_list, &usb9pfs->tx_req_list);
>>+	spin_unlock_irqrestore(&usb9pfs->lock, flags);
>>+
>>+	spin_lock_irqsave(&usb9pfs->req_lock, flags);
>>+	usb9pfs_transmit(usb9pfs);
>>+	spin_unlock_irqrestore(&usb9pfs->req_lock, flags);
>>+
>>+	return 0;
>>+}
>>+
>>+/* We don't currently allow canceling of requests */
>
>What would we gain by allowing those? In other words,
>what are we losing?

As suggested by Dominique I added the cancel callback.

>Regards,
>
>Andrzej
>
>>+static int p9_usbg_cancel(struct p9_client *client, struct p9_req_t *req)
>>+{
>>+	return 1;
>>+}
>>+
>>+static struct p9_trans_module p9_usbg_trans = {
>>+	.name = "usbg",
>>+	.create = p9_usbg_create,
>>+	.close = p9_usbg_close,
>>+	.request = p9_usbg_request,
>>+	.cancel = p9_usbg_cancel,
>>+	.owner = THIS_MODULE,
>>+};
>>+
>>+/*-------------------------------------------------------------------------*/
>>+
>>+#define USB_PROTOCOL_9PFS	0x09
>>+
>>+static struct usb_interface_descriptor usb9pfs_intf = {
>>+	.bLength =		sizeof(usb9pfs_intf),
>>+	.bDescriptorType =	USB_DT_INTERFACE,
>>+
>>+	.bNumEndpoints =	2,
>>+	.bInterfaceClass =	USB_CLASS_VENDOR_SPEC,
>>+	.bInterfaceSubClass =	USB_SUBCLASS_VENDOR_SPEC,
>>+	.bInterfaceProtocol =   USB_PROTOCOL_9PFS,
>>+
>>+	/* .iInterface = DYNAMIC */
>>+};
>>+
>>+/* full speed support: */
>>+
>>+static struct usb_endpoint_descriptor fs_usb9pfs_source_desc = {
>>+	.bLength =		USB_DT_ENDPOINT_SIZE,
>>+	.bDescriptorType =	USB_DT_ENDPOINT,
>>+
>>+	.bEndpointAddress =	USB_DIR_IN,
>>+	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
>>+};
>>+
>>+static struct usb_endpoint_descriptor fs_usb9pfs_sink_desc = {
>>+	.bLength =		USB_DT_ENDPOINT_SIZE,
>>+	.bDescriptorType =	USB_DT_ENDPOINT,
>>+
>>+	.bEndpointAddress =	USB_DIR_OUT,
>>+	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
>>+};
>>+
>>+static struct usb_descriptor_header *fs_usb9pfs_descs[] = {
>>+	(struct usb_descriptor_header *)&usb9pfs_intf,
>>+	(struct usb_descriptor_header *)&fs_usb9pfs_sink_desc,
>>+	(struct usb_descriptor_header *)&fs_usb9pfs_source_desc,
>>+	NULL,
>>+};
>>+
>>+/* high speed support: */
>>+
>>+static struct usb_endpoint_descriptor hs_usb9pfs_source_desc = {
>>+	.bLength =		USB_DT_ENDPOINT_SIZE,
>>+	.bDescriptorType =	USB_DT_ENDPOINT,
>>+
>>+	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
>>+	.wMaxPacketSize =	cpu_to_le16(512),
>>+};
>>+
>>+static struct usb_endpoint_descriptor hs_usb9pfs_sink_desc = {
>>+	.bLength =		USB_DT_ENDPOINT_SIZE,
>>+	.bDescriptorType =	USB_DT_ENDPOINT,
>>+
>>+	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
>>+	.wMaxPacketSize =	cpu_to_le16(512),
>>+};
>>+
>>+static struct usb_descriptor_header *hs_usb9pfs_descs[] = {
>>+	(struct usb_descriptor_header *)&usb9pfs_intf,
>>+	(struct usb_descriptor_header *)&hs_usb9pfs_source_desc,
>>+	(struct usb_descriptor_header *)&hs_usb9pfs_sink_desc,
>>+	NULL,
>>+};
>>+
>>+/* super speed support: */
>>+
>>+static struct usb_endpoint_descriptor ss_usb9pfs_source_desc = {
>>+	.bLength =		USB_DT_ENDPOINT_SIZE,
>>+	.bDescriptorType =	USB_DT_ENDPOINT,
>>+
>>+	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
>>+	.wMaxPacketSize =	cpu_to_le16(1024),
>>+};
>>+
>>+static struct usb_ss_ep_comp_descriptor ss_usb9pfs_source_comp_desc = {
>>+	.bLength =		USB_DT_SS_EP_COMP_SIZE,
>>+	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
>>+	.bMaxBurst =		0,
>>+	.bmAttributes =		0,
>>+	.wBytesPerInterval =	0,
>>+};
>>+
>>+static struct usb_endpoint_descriptor ss_usb9pfs_sink_desc = {
>>+	.bLength =		USB_DT_ENDPOINT_SIZE,
>>+	.bDescriptorType =	USB_DT_ENDPOINT,
>>+
>>+	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
>>+	.wMaxPacketSize =	cpu_to_le16(1024),
>>+};
>>+
>>+static struct usb_ss_ep_comp_descriptor ss_usb9pfs_sink_comp_desc = {
>>+	.bLength =		USB_DT_SS_EP_COMP_SIZE,
>>+	.bDescriptorType =	USB_DT_SS_ENDPOINT_COMP,
>>+	.bMaxBurst =		0,
>>+	.bmAttributes =		0,
>>+	.wBytesPerInterval =	0,
>>+};
>>+
>>+static struct usb_descriptor_header *ss_usb9pfs_descs[] = {
>>+	(struct usb_descriptor_header *)&usb9pfs_intf,
>>+	(struct usb_descriptor_header *)&ss_usb9pfs_source_desc,
>>+	(struct usb_descriptor_header *)&ss_usb9pfs_source_comp_desc,
>>+	(struct usb_descriptor_header *)&ss_usb9pfs_sink_desc,
>>+	(struct usb_descriptor_header *)&ss_usb9pfs_sink_comp_desc,
>>+	NULL,
>>+};
>>+
>>+/* function-specific strings: */
>>+static struct usb_string strings_usb9pfs[] = {
>>+	[0].s = "usb9pfs input to output",
>>+	{  }			/* end of list */
>>+};
>>+
>>+static struct usb_gadget_strings stringtab_usb9pfs = {
>>+	.language	= 0x0409,	/* en-us */
>>+	.strings	= strings_usb9pfs,
>>+};
>>+
>>+static struct usb_gadget_strings *usb9pfs_strings[] = {
>>+	&stringtab_usb9pfs,
>>+	NULL,
>>+};
>>+
>>+/*-------------------------------------------------------------------------*/
>>+
>>+static int usb9pfs_func_bind(struct usb_configuration *c,
>>+			     struct usb_function *f)
>>+{
>>+	struct f_usb9pfs *usb9pfs = func_to_usb9pfs(f);
>>+	struct usb_composite_dev *cdev = c->cdev;
>>+	int ret;
>>+	int id;
>>+
>>+	/* allocate interface ID(s) */
>>+	id = usb_interface_id(c, f);
>>+	if (id < 0)
>>+		return id;
>>+	usb9pfs_intf.bInterfaceNumber = id;
>>+
>>+	id = usb_string_id(cdev);
>>+	if (id < 0)
>>+		return id;
>>+	strings_usb9pfs[0].id = id;
>>+	usb9pfs_intf.iInterface = id;
>>+
>>+	/* allocate endpoints */
>>+	usb9pfs->in_ep = usb_ep_autoconfig(cdev->gadget,
>>+					   &fs_usb9pfs_source_desc);
>>+	if (!usb9pfs->in_ep)
>>+		goto autoconf_fail;
>>+
>>+	usb9pfs->out_ep = usb_ep_autoconfig(cdev->gadget,
>>+					    &fs_usb9pfs_sink_desc);
>>+	if (!usb9pfs->out_ep)
>>+		goto autoconf_fail;
>>+
>>+	/* support high speed hardware */
>>+	hs_usb9pfs_source_desc.bEndpointAddress =
>>+		fs_usb9pfs_source_desc.bEndpointAddress;
>>+	hs_usb9pfs_sink_desc.bEndpointAddress = fs_usb9pfs_sink_desc.bEndpointAddress;
>>+
>>+	/* support super speed hardware */
>>+	ss_usb9pfs_source_desc.bEndpointAddress =
>>+		fs_usb9pfs_source_desc.bEndpointAddress;
>>+	ss_usb9pfs_sink_desc.bEndpointAddress = fs_usb9pfs_sink_desc.bEndpointAddress;
>>+
>>+	ret = usb_assign_descriptors(f, fs_usb9pfs_descs, hs_usb9pfs_descs,
>>+				     ss_usb9pfs_descs, ss_usb9pfs_descs);
>>+	if (ret)
>>+		return ret;
>>+
>>+	dev_dbg(&cdev->gadget->dev, "%s speed %s: IN/%s, OUT/%s\n",
>>+		(gadget_is_superspeed(c->cdev->gadget) ? "super" :
>>+		(gadget_is_dualspeed(c->cdev->gadget) ? "dual" : "full")),
>>+			f->name, usb9pfs->in_ep->name, usb9pfs->out_ep->name);
>>+	return 0;
>>+
>>+autoconf_fail:
>>+	ERROR(cdev, "%s: can't autoconfigure on %s\n",
>>+	      f->name, cdev->gadget->name);
>>+	return -ENODEV;
>>+}
>>+
>>+static void usb9pfs_free_func(struct usb_function *f)
>>+{
>>+	struct f_usb9pfs *usb9pfs = func_to_usb9pfs(f);
>>+	struct f_usb9pfs_opts *opts;
>>+
>>+	ida_free(&usb9pfs_ida, usb9pfs->index);
>>+	kfree(usb9pfs->tag);
>>+	kfree(usb9pfs);
>>+
>>+	opts = container_of(f->fi, struct f_usb9pfs_opts, func_inst);
>>+
>>+	mutex_lock(&opts->lock);
>>+	opts->refcnt--;
>>+	mutex_unlock(&opts->lock);
>>+
>>+	usb_free_all_descriptors(f);
>>+}
>>+
>>+static int usb9pfs_set_alt(struct usb_function *f,
>>+			   unsigned int intf, unsigned int alt)
>>+{
>>+	return 0;
>>+}
>>+
>>+static void usb9pfs_disable(struct usb_function *f)
>>+{
>>+}
>>+
>>+static struct usb_function *usb9pfs_alloc(struct usb_function_instance *fi)
>>+{
>>+	struct f_usb9pfs_opts *usb9pfs_opts;
>>+	struct f_usb9pfs *usb9pfs;
>>+
>>+	usb9pfs = kzalloc(sizeof(*usb9pfs), GFP_KERNEL);
>>+	if (!usb9pfs)
>>+		return ERR_PTR(-ENOMEM);
>>+
>>+	usb9pfs_opts = container_of(fi, struct f_usb9pfs_opts, func_inst);
>>+
>>+	mutex_lock(&usb9pfs_opts->lock);
>>+	usb9pfs_opts->refcnt++;
>>+	mutex_unlock(&usb9pfs_opts->lock);
>>+
>>+	usb9pfs->buflen = usb9pfs_opts->buflen;
>>+
>>+	usb9pfs->function.name = "usb9pfs";
>>+	usb9pfs->function.bind = usb9pfs_func_bind;
>>+	usb9pfs->function.set_alt = usb9pfs_set_alt;
>>+	usb9pfs->function.disable = usb9pfs_disable;
>>+	usb9pfs->function.strings = usb9pfs_strings;
>>+
>>+	usb9pfs->function.free_func = usb9pfs_free_func;
>>+
>>+	mutex_lock(&usb9pfs_ida_lock);
>>+
>>+	usb9pfs->index = ida_alloc_max(&usb9pfs_ida, U8_MAX, GFP_KERNEL);
>>+	if (usb9pfs->index < 0) {
>>+		struct usb_function *ret = ERR_PTR(usb9pfs->index);
>>+
>>+		kfree(usb9pfs);
>>+		mutex_unlock(&usb9pfs_ida_lock);
>>+		return ret;
>>+	}
>>+
>>+	mutex_unlock(&usb9pfs_ida_lock);
>>+
>>+	usb9pfs->tag = kasprintf(GFP_KERNEL, "%s%d", usb9pfs->function.name,
>>+				 usb9pfs->index);
>>+
>>+	INIT_LIST_HEAD(&usb9pfs->function_list);
>>+
>>+	mutex_lock(&usb9pfs_lock);
>>+	list_add_tail(&usb9pfs->function_list, &usbg_function_list);
>>+	mutex_unlock(&usb9pfs_lock);
>>+
>>+	return &usb9pfs->function;
>>+}
>>+
>>+static inline struct f_usb9pfs_opts *to_f_usb9pfs_opts(struct config_item *item)
>>+{
>>+	return container_of(to_config_group(item), struct f_usb9pfs_opts,
>>+			    func_inst.group);
>>+}
>>+
>>+static void usb9pfs_attr_release(struct config_item *item)
>>+{
>>+	struct f_usb9pfs_opts *usb9pfs_opts = to_f_usb9pfs_opts(item);
>>+
>>+	usb_put_function_instance(&usb9pfs_opts->func_inst);
>>+}
>>+
>>+static struct configfs_item_operations usb9pfs_item_ops = {
>>+	.release		= usb9pfs_attr_release,
>>+};
>>+
>>+static ssize_t f_usb9pfs_opts_buflen_show(struct config_item *item, char *page)
>>+{
>>+	struct f_usb9pfs_opts *opts = to_f_usb9pfs_opts(item);
>>+	int result;
>>+
>>+	mutex_lock(&opts->lock);
>>+	result = sprintf(page, "%d\n", opts->buflen);
>>+	mutex_unlock(&opts->lock);
>>+
>>+	return result;
>>+}
>>+
>>+static ssize_t f_usb9pfs_opts_buflen_store(struct config_item *item,
>>+					   const char *page, size_t len)
>>+{
>>+	struct f_usb9pfs_opts *opts = to_f_usb9pfs_opts(item);
>>+	int ret;
>>+	u32 num;
>>+
>>+	mutex_lock(&opts->lock);
>>+	if (opts->refcnt) {
>>+		ret = -EBUSY;
>>+		goto end;
>>+	}
>>+
>>+	ret = kstrtou32(page, 0, &num);
>>+	if (ret)
>>+		goto end;
>>+
>>+	opts->buflen = num;
>>+	ret = len;
>>+end:
>>+	mutex_unlock(&opts->lock);
>>+	return ret;
>>+}
>>+
>>+CONFIGFS_ATTR(f_usb9pfs_opts_, buflen);
>>+
>>+static struct configfs_attribute *usb9pfs_attrs[] = {
>>+	&f_usb9pfs_opts_attr_buflen,
>>+	NULL,
>>+};
>>+
>>+static const struct config_item_type usb9pfs_func_type = {
>>+	.ct_item_ops	= &usb9pfs_item_ops,
>>+	.ct_attrs	= usb9pfs_attrs,
>>+	.ct_owner	= THIS_MODULE,
>>+};
>>+
>>+static void usb9pfs_free_instance(struct usb_function_instance *fi)
>>+{
>>+	struct f_usb9pfs_opts *usb9pfs_opts;
>>+
>>+	usb9pfs_opts = container_of(fi, struct f_usb9pfs_opts, func_inst);
>>+	kfree(usb9pfs_opts);
>>+}
>>+
>>+static struct usb_function_instance *usb9pfs_alloc_instance(void)
>>+{
>>+	struct f_usb9pfs_opts *usb9pfs_opts;
>>+
>>+	usb9pfs_opts = kzalloc(sizeof(*usb9pfs_opts), GFP_KERNEL);
>>+	if (!usb9pfs_opts)
>>+		return ERR_PTR(-ENOMEM);
>>+
>>+	mutex_init(&usb9pfs_opts->lock);
>>+
>>+	usb9pfs_opts->func_inst.free_func_inst = usb9pfs_free_instance;
>>+
>>+	usb9pfs_opts->buflen = DEFAULT_BUFLEN;
>>+
>>+	config_group_init_type_name(&usb9pfs_opts->func_inst.group, "",
>>+				    &usb9pfs_func_type);
>>+
>>+	return &usb9pfs_opts->func_inst;
>>+}
>>+DECLARE_USB_FUNCTION(usb9pfs, usb9pfs_alloc_instance, usb9pfs_alloc);
>>+
>>+static int __init usb9pfs_modinit(void)
>>+{
>>+	int ret;
>>+
>>+	INIT_LIST_HEAD(&usbg_function_list);
>>+
>>+	ret = usb_function_register(&usb9pfsusb_func);
>>+	if (!ret)
>>+		v9fs_register_trans(&p9_usbg_trans);
>>+
>>+	return ret;
>>+}
>>+
>>+static void __exit usb9pfs_modexit(void)
>>+{
>>+	usb_function_unregister(&usb9pfsusb_func);
>>+	v9fs_unregister_trans(&p9_usbg_trans);
>>+}
>>+
>>+module_init(usb9pfs_modinit);
>>+module_exit(usb9pfs_modexit);
>>+
>>+MODULE_ALIAS_9P("usbg");
>>+MODULE_LICENSE("GPL");
>>+MODULE_DESCRIPTION("USB gadget 9pfs transport");
>>+MODULE_AUTHOR("Michael Grzeschik");
>>
>
>
>

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]