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

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

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.

Other than that the driver looks really good now, and will allow using
existing RealTek PHY drivers independently of whether they are used with
RealTek's switch SoCs or with non-RealTek systems -- this has always
been a big issue with OpenWrt's current implementation and I look
forward to use this driver instead asap ;)