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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 11 Apr 2019 20:59:17 -0700
From:   John Stultz <john.stultz@...aro.org>
To:     Yu Chen <chenyu56@...wei.com>
Cc:     Linux USB List <linux-usb@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, lkml <linux-kernel@...r.kernel.org>,
        Zhuangluan Su <suzhuangluan@...ilicon.com>,
        Kongfei <kongfei@...ilicon.com>,
        "Liuyu (R)" <liuyu712@...ilicon.com>, wanghu17@...ilicon.com,
        butao <butao@...ilicon.com>, chenyao11@...wei.com,
        fangshengzhou@...ilicon.com,
        Li Pengcheng <lipengcheng8@...wei.com>,
        songxiaowei@...ilicon.com, YiPing Xu <xuyiping@...ilicon.com>,
        xuyoujun4@...wei.com, Yudongbin <yudongbin@...ilicon.com>,
        Zang Leigang <zangleigang@...ilicon.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Subject: Re: [PATCH v5 09/13] usb: roles: Add usb role switch notifier.

On Thu, Apr 11, 2019 at 6:12 PM John Stultz <john.stultz@...aro.org> wrote:
>
> On Thu, Mar 28, 2019 at 9:14 PM Yu Chen <chenyu56@...wei.com> wrote:
> >
> > This patch adds notifier for drivers want to be informed of the usb role
> > switch.
> >
> > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > Cc: Hans de Goede <hdegoede@...hat.com>
> > Cc: Andy Shevchenko <andy.shevchenko@...il.com>
> > Cc: John Stultz <john.stultz@...aro.org>
> > Suggested-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > Signed-off-by: Yu Chen <chenyu56@...wei.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