netdev - Re: [net-next,PATCH 1/2] net: dsa: microchip: Add emulated MIIM access to switch LED config registers

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: <32b6985e-3897-4317-bdf7-d78ffe89c38d@redhat.com>
Date: Thu, 16 Jan 2025 10:32:58 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Marek Vasut <marex@...x.de>, netdev@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>, Andrew Lunn <andrew@...n.ch>,
 Eric Dumazet <edumazet@...gle.com>, Heiner Kallweit <hkallweit1@...il.com>,
 Jakub Kicinski <kuba@...nel.org>, Russell King <linux@...linux.org.uk>,
 Tristram Ha <tristram.ha@...rochip.com>, UNGLinuxDriver@...rochip.com,
 Vladimir Oltean <olteanv@...il.com>, Woojung Huh
 <woojung.huh@...rochip.com>, linux-kernel@...r.kernel.org
Subject: Re: [net-next,PATCH 1/2] net: dsa: microchip: Add emulated MIIM
 access to switch LED config registers

On 1/13/25 1:15 AM, Marek Vasut wrote:
> The KSZ87xx switch DSA driver binds a simplified KSZ8795 switch PHY driver to
> each port. The KSZ8795 switch PHY driver is part of drivers/net/phy/micrel.c
> and uses generic PHY register accessors to access MIIM registers 0x00..0x05,
> 0x1d and 0x1f . The MII access is implemented by the KSZ87xx switch DSA driver
> and internally done over whichever interface the KSZ87xx switch is connected
> to the SoC.
> 
> In order to configure LEDs from the KSZ8795 switch PHY driver, it is necessary
> to expose the LED control registers to the PHY driver, however, the LED control
> registers are not part of the MIIM block, they are in Switch Config Registers
> block.
> 
> This preparatory patch exposes the LED control bits in those Switch Config
> Registers by mapping them at high addresses in the MIIM space, so the PHY
> driver can access those registers and surely not collide with the existing
> MIIM block registers. The two registers which are exposed are the global
> Register 11 (0x0B): Global Control 9 as MIIM block register 0x0b00 and
> port specific Register 29/45/61 (0x1D/0x2D/0x3D): Port 1/2/3 Control 10
> as MIIM block register 0x0d00 . The access to those registers is further
> restricted only to the LED configuration bits to prevent the PHY driver
> or userspace tools like 'phytool' from tampering with any other switch
> configuration through this interface.
> 
> Signed-off-by: Marek Vasut <marex@...x.de>
> ---
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Andrew Lunn <andrew@...n.ch>
> Cc: Eric Dumazet <edumazet@...gle.com>
> Cc: Heiner Kallweit <hkallweit1@...il.com>
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: Paolo Abeni <pabeni@...hat.com>
> Cc: Russell King <linux@...linux.org.uk>
> Cc: Tristram Ha <tristram.ha@...rochip.com>
> Cc: UNGLinuxDriver@...rochip.com
> Cc: Vladimir Oltean <olteanv@...il.com>
> Cc: Woojung Huh <woojung.huh@...rochip.com>
> Cc: linux-kernel@...r.kernel.org
> Cc: netdev@...r.kernel.org
> ---
>  drivers/net/dsa/microchip/ksz8.c | 47 ++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> index da7110d675583..9698bf53378af 100644
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c
> @@ -1044,6 +1044,22 @@ int ksz8_r_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 *val)
>  			return ret;
>  
>  		break;
> +	/* Emulated access to Register 11 (0x0B): Global Control 9 */
> +	case (REG_SW_CTRL_9 << 8):

I think it's better to add a macro for the new registers

> +		ret = ksz_read8(dev, REG_SW_CTRL_9, &val1);
> +		if (ret)
> +			return ret;
> +
> +		data = val1 & 0x30;	/* LED Mode */

In a similar way, it's better to add meaningful macros for the relevant
register fields. You should consider using the *FIELD*() helpers, too.

/P