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
