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
