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

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

[Xu Yang]

Generally, usb role switch device will be registered by usb controller
driver. Then this usb controller device will be the parent of the usb
role switch device. And also the usb controller device will not be a
standalone device, it may be registered by other glue drivers. Currently,
the glue driver can't aware the usage of usb role switch device. So it
will remove usb controller device when the glue driver is deregistered.
In this case, below kernel dump will be shown if the user of usb role
swich (such as tcpm) tries to put it.

[  248.891875] Hardware name: NXP i.MX95 19X19 board (DT)
[  248.896998] pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[  248.903948] pc : usb_role_switch_put+0x28/0x4c
[  248.908385] lr : tcpm_unregister_port+0xb8/0xf8 [tcpm]
[  248.913533] sp : ffff8000836fbbc0
[  248.916835] x29: ffff8000836fbbc0 x28: ffff0000899fd880 x27: 0000000000000000
[  248.923959] x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000
[  248.931083] x23: ffff000081417970 x22: ffff00008aab10e8 x21: ffff00008aab0080
[  248.938207] x20: ffff00008aab3110 x19: ffff00008a138c00 x18: ffffffffffffffff
[  248.945331] x17: 0000000000000000 x16: 00e0030000000000 x15: 0000000000000000
[  248.952455] x14: 0000000000000001 x13: 0000000000000040 x12: 0000000000000000
[  248.959579] x11: 0000000000000000 x10: ffffffffffffffff x9 : ffff8000836fbaf0
[  248.966703] x8 : ffff8000836fbaf0 x7 : ffff000084237728 x6 : 0000000000000400
[  248.973827] x5 : 0000000041001000 x4 : fffffc000228ee60 x3 : 00000000820000f4
[  248.980951] x2 : ffff00008a3b9b28 x1 : 00000000820000f5 x0 : 0000000000000000
[  248.988076] Call trace:
[  248.990512]  usb_role_switch_put+0x28/0x4c
[  248.994602]  tcpm_unregister_port+0xb8/0xf8 [tcpm]
[  248.999385]  tcpci_remove+0x5c/0xbc [tcpci]
[  249.003571]  i2c_device_remove+0x2c/0x9c
[  249.007489]  device_remove+0x4c/0x80
[  249.011059]  device_release_driver_internal+0x1c8/0x224
[  249.016268]  driver_detach+0x50/0x98
[  249.019830]  bus_remove_driver+0x6c/0xbc
[  249.023739]  driver_unregister+0x30/0x60
[  249.027647]  i2c_del_driver+0x54/0x68
[  249.031296]  tcpci_i2c_driver_exit+0x18/0x990 [tcpci]
[  249.036340]  __arm64_sys_delete_module+0x180/0x260
[  249.041124]  invoke_syscall+0x48/0x114
[  249.044868]  el0_svc_common.constprop.0+0xc8/0xe8
[  249.049557]  do_el0_svc+0x20/0x2c
[  249.052858]  el0_svc+0x40/0xf4
[  249.055909]  el0t_64_sync_handler+0x13c/0x158
[  249.060251]  el0t_64_sync+0x190/0x194
[  249.063904] Code: b140041f 540000e8 f9402000 f9403400 (f9400800)
[  249.069985] ---[ end trace 0000000000000000 ]---

To fix this issue, this will try to get/put all relevant modules when the
user tries to get/put usb role switch device.

