netdev - Re: [PATCH net v1 1/1] net: phy: micrel: ksz8081: disable broadcast only if PHY address is not 0

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: Thu, 27 Jun 2024 18:51:40 +0200
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Andrew Lunn <andrew@...n.ch>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	stable@...r.kernel.org, kernel@...gutronix.de,
	linux-kernel@...r.kernel.org, Russell King <linux@...linux.org.uk>,
	netdev@...r.kernel.org, Lukasz Majewski <lukma@...x.de>
Subject: Re: [PATCH net v1 1/1] net: phy: micrel: ksz8081: disable broadcast
 only if PHY address is not 0

Hi Andrew,

hm... looks like my previous answer is lost. I had here some hiccups on
my side. Sending new answer, do not wunder if there will be double
mail.

On Thu, Jun 27, 2024 at 03:56:42PM +0200, Andrew Lunn wrote:
> On Thu, Jun 27, 2024 at 07:33:53AM +0200, Oleksij Rempel wrote:
> > Do not disable broadcast if we are using address 0 (broadcast) to
> > communicate with this device. Otherwise we will use proper driver but no
> > communication will be possible and no link changes will be detected.
> > There are two scenarios where we can run in to this situation:
> > - PHY is bootstrapped for address 0
> 
> What do you mean by bootstrapped to address 0? The strapping pins set
> it to some other address, but the bootloader wrote to registers and
> moved it to address 0?

No no. Just strapping from HW perspective, no SW is involved.

> > - no PHY address is known and linux is scanning the MDIO bus, so first
> >   respond and attached device will be on address 0.
> 
> So in this case, the PHY is really at address X, where X != 0. It
> responds to all read requests, so the scanning finds it at all
> addresses. It also stomps over other devices on the bus when scanning
> for them, or probing them.
> 
> I'm not sure the current code is correct. But it is also going to be
> messy to not break backwards compatibility for DT blobs say the device
> is at address 0, when in fact it is not.

It looks like this is the actual case on my board.

> Is it possible to read the devices actual address from registers?

Yes.

> I'm wondering if probe should do something like:
> 
> int actual_address = phydev_read(phydev, 0x42);
> 
> if (actual_address == 0) {
> 	if (type->has_broadcast_disable) {
> 		phydev_dbg(phydev, "Disabling broadcast\n");
> 		kszphy_broadcast_disable(phydev);
> 	}
> 
> } else {
> 	if (actual_address != 0 &&
> 	 phydev->mdio.addr != actual_address &&
> 	 phydev->mdio.addr != 0) {
> 		if (type->has_broadcast_disable) {
> 			phydev_dbg(phydev, "Disabling broadcast\n");
> 			kszphy_broadcast_disable(phydev);
> 		}
>         return -ENODEV;
> 	}
> }
> 
> So if the devices really has an address is zero, turn off
> broadcast. That will stop it stomping over other devices, but the

If i understand the documentation correctly, disable broadcast bit is
designed to resolve conflict between two KSZ8081 PHYs on same bus
in one shot:
"For applications that require two KSZ8081RNA/RND PHYs to share the same
MDIO interface with one PHY set to address 0h and the other PHY set to
address 3h, use PHY address 0h (defaults to broadcast after power-up) to
set both PHYs’ Register 16h, Bit [9] to ‘1’ to assign PHY address 0h as
a unique (non-broadcast) PHY address."

For this scenario, we can't read configured address from the HW in the
first place, this can be done safely only if (phydev->mdio.addr != 0)

> damage is probably already done in terms of scanning.
> 
> If the devices is really at some address other than 0, and we are
> probing at a different address, and that address is not 0, turn off
> broadcast and say the device does not exist. I think we need to
> special case 0 because there are going to be some DT descriptions
> which say the device is at 0, when in fact it is not. We might want to
> add a phydev_warn() about this, to try to get the DT fixed.

Assuming there are no other devices on the bus listening for broadcast
too.

> > The fixes tag points to the latest refactoring, not to the initial point
> > where kszphy_broadcast_disable() was introduced.
> > 
> > Fixes: 79e498a9c7da0 ("net: phy: micrel: Restore led_mode and clk_sel on resume")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
> 
> Do you have a board which is going wrong because of this?

Yes, I can avoid this bug on the DT level. But the code side seems to be
broken too.

> Do you plan to submit patches for earlier stable releases?

Not so far. Should I?

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |