netdev - Re: [PATCH 2/2] net: phy: realtek: add dt property to disable broadcast PHY address

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]

Message-ID: <ae46016c-c391-42c1-854e-075e7ee03a62@lunn.ch>
Date: Tue, 3 Dec 2024 03:54:17 +0100
From: Andrew Lunn <andrew@...n.ch>
To: 万致远 <kmlinuxm@...il.com>
Cc: kuba@...nel.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	willy.liu@...ltek.com, Yuki Lee <febrieac@...look.com>
Subject: Re: [PATCH 2/2] net: phy: realtek: add dt property to disable
 broadcast PHY address

> > I think you can do this without needing a new property. The DT binding
> > has:
> >
> >             reg = <4>;
> >
> > This is the address the PHY should respond on. If reg is not 0, then
> > broadcast is not wanted.
> >
> First, broadcast has no relationship with PHY address, it allows MAC
> broadcast command to all supported PHY on the MDIO bus.
> 
> I can't assume that there's no user use this feature to configure multiple
> PHY devices (e.g. there's like 3 or more PHYs on board, their address
> represented as 1, 2, 3. When this feature is enabled (default behavior),
> users can send commands to address 0 to configure common parameters shared
> by these PHYs) at the same time.

phylib does not do that. Each PHY is considered a single entity. User
space could in theory do it via phy_do_ioctl(), but that is a very
risky thing to do, there is no locking, and you are likely to confuse
phylib and/or the PHY driver.

So we don't actually need the broadcast feature.

> Again, the broadcast address is shared by all PHYs on MDIO which
> support this feature, it's handy for MAC to change multiple PHYs
> setting at the same time.

Please point me at a MAC driver doing this.

> I would recommend to add this feature, because it doesn't change the
> behavior of this driver, and allows this PHY works together with
> other PHY or switch chip which don't support this feature, like mt7530 or
> Marvell ones.

I agree we should be disabling this when it is safe to disable, but i
don't agree we need a new property, reg is sufficient.

	Andrew