RE: [EXT] Re: [PATCH] usb: roles: try to get/put all relevant modules

Xu Yang
2024-01-18T15:52:55Z

Hi Alan,

>
> On Thu, Jan 18, 2024 at 03:54:16PM +0100, Greg KH wrote:
> > On Thu, Jan 18, 2024 at 02:38:58PM +0200, Heikki Krogerus wrote:
> > > It is "being used" when something (user) acquires reference to the
> > > role switch that it provides. The "user" is in most cases USB Type-C
> > > port driver - what ever knows the role the port needs to be in.
> > >
> > > USB role switch is meant to act as a "resource" like any other. So
> > > when you acquire for example a GPIO, the gpiodev driver is pinned down
> > > by calling module_get() just like this driver is doing to the switch
> > > provider.
> >
> > So again, if the hardware is present in the system, the module reference
> > count will always be increased and can never be removed no matter what a
> > user does? That feels wrong if so.
>
> It's not quite that bad. Even if the USB role-switch hardware is
> present in the system, the module controlling it can be removed if the
> role-switch resource isn't being used (which will cause the module count
> will go back down to 0). This may require the user first to remove the
> module that's using the resource.
>
> > > > Any module should be able to be removed any time, unless there is a
> > > > "code" requirement of it being present. The driver model structures
> > > > should make this possible if used properly. Trying to much around in
> > > > various parent devices and the drivers bound to parents should NEVER be
> > > > done as happens here, sorry I missed that in the review process.
> > >
> > > If this is not something that this kind of device class should be
> > > doing, then let's remove the whole thing. But if that is the case,
> > > then shouldn't we remove that from all the other bus and device class
> > > drivers as well?
> >
> > I started to remove it many years ago, but then something prevented that
> > as there was actually some valid uses, but I can't remember at the
> > moment. Try taking it out and see what happens! :)
>
> The problem here is that a device is being used by module B that isn't a
> descendant of the device's driver module A. If B didn't take a
> reference to A then A could be unloaded while B was still using the
> device. When B finally drops its reference to the device, the
> driver-model release call would get a NULL-pointer violation, since the
> code for the release method was part of A and has been unloaded from
> memory.
>
> In the current code, B takes a module reference to A and drops that
> reference after dropping its reference to the device. The bug that Xu
> Yang wants to fix (the original reason for this discussion) has to do
> with the way B drops its module reference to A. It gets the module
> handle of A by looking up the owner of the device's driver -- and that
> approach doesn't work if the device has been unbound from its driver,
> for the obvious reason. The proper solution to this problem is for B to
> cache A's handle at the time when it first acquires the module
> reference.
>

I've tried your suggestion and it appears to be working fine. Now I'm not sure if
the module get/put parts should be removed or to fix the NULL pointer issue. I'm
working on this issue, so I have time to fix it. I think if first way is taken, the status
of usb_role_switch device should be updated when it's registered/unregisterd. Or
other issues will occur since the user doesn't know the change of usb_role_switch
device.

Thanks,
Xu Yang