netdev - Re: [net-next] net: ethernet: rtsn: Add support for Renesas Ethernet-TSN

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]

Date: Tue, 16 Apr 2024 10:58:02 +0200
From: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
To: Andrew Lunn <andrew@...n.ch>
Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [net-next] net: ethernet: rtsn: Add support for Renesas
 Ethernet-TSN

Hi Andrew,

Thanks for your thorough review, much appreciated.

I agree with all suggestions except one and will fix those for v2.

On 2024-04-16 00:55:12 +0200, Andrew Lunn wrote:

<snip>

> > +static void rtsn_set_delay_mode(struct rtsn_private *priv)
> > +{
> > +	struct device_node *np = priv->ndev->dev.parent->of_node;
> > +	u32 delay;
> > +	u32 val;
> > +
> > +	val = 0;
> > +
> > +	/* Valid values are 0 and 1800, according to DT bindings */
> 
> The bindings should not matter. It is what the hardware supports. The
> bindings should match the hardware, since it is hard to modify the
> hardware to make it match the binding.

I agree the comment could be improved. It should likely point to the 
datasheet instead. See below for why.

> 
> > +	if (!of_property_read_u32(np, "rx-internal-delay-ps", &delay))
> > +		if (delay)
> > +			val |= GPOUT_RDM;
> > +
> > +	/* Valid values are 0 and 2000, according to DT bindings */
> > +	if (!of_property_read_u32(np, "tx-internal-delay-ps", &delay))
> > +		if (delay)
> > +			val |= GPOUT_TDM;
> > +
> > +	rtsn_write(priv, GPOUT, val);
> 
> So you seem to be using it as bool?

Yes.

> That is wrong. It is a number of pico seconds!

The issue is that the hardware only supports no delay or a fixed delay 
that can depend on electric properties of the board. The datasheets 
states that the typical Rx delay is 1800 ps while the typical Tx delay 
is 2000 ps. The hardware register implementation for this is a single 
bit for each delay, on or off.

To model this in the bindings after some discussions [1] the standard 
property was picked over a vendor specific bool variant of it. Here in 
the driver I tried to document that the binding will enforce the value 
to either be 0 or {1800,2000}, but that for the hardware it should be 
treated as a on/off switch.

<snip>

1. https://lore.kernel.org/linux-renesas-soc/ZVzbigCtv2q_2-Bx@oden.dyn.berto.se/

-- 
Kind Regards,
Niklas Söderlund