netdev - Re: [PATCH] net: stmmac: dwmac-tegra: Fix link bring-up sequence

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: <qcdec6h776mb5vms54wksqmkoterxj4vt7tndtfppck2ao733t@nlhyy7yhwfgf>
Date: Wed, 25 Sep 2024 15:40:20 +0200
From: Thierry Reding <thierry.reding@...il.com>
To: Paritosh Dixit <paritoshd@...dia.com>
Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>, 
	Jose Abreu <joabreu@...opsys.com>, "David S . Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Maxime Coquelin <mcoquelin.stm32@...il.com>, 
	Jonathan Hunter <jonathanh@...dia.com>, Bhadram Varka <vbhadram@...dia.com>, 
	Revanth Kumar Uppala <ruppala@...dia.com>, netdev@...r.kernel.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH] net: stmmac: dwmac-tegra: Fix link bring-up sequence

On Mon, Sep 23, 2024 at 09:44:10AM GMT, Paritosh Dixit wrote:
> The Tegra MGBE driver sometimes fails to initialize, reporting the
> following error, and as a result, it is unable to acquire an IP
> address with DHCP:
> 
>  tegra-mgbe 6800000.ethernet: timeout waiting for link to become ready
> 
> As per the recommendation from the Tegra hardware design team, fix this
> issue by:
> - clearing the PHY_RDY bit before setting the CDR_RESET bit and then
> setting PHY_RDY bit before clearing CDR_RESET bit. This ensures valid
> data is present at UPHY RX inputs before starting the CDR lock.

Did you do any testing with only these changes and without the delays?
Sounds to me like the sequence was blatantly wrong before, so maybe
fixing that already fixes the issue?

> - adding the required delays when bringing up the UPHY lane. Note we
> need to use delays here because there is no alternative, such as
> polling, for these cases.

One reason why I'm hoping that's enough is because ndelay() isn't great.
For one it can return early, and it's also usually not very precise. If
I look at the boot log on a Tegra234 device, the architecture timer (off
of which the ndelay() implementation on arm64 runs) runs at 31.25 MHz so
that gives us around 32 ns of precision.

On the other hand, some of these delays are fairly long for busy loops.
I'm not too worried about those 50ns ones, but the 500ns is quite a long
time (from the point of view of a CPU).

All in all, I wonder if we wouldn't be better off increasing these
delays to the point where we can use usleep_range(). That will make
the overall lane bringup slightly longer (though it should still be well
below 1ms, so hardly noticeable from a user's perspective) but has the
benefit of not blocking the CPU during this time.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)