netdev - Re: [net-next PATCH v7 5/6] net: phy: Add support for Aeonsemi AS21xxx PHYs

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: <67fe5956.050a0220.347569.4476@mx.google.com>
Date: Tue, 15 Apr 2025 15:04:17 +0200
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>,
	Andrei Botila <andrei.botila@....nxp.com>,
	Sabrina Dubroca <sd@...asysnail.net>,
	Eric Woudstra <ericwouds@...il.com>,
	Daniel Golle <daniel@...rotopia.org>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.or
Subject: Re: [net-next PATCH v7 5/6] net: phy: Add support for Aeonsemi
 AS21xxx PHYs

On Tue, Apr 15, 2025 at 01:55:29PM +0100, Russell King (Oracle) wrote:
> On Tue, Apr 15, 2025 at 01:24:16PM +0200, Christian Marangi wrote:
> > On Tue, Apr 15, 2025 at 10:37:49AM +0100, Russell King (Oracle) wrote:
> > > > +static int aeon_ipcs_wait_cmd(struct phy_device *phydev, bool parity_status)
> > > > +{
> > > > +	u16 val;
> > > > +
> > > > +	/* Exit condition logic:
> > > > +	 * - Wait for parity bit equal
> > > > +	 * - Wait for status success, error OR ready
> > > > +	 */
> > > > +	return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1, VEND1_IPC_STS, val,
> > > > +					 FIELD_GET(AEON_IPC_STS_PARITY, val) == parity_status &&
> > > > +					 (val & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_RCVD &&
> > > > +					 (val & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_PROCESS &&
> > > > +					 (val & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_BUSY,
> > > > +					 AEON_IPC_DELAY, AEON_IPC_TIMEOUT, false);
> > > 
> > > Hmm. I'm wondering whether:
> > > 
> > > static bool aeon_ipc_ready(u16 val, bool parity_status)
> > > {
> > > 	u16 status;
> > > 
> > > 	if (FIELD_GET(AEON_IPC_STS_PARITY, val) != parity_status)
> > > 		return false;
> > > 
> > > 	status = val & AEON_IPC_STS_STATUS;
> > > 
> > > 	return status != AEON_IPC_STS_STATUS_RCVD &&
> > > 	       status != AEON_IPC_STS_STATUS_PROCESS &&
> > > 	       status != AEON_IPC_STS_STATUS_BUSY;
> > > }
> > > 
> > > would be better, and then maybe you can fit the code into less than 80
> > > columns. I'm not a fan of FIELD_PREP_CONST() when it causes differing
> > > usage patterns like the above (FIELD_GET(AEON_IPC_STS_STATUS, val)
> > > would match the coding style, and probably makes no difference to the
> > > code emitted.)
> > > 
> > 
> > You are suggesting to use a generic readx function or use a while +
> > sleep to use the suggested _ready function?
> 
> To write the other part of it (I thought this would be obvious!):
> 
> +	return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1, VEND1_IPC_STS, val,
> +					 aeon_ipc_ready(val, parity_status),
> +					 AEON_IPC_DELAY, AEON_IPC_TIMEOUT, false);
>

Eh I never considered that I could totally pass entire function in the
condition part. 

> > Mhhh I think I will have to create __ function for locked and non-locked
> > variant. I think I woulkd just handle the lock in the function using
> > send and rcv and maybe add some smatch tag to make sure the lock is
> > taken when entering those functions.
> 
> If you don't need the receive part, then pass NULL in for the receive
> data pointer, and use that to conditionalise that part?
> 

Ok yes that can also work.

-- 
	Ansuel