Thanks for the review! I just send out v10 with your suggestions: https://lore.kernel.org/all/20240116-ml-topic-u9p-v10-0-a85fdeac2c52@xxxxxxxxxxxxxx/ On Fri, Aug 23, 2024 at 11:45:23AM +0200, Christophe JAILLET wrote:
Le 23/08/2024 à 09:36, Michael Grzeschik a écrit :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. Tested-by: Andrzej Pietrasiewicz <andrzej.p-ZGY8ohtN/8qB+jHODAdFcQ@xxxxxxxxxxxxxxxx> Signed-off-by: Michael Grzeschik <m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@xxxxxxxxxxxxxxxx> ---Hi, a few nitpicks below and 1 or 2 real questions.+#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/func_utils.h> +#include <linux/completion.h>Keep it in alphabetic order?
Did that!
+ +#include <net/9p/9p.h> +#include <net/9p/client.h> +#include <net/9p/transport.h> +...+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; + struct p9_req_t *p9_tx_req = req->context; + unsigned long flags; + + /* reset zero packages */ + req->zero = 0; + + if (req->status) { + dev_err(&cdev->gadget->dev, "%s usb9pfs complete --> %d, %d/%d\n", + ep->name, req->status, req->actual, req->length); + return; + } + + dev_dbg(&cdev->gadget->dev, "%s usb9pfs complete --> %d, %d/%d\n", + ep->name, req->status, req->actual, req->length); + + spin_lock_irqsave(&usb9pfs->lock, flags); + WRITE_ONCE(p9_tx_req->status, REQ_STATUS_SENT); + + p9_req_put(usb9pfs->client, p9_tx_req); + + spin_unlock_irqrestore(&usb9pfs->lock, flags); + + complete(&usb9pfs->send); + + return;Is it needed?
Nope, gone!
+}...+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; + + if (req->status) { + dev_err(&cdev->gadget->dev, "%s usb9pfs complete --> %d, %d/%d\n", + ep->name, req->status, req->actual, req->length); + return; + } + + p9_rx_req = usb9pfs_rx_header(usb9pfs, req->buf); + if (!p9_rx_req) { + return; + }The { } could be removd.
Done!
+ + 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); + + complete(&usb9pfs->received); + + return;Is it needed?
No.
+}...+static int alloc_requests(struct usb_composite_dev *cdev, + struct f_usb9pfs *usb9pfs) +{ + int ret = 0; + + usb9pfs->in_req = usb_ep_alloc_request(usb9pfs->in_ep, GFP_ATOMIC); + if (!usb9pfs->in_req) { + ret = -ENOENT; + goto fail; + } + + usb9pfs->out_req = alloc_ep_req(usb9pfs->out_ep, usb9pfs->buflen); + if (!usb9pfs->out_req) { + ret = -ENOENT; + 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 ret;return 0; to be more explicit? (and would avoid the = 0 when declared)
Good point. Did that.
+ +fail_in: + usb_ep_free_request(usb9pfs->in_ep, usb9pfs->in_req); +fail: + return ret; +} + +static int enable_endpoint(struct usb_composite_dev *cdev, + struct f_usb9pfs *usb9pfs, struct usb_ep *ep) +{ + int ret; + + ret = config_ep_by_speed(cdev->gadget, &usb9pfs->function, ep); + if (ret) + return ret; + + ret = usb_ep_enable(ep); + if (ret < 0) + return ret; + + ep->driver_data = usb9pfs; + + return ret;return 0; to be more explicit?
Agreed!
+}...+static int p9_usbg_create(struct p9_client *client, const char *devname, char *args) +{ + struct f_usb9pfs_dev *dev; + struct f_usb9pfs_dev *tmp; + struct f_usb9pfs *usb9pfs; + struct usb_function *f; + int ret = -ENOENT; + int found = 0; + + if (!devname) + return -EINVAL; + + mutex_lock(&usb9pfs_lock);Using cleanup.h would simplify locking in early exit paths.
Whoo, cleanup.h is a peace of art. I totally forgot about that. I also addressed the the spinlocks which seemed obvious.
+ list_for_each_entry_safe(dev, tmp, &usbg_instance_list, usb9pfs_instance) {Why the _safe version is used here? The list is not modify here directly.
No need. I used the non safe version instead.
+ if (!strncmp(devname, dev->tag, strlen(devname))) { + if (!dev->inuse) { + dev->inuse = true; + found = 1; + break; + } + ret = -EBUSY; + break; + } + } + + if (!found) { + mutex_unlock(&usb9pfs_lock); + pr_err("no channels available for device %s\n", devname); + return ret; + } + + usb9pfs = dev->usb9pfs; + if (!usb9pfs) { + mutex_unlock(&usb9pfs_lock); + return -EINVAL; + } + + f = &usb9pfs->function; + + client->trans = (void *)usb9pfs; + if (!usb9pfs->in_req) + client->status = Disconnected; + else + client->status = Connected; + usb9pfs->client = client; + + client->trans_mod->maxsize = usb9pfs->buflen; + + complete(&usb9pfs->received); + mutex_unlock(&usb9pfs_lock); + + return 0; +}...+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 ret; + + mutex_lock(&opts->lock); + ret = sprintf(page, "%d\n", opts->buflen);sysfs_emit()?
Good point!
+ mutex_unlock(&opts->lock); + + return ret; +}...+static struct f_usb9pfs_dev *_usb9pfs_do_find_dev(const char *tag) +{ + struct f_usb9pfs_dev *usb9pfs_dev; + struct f_usb9pfs_dev *tmp; + + if (!tag) + return NULL; + + list_for_each_entry_safe(usb9pfs_dev, tmp, &usbg_instance_list, usb9pfs_instance) {Why the _safe version is used here? The list is not modify here directly.
No need for _safe. I switched back to the generic version.
+ if (strcmp(usb9pfs_dev->tag, tag) == 0) + return usb9pfs_dev; + } + + return NULL; +}...+static void usb9pfs_free_instance(struct usb_function_instance *fi) +{ + struct f_usb9pfs_opts *usb9pfs_opts = + container_of(fi, struct f_usb9pfs_opts, func_inst); + struct f_usb9pfs_dev *dev = usb9pfs_opts->dev; + + list_del(&dev->usb9pfs_instance);When it is added in the usbg_instance_list list below, it is protected by the usb9pfs_lock mutex. Should it be also protected when it is removed?
Using the mutex here is valid.
+ kfree(usb9pfs_opts); +} + +static struct usb_function_instance *usb9pfs_alloc_instance(void) +{ + struct f_usb9pfs_opts *usb9pfs_opts; + struct f_usb9pfs_dev *dev; + + usb9pfs_opts = kzalloc(sizeof(*usb9pfs_opts), GFP_KERNEL); + if (!usb9pfs_opts) + return ERR_PTR(-ENOMEM); + + mutex_init(&usb9pfs_opts->lock); + + usb9pfs_opts->func_inst.set_inst_name = usb9pfs_set_inst_tag; + usb9pfs_opts->func_inst.free_func_inst = usb9pfs_free_instance; + + usb9pfs_opts->buflen = DEFAULT_BUFLEN; + + mutex_lock(&usb9pfs_lock); + dev = kzalloc(sizeof(*dev), GFP_KERNEL);If kzalloc() was called outside of the mutex, it would be slightly simpler, IMHO.
Totally. I do the alloc and ..
+ if (IS_ERR(dev)) { + mutex_unlock(&usb9pfs_lock); + kfree(usb9pfs_opts); + return ERR_CAST(dev); + } + list_add_tail(&dev->usb9pfs_instance, &usbg_instance_list); + mutex_unlock(&usb9pfs_lock); + + usb9pfs_opts->dev = dev; + dev->opts = usb9pfs_opts;'dev' is added to the usbg_instance_list list within a mutex section, so it looks possible that this list is also accessed from somewhere else.Would it make sense to initialize dev->opts also within the mutex section, to avoid concurrent access to this new item, when dev->opts is still NULL?
.. the allocation all before even adding the prepared instance to the list. This should make it safe. I actually moved the list alternation to be the last operation in this function.
+ + config_group_init_type_name(&usb9pfs_opts->func_inst.group, "", + &usb9pfs_func_type); + + return &usb9pfs_opts->func_inst; +}... CJ
-- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
