[PATCH] net: eth-phy: Handle gpio_request_by_name() return value

[PATCH] net: eth-phy: Handle gpio_request_by_name() return value

[Marek Vasut]

The gpio_request_by_name() returns zero in case of success, however the
conditional return value check in gpio_request_by_name() checks only for
(ret != -ENOENT) and if the condition is true, returns ret outright.

This leads to a situation where successful gpio_request_by_name() return
leads to immediate successful eth_phy_of_to_plat() return as well, and
to skipped parsing of "reset-assert-us" and "reset-deassert-us", so the
PHY driver operates with valid reset GPIO, but with assert/deassert times
set to default, which is 0, instead of the values from DT. This breaks
PHY reset.

Fix this by checking if return value is non-zero and then for this one
single allowed non-zero return value, -ENOENT.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Ramon Fried <rfried.dev@gmail.com>
---
 drivers/net/eth-phy-uclass.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/eth-phy-uclass.c b/drivers/net/eth-phy-uclass.c
index c04bab944d6..a9b358ee234 100644
--- a/drivers/net/eth-phy-uclass.c
+++ b/drivers/net/eth-phy-uclass.c
@@ -138,7 +138,7 @@ static int eth_phy_of_to_plat(struct udevice *dev)
 	ret = gpio_request_by_name(dev, "reset-gpios", 0,
 				   &uc_priv->reset_gpio,
 				   GPIOD_IS_OUT);
-	if (ret != -ENOENT)
+	if (ret && ret != -ENOENT)
 		return ret;
 
 	uc_priv->reset_assert_delay = dev_read_u32_default(dev, "reset-assert-us", 0);
-- 
2.33.0

Re: [PATCH] net: eth-phy: Handle gpio_request_by_name() return value

[Ramon Fried]

On Sat, Nov 13, 2021 at 4:23 AM Marek Vasut <marex@denx.de> wrote:
>
> The gpio_request_by_name() returns zero in case of success, however the
> conditional return value check in gpio_request_by_name() checks only for
> (ret != -ENOENT) and if the condition is true, returns ret outright.
>
> This leads to a situation where successful gpio_request_by_name() return
> leads to immediate successful eth_phy_of_to_plat() return as well, and
> to skipped parsing of "reset-assert-us" and "reset-deassert-us", so the
> PHY driver operates with valid reset GPIO, but with assert/deassert times
> set to default, which is 0, instead of the values from DT. This breaks
> PHY reset.
>
> Fix this by checking if return value is non-zero and then for this one
> single allowed non-zero return value, -ENOENT.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> ---
>  drivers/net/eth-phy-uclass.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/eth-phy-uclass.c b/drivers/net/eth-phy-uclass.c
> index c04bab944d6..a9b358ee234 100644
> --- a/drivers/net/eth-phy-uclass.c
> +++ b/drivers/net/eth-phy-uclass.c
> @@ -138,7 +138,7 @@ static int eth_phy_of_to_plat(struct udevice *dev)
>         ret = gpio_request_by_name(dev, "reset-gpios", 0,
>                                    &uc_priv->reset_gpio,
>                                    GPIOD_IS_OUT);
> -       if (ret != -ENOENT)
> +       if (ret && ret != -ENOENT)
>                 return ret;
>
>         uc_priv->reset_assert_delay = dev_read_u32_default(dev, "reset-assert-us", 0);
> --
> 2.33.0
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

Re: [PATCH] net: eth-phy: Handle gpio_request_by_name() return value

[Ramon Fried]

On Tue, Nov 16, 2021 at 7:52 AM Ramon Fried <rfried.dev@gmail.com> wrote:
>
> On Sat, Nov 13, 2021 at 4:23 AM Marek Vasut <marex@denx.de> wrote:
> >
> > The gpio_request_by_name() returns zero in case of success, however the
> > conditional return value check in gpio_request_by_name() checks only for
> > (ret != -ENOENT) and if the condition is true, returns ret outright.
> >
> > This leads to a situation where successful gpio_request_by_name() return
> > leads to immediate successful eth_phy_of_to_plat() return as well, and
> > to skipped parsing of "reset-assert-us" and "reset-deassert-us", so the
> > PHY driver operates with valid reset GPIO, but with assert/deassert times
> > set to default, which is 0, instead of the values from DT. This breaks
> > PHY reset.
> >
> > Fix this by checking if return value is non-zero and then for this one
> > single allowed non-zero return value, -ENOENT.
> >
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Ramon Fried <rfried.dev@gmail.com>
> > ---
> >  drivers/net/eth-phy-uclass.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/eth-phy-uclass.c b/drivers/net/eth-phy-uclass.c
> > index c04bab944d6..a9b358ee234 100644
> > --- a/drivers/net/eth-phy-uclass.c
> > +++ b/drivers/net/eth-phy-uclass.c
> > @@ -138,7 +138,7 @@ static int eth_phy_of_to_plat(struct udevice *dev)
> >         ret = gpio_request_by_name(dev, "reset-gpios", 0,
> >                                    &uc_priv->reset_gpio,
> >                                    GPIOD_IS_OUT);
> > -       if (ret != -ENOENT)
> > +       if (ret && ret != -ENOENT)
> >                 return ret;
> >
> >         uc_priv->reset_assert_delay = dev_read_u32_default(dev, "reset-assert-us", 0);
> > --
> > 2.33.0
> >
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
Applied to u-boot-net/next,
Thanks.

