linux-kernel - Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system firmware on a Dell hardware

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:   Wed, 16 Feb 2022 10:30:41 +0800
From:   Kai-Heng Feng <kai.heng.feng@...onical.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Heiner Kallweit <hkallweit1@...il.com>, linux@...linux.org.uk,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] net: phy: marvell: Honor phy LED set by system
 firmware on a Dell hardware

On Wed, Feb 16, 2022 at 4:27 AM Andrew Lunn <andrew@...n.ch> wrote:
>
> On Mon, Feb 14, 2022 at 01:40:43PM +0800, Kai-Heng Feng wrote:
> > On Sun, Jan 23, 2022 at 5:18 AM Andrew Lunn <andrew@...n.ch> wrote:
> > >
> > > > One more idea:
> > > > The hw reset default for register 16 is 0x101e. If the current value
> > > > is different when entering config_init then we could preserve it
> > > > because intentionally a specific value has been set.
> > > > Only if we find the hw reset default we'd set the values according
> > > > to the current code.
> > >
> > > We can split the problem into two.
> > >
> > > 1) I think saving LED configuration over suspend/resume is not an
> > > issue. It is probably something we will be needed if we ever get PHY
> > > LED configuration via sys/class/leds.
> > >
> > > 2) Knowing something else has configured the LEDs and the Linux driver
> > > should not touch it. In general, Linux tries not to trust the
> > > bootloader, because experience has shown bad things can happen when
> > > you do. We cannot tell if the LED configuration is different to
> > > defaults because something has deliberately set it, or it is just
> > > messed up, maybe from the previous boot/kexec, maybe by the
> > > bootloader. Even this Dell system BIOS gets it wrong, it configures
> > > the LED on power on, but not resume !?!?!. And what about reboot?
> >
> > The LED will be reconfigured correctly after each reboot.
> > The platform firmware folks doesn't want to restore the value on
> > resume because the Windows driver already does that. They are afraid
> > it may cause regression if firmware does the same thing.
>
> How can it cause regressions? Why would the Windows driver decide that
> if the PHY already has the correct configuration is should mess it all
> up? Have you looked at the sources and check what it does?

Unfortunately I don't and won't have access to the driver source for Windows.

>
> Anyway, we said that we need to save and restore the LED configuration
> over suspend/resume because at some point in the maybe distant future,
> we are going to support user configuration of the LEDs via
> /sys/class/leds. So you can add the needed support to the PHY driver.

OK.

>
> > This is an ACPI based platform and we are working on new firmware
> > property "use-firmware-led" to give driver a hint:
> > ...
> >     Scope (_SB.PC00.OTN0)
> >     {
> >         Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
> >         {
> >             ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* Device
> > Properties for _DSD */,
> >             Package (0x01)
> >             {
> >                 Package (0x02)
> >                 {
> >                     "use-firmware-led",
> >                     One
> >                 }
> >             }
> >         })
> >     }
> > ...
> >
> > Because the property is under PCI device namespace, I am not sure how
> > to (cleanly) bring the property from the phylink side to phydev side.
> > Do you have any suggestion?
>
> I'm no ACPI expert, but i think
> Documentation/firmware-guide/acpi/dsd/phy.rst gives you the basis:
>
>     During the MDIO bus driver initialization, PHYs on this bus are probed
>     using the _ADR object as shown below and are registered on the MDIO bus.
>
>       Scope(\_SB.MDI0)
>       {
>         Device(PHY1) {
>           Name (_ADR, 0x1)
>         } // end of PHY1
>
>         Device(PHY2) {
>           Name (_ADR, 0x2)
>         } // end of PHY2
>       }
>
> These are the PHYs on the MDIO bus. I _think_ that next to the Name,
> you can add additional properties, like your "use-firmware-led". This
> would then be very similar to DT, which is in effect what ACPI is
> copying. So you need to update this document with your new property,
> making it clear that this property only applies to boot, not
> suspend/resume. And fwnode_mdiobus_register_phy() can look for the
> property and set a flag in the phydev structure indicating that ACPI
> is totally responsible for LEDs at boot time.

The problem here is there's no MDIO bus in ACPI namespace, namely no
"Scope(\_SB.MDI0)" on this platform.

Since the phydev doesn't have a fwnode, the new property needs to be
passed from phylink to phydev, and right now I can't find a clean way
to do it.

Kai-Heng

>
>         Andrew