Hi Greg, On Thu, Jan 18, 2024 at 10:53:34AM +0100, Greg KH wrote: > On Thu, Jan 18, 2024 at 11:22:06AM +0200, Heikki Krogerus wrote: > > Hi, > > > > On Wed, Jan 17, 2024 at 05:57:02AM +0000, Xu Yang wrote: > > > Hi Alan, > > > > > > > > > > > On Tue, Jan 16, 2024 at 05:44:47AM +0000, Xu Yang wrote: > > > > > Hi Alan, > > > > > > > > > > > > > > > > > Those of us unfamiliar with this code need you to explain a lot more > > > > > > about what's going on. > > > > > > > > > > > > On Mon, Jan 15, 2024 at 03:02:06AM +0000, Xu Yang wrote: > > > > > > > Taking below diagram as example: > > > > > > > > > > > > > > ci_hdrc.0 register usb get tcpm_port > > > > > > > (driver: ci_hdrc) ---------> role <---- (driver: tcpm) > > > > > > > ^ ^ switch | ^ > > > > > > > | | | | > > > > > > > +1| | +1 | |+1 > > > > > > > | +-------------------------------------+ | > > > > > > > | | > > > > > > > 4c200000.usb 1-0050 > > > > > > > (driver: ci_hdrc_imx) (driver: tcpci) > > > > > > > > > > > > > > 1. Driver ci_hdrc_imx and tcpci are built as module at least. > > > > > > > 2. When module ci_hdrc_imx is loaded, it will register ci_hdrc.0 device > > > > > > > and try to get ci_hdrc module's reference. > > > > > > > > > > > > This is very confusing. Normally, a device is registered by the parent > > > > > > module and its driver belongs in the child module. When the child > > > > > > module is loaded it automatically gets a reference to the parent module, > > > > > > because it calls functions that are defined in the parent. I don't know > > > > > > of any cases where a parent module takes a reference to one of its > > > > > > children -- this would make it impossible to unload the child module! > > > > > > > > > > > > In your diagram I can't tell whether ci_hdrc is the parent module and > > > > > > ci_hdrc_imx is the child, or vice versa. I'll guess that ci_hdrc_imx is > > > > > > the child, since it the one which gets a reference to the other. But > > > > > > now we have the ci_hdrc.0 device being registered by the child module > > > > > > and its driver belonging to the parent module, which is backward! > > > > > > > > > > > > Very difficult to understand. Please explain more fully. > > > > > > > > > > I checked again and let me correct the words. > > > > > > > > > > 2. When module ci_hdrc_imx is loaded, it will register ci_hdrc.0 device. > > > > > At the same time, the reference of module ci_hdrc is added by 1 > > > > > automatically due to ci_hdrc_imx calls some functions in module ci_hdrc. > > > > > ci_hdrc will register usb-role-switch device. > > > > > > > > > > Therefore, module ci_hdrc_imx depends on module ci_hdrc. Device ci_hdrc.0 > > > > > is a child of 4c200000.usb. > > > > > > > > And ci_hdrc_imx is a child module of ci_hdrc. Got it. > > > > > > > > > > > ci_hdrc will register > > > > > > > usb-role-switch device. > > > > > > > 3. When module tcpci is loaded, it will register tcpm port device and try > > > > > > > to get tcpm module's reference. The tcpm module will get usb-role-switch > > > > > > > which is registered by ci_hdrc. > > > > > > > > > > > > What do you mean by "will get"? Do you mean that tcpm will become the > > > > > > driver for the usb_role_switch device? Or do you mean that it simply > > > > > > calls get_device(&usb_role_switch)? > > > > > > > > > > > > If the latter is the case, how does the tcpm driver learn the address of > > > > > > usb_role_switch in the first place? > > > > > > > > > > Via > > > > > port->role_sw = usb_role_switch_get(port->dev) > > > > > or > > > > > port->role_sw = fwnode_usb_role_switch_get(tcpc->fwnode). > > > > > > > > > > The usb controller will register usb-role-swtich device to the global list > > > > > of usb_role class. The fwnode of usb-role-swtich device is also set to usb > > > > > controller's fwnode. Initially, a fwnode graph between usb controller of > > > > > node and tcpm connector node had already been established. These two > > > > > functions will find usb-role-swtich device based on this fwnode graph > > > > > and fwnode matching. > > > > > > > > If usb_role_switch_get() gives away references to the usb_role_switch > > > > device, it should have a way to take those references back. But I guess > > > > it doesn't. > > > > > > > > > After usb-role-switce device is found, these two > > > > > functions will call: try_module_get(sw->dev.parent->driver->owner). > > > > > > > > You mean usb_role_switch_get() and fwnode_usb_role_switch_get() do this? > > > > > > Yes. > > > > > > > > > > > > Here sw->dev.parent is device ci_hdrc.0. sw->dev.parent->driver is ci_hdrc. > > > > > > > > > > > > > > > > > > In current design, tcpm will also try to > > > > > > > get ci_hdrc module's reference after get usb-role-switch. > > > > > > > > > > > > This might be a bug. There should not be any need for the tcpm driver > > > > > > to take a reference to the ci_hdrc module. But there should be a way > > > > > > for the ci_hdrc driver to notify tcpm when the usb_role_switch device is > > > > > > about to be unregistered. If tcpm is usb_role_switch's driver then this > > > > > > notification happens automatically, by means of the .remove() callback. > > > > > > > > > > I'm not the designer of usb_role class driver. Not sure if this is needed to get > > > > > module reference of its parent device's driver. Maybe need @heikki's input. > > > > > > > > > > @heikki.krogerus, can you give some explanations? > > > > > > > > Yes, please, some additional explanation would help. > > > > > > > > > > > 4. Due to no modules depend on ci_hdrc_imx, ci_hdrc_imx can be manually > > > > > > > unloaded. Then device ci_hdrc.0 will be removed by ci_hdrc_imx and > > > > > > > device usb-role-switch is also unregistered. > > > > > > > > > > > > At this point, tcpm should learn that it has to drop all its references > > > > > > to usb_role_swich. Since the module which registered usb_role_switch > > > > > > isn't tcpm's ancestor, tcpm must not keep _any_ references to the device > > > > > > after it is unregistered. > > > > > > > > > > Yes, I also think so. > > > > > > > > > > > > > > > > > Well, strictly speaking that's not true. By misusing the driver model, > > > > > > tcpm could keep a reference to the ci_hdrc module until it was finished > > > > > > using usb_role_switch. Is that what you are trying to do? > > > > > > > > > > No, I'm trying to get module reference of ci_hdrc_imx too. Then, > > > > > ci_hdrc_imx can't be unloaded before tcpci module unloaded. > > > > > > > > You shouldn't do this. Users should be able to unload ci_hdrc_imx > > > > whenever they want, even if tcpci is still loaded. > > > > > > Okay. Understand. > > > > > > > > > > > > > > 5. Then, if I try to unload module tcpci, "NULL pointer dereference" > > > > > > > will be shown due to below code: > > > > > > > > > > > > > > module_put(sw->dev.parent->driver->owner); > > > > > > > > I forgot to ask: What function makes this call? Is it part of the > > > > usb_role class driver? > > > > > > usb_role_switch_put() do this. > > > Yes, it's a function of usb_role class driver. > > > > > > > > > > > > > > parent->driver is NULL at this time. > > > > > > > > > > > > What is dev at this point? And what is dev.parent? And what did > > > > > > dev.parent->driver used to be before it was set to NULL? > > > > > > > > > > Here sw->dev is usb-role-switch device. sw->dev.parent is ci_hdrc.0 device. > > > > > sw->dev.parent->driver was ci_hdrc. > > > > > > > > Which is now gone, right. I understand. > > > > > > > > Let's see what Heikki has to say. > > > > > > > > However, assuming he wants to continue misusing the driver model in this > > > > way, what you should do is add a new field to sw, where you will store > > > > sw->dev.parent->driver.owner at the time of the try_module_get() call > > > > (but only if the call succeeds!). Then when the module_put() call runs, > > > > have it use the value stored in this new field instead of dereferencing > > > > sw->dev.parent->driver.owner. > > > > > > It sounds like a better solution. > > > Thanks for the suggestion! > > > > If there is a better way of doing this, then let's use it. I'm happy > > with what ever works. > > > > The only requirement here is that we have some way of preventing the > > role switch provider from being unloaded while it's being used. > > Why? What defines "being used"? 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. > 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? thanks, -- heikki
