Re: [PATCH 1/3] usb: gadget: function: 9pfs

From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
To: Michael Grzeschik <m.grzeschik@pengutronix.de>,
	Eric Van Hensbergen <ericvh@kernel.org>,
	Latchesar Ionkov <lucho@ionkov.net>,
	Dominique Martinet <asmadeus@codewreck.org>,
	Christian Schoenebeck <linux_oss@crudebyte.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: v9fs@lists.linux.dev, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
	kernel@pengutronix.de
Subject: Re: [PATCH 1/3] usb: gadget: function: 9pfs
Date: Tue, 16 Jan 2024 22:14:10 +0100	[thread overview]
Message-ID: <de0bd3d3-bb7b-4f94-aae2-0d9323b273fb@wanadoo.fr> (raw)
In-Reply-To: <20240116-ml-topic-u9p-v1-1-ad8c306f9a4e@pengutronix.de>

Le 16/01/2024 à 02:49, 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.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>   Documentation/filesystems/9p.rst     |  12 +
>   drivers/usb/gadget/Kconfig           |  11 +
>   drivers/usb/gadget/function/Makefile |   2 +
>   drivers/usb/gadget/function/f_9pfs.c | 849 +++++++++++++++++++++++++++++++++++
>   4 files changed, 874 insertions(+)

...

> +static int alloc_requests(struct usb_composite_dev *cdev,
> +			  struct f_usb9pfs *usb9pfs)
> +{
> +	int result = 0;
> +
> +	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->out_req->context = usb9pfs;
> +
> +	return 0;
> +
> +fail_in:
> +	usb_ep_free_request(usb9pfs->in_ep, usb9pfs->in_req);
> +fail:
> +	return result;

'result' is never overwritten, so we always return 0.
Is it on purpose?

> +}

...

> +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_simple_get(&usb9pfs_ida, 0, 100, GFP_KERNEL);

This API will soon be removed.
Please use ida_alloc_max() instead.

Also, there is no corresponding ida_free()? (or isa_simple_remove())

> +	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);

Same here, it seems to be freed nowhere?
I think that both should be added in usb9pfs_free_func().

CJ

> +
> +	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;
> +}

...