Fixes: 5c54fcac9a9d ("usb: roles: Take care of driver module reference counting")
cc: <stable@vger.kernel.org>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/usb/roles/class.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index ae41578bd014..41060e354174 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -34,6 +34,24 @@ struct usb_role_switch {
 
 #define to_role_switch(d)	container_of(d, struct usb_role_switch, dev)
 
+void usb_role_switch_get_modules(struct device *dev)
+{
+	while (dev) {
+		if (dev->driver)
+			WARN_ON(!try_module_get(dev->driver->owner));
+		dev = dev->parent;
+	}
+}
+
+void usb_role_switch_put_modules(struct device *dev)
+{
+	while (dev) {
+		if (dev->driver)
+			module_put(dev->driver->owner);
+		dev = dev->parent;
+	}
+}
+
 /**
  * usb_role_switch_set_role - Set USB role for a switch
  * @sw: USB role switch
@@ -135,7 +153,7 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
 						  usb_role_switch_match);
 
 	if (!IS_ERR_OR_NULL(sw))
-		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
+		usb_role_switch_get_modules(sw->dev.parent);
 
 	return sw;
 }
@@ -157,7 +175,7 @@ struct usb_role_switch *fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
 		sw = fwnode_connection_find_match(fwnode, "usb-role-switch",
 						  NULL, usb_role_switch_match);
 	if (!IS_ERR_OR_NULL(sw))
-		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
+		usb_role_switch_get_modules(sw->dev.parent);
 
 	return sw;
 }
@@ -172,7 +190,7 @@ EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
 void usb_role_switch_put(struct usb_role_switch *sw)
 {
 	if (!IS_ERR_OR_NULL(sw)) {
-		module_put(sw->dev.parent->driver->owner);
+		usb_role_switch_put_modules(sw->dev.parent);
 		put_device(&sw->dev);
 	}
 }
@@ -195,7 +213,7 @@ usb_role_switch_find_by_fwnode(const struct fwnode_handle *fwnode)
 
 	dev = class_find_device_by_fwnode(&role_class, fwnode);
 	if (dev)
-		WARN_ON(!try_module_get(dev->parent->driver->owner));
+		usb_role_switch_get_modules(dev->parent);
 
 	return dev ? to_role_switch(dev) : NULL;
 }
-- 
2.34.1

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

[Greg KH]

On Fri, Jan 12, 2024 at 04:01:08PM +0800, Xu Yang wrote:
> +void usb_role_switch_get_modules(struct device *dev)
> +{
> +	while (dev) {
> +		if (dev->driver)
> +			WARN_ON(!try_module_get(dev->driver->owner));

You just crashed all systems that have panic-on-warn enabled, which is
by far (i.e. in the billions) the huge majority of Linux systems in the
world.

If this is something that can fail, then properly handle the issue,
don't just give up and say "let's fill the kernel log with a mess and
reboot the box!".

Also, are you SURE that module owners are what you want to track here?
That's not usually the real solution for things like this, remember
module reference counts deal with code, but the driver model deals with
data...

thanks,

greg k-h

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

[Greg KH]

On Fri, Jan 12, 2024 at 09:24:11AM +0100, Greg KH wrote:
> On Fri, Jan 12, 2024 at 04:01:08PM +0800, Xu Yang wrote:
> > +void usb_role_switch_get_modules(struct device *dev)
> > +{
> > +	while (dev) {
> > +		if (dev->driver)
> > +			WARN_ON(!try_module_get(dev->driver->owner));
> 
> You just crashed all systems that have panic-on-warn enabled, which is
> by far (i.e. in the billions) the huge majority of Linux systems in the
> world.
> 
> If this is something that can fail, then properly handle the issue,
> don't just give up and say "let's fill the kernel log with a mess and
> reboot the box!".

Ah, I see now you are just moving the code, but please, let's fix this
properly, don't persist in the wrong code here.

thanks,

greg k-h

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

[Xu Yang]

Hi Greg,

> 
> On Fri, Jan 12, 2024 at 09:24:11AM +0100, Greg KH wrote:
> > On Fri, Jan 12, 2024 at 04:01:08PM +0800, Xu Yang wrote:
> > > +void usb_role_switch_get_modules(struct device *dev)
> > > +{
> > > +   while (dev) {
> > > +           if (dev->driver)
> > > +                   WARN_ON(!try_module_get(dev->driver->owner));
> >
> > You just crashed all systems that have panic-on-warn enabled, which is
> > by far (i.e. in the billions) the huge majority of Linux systems in the
> > world.
> >
> > If this is something that can fail, then properly handle the issue,
> > don't just give up and say "let's fill the kernel log with a mess and
> > reboot the box!".
> 
> Ah, I see now you are just moving the code, but please, let's fix this
> properly, don't persist in the wrong code here.

This is a true module dependency issue and it only occurs when
load/unload modules. The dependency of usb controller glue driver,
usb controller driver and the user driver (such as tcpm) of usb role
switch is not correctly established.

This patch is used to adjust dependency of them, without it, two issues
may happen:
1. "NULL pointer dereference" kernel dump will be shown
2.  The reference count of usb controller module may never decrease to 0

Thanks, 
Xu Yang

> 
> thanks,
> 
> greg k-h

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

[Greg KH]

On Fri, Jan 12, 2024 at 09:28:04AM +0000, Xu Yang wrote:
> Hi Greg,
> 
> > 
> > On Fri, Jan 12, 2024 at 09:24:11AM +0100, Greg KH wrote:
> > > On Fri, Jan 12, 2024 at 04:01:08PM +0800, Xu Yang wrote:
> > > > +void usb_role_switch_get_modules(struct device *dev)
> > > > +{
> > > > +   while (dev) {
> > > > +           if (dev->driver)
> > > > +                   WARN_ON(!try_module_get(dev->driver->owner));
> > >
> > > You just crashed all systems that have panic-on-warn enabled, which is
> > > by far (i.e. in the billions) the huge majority of Linux systems in the
> > > world.
> > >
> > > If this is something that can fail, then properly handle the issue,
> > > don't just give up and say "let's fill the kernel log with a mess and
> > > reboot the box!".
> > 
> > Ah, I see now you are just moving the code, but please, let's fix this
> > properly, don't persist in the wrong code here.
> 
> This is a true module dependency issue and it only occurs when
> load/unload modules. The dependency of usb controller glue driver,
> usb controller driver and the user driver (such as tcpm) of usb role
> switch is not correctly established.

Then the driver model is not being used properly, as no module
references should be needed here.

> This patch is used to adjust dependency of them, without it, two issues
> may happen:
> 1. "NULL pointer dereference" kernel dump will be shown

For when?

> 2.  The reference count of usb controller module may never decrease to 0

The reference shouldn't have been increased as you want to be able to
unload the driver if a device is still present in the system.

So I think there's a larger issue here, module references shouldn't be
incremented just if a driver binds to a device, right?  Only if other
code is using the module, which is different.

thanks,

greg k-h

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

[Xu Yang]

Hi Greg,

> 
> On Fri, Jan 12, 2024 at 09:28:04AM +0000, Xu Yang wrote:
> > Hi Greg,
> >
> > >
> > > On Fri, Jan 12, 2024 at 09:24:11AM +0100, Greg KH wrote:
> > > > On Fri, Jan 12, 2024 at 04:01:08PM +0800, Xu Yang wrote:
> > > > > +void usb_role_switch_get_modules(struct device *dev)
> > > > > +{
> > > > > +   while (dev) {
> > > > > +           if (dev->driver)
> > > > > +                   WARN_ON(!try_module_get(dev->driver->owner));
> > > >
> > > > You just crashed all systems that have panic-on-warn enabled, which is
> > > > by far (i.e. in the billions) the huge majority of Linux systems in the
> > > > world.
> > > >
> > > > If this is something that can fail, then properly handle the issue,
> > > > don't just give up and say "let's fill the kernel log with a mess and
> > > > reboot the box!".
> > >
> > > Ah, I see now you are just moving the code, but please, let's fix this
> > > properly, don't persist in the wrong code here.
> >
> > This is a true module dependency issue and it only occurs when
> > load/unload modules. The dependency of usb controller glue driver,
> > usb controller driver and the user driver (such as tcpm) of usb role
> > switch is not correctly established.
> 
> Then the driver model is not being used properly, as no module
> references should be needed here.

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. 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. In current design, tcpm will also try to
   get ci_hdrc module's reference after get usb-role-switch.

> 
> > This patch is used to adjust dependency of them, without it, two issues
> > may happen:
> > 1. "NULL pointer dereference" kernel dump will be shown
> 
> For when?

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

> 
> > 2.  The reference count of usb controller module may never decrease to 0
> 
> The reference shouldn't have been increased as you want to be able to
> unload the driver if a device is still present in the system.
> 
> So I think there's a larger issue here, module references shouldn't be
> incremented just if a driver binds to a device, right?  Only if other
> code is using the module, which is different.

6. If ci_hdrc is also built as module, the module reference will keep at
   value 1 at least due to it failed to subtract 1 at step 5.

This issue is observed on chipidea and dwc3, the usb glue driver shouldn't
be unloaded befer tcpci module.

Thanks,
Xu Yang

> 
> thanks,
> 
> greg k-h

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

[Greg KH]

On Mon, Jan 15, 2024 at 03:02:06AM +0000, Xu Yang wrote:
> Hi Greg,
> 
> > 
> > On Fri, Jan 12, 2024 at 09:28:04AM +0000, Xu Yang wrote:
> > > Hi Greg,
> > >
> > > >
> > > > On Fri, Jan 12, 2024 at 09:24:11AM +0100, Greg KH wrote:
> > > > > On Fri, Jan 12, 2024 at 04:01:08PM +0800, Xu Yang wrote:
> > > > > > +void usb_role_switch_get_modules(struct device *dev)
> > > > > > +{
> > > > > > +   while (dev) {
> > > > > > +           if (dev->driver)
> > > > > > +                   WARN_ON(!try_module_get(dev->driver->owner));
> > > > >
> > > > > You just crashed all systems that have panic-on-warn enabled, which is
> > > > > by far (i.e. in the billions) the huge majority of Linux systems in the
> > > > > world.
> > > > >
> > > > > If this is something that can fail, then properly handle the issue,
> > > > > don't just give up and say "let's fill the kernel log with a mess and
> > > > > reboot the box!".
> > > >
> > > > Ah, I see now you are just moving the code, but please, let's fix this
> > > > properly, don't persist in the wrong code here.
> > >
> > > This is a true module dependency issue and it only occurs when
> > > load/unload modules. The dependency of usb controller glue driver,
> > > usb controller driver and the user driver (such as tcpm) of usb role
> > > switch is not correctly established.
> > 
> > Then the driver model is not being used properly, as no module
> > references should be needed here.
> 
> 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. 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. In current design, tcpm will also try to
>    get ci_hdrc module's reference after get usb-role-switch.
> 
> > 
> > > This patch is used to adjust dependency of them, without it, two issues
> > > may happen:
> > > 1. "NULL pointer dereference" kernel dump will be shown
> > 
> > For when?
> 
> 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.
> 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.

So that means that someone is not properly cleaning up the devices when
the module is removed, can you fix that root problem here instead?

thanks,

greg k-h

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

[Alan Stern]

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

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

[Xu Yang]

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.

> 
> >  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. After usb-role-switce device is found, these two
functions will call: try_module_get(sw->dev.parent->driver->owner).

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?

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

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

Here sw->dev is usb-role-switch device. sw->dev.parent is ci_hdrc.0 device.
sw->dev.parent->driver was ci_hdrc.

Thanks,
Xu Yang

> 
> Alan Stern

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

[Alan Stern]

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

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

[Xu Yang]

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!

Best Regards,
Xu Yang

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

[Heikki Krogerus]

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.

thanks,

-- 
heikki

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

[Greg KH]

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

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.

thanks,

greg k-h

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

[Heikki Krogerus]

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

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

[Greg KH]

On Thu, Jan 18, 2024 at 02:38:58PM +0200, Heikki Krogerus wrote:
> 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.

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.

> > 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!  :)

Sorry, I don't have the time in the near future to work on this...

greg k-h

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

[Alan Stern]

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.

Alan Stern

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

[Xu Yang]

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

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

[Alan Stern]

On Thu, Jan 18, 2024 at 03:52:52PM +0000, Xu Yang wrote:
> 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.

These really are questions for Heikki, not me.  Can you at least show us 
the patch you've been testing?

I agree that the status of the current USB role should be exposed to 
userspace somehow.

Alan Stern

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

[Xu Yang]

Hi,

> 
> On Thu, Jan 18, 2024 at 03:52:52PM +0000, Xu Yang wrote:
> > 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.
> 
> These really are questions for Heikki, not me.  Can you at least show us
> the patch you've been testing?

I have a simple test based on below changes:

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index ae41578bd014..d55a5d8d4fc4 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -20,6 +20,7 @@ static const struct class role_class = {
 
 struct usb_role_switch {
 	struct device dev;
+	struct module *module;
 	struct mutex lock; /* device lock*/
 	enum usb_role role;
 
@@ -135,7 +136,7 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
 						  usb_role_switch_match);
 
 	if (!IS_ERR_OR_NULL(sw))
