Re: [PATCH net-next v8 6/9] net: mdio: realtek-rtl9300: Add page tracking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The software cache for the PHY page register (`priv->page[port]`) is not cleared when the PHY is detached and reset, leading to corrupted MDIO reads/writes on subsequent attachments.
- [Medium] Memory leak of `priv` when `otto_emdio_init_polling()` fails.
--

--- Patch [6]: [PATCH net-next v8 6/9] net: mdio: realtek-rtl9300: Add page tracking ---
commit 97666ebf004388a052d7dc76edffdda3f1aad8d5
Author: Markus Stockhausen <markus.stockhausen@gmx.de>

    net: mdio: realtek-rtl9300: Add page tracking
    
    The hardware polling unit of the Realtek switches has a very special
    handling for c22 PHY register 31 (aka Realtek page register) in place.

[ ... ]

> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> index 0d5b9530e43b1..6986bed5258ab 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c

[ ... ]

> @@ -356,7 +358,7 @@ static int otto_emdio_9300_read_c22(struct mii_bus *bus, int port, int regnum, u
>  	struct otto_emdio_cmd_regs cmd_data = {
>  		.c22_data	= FIELD_PREP(RTL9300_PHY_CTRL_REG_ADDR, regnum) |
>  				  FIELD_PREP(RTL9300_PHY_CTRL_PARK_PAGE, 0x1f) |
> -				  FIELD_PREP(RTL9300_PHY_CTRL_MAIN_PAGE, RAW_PAGE(priv)),
> +				  FIELD_PREP(RTL9300_PHY_CTRL_MAIN_PAGE, priv->page[port]),

[Severity: High]
Does the software cache for the PHY page register (priv->page[port]) need to
be cleared when the PHY is detached and reset?

If a PHY's page register was left non-zero (e.g. by userspace MDIO tools or
an interrupted PHY driver sequence), bringing the interface down detaches the
PHY and asserts a physical reset. The hardware resets its internal page to 0,
but the software cache retains the stale non-zero value.

When the interface is brought back up, the stale page cache is injected into
all subsequent MDIO accesses here. This can cause the PHY framework to
operate on the wrong register page, breaking PHY initialization and network
connectivity.

For example, in otto_emdio_notify_phy_detach():

static void otto_emdio_notify_phy_detach(struct phy_device *phydev)
{
...
	ret = otto_emdio_set_port_polling(priv, port, false);
	if (ret)
		dev_err(bus->parent, "failed to disable polling for port %d\n", port);
}

Should priv->page[port] be reset to 0 here to synchronize with the PHY's
physical reset?

>  		.io_data	= FIELD_PREP(RTL9300_PHY_CTRL_INDATA, port),
>  	};

[ ... ]

Also, unrelated to the diff hunks above, I noticed a potential issue in the
probe error path:

[Severity: Medium]
Does the failure path for otto_emdio_init_polling() in otto_emdio_probe()
leak the priv structure and its embedded mutex?

If otto_emdio_init_polling(priv) fails during driver probe:

	err = otto_emdio_init_polling(priv);
	if (err)
		return err;

This direct return bypasses the err_out label where kref_put() normally
cleans up priv and destroys its mutex, resulting in a memory leak.
Should this use goto err_out instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727191559.19617-1-markus.stockhausen@gmx.de?part=6