* [PATCH v3 0/3] usb: gadget: 9pfs transport
@ 2024-02-26 13:56 Michael Grzeschik
2024-02-26 13:56 ` [PATCH v3 1/3] usb: gadget: function: move u_f.h to include/linux/usb/ Michael Grzeschik
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Michael Grzeschik @ 2024-02-26 13:56 UTC (permalink / raw)
To: Eric Van Hensbergen, Latchesar Ionkov, Dominique Martinet,
Christian Schoenebeck, Jonathan Corbet, Greg Kroah-Hartman
Cc: v9fs, linux-doc, linux-kernel, linux-usb, kernel,
Michael Grzeschik
This series is adding support to mount usb hostside exported 9pfs
filesystems via the usb gadget interface. It also includes a simple tool
(p9_fwd.py) to translate an tcp 9pfs transport and reuse it via the usb
interface.
+--------------------------+ | +--------------------------+
| 9PFS mounting client | | | 9PFS exporting server |
SW | | | | |
| (this:trans_usbg) | | |(e.g. diod or nfs-ganesha)|
+-------------^------------+ | +-------------^------------+
| | |
| | +------v------+
| | | p9_fwd.py |
| | +------^------+
| | |
------------------|------------------------------------|-------------
| | |
+-------------v------------+ | +-------------v------------+
| | | | |
HW | USB Device Controller <---------> USB Host Controller |
| | | | |
+--------------------------+ | +--------------------------+
The USB host exports a filesystem, while the gadget on the USB device
side makes it mountable.
Diod (9pfs server) and the forwarder are on the development host, where
the root filesystem is actually stored. The gadget is initialized during
boot (or later) on the embedded board. Then the forwarder will find it
on the USB bus and start forwarding requests.
In this case the 9p requests come from the device and are handled by the
host. The reason is that USB device ports are normally not available on
PCs, so a connection in the other direction would not work.
One use-case is to use it as an alternative to NFS root booting during
the development of embedded Linux devices.
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
Changes in v3:
- dropped patch "usb: gadget: legacy: add 9pfs multi gadget" as discussed with gregkh
- Link to v2: https://lore.kernel.org/r/20240116-ml-topic-u9p-v2-0-b46cbf592962@pengutronix.de
Changes in v2:
- improved the commit messages
- introduced an patch to move the header u_f.h to include/linux/usb to compile usb gadget functions treewide
- moved usbg gadget function to net/9p/
- adderessed several comments in function driver, like the cleanup path and kbuild errors
- improved the documentation in Documentation/filesystems/9p.rst
- Link to v1: https://lore.kernel.org/r/20240116-ml-topic-u9p-v1-0-ad8c306f9a4e@pengutronix.de
---
Michael Grzeschik (3):
usb: gadget: function: move u_f.h to include/linux/usb/
net/9p/usbg: Add new usb gadget function transport
tools: usb: p9_fwd: add usb gadget packet forwarder script
Documentation/filesystems/9p.rst | 47 ++
drivers/usb/gadget/configfs.c | 2 +-
drivers/usb/gadget/function/f_fs.c | 2 +-
drivers/usb/gadget/function/f_hid.c | 2 +-
drivers/usb/gadget/function/f_loopback.c | 2 +-
drivers/usb/gadget/function/f_midi.c | 2 +-
drivers/usb/gadget/function/f_midi2.c | 2 +-
drivers/usb/gadget/function/f_sourcesink.c | 2 +-
drivers/usb/gadget/u_f.c | 2 +-
{drivers/usb/gadget => include/linux/usb}/u_f.h | 0
net/9p/Kconfig | 6 +
net/9p/Makefile | 4 +
net/9p/trans_usbg.c | 871 ++++++++++++++++++++++++
tools/usb/p9_fwd.py | 194 ++++++
14 files changed, 1130 insertions(+), 8 deletions(-)
---
base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
change-id: 20240116-ml-topic-u9p-895274530eb1
Best regards,
--
Michael Grzeschik <m.grzeschik@pengutronix.de>
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v3 1/3] usb: gadget: function: move u_f.h to include/linux/usb/ 2024-02-26 13:56 [PATCH v3 0/3] usb: gadget: 9pfs transport Michael Grzeschik @ 2024-02-26 13:56 ` Michael Grzeschik 2024-02-26 13:56 ` [PATCH v3 2/3] net/9p/usbg: Add new usb gadget function transport Michael Grzeschik ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Michael Grzeschik @ 2024-02-26 13:56 UTC (permalink / raw) To: Eric Van Hensbergen, Latchesar Ionkov, Dominique Martinet, Christian Schoenebeck, Jonathan Corbet, Greg Kroah-Hartman Cc: v9fs, linux-doc, linux-kernel, linux-usb, kernel, Michael Grzeschik We move the u_f.h header to include/linux/usb to be able to compile function drivers outside of the drivers/usb/gadget/function directory. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- v2 -> v3: - v1 -> v2: - new introduced patch --- drivers/usb/gadget/configfs.c | 2 +- drivers/usb/gadget/function/f_fs.c | 2 +- drivers/usb/gadget/function/f_hid.c | 2 +- drivers/usb/gadget/function/f_loopback.c | 2 +- drivers/usb/gadget/function/f_midi.c | 2 +- drivers/usb/gadget/function/f_midi2.c | 2 +- drivers/usb/gadget/function/f_sourcesink.c | 2 +- drivers/usb/gadget/u_f.c | 2 +- {drivers/usb/gadget => include/linux/usb}/u_f.h | 0 9 files changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c index ce3cfa1f36f51..4fa3eeafba814 100644 --- a/drivers/usb/gadget/configfs.c +++ b/drivers/usb/gadget/configfs.c @@ -8,8 +8,8 @@ #include <linux/usb/composite.h> #include <linux/usb/gadget_configfs.h> #include <linux/usb/webusb.h> +#include <linux/usb/u_f.h> #include "configfs.h" -#include "u_f.h" #include "u_os_desc.h" int check_user_usb_string(const char *name, diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c index 6bff6cb937891..c7f6e06f39b31 100644 --- a/drivers/usb/gadget/function/f_fs.c +++ b/drivers/usb/gadget/function/f_fs.c @@ -30,6 +30,7 @@ #include <linux/usb/ccid.h> #include <linux/usb/composite.h> #include <linux/usb/functionfs.h> +#include <linux/usb/u_f.h> #include <linux/aio.h> #include <linux/kthread.h> @@ -37,7 +38,6 @@ #include <linux/eventfd.h> #include "u_fs.h" -#include "u_f.h" #include "u_os_desc.h" #include "configfs.h" diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c index 3c8a9dd585c09..a5b667a03ca83 100644 --- a/drivers/usb/gadget/function/f_hid.c +++ b/drivers/usb/gadget/function/f_hid.c @@ -16,8 +16,8 @@ #include <linux/wait.h> #include <linux/sched.h> #include <linux/usb/g_hid.h> +#include <linux/usb/u_f.h> -#include "u_f.h" #include "u_hid.h" #define HIDG_MINORS 4 diff --git a/drivers/usb/gadget/function/f_loopback.c b/drivers/usb/gadget/function/f_loopback.c index 17ac6ace0cffa..d500025f45128 100644 --- a/drivers/usb/gadget/function/f_loopback.c +++ b/drivers/usb/gadget/function/f_loopback.c @@ -14,9 +14,9 @@ #include <linux/module.h> #include <linux/err.h> #include <linux/usb/composite.h> +#include <linux/usb/u_f.h> #include "g_zero.h" -#include "u_f.h" /* * LOOPBACK FUNCTION ... a testing vehicle for USB peripherals, diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c index 20c6fbd94f32d..2d551ba385c71 100644 --- a/drivers/usb/gadget/function/f_midi.c +++ b/drivers/usb/gadget/function/f_midi.c @@ -33,8 +33,8 @@ #include <linux/usb/gadget.h> #include <linux/usb/audio.h> #include <linux/usb/midi.h> +#include <linux/usb/u_f.h> -#include "u_f.h" #include "u_midi.h" MODULE_AUTHOR("Ben Williamson"); diff --git a/drivers/usb/gadget/function/f_midi2.c b/drivers/usb/gadget/function/f_midi2.c index ec8cd7c7bbfc1..ad59be1f9c54d 100644 --- a/drivers/usb/gadget/function/f_midi2.c +++ b/drivers/usb/gadget/function/f_midi2.c @@ -18,8 +18,8 @@ #include <linux/usb/gadget.h> #include <linux/usb/audio.h> #include <linux/usb/midi-v2.h> +#include <linux/usb/u_f.h> -#include "u_f.h" #include "u_midi2.h" struct f_midi2; diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c index 2edbd9b510d60..4283fb2e365bd 100644 --- a/drivers/usb/gadget/function/f_sourcesink.c +++ b/drivers/usb/gadget/function/f_sourcesink.c @@ -13,10 +13,10 @@ #include <linux/device.h> #include <linux/module.h> #include <linux/usb/composite.h> +#include <linux/usb/u_f.h> #include <linux/err.h> #include "g_zero.h" -#include "u_f.h" /* * SOURCE/SINK FUNCTION ... a primary testing vehicle for USB peripheral diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c index 6aea1ecb39999..90a7f742549a4 100644 --- a/drivers/usb/gadget/u_f.c +++ b/drivers/usb/gadget/u_f.c @@ -8,7 +8,7 @@ * Author: Andrzej Pietrasiewicz <andrzejtp2010@gmail.com> */ -#include "u_f.h" +#include <linux/usb/u_f.h> #include <linux/usb/ch9.h> struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len) diff --git a/drivers/usb/gadget/u_f.h b/include/linux/usb/u_f.h similarity index 100% rename from drivers/usb/gadget/u_f.h rename to include/linux/usb/u_f.h -- 2.39.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 2/3] net/9p/usbg: Add new usb gadget function transport 2024-02-26 13:56 [PATCH v3 0/3] usb: gadget: 9pfs transport Michael Grzeschik 2024-02-26 13:56 ` [PATCH v3 1/3] usb: gadget: function: move u_f.h to include/linux/usb/ Michael Grzeschik @ 2024-02-26 13:56 ` Michael Grzeschik 2024-03-06 15:18 ` Andrzej Pietrasiewicz 2024-02-26 13:56 ` [PATCH v3 3/3] tools: usb: p9_fwd: add usb gadget packet forwarder script Michael Grzeschik 2024-03-01 12:06 ` [PATCH v3 0/3] usb: gadget: 9pfs transport Andrzej Pietrasiewicz 3 siblings, 1 reply; 9+ messages in thread From: Michael Grzeschik @ 2024-02-26 13:56 UTC (permalink / raw) To: Eric Van Hensbergen, Latchesar Ionkov, Dominique Martinet, Christian Schoenebeck, Jonathan Corbet, Greg Kroah-Hartman Cc: v9fs, linux-doc, linux-kernel, linux-usb, kernel, Michael Grzeschik 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. 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, ... + 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; +}; + +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; + +static int usb9pfs_tx_fill(struct f_usb9pfs *usb9pfs, struct usb_request *req, + 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); + + dev_dbg(&cdev->gadget->dev, "tx submit --> %d\n", retval); + + return retval; +} + +static int usb9pfs_rx_fill(struct f_usb9pfs *usb9pfs, struct usb_request *req, + 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; +} + +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); + + 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) { + 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; + } +} + +static struct p9_req_t *usb9pfs_rx_header(struct f_usb9pfs *usb9pfs, + struct usb_request *req) +{ + 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; + } + + 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; + } + + 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; + + 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; + } + + 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) +{ + disable_ep(cdev, in); + disable_ep(cdev, out); +} + +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); + 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); +} + +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; +} + +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; +} + +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); +} + +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 */ +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"); -- 2.39.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/3] net/9p/usbg: Add new usb gadget function transport 2024-02-26 13:56 ` [PATCH v3 2/3] net/9p/usbg: Add new usb gadget function transport Michael Grzeschik @ 2024-03-06 15:18 ` Andrzej Pietrasiewicz 2024-03-06 19:54 ` Dominique Martinet 2024-04-29 23:34 ` Michael Grzeschik 0 siblings, 2 replies; 9+ messages in thread From: Andrzej Pietrasiewicz @ 2024-03-06 15:18 UTC (permalink / raw) To: Michael Grzeschik, Eric Van Hensbergen, Latchesar Ionkov, Dominique Martinet, Christian Schoenebeck, Jonathan Corbet, Greg Kroah-Hartman Cc: v9fs, linux-doc, linux-kernel, linux-usb, kernel Hi Michael, 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? 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). > + > 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 ? > +}; > + > +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? > + > +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). > + > + dev_dbg(&cdev->gadget->dev, "tx submit --> %d\n", retval); > + > + return retval; > +} The return value of this function is never used. > + > +static int usb9pfs_rx_fill(struct f_usb9pfs *usb9pfs, struct usb_request *req, 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. > + > +static int usb9pfs_transmit(struct f_usb9pfs *usb9pfs) Might be worth documenting that this needs to be called with req_lock held. > +{ > + 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. > + 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. > + } > +} > + > +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. > +{ > + 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... > + } > + > + 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. > + } > + > + 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); ? > + 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? > + > +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? > + > +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. > +} > + > +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? > +} > + > +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? 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"); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/3] net/9p/usbg: Add new usb gadget function transport 2024-03-06 15:18 ` Andrzej Pietrasiewicz @ 2024-03-06 19:54 ` Dominique Martinet 2024-04-29 23:34 ` Michael Grzeschik 1 sibling, 0 replies; 9+ messages in thread From: Dominique Martinet @ 2024-03-06 19:54 UTC (permalink / raw) To: Andrzej Pietrasiewicz Cc: Michael Grzeschik, Eric Van Hensbergen, Latchesar Ionkov, Christian Schoenebeck, Jonathan Corbet, Greg Kroah-Hartman, v9fs, linux-doc, linux-kernel, linux-usb, kernel Andrzej Pietrasiewicz wrote on Wed, Mar 06, 2024 at 04:18:54PM +0100: > 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? If the trans_usbg code can detect the host device is gone it should mark client->status as Disconnected then wake up all pending requests (something like p9_conn_cancel in trans_fd.c) Sorry I haven't found time to test/review either; I'd still be interested in running this in qemu for non-reg over using a real device so need to spend a few minutes figuring out the exact command line required.. I'll try to find some time this weekend -- Dominique ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/3] net/9p/usbg: Add new usb gadget function transport 2024-03-06 15:18 ` Andrzej Pietrasiewicz 2024-03-06 19:54 ` Dominique Martinet @ 2024-04-29 23:34 ` Michael Grzeschik 2024-04-30 8:06 ` Andrzej Pietrasiewicz 1 sibling, 1 reply; 9+ messages in thread From: Michael Grzeschik @ 2024-04-29 23:34 UTC (permalink / raw) To: Andrzej Pietrasiewicz Cc: Eric Van Hensbergen, Latchesar Ionkov, Dominique Martinet, Christian Schoenebeck, Jonathan Corbet, Greg Kroah-Hartman, v9fs, linux-usb, linux-kernel, kernel, linux-doc [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/3] net/9p/usbg: Add new usb gadget function transport 2024-04-29 23:34 ` Michael Grzeschik @ 2024-04-30 8:06 ` Andrzej Pietrasiewicz 0 siblings, 0 replies; 9+ messages in thread From: Andrzej Pietrasiewicz @ 2024-04-30 8:06 UTC (permalink / raw) To: Michael Grzeschik Cc: Eric Van Hensbergen, Latchesar Ionkov, Dominique Martinet, Christian Schoenebeck, Jonathan Corbet, Greg Kroah-Hartman, v9fs, linux-usb, linux-kernel, kernel, linux-doc Hi Michael, W dniu 30.04.2024 o 01:34, Michael Grzeschik pisze: > Hi Andrzej! > > Based on your feedback I rolled out v4. > > https://lore.kernel.org/r/20240116-ml-topic-u9p-v4-0-722ed28b0ade@pengutronix.de Thanks. I will be able to have a look and try it next week. Should you not hear from me until 10 May I wouldn't mind being pinged afterwards. Regards, Andrzej > > 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"); >>> >> >> >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 3/3] tools: usb: p9_fwd: add usb gadget packet forwarder script 2024-02-26 13:56 [PATCH v3 0/3] usb: gadget: 9pfs transport Michael Grzeschik 2024-02-26 13:56 ` [PATCH v3 1/3] usb: gadget: function: move u_f.h to include/linux/usb/ Michael Grzeschik 2024-02-26 13:56 ` [PATCH v3 2/3] net/9p/usbg: Add new usb gadget function transport Michael Grzeschik @ 2024-02-26 13:56 ` Michael Grzeschik 2024-03-01 12:06 ` [PATCH v3 0/3] usb: gadget: 9pfs transport Andrzej Pietrasiewicz 3 siblings, 0 replies; 9+ messages in thread From: Michael Grzeschik @ 2024-02-26 13:56 UTC (permalink / raw) To: Eric Van Hensbergen, Latchesar Ionkov, Dominique Martinet, Christian Schoenebeck, Jonathan Corbet, Greg Kroah-Hartman Cc: v9fs, linux-doc, linux-kernel, linux-usb, kernel, Michael Grzeschik This patch is adding an small python tool to forward 9pfs requests from the USB gadget to an existing 9pfs TCP server. Since currently all 9pfs servers lack support for the usb transport this tool is an useful helper to get started. Refer the Documentation section "USBG Example" in Documentation/filesystems/9p.rst on how to use it. Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- v2 -> v3: - v1 -> v2: - added usbg 9pfs detailed instructions to 9p.rst doc --- Documentation/filesystems/9p.rst | 32 +++++++ tools/usb/p9_fwd.py | 194 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 226 insertions(+) diff --git a/Documentation/filesystems/9p.rst b/Documentation/filesystems/9p.rst index 64439068a8fc5..264265c72ba67 100644 --- a/Documentation/filesystems/9p.rst +++ b/Documentation/filesystems/9p.rst @@ -67,6 +67,38 @@ To mount a 9p FS on a USB Host accessible via the gadget as root filesystem:: where mount_tag is the tag associated by the usb gadget transport. The pattern is usb9pfs0, usb9pfs1, ... +USBG Example +============ + +The USB host exports a filesystem, while the gadget on the USB device +side makes it mountable. + +Diod (9pfs server) and the forwarder are on the development host, where +the root filesystem is actually stored. The gadget is initialized during +boot (or later) on the embedded board. Then the forwarder will find it +on the USB bus and start forwarding requests. + +In this case the 9p requests come from the device and are handled by the +host. The reason is that USB device ports are normally not available on +PCs, so a connection in the other direction would not work. + +When using the usbg transport, for now there is no native usb host +service capable to handle the requests from the gadget driver. For +this we have to use the extra python tool p9_fwd.py from tools/usb. + +Just start the 9pfs capable network server like diod/nfs-ganesha e.g.: + + $ diod -f -n -d 0 -S -l 0.0.0.0:9999 -e $PWD + +Then start the python transport: + + $ python $kernel_dir/tools/usb/p9_fwd.py -p 9999 + +After that the gadget driver can be used as described above. + +One use-case is to use it as an alternative to NFS root booting during +the development of embedded Linux devices. + Options ======= diff --git a/tools/usb/p9_fwd.py b/tools/usb/p9_fwd.py new file mode 100755 index 0000000000000..95208df11abef --- /dev/null +++ b/tools/usb/p9_fwd.py @@ -0,0 +1,194 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: GPL-2.0 + +import argparse +import errno +import logging +import socket +import struct +import sys +import time + +import usb.core +import usb.util + + +class Forwarder: + HEXDUMP_FILTER = ( + "".join(chr(x).isprintable() and chr(x) or "." for x in range(128)) + "." * 128 + ) + + @staticmethod + def _log_hexdump(data): + if not logging.root.isEnabledFor(logging.TRACE): + return + L = 16 + for c in range(0, len(data), L): + chars = data[c : c + L] + dump = " ".join(f"{x:02x}" for x in chars) + printable = "".join(HEXDUMP_FILTER[x] for x in chars) + line = f"{c:08x} {dump:{L*3}s} |{printable:{L}s}|" + logging.root.log(logging.TRACE, "%s", line) + + def __init__(self, server): + self.stats = { + "c2s packets": 0, + "c2s bytes": 0, + "s2c packets": 0, + "s2c bytes": 0, + } + self.stats_logged = time.monotonic() + + dev = usb.core.find(idVendor=0x1D6B, idProduct=0x0109) + if dev is None: + raise ValueError("Device not found") + + logging.info(f"found device: {dev.bus}/{dev.address}") + + # dev.set_configuration() is not necessary since g_multi has only one + usb9pfs = None + # g_multi adds 9pfs as last interface + cfg = dev.get_active_configuration() + for intf in cfg: + # we have to detach the usb-storage driver from multi gadget since + # stall option could be set, which will lead to spontaneous port + # resets and our transfers will run dead + if intf.bInterfaceClass == 0x08: + if dev.is_kernel_driver_active(intf.bInterfaceNumber): + dev.detach_kernel_driver(intf.bInterfaceNumber) + + if ( + intf.bInterfaceClass == 0xFF + and intf.bInterfaceSubClass == 0xFF + and intf.bInterfaceProtocol == 0x09 + ): + usb9pfs = intf + if usb9pfs is None: + raise ValueError("Interface not found") + + logging.info(f"claiming interface:\n{usb9pfs}") + usb.util.claim_interface(dev, usb9pfs.bInterfaceNumber) + ep_out = usb.util.find_descriptor( + usb9pfs, + custom_match=lambda e: usb.util.endpoint_direction(e.bEndpointAddress) + == usb.util.ENDPOINT_OUT, + ) + assert ep_out is not None + ep_in = usb.util.find_descriptor( + usb9pfs, + custom_match=lambda e: usb.util.endpoint_direction(e.bEndpointAddress) + == usb.util.ENDPOINT_IN, + ) + assert ep_in is not None + logging.info(f"interface claimed") + + self.ep_out = ep_out + self.ep_in = ep_in + self.dev = dev + + # create and connect socket + self.s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + self.s.connect(server) + + logging.info(f"connected to server") + + def c2s(self): + """forward a request from the USB client to the TCP server""" + data = None + while data is None: + try: + logging.log(logging.TRACE, "c2s: reading") + data = self.ep_in.read(self.ep_in.wMaxPacketSize) + except usb.core.USBTimeoutError: + logging.log(logging.TRACE, "c2s: reading timed out") + continue + except usb.core.USBError as e: + if e.errno == errno.EIO: + logging.debug("c2s: reading failed with %s, retrying", repr(e)) + time.sleep(0.5) + continue + else: + logging.error("c2s: reading failed with %s, aborting", repr(e)) + raise + size = struct.unpack("<I", data[:4])[0] + while len(data) < size: + data += self.ep_in.read(size - len(data)) + logging.log(logging.TRACE, "c2s: writing") + self._log_hexdump(data) + self.s.send(data) + logging.debug("c2s: forwarded %i bytes", size) + self.stats["c2s packets"] += 1 + self.stats["c2s bytes"] += size + + def s2c(self): + """forward a response from the TCP server to the USB client""" + logging.log(logging.TRACE, "s2c: reading") + data = self.s.recv(4) + size = struct.unpack("<I", data[:4])[0] + while len(data) < size: + data += self.s.recv(size - len(data)) + logging.log(logging.TRACE, "s2c: writing") + self._log_hexdump(data) + while data: + written = self.ep_out.write(data) + assert written > 0 + data = data[written:] + if size % self.ep_out.wMaxPacketSize == 0: + logging.log(logging.TRACE, "sending zero length packet") + self.ep_out.write(b"") + logging.debug("s2c: forwarded %i bytes", size) + self.stats["s2c packets"] += 1 + self.stats["s2c bytes"] += size + + def log_stats(self): + logging.info("statistics:") + for k, v in self.stats.items(): + logging.info(f" {k+':':14s} {v}") + + def log_stats_interval(self, interval=5): + if (time.monotonic() - self.stats_logged) < interval: + return + + self.log_stats() + self.stats_logged = time.monotonic() + + +def main(): + parser = argparse.ArgumentParser( + description="Forward 9PFS requests from USB to TCP", + ) + + parser.add_argument( + "-s", "--server", type=str, default="127.0.0.1", help="server hostname" + ) + parser.add_argument("-p", "--port", type=int, default=564, help="server port") + parser.add_argument("-v", "--verbose", action="count", default=0) + + args = parser.parse_args() + + logging.TRACE = logging.DEBUG - 5 + logging.addLevelName(logging.TRACE, "TRACE") + + if args.verbose >= 2: + level = logging.TRACE + elif args.verbose: + level = logging.DEBUG + else: + level = logging.INFO + logging.basicConfig( + level=level, format="%(asctime)-15s %(levelname)-8s %(message)s" + ) + + f = Forwarder(server=(args.server, args.port)) + + try: + while True: + f.c2s() + f.s2c() + f.log_stats_interval() + finally: + f.log_stats() + + +if __name__ == "__main__": + main() -- 2.39.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 0/3] usb: gadget: 9pfs transport 2024-02-26 13:56 [PATCH v3 0/3] usb: gadget: 9pfs transport Michael Grzeschik ` (2 preceding siblings ...) 2024-02-26 13:56 ` [PATCH v3 3/3] tools: usb: p9_fwd: add usb gadget packet forwarder script Michael Grzeschik @ 2024-03-01 12:06 ` Andrzej Pietrasiewicz 3 siblings, 0 replies; 9+ messages in thread From: Andrzej Pietrasiewicz @ 2024-03-01 12:06 UTC (permalink / raw) To: Michael Grzeschik, Eric Van Hensbergen, Latchesar Ionkov, Dominique Martinet, Christian Schoenebeck, Jonathan Corbet, Greg Kroah-Hartman Cc: v9fs, linux-doc, linux-kernel, linux-usb, kernel Hi Michael, W dniu 26.02.2024 o 14:56, Michael Grzeschik pisze: > This series is adding support to mount usb hostside exported 9pfs > filesystems via the usb gadget interface. It also includes a simple tool > (p9_fwd.py) to translate an tcp 9pfs transport and reuse it via the usb > interface. > > +--------------------------+ | +--------------------------+ > | 9PFS mounting client | | | 9PFS exporting server | > SW | | | | | > | (this:trans_usbg) | | |(e.g. diod or nfs-ganesha)| > +-------------^------------+ | +-------------^------------+ > | | | > | | +------v------+ > | | | p9_fwd.py | > | | +------^------+ > | | | > ------------------|------------------------------------|------------- > | | | > +-------------v------------+ | +-------------v------------+ > | | | | | > HW | USB Device Controller <---------> USB Host Controller | > | | | | | > +--------------------------+ | +--------------------------+ > > The USB host exports a filesystem, while the gadget on the USB device > side makes it mountable. > > Diod (9pfs server) and the forwarder are on the development host, where > the root filesystem is actually stored. The gadget is initialized during > boot (or later) on the embedded board. Then the forwarder will find it > on the USB bus and start forwarding requests. > > In this case the 9p requests come from the device and are handled by the > host. The reason is that USB device ports are normally not available on > PCs, so a connection in the other direction would not work. > > One use-case is to use it as an alternative to NFS root booting during > the development of embedded Linux devices. > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > Changes in v3: > - dropped patch "usb: gadget: legacy: add 9pfs multi gadget" as discussed with gregkh I will give the dropped patch a try to see how it works, as it should work the same way regardless of how the gadget is composed, legacy or configfs. Speaking of the latter, IMO there _are_ valid usecases other than having 9pfs rootfs, so configfs support should be there. If you intend to send the patches adding configfs support then kindly Cc me and I will test and review it for you. Regards, Andrzej > - Link to v2: https://lore.kernel.org/r/20240116-ml-topic-u9p-v2-0-b46cbf592962@pengutronix.de > > Changes in v2: > - improved the commit messages > - introduced an patch to move the header u_f.h to include/linux/usb to compile usb gadget functions treewide > - moved usbg gadget function to net/9p/ > - adderessed several comments in function driver, like the cleanup path and kbuild errors > - improved the documentation in Documentation/filesystems/9p.rst > - Link to v1: https://lore.kernel.org/r/20240116-ml-topic-u9p-v1-0-ad8c306f9a4e@pengutronix.de > > --- > Michael Grzeschik (3): > usb: gadget: function: move u_f.h to include/linux/usb/ > net/9p/usbg: Add new usb gadget function transport > tools: usb: p9_fwd: add usb gadget packet forwarder script > > Documentation/filesystems/9p.rst | 47 ++ > drivers/usb/gadget/configfs.c | 2 +- > drivers/usb/gadget/function/f_fs.c | 2 +- > drivers/usb/gadget/function/f_hid.c | 2 +- > drivers/usb/gadget/function/f_loopback.c | 2 +- > drivers/usb/gadget/function/f_midi.c | 2 +- > drivers/usb/gadget/function/f_midi2.c | 2 +- > drivers/usb/gadget/function/f_sourcesink.c | 2 +- > drivers/usb/gadget/u_f.c | 2 +- > {drivers/usb/gadget => include/linux/usb}/u_f.h | 0 > net/9p/Kconfig | 6 + > net/9p/Makefile | 4 + > net/9p/trans_usbg.c | 871 ++++++++++++++++++++++++ > tools/usb/p9_fwd.py | 194 ++++++ > 14 files changed, 1130 insertions(+), 8 deletions(-) > --- > base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3 > change-id: 20240116-ml-topic-u9p-895274530eb1 > > Best regards, ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-04-30 8:07 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-02-26 13:56 [PATCH v3 0/3] usb: gadget: 9pfs transport Michael Grzeschik 2024-02-26 13:56 ` [PATCH v3 1/3] usb: gadget: function: move u_f.h to include/linux/usb/ Michael Grzeschik 2024-02-26 13:56 ` [PATCH v3 2/3] net/9p/usbg: Add new usb gadget function transport Michael Grzeschik 2024-03-06 15:18 ` Andrzej Pietrasiewicz 2024-03-06 19:54 ` Dominique Martinet 2024-04-29 23:34 ` Michael Grzeschik 2024-04-30 8:06 ` Andrzej Pietrasiewicz 2024-02-26 13:56 ` [PATCH v3 3/3] tools: usb: p9_fwd: add usb gadget packet forwarder script Michael Grzeschik 2024-03-01 12:06 ` [PATCH v3 0/3] usb: gadget: 9pfs transport Andrzej Pietrasiewicz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).