[PATCH] ftgmac100: use bus name in mdio error messages

[PATCH] ftgmac100: use bus name in mdio error messages

[Zev Weiss]

Previously we'd been using a device name retrieved via
ftgmac100_data->phydev, but the mdio read/write functions may be
called before that member is initialized in ftgmac100_phy_init(),
leading to a NULL pointer dereference while printing the error message
issued if the mdio access fails.  We can instead use bus->name, which
is already available at that point.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
Fixes: 538e75d3fc54 ("net: ftgmac100: add MDIO bus and phylib support")
---
 drivers/net/ftgmac100.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ftgmac100.c b/drivers/net/ftgmac100.c
index 999941de14ac..aa1d63880c8c 100644
--- a/drivers/net/ftgmac100.c
+++ b/drivers/net/ftgmac100.c
@@ -117,7 +117,7 @@ static int ftgmac100_mdio_read(struct mii_dev *bus, int phy_addr, int dev_addr,
 				 FTGMAC100_MDIO_TIMEOUT_USEC);
 	if (ret) {
 		pr_err("%s: mdio read failed (phy:%d reg:%x)\n",
-		       priv->phydev->dev->name, phy_addr, reg_addr);
+		       bus->name, phy_addr, reg_addr);
 		return ret;
 	}
 
@@ -149,7 +149,7 @@ static int ftgmac100_mdio_write(struct mii_dev *bus, int phy_addr, int dev_addr,
 				 FTGMAC100_MDIO_TIMEOUT_USEC);
 	if (ret) {
 		pr_err("%s: mdio write failed (phy:%d reg:%x)\n",
-		       priv->phydev->dev->name, phy_addr, reg_addr);
+		       bus->name, phy_addr, reg_addr);
 	}
 
 	return ret;
-- 
2.36.1

Re: [PATCH] ftgmac100: use bus name in mdio error messages

[Zev Weiss]

On Mon, May 16, 2022 at 09:38:25PM PDT, Zev Weiss wrote:
> Previously we'd been using a device name retrieved via
> ftgmac100_data->phydev, but the mdio read/write functions may be
> called before that member is initialized in ftgmac100_phy_init(),
> leading to a NULL pointer dereference while printing the error message
> issued if the mdio access fails.  We can instead use bus->name, which
> is already available at that point.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> Fixes: 538e75d3fc54 ("net: ftgmac100: add MDIO bus and phylib support")
> ---
>  drivers/net/ftgmac100.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Sorry, forgot the subject-prefix tag on this one -- this is for u-boot 
v2019.04-aspeed-openbmc in case it wasn't clear.

Zev

Re: [PATCH] ftgmac100: use bus name in mdio error messages

[Joel Stanley]

On Tue, 17 May 2022 at 04:51, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On Mon, May 16, 2022 at 09:38:25PM PDT, Zev Weiss wrote:
> > Previously we'd been using a device name retrieved via
> > ftgmac100_data->phydev, but the mdio read/write functions may be
> > called before that member is initialized in ftgmac100_phy_init(),
> > leading to a NULL pointer dereference while printing the error message
> > issued if the mdio access fails.  We can instead use bus->name, which
> > is already available at that point.
> >
> > Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> > Fixes: 538e75d3fc54 ("net: ftgmac100: add MDIO bus and phylib support")
> > ---
> >  drivers/net/ftgmac100.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
>
> Sorry, forgot the subject-prefix tag on this one -- this is for u-boot
> v2019.04-aspeed-openbmc in case it wasn't clear.

Thanks, I figured that out :)

How do you reproduce this one?

I didn't realise we had downstream changes for this driver, we should
ask aspeed to submit their downstream changes to u-boot mainline.

Re: [PATCH] ftgmac100: use bus name in mdio error messages

[Zev Weiss]

On Mon, May 16, 2022 at 09:54:35PM PDT, Joel Stanley wrote:
> On Tue, 17 May 2022 at 04:51, Zev Weiss <zev@bewilderbeest.net> wrote:
> >
> > On Mon, May 16, 2022 at 09:38:25PM PDT, Zev Weiss wrote:
> > > Previously we'd been using a device name retrieved via
> > > ftgmac100_data->phydev, but the mdio read/write functions may be
> > > called before that member is initialized in ftgmac100_phy_init(),
> > > leading to a NULL pointer dereference while printing the error message
> > > issued if the mdio access fails.  We can instead use bus->name, which
> > > is already available at that point.
> > >
> > > Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> > > Fixes: 538e75d3fc54 ("net: ftgmac100: add MDIO bus and phylib support")
> > > ---
> > >  drivers/net/ftgmac100.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> >
> > Sorry, forgot the subject-prefix tag on this one -- this is for u-boot
> > v2019.04-aspeed-openbmc in case it wasn't clear.
> 
> Thanks, I figured that out :)
> 
> How do you reproduce this one?
> 

I'm in the process of trying to transition e3c246d4i off of the 2016 
branch, and with the 2019 branch I'm finding u-boot spewing a bunch of 
output like

  : mdio read failed (phy:0 reg:2)
  : mdio read failed (phy:1 reg:2)
  : mdio read failed (phy:2 reg:2)
  ...

