netdev - Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

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: <20200618140623.GC1551@shell.armlinux.org.uk>
Date:   Thu, 18 Jun 2020 15:06:23 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Ioana Ciornei <ioana.ciornei@....com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        vladimir.oltean@....com, claudiu.manoil@....com,
        alexandru.marginean@....com, michael@...le.cc, andrew@...n.ch,
        f.fainelli@...il.com
Subject: Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

On Thu, Jun 18, 2020 at 03:08:36PM +0300, Ioana Ciornei wrote:
> Add a Lynx PCS MDIO module which exposes the necessary operations to
> drive the PCS using PHYLINK.
> 
> The majority of the code is extracted from the Felix DSA driver, which
> will be also modified in a later patch, and exposed as a separate module
> for code reusability purposes.
> 
> At the moment, USXGMII (only with in-band AN and speeds up to 2500),
> SGMII, QSGMII and 2500Base-X (only w/o in-band AN) are supported by the
> Lynx PCS MDIO module since these were also supported by Felix.
> 
> The module can only be enabled by the drivers in need and not user
> selectable.

Is this the same PCS found in the LX2160A?  It looks very similar.

> +/* 2500Base-X is SerDes protocol 7 on Felix and 6 on ENETC. It is a SerDes lane
> + * clocked at 3.125 GHz which encodes symbols with 8b/10b and does not have
> + * auto-negotiation of any link parameters. Electrically it is compatible with
> + * a single lane of XAUI.
> + * The hardware reference manual wants to call this mode SGMII, but it isn't
> + * really, since the fundamental features of SGMII:
> + * - Downgrading the link speed by duplicating symbols
> + * - Auto-negotiation
> + * are not there.

I welcome that others are waking up to the industry wide obfuscation of
terminology surrounding "SGMII" and "1000base-X", and calling it out
where it is blatently incorrectly described in documentation.

> + * The speed is configured at 1000 in the IF_MODE because the clock frequency
> + * is actually given by a PLL configured in the Reset Configuration Word (RCW).
> + * Since there is no difference between fixed speed SGMII w/o AN and 802.3z w/o
> + * AN, we call this PHY interface type 2500Base-X. In case a PHY negotiates a
> + * lower link speed on line side, the system-side interface remains fixed at
> + * 2500 Mbps and we do rate adaptation through pause frames.

We have systems that do have AN with 2500base-X however - which is what
you want when you couple two potentially remote systems over a fibre
cable.  The AN in 802.3z (1000base-X) is used to negotiate:

- duplex
- pause mode

although in practice, half-duplex is not supported by lots of hardware,
which leaves just pause mode.  It is useful to have pause mode
negotiation remain present, whether it's 1000base-X or 2500base-X, but
obviously within the hardware boundaries.

I suspect the hardware is capable of supporting 802.3z AN when operating
at 2500base-X, but not the SGMII symbol duplication for slower speeds.

> +struct mdio_lynx_pcs *mdio_lynx_pcs_create(struct mdio_device *mdio_dev)
> +{
> +	struct mdio_lynx_pcs *pcs;
> +
> +	if (WARN_ON(!mdio_dev))
> +		return NULL;
> +
> +	pcs = kzalloc(sizeof(*pcs), GFP_KERNEL);
> +	if (!pcs)
> +		return NULL;
> +
> +	pcs->dev = mdio_dev;
> +	pcs->an_restart = lynx_pcs_an_restart;
> +	pcs->get_state = lynx_pcs_get_state;
> +	pcs->link_up = lynx_pcs_link_up;
> +	pcs->config = lynx_pcs_config;

We really should not have these private structure interfaces.  Private
structure-based driver specific interfaces really don't constitute a
sane approach to interface design.

Would it work if there was a "struct mdio_device" add to the
phylink_config structure, and then you could have the phylink_pcs_ops
embedded into this driver?

If not, then we need some kind of mdio_pcs_device that offers this
kind of functionality.

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