netdev - Re: [PATCH net-next v9] net: mdio: Add RTL9300 MDIO driver

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: <Z88FBR7m1olkTXxR@pidgin.makrotopia.org>
Date: Mon, 10 Mar 2025 15:28:05 +0000
From: Daniel Golle <daniel@...rotopia.org>
To: Chris Packham <Chris.Packham@...iedtelesis.co.nz>
Cc: "andrew@...n.ch" <andrew@...n.ch>,
	"hkallweit1@...il.com" <hkallweit1@...il.com>,
	"linux@...linux.org.uk" <linux@...linux.org.uk>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"sander@...nheule.net" <sander@...nheule.net>,
	"markus.stockhausen@....de" <markus.stockhausen@....de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v9] net: mdio: Add RTL9300 MDIO driver

On Mon, Mar 10, 2025 at 02:07:26AM +0000, Chris Packham wrote:
> Hi Daniel,
> 
> On 10/03/2025 14:31, Daniel Golle wrote:
> > Hi Chris,
> >
> > On Mon, Mar 10, 2025 at 12:25:36PM +1300, Chris Packham wrote:
> >> Add a driver for the MDIO controller on the RTL9300 family of Ethernet
> >> switches with integrated SoC. There are 4 physical SMI interfaces on the
> >> RTL9300 however access is done using the switch ports. The driver takes
> >> the MDIO bus hierarchy from the DTS and uses this to configure the
> >> switch ports so they are associated with the correct PHY. This mapping
> >> is also used when dealing with software requests from phylib.
> >>
> >> Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
> >> ---
> >> ...
> >> +static int rtl9300_mdio_read_c22(struct mii_bus *bus, int phy_id, int regnum)
> >> +{
> >> +	struct rtl9300_mdio_chan *chan = bus->priv;
> >> +	struct rtl9300_mdio_priv *priv;
> >> +	struct regmap *regmap;
> >> +	int port;
> >> +	u32 val;
> >> +	int err;
> >> +
> >> +	priv = chan->priv;
> >> +	regmap = priv->regmap;
> >> +
> >> +	port = rtl9300_mdio_phy_to_port(bus, phy_id);
> >> +	if (port < 0)
> >> +		return port;
> >> +
> >> +	mutex_lock(&priv->lock);
> >> +	err = rtl9300_mdio_wait_ready(priv);
> >> +	if (err)
> >> +		goto out_err;
> >> +
> >> +	err = regmap_write(regmap, SMI_ACCESS_PHY_CTRL_2, FIELD_PREP(PHY_CTRL_INDATA, port));
> >> +	if (err)
> >> +		goto out_err;
> >> +
> >> +	val = FIELD_PREP(PHY_CTRL_REG_ADDR, regnum) |
> >> +	      FIELD_PREP(PHY_CTRL_PARK_PAGE, 0x1f) |
> >> +	      FIELD_PREP(PHY_CTRL_MAIN_PAGE, 0xfff) |
> >> +	      PHY_CTRL_READ | PHY_CTRL_TYPE_C22 | PHY_CTRL_CMD;
> > Using "raw" access to the PHY and thereby bypassing the MDIO
> > controller's support for hardware-assisted page access is problematic.
> > The MDIO controller also polls all PHYs status in hardware and hence
> > be aware of the currently selected page. Using raw access to switch
> > the page of a PHY "behind the back" of the hardware polling mechanism
> > results in in occassional havoc on link status changes in case Linux'
> > reading the phy status overlaps with the hardware polling.
> > This is esp. when using RealTek's 2.5GBit/s PHYs which require using
> > paged access in their read_status() function.
> >
> > Markus Stockhausen (already in Cc) has implemented a nice solution to
> > this problem, including documentation, see
> > https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/realtek/files-6.6/drivers/net/ethernet/rtl838x_eth.c;h=4b79090696e341ed1e432a7ec5c0f7f92776f0e1;hb=HEAD#l1631
> 
> I read that code/comment a few times and I must admit I still don't 
> quite get it. Part of the problem might be that my hardware platform is 
> using C45 PHYs and that's what I've been testing with. The C22
> support
> is based on my reading of the datasheet and some of what I can glean 
> from openwrt (although I completely missed that comment when I read 
> through the driver the first time).

Yes, this issue exists only with Clause-22 access, Clause-45 doesn't
require any of that. Also note that OpenWrt has recently switched
implementation from a (not very upstream friendly) approach requiring
dedicated support for paged access to Markus' new implementation which
also added the comment.

> 
> > Including a similar mechanism in this driver for C22 read and write
> > operations would be my advise, so hardware-assisted access to the PHY
> > pages is always used, and hence the hardware polling mechanism is aware
> > of the currently selected page.
> 
> So far upstream Linux doesn't have generic paged PHY register functions. 
> It sounds like that'd be a prerequisite for this.

No, that's exactly what Markus has improved about the implementation
compared to the previous approach:
We simply intercept access to C22 register 0x1f. For write access the
to-be-selected page is stored in the priv struct of the MDIO bus, in a
0-initialized array for each MDIO bus address. C22 read access to
register 0x1f can read back that value, all without actually acessing the
hardware.
Any subsequent C22 read or write operation will then use the selected
page stored for that PHY in the memory associated with the MDIO bus.

In this way page selection is left to the MDIO controller which also
carries out polling in background (see SMI_POLL_CTRL), and clashes due
to congruent access by Linux and hardware polling don't occur.