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

Alan Stern
2024-01-16T16:03:10Z

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?

> 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.

> > > 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?

> > > 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.

Alan Stern