Netdev Archive on lore.kernel.org
 help / color / Atom feed
From: Ard Biesheuvel <ardb@kernel.org>
To: Andrew Lunn <andrew@lunn.ch>
Cc: "Daniel Thompson" <daniel.thompson@linaro.org>,
	"Sumit Garg" <sumit.garg@linaro.org>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"Masami Hiramatsu" <mhiramat@kernel.org>,
	steve@einval.com,
	"Ilias Apalodimas" <ilias.apalodimas@linaro.org>,
	"open list:BPF JIT for MIPS (32-BIT AND 64-BIT)"
	<netdev@vger.kernel.org>, "Willy Liu" <willy.liu@realtek.com>,
	"David S. Miller" <davem@davemloft.net>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Sasha Levin" <sashal@kernel.org>,
	"Florian Fainelli" <f.fainelli@gmail.com>,
	"Heiner Kallweit" <hkallweit1@gmail.com>,
	"Masahisa Kojima" <masahisa.kojima@linaro.org>
Subject: Re: realtek PHY commit bbc4d71d63549 causes regression
Date: Sun, 18 Oct 2020 12:56:02 +0200
Message-ID: <CAMj1kXF_mRBnTzee4j7+e9ogKiW=BXQ8-nbgq2wDcw0zaL1d5w@mail.gmail.com> (raw)
In-Reply-To: <CAMj1kXGO=5MsbLYvng4JWdNhJ3Nb0TSFKvnT-ZhjF2xcO9dZaw@mail.gmail.com>