-		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
+		WARN_ON(!try_module_get(sw->module));
 
 	return sw;
 }
@@ -157,7 +158,7 @@ struct usb_role_switch *fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
 		sw = fwnode_connection_find_match(fwnode, "usb-role-switch",
 						  NULL, usb_role_switch_match);
 	if (!IS_ERR_OR_NULL(sw))
-		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
+		WARN_ON(!try_module_get(sw->module));
 
 	return sw;
 }
@@ -172,7 +173,7 @@ EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
 void usb_role_switch_put(struct usb_role_switch *sw)
 {
 	if (!IS_ERR_OR_NULL(sw)) {
-		module_put(sw->dev.parent->driver->owner);
+		module_put(sw->module);
 		put_device(&sw->dev);
 	}
 }
@@ -189,15 +190,18 @@ struct usb_role_switch *
 usb_role_switch_find_by_fwnode(const struct fwnode_handle *fwnode)
 {
 	struct device *dev;
+	struct usb_role_switch *sw = NULL;
 
 	if (!fwnode)
 		return NULL;
 
 	dev = class_find_device_by_fwnode(&role_class, fwnode);
-	if (dev)
-		WARN_ON(!try_module_get(dev->parent->driver->owner));
+	if(dev) {
+		sw = to_role_switch(dev);
+		WARN_ON(!try_module_get(sw->module));
+	}
 
-	return dev ? to_role_switch(dev) : NULL;
+	return sw;
 }
 EXPORT_SYMBOL_GPL(usb_role_switch_find_by_fwnode);
 
