diff mbox series

[v2] net: ti: am65-cpsw-nuss: handle missing PHY in am65_cpsw_phy_init() gracefully

Message ID 20240328062914.486999-1-alexander.sverdlin@siemens.com
State Changes Requested
Delegated to: Tom Rini
Headers show
Series [v2] net: ti: am65-cpsw-nuss: handle missing PHY in am65_cpsw_phy_init() gracefully | expand

Commit Message

Sverdlin, Alexander March 28, 2024, 6:29 a.m. UTC 
From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

am65_cpsw_ofdata_parse_phy() tries to handle the case when PHY is not
specified in DT gracefully:

am65_cpsw_nuss_port ethernet@8000000port@1: can't parse phy-handle port 1 (-2)

am65_cpsw_mdio_init() in turn is prepared for this, checks
if priv->has_phy == 0 and bails out (leaving cpsw_common->bus == NULL).

am65_cpsw_phy_init() however is not prepared for this and calls
phy_connect(cpsw_common->bus, ...) unconditionally, which leads to:

"Synchronous Abort" handler, esr 0x8600000d, far 0x0
elr: ffffffff808ab000 lr : 000000008083bde4 (reloc)

where lr points to the instruction right after bus->read() in get_phy_id().

Fixes: 9d0dca1199d1 ("net: ethernet: ti: Introduce am654 gigabit eth switch subsystem driver")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
v2: rewritten subject; "is turn" -> "in turn" further down in message body

 drivers/net/ti/am65-cpsw-nuss.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Siddharth Vadapalli March 28, 2024, 6:46 a.m. UTC | #1 
On Thu, Mar 28, 2024 at 07:29:10AM +0100, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> am65_cpsw_ofdata_parse_phy() tries to handle the case when PHY is not
> specified in DT gracefully:
> 
> am65_cpsw_nuss_port ethernet@8000000port@1: can't parse phy-handle port 1 (-2)
> 
> am65_cpsw_mdio_init() in turn is prepared for this, checks
> if priv->has_phy == 0 and bails out (leaving cpsw_common->bus == NULL).
> 
> am65_cpsw_phy_init() however is not prepared for this and calls
> phy_connect(cpsw_common->bus, ...) unconditionally, which leads to:
> 
> "Synchronous Abort" handler, esr 0x8600000d, far 0x0
> elr: ffffffff808ab000 lr : 000000008083bde4 (reloc)
> 
> where lr points to the instruction right after bus->read() in get_phy_id().
> 
> Fixes: 9d0dca1199d1 ("net: ethernet: ti: Introduce am654 gigabit eth switch subsystem driver")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>

