linux-kernel - 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 PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 26 May 2020 12:48:11 -0500
From:   Dan Murphy <dmurphy@...com>
To:     Andrew Lunn <andrew@...n.ch>
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

Andrew

On 5/23/20 5:07 PM, Andrew Lunn wrote:
>>> 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.


I re-worked v3 to be a bit more of a helper and incorporated Florian's 
and you comments

Dan


>       Andrew