@@ -339,6 +343,7 @@ usb_role_switch_register(struct device *parent,
 	sw->get = desc->get;
 
 	sw->dev.parent = parent;
+	sw->module = parent->driver->owner;
 	sw->dev.fwnode = desc->fwnode;
 	sw->dev.class = &role_class;
 	sw->dev.type = &usb_role_dev_type;

Thanks,
Xu Yang

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

[Heikki Krogerus]

On Fri, Jan 19, 2024 at 08:18:51AM +0000, Xu Yang wrote:
> Hi,
> 
> > 
> > On Thu, Jan 18, 2024 at 03:52:52PM +0000, Xu Yang wrote:
> > > 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.
> > 
> > These really are questions for Heikki, not me.  Can you at least show us
> > the patch you've been testing?
> 
> I have a simple test based on below changes:
> 
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index ae41578bd014..d55a5d8d4fc4 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -20,6 +20,7 @@ static const struct class role_class = {
>  
>  struct usb_role_switch {
>  	struct device dev;
> +	struct module *module;
>  	struct mutex lock; /* device lock*/
>  	enum usb_role role;
>  
> @@ -135,7 +136,7 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
>  						  usb_role_switch_match);
>  
>  	if (!IS_ERR_OR_NULL(sw))
> -		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
> +		WARN_ON(!try_module_get(sw->module));
>  
>  	return sw;
>  }
> @@ -157,7 +158,7 @@ struct usb_role_switch *fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
>  		sw = fwnode_connection_find_match(fwnode, "usb-role-switch",
>  						  NULL, usb_role_switch_match);
>  	if (!IS_ERR_OR_NULL(sw))
> -		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
> +		WARN_ON(!try_module_get(sw->module));
>  
>  	return sw;
>  }
> @@ -172,7 +173,7 @@ EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
>  void usb_role_switch_put(struct usb_role_switch *sw)
>  {
>  	if (!IS_ERR_OR_NULL(sw)) {
> -		module_put(sw->dev.parent->driver->owner);
> +		module_put(sw->module);
>  		put_device(&sw->dev);
>  	}
>  }
> @@ -189,15 +190,18 @@ struct usb_role_switch *
>  usb_role_switch_find_by_fwnode(const struct fwnode_handle *fwnode)
>  {
>  	struct device *dev;
> +	struct usb_role_switch *sw = NULL;
>  
>  	if (!fwnode)
>  		return NULL;
>  
>  	dev = class_find_device_by_fwnode(&role_class, fwnode);
> -	if (dev)
> -		WARN_ON(!try_module_get(dev->parent->driver->owner));
> +	if(dev) {
> +		sw = to_role_switch(dev);
> +		WARN_ON(!try_module_get(sw->module));
> +	}
>  
> -	return dev ? to_role_switch(dev) : NULL;
> +	return sw;
>  }
>  EXPORT_SYMBOL_GPL(usb_role_switch_find_by_fwnode);
>  
> @@ -339,6 +343,7 @@ usb_role_switch_register(struct device *parent,
>  	sw->get = desc->get;
>  
>  	sw->dev.parent = parent;
> +	sw->module = parent->driver->owner;
>  	sw->dev.fwnode = desc->fwnode;
>  	sw->dev.class = &role_class;
>  	sw->dev.type = &usb_role_dev_type;

This looks good to me.

-- 
heikki

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

[Alan Stern]

On Fri, Jan 19, 2024 at 08:18:51AM +0000, Xu Yang wrote:
> Hi,
> 
> > 
> > On Thu, Jan 18, 2024 at 03:52:52PM +0000, Xu Yang wrote:
> > > 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.
> > 
> > These really are questions for Heikki, not me.  Can you at least show us
> > the patch you've been testing?
> 
> I have a simple test based on below changes:
> 
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index ae41578bd014..d55a5d8d4fc4 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -20,6 +20,7 @@ static const struct class role_class = {
>  
>  struct usb_role_switch {
>  	struct device dev;
> +	struct module *module;
>  	struct mutex lock; /* device lock*/
>  	enum usb_role role;
>  
> @@ -135,7 +136,7 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
>  						  usb_role_switch_match);
>  
>  	if (!IS_ERR_OR_NULL(sw))
> -		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
> +		WARN_ON(!try_module_get(sw->module));
>  
>  	return sw;
>  }

I'm surprised that these functions don't handle errors in 
try_module_get().  Is there some reason for this?

> @@ -157,7 +158,7 @@ struct usb_role_switch *fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
>  		sw = fwnode_connection_find_match(fwnode, "usb-role-switch",
>  						  NULL, usb_role_switch_match);
>  	if (!IS_ERR_OR_NULL(sw))
> -		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
> +		WARN_ON(!try_module_get(sw->module));
>  
>  	return sw;
>  }
> @@ -172,7 +173,7 @@ EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
>  void usb_role_switch_put(struct usb_role_switch *sw)
>  {
>  	if (!IS_ERR_OR_NULL(sw)) {
> -		module_put(sw->dev.parent->driver->owner);
> +		module_put(sw->module);
>  		put_device(&sw->dev);
>  	}
>  }

