netdev - Re: [PATCH net-next v2 4/4] net: dp83869: Add RGMII internal delay configuration

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: <20200523220741.GO610998@lunn.ch>
Date:   Sun, 24 May 2020 00:07:41 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Dan Murphy <dmurphy@...com>
Cc:     Florian Fainelli <f.fainelli@...il.com>, hkallweit1@...il.com,
        davem@...emloft.net, robh@...nel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH net-next v2 4/4] net: dp83869: Add RGMII internal delay
 configuration

> > Any why is your PHY special, in that is does care about out of range
> > delays, when others using new the new core helper don't?
> 
> We are not rounding to nearest here.  Basically the helper works to find the
> best match
> 
> If the delay passed in is less than or equal to the smallest delay then
> return the smallest delay index
> 
> If the delay passed in is greater then the largest delay then return the max
> delay index

+               /* Find an approximate index by looking up the table */
+               if (delay > delay_values[i - 1] &&
+                   delay < delay_values[i]) {
+                       if (delay - delay_values[i - 1] < delay_values[i] - delay)
+                               return i - 1;
+                       else
+                               return i;

This appears to round to the nearest value when it is not an exact
match.

The documentation is a hint to the DT developer what value to put in
DT. By saying it rounders, the developer does not need to go digging
through the source code to find an exact value, otherwise -EINVAL will
be returned. They can just use the value the HW engineer suggested,
and the PHY will pick whatever is nearest.

> Not sure what you mean about this PHY being special.  This helper is
> not PHY specific.

As you said, if out of range, the helper returns the top/bottom
value. Your PHY is special, the top/bottom value is not good enough,
you throw an error.

The point of helpers is to give uniform behaviour. We have one line
helpers, simply because they give uniform behaviour, rather than have
each driver do it subtlety different. But it also means drivers should
try to not add additional constraints over what the helper already
has, unless it is actually required by the hardware.

> After I think about this more I am thinking a helper may be over kill here
> and the delay to setting should be done within the PHY driver itself

The helper is useful, it will result in uniform handling of rounding
between DT values and what the PHY can actually do. But please also
move your range check and error message inside the helper.

     Andrew