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