If try_module_get() failed then this would be an unbalanced 
module_put().

However, these problems were already present in the driver and this 
patch doesn't change them.  So the patch looks okay to me.

Alan Stern

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

[Xu Yang]

Hi Greg,

> 
> > > 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.
> 
> 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.
> 
> > > 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!  :)

I've a simple test based on below changes too. This will remove
module_get/put() parts and add a flag to indicate the current status
of usb-role-switch device. It works too.

diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index ae41578bd014..313f378d1a74 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -22,6 +22,7 @@ struct usb_role_switch {
 	struct device dev;
 	struct mutex lock; /* device lock*/
 	enum usb_role role;
+	bool registered;
 
 	/* From descriptor */
 	struct device *usb2_port;
@@ -48,6 +49,9 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role)
 	if (IS_ERR_OR_NULL(sw))
 		return 0;
 
+	if (!sw->registered)
+		return 0;
+
 	mutex_lock(&sw->lock);
 
 	ret = sw->set(sw, role);
@@ -76,6 +80,9 @@ enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw)
 	if (IS_ERR_OR_NULL(sw))
 		return USB_ROLE_NONE;
 
+	if (!sw->registered)
+		return USB_ROLE_NONE;
+
 	mutex_lock(&sw->lock);
 
 	if (sw->get)
@@ -134,9 +141,6 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
 		sw = device_connection_find_match(dev, "usb-role-switch", NULL,
 						  usb_role_switch_match);
 
-	if (!IS_ERR_OR_NULL(sw))
-		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
-
 	return sw;
 }
 EXPORT_SYMBOL_GPL(usb_role_switch_get);
@@ -156,9 +160,6 @@ struct usb_role_switch *fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
 	if (!sw)
 		sw = fwnode_connection_find_match(fwnode, "usb-role-switch",
 						  NULL, usb_role_switch_match);
-	if (!IS_ERR_OR_NULL(sw))
-		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
-
 	return sw;
 }
 EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
@@ -172,7 +173,6 @@ EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
 void usb_role_switch_put(struct usb_role_switch *sw)
 {
 	if (!IS_ERR_OR_NULL(sw)) {
-		module_put(sw->dev.parent->driver->owner);
 		put_device(&sw->dev);
 	}
 }
@@ -194,9 +194,6 @@ usb_role_switch_find_by_fwnode(const struct fwnode_handle *fwnode)
 		return NULL;
 
 	dev = class_find_device_by_fwnode(&role_class, fwnode);
-	if (dev)
-		WARN_ON(!try_module_get(dev->parent->driver->owner));
-
 	return dev ? to_role_switch(dev) : NULL;
 }
 EXPORT_SYMBOL_GPL(usb_role_switch_find_by_fwnode);
@@ -352,6 +349,8 @@ usb_role_switch_register(struct device *parent,
 		return ERR_PTR(ret);
 	}
 
+	sw->registered = true;
+
 	/* TODO: Symlinks for the host port and the device controller. */
 
 	return sw;
@@ -366,8 +365,10 @@ EXPORT_SYMBOL_GPL(usb_role_switch_register);
  */
 void usb_role_switch_unregister(struct usb_role_switch *sw)
 {
-	if (!IS_ERR_OR_NULL(sw))
+	if (!IS_ERR_OR_NULL(sw)) {
+		sw->registered = false;
 		device_unregister(&sw->dev);
+	}
 }
 EXPORT_SYMBOL_GPL(usb_role_switch_unregister);

Thanks,
Xu Yang

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

[Alan Stern]

