Re: [PATCH 0/3] usb: gadget: 9pfs transport

From: Dominique Martinet <asmadeus@codewreck.org>
To: Michael Grzeschik <m.grzeschik@pengutronix.de>
Cc: Eric Van Hensbergen <ericvh@kernel.org>,
	Latchesar Ionkov <lucho@ionkov.net>,
	Christian Schoenebeck <linux_oss@crudebyte.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	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 0/3] usb: gadget: 9pfs transport
Date: Tue, 16 Jan 2024 20:45:21 +0900	[thread overview]
Message-ID: <ZaZsUQUhSlMPLJg0@codewreck.org> (raw)
In-Reply-To: <20240116-ml-topic-u9p-v1-0-ad8c306f9a4e@pengutronix.de>

Michael Grzeschik wrote on Tue, Jan 16, 2024 at 02:49:40AM +0100:
> This series is adding support to mount 9pfs exported filesystems via the
> usb gadget interface. It also includes tools and descriptions on how to
> translate an tcp 9pfs and use it via the usb interface.

So I didn't have time to look at everything through, just want to make
sure, this series allows sharing data from an usb gadget (e.g. some
device with storage) over 9p as an alternative to things like MTP ?

I don't quite understand what the forwarder and diod have to do with
this; you're emulating a fake usb device with the forwarder that just
transmits requests to diod as backend implementation?
But 'usb.core.find(idVendor=0x1D6B, idProduct=0x0109)' looks like it's
searching for a real device not creating one, so that doesn't seem to
match up...

If you have any background information on where you're coming from and
where this is headed it'd be great to include in the cover letter.

While I had a quick look I'll spare you a second mail for the first
patch:
Michael Grzeschik wrote on Tue, Jan 16, 2024 at 02:49:41AM +0100: 
> +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,
> +};

This is missing a MODULE_ALIAS_9P("usbg") if you want the module to
auto-load on `mount -t trans=usbg` -- assuming this can build as a
module.

I'm also a bit worried that this net/9p-centric code is now also split
with drivers/usb/gadget/function/f_9pfs.c and I'll bet you the build
will break once in a while when we update global 9p client.c or
similar -- I'd be more comfortable having a net/9p/trans_usbg.c or
equivalent if possible.
Is there a reason this has to be in the usb gadget tree?
(Well, I assume from the usb gadget point of view, it's reasonable to
similarily prefer this code to stay close to drivers/usb/gadget..)

Thanks,
-- 
Dominique Martinet | Asmadeus