[PATCH net v4] net: dsa: mt7530: fix impossible MDIO address and issue warning
Vladimir Oltean
olteanv at gmail.com
Thu Jul 4 10:16:04 PDT 2024
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 at makrotopia.org>
> Reviewed-by: Andrew Lunn <andrew at lunn.ch>
> Tested-by: Arınç ÜNAL <arinc.unal at arinc9.com>
> Reviewed-by: Arınç ÜNAL <arinc.unal at 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().
More information about the linux-arm-kernel
mailing list