| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fffb9372-7b25-450a-b466-51afe82ad126@collabora.com>
Date: Tue, 30 Apr 2024 10:06:57 +0200
From: Andrzej Pietrasiewicz <andrzej.p@...labora.com>
To: Michael Grzeschik <mgr@...gutronix.de>
Cc: Eric Van Hensbergen <ericvh@...nel.org>,
Latchesar Ionkov <lucho@...kov.net>,
Dominique Martinet <asmadeus@...ewreck.org>,
Christian Schoenebeck <linux_oss@...debyte.com>,
Jonathan Corbet <corbet@....net>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>, v9fs@...ts.linux.dev,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
kernel@...gutronix.de, linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 2/3] net/9p/usbg: Add new usb gadget function transport
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@...gutronix.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@...gutronix.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");
>>>
>>
>>
>>
>
Powered by blists - more mailing lists