diff mbox series

[v2,3/4] usb: gadget: legacy: add 9pfs multi gadget

Message ID 20240116-ml-topic-u9p-v2-3-b46cbf592962@pengutronix.de (mailing list archive)
State Superseded
Headers show
Series usb: gadget: 9pfs transport | expand

Commit Message

Michael Grzeschik Feb. 2, 2024, 12:05 a.m. UTC
Add the newly introduced 9pfs transport gadget interface with an new
multi composed gadget together with acm and eem.

When using this legacy module, it is also possible to
mount the 9PFS usb dir as root filesystem. Just follow the
instrucitons from Documentation/filesystems/9p.rst

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---
v1 -> v2:
  - deleted the usbg 9pfs detailed instruction from commit message
  - added depends on net for NET_9P dependency
---
 drivers/usb/gadget/legacy/9pfs.c   | 268 +++++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/legacy/Kconfig  |  16 +++
 drivers/usb/gadget/legacy/Makefile |   2 +
 3 files changed, 286 insertions(+)

Comments

Greg Kroah-Hartman Feb. 17, 2024, 3:59 p.m. UTC | #1
On Fri, Feb 02, 2024 at 01:05:12AM +0100, Michael Grzeschik wrote:
> Add the newly introduced 9pfs transport gadget interface with an new
> multi composed gadget together with acm and eem.
> 
> When using this legacy module, it is also possible to
> mount the 9PFS usb dir as root filesystem. Just follow the
> instrucitons from Documentation/filesystems/9p.rst

Why are we adding new "legacy" gadgets?  What's wrong with the "correct"
api instead?  You need a lot of justification here to add something to
an api we want to one day just delete.

And can you wrap your changelog at 72 columns?


> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> 
> ---
> v1 -> v2:
>   - deleted the usbg 9pfs detailed instruction from commit message
>   - added depends on net for NET_9P dependency
> ---
>  drivers/usb/gadget/legacy/9pfs.c   | 268 +++++++++++++++++++++++++++++++++++++
>  drivers/usb/gadget/legacy/Kconfig  |  16 +++
>  drivers/usb/gadget/legacy/Makefile |   2 +
>  3 files changed, 286 insertions(+)
> 
> diff --git a/drivers/usb/gadget/legacy/9pfs.c b/drivers/usb/gadget/legacy/9pfs.c
> new file mode 100644
> index 0000000000000..3ac7f2e92c5a3
> --- /dev/null
> +++ b/drivers/usb/gadget/legacy/9pfs.c
> @@ -0,0 +1,268 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * usb9pfs.c -- Gadget usb9pfs
> + *
> + * Copyright (C) 2023 Michael Grzeschik
> + */
> +
> +/*
> + * Gadget usb9pfs only needs two bulk endpoints, and will use the usb9pfs usb
> + * transport to mount host filesystem via usb gadget. This driver will
> + * also add one ACM and NCM interface.

Why "also"?  What are those interfaces going to be used for and what do
they have to do with 9pfs?

> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/usb/composite.h>
> +#include <linux/netdevice.h>
> +
> +#include "u_eem.h"
> +#include "u_ether.h"
> +
> +/*-------------------------------------------------------------------------*/
> +USB_GADGET_COMPOSITE_OPTIONS();
> +
> +USB_ETHERNET_MODULE_PARAMETERS();
> +
> +/* Defines */
> +
> +#define DRIVER_VERSION_STR "v1.0"
> +#define DRIVER_VERSION_NUM 0x1000
> +
> +#define DRIVER_DESC	"Composite Gadget (9P + ACM + NCM)"
> +
> +/*-------------------------------------------------------------------------*/
> +
> +#define DRIVER_VENDOR_NUM	0x1d6b		/* Linux Foundation */
> +#define DRIVER_PRODUCT_NUM	0x0109		/* Linux-USB 9PFS Gadget */
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static struct usb_device_descriptor device_desc = {
> +	.bLength =		sizeof(device_desc),
> +	.bDescriptorType =	USB_DT_DEVICE,
> +
> +	/* .bcdUSB = DYNAMIC */
> +
> +	.bDeviceClass =		USB_CLASS_MISC,
> +	.bDeviceSubClass =	2,
> +	.bDeviceProtocol =	1,
> +
> +	/* .bMaxPacketSize0 = f(hardware) */
> +
> +	/* Vendor and product id can be overridden by module parameters.  */
> +	.idVendor =		cpu_to_le16(DRIVER_VENDOR_NUM),
> +	.idProduct =		cpu_to_le16(DRIVER_PRODUCT_NUM),
> +	/* .bcdDevice = f(hardware) */
> +	/* .iManufacturer = DYNAMIC */
> +	/* .iProduct = DYNAMIC */
> +	/* NO SERIAL NUMBER */
> +	/*.bNumConfigurations =	DYNAMIC*/
> +};
> +
> +static const struct usb_descriptor_header *otg_desc[2];
> +
> +static struct usb_string strings_dev[] = {
> +	[USB_GADGET_MANUFACTURER_IDX].s = "",
> +	[USB_GADGET_PRODUCT_IDX].s = DRIVER_DESC,
> +	[USB_GADGET_SERIAL_IDX].s = "",
> +	{  }			/* end of list */
> +};
> +
> +static struct usb_gadget_strings stringtab_dev = {
> +	.language	= 0x0409,	/* en-us */
> +	.strings	= strings_dev,
> +};
> +
> +static struct usb_gadget_strings *dev_strings[] = {
> +	&stringtab_dev,
> +	NULL,
> +};
> +
> +static struct usb_configuration cdc_driver_conf = {
> +	.label          = DRIVER_DESC,
> +	.bConfigurationValue = 1,
> +	/* .iConfiguration = DYNAMIC */
> +	.bmAttributes   = USB_CONFIG_ATT_SELFPOWER,
> +};
> +
> +static struct usb_function *f_9pfs;
> +static struct usb_function_instance *fi_9pfs;
> +
> +static struct usb_function *f_acm;
> +static struct usb_function_instance *fi_acm;
> +
> +static struct usb_function *f_eem;
> +static struct usb_function_instance *fi_eem;
> +
> +static int cdc_do_config(struct usb_configuration *c)
> +{
> +	int ret;
> +
> +	if (gadget_is_otg(c->cdev->gadget)) {
> +		c->descriptors = otg_desc;
> +		c->bmAttributes |= USB_CONFIG_ATT_WAKEUP;
> +	}
> +
> +	f_9pfs = usb_get_function(fi_9pfs);
> +	if (IS_ERR(f_9pfs))
> +		return PTR_ERR(f_9pfs);
> +
> +	ret = usb_add_function(c, f_9pfs);
> +	if (ret < 0)
> +		goto err_func_9pfs;
> +
> +	f_acm = usb_get_function(fi_acm);
> +	if (IS_ERR(f_acm)) {
> +		ret = PTR_ERR(f_acm);
> +		goto err_func_acm;
> +	}
> +
> +	ret = usb_add_function(c, f_acm);
> +	if (ret)
> +		goto err_conf;
> +
> +	f_eem = usb_get_function(fi_eem);
> +	if (IS_ERR(f_eem)) {
> +		ret = PTR_ERR(f_eem);
> +		goto err_eem;
> +	}
> +
> +	ret = usb_add_function(c, f_eem);
> +	if (ret)
> +		goto err_run;
> +
> +	return 0;
> +err_run:
> +	usb_put_function(f_eem);
> +err_eem:
> +	usb_remove_function(c, f_acm);
> +err_conf:
> +	usb_put_function(f_acm);
> +err_func_acm:
> +	usb_remove_function(c, f_9pfs);
> +err_func_9pfs:
> +	usb_put_function(f_9pfs);
> +	return ret;
> +}
> +
> +static int usb9pfs_bind(struct usb_composite_dev *cdev)
> +{
> +	struct f_eem_opts	*eem_opts = NULL;
> +	int status;
> +
> +	fi_9pfs = usb_get_function_instance("usb9pfs");
> +	if (IS_ERR(fi_9pfs)) {
> +		if (PTR_ERR(fi_9pfs) == -ENOENT)
> +			return -EPROBE_DEFER;
> +		return PTR_ERR(fi_9pfs);
> +	}
> +
> +	/* set up serial link layer */
> +	fi_acm = usb_get_function_instance("acm");
> +	if (IS_ERR(fi_acm)) {
> +		if (PTR_ERR(fi_9pfs) == -ENOENT)
> +			return -EPROBE_DEFER;
> +		status = PTR_ERR(fi_acm);
> +		goto err_conf_acm;
> +	}
> +
> +	fi_eem = usb_get_function_instance("eem");
> +	if (IS_ERR(fi_eem)) {
> +		if (PTR_ERR(fi_9pfs) == -ENOENT)
> +			return -EPROBE_DEFER;
> +		status = PTR_ERR(fi_eem);
> +		goto err_conf_eem;
> +	}
> +
> +	eem_opts = container_of(fi_eem, struct f_eem_opts, func_inst);
> +
> +	gether_set_qmult(eem_opts->net, qmult);
> +	if (!gether_set_host_addr(eem_opts->net, host_addr))
> +		pr_info("using host ethernet address: %s", host_addr);
> +	if (!gether_set_dev_addr(eem_opts->net, dev_addr))
> +		pr_info("using self ethernet address: %s", dev_addr);
> +
> +	/* Allocate string descriptor numbers ... note that string
> +	 * contents can be overridden by the composite_dev glue.
> +	 */
> +	status = usb_string_ids_tab(cdev, strings_dev);
> +	if (status < 0)
> +		return status;
> +
> +	device_desc.iManufacturer = strings_dev[USB_GADGET_MANUFACTURER_IDX].id;
> +	device_desc.iProduct = strings_dev[USB_GADGET_PRODUCT_IDX].id;
> +	device_desc.iSerialNumber = strings_dev[USB_GADGET_SERIAL_IDX].id;
> +
> +	/* support OTG systems */
> +	if (gadget_is_otg(cdev->gadget)) {
> +		if (!otg_desc[0]) {
> +			struct usb_descriptor_header *usb_desc;
> +
> +			usb_desc = usb_otg_descriptor_alloc(cdev->gadget);
> +			if (!usb_desc) {
> +				status = -ENOMEM;
> +				goto err_conf_otg;
> +			}
> +			usb_otg_descriptor_init(cdev->gadget, usb_desc);
> +			otg_desc[0] = usb_desc;
> +			otg_desc[1] = NULL;
> +		}
> +	}
> +
> +	status = usb_add_config(cdev, &cdc_driver_conf, cdc_do_config);
> +	if (status)
> +		goto err_free_otg_desc;
> +
> +	usb_ep_autoconfig_reset(cdev->gadget);
> +	usb_composite_overwrite_options(cdev, &coverwrite);
> +
> +	dev_info(&cdev->gadget->dev, DRIVER_DESC " version: " DRIVER_VERSION_STR "\n");

When drivers are working properly, they are quiet.

thanks,

greg k-h
Michael Grzeschik Feb. 19, 2024, 1:48 a.m. UTC | #2
On Sat, Feb 17, 2024 at 04:59:28PM +0100, Greg Kroah-Hartman wrote:
>On Fri, Feb 02, 2024 at 01:05:12AM +0100, Michael Grzeschik wrote:
>> Add the newly introduced 9pfs transport gadget interface with an new
>> multi composed gadget together with acm and eem.
>>
>> When using this legacy module, it is also possible to
>> mount the 9PFS usb dir as root filesystem. Just follow the
>> instrucitons from Documentation/filesystems/9p.rst
>
>Why are we adding new "legacy" gadgets?  What's wrong with the "correct"
>api instead?  You need a lot of justification here to add something to
>an api we want to one day just delete.

Without the legacy gadget there is no real solution to mount
the 9pfs via the gadget as rootfs. The "correct" api is configfs
which will need the user to have some filesystem to mount it to.

There is the relatively new concept of bootconfig which sounds
promising to describe an complete configfs tree from system boot.

However this is some future talk for now, so we would like to
stick with the legacy setup to be able to mount the 9pfs rootfs.

I will improve the commit message to make this clear.

>And can you wrap your changelog at 72 columns?

Sure

>
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>
>> ---
>> v1 -> v2:
>>   - deleted the usbg 9pfs detailed instruction from commit message
>>   - added depends on net for NET_9P dependency
>> ---
>>  drivers/usb/gadget/legacy/9pfs.c   | 268 +++++++++++++++++++++++++++++++++++++
>>  drivers/usb/gadget/legacy/Kconfig  |  16 +++
>>  drivers/usb/gadget/legacy/Makefile |   2 +
>>  3 files changed, 286 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/legacy/9pfs.c b/drivers/usb/gadget/legacy/9pfs.c
>> new file mode 100644
>> index 0000000000000..3ac7f2e92c5a3
>> --- /dev/null
>> +++ b/drivers/usb/gadget/legacy/9pfs.c
>> @@ -0,0 +1,268 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * usb9pfs.c -- Gadget usb9pfs
>> + *
>> + * Copyright (C) 2023 Michael Grzeschik
>> + */
>> +
>> +/*
>> + * Gadget usb9pfs only needs two bulk endpoints, and will use the usb9pfs usb
>> + * transport to mount host filesystem via usb gadget. This driver will
>> + * also add one ACM and NCM interface.
>
>Why "also"?  What are those interfaces going to be used for and what do
>they have to do with 9pfs?

They are not necessary to be used with 9pfs. But since we introduce an
new legacy module which is fully claiming the UDC, it would make sense
to leave the other endpoints unavailable but instead add some common
interfaces like ecm and acm.

I will also improve the comment and the commit message to point this
out.

>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/usb/composite.h>
>> +#include <linux/netdevice.h>
>> +
>> +#include "u_eem.h"
>> +#include "u_ether.h"
>> +
>> +/*-------------------------------------------------------------------------*/
>> +USB_GADGET_COMPOSITE_OPTIONS();
>> +
>> +USB_ETHERNET_MODULE_PARAMETERS();
>> +
>> +/* Defines */
>> +
>> +#define DRIVER_VERSION_STR "v1.0"
>> +#define DRIVER_VERSION_NUM 0x1000
>> +
>> +#define DRIVER_DESC	"Composite Gadget (9P + ACM + NCM)"
>> +
>> +/*-------------------------------------------------------------------------*/
>> +
>> +#define DRIVER_VENDOR_NUM	0x1d6b		/* Linux Foundation */
>> +#define DRIVER_PRODUCT_NUM	0x0109		/* Linux-USB 9PFS Gadget */
>> +
>> +/*-------------------------------------------------------------------------*/
>> +
>> +static struct usb_device_descriptor device_desc = {
>> +	.bLength =		sizeof(device_desc),
>> +	.bDescriptorType =	USB_DT_DEVICE,
>> +
>> +	/* .bcdUSB = DYNAMIC */
>> +
>> +	.bDeviceClass =		USB_CLASS_MISC,
>> +	.bDeviceSubClass =	2,
>> +	.bDeviceProtocol =	1,
>> +
>> +	/* .bMaxPacketSize0 = f(hardware) */
>> +
>> +	/* Vendor and product id can be overridden by module parameters.  */
>> +	.idVendor =		cpu_to_le16(DRIVER_VENDOR_NUM),
>> +	.idProduct =		cpu_to_le16(DRIVER_PRODUCT_NUM),
>> +	/* .bcdDevice = f(hardware) */
>> +	/* .iManufacturer = DYNAMIC */
>> +	/* .iProduct = DYNAMIC */
>> +	/* NO SERIAL NUMBER */
>> +	/*.bNumConfigurations =	DYNAMIC*/
>> +};
>> +
>> +static const struct usb_descriptor_header *otg_desc[2];
>> +
>> +static struct usb_string strings_dev[] = {
>> +	[USB_GADGET_MANUFACTURER_IDX].s = "",
>> +	[USB_GADGET_PRODUCT_IDX].s = DRIVER_DESC,
>> +	[USB_GADGET_SERIAL_IDX].s = "",
>> +	{  }			/* end of list */
>> +};
>> +
>> +static struct usb_gadget_strings stringtab_dev = {
>> +	.language	= 0x0409,	/* en-us */
>> +	.strings	= strings_dev,
>> +};
>> +
>> +static struct usb_gadget_strings *dev_strings[] = {
>> +	&stringtab_dev,
>> +	NULL,
>> +};
>> +
>> +static struct usb_configuration cdc_driver_conf = {
>> +	.label          = DRIVER_DESC,
>> +	.bConfigurationValue = 1,
>> +	/* .iConfiguration = DYNAMIC */
>> +	.bmAttributes   = USB_CONFIG_ATT_SELFPOWER,
>> +};
>> +
>> +static struct usb_function *f_9pfs;
>> +static struct usb_function_instance *fi_9pfs;
>> +
>> +static struct usb_function *f_acm;
>> +static struct usb_function_instance *fi_acm;
>> +
>> +static struct usb_function *f_eem;
>> +static struct usb_function_instance *fi_eem;
>> +
>> +static int cdc_do_config(struct usb_configuration *c)
>> +{
>> +	int ret;
>> +
>> +	if (gadget_is_otg(c->cdev->gadget)) {
>> +		c->descriptors = otg_desc;
>> +		c->bmAttributes |= USB_CONFIG_ATT_WAKEUP;
>> +	}
>> +
>> +	f_9pfs = usb_get_function(fi_9pfs);
>> +	if (IS_ERR(f_9pfs))
>> +		return PTR_ERR(f_9pfs);
>> +
>> +	ret = usb_add_function(c, f_9pfs);
>> +	if (ret < 0)
>> +		goto err_func_9pfs;
>> +
>> +	f_acm = usb_get_function(fi_acm);
>> +	if (IS_ERR(f_acm)) {
>> +		ret = PTR_ERR(f_acm);
>> +		goto err_func_acm;
>> +	}
>> +
>> +	ret = usb_add_function(c, f_acm);
>> +	if (ret)
>> +		goto err_conf;
>> +
>> +	f_eem = usb_get_function(fi_eem);
>> +	if (IS_ERR(f_eem)) {
>> +		ret = PTR_ERR(f_eem);
>> +		goto err_eem;
>> +	}
>> +
>> +	ret = usb_add_function(c, f_eem);
>> +	if (ret)
>> +		goto err_run;
>> +
>> +	return 0;
>> +err_run:
>> +	usb_put_function(f_eem);
>> +err_eem:
>> +	usb_remove_function(c, f_acm);
>> +err_conf:
>> +	usb_put_function(f_acm);
>> +err_func_acm:
>> +	usb_remove_function(c, f_9pfs);
>> +err_func_9pfs:
>> +	usb_put_function(f_9pfs);
>> +	return ret;
>> +}
>> +
>> +static int usb9pfs_bind(struct usb_composite_dev *cdev)
>> +{
>> +	struct f_eem_opts	*eem_opts = NULL;
>> +	int status;
>> +
>> +	fi_9pfs = usb_get_function_instance("usb9pfs");
>> +	if (IS_ERR(fi_9pfs)) {
>> +		if (PTR_ERR(fi_9pfs) == -ENOENT)
>> +			return -EPROBE_DEFER;
>> +		return PTR_ERR(fi_9pfs);
>> +	}
>> +
>> +	/* set up serial link layer */
>> +	fi_acm = usb_get_function_instance("acm");
>> +	if (IS_ERR(fi_acm)) {
>> +		if (PTR_ERR(fi_9pfs) == -ENOENT)
>> +			return -EPROBE_DEFER;
>> +		status = PTR_ERR(fi_acm);
>> +		goto err_conf_acm;
>> +	}
>> +
>> +	fi_eem = usb_get_function_instance("eem");
>> +	if (IS_ERR(fi_eem)) {
>> +		if (PTR_ERR(fi_9pfs) == -ENOENT)
>> +			return -EPROBE_DEFER;
>> +		status = PTR_ERR(fi_eem);
>> +		goto err_conf_eem;
>> +	}
>> +
>> +	eem_opts = container_of(fi_eem, struct f_eem_opts, func_inst);
>> +
>> +	gether_set_qmult(eem_opts->net, qmult);
>> +	if (!gether_set_host_addr(eem_opts->net, host_addr))
>> +		pr_info("using host ethernet address: %s", host_addr);
>> +	if (!gether_set_dev_addr(eem_opts->net, dev_addr))
>> +		pr_info("using self ethernet address: %s", dev_addr);
>> +
>> +	/* Allocate string descriptor numbers ... note that string
>> +	 * contents can be overridden by the composite_dev glue.
>> +	 */
>> +	status = usb_string_ids_tab(cdev, strings_dev);
>> +	if (status < 0)
>> +		return status;
>> +
>> +	device_desc.iManufacturer = strings_dev[USB_GADGET_MANUFACTURER_IDX].id;
>> +	device_desc.iProduct = strings_dev[USB_GADGET_PRODUCT_IDX].id;
>> +	device_desc.iSerialNumber = strings_dev[USB_GADGET_SERIAL_IDX].id;
>> +
>> +	/* support OTG systems */
>> +	if (gadget_is_otg(cdev->gadget)) {
>> +		if (!otg_desc[0]) {
>> +			struct usb_descriptor_header *usb_desc;
>> +
>> +			usb_desc = usb_otg_descriptor_alloc(cdev->gadget);
>> +			if (!usb_desc) {
>> +				status = -ENOMEM;
>> +				goto err_conf_otg;
>> +			}
>> +			usb_otg_descriptor_init(cdev->gadget, usb_desc);
>> +			otg_desc[0] = usb_desc;
>> +			otg_desc[1] = NULL;
>> +		}
>> +	}
>> +
>> +	status = usb_add_config(cdev, &cdc_driver_conf, cdc_do_config);
>> +	if (status)
>> +		goto err_free_otg_desc;
>> +
>> +	usb_ep_autoconfig_reset(cdev->gadget);
>> +	usb_composite_overwrite_options(cdev, &coverwrite);
>> +
>> +	dev_info(&cdev->gadget->dev, DRIVER_DESC " version: " DRIVER_VERSION_STR "\n");
>
>When drivers are working properly, they are quiet.

Right, I will fix that.

Thanks,
Michael
Greg Kroah-Hartman Feb. 19, 2024, 8:13 a.m. UTC | #3
On Mon, Feb 19, 2024 at 02:48:43AM +0100, Michael Grzeschik wrote:
> On Sat, Feb 17, 2024 at 04:59:28PM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Feb 02, 2024 at 01:05:12AM +0100, Michael Grzeschik wrote:
> > > Add the newly introduced 9pfs transport gadget interface with an new
> > > multi composed gadget together with acm and eem.
> > > 
> > > When using this legacy module, it is also possible to
> > > mount the 9PFS usb dir as root filesystem. Just follow the
> > > instrucitons from Documentation/filesystems/9p.rst
> > 
> > Why are we adding new "legacy" gadgets?  What's wrong with the "correct"
> > api instead?  You need a lot of justification here to add something to
> > an api we want to one day just delete.
> 
> Without the legacy gadget there is no real solution to mount
> the 9pfs via the gadget as rootfs. The "correct" api is configfs
> which will need the user to have some filesystem to mount it to.

That's what your initramfs is for.  Why can't you just use that?

> There is the relatively new concept of bootconfig which sounds
> promising to describe an complete configfs tree from system boot.

Great, but until that happens, again, just use initramfs.

> However this is some future talk for now, so we would like to
> stick with the legacy setup to be able to mount the 9pfs rootfs.

I'd prefer to NOT add new legacy gadget drivers, and do everything
possible to delete them all from the tree "soon".

> > > +/*
> > > + * Gadget usb9pfs only needs two bulk endpoints, and will use the usb9pfs usb
> > > + * transport to mount host filesystem via usb gadget. This driver will
> > > + * also add one ACM and NCM interface.
> > 
> > Why "also"?  What are those interfaces going to be used for and what do
> > they have to do with 9pfs?
> 
> They are not necessary to be used with 9pfs. But since we introduce an
> new legacy module which is fully claiming the UDC, it would make sense
> to leave the other endpoints unavailable but instead add some common
> interfaces like ecm and acm.

But if no one needs/wants them, why make this complex?  Again, configfs
can handle the composition of this if you need it, which is why that
"new" interface was created.

thanks,

greg k-h
Michael Grzeschik Feb. 19, 2024, 8:02 p.m. UTC | #4
On Mon, Feb 19, 2024 at 09:13:03AM +0100, Greg Kroah-Hartman wrote:
>On Mon, Feb 19, 2024 at 02:48:43AM +0100, Michael Grzeschik wrote:
>> On Sat, Feb 17, 2024 at 04:59:28PM +0100, Greg Kroah-Hartman wrote:
>> > On Fri, Feb 02, 2024 at 01:05:12AM +0100, Michael Grzeschik wrote:
>> > > Add the newly introduced 9pfs transport gadget interface with an new
>> > > multi composed gadget together with acm and eem.
>> > >
>> > > When using this legacy module, it is also possible to
>> > > mount the 9PFS usb dir as root filesystem. Just follow the
>> > > instrucitons from Documentation/filesystems/9p.rst
>> >
>> > Why are we adding new "legacy" gadgets?  What's wrong with the "correct"
>> > api instead?  You need a lot of justification here to add something to
>> > an api we want to one day just delete.
>>
>> Without the legacy gadget there is no real solution to mount
>> the 9pfs via the gadget as rootfs. The "correct" api is configfs
>> which will need the user to have some filesystem to mount it to.
>
>That's what your initramfs is for.  Why can't you just use that?

Because out of convenience and to make it just work as simple as
nfsrootfs. With this we now need to maintain an full initramfs,
that has to come from somewhere.

>> There is the relatively new concept of bootconfig which sounds
>> promising to describe an complete configfs tree from system boot.
>
>Great, but until that happens, again, just use initramfs.

Hmm, okay.

>> However this is some future talk for now, so we would like to
>> stick with the legacy setup to be able to mount the 9pfs rootfs.
>
>I'd prefer to NOT add new legacy gadget drivers, and do everything
>possible to delete them all from the tree "soon".

When you mean this "soon" is in the near future, you are probably right
then.

>> > > +/*
>> > > + * Gadget usb9pfs only needs two bulk endpoints, and will use the usb9pfs usb
>> > > + * transport to mount host filesystem via usb gadget. This driver will
>> > > + * also add one ACM and NCM interface.
>> >
>> > Why "also"?  What are those interfaces going to be used for and what do
>> > they have to do with 9pfs?
>>
>> They are not necessary to be used with 9pfs. But since we introduce an
>> new legacy module which is fully claiming the UDC, it would make sense
>> to leave the other endpoints unavailable but instead add some common
>> interfaces like ecm and acm.
>
>But if no one needs/wants them, why make this complex?  Again, configfs
>can handle the composition of this if you need it, which is why that
>"new" interface was created.


Okay, What about the rest of the series? Can you just skip this patch
then for? Or do you want me to send the series again without this
legacy driver. There are no dependencies to this in that series.

Michael
Greg Kroah-Hartman Feb. 19, 2024, 8:07 p.m. UTC | #5
On Mon, Feb 19, 2024 at 09:02:54PM +0100, Michael Grzeschik wrote:
> Okay, What about the rest of the series? Can you just skip this patch
> then for? Or do you want me to send the series again without this
> legacy driver. There are no dependencies to this in that series.

I don't remember what the rest of this series was, it is long gone from
my review queue :(

So yes, please resend.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/gadget/legacy/9pfs.c b/drivers/usb/gadget/legacy/9pfs.c
new file mode 100644
index 0000000000000..3ac7f2e92c5a3
--- /dev/null
+++ b/drivers/usb/gadget/legacy/9pfs.c
@@ -0,0 +1,268 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * usb9pfs.c -- Gadget usb9pfs
+ *
+ * Copyright (C) 2023 Michael Grzeschik
+ */
+
+/*
+ * Gadget usb9pfs only needs two bulk endpoints, and will use the usb9pfs usb
+ * transport to mount host filesystem via usb gadget. This driver will
+ * also add one ACM and NCM interface.
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/usb/composite.h>
+#include <linux/netdevice.h>
+
+#include "u_eem.h"
+#include "u_ether.h"
+
+/*-------------------------------------------------------------------------*/
+USB_GADGET_COMPOSITE_OPTIONS();
+
+USB_ETHERNET_MODULE_PARAMETERS();
+
+/* Defines */
+
+#define DRIVER_VERSION_STR "v1.0"
+#define DRIVER_VERSION_NUM 0x1000
+
+#define DRIVER_DESC	"Composite Gadget (9P + ACM + NCM)"
+
+/*-------------------------------------------------------------------------*/
+
+#define DRIVER_VENDOR_NUM	0x1d6b		/* Linux Foundation */
+#define DRIVER_PRODUCT_NUM	0x0109		/* Linux-USB 9PFS Gadget */
+
+/*-------------------------------------------------------------------------*/
+
+static struct usb_device_descriptor device_desc = {
+	.bLength =		sizeof(device_desc),
+	.bDescriptorType =	USB_DT_DEVICE,
+
+	/* .bcdUSB = DYNAMIC */
+
+	.bDeviceClass =		USB_CLASS_MISC,
+	.bDeviceSubClass =	2,
+	.bDeviceProtocol =	1,
+
+	/* .bMaxPacketSize0 = f(hardware) */
+
+	/* Vendor and product id can be overridden by module parameters.  */
+	.idVendor =		cpu_to_le16(DRIVER_VENDOR_NUM),
+	.idProduct =		cpu_to_le16(DRIVER_PRODUCT_NUM),
+	/* .bcdDevice = f(hardware) */
+	/* .iManufacturer = DYNAMIC */
+	/* .iProduct = DYNAMIC */
+	/* NO SERIAL NUMBER */
+	/*.bNumConfigurations =	DYNAMIC*/
+};
+
+static const struct usb_descriptor_header *otg_desc[2];
+
+static struct usb_string strings_dev[] = {
+	[USB_GADGET_MANUFACTURER_IDX].s = "",
+	[USB_GADGET_PRODUCT_IDX].s = DRIVER_DESC,
+	[USB_GADGET_SERIAL_IDX].s = "",
+	{  }			/* end of list */
+};
+
+static struct usb_gadget_strings stringtab_dev = {
+	.language	= 0x0409,	/* en-us */
+	.strings	= strings_dev,
+};
+
+static struct usb_gadget_strings *dev_strings[] = {
+	&stringtab_dev,
+	NULL,
+};
+
+static struct usb_configuration cdc_driver_conf = {
+	.label          = DRIVER_DESC,
+	.bConfigurationValue = 1,
+	/* .iConfiguration = DYNAMIC */
+	.bmAttributes   = USB_CONFIG_ATT_SELFPOWER,
+};
+
+static struct usb_function *f_9pfs;
+static struct usb_function_instance *fi_9pfs;
+
+static struct usb_function *f_acm;
+static struct usb_function_instance *fi_acm;
+
+static struct usb_function *f_eem;
+static struct usb_function_instance *fi_eem;
+
+static int cdc_do_config(struct usb_configuration *c)
+{
+	int ret;
+
+	if (gadget_is_otg(c->cdev->gadget)) {
+		c->descriptors = otg_desc;
+		c->bmAttributes |= USB_CONFIG_ATT_WAKEUP;
+	}
+
+	f_9pfs = usb_get_function(fi_9pfs);
+	if (IS_ERR(f_9pfs))
+		return PTR_ERR(f_9pfs);
+
+	ret = usb_add_function(c, f_9pfs);
+	if (ret < 0)
+		goto err_func_9pfs;
+
+	f_acm = usb_get_function(fi_acm);
+	if (IS_ERR(f_acm)) {
+		ret = PTR_ERR(f_acm);
+		goto err_func_acm;
+	}
+
+	ret = usb_add_function(c, f_acm);
+	if (ret)
+		goto err_conf;
+
+	f_eem = usb_get_function(fi_eem);
+	if (IS_ERR(f_eem)) {
+		ret = PTR_ERR(f_eem);
+		goto err_eem;
+	}
+
+	ret = usb_add_function(c, f_eem);
+	if (ret)
+		goto err_run;
+
+	return 0;
+err_run:
+	usb_put_function(f_eem);
+err_eem:
+	usb_remove_function(c, f_acm);
+err_conf:
+	usb_put_function(f_acm);
+err_func_acm:
+	usb_remove_function(c, f_9pfs);
+err_func_9pfs:
+	usb_put_function(f_9pfs);
+	return ret;
+}
+
+static int usb9pfs_bind(struct usb_composite_dev *cdev)
+{
+	struct f_eem_opts	*eem_opts = NULL;
+	int status;
+
+	fi_9pfs = usb_get_function_instance("usb9pfs");
+	if (IS_ERR(fi_9pfs)) {
+		if (PTR_ERR(fi_9pfs) == -ENOENT)
+			return -EPROBE_DEFER;
+		return PTR_ERR(fi_9pfs);
+	}
+
+	/* set up serial link layer */
+	fi_acm = usb_get_function_instance("acm");
+	if (IS_ERR(fi_acm)) {
+		if (PTR_ERR(fi_9pfs) == -ENOENT)
+			return -EPROBE_DEFER;
+		status = PTR_ERR(fi_acm);
+		goto err_conf_acm;
+	}
+
+	fi_eem = usb_get_function_instance("eem");
+	if (IS_ERR(fi_eem)) {
+		if (PTR_ERR(fi_9pfs) == -ENOENT)
+			return -EPROBE_DEFER;
+		status = PTR_ERR(fi_eem);
+		goto err_conf_eem;
+	}
+
+	eem_opts = container_of(fi_eem, struct f_eem_opts, func_inst);
+
+	gether_set_qmult(eem_opts->net, qmult);
+	if (!gether_set_host_addr(eem_opts->net, host_addr))
+		pr_info("using host ethernet address: %s", host_addr);
+	if (!gether_set_dev_addr(eem_opts->net, dev_addr))
+		pr_info("using self ethernet address: %s", dev_addr);
+
+	/* Allocate string descriptor numbers ... note that string
+	 * contents can be overridden by the composite_dev glue.
+	 */
+	status = usb_string_ids_tab(cdev, strings_dev);
+	if (status < 0)
+		return status;
+
+	device_desc.iManufacturer = strings_dev[USB_GADGET_MANUFACTURER_IDX].id;
+	device_desc.iProduct = strings_dev[USB_GADGET_PRODUCT_IDX].id;
+	device_desc.iSerialNumber = strings_dev[USB_GADGET_SERIAL_IDX].id;
+
+	/* support OTG systems */
+	if (gadget_is_otg(cdev->gadget)) {
+		if (!otg_desc[0]) {
+			struct usb_descriptor_header *usb_desc;
+
+			usb_desc = usb_otg_descriptor_alloc(cdev->gadget);
+			if (!usb_desc) {
+				status = -ENOMEM;
+				goto err_conf_otg;
+			}
+			usb_otg_descriptor_init(cdev->gadget, usb_desc);
+			otg_desc[0] = usb_desc;
+			otg_desc[1] = NULL;
+		}
+	}
+
+	status = usb_add_config(cdev, &cdc_driver_conf, cdc_do_config);
+	if (status)
+		goto err_free_otg_desc;
+
+	usb_ep_autoconfig_reset(cdev->gadget);
+	usb_composite_overwrite_options(cdev, &coverwrite);
+
+	dev_info(&cdev->gadget->dev, DRIVER_DESC " version: " DRIVER_VERSION_STR "\n");
+
+	return 0;
+
+err_free_otg_desc:
+	kfree(otg_desc[0]);
+	otg_desc[0] = NULL;
+err_conf_otg:
+	usb_put_function_instance(fi_eem);
+err_conf_eem:
+	usb_put_function_instance(fi_acm);
+err_conf_acm:
+	usb_put_function_instance(fi_9pfs);
+	return status;
+}
+
+static int usb9pfs_unbind(struct usb_composite_dev *cdev)
+{
+	if (!IS_ERR_OR_NULL(f_eem))
+		usb_put_function(f_eem);
+	usb_put_function_instance(fi_eem);
+	if (!IS_ERR_OR_NULL(f_acm))
+		usb_put_function(f_acm);
+	usb_put_function_instance(fi_acm);
+	if (!IS_ERR_OR_NULL(f_9pfs))
+		usb_put_function(f_9pfs);
+	usb_put_function_instance(fi_9pfs);
+	kfree(otg_desc[0]);
+	otg_desc[0] = NULL;
+
+	return 0;
+}
+
+static struct usb_composite_driver usb9pfs_driver = {
+	.name		= "usb9pfs",
+	.dev		= &device_desc,
+	.strings	= dev_strings,
+	.max_speed	= USB_SPEED_SUPER,
+	.bind		= usb9pfs_bind,
+	.unbind		= usb9pfs_unbind,
+};
+
+module_usb_composite_driver(usb9pfs_driver);
+
+MODULE_AUTHOR("Michael Grzeschik");
+MODULE_LICENSE("GPL");
diff --git a/drivers/usb/gadget/legacy/Kconfig b/drivers/usb/gadget/legacy/Kconfig
index 0a7b382fbe27c..8b2aab155e0cf 100644
--- a/drivers/usb/gadget/legacy/Kconfig
+++ b/drivers/usb/gadget/legacy/Kconfig
@@ -62,6 +62,22 @@  config USB_ZERO
 	  Say "y" to link the driver statically, or "m" to build a
 	  dynamically linked module called "g_zero".
 
+config USB_9PFS
+	tristate "Gadget 9PFS"
+	depends on NET
+	select USB_LIBCOMPOSITE
+	select USB_U_SERIAL
+	select USB_U_ETHER
+	select USB_F_ACM
+	select USB_F_EEM
+	select NET_9P_USBG
+	select NET_9P
+	help
+	  9PFS Transport Layer
+
+	  Say "y" to link the driver statically, or "m" to build a
+	  dynamically linked module called "g_9pfs".
+
 config USB_ZERO_HNPTEST
 	bool "HNP Test Device"
 	depends on USB_ZERO && USB_OTG
diff --git a/drivers/usb/gadget/legacy/Makefile b/drivers/usb/gadget/legacy/Makefile
index 4d864bf82799d..99c098800d898 100644
--- a/drivers/usb/gadget/legacy/Makefile
+++ b/drivers/usb/gadget/legacy/Makefile
@@ -8,6 +8,7 @@  ccflags-y			+= -I$(srctree)/drivers/usb/gadget/udc/
 ccflags-y			+= -I$(srctree)/drivers/usb/gadget/function/
 
 g_zero-y			:= zero.o
+g_9pfs-y			:= 9pfs.o
 g_audio-y			:= audio.o
 g_ether-y			:= ether.o
 g_serial-y			:= serial.o
@@ -26,6 +27,7 @@  g_acm_ms-y			:= acm_ms.o
 g_tcm_usb_gadget-y		:= tcm_usb_gadget.o
 
 obj-$(CONFIG_USB_ZERO)		+= g_zero.o
+obj-$(CONFIG_USB_9PFS)		+= g_9pfs.o
 obj-$(CONFIG_USB_AUDIO)		+= g_audio.o
 obj-$(CONFIG_USB_ETH)		+= g_ether.o
 obj-$(CONFIG_USB_GADGETFS)	+= gadgetfs.o