linux-kernel - Re: [PATCH RFC net-next 2/2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

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: <Z5ilrp1c6lbMGqbl@shell.armlinux.org.uk>
Date: Tue, 28 Jan 2025 09:38:54 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Tristram.Ha@...rochip.com
Cc: Woojung Huh <woojung.huh@...rochip.com>, Andrew Lunn <andrew@...n.ch>,
	Vladimir Oltean <olteanv@...il.com>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	UNGLinuxDriver@...rochip.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC net-next 2/2] net: dsa: microchip: Add SGMII port
 support to KSZ9477 switch

On Mon, Jan 27, 2025 at 07:32:26PM -0800, Tristram.Ha@...rochip.com wrote:
> @@ -161,6 +161,113 @@ static int ksz9477_wait_alu_sta_ready(struct ksz_device *dev)
>  					10, 1000);
>  }
>  
> +static void port_sgmii_s(struct ksz_device *dev, uint port, u16 devid, u16 reg,
> +			 u16 len)
> +{
> +	u32 data;
> +
> +	data = (devid & MII_MMD_CTRL_DEVAD_MASK) << 16;
> +	data |= reg;
> +	if (len > 1)
> +		data |= PORT_SGMII_AUTO_INCR;
> +	ksz_pwrite32(dev, port, REG_PORT_SGMII_ADDR__4, data);
> +}
> +
> +static void port_sgmii_r(struct ksz_device *dev, uint port, u16 devid, u16 reg,
> +			 u16 *buf, u16 len)
> +{
> +	u32 data;
> +
> +	mutex_lock(&dev->sgmii_mutex);

You won't need sgmii_mutex if all accesses go through the MDIO bus
accessors (please do so and get rid of this mutex.)

> +	port_sgmii_s(dev, port, devid, reg, len);
> +	while (len) {
> +		ksz_pread32(dev, port, REG_PORT_SGMII_DATA__4, &data);
> +		*buf++ = (u16)data;
> +		len--;
> +	}
> +	mutex_unlock(&dev->sgmii_mutex);
> +}
> +
> +static void port_sgmii_w(struct ksz_device *dev, uint port, u16 devid, u16 reg,
> +			 u16 *buf, u16 len)
> +{
> +	u32 data;
> +
> +	mutex_lock(&dev->sgmii_mutex);
> +	port_sgmii_s(dev, port, devid, reg, len);
> +	while (len) {
> +		data = *buf++;
> +		ksz_pwrite32(dev, port, REG_PORT_SGMII_DATA__4, data);
> +		len--;
> +	}
> +	mutex_unlock(&dev->sgmii_mutex);
> +}

I don't see any need for any of the above complexity for writing
multiple registers. All your calls to the above two functions only
pass a value of 1 for len.

> +
> +static void port_sgmii_reset(struct ksz_device *dev, uint p)
> +{
> +	u16 ctrl = BMCR_RESET;
> +
> +	port_sgmii_w(dev, p, MDIO_MMD_VEND2, MII_BMCR, &ctrl, 1);
> +}

doesn't xpcs_create() result in xpcs->need_reset being set true, and
thus when the XPCS is used, cause xpcs_pre_config() to do a soft reset
of the XPCS? More importantly, the XPCS driver waits for the reset
to complete...

> @@ -978,6 +1085,13 @@ void ksz9477_get_caps(struct ksz_device *dev, int port,
>  
>  	if (dev->info->gbit_capable[port])
>  		config->mac_capabilities |= MAC_1000FD;
> +
> +	if (ksz_is_sgmii_port(dev, port)) {
> +		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_SGMII,
> +			  config->supported_interfaces);
> +	}

	f (ksz_is_sgmii_port(dev, port))
		phy_interface_or(config->supported_interfaces,
				 config->supported_interfaces,
				 xpcs_to_phylink_pcs(port->xpcs)->supported_interfaces);

If xpcs_to_phylink_pcs(port->xpcs)->supported_interfaces does not
accurately indicate the interfaces that are supported on your XPCS
model, then you need further xpcs driver changes.

> +static inline int ksz_get_sgmii_port(struct ksz_device *dev)
> +{
> +	return (dev->info->sgmii_port - 1);
> +}
> +
> +static inline bool ksz_has_sgmii_port(struct ksz_device *dev)
> +{
> +	return (dev->info->sgmii_port > 0);
> +}
> +
> +static inline bool ksz_is_sgmii_port(struct ksz_device *dev, int port)
> +{
> +	return (dev->info->sgmii_port == port + 1);
> +}

No need for the parens in any of the above three.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!