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: <67e53859.050a0220.1a855c.24af@mx.google.com>
Date: Thu, 27 Mar 2025 12:36:56 +0100
From: Christian Marangi <ansuelsmth@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
	Marek BehĂșn <kabel@...nel.org>,
	Eric Woudstra <ericwouds@...il.com>,
	Daniel Golle <daniel@...rotopia.org>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next RFC PATCH v3 3/4] net: phy: Add support for Aeonsemi
 AS21xxx PHYs

On Thu, Mar 27, 2025 at 12:30:42PM +0100, Christian Marangi wrote:
> On Thu, Mar 27, 2025 at 11:24:26AM +0000, Russell King (Oracle) wrote:
> > On Thu, Mar 27, 2025 at 12:35:03AM +0100, Christian Marangi wrote:
> > > +static int as21xxx_match_phy_device(struct phy_device *phydev,
> > > +				    const struct phy_driver *phydrv)
> > > +{
> > > +	struct as21xxx_priv *priv;
> > > +	u32 phy_id;
> > > +	int ret;
> > > +
> > > +	/* Skip PHY that are not AS21xxx or already have firmware loaded */
> > > +	if (phydev->c45_ids.device_ids[MDIO_MMD_PCS] != PHY_ID_AS21XXX)
> > > +		return phydev->phy_id == phydrv->phy_id;
> > 
> > Isn't phydev->phy_id zero here for a clause 45 PHY? If the firmware
> > has been loaded, I believ eyou said that PHY_ID_AS21XXX won't be
> > used, so the if() will be true, and because we've read clause 45
> > IDs, phydev->phy_id will be zero meaning this will never match. So
> > a PHY with firmware loaded won't ever match any of these drivers.
> > This is probably not what you want.
> 
> You are right. I will generalize the function to skip having to redo the
> logic. With FW loaded either c45 and c22 ID are filled in.
> 
> > 
> > I'd suggest converting the tail of phy_bus_match() so that you can
> > call that to do the standard matching using either C22 or C45 IDs
> > as appropriate without duplicating that code.
> > 
> > > +
> > > +	/* Read PHY ID to handle firmware just loaded */
> > > +	ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_PHYSID1);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	phy_id = ret << 16;
> > > +
> > > +	ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_PHYSID2);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	phy_id |= ret;
> > > +
> > > +	/* With PHY ID not the generic AS21xxx one assume
> > > +	 * the firmware just loaded
> > > +	 */
> > > +	if (phy_id != PHY_ID_AS21XXX)
> > > +		return phy_id == phydrv->phy_id;
> > > +
> > > +	/* Allocate temp priv and load the firmware */
> > > +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > > +	if (!priv)
> > > +		return -ENOMEM;
> > > +
> > > +	mutex_init(&priv->ipc_lock);
> > > +
> > > +	ret = aeon_firmware_load(phydev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = aeon_ipc_sync_parity(phydev, priv);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* Enable PTP clk if not already Enabled */
> > > +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PTP_CLK,
> > > +			       VEND1_PTP_CLK_EN);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = aeon_dpc_ra_enable(phydev, priv);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	mutex_destroy(&priv->ipc_lock);
> > > +	kfree(priv);
> > > +
> > > +	/* Return not maching anyway as PHY ID will change after
> > > +	 * firmware is loaded.
> > 
> > Also "This relies on the driver probe order."
> > 
> > > +	 */
> > > +	return 0;
> > > +}
> > > +
> > > +static struct phy_driver as21xxx_drivers[] = {
> > > +	{
> > > +		/* PHY expose in C45 as 0x7500 0x9410
> > > +		 * before firmware is loaded.
> > 
> > Also "This driver entry must be attempted first to load the firmware and
> > thus update the ID registers."
> > 
> > > +		 */
> > > +		PHY_ID_MATCH_EXACT(PHY_ID_AS21XXX),
> > > +		.name		= "Aeonsemi AS21xxx",
> > > +		.match_phy_device = as21xxx_match_phy_device,
> > > +	},
> > > +	{
> > > +		PHY_ID_MATCH_EXACT(PHY_ID_AS21011JB1),
> > > +		.name		= "Aeonsemi AS21011JB1",
> > > +		.probe		= as21xxx_probe,
> > > +		.match_phy_device = as21xxx_match_phy_device,
> > > +		.read_status	= as21xxx_read_status,
> > > +		.led_brightness_set = as21xxx_led_brightness_set,
> > > +		.led_hw_is_supported = as21xxx_led_hw_is_supported,
> > > +		.led_hw_control_set = as21xxx_led_hw_control_set,
> > > +		.led_hw_control_get = as21xxx_led_hw_control_get,
> > > +		.led_polarity_set = as21xxx_led_polarity_set,
> > 
> > If I'm reading these driver entries correctly, the only reason for
> > having separate entries is to be able to have a unique name printed
> > for each - the methods themselves are all identical.
> > 
> > My feeling is that is not a sufficient reason to duplicate the driver
> > entries, which adds bloat (not only in terms of static data, but also
> > the data structures necessary to support each entry in sysfs.) However,
> > lets see what Andrew says.
> >
> 
> If you remember that was one of my crazy project in trying to reduce the
> array but I remember it did end up bad or abbandoned with the problem of
> having to reinvent each PHY. Probably my changes caused too much patch
> delta.
> 
> The proposal was exactly to pack all the struct that have similar OPs
> with introducing an array of PHY ID for each driver.
>

Found the old series. [1]

[1] https://lore.kernel.org/all/20240218190034.15447-1-ansuelsmth@gmail.com/

-- 
	Ansuel

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