Regards,
Siddharth.
Nishanth Menon March 28, 2024, 11:45 a.m. UTC | #2 
On 07:29-20240328, A. Sverdlin wrote:
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> am65_cpsw_ofdata_parse_phy() tries to handle the case when PHY is not
> specified in DT gracefully:
> 
> am65_cpsw_nuss_port ethernet@8000000port@1: can't parse phy-handle port 1 (-2)
> 
> am65_cpsw_mdio_init() in turn is prepared for this, checks
> if priv->has_phy == 0 and bails out (leaving cpsw_common->bus == NULL).
> 
> am65_cpsw_phy_init() however is not prepared for this and calls
> phy_connect(cpsw_common->bus, ...) unconditionally, which leads to:
> 
> "Synchronous Abort" handler, esr 0x8600000d, far 0x0
> elr: ffffffff808ab000 lr : 000000008083bde4 (reloc)
> 
> where lr points to the instruction right after bus->read() in get_phy_id().
> 
> Fixes: 9d0dca1199d1 ("net: ethernet: ti: Introduce am654 gigabit eth switch subsystem driver")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> ---
> v2: rewritten subject; "is turn" -> "in turn" further down in message body
> 
>  drivers/net/ti/am65-cpsw-nuss.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
> index 6da018c0f9d5..d1e452b6b43c 100644
> --- a/drivers/net/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ti/am65-cpsw-nuss.c
> @@ -722,6 +722,9 @@ static int am65_cpsw_phy_init(struct udevice *dev)
>  	u32 supported = PHY_GBIT_FEATURES;
>  	int ret;
>  
> +	if (!priv->has_phy || !cpsw_common->bus)
> +		return 0;
> +
>  	phydev = phy_connect(cpsw_common->bus,
>  			     priv->phy_addr,
>  			     priv->dev,
> -- 
> 2.44.0
> 

Reviewed-by: Nishanth Menon <nm@ti.com>


Thanks for fixing this.

Btw, this no longer applies on next. only applies on master. Depending
on where Tom would like to apply this change, might need a rebase.

CC Roger.
Sverdlin, Alexander March 28, 2024, 11:53 a.m. UTC | #3 
Hi Nishanth!

On Thu, 2024-03-28 at 06:45 -0500, Nishanth Menon wrote:
> On 07:29-20240328, A. Sverdlin wrote:
> > From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > 
> > am65_cpsw_ofdata_parse_phy() tries to handle the case when PHY is not
> > specified in DT gracefully:
> > 
> > am65_cpsw_nuss_port ethernet@8000000port@1: can't parse phy-handle port 1 (-2)
> > 
> > am65_cpsw_mdio_init() in turn is prepared for this, checks
> > if priv->has_phy == 0 and bails out (leaving cpsw_common->bus == NULL).
> > 
> > am65_cpsw_phy_init() however is not prepared for this and calls
> > phy_connect(cpsw_common->bus, ...) unconditionally, which leads to:
> > 
> > "Synchronous Abort" handler, esr 0x8600000d, far 0x0
> > elr: ffffffff808ab000 lr : 000000008083bde4 (reloc)
> > 
> > where lr points to the instruction right after bus->read() in get_phy_id().
> > 
> > Fixes: 9d0dca1199d1 ("net: ethernet: ti: Introduce am654 gigabit eth switch subsystem driver")
> > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> > ---
> > v2: rewritten subject; "is turn" -> "in turn" further down in message body
> > 
> >   drivers/net/ti/am65-cpsw-nuss.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
> > index 6da018c0f9d5..d1e452b6b43c 100644
> > --- a/drivers/net/ti/am65-cpsw-nuss.c
> > +++ b/drivers/net/ti/am65-cpsw-nuss.c
> > @@ -722,6 +722,9 @@ static int am65_cpsw_phy_init(struct udevice *dev)
> >         u32 supported = PHY_GBIT_FEATURES;
> >         int ret;
> >   
> > +       if (!priv->has_phy || !cpsw_common->bus)
> > +               return 0;
> > +
> >         phydev = phy_connect(cpsw_common->bus,
> >                              priv->phy_addr,
> >                              priv->dev,
> > -- 
> > 2.44.0
> > 
> 
> Reviewed-by: Nishanth Menon <nm@ti.com>
> 
> 
> Thanks for fixing this.
> 
> Btw, this no longer applies on next. only applies on master. Depending
> on where Tom would like to apply this change, might need a rebase.

Thanks for the hint! I've totally missed this!
And I'm even not sure the patch makes sense on "next".
I'd need to retest if the problem is still there (and if there would be one,
then it's rather subsystem problem, not am65-cpsw any more.

So the patch only makes sense for stable branches, if any.
Nishanth Menon March 28, 2024, 12:04 p.m. UTC | #4 
On 11:53-20240328, Sverdlin, Alexander wrote:
[..]
> > Btw, this no longer applies on next. only applies on master. Depending
> > on where Tom would like to apply this change, might need a rebase.
> 
> Thanks for the hint! I've totally missed this!
> And I'm even not sure the patch makes sense on "next".
> I'd need to retest if the problem is still there (and if there would be one,
> then it's rather subsystem problem, not am65-cpsw any more.
> 
> So the patch only makes sense for stable branches, if any.

On next, I think a variant of the patch will be needed as well. I see
the same bug on latest next as well (ab8d9ca3044a Merge tag
'v2024.04-rc5' into next)

https://gist.github.com/nmenon/691ccd210ed7e1add4c865ca9a698f2e
diff mbox series

Patch

diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
index 6da018c0f9d5..d1e452b6b43c 100644
--- a/drivers/net/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ti/am65-cpsw-nuss.c
@@ -722,6 +722,9 @@  static int am65_cpsw_phy_init(struct udevice *dev)
 	u32 supported = PHY_GBIT_FEATURES;
 	int ret;
 
+	if (!priv->has_phy || !cpsw_common->bus)
+		return 0;
+
 	phydev = phy_connect(cpsw_common->bus,
 			     priv->phy_addr,
 			     priv->dev,