netdev - Re: [PATCH net-next 6/6] net: dsa: sja1105: parse {rx,tx}-internal-delay-ps properties for RGMII delays

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: <20211014120010.im3bvg6ga7i2423n@skbuf>
Date:   Thu, 14 Oct 2021 12:00:11 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "David S . Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Rob Herring <robh+dt@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Prasanna Vengateshan <prasanna.vengateshan@...rochip.com>,
        Ansuel Smith <ansuelsmth@...il.com>,
        Alvin Šipraga <alsi@...g-olufsen.dk>
Subject: Re: [PATCH net-next 6/6] net: dsa: sja1105: parse
 {rx,tx}-internal-delay-ps properties for RGMII delays

On Wed, Oct 13, 2021 at 05:24:48PM -0700, Jakub Kicinski wrote:
> Some take it or leave it comments, checkpatch pointed out some extra
> brackets so I had a look at the patch.
> 
> On Thu, 14 Oct 2021 01:23:13 +0300 Vladimir Oltean wrote:
> > +	int rx_delay = -1, tx_delay = -1;
> >  
> > +	if (!phy_interface_mode_is_rgmii(phy_mode))
> > +		return 0;
> >  
> > +	of_property_read_u32(port_dn, "rx-internal-delay-ps", &rx_delay);
> > +	of_property_read_u32(port_dn, "tx-internal-delay-ps", &tx_delay);
> 
> If I'm reading this right you're depending on delays being left as -1
> in case the property reads fail. Is this commonly accepted practice?

It works.

> Why not code it up as:
> 
> 	u32 rx_delay;
> 
> 	if (of_property_read_u32(...))
> 		rx_delay = 0;
> 	else if (rx_delay != clamp(rx_delay, ...MIN, ...MAX)
> 		goto err;
> 
> or some such?

"or some such" is not functionally equivalent.

This is what would be functionally equivalent, and following your
suggestion to check the return code of of_property_read_u32 instead of
assigning default values, and to use clamp() instead of open-coding the
bounds checks.

static int sja1105_parse_rgmii_delays(struct sja1105_private *priv, int port,
				      struct device_node *port_dn)
{
	phy_interface_t phy_mode = priv->phy_mode[port];
	struct device *dev = &priv->spidev->dev;
	int rx_delay, tx_delay;
	int err_rx, err_tx;

	if (!phy_interface_mode_is_rgmii(phy_mode))
		return 0;

	err_rx = of_property_read_u32(port_dn, "rx-internal-delay-ps", &rx_delay);
	if (err_rx)
		rx_delay = 0;

	err_tx = of_property_read_u32(port_dn, "tx-internal-delay-ps", &tx_delay);
	if (err_tx)
		tx_delay = 0;

	if (err_rx && err_tx) {
		if (priv->fixed_link[port]) {
			dev_warn(dev,
				 "Port %d interpreting RGMII delay settings based on \"phy-mode\" property, "
				 "please update device tree to specify \"rx-internal-delay-ps\" and "
				 "\"tx-internal-delay-ps\"",
				 port);

			if (phy_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
			    phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
				rx_delay = 2000;

			if (phy_mode == PHY_INTERFACE_MODE_RGMII_TXID ||
			    phy_mode == PHY_INTERFACE_MODE_RGMII_ID)
				tx_delay = 2000;
		}
	} else {
		if ((rx_delay && rx_delay != clamp(rx_delay, SJA1105_RGMII_DELAY_MIN_PS, SJA1105_RGMII_DELAY_MAX_PS)) ||
		    (tx_delay && tx_delay != clamp(tx_delay, SJA1105_RGMII_DELAY_MIN_PS, SJA1105_RGMII_DELAY_MAX_PS))) {
			dev_err(dev,
				"port %d RGMII delay values out of range, must be between %d and %d ps\n",
				port, SJA1105_RGMII_DELAY_MIN_PS, SJA1105_RGMII_DELAY_MAX_PS);
			return -ERANGE;
		}
	}

	priv->rgmii_rx_delay_ps[port] = rx_delay;
	priv->rgmii_tx_delay_ps[port] = tx_delay;

	return 0;
}

> 
> > +	if ((rx_delay && rx_delay < SJA1105_RGMII_DELAY_MIN_PS) ||
> > +	    (tx_delay && tx_delay < SJA1105_RGMII_DELAY_MIN_PS) ||
> > +	    (rx_delay > SJA1105_RGMII_DELAY_MAX_PS) ||
> > +	    (tx_delay > SJA1105_RGMII_DELAY_MAX_PS)) {
> 
> nit: checkpatch says the brackets around the latter two are unnecessary,
>      just in case it's not for symmetry / on purpose

It is on purpose.