Re: [net-next] net: phy: marvell-88q2xxx: Init PHY private structure for mv88q211x

[Dimitri Fedrau <dima.fedrau@gmail.com>]

Hi Niklas,

Am Fri, Feb 14, 2025 at 06:46:50PM +0100 schrieb Niklas Söderlund:
> When adding LED support for mv88q222x devices the PHY private data
> structure was added to the mv88q211x code path, the data structure is
> however only allocated during mv88q222x probe. This results in a nullptr
> deference for mv88q2110 devices.
> 
> 	Unable to handle kernel NULL pointer dereference at virtual address 0000000000000001
> 	Mem abort info:
> 	  ESR = 0x0000000096000004
> 	  EC = 0x25: DABT (current EL), IL = 32 bits
> 	  SET = 0, FnV = 0
> 	  EA = 0, S1PTW = 0
> 	  FSC = 0x04: level 0 translation fault
> 	Data abort info:
> 	  ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
> 	  CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> 	  GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> 	[0000000000000001] user address but active_mm is swapper
> 	Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> 	CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0-rc1-arm64-renesas-00342-ga3783dbf2574 #7
> 	Hardware name: Renesas White Hawk Single board based on r8a779g2 (DT)
> 	pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> 	pc : mv88q2xxx_config_init+0x28/0x84
> 	lr : mv88q2110_config_init+0x98/0xb0
> 	sp : ffff8000823eb9d0
> 	x29: ffff8000823eb9d0 x28: ffff000440942000 x27: ffff80008144e400
> 	x26: 0000000000001002 x25: 0000000000000000 x24: 0000000000000000
> 	x23: 0000000000000009 x22: ffff8000810534f0 x21: ffff800081053550
> 	x20: 0000000000000000 x19: ffff0004437d6800 x18: 0000000000000018
> 	x17: 00000000000961c8 x16: ffff0006bef75ec0 x15: 0000000000000001
> 	x14: 0000000000000001 x13: ffff000440218080 x12: 071c71c71c71c71c
> 	x11: ffff000440218080 x10: 0000000000001420 x9 : ffff8000823eb770
> 	x8 : ffff8000823eb650 x7 : ffff8000823eb750 x6 : ffff8000823eb710
> 	x5 : 0000000000000000 x4 : 0000000000000800 x3 : 0000000000000001
> 	x2 : 0000000000000000 x1 : 00000000ffffffff x0 : ffff0004437d6800
> 	Call trace:
> 	 mv88q2xxx_config_init+0x28/0x84 (P)
> 	 mv88q2110_config_init+0x98/0xb0
> 	 phy_init_hw+0x64/0x9c
> 	 phy_attach_direct+0x118/0x320
> 	 phy_connect_direct+0x24/0x80
> 	 of_phy_connect+0x5c/0xa0
> 	 rtsn_open+0x5bc/0x78c
>	 __dev_open+0xf8/0x1fc
> 	 __dev_change_flags+0x198/0x220
> 	 dev_change_flags+0x20/0x64
> 	 ip_auto_config+0x270/0xefc
> 	 do_one_initcall+0xe4/0x22c
> 	 kernel_init_freeable+0x2a8/0x308
> 	 kernel_init+0x20/0x130
> 	 ret_from_fork+0x10/0x20
> 	Code: b907e404 f9432814 3100083f 540000e3 (39400680)
> 	---[ end trace 0000000000000000 ]---
> 	Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> 	SMP: stopping secondary CPUs
> 	Kernel Offset: disabled
> 	CPU features: 0x000,00000070,00801250,8200700b
> 	Memory Limit: none
> 	---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---
> 
> Fix this by using a generic probe function for both mv88q211x and
> mv88q222x devices that allocates the PHY private data structure, while
> only the mv88q222x probes for LED support.
> 
> Fixes: a3783dbf2574 ("net: phy: marvell-88q2xxx: Add support for PHY LEDs on 88q2xxx")
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/net/phy/marvell-88q2xxx.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index bad5e7b2357d..4fca9ffc90fe 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -825,13 +825,24 @@ static int mv88q2xxx_leds_probe(struct phy_device *phydev)
>  static int mv88q2xxx_probe(struct phy_device *phydev)
>  {
>  	struct mv88q2xxx_priv *priv;
> -	int ret;
>  
>  	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
>  	phydev->priv = priv;
> +
> +	return 0;
> +}
> +
> +static int mv88q222x_probe(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = mv88q2xxx_probe(phydev);
> +	if (ret)
> +		return ret;
> +
>  	ret = mv88q2xxx_leds_probe(phydev);
>  	if (ret)
>  		return ret;
> @@ -1101,6 +1112,7 @@ static struct phy_driver mv88q2xxx_driver[] = {
>  		.phy_id			= MARVELL_PHY_ID_88Q2110,
>  		.phy_id_mask		= MARVELL_PHY_ID_MASK,
>  		.name			= "mv88q2110",
> +		.probe			= mv88q2xxx_probe,
>  		.get_features		= mv88q2xxx_get_features,
>  		.config_aneg		= mv88q2xxx_config_aneg,
>  		.config_init		= mv88q2110_config_init,
> @@ -1115,7 +1127,7 @@ static struct phy_driver mv88q2xxx_driver[] = {
>  		.phy_id_mask		= MARVELL_PHY_ID_MASK,
>  		.name			= "mv88q2220",
>  		.flags			= PHY_POLL_CABLE_TEST,
> -		.probe			= mv88q2xxx_probe,
> +		.probe			= mv88q222x_probe,
>  		.get_features		= mv88q2xxx_get_features,
>  		.config_aneg		= mv88q2xxx_config_aneg,
>  		.aneg_done		= genphy_c45_aneg_done,
> -- 
> 2.48.1
> 
thanks for your patch. You could also just switch to mv88q2xxx_probe.
Then you would get HWMON and LED support while fixing this. 88Q2110
supports both.

Best regards,
Dimitri Fedrau