| Message ID | 644d00edd2932c1a0d0a1bf368dd25427d1d9f64.1529647619.git.chunfeng.yun@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Chunfeng, thank you for the patch! On Fri, Jun 22, 2018 at 8:33 AM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote: > > 1. use IS_ERR() but not IS_ERR_OR_NULL() because devm_of_phy_get_by_index() > never return NULL value; ACK - good catch! > 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 <chunfeng.yun@mediatek.com> > --- > 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 Regards Martin [0] https://elixir.bootlin.com/linux/v4.18-rc2/source/drivers/phy/phy-core.c#L421 [1] https://lkml.org/lkml/2018/4/19/88
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 <chunfeng.yun@mediatek.com> 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 <chunfeng.yun@mediatek.com> > > --- > > 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
On Sun, 2018-06-24 at 20:00 +0200, Martin Blumenstingl wrote: > Hello Chunfeng, > > thank you for the patch! > > On Fri, Jun 22, 2018 at 8:33 AM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote: > > > > 1. use IS_ERR() but not IS_ERR_OR_NULL() because devm_of_phy_get_by_index() > > never return NULL value; > ACK - good catch! > > > 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 A little confused, we can avoid this error by not using a disabled or "usb-nop-xceiv" phy in host node. If ignore error of -ENODEV, we will also skip an error case which we don't want it happen. > > 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 <chunfeng.yun@mediatek.com> > > --- > > 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 > > > Regards > Martin > > > [0] https://elixir.bootlin.com/linux/v4.18-rc2/source/drivers/phy/phy-core.c#L421 > [1] https://lkml.org/lkml/2018/4/19/88
Hi Chunfeng, On Mon, Jun 25, 2018 at 8:37 AM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote: > > On Sun, 2018-06-24 at 20:00 +0200, Martin Blumenstingl wrote: > > Hello Chunfeng, > > > > thank you for the patch! > > > > On Fri, Jun 22, 2018 at 8:33 AM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote: > > > > > > 1. use IS_ERR() but not IS_ERR_OR_NULL() because devm_of_phy_get_by_index() > > > never return NULL value; > > ACK - good catch! > > > > > 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 > A little confused, we can avoid this error by not using a disabled or > "usb-nop-xceiv" phy in host node. I think that would be the mid or long term goal > If ignore error of -ENODEV, we will also skip an error case which we > don't want it happen. as far as I understand the problem is that there are .dts files out there which specify "usb-nop-xceiv" in the "phys" property, see [0] (there might be even some more out-of-tree cases) so I think the -ENODEV check can only be removed once nothing uses "usb-nop-xceiv" anymore Regards Martin [0] https://patchwork.kernel.org/patch/10160181/
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); roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL); if (!roothub_entry)
1. use IS_ERR() but not IS_ERR_OR_NULL() because devm_of_phy_get_by_index() never return NULL value; 2. devm_of_phy_get_by_index() should not fail for a valid index Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> --- drivers/usb/core/phy.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)