On Fri, Jan 19, 2024 at 11:13:23AM +0000, Xu Yang wrote:
> I've a simple test based on below changes too. This will remove
> module_get/put() parts and add a flag to indicate the current status
> of usb-role-switch device. It works too.
> 
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index ae41578bd014..313f378d1a74 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -22,6 +22,7 @@ struct usb_role_switch {
>  	struct device dev;
>  	struct mutex lock; /* device lock*/
>  	enum usb_role role;
> +	bool registered;
>  
>  	/* From descriptor */
>  	struct device *usb2_port;
> @@ -48,6 +49,9 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role)
>  	if (IS_ERR_OR_NULL(sw))
>  		return 0;
>  
> +	if (!sw->registered)
> +		return 0;
> +
>  	mutex_lock(&sw->lock);
>  
>  	ret = sw->set(sw, role);
> @@ -76,6 +80,9 @@ enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw)
>  	if (IS_ERR_OR_NULL(sw))
>  		return USB_ROLE_NONE;
>  
> +	if (!sw->registered)
> +		return USB_ROLE_NONE;
> +
>  	mutex_lock(&sw->lock);
>  
>  	if (sw->get)
> @@ -134,9 +141,6 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
>  		sw = device_connection_find_match(dev, "usb-role-switch", NULL,
>  						  usb_role_switch_match);
>  
> -	if (!IS_ERR_OR_NULL(sw))
> -		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
> -
>  	return sw;
>  }
>  EXPORT_SYMBOL_GPL(usb_role_switch_get);
> @@ -156,9 +160,6 @@ struct usb_role_switch *fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
>  	if (!sw)
>  		sw = fwnode_connection_find_match(fwnode, "usb-role-switch",
>  						  NULL, usb_role_switch_match);
> -	if (!IS_ERR_OR_NULL(sw))
> -		WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
> -
>  	return sw;
>  }
>  EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
> @@ -172,7 +173,6 @@ EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
>  void usb_role_switch_put(struct usb_role_switch *sw)
>  {
>  	if (!IS_ERR_OR_NULL(sw)) {
> -		module_put(sw->dev.parent->driver->owner);
>  		put_device(&sw->dev);
>  	}
>  }
> @@ -194,9 +194,6 @@ usb_role_switch_find_by_fwnode(const struct fwnode_handle *fwnode)
>  		return NULL;
>  
>  	dev = class_find_device_by_fwnode(&role_class, fwnode);
> -	if (dev)
> -		WARN_ON(!try_module_get(dev->parent->driver->owner));
> -
>  	return dev ? to_role_switch(dev) : NULL;
>  }
>  EXPORT_SYMBOL_GPL(usb_role_switch_find_by_fwnode);
> @@ -352,6 +349,8 @@ usb_role_switch_register(struct device *parent,
>  		return ERR_PTR(ret);
>  	}
>  
> +	sw->registered = true;
> +
>  	/* TODO: Symlinks for the host port and the device controller. */
>  
>  	return sw;
> @@ -366,8 +365,10 @@ EXPORT_SYMBOL_GPL(usb_role_switch_register);
>   */
>  void usb_role_switch_unregister(struct usb_role_switch *sw)
>  {
> -	if (!IS_ERR_OR_NULL(sw))
> +	if (!IS_ERR_OR_NULL(sw)) {
> +		sw->registered = false;
>  		device_unregister(&sw->dev);
> +	}
>  }
>  EXPORT_SYMBOL_GPL(usb_role_switch_unregister);

What happens if the provider module is unloaded but then 
usb_role_switch_put() is called after usb_role_switch_unregister()?  
Won't there be a NULL pointer dereference inside the put_device() call?

Alan Stern

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

[Xu Yang]

Hi Alan,

> 
> On Fri, Jan 19, 2024 at 11:13:23AM +0000, Xu Yang wrote:
> > I've a simple test based on below changes too. This will remove
> > module_get/put() parts and add a flag to indicate the current status
> > of usb-role-switch device. It works too.
> >
> > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> > index ae41578bd014..313f378d1a74 100644
> > --- a/drivers/usb/roles/class.c
> > +++ b/drivers/usb/roles/class.c
> > @@ -22,6 +22,7 @@ struct usb_role_switch {
> >       struct device dev;
> >       struct mutex lock; /* device lock*/
> >       enum usb_role role;
> > +     bool registered;
> >
> >       /* From descriptor */
> >       struct device *usb2_port;
> > @@ -48,6 +49,9 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role)
> >       if (IS_ERR_OR_NULL(sw))
> >               return 0;
> >
> > +     if (!sw->registered)
> > +             return 0;
> > +
> >       mutex_lock(&sw->lock);
> >
> >       ret = sw->set(sw, role);
> > @@ -76,6 +80,9 @@ enum usb_role usb_role_switch_get_role(struct usb_role_switch *sw)
> >       if (IS_ERR_OR_NULL(sw))
> >               return USB_ROLE_NONE;
> >
> > +     if (!sw->registered)
> > +             return USB_ROLE_NONE;
> > +
> >       mutex_lock(&sw->lock);
> >
> >       if (sw->get)
> > @@ -134,9 +141,6 @@ struct usb_role_switch *usb_role_switch_get(struct device *dev)
> >               sw = device_connection_find_match(dev, "usb-role-switch", NULL,
> >                                                 usb_role_switch_match);
> >
> > -     if (!IS_ERR_OR_NULL(sw))
> > -             WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
> > -
> >       return sw;
> >  }
> >  EXPORT_SYMBOL_GPL(usb_role_switch_get);
> > @@ -156,9 +160,6 @@ struct usb_role_switch *fwnode_usb_role_switch_get(struct fwnode_handle *fwnode)
> >       if (!sw)
> >               sw = fwnode_connection_find_match(fwnode, "usb-role-switch",
> >                                                 NULL, usb_role_switch_match);
> > -     if (!IS_ERR_OR_NULL(sw))
> > -             WARN_ON(!try_module_get(sw->dev.parent->driver->owner));
> > -
> >       return sw;
> >  }
> >  EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
> > @@ -172,7 +173,6 @@ EXPORT_SYMBOL_GPL(fwnode_usb_role_switch_get);
> >  void usb_role_switch_put(struct usb_role_switch *sw)
> >  {
> >       if (!IS_ERR_OR_NULL(sw)) {
> > -             module_put(sw->dev.parent->driver->owner);
> >               put_device(&sw->dev);
> >       }
> >  }
> > @@ -194,9 +194,6 @@ usb_role_switch_find_by_fwnode(const struct fwnode_handle *fwnode)
> >               return NULL;
> >
> >       dev = class_find_device_by_fwnode(&role_class, fwnode);
> > -     if (dev)
> > -             WARN_ON(!try_module_get(dev->parent->driver->owner));
> > -
> >       return dev ? to_role_switch(dev) : NULL;
> >  }
> >  EXPORT_SYMBOL_GPL(usb_role_switch_find_by_fwnode);
> > @@ -352,6 +349,8 @@ usb_role_switch_register(struct device *parent,
> >               return ERR_PTR(ret);
> >       }
> >
> > +     sw->registered = true;
> > +
> >       /* TODO: Symlinks for the host port and the device controller. */
> >
> >       return sw;
> > @@ -366,8 +365,10 @@ EXPORT_SYMBOL_GPL(usb_role_switch_register);
> >   */
> >  void usb_role_switch_unregister(struct usb_role_switch *sw)
> >  {
> > -     if (!IS_ERR_OR_NULL(sw))
> > +     if (!IS_ERR_OR_NULL(sw)) {
> > +             sw->registered = false;
> >               device_unregister(&sw->dev);
> > +     }
> >  }
> >  EXPORT_SYMBOL_GPL(usb_role_switch_unregister);
> 
> What happens if the provider module is unloaded but then
> usb_role_switch_put() is called after usb_role_switch_unregister()?
> Won't there be a NULL pointer dereference inside the put_device() call?

