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

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] [day] [month] [year] [list]

Message-ID: <parqpamou43mzu6hlcszju2tk5jjikea4fcagtpcluklmaqwrf@zmulhigs65w4>
Date: Tue, 3 Feb 2026 12:13:50 +0100
From: Heinrich Toews <ht@...-software.de>
To: Jakub Kicinski <kuba@...nel.org>
Cc: f.fainelli@...il.com, robh+dt@...nel.org, andrew@...n.ch, 
	olteanv@...il.com, netdev@...r.kernel.org, krzysztof.kozlowski+dt@...aro.org
Subject: Re: [v1,3/3] net: dsa: microchip: add single-led-mode support for
 KSZ9477

Hello Jakub,

On Wed, Jan 28, 2026 at 06:25:48PM -0800, Jakub Kicinski wrote:
> 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.

Thanks, I see the problem here. Indeed, in a fixed-link configuration, phydev
will be NULL here.

Since the LED mode configuration is specific to the PHY, I will add a 
NULL check to ensure we only proceed if a PHY is actually attached to 
the port.

> > +		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.

I re-checked the datasheet (DS00002392C) and indeed the switch has seven 
ports, of which only five have integrated PHYs. The two external MAC 
ports (5 and 6) can be connected to a host processor, another switch, 
or an external PHY.

As you pointed out, if these are used as fixed-links, phydev will be 
NULL. I will add a check to ensure we only proceed if a PHY is 
attached to the port.

I will fix this in v2.

Regards,
Heinrich