linux-kernel - Re: [PATCH] net: phy: mscc: Add auto-negotiation feature to VSC8514

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: <Z63OnZTBMeAbzxrB@shell.armlinux.org.uk>
Date: Thu, 13 Feb 2025 10:51:09 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Chintan Vankar <c-vankar@...com>
Cc: Rosen Penev <rosenp@...il.com>,
	Christophe JAILLET <christophe.jaillet@...adoo.fr>,
	Paolo Abeni <pabeni@...hat.com>, Jakub Kicinski <kuba@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	"David S. Miller" <davem@...emloft.net>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Andrew Lunn <andrew@...n.ch>, s-vadapalli@...com,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH] net: phy: mscc: Add auto-negotiation feature to VSC8514

On Thu, Feb 13, 2025 at 03:51:50PM +0530, Chintan Vankar wrote:
> Add function vsc85xx_config_inband_aneg() in mscc_main.c to enable
> auto-negotiation. The function enables auto-negotiation by configuring
> the MAC SerDes PCS Control register of VSC8514.
> 
> Invoke the vsc85xx_config_inband_aneg() function from the
> vsc8514_config_init() function present in mscc_main.c to start the
> auto-negotiation process. This is required to get Ethernet working with
> the Quad port Ethernet Add-On card connected to J7 common processor board.

A few points:

1. please read the netdev FAQ:
   https://www.kernel.org/doc/html/v5.6/networking/netdev-FAQ.html
   specifically the first question. Please also note the delay
   requirement for resending patches.

2. Is this a fix? Does something not work at the moment?

3. Will always forcing the inband signalling to be enabled result in
   another existing user that doesn't provide the inband signalling
   now failing? Do you know for sure that this won't disrupt any other
   users of this PHY?

4. Maybe consider using phylink in the ethernet driver and populating
   the .inband_caps() and .config_inband() methods which will allow
   the inband signalling properties to be negotiated between the MAC's
   PCS and the PHY.

Other points inline below:

> +static int vsc85xx_config_inband_aneg(struct phy_device *phydev, bool enabled)
> +{
> +	u16 reg_val = 0;
> +	int rc;
> +
> +	if (enabled)
> +		reg_val = MSCC_PHY_SERDES_ANEG;
> +
> +	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_3,
> +			      MSCC_PHY_SERDES_PCS_CTRL, MSCC_PHY_SERDES_ANEG,
> +			      reg_val);
> +
> +	return rc;

Why not simply:

	return phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_3,
				MSCC_PHY_SERDES_PCS_CTRL,
				MSCC_PHY_SERDES_ANEG,
				reg_val);

?

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