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
