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

Message-ID: <YgyqJAokWhXvDPik@lunn.ch>
Date:   Wed, 16 Feb 2022 08:39:16 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Kai-Heng Feng <kai.heng.feng@...onical.com>
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

> > > 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.

So add it. Basically, copy what DT does. I assume there is a node for
the Ethernet device? And is the MDIO bus driver instantiated by the
Ethernet device? So you can add the MDIO node as a sub node of the
Ethernet device. When you register the MDIO bus using
acpi_mdiobus_register() pass a pointer to this MDIO sub node.

     Andrew