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

Alan Stern
2024-01-15T16:31:46Z

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