linux-kernel - Re: [PATCH 3/3] net: mdio: reset PHY before attempting to access registers in fwnode_mdiobus_register

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: <aG-R4gWfI5QKRo5w@debianbuilder>
Date: Thu, 10 Jul 2025 12:11:46 +0200
From: Buday Csaba <buday.csaba@...lan.hu>
To: Andrew Lunn <andrew@...n.ch>
CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	Csókás Bence <csokas.bence@...lan.hu>, Heiner Kallweit
	<hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>, "David S.
 Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, "Jakub
 Kicinski" <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH 3/3] net: mdio: reset PHY before attempting to access
 registers in fwnode_mdiobus_register_phy

On Wed, Jul 09, 2025 at 03:41:45PM +0200, Andrew Lunn wrote:
> On Wed, Jul 09, 2025 at 03:32:22PM +0200, Buday Csaba wrote:
> > Some PHYs (e.g. LAN8710A) require a reset after power-on,even for
> > MDIO register access.
> > The current implementation of fwnode_mdiobus_register_phy() and
> > get_phy_device() attempt to read the id registers without ensuring
> > that the PHY had a reset before, which can fail on these devices.
> >
> > This patch addresses that shortcoming, by always resetting the PHY
> > (when such property is given in the device tree). To keep the code
> > impact minimal, a change was also needed in phy_device_remove() to
> > prevent asserting the reset on device removal.
> >
> > According to the documentation of phy_device_remove(), it should
> > reverse the effect of phy_device_register(). Since the reset GPIO
> > is in undefined state before that, it should be acceptable to leave
> > it unchanged during removal.
> >
> > Signed-off-by: Buday Csaba <buday.csaba@...lan.hu>
> > Cc: Csókás Bence <csokas.bence@...lan.hu>
>
> This appears to be a respost of the previous patch.
>
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html says:
>
> don’t repost your patches within one 24h period

Sorry for that, this is my first kernel patch, and git send-email tricked me.
I will paste your earlier reply below, and continue in this thread.

>
> This is specific to this device, so the driver for this device should
> take care of the reset.

I think it may affect every chip that has the `PHY_RST_AFTER_CLK_EN`
flag set. That is still not a terribly lot of devices, but it is more than
just this one. There are a lot of DT-s out there which use the (deprecated?)
`phy-reset-gpios` property of the fec driver. When that property is
present, that will reset the PHY chip before it gets to get_phy_device().
Even with this chip, only about 10% of them fail to report their ID without
reset (in our systems and startup configuration).

>
> To solve the chicken/egg, you need to put a compatible in the PHY node
> listing the ID of the PHY. That will cause the correct PHY driver to
> load, and probe. The probe can then reset it.

Yes, that approach does work — and I've tested it successfully with the
other two patches I sent. However, when a PHY ID is specified in the DT, it
is taken directly, not read from the hardware. This may lead to issues with
revision-specific logic, since the revision bits won’t necessarily match
the actual chip. For LAN8710A this isn’t currently a problem, but it may be
for other PHYs.

>
> We have to be careful about changing the reset behaviour, it is likely
> to break PHYs which currently work, but stop working when they get an
> unexpected reset.

I agree that changing the reset logic must be done with caution.

But these lines of code are actually the first, when the kernel tries to 
access the PHY. Is it not a good practice, to do that from a known,
reproducible state? That is what the reset is for. Without the reset, the
PHY may be in whatever state it was left previously. That could lead to 
hard to reproduce effects, like failing after restart, failing because
there was a change in the bootloader, etc.

A few lines later fwnode_mdiobus_phy_device_register() is called
anyway, that will also reset the PHY in the same conditions (DT based
system, and either `reset-gpios` or `reset-ctrl` is defined).

My intent was to ensure that if the system asks for a reset, it actually
happens before the PHY is accessed, rather than mid-way through
registration.

Changing phy_device_remove() may be more concerning. I can create a patch,
that leaves that intact, and only changes reset behaviour during reset, but
that will be a bit longer.

>
>     Andrew
>
> ---
> pw-bot: cr
>

Csaba