Re: [PATCH] net: eth-phy: Handle gpio_request_by_name() return value

[Marek Vasut]

On 12/2/21 06:03, Ramon Fried wrote:
> On Tue, Nov 16, 2021 at 7:52 AM Ramon Fried <rfried.dev@gmail.com> wrote:
>>
>> On Sat, Nov 13, 2021 at 4:23 AM Marek Vasut <marex@denx.de> wrote:
>>>
>>> The gpio_request_by_name() returns zero in case of success, however the
>>> conditional return value check in gpio_request_by_name() checks only for
>>> (ret != -ENOENT) and if the condition is true, returns ret outright.
>>>
>>> This leads to a situation where successful gpio_request_by_name() return
>>> leads to immediate successful eth_phy_of_to_plat() return as well, and
>>> to skipped parsing of "reset-assert-us" and "reset-deassert-us", so the
>>> PHY driver operates with valid reset GPIO, but with assert/deassert times
>>> set to default, which is 0, instead of the values from DT. This breaks
>>> PHY reset.
>>>
>>> Fix this by checking if return value is non-zero and then for this one
>>> single allowed non-zero return value, -ENOENT.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Ramon Fried <rfried.dev@gmail.com>
>>> ---
>>>   drivers/net/eth-phy-uclass.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/eth-phy-uclass.c b/drivers/net/eth-phy-uclass.c
>>> index c04bab944d6..a9b358ee234 100644
>>> --- a/drivers/net/eth-phy-uclass.c
>>> +++ b/drivers/net/eth-phy-uclass.c
>>> @@ -138,7 +138,7 @@ static int eth_phy_of_to_plat(struct udevice *dev)
>>>          ret = gpio_request_by_name(dev, "reset-gpios", 0,
>>>                                     &uc_priv->reset_gpio,
>>>                                     GPIOD_IS_OUT);
>>> -       if (ret != -ENOENT)
>>> +       if (ret && ret != -ENOENT)
>>>                  return ret;
>>>
>>>          uc_priv->reset_assert_delay = dev_read_u32_default(dev, "reset-assert-us", 0);
>>> --
>>> 2.33.0
>>>
>> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
> Applied to u-boot-net/next,

Note that this fixes a grave bug in the PHY reset code, so it should go 
into current release.

Re: [PATCH] net: eth-phy: Handle gpio_request_by_name() return value

[Ramon Fried]

On Thu, Dec 2, 2021 at 7:35 AM Marek Vasut <marex@denx.de> wrote:
>
> On 12/2/21 06:03, Ramon Fried wrote:
> > On Tue, Nov 16, 2021 at 7:52 AM Ramon Fried <rfried.dev@gmail.com> wrote:
> >>
> >> On Sat, Nov 13, 2021 at 4:23 AM Marek Vasut <marex@denx.de> wrote:
> >>>
> >>> The gpio_request_by_name() returns zero in case of success, however the
> >>> conditional return value check in gpio_request_by_name() checks only for
> >>> (ret != -ENOENT) and if the condition is true, returns ret outright.
> >>>
> >>> This leads to a situation where successful gpio_request_by_name() return
> >>> leads to immediate successful eth_phy_of_to_plat() return as well, and
> >>> to skipped parsing of "reset-assert-us" and "reset-deassert-us", so the
> >>> PHY driver operates with valid reset GPIO, but with assert/deassert times
> >>> set to default, which is 0, instead of the values from DT. This breaks
> >>> PHY reset.
> >>>
> >>> Fix this by checking if return value is non-zero and then for this one
> >>> single allowed non-zero return value, -ENOENT.
> >>>
> >>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>> Cc: Ramon Fried <rfried.dev@gmail.com>
> >>> ---
> >>>   drivers/net/eth-phy-uclass.c | 2 +-
> >>>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/net/eth-phy-uclass.c b/drivers/net/eth-phy-uclass.c
> >>> index c04bab944d6..a9b358ee234 100644
> >>> --- a/drivers/net/eth-phy-uclass.c
> >>> +++ b/drivers/net/eth-phy-uclass.c
> >>> @@ -138,7 +138,7 @@ static int eth_phy_of_to_plat(struct udevice *dev)
> >>>          ret = gpio_request_by_name(dev, "reset-gpios", 0,
> >>>                                     &uc_priv->reset_gpio,
> >>>                                     GPIOD_IS_OUT);
> >>> -       if (ret != -ENOENT)
> >>> +       if (ret && ret != -ENOENT)
> >>>                  return ret;
> >>>
> >>>          uc_priv->reset_assert_delay = dev_read_u32_default(dev, "reset-assert-us", 0);
> >>> --
> >>> 2.33.0
> >>>
> >> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
> > Applied to u-boot-net/next,
>
> Note that this fixes a grave bug in the PHY reset code, so it should go
> into current release.
OK.