(sometimes with some amount of garbage before the colon at the start of 
the line, and with "eth1" instead after this patch.)

I'm currently experimenting with various Kconfig settings and dts tweaks 
(on two different variants of the hardware) to try to narrow down the 
underlying cause and hopefully eliminate the error spew entirely, but in 
the meantime I figured we might as well get that error-reporting path 
smoothed out a bit.

> I didn't realise we had downstream changes for this driver, we should
> ask aspeed to submit their downstream changes to u-boot mainline.

That sounds appropriate -- though as Cédric pointed out, since the 
relevant parts for this particular patch are already in mainline u-boot 
I should probably send this upstream as well.

Re: [PATCH] ftgmac100: use bus name in mdio error messages

[Joel Stanley]

On Tue, 17 May 2022 at 09:28, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On Mon, May 16, 2022 at 09:54:35PM PDT, Joel Stanley wrote:
> > On Tue, 17 May 2022 at 04:51, Zev Weiss <zev@bewilderbeest.net> wrote:
> > >
> > > On Mon, May 16, 2022 at 09:38:25PM PDT, Zev Weiss wrote:
> > > > Previously we'd been using a device name retrieved via
> > > > ftgmac100_data->phydev, but the mdio read/write functions may be
> > > > called before that member is initialized in ftgmac100_phy_init(),
> > > > leading to a NULL pointer dereference while printing the error message
> > > > issued if the mdio access fails.  We can instead use bus->name, which
> > > > is already available at that point.
> > > >
> > > > Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> > > > Fixes: 538e75d3fc54 ("net: ftgmac100: add MDIO bus and phylib support")
> > > > ---
> > > >  drivers/net/ftgmac100.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > >
> > > Sorry, forgot the subject-prefix tag on this one -- this is for u-boot
> > > v2019.04-aspeed-openbmc in case it wasn't clear.
> >
> > Thanks, I figured that out :)
> >
> > How do you reproduce this one?
> >
>
> I'm in the process of trying to transition e3c246d4i off of the 2016
> branch, and with the 2019 branch I'm finding u-boot spewing a bunch of
> output like
>
>   : mdio read failed (phy:0 reg:2)
>   : mdio read failed (phy:1 reg:2)
>   : mdio read failed (phy:2 reg:2)
>   ...
>
> (sometimes with some amount of garbage before the colon at the start of
> the line, and with "eth1" instead after this patch.)

Interesting. Are all of the clocks turned on in the same way as the
old branch? Dumping the SCU registers might give you a clue.

>
> I'm currently experimenting with various Kconfig settings and dts tweaks
> (on two different variants of the hardware) to try to narrow down the
> underlying cause and hopefully eliminate the error spew entirely, but in
> the meantime I figured we might as well get that error-reporting path
> smoothed out a bit.

Thanks for the explanation.

I'll apply it to our tree now, but also send it to mainline when you
get a chance.

>
> > I didn't realise we had downstream changes for this driver, we should
> > ask aspeed to submit their downstream changes to u-boot mainline.
>
> That sounds appropriate -- though as Cédric pointed out, since the
> relevant parts for this particular patch are already in mainline u-boot
> I should probably send this upstream as well.

+1

Re: [PATCH] ftgmac100: use bus name in mdio error messages

[Cédric Le Goater]

Hello Zev,

On 5/17/22 06:38, Zev Weiss wrote:
> Previously we'd been using a device name retrieved via
> ftgmac100_data->phydev, but the mdio read/write functions may be
> called before that member is initialized in ftgmac100_phy_init(),
> leading to a NULL pointer dereference while printing the error message
> issued if the mdio access fails.  We can instead use bus->name, which
> is already available at that point.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> Fixes: 538e75d3fc54 ("net: ftgmac100: add MDIO bus and phylib support")

You should send upstream.

Thanks,

C.

> ---
>   drivers/net/ftgmac100.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ftgmac100.c b/drivers/net/ftgmac100.c
> index 999941de14ac..aa1d63880c8c 100644
> --- a/drivers/net/ftgmac100.c
> +++ b/drivers/net/ftgmac100.c
> @@ -117,7 +117,7 @@ static int ftgmac100_mdio_read(struct mii_dev *bus, int phy_addr, int dev_addr,
>   				 FTGMAC100_MDIO_TIMEOUT_USEC);
>   	if (ret) {
>   		pr_err("%s: mdio read failed (phy:%d reg:%x)\n",
> -		       priv->phydev->dev->name, phy_addr, reg_addr);
> +		       bus->name, phy_addr, reg_addr);
>   		return ret;
>   	}
>   
> @@ -149,7 +149,7 @@ static int ftgmac100_mdio_write(struct mii_dev *bus, int phy_addr, int dev_addr,
>   				 FTGMAC100_MDIO_TIMEOUT_USEC);
>   	if (ret) {
>   		pr_err("%s: mdio write failed (phy:%d reg:%x)\n",
> -		       priv->phydev->dev->name, phy_addr, reg_addr);
> +		       bus->name, phy_addr, reg_addr);
>   	}
>   
>   	return ret;