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
