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=-7.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED 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 78AA2C10F13 for ; Fri, 12 Apr 2019 01:12:48 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 48B3B20869 for ; Fri, 12 Apr 2019 01:12:48 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="yRPZ6tuI" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726708AbfDLBMp (ORCPT ); Thu, 11 Apr 2019 21:12:45 -0400 Received: from mail-wm1-f67.google.com ([209.85.128.67]:40770 "EHLO mail-wm1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726640AbfDLBMo (ORCPT ); Thu, 11 Apr 2019 21:12:44 -0400 Received: by mail-wm1-f67.google.com with SMTP id z24so8942807wmi.5 for ; Thu, 11 Apr 2019 18:12:43 -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=+6t9fDuLGcu+F3sQB7g2ficOIVeXLxhNqKwl9GmNOHE=; b=yRPZ6tuIJ8qTSy3AYcGz9Vimw4SGUiH6/6xdFBD6xllQCoGYzJK2iZPSTH+MvV4ACF n+p2vkJMXfaVFYl58duMQhiVMEFvgoT0h/Y7FcOYFMVDQTVD1C6363Ute6pSRt5d8L7V 5MKjlkiwuTBZUf66CFNKvRwCeDop6gJO/34nKaxo82HjoMdSFAFe9WygWUhYVecdgoXZ Nz7GUKzw6dtDaM3ogJ9W5sZ6PJH3QTUDgvmMyVqMQ2Y6TpV7+jz5C9FMCyMOpgBVcZoW xuRVh5uv46cC50DiezLLSziaED0hZIEBpqHYE43YuFWIA9KCsbvPjBG2lQH1Awdi23y+ Z1dA== 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=+6t9fDuLGcu+F3sQB7g2ficOIVeXLxhNqKwl9GmNOHE=; b=DkDROtxCBFwG4ZhjGRAUwgNhAXL1lUv8xRVDprPboRJwNjx6jtlnyCPavPdP+ZDj1L ZzWmsNqIfc2MwSTzpU6S1UQObJiNdQ+oxOrxLXHzKx6F9f6K45oPZpYODKDTdyRU8hs5 WYQemP0gQYEiGekRuMuNFEtW3IvMpvcQxSgB+t5Zk/s04Wl8NG9i+wBmoM7l4BDfmoIi 9BbXef2OMCjvdBU36g8dEpKOmbAbjbgNodiFFhd5PWI0pto/LkvAWD7Vyws5sd8R9+ZE q8liiD8LHRv+CWC2Wi7WDqvJwtuQW+0Jpbb8mYqnVOwsB8g1JRTGOl279S0Z2TuIZ7HC dbcA== X-Gm-Message-State: APjAAAU3SSjSJjyiHfgeWbOP0Oo0xrrkP2/OwRYVODiDFeFP7ihhJTFn TRqcBa/bDzINXGhpbktBGPBUjz+dlsFwac3rimEVpA== X-Google-Smtp-Source: APXvYqwYuRDeUgwOa9EiKKBjsIxY1nRhXsSMG4LlmEAt0MXG+Zv9NmSEmInkrpRSEySYfHClcM4otK9ut+dM6HxszQ4= X-Received: by 2002:a1c:4d12:: with SMTP id o18mr8196227wmh.81.1555031562719; Thu, 11 Apr 2019 18:12:42 -0700 (PDT) MIME-Version: 1.0 References: <20190329041409.70138-1-chenyu56@huawei.com> <20190329041409.70138-10-chenyu56@huawei.com> In-Reply-To: <20190329041409.70138-10-chenyu56@huawei.com> From: John Stultz Date: Thu, 11 Apr 2019 18:12:30 -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, 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. 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 18:12:30 -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: <20190329041409.70138-10-chenyu56@huawei.com> 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, 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. 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 18:12:30 -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: T24gVGh1LCBNYXIgMjgsIDIwMTkgYXQgOToxNCBQTSBZdSBDaGVuIDxjaGVueXU1NkBodWF3ZWku Y29tPiB3cm90ZToKPgo+IFRoaXMgcGF0Y2ggYWRkcyBub3RpZmllciBmb3IgZHJpdmVycyB3YW50 IHRvIGJlIGluZm9ybWVkIG9mIHRoZSB1c2Igcm9sZQo+IHN3aXRjaC4KPgo+IENjOiBHcmVnIEty b2FoLUhhcnRtYW4gPGdyZWdraEBsaW51eGZvdW5kYXRpb24ub3JnPgo+IENjOiBIZWlra2kgS3Jv Z2VydXMgPGhlaWtraS5rcm9nZXJ1c0BsaW51eC5pbnRlbC5jb20+Cj4gQ2M6IEhhbnMgZGUgR29l ZGUgPGhkZWdvZWRlQHJlZGhhdC5jb20+Cj4gQ2M6IEFuZHkgU2hldmNoZW5rbyA8YW5keS5zaGV2 Y2hlbmtvQGdtYWlsLmNvbT4KPiBDYzogSm9obiBTdHVsdHogPGpvaG4uc3R1bHR6QGxpbmFyby5v cmc+Cj4gU3VnZ2VzdGVkLWJ5OiBIZWlra2kgS3JvZ2VydXMgPGhlaWtraS5rcm9nZXJ1c0BsaW51 eC5pbnRlbC5jb20+Cj4gU2lnbmVkLW9mZi1ieTogWXUgQ2hlbiA8Y2hlbnl1NTZAaHVhd2VpLmNv bT4KCkhleSBZdSBDaGVuIQogICBUaGFua3MgYWdhaW4gZm9yIHNlbmRpbmcgdGhpcyBwYXRjaCBv dXQhIEFzIG1lbnRpb25lZCBpbiBteQpjb21tZW50cyB3aXRoIHRoZSBvdGhlciBwYXRjaGVzLCBJ J3ZlIGdvdCBvbmUgcHJvcG9zYWwgSSB3YW50ZWQgdG8Kc2hhcmUgdG8gdHJ5IHRvIGF2b2lkICBz dGF0ZSBpbml0aWFsaXphdGlvbiByYWNlcyB0aGF0IEkndmUgc2VlbiB3aXRoCnRoaXMgcGF0Y2hz ZXQuCgo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL3VzYi9yb2xlcy9jbGFzcy5jIGIvZHJpdmVycy91 c2Ivcm9sZXMvY2xhc3MuYwo+IGluZGV4IGY0NWQ4ZGY1Y2ZiOC4uZTJjYWFhNjY1ZDZlIDEwMDY0 NAo+IC0tLSBhL2RyaXZlcnMvdXNiL3JvbGVzL2NsYXNzLmMKPiArKysgYi9kcml2ZXJzL3VzYi9y b2xlcy9jbGFzcy5jCj4gQEAgLTU4LDYgKzYxLDIwIEBAIGludCB1c2Jfcm9sZV9zd2l0Y2hfc2V0 X3JvbGUoc3RydWN0IHVzYl9yb2xlX3N3aXRjaCAqc3csIGVudW0gdXNiX3JvbGUgcm9sZSkKPiAg fQo+ICBFWFBPUlRfU1lNQk9MX0dQTCh1c2Jfcm9sZV9zd2l0Y2hfc2V0X3JvbGUpOwo+Cj4gK2lu dCB1c2Jfcm9sZV9zd2l0Y2hfcmVnaXN0ZXJfbm90aWZpZXIoc3RydWN0IHVzYl9yb2xlX3N3aXRj aCAqc3csCj4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBzdHJ1Y3Qgbm90 aWZpZXJfYmxvY2sgKm5iKQo+ICt7Cj4gKyAgICAgICByZXR1cm4gYmxvY2tpbmdfbm90aWZpZXJf Y2hhaW5fcmVnaXN0ZXIoJnN3LT5uaCwgbmIpOwo+ICt9Cj4gK0VYUE9SVF9TWU1CT0xfR1BMKHVz Yl9yb2xlX3N3aXRjaF9yZWdpc3Rlcl9ub3RpZmllcik7CgpBcyBub3RlZCBlYXJsaWVyLCBvbmUg aXNzdWUgSSd2ZSBzZWVuIGlzIHRoYXQgdGhlIGhpc2lfaGlrZXlfdXNiCmRyaXZlcidzIG5vdGlm aWVyIG1heSBub3QgZ2V0IGNhbGxlZCBlYXJseSBlbm91Z2ggdG8gcmVjZWl2ZQpub3RpZmljYXRp b24gb2YgdGhlIGluaXRpYWwgdXNiIHN0YXRlLgoKSXQgc2VlbXMgbGlrZSBvbiByZWdpc3RyYXRp b24gaGVyZSwgd2Ugc2hvdWxkIHRha2UgdGhlIGxvY2ssIHJlYWQgdGhlCnJvbGUgc3RhdGUgYW5k IGltbWVkaWF0ZWx5IGNhbGwgdGhlIG5vdGlmaWVyIHRvIHByb3Blcmx5IGluaXRpYWxpemUKaXQ/ IEkgc3VzcGVjdCB0aGF0IHNob3VsZCBjbG9zZSB0aGUgd2luZG93IGZvciBhbnkgc3RhdGUgcmFj ZXMgYXJvdW5kCmRyaXZlciBwcm9iZSB0aW1pbmdzIGFuZAoKRG9lcyB0aGF0IG1ha2Ugc2Vuc2U/ IEkgaGF2ZSByb3VnaGx5IHByb3RvdHlwZWQgdGhpcyBidXQgbmVlZCB0byBkbwphZGRpdGlvbmFs IHRlc3RpbmcuCgp0aGFua3MKLWpvaG4K