netdev - Re: [PATCH net-next 5/7] net: phy: introduce ethtool_phy

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: <20240912101724.6233b1e4@fedora.home>
Date: Thu, 12 Sep 2024 10:17:24 +0200
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: davem@...emloft.net, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com, Andrew Lunn
 <andrew@...n.ch>, Jakub Kicinski <kuba@...nel.org>, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Russell King
 <linux@...linux.org.uk>, linux-arm-kernel@...ts.infradead.org, Christophe
 Leroy <christophe.leroy@...roup.eu>, Herve Codina
 <herve.codina@...tlin.com>, Florian Fainelli <f.fainelli@...il.com>, Heiner
 Kallweit <hkallweit1@...il.com>, Vladimir Oltean <vladimir.oltean@....com>,
 Marek Behún <kabel@...nel.org>, Köry
 Maincent <kory.maincent@...tlin.com>
Subject: Re: [PATCH net-next 5/7] net: phy: introduce ethtool_phy_ops to get
 and set phy configuration

Hello Oleksij,

On Thu, 12 Sep 2024 06:46:29 +0200
Oleksij Rempel <o.rempel@...gutronix.de> wrote:

> Hi Maxime,
> 
> On Wed, Sep 11, 2024 at 11:27:09PM +0200, Maxime Chevallier wrote:
> ...
> >  
> > +/**
> > + * struct phy_device_config - General PHY device configuration parameters for
> > + * status reporting and bulk configuration
> > + *
> > + * A structure containing generic PHY device information, allowing to expose
> > + * internal status to userspace, and perform PHY configuration in a controlled
> > + * manner.
> > + *
> > + * @isolate: The MII-side isolation status of the PHY
> > + * @loopback: The loopback state of the PHY
> > + */
> > +struct phy_device_config {
> > +	bool isolate;
> > +	bool loopback;
> > +};  
>  
> I would recommend to have loopback enum. There are different levels of
> loopback:
> https://www.ti.com/document-viewer/DP83TD510E/datasheet#GUID-50834313-DEF1-42FB-BA00-9B0902B2D7E4/TITLE-SNLS656SNLS5055224
> 
> I imagine something like this:
> 
> /*
>  * enum phy_loopback_mode - PHY loopback modes
>  * These modes represent different loopback configurations to
>  * facilitate in-circuit testing of the PHY's digital and analog paths.
>  */
> enum phy_loopback_mode {
> 	PHY_LOOPBACK_NONE = 0,		/* No loopback mode enabled */
> 	PHY_LOOPBACK_MII,		/* MII Loopback: MAC to PHY internal loopback */
> 	PHY_LOOPBACK_PCS,		/* PCS Loopback: PCS layer loopback, no signal processing */
> 	PHY_LOOPBACK_DIGITAL,		/* Digital Loopback: Loops back entire digital TX/RX path */
> 	PHY_LOOPBACK_ANALOG,		/* Analog Loopback: Loops back after analog front-end */
> 	PHY_LOOPBACK_FAR_END		/* Far-End (Reverse) Loopback: Receiver to MAC interface loopback */
> };

I agree with you on that, having the ability to fine-tune where the
loopback happens is really useful for debug. The main problem I would
see is to come-up with a set of modes that are somewhat generic, as
vendors implement a wide variety of loopback modes.

For example, on BaseT4 PHYs the Analog loopback doesn't exist and is
more akin to using a loopback stub, whereas the Digital loopback would
be a loopback at the PMD level (I don't know if that even exists).

That being said, the list of possible places to setup the loopback
within a PHY is finite and it's conceivable to come-up with a set of
loopback modes.

Thanks a lot for the feedback,

Maxime