On Sun, 18 Oct 2020 at 12:35, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Sun, 18 Oct 2020 at 01:02, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Sun, Oct 18, 2020 at 12:19:25AM +0200, Ard Biesheuvel wrote:
> > > (cc'ing some folks who may care about functional networking on SynQuacer)
> > >
> > > On Sat, 17 Oct 2020 at 21:49, Andrew Lunn <andrew@lunn.ch> wrote:
> > > >
> > > > > So we can fix this firmware by just setting phy-mode to the empty
> > > > > string, right?
> > > >
> > > > I've never actually tried it, but i think so. There are no DT files
> > > > actually doing that, so you really do need to test it and see. And
> > > > there might be some differences between device_get_phy_mode() and
> > > > of_get_phy_mode().
> > > >
> > >
> > > Yes, that works fine. Fixed now in the latest firmware build [0]
> >
> > 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? 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. This also means that ACPI is simply not the right fit for all
> designs, and we should push back harder against the tick-the-box
> exercises that are going on to use ACPI for describing unusual designs
> that are never going to boot RHEL or other ACPI-only distros anyway.
>
> > > But that still leaves the question whether and how to work around this
> > > for units in the field. Ignoring the PHY mode in the driver would
> > > help, as all known hardware ships with firmware that configures the
> > > PHY with the correct settings, but we will lose the ability to use
> > > other PHY modes in the future, in case the SoC is ever used with DT
> > > based minimal firmware that does not configure networking.
> > >
> > > Since ACPI implies rich firmware, we could make ACPI probing of the
> > > driver ignore the phy-mode setting in the DSDT.
> >
> > I'm O.K. with that, for this driver, but please add a comment in the
> > code about why ACPI ignores DSDT, because of older broken firmware.
> >
>
> Sure.
>
> > > But if we don't do the same for DT, it would mean DT users are
> > > forced to upgrade their firmware, and hopefully do so before
> > > upgrading to a kernel that breaks networking.
> >
> > Nothing new there. As i said, we have been through this before with
> > the Atheros PHY and others.
> >
> > One option would be to move the DT into the kernel and fix it. Most
> > distributions already use the DT version shipped with the kernel, so
> > they would automatically get the fixed DT at the same time as the
> > kernel which needs the fix.
> >
>
> The DT is not a flat file that you can simply source from elsewhere -
> it is constructed at boot using firmware settings and other data that
> is different between systems.
>
>
> 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.
>
> 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? It
> doesn't have a fixes tag or cc:stable, and given that it is not
> actually correct to begin with, there seems to be little justification
> for this. Surely, no platform exists in the field that requires this
> commit (as it is broken) or the followup fix (since only was never
> taken into account before)
>
> In summary, I think that - even if we agree that the way forward is to
> fix the firmware and make the driver do the right thing without any
> quirks - these patches do not belong in -stable, and should be
> reverted, unless anyone can point out a system that was working
> before, got broken and was fixed again by f81dadbcf7fd (i.e., a true
> regression)

Oops - f81dadbcf7fd was never backported, only the fix was. Apologies
for mixing that up.

However, that leaves the question why bbc4d71d63549bcd was backported,
although I understand why the discussion is a bit trickier there. But
if it did not fix a regression, only broken code that never worked in
the first place, I am not convinced it belongs in -stable.

  reply index

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-17 14:20 Ard Biesheuvel
2020-10-17 14:44 ` Andrew Lunn
2020-10-17 14:46   ` Ard Biesheuvel
2020-10-17 15:11     ` Andrew Lunn
2020-10-17 15:18       ` Ard Biesheuvel
2020-10-17 16:14         ` Ilias Apalodimas
2020-10-17 16:17           ` Ard Biesheuvel
2020-10-17 16:21             ` Ilias Apalodimas
2020-10-17 18:04             ` Andrew Lunn
2020-10-17 18:11               ` Ard Biesheuvel
2020-10-17 18:17                 ` Ard Biesheuvel
2020-10-17 18:27                 ` Andrew Lunn
2020-10-17 18:55                   ` Ard Biesheuvel
2020-10-17 19:49                     ` Andrew Lunn
2020-10-17 22:19                       ` Ard Biesheuvel
2020-10-17 23:02                         ` Andrew Lunn
2020-10-18 10:35                           ` Ard Biesheuvel
2020-10-18 10:56                             ` Ard Biesheuvel [this message]
2020-10-18 15:45                               ` Andrew Lunn
2020-10-25 14:16                                 ` Ard Biesheuvel
2020-10-25 14:28                                   ` Andrew Lunn
2020-10-25 14:34                                     ` Ard Biesheuvel
2020-10-25 14:42                                       ` Andrew Lunn
2020-10-29 14:21                                         ` Ilias Apalodimas
2020-10-29 14:39                                           ` Andrew Lunn
2020-10-29 14:42                                             ` Ard Biesheuvel
2020-10-29 14:50                                               ` Andrew Lunn
2020-10-29 14:46                                             ` Ilias Apalodimas
2020-11-05 17:31                                               ` Jernej Škrabec
2020-11-13 13:50                                                 ` Arnd Bergmann
2020-11-13 14:44                                                   ` Andrew Lunn
2020-11-13 15:33                                                     ` Arnd Bergmann
2020-11-13 16:56                                                       ` Andrew Lunn
2020-11-13 21:26                                                         ` Arnd Bergmann
2020-11-13 22:43                                                           ` Andrew Lunn
2020-11-13 22:49                                                             ` Ard Biesheuvel
2020-11-14  0:40                                                               ` Andrew Lunn
2020-11-14 10:09                                                                 ` Ard Biesheuvel
2020-10-18 15:38                             ` Andrew Lunn
2020-10-18 14:20                         ` Masami Hiramatsu
2020-10-17 18:01           ` Andrew Lunn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMj1kXF_mRBnTzee4j7+e9ogKiW=BXQ8-nbgq2wDcw0zaL1d5w@mail.gmail.com' \
    --to=ardb@kernel.org \
    --cc=alex.bennee@linaro.org \
    --cc=andrew@lunn.ch \
    --cc=daniel.thompson@linaro.org \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=ilias.apalodimas@linaro.org \
    --cc=kuba@kernel.org \
    --cc=masahisa.kojima@linaro.org \
    --cc=mhiramat@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=sashal@kernel.org \
    --cc=steve@einval.com \
    --cc=sumit.garg@linaro.org \
    --cc=willy.liu@realtek.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git