The get_device() will be called after the user successfully get usb_role_switch
device. So the resource of sw will continue to exist until usb_role_switch_put()
is called.

Thanks,
Xu Yang

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

[Alan Stern]

On Fri, Jan 19, 2024 at 03:23:50PM +0000, Xu Yang wrote:
> > What happens if the provider module is unloaded but then
> > usb_role_switch_put() is called after usb_role_switch_unregister()?
> > Won't there be a NULL pointer dereference inside the put_device() call?
> 
> The get_device() will be called after the user successfully get usb_role_switch
> device. So the resource of sw will continue to exist until usb_role_switch_put()
> is called.

But look: Your patch essentially prevents usb_role_switch_set_role() 
from running after the role-switch device has been unregistered.  But 
what if someone had already called usb_role_switch_set_role() before the 
device was unregistered?  Won't that eventually lead to problems if the 
provider's module is then unloaded from memory?

To put it another way, all those try_module_get() and module_put() calls 
were originally added to prevent a specific problem from occurring.  
Once you remove them, won't that problem be able to occur again?

Alan Stern

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

[Xu Yang]

Hi Alan,

> 
> On Fri, Jan 19, 2024 at 03:23:50PM +0000, Xu Yang wrote:
> > > What happens if the provider module is unloaded but then
> > > usb_role_switch_put() is called after usb_role_switch_unregister()?
> > > Won't there be a NULL pointer dereference inside the put_device() call?
> >
> > The get_device() will be called after the user successfully get usb_role_switch
> > device. So the resource of sw will continue to exist until usb_role_switch_put()
> > is called.
> 
> But look: Your patch essentially prevents usb_role_switch_set_role()
> from running after the role-switch device has been unregistered.  But
> what if someone had already called usb_role_switch_set_role() before the
> device was unregistered?  Won't that eventually lead to problems if the
> provider's module is then unloaded from memory?

Yes, exactly it may happen.

> 
> To put it another way, all those try_module_get() and module_put() calls
> were originally added to prevent a specific problem from occurring.
> Once you remove them, won't that problem be able to occur again?

Yes, it will. Thanks for reminder about this.

Thanks,
Xu Yang

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

[Xu Yang]

Hi Heikki,

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

Preventing the role switch provider from being unloaded seems infeasible
because the usb controller device (in most cases is the provider) can be
unbinded too. If that happens, the usb role switch device will be unregisterd
too and the provider driver will still exist.

Thanks,
Xu Yang

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

[Greg KH]

On Fri, Jan 12, 2024 at 04:01:08PM +0800, Xu Yang wrote:
> Generally, usb role switch device will be registered by usb controller
> driver. Then this usb controller device will be the parent of the usb
> role switch device. And also the usb controller device will not be a
> standalone device, it may be registered by other glue drivers. Currently,
> the glue driver can't aware the usage of usb role switch device. So it
> will remove usb controller device when the glue driver is deregistered.
> In this case, below kernel dump will be shown if the user of usb role
> swich (such as tcpm) tries to put it.

Note, manual removal of modules is just that, manual.  No module is
every auto-unloaded.

So how are you seeing this error in a real system?

