From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DB7E9C10F0E for ; Fri, 12 Apr 2019 03:59:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9F3202083E for ; Fri, 12 Apr 2019 03:59:33 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="yJUMwLs/" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726770AbfDLD7c (ORCPT ); Thu, 11 Apr 2019 23:59:32 -0400 Received: from mail-wm1-f66.google.com ([209.85.128.66]:35550 "EHLO mail-wm1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726708AbfDLD7c (ORCPT ); Thu, 11 Apr 2019 23:59:32 -0400 Received: by mail-wm1-f66.google.com with SMTP id y197so9210341wmd.0 for ; Thu, 11 Apr 2019 20:59:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=FDjbew8/XnEWPCx2JhORMgpWfv6ak7qB7VTQ5Y2QSSQ=; b=yJUMwLs/VuheROMdvbgcxf4aIlTmHjgP0u0OBcJVi/d+ATgieKvDC5dQ9akuiy+0as 1SWiHpYd3ln7U9/XZXOiWPacUxeA4m7ZwIagBdles+ZrvSDlTmzTaUl6dM3ji725o9D/ xw9PYnbcHoRwWBqHHluLC/I/k+y6vUD2mycjfMOGhH+4e3bOc9yS5uSGQAFodOgnwxAK sNY4u+dATu/fRLitXCIs915unk3mx+at3s+kf8cb4RqsC/ZaE6bv9nbYTxIiPkVx1vDV uzGqJUDVQHxvmLcvsBksSoZ+/X3fb7yHxdoLKCGeividloux9ewo97rgVOUYs1uKvAE2 Uuvw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=FDjbew8/XnEWPCx2JhORMgpWfv6ak7qB7VTQ5Y2QSSQ=; b=I3M2WGwjUi6cSqRSJDUAWYFRHXc8x559WryuDbSyC5nUHDjTIM78LnSHSgbNbTgU38 MwiLAKN7GHa6dNm9Zb4+i4h9mUZYBCGHgW1PdZjMAQrk0IOGgMh2NZ1XLdiOEPW7iIkW 2Jyv60bE6o5bpv5slwSpmyoOgnnwUZNvKqfT3HXZFPz5qocqZKUbOqB8WtcFmss307tv mvHq2j1/b21TxkFQ9HFaRAQsu1Q+afmxnaD14S54mi2tKXylDtbrsHKUQKlc+XvcL9om wdJDEiiNaPVKxpUyy4wqBOtusEffrmFLbW03kI1gpla1v8N2dPzbZhAJm9VPE0goagfQ VbjA== X-Gm-Message-State: APjAAAVZCMkhc1mS7tLHmFwOuvNgPr+qWj6aqgtXc07zgHEWZ7OXftFm 9VuRTJhU4eavxbx95lbF0QMbps5qm2ED6e5G91d8zA== X-Google-Smtp-Source: APXvYqwmNvRI2C5zFcXXGEjgvhsdkSLF3QjkF0eksska2gjzZB75WwT6NhB+7nv2p7FzRVpgPijHAosZyFC7RKRtFr8= X-Received: by 2002:a1c:a103:: with SMTP id k3mr9179534wme.8.1555041570364; Thu, 11 Apr 2019 20:59:30 -0700 (PDT) MIME-Version: 1.0 References: <20190329041409.70138-1-chenyu56@huawei.com> <20190329041409.70138-10-chenyu56@huawei.com> In-Reply-To: From: John Stultz Date: Thu, 11 Apr 2019 20:59:17 -0700 Message-ID: Subject: Re: [PATCH v5 09/13] usb: roles: Add usb role switch notifier. To: Yu Chen Cc: Linux USB List , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , lkml , Zhuangluan Su , Kongfei , "Liuyu (R)" , wanghu17@hisilicon.com, butao , chenyao11@huawei.com, fangshengzhou@hisilicon.com, Li Pengcheng , songxiaowei@hisilicon.com, YiPing Xu , xuyoujun4@huawei.com, Yudongbin , Zang Leigang , Greg Kroah-Hartman , Heikki Krogerus , Hans de Goede , Andy Shevchenko Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Apr 11, 2019 at 6:12 PM John Stultz wrote: > > On Thu, Mar 28, 2019 at 9:14 PM Yu Chen wrote: > > > > This patch adds notifier for drivers want to be informed of the usb role > > switch. > > > > Cc: Greg Kroah-Hartman > > Cc: Heikki Krogerus > > Cc: Hans de Goede > > Cc: Andy Shevchenko > > Cc: John Stultz > > Suggested-by: Heikki Krogerus > > Signed-off-by: Yu Chen > > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Stultz Subject: Re: [PATCH v5 09/13] usb: roles: Add usb role switch notifier. Date: Thu, 11 Apr 2019 20:59:17 -0700 Message-ID: References: <20190329041409.70138-1-chenyu56@huawei.com> <20190329041409.70138-10-chenyu56@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Yu Chen Cc: Linux USB List , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , lkml , Zhuangluan Su , Kongfei , "Liuyu (R)" , wanghu17@hisilicon.com, butao , chenyao11@huawei.com, fangshengzhou@hisilicon.com, Li Pengcheng , songxiaowei@hisilicon.com, YiPing Xu , xuyoujun4@huawei.com, Yudongbin , Zang Leigang , Greg Kroah-Hartman , Heikki Krogerus , Hans de Goede , Andy Shevchenko List-Id: devicetree@vger.kernel.org On Thu, Apr 11, 2019 at 6:12 PM John Stultz wrote: > > On Thu, Mar 28, 2019 at 9:14 PM Yu Chen wrote: > > > > This patch adds notifier for drivers want to be informed of the usb role > > switch. > > > > Cc: Greg Kroah-Hartman > > Cc: Heikki Krogerus > > Cc: Hans de Goede > > Cc: Andy Shevchenko > > Cc: John Stultz > > Suggested-by: Heikki Krogerus > > Signed-off-by: Yu Chen > > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [v5,09/13] usb: roles: Add usb role switch notifier. From: John Stultz Message-Id: Date: Thu, 11 Apr 2019 20:59:17 -0700 To: Yu Chen Cc: Linux USB List , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , lkml , Zhuangluan Su , Kongfei , "Liuyu \(R\)" , wanghu17@hisilicon.com, butao , chenyao11@huawei.com, fangshengzhou@hisilicon.com, Li Pengcheng , songxiaowei@hisilicon.com, YiPing Xu , xuyoujun4@huawei.com, Yudongbin , Zang Leigang , Greg Kroah-Hartman , Heikki Krogerus , Hans de Goede , Andy Shevchenko List-ID: T24gVGh1LCBBcHIgMTEsIDIwMTkgYXQgNjoxMiBQTSBKb2huIFN0dWx0eiA8am9obi5zdHVsdHpA bGluYXJvLm9yZz4gd3JvdGU6Cj4KPiBPbiBUaHUsIE1hciAyOCwgMjAxOSBhdCA5OjE0IFBNIFl1 IENoZW4gPGNoZW55dTU2QGh1YXdlaS5jb20+IHdyb3RlOgo+ID4KPiA+IFRoaXMgcGF0Y2ggYWRk cyBub3RpZmllciBmb3IgZHJpdmVycyB3YW50IHRvIGJlIGluZm9ybWVkIG9mIHRoZSB1c2Igcm9s ZQo+ID4gc3dpdGNoLgo+ID4KPiA+IENjOiBHcmVnIEtyb2FoLUhhcnRtYW4gPGdyZWdraEBsaW51 eGZvdW5kYXRpb24ub3JnPgo+ID4gQ2M6IEhlaWtraSBLcm9nZXJ1cyA8aGVpa2tpLmtyb2dlcnVz QGxpbnV4LmludGVsLmNvbT4KPiA+IENjOiBIYW5zIGRlIEdvZWRlIDxoZGVnb2VkZUByZWRoYXQu Y29tPgo+ID4gQ2M6IEFuZHkgU2hldmNoZW5rbyA8YW5keS5zaGV2Y2hlbmtvQGdtYWlsLmNvbT4K PiA+IENjOiBKb2huIFN0dWx0eiA8am9obi5zdHVsdHpAbGluYXJvLm9yZz4KPiA+IFN1Z2dlc3Rl ZC1ieTogSGVpa2tpIEtyb2dlcnVzIDxoZWlra2kua3JvZ2VydXNAbGludXguaW50ZWwuY29tPgo+ ID4gU2lnbmVkLW9mZi1ieTogWXUgQ2hlbiA8Y2hlbnl1NTZAaHVhd2VpLmNvbT4KPgo+IEhleSBZ dSBDaGVuIQo+ICAgIFRoYW5rcyBhZ2FpbiBmb3Igc2VuZGluZyB0aGlzIHBhdGNoIG91dCEgQXMg bWVudGlvbmVkIGluIG15Cj4gY29tbWVudHMgd2l0aCB0aGUgb3RoZXIgcGF0Y2hlcywgSSd2ZSBn b3Qgb25lIHByb3Bvc2FsIEkgd2FudGVkIHRvCj4gc2hhcmUgdG8gdHJ5IHRvIGF2b2lkICBzdGF0 ZSBpbml0aWFsaXphdGlvbiByYWNlcyB0aGF0IEkndmUgc2VlbiB3aXRoCj4gdGhpcyBwYXRjaHNl dC4KPgo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvdXNiL3JvbGVzL2NsYXNzLmMgYi9kcml2ZXJz L3VzYi9yb2xlcy9jbGFzcy5jCj4gPiBpbmRleCBmNDVkOGRmNWNmYjguLmUyY2FhYTY2NWQ2ZSAx MDA2NDQKPiA+IC0tLSBhL2RyaXZlcnMvdXNiL3JvbGVzL2NsYXNzLmMKPiA+ICsrKyBiL2RyaXZl cnMvdXNiL3JvbGVzL2NsYXNzLmMKPiA+IEBAIC01OCw2ICs2MSwyMCBAQCBpbnQgdXNiX3JvbGVf c3dpdGNoX3NldF9yb2xlKHN0cnVjdCB1c2Jfcm9sZV9zd2l0Y2ggKnN3LCBlbnVtIHVzYl9yb2xl IHJvbGUpCj4gPiAgfQo+ID4gIEVYUE9SVF9TWU1CT0xfR1BMKHVzYl9yb2xlX3N3aXRjaF9zZXRf cm9sZSk7Cj4gPgo+ID4gK2ludCB1c2Jfcm9sZV9zd2l0Y2hfcmVnaXN0ZXJfbm90aWZpZXIoc3Ry dWN0IHVzYl9yb2xlX3N3aXRjaCAqc3csCj4gPiArICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgIHN0cnVjdCBub3RpZmllcl9ibG9jayAqbmIpCj4gPiArewo+ID4gKyAgICAgICBy ZXR1cm4gYmxvY2tpbmdfbm90aWZpZXJfY2hhaW5fcmVnaXN0ZXIoJnN3LT5uaCwgbmIpOwo+ID4g K30KPiA+ICtFWFBPUlRfU1lNQk9MX0dQTCh1c2Jfcm9sZV9zd2l0Y2hfcmVnaXN0ZXJfbm90aWZp ZXIpOwo+Cj4gQXMgbm90ZWQgZWFybGllciwgb25lIGlzc3VlIEkndmUgc2VlbiBpcyB0aGF0IHRo ZSBoaXNpX2hpa2V5X3VzYgo+IGRyaXZlcidzIG5vdGlmaWVyIG1heSBub3QgZ2V0IGNhbGxlZCBl YXJseSBlbm91Z2ggdG8gcmVjZWl2ZQo+IG5vdGlmaWNhdGlvbiBvZiB0aGUgaW5pdGlhbCB1c2Ig c3RhdGUuCj4KPiBJdCBzZWVtcyBsaWtlIG9uIHJlZ2lzdHJhdGlvbiBoZXJlLCB3ZSBzaG91bGQg dGFrZSB0aGUgbG9jaywgcmVhZCB0aGUKPiByb2xlIHN0YXRlIGFuZCBpbW1lZGlhdGVseSBjYWxs IHRoZSBub3RpZmllciB0byBwcm9wZXJseSBpbml0aWFsaXplCj4gaXQ/IEkgc3VzcGVjdCB0aGF0 IHNob3VsZCBjbG9zZSB0aGUgd2luZG93IGZvciBhbnkgc3RhdGUgcmFjZXMgYXJvdW5kCj4gZHJp dmVyIHByb2JlIHRpbWluZ3MgYW5kCj4KPiBEb2VzIHRoYXQgbWFrZSBzZW5zZT8gSSBoYXZlIHJv dWdobHkgcHJvdG90eXBlZCB0aGlzIGJ1dCBuZWVkIHRvIGRvCj4gYWRkaXRpb25hbCB0ZXN0aW5n LgoKU28gdGhlc2Ugc2VlbSB0byBiZSB3b3JraW5nIG9rLgpGaXJzdCwgdGhlIG5vdGlmaWVyIGNh bGwgb24gcmVnaXN0ZXIgY2hhbmdlOgogIGh0dHBzOi8vZ2l0LmxpbmFyby5vcmcvcGVvcGxlL2pv aG4uc3R1bHR6L2FuZHJvaWQtZGV2LmdpdC9jb21taXQvP2g9ZGV2L2hpa2V5OTYwLW1haW5saW5l LVdJUCZpZD0xZDMxMmRiOTc2M2MyNDQ5YzNjZjViMzM4M2IxZjMzMzkwZjhjZTdiCgpTZWNvbmQs IHRoZSBjdXJyZW50X2RyX3JvbGUgY2hhbmdlIGZvciBkd2MzOgogIGh0dHBzOi8vZ2l0LmxpbmFy by5vcmcvcGVvcGxlL2pvaG4uc3R1bHR6L2FuZHJvaWQtZGV2LmdpdC9jb21taXQvP2g9ZGV2L2hp a2V5OTYwLW1haW5saW5lLVdJUCZpZD1iODBiNTM2ZGU3NmViODBmN2RmMDg0NmFhNTRhYzIyZTJi NGZmZTRlCgpGZWVsIGZyZWUgdG8gZm9sZC9yZXdvcmsgdGhvc2UgY2hhbmdlcyBpbnRvIHlvdXIg cGF0Y2ggc2V0LgoKdGhhbmtzCi1qb2huCg==