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  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]
Date:   Thu, 18 Jun 2020 11:01:43 +0100
From:   Russell King - ARM Linux admin <>
To:     Helmut Grohne <>
Cc:     Nicolas Ferre <>,
        "David S. Miller" <>,
        Jakub Kicinski <>,
        Palmer Dabbelt <>,
        Paul Walmsley <>,
        "" <>,
        Florian Fainelli <>
Subject: Re: [PATCH] net: macb: reject unsupported rgmii delays

On Thu, Jun 18, 2020 at 11:05:26AM +0200, Helmut Grohne wrote:
> On Thu, Jun 18, 2020 at 10:45:54AM +0200, Russell King - ARM Linux admin wrote:
> > Why do we need that complexity?  If we decide that we can allow
> > phy-mode = "rgmii" and introduce new properties to control the
> > delay, then we just need:
> > 
> >   rgmii-tx-delay-ps = <nnn>;
> >   rgmii-rx-delay-ps = <nnn>;
> > 
> > specified in the MAC node (to be applied only at the MAC end) or
> > specified in the PHY node (to be applied only at the PHY end.)
> > In the normal case, this would be the standard delay value, but
> > in exceptional cases where supported, the delays can be arbitary.
> > We know there are PHYs out there which allow other delays.
> > 
> > This means each end is responsible for parsing these properties in
> > its own node and applying them - or raising an error if they can't
> > be supported.
> Thank you. That makes a lot more sense while keeping the (imo) important
> properties of my proposal:
>  * It is backwards compatible. These properties override delays
>    specified inside phy-mode. Otherwise the vague phy-mode meaning is
>    retained.
>  * The ambiguity is resolved. It is always clear where delays should be
>    configure and the properties properly account for possible PCB
>    traces.
> It also resolves my original problem. If support for these properties is
> added to macb_main.c, it would simply check that both delays are 0 as
> internal delays are not supported by the hardware.  When I would have
> attempted to configure an rx delay, it would have nicely errored out.

I think we'd want a helper or two to do the parsing and return the
delays, something like:


/* @np here should be the MAC node */
int of_mac_get_delays(struct device_node *np,
		      phy_interface_mode interface,
		      u32 *tx_delay_ps, u32 *rx_delay_ps)
	*tx_delay_ps = PHY_RGMII_DELAY_PS_NONE;
	*rx_delay_ps = PHY_RGMII_DELAY_PS_NONE;

	if (!np)
		return 0;

	if (interface == PHY_INTERFACE_MODE_RGMII) {
		of_property_read_u32(np, "rgmii-tx-delay-ps", tx_delay_ps);
		of_property_read_u32(np, "rgmii-rx-delay-ps", rx_delay_ps);

	return 0;

/* @np here should be the PHY node */
int of_phy_get_delays(struct device_node *np,
		      phy_interface_mode interface,
		      u32 *tx_delay_ps, u32 *rx_delay_ps)
	switch (interface) {
		*tx_delay_ps = PHY_RGMII_DELAY_PS_STD;
		*rx_delay_ps = PHY_RGMII_DELAY_PS_STD;
		return 0;

		*tx_delay_ps = PHY_RGMII_DELAY_PS_NONE;
		*rx_delay_ps = PHY_RGMII_DELAY_PS_STD;
		return 0;

		*tx_delay_ps = PHY_RGMII_DELAY_PS_STD;
		*rx_delay_ps = PHY_RGMII_DELAY_PS_NONE;
		return 0;

		return of_mac_get_delays(np, interface,
					 tx_delay_ps, rx_delay_ps);

as a first cut - validation left up to the user of these.  At least
we then have an easy interface for PHY drivers to use, for example:

static int m88e1121_config_aneg_rgmii_delays(struct phy_device *phydev)
	u32 tx_delay_ps, rx_delay_ps;
	int err;

	err = of_phy_get_delays(phydev->,
				phydev->interface, &tx_delay_ps,
	if (err)
		return err;

	mscr = 0;
	if (tx_delay_ps == PHY_RGMII_DELAY_PS_STD)
		mscr |= MII_88E1121_PHY_MSCR_TX_DELAY;
	else if (tx_delay_ps != PHY_RGMII_DELAY_PS_NONE)
		/* ... log an error to kernel log */
		return -EINVAL;

	if (rx_delay_ps == PHY_RGMII_DELAY_PS_STD)
		mscr |= MII_88E1121_PHY_MSCR_RX_DELAY;
	else if (rx_delay_ps != PHY_RGMII_DELAY_PS_NONE)
		/* ... log an error to kernel log */
		return -EINVAL;

	return phy_modify_paged(phydev, MII_MARVELL_MSCR_PAGE,
				MII_88E1121_PHY_MSCR_DELAY_MASK, mscr);

> How can we achieve wider consensus on this and put it into the dt
> specification? Should there be drivers supporting these first?

Provide an illustration of the idea in code form for consideration? ;)

RMK's Patch system:
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists