netdev - Re: [PATCH net-next v4 08/11] net: dsa: realtek: clean user_mii

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240125111718.armzsazgcjnicc2h@skbuf>
Date: Thu, 25 Jan 2024 13:17:18 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
Cc: netdev@...r.kernel.org, linus.walleij@...aro.org, alsi@...g-olufsen.dk,
	andrew@...n.ch, f.fainelli@...il.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	arinc.unal@...nc9.com, ansuelsmth@...il.com
Subject: Re: [PATCH net-next v4 08/11] net: dsa: realtek: clean user_mii_bus
 setup

On Tue, Jan 23, 2024 at 06:56:00PM -0300, Luiz Angelo Daros de Luca wrote:
> The line assigning dev.of_node in mdio_bus has been removed since the
> subsequent of_mdiobus_register will always overwrite it.

Please use present tense and imperative mood. "Remove the line assigning
dev.of_node, because ...".

> 
> ds->user_mii_bus is not assigned anymore[1].

"As discussed in [1], allow the DSA core to be simplified, by not
assigning ds->user_mii_bus when the MDIO bus is described in OF, as it
is unnecessary."

> It should work as before as long as the switch ports have a valid
> phy-handle property.
> 
> Since commit 3b73a7b8ec38 ("net: mdio_bus: add refcounting for fwnodes
> to mdiobus"), we can put the "mdio" node just after the MDIO bus
> registration.

> The switch unregistration was moved into realtek_common_remove() as
> both interfaces now use the same code path.

Hopefully you can sort this part out in a separate patch that's
unrelated to the user_mii_bus cleanup, ideally in "net: dsa: realtek:
common rtl83xx module".

> 
> [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> ---
>  drivers/net/dsa/realtek/realtek-mdio.c |  5 -----
>  drivers/net/dsa/realtek/realtek-smi.c  | 15 ++-------------
>  drivers/net/dsa/realtek/rtl83xx.c      |  2 ++
>  3 files changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index 0171185ec665..c75b4550802c 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -158,11 +158,6 @@ void realtek_mdio_remove(struct mdio_device *mdiodev)
>  {
>  	struct realtek_priv *priv = dev_get_drvdata(&mdiodev->dev);
>  
> -	if (!priv)
> -		return;
> -

The way I would structure these guards is I would keep them here, and
not in rtl83xx_remove() and rtl83xx_shutdown(). Then I would make sure
that rtl83xx_remove() is the exact opposite of just rtl83xx_probe(), and
rtl83xx_unregister_switch() is the exact opposite of just rtl83xx_register_switch().

And I would fix the error handling path of realtek_smi_probe() and
realtek_mdio_probe() to call rtl83xx_remove() when rtl83xx_register_switch()
fails.

> -	dsa_unregister_switch(priv->ds);
> -
>  	rtl83xx_remove(priv);
>  }
>  EXPORT_SYMBOL_NS_GPL(realtek_mdio_remove, REALTEK_DSA);
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index 0ccb2a6059a6..a89813e527d2 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -331,7 +331,7 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
>  {
>  	struct realtek_priv *priv =  ds->priv;
>  	struct device_node *mdio_np;
> -	int ret;
> +	int ret = 0;
>  
>  	mdio_np = of_get_child_by_name(priv->dev->of_node, "mdio");
>  	if (!mdio_np) {
> @@ -344,15 +344,14 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
>  		ret = -ENOMEM;
>  		goto err_put_node;
>  	}
> +
>  	priv->user_mii_bus->priv = priv;
>  	priv->user_mii_bus->name = "SMI user MII";
>  	priv->user_mii_bus->read = realtek_smi_mdio_read;
>  	priv->user_mii_bus->write = realtek_smi_mdio_write;
>  	snprintf(priv->user_mii_bus->id, MII_BUS_ID_SIZE, "SMI-%d",
>  		 ds->index);
> -	priv->user_mii_bus->dev.of_node = mdio_np;
>  	priv->user_mii_bus->parent = priv->dev;
> -	ds->user_mii_bus = priv->user_mii_bus;
>  
>  	ret = devm_of_mdiobus_register(priv->dev, priv->user_mii_bus, mdio_np);
>  	if (ret) {
> @@ -361,8 +360,6 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds)
>  		goto err_put_node;
>  	}
>  
> -	return 0;
> -
>  err_put_node:
>  	of_node_put(mdio_np);
>  
> @@ -434,14 +431,6 @@ void realtek_smi_remove(struct platform_device *pdev)
>  {
>  	struct realtek_priv *priv = platform_get_drvdata(pdev);
>  
> -	if (!priv)
> -		return;
> -
> -	dsa_unregister_switch(priv->ds);
> -
> -	if (priv->user_mii_bus)
> -		of_node_put(priv->user_mii_bus->dev.of_node);
> -
>  	rtl83xx_remove(priv);
>  }
>  EXPORT_SYMBOL_NS_GPL(realtek_smi_remove, REALTEK_DSA);
> diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
> index 3d07c5662fa4..53bacbacc82e 100644
> --- a/drivers/net/dsa/realtek/rtl83xx.c
> +++ b/drivers/net/dsa/realtek/rtl83xx.c
> @@ -190,6 +190,8 @@ void rtl83xx_remove(struct realtek_priv *priv)
>  	if (!priv)
>  		return;
>  
> +	dsa_unregister_switch(priv->ds);
> +
>  	/* leave the device reset asserted */
>  	if (priv->reset)
>  		gpiod_set_value(priv->reset, 1);
> -- 
> 2.43.0
>