Re: [PATCH v5 09/13] usb: roles: Add usb role switch notifier.

[John Stultz]

On Thu, Mar 28, 2019 at 9:14 PM Yu Chen <chenyu56@xxxxxxxxxx> wrote:
>
> This patch adds notifier for drivers want to be informed of the usb role
> switch.
>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> Cc: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Cc: John Stultz <john.stultz@xxxxxxxxxx>
> Suggested-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Signed-off-by: Yu Chen <chenyu56@xxxxxxxxxx>

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