netdev - Re: [RFC] option to use proper skew timings for Micrel KSZ9021

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: <Y26MGEqo+kdSiQmM@lunn.ch>
Date:   Fri, 11 Nov 2022 18:53:28 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Ian Abbott <abbotti@....co.uk>
Cc:     netdev@...r.kernel.org
Subject: Re: [RFC] option to use proper skew timings for Micrel KSZ9021

> Yes, but you've convinced me that it is slightly more involved than I
> initially thought!
> 
> > If you are going to do this, i think you really should fix all the
> > bugs, not just the step. KSZ9021 has an offset of -840ps. KSZ9031 has
> > an offset of -900ps. So both are broke, in that the skew is expected
> > to be a signed value, 0 meaning 0.
> > 
> > I would suggest a bool property something like:
> > 
> > micrel,skew-equals-real-picoseconds
> > 
> > and you need to update the documentation in a way it is really clear
> > what is going on.
> 
> Perhaps it could allow the renamed properties for the KSZ9131 to be used.
> (They have similar names to the KSZ9021/KSZ9031 properties, but change the
> `-ps` suffix to `-psec`.)

That is also broken. Go check, everything else in DT uses -ps.

> > I would also consider adding a phydev_dbg() which prints the actual ps
> > skew being used, with/without the bug.
> > 
> > And since you are adding more foot guns, please validate the values in
> > DT as strictly as possible, without breaking the existing binding.
> 
> Yes, some min/max clamping of skew values would be good.  The code for
> KSZ9131 does that already.

I would want much more strict checking than that. The old and the new
values probably don't intersect. So if you see an old value while
micrel,skew-equals-real-picoseconds is in force, fail the probe with
-EINVAL. It looks like the old binding silently preforms rounding to
the nearest delay. So you probably should not do the opposite, error
out for a new value when micrel,skew-equals-real-picoseconds is not in
force. But you can add range checks. A negative value is clearly wrong
for the old values and should be -EINVAL. You just need to watch out
for that the current code reads the values as u32, not s32, so you
won't actually see a negative value.

Sometimes it is better to leave broke but working stuff broken and
just live with it.

      Andrew