| Message ID | 20201127062820.588-1-peter.chen@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] usb: roles: reference controller's parent device if existed | expand |
Hi Peter, > -----Original Message----- > From: Peter Chen <peter.chen@kernel.org> > Sent: Friday, November 27, 2020 2:28 PM > To: heikki.krogerus@linux.intel.com > Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Peter > Chen <peter.chen@nxp.com>; Jun Li <jun.li@nxp.com> > Subject: [PATCH 1/1] usb: roles: reference controller's parent device if > existed > > From: Peter Chen <peter.chen@nxp.com> > > For some DRD IP drivers (eg, dwc3/cdns3/chipidea), the core device is created > and deleted by glue layer device. So, if role switch user (eg, tcpci), core > device, and glue layer device are all built as module, and glue layer device > is removed first, the core device's driver ->remove function will be called, > and its device's driver pointer will be NULL, and cause below oops. > > To fix it, if there is a parent for controller device (role switch device's > parent), it references to parent too. This may not enough, the trouble is role switch class isn't aware of the role switch is unregistered by its provider while typec class is using it, this can happen by different ways, if we do role switch provider driver unbind, we still suffer this problem: echo usb_controller_dev > unbind I am not sure if we should prevent this happening at USB controller drivers(provider side), or this is allowed and we should enhance role switch class to be aware of this situation and properly handle it. Thanks Li Jun
On 20-11-30 02:37:54, Jun Li wrote: > Hi Peter, > > -----Original Message----- > > From: Peter Chen <peter.chen@kernel.org> > > Sent: Friday, November 27, 2020 2:28 PM > > To: heikki.krogerus@linux.intel.com > > Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; Peter > > Chen <peter.chen@nxp.com>; Jun Li <jun.li@nxp.com> > > Subject: [PATCH 1/1] usb: roles: reference controller's parent device if > > existed > > > > From: Peter Chen <peter.chen@nxp.com> > > > > For some DRD IP drivers (eg, dwc3/cdns3/chipidea), the core device is created > > and deleted by glue layer device. So, if role switch user (eg, tcpci), core > > device, and glue layer device are all built as module, and glue layer device > > is removed first, the core device's driver ->remove function will be called, > > and its device's driver pointer will be NULL, and cause below oops. > > > > To fix it, if there is a parent for controller device (role switch device's > > parent), it references to parent too. > > This may not enough, the trouble is role switch class isn't aware of > the role switch is unregistered by its provider while typec class is > using it, this can happen by different ways, if we do role switch provider > driver unbind, we still suffer this problem: > > echo usb_controller_dev > unbind > Yes, to fix this issue totally, we need to change the way role switch class used by controller and type-c driver, until now, we still have not a good way. We could fix module unload issue first, unbind two devices may not a common use case. > I am not sure if we should prevent this happening at USB controller > drivers(provider side), or this is allowed and we should enhance > role switch class to be aware of this situation and properly handle it. >
diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c index 97f37077b7f9..e8ff0f7ff4fd 100644 --- a/drivers/usb/roles/class.c +++ b/drivers/usb/roles/class.c @@ -129,8 +129,14 @@ 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)); + if (!IS_ERR_OR_NULL(sw)) { + struct device *dev; + + dev = sw->dev.parent; + WARN_ON(!try_module_get(dev->driver->owner)); + if (dev->parent) + WARN_ON(!try_module_get(dev->parent->driver->owner)); + } return sw; } @@ -151,8 +157,14 @@ 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)); + if (!IS_ERR_OR_NULL(sw)) { + struct device *dev; + + dev = sw->dev.parent; + WARN_ON(!try_module_get(dev->driver->owner)); + if (dev->parent) + WARN_ON(!try_module_get(dev->parent->driver->owner)); + } return sw; } @@ -167,7 +179,13 @@ 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); + struct device *dev; + + dev = sw->dev.parent; + module_put(dev->driver->owner); + if (dev->parent) + module_put(dev->parent->driver->owner); + put_device(&sw->dev); } }