From mboxrd@z Thu Jan 1 00:00:00 1970 From: Johan Hovold Subject: Re: [PATCH 2/2] usb: core: phy: fix return value checking about devm_of_phy_get_by_index() Date: Mon, 25 Jun 2018 07:29:30 +0200 Message-ID: <20180625052930.GA26803@localhost> References: <4c338a129ab14ab41949da5e3c21aed0d77dff8d.1529647619.git.chunfeng.yun@mediatek.com> <644d00edd2932c1a0d0a1bf368dd25427d1d9f64.1529647619.git.chunfeng.yun@mediatek.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Martin Blumenstingl Cc: chunfeng.yun@mediatek.com, johan@kernel.org, gregkh@linuxfoundation.org, felipe.balbi@linux.intel.com, matthias.bgg@gmail.com, linux-usb@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org List-Id: linux-mediatek@lists.infradead.org On Sun, Jun 24, 2018 at 08:00:01PM +0200, Martin Blumenstingl wrote: > Hello Chunfeng, > > thank you for the patch! > > On Fri, Jun 22, 2018 at 8:33 AM Chunfeng Yun wrote: > > 2. devm_of_phy_get_by_index() should not fail for a valid index > I have learned that the PHY framework can return -ENODEV if the PHY is: > 1. supposed to be handled by the legacy USB PHY framework > 2. the PHY node is disabled in devicetree > > see [0] for the code in the PHY framework and [1] for the discussion > with Johan (who informed me of case #1, I added him on this mail) > > > Signed-off-by: Chunfeng Yun > > --- > > drivers/usb/core/phy.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c > > index 9879767..0f972e1 100644 > > --- a/drivers/usb/core/phy.c > > +++ b/drivers/usb/core/phy.c > > @@ -23,14 +23,11 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index, > > struct list_head *list) > > { > > struct usb_phy_roothub *roothub_entry; > > - struct phy *phy = devm_of_phy_get_by_index(dev, dev->of_node, index); > > + struct phy *phy; > > > > - if (IS_ERR_OR_NULL(phy)) { > > - if (!phy || PTR_ERR(phy) == -ENODEV) > > - return 0; > > - else > > - return PTR_ERR(phy); > > - } > > + phy = devm_of_phy_get_by_index(dev, dev->of_node, index); > > + if (IS_ERR(phy)) > > + return PTR_ERR(phy); > @Johan can you please review this as well? maybe we need to keep the > -ENODEV check Indeed, the -ENODEV check is still needed for the reasons you point out above. Johan 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: [2/2] usb: core: phy: fix return value checking about devm_of_phy_get_by_index() From: Johan Hovold Message-Id: <20180625052930.GA26803@localhost> Date: Mon, 25 Jun 2018 07:29:30 +0200 To: Martin Blumenstingl Cc: chunfeng.yun@mediatek.com, johan@kernel.org, gregkh@linuxfoundation.org, felipe.balbi@linux.intel.com, matthias.bgg@gmail.com, linux-usb@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org List-ID: T24gU3VuLCBKdW4gMjQsIDIwMTggYXQgMDg6MDA6MDFQTSArMDIwMCwgTWFydGluIEJsdW1lbnN0 aW5nbCB3cm90ZToKPiBIZWxsbyBDaHVuZmVuZywKPiAKPiB0aGFuayB5b3UgZm9yIHRoZSBwYXRj aCEKPiAKPiBPbiBGcmksIEp1biAyMiwgMjAxOCBhdCA4OjMzIEFNIENodW5mZW5nIFl1biA8Y2h1 bmZlbmcueXVuQG1lZGlhdGVrLmNvbT4gd3JvdGU6Cgo+ID4gMi4gZGV2bV9vZl9waHlfZ2V0X2J5 X2luZGV4KCkgc2hvdWxkIG5vdCBmYWlsIGZvciBhIHZhbGlkIGluZGV4Cj4gSSBoYXZlIGxlYXJu ZWQgdGhhdCB0aGUgUEhZIGZyYW1ld29yayBjYW4gcmV0dXJuIC1FTk9ERVYgaWYgdGhlIFBIWSBp czoKPiAxLiBzdXBwb3NlZCB0byBiZSBoYW5kbGVkIGJ5IHRoZSBsZWdhY3kgVVNCIFBIWSBmcmFt ZXdvcmsKPiAyLiB0aGUgUEhZIG5vZGUgaXMgZGlzYWJsZWQgaW4gZGV2aWNldHJlZQo+IAo+IHNl ZSBbMF0gZm9yIHRoZSBjb2RlIGluIHRoZSBQSFkgZnJhbWV3b3JrIGFuZCBbMV0gZm9yIHRoZSBk aXNjdXNzaW9uCj4gd2l0aCBKb2hhbiAod2hvIGluZm9ybWVkIG1lIG9mIGNhc2UgIzEsIEkgYWRk ZWQgaGltIG9uIHRoaXMgbWFpbCkKPiAKPiA+IFNpZ25lZC1vZmYtYnk6IENodW5mZW5nIFl1biA8 Y2h1bmZlbmcueXVuQG1lZGlhdGVrLmNvbT4KPiA+IC0tLQo+ID4gIGRyaXZlcnMvdXNiL2NvcmUv cGh5LmMgfCAxMSArKysrLS0tLS0tLQo+ID4gIDEgZmlsZSBjaGFuZ2VkLCA0IGluc2VydGlvbnMo KyksIDcgZGVsZXRpb25zKC0pCj4gPgo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvdXNiL2NvcmUv cGh5LmMgYi9kcml2ZXJzL3VzYi9jb3JlL3BoeS5jCj4gPiBpbmRleCA5ODc5NzY3Li4wZjk3MmUx IDEwMDY0NAo+ID4gLS0tIGEvZHJpdmVycy91c2IvY29yZS9waHkuYwo+ID4gKysrIGIvZHJpdmVy cy91c2IvY29yZS9waHkuYwo+ID4gQEAgLTIzLDE0ICsyMywxMSBAQCBzdGF0aWMgaW50IHVzYl9w aHlfcm9vdGh1Yl9hZGRfcGh5KHN0cnVjdCBkZXZpY2UgKmRldiwgaW50IGluZGV4LAo+ID4gICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBzdHJ1Y3QgbGlzdF9oZWFkICpsaXN0KQo+ ID4gIHsKPiA+ICAgICAgICAgc3RydWN0IHVzYl9waHlfcm9vdGh1YiAqcm9vdGh1Yl9lbnRyeTsK PiA+IC0gICAgICAgc3RydWN0IHBoeSAqcGh5ID0gZGV2bV9vZl9waHlfZ2V0X2J5X2luZGV4KGRl diwgZGV2LT5vZl9ub2RlLCBpbmRleCk7Cj4gPiArICAgICAgIHN0cnVjdCBwaHkgKnBoeTsKPiA+ Cj4gPiAtICAgICAgIGlmIChJU19FUlJfT1JfTlVMTChwaHkpKSB7Cj4gPiAtICAgICAgICAgICAg ICAgaWYgKCFwaHkgfHwgUFRSX0VSUihwaHkpID09IC1FTk9ERVYpCj4gPiAtICAgICAgICAgICAg ICAgICAgICAgICByZXR1cm4gMDsKPiA+IC0gICAgICAgICAgICAgICBlbHNlCj4gPiAtICAgICAg ICAgICAgICAgICAgICAgICByZXR1cm4gUFRSX0VSUihwaHkpOwo+ID4gLSAgICAgICB9Cj4gPiAr ICAgICAgIHBoeSA9IGRldm1fb2ZfcGh5X2dldF9ieV9pbmRleChkZXYsIGRldi0+b2Zfbm9kZSwg aW5kZXgpOwo+ID4gKyAgICAgICBpZiAoSVNfRVJSKHBoeSkpCj4gPiArICAgICAgICAgICAgICAg cmV0dXJuIFBUUl9FUlIocGh5KTsKPiBASm9oYW4gY2FuIHlvdSBwbGVhc2UgcmV2aWV3IHRoaXMg YXMgd2VsbD8gbWF5YmUgd2UgbmVlZCB0byBrZWVwIHRoZQo+IC1FTk9ERVYgY2hlY2sKCkluZGVl ZCwgdGhlIC1FTk9ERVYgY2hlY2sgaXMgc3RpbGwgbmVlZGVkIGZvciB0aGUgcmVhc29ucyB5b3Ug cG9pbnQgb3V0CmFib3ZlLgoKSm9oYW4KLS0tClRvIHVuc3Vic2NyaWJlIGZyb20gdGhpcyBsaXN0 OiBzZW5kIHRoZSBsaW5lICJ1bnN1YnNjcmliZSBsaW51eC11c2IiIGluCnRoZSBib2R5IG9mIGEg bWVzc2FnZSB0byBtYWpvcmRvbW9Admdlci5rZXJuZWwub3JnCk1vcmUgbWFqb3Jkb21vIGluZm8g YXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFqb3Jkb21vLWluZm8uaHRtbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: johan@kernel.org (Johan Hovold) Date: Mon, 25 Jun 2018 07:29:30 +0200 Subject: [PATCH 2/2] usb: core: phy: fix return value checking about devm_of_phy_get_by_index() In-Reply-To: References: <4c338a129ab14ab41949da5e3c21aed0d77dff8d.1529647619.git.chunfeng.yun@mediatek.com> <644d00edd2932c1a0d0a1bf368dd25427d1d9f64.1529647619.git.chunfeng.yun@mediatek.com> Message-ID: <20180625052930.GA26803@localhost> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Sun, Jun 24, 2018 at 08:00:01PM +0200, Martin Blumenstingl wrote: > Hello Chunfeng, > > thank you for the patch! > > On Fri, Jun 22, 2018 at 8:33 AM Chunfeng Yun wrote: > > 2. devm_of_phy_get_by_index() should not fail for a valid index > I have learned that the PHY framework can return -ENODEV if the PHY is: > 1. supposed to be handled by the legacy USB PHY framework > 2. the PHY node is disabled in devicetree > > see [0] for the code in the PHY framework and [1] for the discussion > with Johan (who informed me of case #1, I added him on this mail) > > > Signed-off-by: Chunfeng Yun > > --- > > drivers/usb/core/phy.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c > > index 9879767..0f972e1 100644 > > --- a/drivers/usb/core/phy.c > > +++ b/drivers/usb/core/phy.c > > @@ -23,14 +23,11 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index, > > struct list_head *list) > > { > > struct usb_phy_roothub *roothub_entry; > > - struct phy *phy = devm_of_phy_get_by_index(dev, dev->of_node, index); > > + struct phy *phy; > > > > - if (IS_ERR_OR_NULL(phy)) { > > - if (!phy || PTR_ERR(phy) == -ENODEV) > > - return 0; > > - else > > - return PTR_ERR(phy); > > - } > > + phy = devm_of_phy_get_by_index(dev, dev->of_node, index); > > + if (IS_ERR(phy)) > > + return PTR_ERR(phy); > @Johan can you please review this as well? maybe we need to keep the > -ENODEV check Indeed, the -ENODEV check is still needed for the reasons you point out above. Johan