netdev - Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock

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:   Mon, 16 May 2022 22:48:20 +0300
From:   Josua Mayer <josua@...id-run.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Michael Walle <michael@...le.cc>, alexandru.ardelean@...log.com,
        alvaro.karsz@...id-run.com, davem@...emloft.net,
        edumazet@...gle.com, krzysztof.kozlowski+dt@...aro.org,
        michael.hennerich@...log.com, netdev@...r.kernel.org,
        pabeni@...hat.com, robh+dt@...nel.org
Subject: Re: [PATCH v4 1/3] dt-bindings: net: adin: document phy clock

Am 16.05.22 um 20:43 schrieb Jakub Kicinski:
> On Sun, 15 May 2022 10:16:47 +0300 Josua Mayer wrote:
>> Am 13.05.22 um 01:44 schrieb Jakub Kicinski:
>>> On Thu, 12 May 2022 23:20:18 +0200 Michael Walle wrote:
>>>>> It's pure speculation on my side. I don't even know if PHYs use
>>>>> the recovered clock to clock its output towards the MAC or that's
>>>>> a different clock domain.
>>>>>
>>>>> My concern is that people will start to use DT to configure SyncE which
>>>>> is entirely a runtime-controllable thing, and doesn't belong.
>> Okay.
>> However phy drivers do not seem to implement runtime control of those
>> clock output pins currently, so they are configured once in DT.
> To me that means nobody needs the recovered clock.
Doesn't need it, or is overwhelmed by the idea of figuring out how to 
implement it properly.
>>>>> Hence
>>>>> my preference to hide the recovered vs free-running detail if we can
>>>>> pick one that makes most sense for now.
>> I am not a fan of hiding information. The clock configuration register
>> clearly supports this distinction.
> Unless you expose all registers as a direct API to the user you'll be
> "hiding information". I don't think we are exposing all possible
> registers for this PHY, the two bits in question are no different.
>
>> Is this a political stance to say users may not "accidentally" enable
>> SyncE by patching DT?
>> If so we should print a warning message when someone selects it?
> Why would we add a feature and then print a warning? We can always add
> the support later, once we have a use case for it.
I would not call it a feature.
We can e.g. not print a warning, and instead put in the DT binding a 
note that the recovered variants are for SyncE which Linux does not 
support.

As to why we would add the -recovered options,
for starters this allows curious developers to search for the term to 
get an idea which PHYs would technically support it.
That it would also allow tinkering with SyncE to me is a plus, but for 
you clearly a minus, and I can not make a strong case.

So I can imagine to change the bindings as follows:
1. remove the -recovered variants
2. add an explicit note in the commit message that the recovered clock 
is not implemented because we do not have infrastructure for SyncE
3. keep the -free-running suffix, we should imo only hide it on the day 
SyncE can be toggled by another means.

> I see. That makes sense, but then wouldn't it make more sense to pick
> the (simple) free-running one? As for SyncE you'd need the recovered
> clock.
>>> Sounds good.
>> Yep, it seems recovered clock is only for SyncE - and only if there is a
>> master clock on the network. Sadly however documentation is sparse and I
>> do not know if the adi phys would fall back to using their internal
>> clock, or just refuse to operate at all.