Re: [v1,3/3] net: dsa: microchip: add single-led-mode support for KSZ9477

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: dsa: microchip: add single-led-mode support for KSZ9477

This patch adds single LED mode support for KSZ9477 via a device tree
property. The mode is configured during MAC link-up for non-CPU ports
and includes a workaround for Errata Module 19.

> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index e5fa1f5fc09b..bef8ec51d948 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c

[ ... ]

> @@ -3909,6 +3920,24 @@ static void ksz_set_gbit(struct ksz_device *dev, int port, bool gbit)
>  	ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8);
>  }
>
> +static void ksz_enable_single_led_mode(struct ksz_device *dev, int port, struct phy_device *phydev)
> +{
> +	const u16 *regs = dev->info->regs;
> +
> +	ksz_pwrite16(dev, port, regs[P_PHY_MMD_SETUP], 0x0002); /* Set up register address for MMD */
> +	ksz_pwrite16(dev, port,  regs[P_PHY_MMD_DATA], 0x0000); /* Select Register 00h of MMD */
> +	ksz_pwrite16(dev, port, regs[P_PHY_MMD_SETUP], 0x4002); /* Select register data for MMD */
> +	ksz_pwrite16(dev, port,  regs[P_PHY_MMD_DATA], 0x0011); /* Write value 0011h to MMD */
> +
> +	/* According to KSZ9477S Errata DS80000754F (Module 19) 0xfa00 has to
> +	 * be written to the Debug Register 3 to enable Single-LED Mode.
> +	 */
> +	if (phydev->dev_flags & MICREL_KSZ9_LED_ERRATA)
              ^^^^^^

Can phydev be NULL here? The phylink framework passes phydev as NULL when
using fixed-link configurations (MLO_AN_FIXED mode). For example, if a
non-CPU port on KSZ9477 is configured with a fixed-link in device tree
instead of a real PHY, phylink_fwnode_phy_connect() returns early without
attaching a PHY, leaving pl->phydev as NULL.

When link comes up on such a port, phylink_link_up() calls the mac_link_up
callback with this NULL phydev, which then gets passed to this function.

> +		ksz_pwrite16(dev, port, regs[P_PHY_DIGITAL_DEBUG_3], 0xfa00);
> +
> +	dev_info(dev->dev, "port-%d: single-led mode enabled.\n", port);
> +}

[ ... ]

> @@ -3977,6 +4006,9 @@ static void ksz9477_phylink_mac_link_up(struct phylink_config *config,
>  	int port = dp->index;
>  	struct ksz_port *p;
>
> +	if (dev->single_led_mode && port != dev->cpu_port)
> +		ksz_enable_single_led_mode(dev, port, phydev);

The guard here checks single_led_mode and that port is not the CPU port,
but it does not verify that phydev is valid. On KSZ9477, ports 5 and 6
do not have internal PHYs (internal_phy[] is false), and if one of these
external ports is configured with fixed-link while cpu_port is the other
external port, this path will be taken with phydev=NULL.

[ ... ]
-- 
pw-bot: cr