| Message ID | 20211113022311.231744-1-marex@denx.de |
|---|---|
| State | Accepted |
| Commit | 8777033722719a37eac8d07efa3e4b3a665612e1 |
| Delegated to: | Ramon Fried |
| Headers | show |
| Series | net: eth-phy: Handle gpio_request_by_name() return value | expand |
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>
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.
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.
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.
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);
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(-)