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. > 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? > 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. > 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. 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? > 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); > > 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? Alan Stern
