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

Xu Yang
2024-01-19T08:18:56Z

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