On Thu, Feb 24, 2022 at 11:37:39PM +0100, [email protected] wrote:
> On Thu, Feb 24, 2022 at 04:23:26PM -0500, [email protected] wrote:
> > Can you tell us how this should be fixed?
>
> It should be fixed by properly using the driver core to bind/unbind the
> driver to devices like I mentioned previously :)
This would involve creating a "gadget" bus_type (or should it be a
device_type under the platform bus?) and registering the gadgets
on it, right?. Similarly, the gadget drivers would be registered on
this bus. I suppose we can control which drivers get bound to which
gadgets with careful matching code.
> That will be more work, but it's the correct fix here. Otherwise it
> needs to take the same bus locks that the device lives on to keep things
> in sync, like the driver core would do if it were managing these things.
> That could be the "short term" fix if no one wants to do the real work
> needed here. Nothing should be needed to change in the driver core
> itself, it is rightfully thinking it owns the device and can free it
> when needed.
All right, thanks. I'll think about implementing it.
Alan Stern
On Thu, Feb 24, 2022 at 09:06:13PM -0500, [email protected] wrote:
> On Thu, Feb 24, 2022 at 11:37:39PM +0100, [email protected] wrote:
> > On Thu, Feb 24, 2022 at 04:23:26PM -0500, [email protected] wrote:
> > > Can you tell us how this should be fixed?
> >
> > It should be fixed by properly using the driver core to bind/unbind the
> > driver to devices like I mentioned previously :)
>
> This would involve creating a "gadget" bus_type (or should it be a
> device_type under the platform bus?) and registering the gadgets
> on it, right?.
Yes. Or you can use the aux bus for this, which might be easier.
> Similarly, the gadget drivers would be registered on
> this bus. I suppose we can control which drivers get bound to which
> gadgets with careful matching code.
The aux bus might make this easier:
Documentation/driver-api/auxiliary_bus.rst
thanks,
greg k-h
On Fri, Feb 25, 2022 at 09:53:35AM +0100, [email protected] wrote:
> On Thu, Feb 24, 2022 at 09:06:13PM -0500, [email protected] wrote:
> > On Thu, Feb 24, 2022 at 11:37:39PM +0100, [email protected] wrote:
> > > On Thu, Feb 24, 2022 at 04:23:26PM -0500, [email protected] wrote:
> > > > Can you tell us how this should be fixed?
> > >
> > > It should be fixed by properly using the driver core to bind/unbind the
> > > driver to devices like I mentioned previously :)
> >
> > This would involve creating a "gadget" bus_type (or should it be a
> > device_type under the platform bus?) and registering the gadgets
> > on it, right?.
>
> Yes. Or you can use the aux bus for this, which might be easier.
>
> > Similarly, the gadget drivers would be registered on
> > this bus. I suppose we can control which drivers get bound to which
> > gadgets with careful matching code.
>
> The aux bus might make this easier:
> Documentation/driver-api/auxiliary_bus.rst
Won't this end up changing the user-visible filenames and directories in
sysfs for gadgets and gadget drivers?
For instance, currently gadgets get registered under their UDC driver
name, like "net2280" or "at91". If we put them on the aux bus then they
will have to get registered under a name looking something like
"udc.gadget.0", i.e., module name, generic device name, and ID number.
We will be forced to use a generic device name because the aux bus does
matching based on it, and we want every gadget driver to be able to
match every UDC. We don't want some gadget drivers restricted to
net2280 gadgets, others restricted to fotg210 gadgets, and so on.
Alan Stern
On Fri, Feb 25, 2022 at 10:51:48AM -0500, [email protected] wrote:
> On Fri, Feb 25, 2022 at 09:53:35AM +0100, [email protected] wrote:
> > On Thu, Feb 24, 2022 at 09:06:13PM -0500, [email protected] wrote:
> > > On Thu, Feb 24, 2022 at 11:37:39PM +0100, [email protected] wrote:
> > > > On Thu, Feb 24, 2022 at 04:23:26PM -0500, [email protected] wrote:
> > > > > Can you tell us how this should be fixed?
> > > >
> > > > It should be fixed by properly using the driver core to bind/unbind the
> > > > driver to devices like I mentioned previously :)
> > >
> > > This would involve creating a "gadget" bus_type (or should it be a
> > > device_type under the platform bus?) and registering the gadgets
> > > on it, right?.
> >
> > Yes. Or you can use the aux bus for this, which might be easier.
> >
> > > Similarly, the gadget drivers would be registered on
> > > this bus. I suppose we can control which drivers get bound to which
> > > gadgets with careful matching code.
> >
> > The aux bus might make this easier:
> > Documentation/driver-api/auxiliary_bus.rst
>
> Won't this end up changing the user-visible filenames and directories in
> sysfs for gadgets and gadget drivers?
>
> For instance, currently gadgets get registered under their UDC driver
> name, like "net2280" or "at91". If we put them on the aux bus then they
> will have to get registered under a name looking something like
> "udc.gadget.0", i.e., module name, generic device name, and ID number.
Ah, yeah, that isn't good.
> We will be forced to use a generic device name because the aux bus does
> matching based on it, and we want every gadget driver to be able to
> match every UDC. We don't want some gadget drivers restricted to
> net2280 gadgets, others restricted to fotg210 gadgets, and so on.
So yes, I guess it does need to be a "real" bus, sorry.
thanks,
greg k-h
On Sat, Feb 26, 2022 at 10:07:02AM +0100, [email protected] wrote:
> On Fri, Feb 25, 2022 at 10:51:48AM -0500, [email protected] wrote:
> > On Fri, Feb 25, 2022 at 09:53:35AM +0100, [email protected] wrote:
> > > The aux bus might make this easier:
> > > Documentation/driver-api/auxiliary_bus.rst
> >
> > Won't this end up changing the user-visible filenames and directories in
> > sysfs for gadgets and gadget drivers?
> >
> > For instance, currently gadgets get registered under their UDC driver
> > name, like "net2280" or "at91". If we put them on the aux bus then they
> > will have to get registered under a name looking something like
> > "udc.gadget.0", i.e., module name, generic device name, and ID number.
>
> Ah, yeah, that isn't good.
>
> > We will be forced to use a generic device name because the aux bus does
> > matching based on it, and we want every gadget driver to be able to
> > match every UDC. We don't want some gadget drivers restricted to
> > net2280 gadgets, others restricted to fotg210 gadgets, and so on.
>
> So yes, I guess it does need to be a "real" bus, sorry.
It turns out not to be so bad. In fact there are only five
gadget drivers (that is, instances of struct usb_gadget_driver) in the
kernel:
composite_driver_template (gadget/composite.c)
configfs_driver_template (gadget/configfs.c)
gadgetfs_driver (gadget/legacy/inode.c)
driver (gadget/legacy/raw_gadget.c)
dbgp_driver (gadget/legacy/dbgp.c)
Everything else is implemented as a "function" driver. So the gadget
driver's name doesn't mean very much to the user anyway.
The interaction between the gadget subsystem and the device core is
rather peculiar. Each UDC controller is represented by a pair of device
structures: the .dev fields in struct usb_udc and struct usb_gadget.
These two are siblings -- they always have the same parent (see
usb_add_gadget() in gadget/udc/core.c). Furthermore, they have the same
driver; that is, udc->dev.driver is always the same as
gadget->dev.driver (see udc_bind_to_driver()). Which is doubly odd,
because gadget drivers manage only gadget devices, not udc devices. The
major difference between them is that the usb_udc is a class device
whereas the usb_gadget is a "real" device.
Currently neither the udc device nor the gadget device is registered on
any bus. IMO it would make sense to leave udc->dev.driver always set to
NULL, keep the udc devices bus-less, and put the gadget devices on the
aux bus.
Does that sound reasonable? I'll work on a patch to do it.
Alan Stern