Re: [PATCH 5.10 073/116] net: dsa: lantiq_gswip: dont use devres for mdiobus

[Alexey Khoroshilov]

On 14.02.2022 12:26, Greg Kroah-Hartman wrote:
> From: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> 
> [ Upstream commit 0d120dfb5d67edc5bcd1804e167dba2b30809afd ]
> 
> As explained in commits:
> 74b6d7d13307 ("net: dsa: realtek: register the MDIO bus under devres")
> 5135e96a3dd2 ("net: dsa: don't allocate the slave_mii_bus using devres")
> 
> mdiobus_free() will panic when called from devm_mdiobus_free() <-
> devres_release_all() <- __device_release_driver(), and that mdiobus was
> not previously unregistered.
> 
> The GSWIP switch is a platform device, so the initial set of constraints
> that I thought would cause this (I2C or SPI buses which call ->remove on
> ->shutdown) do not apply. But there is one more which applies here.
> 
> If the DSA master itself is on a bus that calls ->remove from ->shutdown
> (like dpaa2-eth, which is on the fsl-mc bus), there is a device link
> between the switch and the DSA master, and device_links_unbind_consumers()
> will unbind the GSWIP switch driver on shutdown.
> 
> So the same treatment must be applied to all DSA switch drivers, which
> is: either use devres for both the mdiobus allocation and registration,
> or don't use devres at all.
> 
> The gswip driver has the code structure in place for orderly mdiobus
> removal, so just replace devm_mdiobus_alloc() with the non-devres
> variant, and add manual free where necessary, to ensure that we don't
> let devres free a still-registered bus.
> 
> Fixes: ac3a68d56651 ("net: phy: don't abuse devres in devm_mdiobus_register()")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> Reviewed-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
> Signed-off-by: Jakub Kicinski <kuba@xxxxxxxxxx>
> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> ---
>  drivers/net/dsa/lantiq_gswip.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
> index 4d23a7aba7961..ed517985ca88e 100644
> --- a/drivers/net/dsa/lantiq_gswip.c
> +++ b/drivers/net/dsa/lantiq_gswip.c
> @@ -495,8 +495,9 @@ static int gswip_mdio_rd(struct mii_bus *bus, int addr, int reg)
>  static int gswip_mdio(struct gswip_priv *priv, struct device_node *mdio_np)
>  {
>  	struct dsa_switch *ds = priv->ds;
> +	int err;
>  
> -	ds->slave_mii_bus = devm_mdiobus_alloc(priv->dev);
> +	ds->slave_mii_bus = mdiobus_alloc();
>  	if (!ds->slave_mii_bus)
>  		return -ENOMEM;
>  
> @@ -509,7 +510,11 @@ static int gswip_mdio(struct gswip_priv *priv, struct device_node *mdio_np)
>  	ds->slave_mii_bus->parent = priv->dev;
>  	ds->slave_mii_bus->phy_mask = ~ds->phys_mii_mask;
>  
> -	return of_mdiobus_register(ds->slave_mii_bus, mdio_np);
> +	err = of_mdiobus_register(ds->slave_mii_bus, mdio_np);
> +	if (err)
> +		mdiobus_free(ds->slave_mii_bus);
> +
> +	return err;
>  }
>  
>  static int gswip_pce_table_entry_read(struct gswip_priv *priv,
> @@ -2086,8 +2091,10 @@ static int gswip_probe(struct platform_device *pdev)
>  	gswip_mdio_mask(priv, GSWIP_MDIO_GLOB_ENABLE, 0, GSWIP_MDIO_GLOB);
>  	dsa_unregister_switch(priv->ds);
>  mdio_bus:
> -	if (mdio_np)
> +	if (mdio_np) {
>  		mdiobus_unregister(priv->ds->slave_mii_bus);
> +		mdiobus_free(priv->ds->slave_mii_bus);
> +	}
>  put_mdio_node:
>  	of_node_put(mdio_np);
>  	for (i = 0; i < priv->num_gphy_fw; i++)
> @@ -2107,6 +2114,7 @@ static int gswip_remove(struct platform_device *pdev)
>  
>  	if (priv->ds->slave_mii_bus) {
>  		mdiobus_unregister(priv->ds->slave_mii_bus);
> +		mdiobus_free(priv->ds->slave_mii_bus);
>  		of_node_put(priv->ds->slave_mii_bus->dev.of_node);
>  	}


Should
  of_node_put(priv->ds->slave_mii_bus->dev.of_node);
be here before
  mdiobus_free(priv->ds->slave_mii_bus);
?

--
Best regards,
Alexey Khoroshilov
Linux Verification Center, ISPRAS