netdev - Re: [PATCH net-next v4 09/11] net: dsa: realtek: migrate 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: <20240125160511.pskpwroyrdmooxrg@skbuf>
Date: Thu, 25 Jan 2024 18:05:11 +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 09/11] net: dsa: realtek: migrate
 user_mii_bus setup to realtek-dsa

On Tue, Jan 23, 2024 at 06:56:01PM -0300, Luiz Angelo Daros de Luca wrote:
> In the user MDIO driver, despite numerous references to SMI, including
> its compatible string, there's nothing inherently specific about the SMI
> interface in the user MDIO bus. Consequently, the code has been migrated
> to the rtl83xx module. All references to SMI have been eliminated.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@...il.com>
> ---
> diff --git a/drivers/net/dsa/realtek/rtl83xx.c b/drivers/net/dsa/realtek/rtl83xx.c
> index 53bacbacc82e..525d8c014136 100644
> --- a/drivers/net/dsa/realtek/rtl83xx.c
> +++ b/drivers/net/dsa/realtek/rtl83xx.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  
>  #include <linux/module.h>
> +#include <linux/of_mdio.h>
>  
>  #include "realtek.h"
>  #include "rtl83xx.h"
> @@ -42,6 +43,72 @@ void rtl83xx_unlock(void *ctx)
>  }
>  EXPORT_SYMBOL_NS_GPL(rtl83xx_unlock, REALTEK_DSA);
>  
> +static int rtl83xx_user_mdio_read(struct mii_bus *bus, int addr, int regnum)
> +{
> +	struct realtek_priv *priv = bus->priv;
> +
> +	return priv->ops->phy_read(priv, addr, regnum);
> +}
> +
> +static int rtl83xx_user_mdio_write(struct mii_bus *bus, int addr, int regnum,
> +				   u16 val)
> +{
> +	struct realtek_priv *priv = bus->priv;
> +
> +	return priv->ops->phy_write(priv, addr, regnum, val);
> +}

Do we actually need to go through this extra indirection, or can the
priv->ops->phy_read/write() prototypes be made to take just struct
mii_bus * as their first argument?

> +
> +/**
> + * rtl83xx_setup_user_mdio() - register the user mii bus driver
> + * @ds: DSA switch associated with this user_mii_bus
> + *
> + * This function first gets and mdio node under the dev OF node, aborting
> + * if missing. That mdio node describing an mdio bus is used to register a
> + * new mdio bus.
> + *
> + * Context: Any context.
> + * Return: 0 on success, negative value for failure.
> + */
> +int rtl83xx_setup_user_mdio(struct dsa_switch *ds)
> +{
> +	struct realtek_priv *priv =  ds->priv;

Please remove the extra space here in " =  ds->priv".

> +	struct device_node *mdio_np;
> +	int ret = 0;
> +
> +	mdio_np = of_get_child_by_name(priv->dev->of_node, "mdio");
> +	if (!mdio_np) {
> +		dev_err(priv->dev, "no MDIO bus node\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->user_mii_bus = devm_mdiobus_alloc(priv->dev);
> +	if (!priv->user_mii_bus) {
> +		ret = -ENOMEM;
> +		goto err_put_node;
> +	}
> +
> +	priv->user_mii_bus->priv = priv;
> +	priv->user_mii_bus->name = "Realtek user MII";
> +	priv->user_mii_bus->read = rtl83xx_user_mdio_read;
> +	priv->user_mii_bus->write = rtl83xx_user_mdio_write;
> +	snprintf(priv->user_mii_bus->id, MII_BUS_ID_SIZE, "Realtek-%d",
> +		 ds->index);

There isn't much consistency here, but maybe something derived from
dev_name(dev) or %pOF would make it clearer that it describes a switch's
internal MDIO bus, rather than just any Realtek thing?

> +	priv->user_mii_bus->parent = priv->dev;
> +
> +	ret = devm_of_mdiobus_register(priv->dev, priv->user_mii_bus, mdio_np);
> +	if (ret) {
> +		dev_err(priv->dev, "unable to register MDIO bus %s\n",
> +			priv->user_mii_bus->id);
> +		goto err_put_node;
> +	}

Maybe this function would look a bit less cluttered with a temporary
struct mii_bus *bus variable, and priv->user_mii_bus gets assigned to
"bus" at the end (on success), and another struct device *dev = priv->dev.

> +
> +err_put_node:
> +	of_node_put(mdio_np);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(rtl83xx_setup_user_mdio, REALTEK_DSA);
> +
>  /**
>   * rtl83xx_probe() - probe a Realtek switch
>   * @dev: the device being probed
> diff --git a/drivers/net/dsa/realtek/rtl83xx.h b/drivers/net/dsa/realtek/rtl83xx.h
> index 9eb8197a58fa..b5d464bb850d 100644
> --- a/drivers/net/dsa/realtek/rtl83xx.h
> +++ b/drivers/net/dsa/realtek/rtl83xx.h
> @@ -12,6 +12,7 @@ struct realtek_interface_info {
>  
>  void rtl83xx_lock(void *ctx);
>  void rtl83xx_unlock(void *ctx);
> +int rtl83xx_setup_user_mdio(struct dsa_switch *ds);
>  struct realtek_priv *
>  rtl83xx_probe(struct device *dev,
>  	      const struct realtek_interface_info *interface_info);
> -- 
> 2.43.0
> 

Otherwise, this is in principle ok and what I wanted to see.