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