> 
> [  248.891875] Hardware name: NXP i.MX95 19X19 board (DT)
> [  248.896998] pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [  248.903948] pc : usb_role_switch_put+0x28/0x4c
> [  248.908385] lr : tcpm_unregister_port+0xb8/0xf8 [tcpm]
> [  248.913533] sp : ffff8000836fbbc0
> [  248.916835] x29: ffff8000836fbbc0 x28: ffff0000899fd880 x27: 0000000000000000
> [  248.923959] x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000
> [  248.931083] x23: ffff000081417970 x22: ffff00008aab10e8 x21: ffff00008aab0080
> [  248.938207] x20: ffff00008aab3110 x19: ffff00008a138c00 x18: ffffffffffffffff
> [  248.945331] x17: 0000000000000000 x16: 00e0030000000000 x15: 0000000000000000
> [  248.952455] x14: 0000000000000001 x13: 0000000000000040 x12: 0000000000000000
> [  248.959579] x11: 0000000000000000 x10: ffffffffffffffff x9 : ffff8000836fbaf0
> [  248.966703] x8 : ffff8000836fbaf0 x7 : ffff000084237728 x6 : 0000000000000400
> [  248.973827] x5 : 0000000041001000 x4 : fffffc000228ee60 x3 : 00000000820000f4
> [  248.980951] x2 : ffff00008a3b9b28 x1 : 00000000820000f5 x0 : 0000000000000000
> [  248.988076] Call trace:
> [  248.990512]  usb_role_switch_put+0x28/0x4c
> [  248.994602]  tcpm_unregister_port+0xb8/0xf8 [tcpm]
> [  248.999385]  tcpci_remove+0x5c/0xbc [tcpci]
> [  249.003571]  i2c_device_remove+0x2c/0x9c
> [  249.007489]  device_remove+0x4c/0x80
> [  249.011059]  device_release_driver_internal+0x1c8/0x224
> [  249.016268]  driver_detach+0x50/0x98
> [  249.019830]  bus_remove_driver+0x6c/0xbc
> [  249.023739]  driver_unregister+0x30/0x60
> [  249.027647]  i2c_del_driver+0x54/0x68
> [  249.031296]  tcpci_i2c_driver_exit+0x18/0x990 [tcpci]
> [  249.036340]  __arm64_sys_delete_module+0x180/0x260
> [  249.041124]  invoke_syscall+0x48/0x114
> [  249.044868]  el0_svc_common.constprop.0+0xc8/0xe8
> [  249.049557]  do_el0_svc+0x20/0x2c
> [  249.052858]  el0_svc+0x40/0xf4
> [  249.055909]  el0t_64_sync_handler+0x13c/0x158
> [  249.060251]  el0t_64_sync+0x190/0x194
> [  249.063904] Code: b140041f 540000e8 f9402000 f9403400 (f9400800)
> [  249.069985] ---[ end trace 0000000000000000 ]---
> 
> To fix this issue, this will try to get/put all relevant modules when the
> user tries to get/put usb role switch device.

With this change, is the module now unable to be loaded if the hardware
is present?

And again, why isn't this properly represented in the driver model so
that if the module is unloaded, the relevant code paths are not properly
cleaned up and the device is removed _before_ the module is unloaded?

thanks,

greg k-h

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

[kernel test robot]

Hi Xu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus hid/for-next westeri-thunderbolt/next linus/master v6.7 next-20240112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Xu-Yang/usb-roles-try-to-get-put-all-relevant-modules/20240112-155735
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20240112080108.1147450-1-xu.yang_2%40nxp.com
patch subject: [PATCH] usb: roles: try to get/put all relevant modules
config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20240115/202401150608.0Ok2pmGw-lkp@intel.com/config)
compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project 9bde5becb44ea071f5e1fa1f5d4071dc8788b18c)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240115/202401150608.0Ok2pmGw-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401150608.0Ok2pmGw-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/usb/roles/class.c:37:6: warning: no previous prototype for function 'usb_role_switch_get_modules' [-Wmissing-prototypes]
      37 | void usb_role_switch_get_modules(struct device *dev)
         |      ^
   drivers/usb/roles/class.c:37:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
      37 | void usb_role_switch_get_modules(struct device *dev)
         | ^
         | static 
>> drivers/usb/roles/class.c:46:6: warning: no previous prototype for function 'usb_role_switch_put_modules' [-Wmissing-prototypes]
      46 | void usb_role_switch_put_modules(struct device *dev)
         |      ^
   drivers/usb/roles/class.c:46:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
      46 | void usb_role_switch_put_modules(struct device *dev)
         | ^
         | static 
   2 warnings generated.


vim +/usb_role_switch_get_modules +37 drivers/usb/roles/class.c

    36	
  > 37	void usb_role_switch_get_modules(struct device *dev)
    38	{
    39		while (dev) {
    40			if (dev->driver)
    41				WARN_ON(!try_module_get(dev->driver->owner));
    42			dev = dev->parent;
    43		}
    44	}
    45	
  > 46	void usb_role_switch_put_modules(struct device *dev)
    47	{
    48		while (dev) {
    49			if (dev->driver)
    50				module_put(dev->driver->owner);
    51			dev = dev->parent;
    52		}
    53	}
    54	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Xu,

kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus hid/for-next westeri-thunderbolt/next linus/master v6.7 next-20240112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Xu-Yang/usb-roles-try-to-get-put-all-relevant-modules/20240112-155735
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20240112080108.1147450-1-xu.yang_2%40nxp.com
patch subject: [PATCH] usb: roles: try to get/put all relevant modules
config: i386-randconfig-061-20240115 (https://download.01.org/0day-ci/archive/20240115/202401151012.Dzvmty7a-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240115/202401151012.Dzvmty7a-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401151012.Dzvmty7a-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/usb/roles/class.c:37:6: sparse: sparse: symbol 'usb_role_switch_get_modules' was not declared. Should it be static?
>> drivers/usb/roles/class.c:46:6: sparse: sparse: symbol 'usb_role_switch_put_modules' was not declared. Should it be static?

vim +/usb_role_switch_get_modules +37 drivers/usb/roles/class.c

    36	
  > 37	void usb_role_switch_get_modules(struct device *dev)
    38	{
    39		while (dev) {
    40			if (dev->driver)
    41				WARN_ON(!try_module_get(dev->driver->owner));
    42			dev = dev->parent;
    43		}
    44	}
    45	
  > 46	void usb_role_switch_put_modules(struct device *dev)
    47	{
    48		while (dev) {
    49			if (dev->driver)
    50				module_put(dev->driver->owner);
    51			dev = dev->parent;
    52		}
    53	}
    54	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki