Re: [PATCH net v4] net: dsa: mt7530: fix impossible MDIO address and issue warning

[Vladimir Oltean <olteanv@gmail.com>]

On Thu, Jul 04, 2024 at 04:08:22PM +0100, Daniel Golle wrote:
> The MDIO address of the MT7530 and MT7531 switch ICs can be configured
> using bootstrap pins. However, there are only 4 possible options for the
> switch itself: 7, 15, 23 and 31. As in MediaTek's SDK the address of the
> switch is wrongly stated in the device tree as 0 (while in reality it is
> 31), warn the user about such broken device tree and make a good guess
> what was actually intended.

Zero is the MDIO broadcast address. Doesn't the switch respond to it, or
what's exactly the problem?

> 
> This is imporant also to not break compatibility with older Device Trees

important

> as with commit 868ff5f4944a ("net: dsa: mt7530-mdio: read PHY address of
> switch from device tree") the address in device tree will be taken into
> account, while before it was hard-coded to 0x1f.
> 
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")

I fail to understand the logic behind blaming this commit. There was no
observable issue prior to 868ff5f4944a ("net: dsa: mt7530-mdio: read PHY
address of switch from device tree"), was there? That's what 'git bisect'
with a broken device tree would point towards?

> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Reviewed-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
> Changes since v3 [3]:
>  - simplify calculation of correct address
> 
> Changes since v2 [2]:
>  - use macros instead of magic numbers
>  - introduce helper functions
>  - register new device on MDIO bus instead of messing with the address
>    and schedule delayed_work to unregister the "wrong" device.
>    This is a slightly different approach than suggested by Russell, but
>    imho makes things much easier than keeping the "wrong" device and
>    having to deal with keeping the removal of both devices linked.
>  - improve comments
> 
> Changes since v1 [1]:
>  - use FW_WARN as suggested.
>  - fix build on net tree which doesn't have 'mdiodev' as member of the
>    priv struct. Imho including this patch as fix makes sense to warn
>    users about broken firmware, even if the change introducing the
>    actual breakage is only present in net-next for now.
> 
> [1]: https://patchwork.kernel.org/project/netdevbpf/patch/e615351aefba25e990215845e4812e6cb8153b28.1714433716.git.daniel@makrotopia.org/
> [2]: https://patchwork.kernel.org/project/netdevbpf/patch/11f5f127d0350e72569c36f9060b6e642dfaddbb.1714514208.git.daniel@makrotopia.org/
> [3]: https://patchwork.kernel.org/project/netdevbpf/patch/7e3fed489c0bbca84a386b1077c61589030ff4ab.1719963228.git.daniel@makrotopia.org/
> 
>  drivers/net/dsa/mt7530-mdio.c | 91 +++++++++++++++++++++++++++++++++++
>  1 file changed, 91 insertions(+)
> 
> diff --git a/drivers/net/dsa/mt7530-mdio.c b/drivers/net/dsa/mt7530-mdio.c
> index 51df42ccdbe6..2037ed944801 100644
> --- a/drivers/net/dsa/mt7530-mdio.c
> +++ b/drivers/net/dsa/mt7530-mdio.c
> @@ -11,6 +11,7 @@
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/workqueue.h>
>  #include <net/dsa.h>
>  
>  #include "mt7530.h"
> @@ -136,6 +137,92 @@ static const struct of_device_id mt7530_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, mt7530_of_match);
>  
> +static int
> +mt7530_correct_addr(int phy_addr)

The prototype fits onto a single line.

> +{
> +	/* The corrected address is calculated as stated below:
> +	 * 0~6, 31 -> 31
> +	 * 7~14    -> 7
> +	 * 15~22   -> 15
> +	 * 23~30   -> 23
> +	 */
> +return (phy_addr - MT7530_NUM_PORTS & ~MT7530_NUM_PORTS) + MT7530_NUM_PORTS & PHY_MAX_ADDR - 1;

In addition to being weirdly indented and having difficult to follow
logic.. Why not opt for the simple, self-documenting variant below?

	switch (phy_addr) {
	case 0 ... 6:
	case 31:
		return 31;
	case 7 ... 14:
		return 7;
	case 15 ... 22:
		return 15;
	case 23 ... 30:
		return 23;
	default:
		return -EINVAL ???
	}

> +}
> +
> +static bool
> +mt7530_is_invalid_addr(int phy_addr)
> +{
> +	/* Only MDIO bus addresses 7, 15, 23, and 31 are valid options,
> +	 * which all have the least significant three bits set. Check
> +	 * for this.
> +	 */
> +	return (phy_addr & MT7530_NUM_PORTS) != MT7530_NUM_PORTS;

Why not implement this in terms of phy_addr != mt7530_correct_addr(phy_addr)?

> +}
> +
> +struct remove_impossible_priv {
> +	struct delayed_work remove_impossible_work;
> +	struct mdio_device *mdiodev;
> +};
> +
> +static void
> +mt7530_remove_impossible(struct work_struct *work)

Fits onto a single line.

> +{
> +	struct remove_impossible_priv *priv = container_of(work, struct remove_impossible_priv,
> +							   remove_impossible_work.work);
> +	struct mdio_device *mdiodev = priv->mdiodev;
> +
> +	mdio_device_remove(mdiodev);
> +	mdio_device_free(mdiodev);
> +	kfree(priv);
> +}
> +
> +static int
> +mt7530_reregister(struct mdio_device *mdiodev)
> +{
> +	/* If the address in DT must be wrong, make a good guess about
> +	 * the most likely intention, issue a warning, register a new
> +	 * MDIO device at the correct address and schedule the removal
> +	 * of the device having an impossible address.
> +	 */
> +	struct fwnode_handle *fwnode = dev_fwnode(&mdiodev->dev);
> +	int corrected_addr = mt7530_correct_addr(mdiodev->addr);
> +	struct remove_impossible_priv *rem_priv;
> +	struct mdio_device *new_mdiodev;
> +	int ret;
> +
> +	rem_priv = kmalloc(sizeof(*rem_priv), GFP_KERNEL);
> +	if (!rem_priv)
> +		return -ENOMEM;
> +
> +	new_mdiodev = mdio_device_create(mdiodev->bus, corrected_addr);
> +	if (IS_ERR(new_mdiodev)) {
> +		ret = PTR_ERR(new_mdiodev);
> +		goto out_free_work;
> +	}
> +	device_set_node(&new_mdiodev->dev, fwnode);
> +
> +	ret = mdio_device_register(new_mdiodev);
> +	if (WARN_ON(ret))
> +		goto out_free_dev;
> +
> +	dev_warn(&mdiodev->dev, FW_WARN
> +		 "impossible switch MDIO address in device tree, assuming %d\n",
> +		 corrected_addr);
> +
> +	/* schedule impossible device for removal from mdio bus */
> +	rem_priv->mdiodev = mdiodev;
> +	INIT_DELAYED_WORK(&rem_priv->remove_impossible_work, mt7530_remove_impossible);
> +	schedule_delayed_work(&rem_priv->remove_impossible_work, 0);

What makes it so that mt7530_remove_impossible() is actually guaranteed
to run after the probing of the mdio_device @ the incorrect address
_finishes_? mdio_device_remove() will not work on a device which has
probing in progress, will it?

There's also the more straightforward option of fixing up priv->mdiodev->addr
in mt7530.c to be something like priv->switch_base, which is derived
from priv->mdiodev->addr, with a fallback to 0x1f if the latter is zero,
and a FW_WARN().