| Message ID | 20190329041409.70138-10-chenyu56@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for usb on Hikey960 | expand |
On Fri, Mar 29, 2019 at 12:14:05PM +0800, Yu Chen wrote: > This patch adds notifier for drivers want to be informed of the usb role > switch. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: John Stultz <john.stultz@linaro.org> > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Signed-off-by: Yu Chen <chenyu56@huawei.com> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > v5: > * Split the patch into two patches, the first one introduces stubs for the > exiting functions, and this patch adds notifier functions. > --- > --- > drivers/usb/roles/class.c | 20 +++++++++++++++++++- > include/linux/usb/role.h | 16 ++++++++++++++++ > 2 files changed, 35 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c > index f45d8df5cfb8..e2caaa665d6e 100644 > --- a/drivers/usb/roles/class.c > +++ b/drivers/usb/roles/class.c > @@ -20,6 +20,7 @@ struct usb_role_switch { > struct device dev; > struct mutex lock; /* device lock*/ > enum usb_role role; > + struct blocking_notifier_head nh; > > /* From descriptor */ > struct device *usb2_port; > @@ -49,8 +50,10 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role) > mutex_lock(&sw->lock); > > ret = sw->set(sw->dev.parent, role); > - if (!ret) > + if (!ret) { > sw->role = role; > + blocking_notifier_call_chain(&sw->nh, role, NULL); > + } > > mutex_unlock(&sw->lock); > > @@ -58,6 +61,20 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role) > } > EXPORT_SYMBOL_GPL(usb_role_switch_set_role); > > +int usb_role_switch_register_notifier(struct usb_role_switch *sw, > + struct notifier_block *nb) > +{ > + return blocking_notifier_chain_register(&sw->nh, nb); > +} > +EXPORT_SYMBOL_GPL(usb_role_switch_register_notifier); > + > +int usb_role_switch_unregister_notifier(struct usb_role_switch *sw, > + struct notifier_block *nb) > +{ > + return blocking_notifier_chain_unregister(&sw->nh, nb); > +} > +EXPORT_SYMBOL_GPL(usb_role_switch_unregister_notifier); > + > /** > * usb_role_switch_get_role - Get the USB role for a switch > * @sw: USB role switch > @@ -271,6 +288,7 @@ usb_role_switch_register(struct device *parent, > return ERR_PTR(-ENOMEM); > > mutex_init(&sw->lock); > + BLOCKING_INIT_NOTIFIER_HEAD(&sw->nh); > > sw->allow_userspace_control = desc->allow_userspace_control; > sw->usb2_port = desc->usb2_port; > diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h > index da2b9641b877..99d8b8e4fe61 100644 > --- a/include/linux/usb/role.h > +++ b/include/linux/usb/role.h > @@ -53,6 +53,10 @@ struct usb_role_switch * > usb_role_switch_register(struct device *parent, > const struct usb_role_switch_desc *desc); > void usb_role_switch_unregister(struct usb_role_switch *sw); > +int usb_role_switch_register_notifier(struct usb_role_switch *sw, > + struct notifier_block *nb); > +int usb_role_switch_unregister_notifier(struct usb_role_switch *sw, > + struct notifier_block *nb); > #else > static inline int usb_role_switch_set_role(struct usb_role_switch *sw, > enum usb_role role) > @@ -80,6 +84,18 @@ usb_role_switch_register(struct device *parent, > } > > static inline void usb_role_switch_unregister(struct usb_role_switch *sw) { } > + > +static int usb_role_switch_register_notifier(struct usb_role_switch *sw, > + struct notifier_block *nb) > +{ > + return -ENODEV; > +} > + > +static int usb_role_switch_unregister_notifier(struct usb_role_switch *sw, > + struct notifier_block *nb) > +{ > + return -ENODEV; > +} > #endif > > #endif /* __LINUX_USB_ROLE_H */ > -- > 2.15.0-rc2 thanks,
On Thu, Mar 28, 2019 at 9:14 PM Yu Chen <chenyu56@huawei.com> wrote: > > This patch adds notifier for drivers want to be informed of the usb role > switch. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: John Stultz <john.stultz@linaro.org> > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Signed-off-by: Yu Chen <chenyu56@huawei.com> Hey Yu Chen! Thanks again for sending this patch out! As mentioned in my comments with the other patches, I've got one proposal I wanted to share to try to avoid state initialization races that I've seen with this patchset. > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c > index f45d8df5cfb8..e2caaa665d6e 100644 > --- a/drivers/usb/roles/class.c > +++ b/drivers/usb/roles/class.c > @@ -58,6 +61,20 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role) > } > EXPORT_SYMBOL_GPL(usb_role_switch_set_role); > > +int usb_role_switch_register_notifier(struct usb_role_switch *sw, > + struct notifier_block *nb) > +{ > + return blocking_notifier_chain_register(&sw->nh, nb); > +} > +EXPORT_SYMBOL_GPL(usb_role_switch_register_notifier); As noted earlier, one issue I've seen is that the hisi_hikey_usb driver's notifier may not get called early enough to receive notification of the initial usb state. It seems like on registration here, we should take the lock, read the role state and immediately call the notifier to properly initialize it? I suspect that should close the window for any state races around driver probe timings and Does that make sense? I have roughly prototyped this but need to do additional testing. thanks -john
On Thu, Apr 11, 2019 at 6:12 PM John Stultz <john.stultz@linaro.org> wrote: > > On Thu, Mar 28, 2019 at 9:14 PM Yu Chen <chenyu56@huawei.com> wrote: > > > > This patch adds notifier for drivers want to be informed of the usb role > > switch. > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Cc: Hans de Goede <hdegoede@redhat.com> > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > > Cc: John Stultz <john.stultz@linaro.org> > > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Signed-off-by: Yu Chen <chenyu56@huawei.com> > > Hey Yu Chen! > Thanks again for sending this patch out! As mentioned in my > comments with the other patches, I've got one proposal I wanted to > share to try to avoid state initialization races that I've seen with > this patchset. > > > diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c > > index f45d8df5cfb8..e2caaa665d6e 100644 > > --- a/drivers/usb/roles/class.c > > +++ b/drivers/usb/roles/class.c > > @@ -58,6 +61,20 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role) > > } > > EXPORT_SYMBOL_GPL(usb_role_switch_set_role); > > > > +int usb_role_switch_register_notifier(struct usb_role_switch *sw, > > + struct notifier_block *nb) > > +{ > > + return blocking_notifier_chain_register(&sw->nh, nb); > > +} > > +EXPORT_SYMBOL_GPL(usb_role_switch_register_notifier); > > As noted earlier, one issue I've seen is that the hisi_hikey_usb > driver's notifier may not get called early enough to receive > notification of the initial usb state. > > It seems like on registration here, we should take the lock, read the > role state and immediately call the notifier to properly initialize > it? I suspect that should close the window for any state races around > driver probe timings and > > Does that make sense? I have roughly prototyped this but need to do > additional testing. So these seem to be working ok. First, the notifier call on register change: https://git.linaro.org/people/john.stultz/android-dev.git/commit/?h=dev/hikey960-mainline-WIP&id=1d312db9763c2449c3cf5b3383b1f33390f8ce7b Second, the current_dr_role change for dwc3: https://git.linaro.org/people/john.stultz/android-dev.git/commit/?h=dev/hikey960-mainline-WIP&id=b80b536de76eb80f7df0846aa54ac22e2b4ffe4e Feel free to fold/rework those changes into your patch set. thanks -john
diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c index f45d8df5cfb8..e2caaa665d6e 100644 --- a/drivers/usb/roles/class.c +++ b/drivers/usb/roles/class.c @@ -20,6 +20,7 @@ struct usb_role_switch { struct device dev; struct mutex lock; /* device lock*/ enum usb_role role; + struct blocking_notifier_head nh; /* From descriptor */ struct device *usb2_port; @@ -49,8 +50,10 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role) mutex_lock(&sw->lock); ret = sw->set(sw->dev.parent, role); - if (!ret) + if (!ret) { sw->role = role; + blocking_notifier_call_chain(&sw->nh, role, NULL); + } mutex_unlock(&sw->lock); @@ -58,6 +61,20 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role) } EXPORT_SYMBOL_GPL(usb_role_switch_set_role); +int usb_role_switch_register_notifier(struct usb_role_switch *sw, + struct notifier_block *nb) +{ + return blocking_notifier_chain_register(&sw->nh, nb); +} +EXPORT_SYMBOL_GPL(usb_role_switch_register_notifier); + +int usb_role_switch_unregister_notifier(struct usb_role_switch *sw, + struct notifier_block *nb) +{ + return blocking_notifier_chain_unregister(&sw->nh, nb); +} +EXPORT_SYMBOL_GPL(usb_role_switch_unregister_notifier); + /** * usb_role_switch_get_role - Get the USB role for a switch * @sw: USB role switch @@ -271,6 +288,7 @@ usb_role_switch_register(struct device *parent, return ERR_PTR(-ENOMEM); mutex_init(&sw->lock); + BLOCKING_INIT_NOTIFIER_HEAD(&sw->nh); sw->allow_userspace_control = desc->allow_userspace_control; sw->usb2_port = desc->usb2_port; diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h index da2b9641b877..99d8b8e4fe61 100644 --- a/include/linux/usb/role.h +++ b/include/linux/usb/role.h @@ -53,6 +53,10 @@ struct usb_role_switch * usb_role_switch_register(struct device *parent, const struct usb_role_switch_desc *desc); void usb_role_switch_unregister(struct usb_role_switch *sw); +int usb_role_switch_register_notifier(struct usb_role_switch *sw, + struct notifier_block *nb); +int usb_role_switch_unregister_notifier(struct usb_role_switch *sw, + struct notifier_block *nb); #else static inline int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role) @@ -80,6 +84,18 @@ usb_role_switch_register(struct device *parent, } static inline void usb_role_switch_unregister(struct usb_role_switch *sw) { } + +static int usb_role_switch_register_notifier(struct usb_role_switch *sw, + struct notifier_block *nb) +{ + return -ENODEV; +} + +static int usb_role_switch_unregister_notifier(struct usb_role_switch *sw, + struct notifier_block *nb) +{ + return -ENODEV; +} #endif #endif /* __LINUX_USB_ROLE_H */
This patch adds notifier for drivers want to be informed of the usb role switch. Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> Cc: Hans de Goede <hdegoede@redhat.com> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: John Stultz <john.stultz@linaro.org> Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> Signed-off-by: Yu Chen <chenyu56@huawei.com> --- v5: * Split the patch into two patches, the first one introduces stubs for the exiting functions, and this patch adds notifier functions. --- --- drivers/usb/roles/class.c | 20 +++++++++++++++++++- include/linux/usb/role.h | 16 ++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-)