netdev - Re: [PATCH 07/12] dt-bindings: net: Add StarFive JH7100 SoC

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] [day] [month] [year] [list]

Message-ID: <d8ebed94-fd1b-4ee5-ae04-f8b6f66cd5a6@collabora.com>
Date: Fri, 27 Oct 2023 17:55:51 +0300
From: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Emil Renner Berthing <kernel@...il.dk>, Conor Dooley <conor@...nel.org>,
 Palmer Dabbelt <palmer@...belt.com>, Paul Walmsley
 <paul.walmsley@...ive.com>, Albert Ou <aou@...s.berkeley.edu>,
 Giuseppe Cavallaro <peppe.cavallaro@...com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 Jose Abreu <joabreu@...opsys.com>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Richard Cochran <richardcochran@...il.com>,
 Sagar Kadam <sagar.kadam@...ive.com>,
 Yanhong Wang <yanhong.wang@...rfivetech.com>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
 linux-riscv@...ts.infradead.org, linux-stm32@...md-mailman.stormreply.com,
 linux-arm-kernel@...ts.infradead.org, kernel@...labora.com
Subject: Re: [PATCH 07/12] dt-bindings: net: Add StarFive JH7100 SoC

On 2/17/23 17:25, Cristian Ciocaltea wrote:
> On 2/17/23 15:30, Andrew Lunn wrote:
>>>> I would actually say it shows we don't understand what is going on
>>>> with delays. "rgmii" is not every often the correct value. The fact it
>>>> works suggests the MAC is adding delays.
>>>>
>>>> What value are you using for starfive,gtxclk-dlychain ?
>>>
>>> This is set to '4' in patch 12/12.
>>>
>>>> Try 0 and then "rgmii-id"
>>>
>>> I made some more tests and it seems the only stable configuration is
>>> "rgmii"
>>> with "starfive,gtxclk-dlychain" set to 4:
>>>
>>> phy-mode | dlychain | status
>>> ---------+----------+--------------------------------------------
>>> rgmii    |        4 | OK (no issues observed)
>>> rgmii-id |        4 | BROKEN (errors reported [1])
>>> rgmii    |        0 | UNRELIABLE (no errors, but frequent stalls)
>>> rgmii-id |        0 | BROKEN (errors reported)
>>>
>>> [1] Reported errors in case of BROKEN status:
>>> $ grep '' /sys/class/net/eth0/statistics/* | grep -v ':0$'
>>
>> Thanks for the testing.
>>
>> So it seems like something is adding delays when it probably should
>> not. Ideally we want to know what.
>>
>> There is a danger here, something which has happened in the past. A
>> PHY which ignored "rgmii" and actually did power on defaults which was
>> "rgmii-id". As a result, lots of boards put "rmgii" in there DT blob,
>> which 'worked'. Until a board came along which really did need
>> "rgmii". The developer bringing that board up debugged the PHY, found
>> the problem and made it respect "rgmii" so their board worked. And the
>> fix broke a number of 'working' boards which had the wrong "rgmii"
>> instead of "rgmii-id".
> 
> Thanks for the heads-up.
> 
>> So you have a choice. Go with 4 and "rgmii", but put in a big fat
>> warning, "Works somehow but is technically wrong and will probably
>> break sometime in the future". Or try to understand what is really
>> going on here, were are the delays coming from, and fix the issue.
>>
>>        Andrew
> 
> I will try to analyze this further.

As the non-coherent DMA work this series depended on has been completed,
I started to investigate further the "rgmii-id" issue.
I couldn't spot anything wrong in the Motorcomm PHY driver, but
eventually got this working by adjusting rx-internal-delay-ps.

Will do some more testing before submitting v2.

Thanks,
Cristian