On Fri, Jan 19, 2024 at 08:18:51AM +0000, Xu Yang wrote:
> 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;
> }
I'm surprised that these functions don't handle errors in
try_module_get(). Is there some reason for this?
> @@ -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);
> }
> }
If try_module_get() failed then this would be an unbalanced
module_put().
However, these problems were already present in the driver and this
patch doesn't change them. So the patch looks okay to me.
Alan Stern
