netdev - Re: realtek PHY commit bbc4d71d63549 causes regression

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:   Sun, 18 Oct 2020 17:38:13 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     Daniel Thompson <daniel.thompson@...aro.org>,
        Sumit Garg <sumit.garg@...aro.org>,
        Alex Bennée <alex.bennee@...aro.org>,
        Masami Hiramatsu <mhiramat@...nel.org>, steve@...val.com,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        "open list:BPF JIT for MIPS (32-BIT AND 64-BIT)" 
        <netdev@...r.kernel.org>, Willy Liu <willy.liu@...ltek.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Sasha Levin <sashal@...nel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Masahisa Kojima <masahisa.kojima@...aro.org>
Subject: Re: realtek PHY commit bbc4d71d63549 causes regression

> > Thanks for reporting back on that. It probably needs to be something
> > we always recommend for ACPI systems, either use "", or preferably no
> > phy-mode at all, and let the driver default to NA. ACPI and networking
> > is at a very early stage at the moment, since UEFA says nothing about
> > it. It will take a while before we figure out the best practices, and
> > some vendor gets something added to the ACPI specifications.
> >
> 
> You mean MDIO topology, right?

That is part of it. I2C for SFPs, ethernet switches, etc.  I think
SFPs are going to be the real sticking point, since when you get above
10Gbps, you are into the land of SFPs, and server platforms are those
which will be going above 10G first.

> Every x86 PC and server in the world uses ACPI, and networking
> doesn't seem to be a problem there, so it is simply a matter of
> choosing the right abstraction level. I know this is much easier to
> achieve when all the network controllers are on PCIe cards, but the
> point remains valid: exhaustively describing the entire SoC like we
> do for DT is definitely not the right choice for ACPI systems.

Agreed. And i am pushing back. But we also have the conflict that some
SoC IP is used in systems that will boot RHEL, Debian, etc, and in
systems that are DT based. Do you want to write two drivers? One
assuming ACPI is doing all the work, and another where DT describes
the system and drivers and network core does all the work configuring
the hardware. For the same piece of hardware?

> I do have a question about the history here, btw. As I understand it,
> before commit f81dadbcf7fd ("net: phy: realtek: Add rtl8211e rx/tx
> delays config"), the phy-mode setting did not have any effect on
> Realtek PHYs in the first place, right? Since otherwise, this platform
> would never have worked from the beginning.

I suspect it did work to some extent, but not fully. So it could be,
it worked for whatever platform Serge was using, but failed in some
other cases, e.g. you board, and left the delays alone.

Later the vendor came along and said the code was wrong, and provided
some bits of information from the datasheet which is not publicly
available. As a result f81dadbcf7fd happened. Since that fixed a
previous patch, it was given a Fixes: tag and i assume back ported.

> So commit f81dadbcf7fd was backported to -stable, even though it
> didn't actually work, and had to be fixed in bbc4d71d63549bcd ("net:
> phy: realtek: fix rtl8211e rx/tx delay config"), which is the commit
> that causes the regression on these boards.
> 
> So why was commit f81dadbcf7fd sent to -stable in the first place?

f81dadbcf7fd first appears in v5.2. I don't see it in 4.19.152, the
LTS kernel older than that. So it is not in -stable.

    Andrew