Re: [PATCH net-next v5 07/11] net: pcs: xpcs: add Rockchip RK3568 platform glue driver

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect MMD write remapping for MII vendor registers breaks configuration for ports 1, 2, and 3.
- [High] Calculation of `eee_mult_fact` overflows the 32-bit `unsigned long` on 32-bit architectures, leading to completely incorrect EEE configurations.
- [Medium] Acquiring `device_lock()` of another device from within a probe path causes a lockdep warning (recursive locking).
--

commit ba02c9523dcac3b326b325ec63d9a16255030d26
Author: Coia Prant [off-list ref]
net: pcs: xpcs: add Rockchip RK3568 platform glue driver

This commit adds the Rockchip RK3568 platform glue driver for the Synopsys
DesignWare XPCS. It creates a virtual MDIO bus to translate standard MDIO
operations into APB3 register accesses with address remapping.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf (Page 2078)

[ ... ]

+static int xpcs_rk_mdio_write_remapping(int addr, int dev, int reg)
+{
+	switch (dev) {
+	case MDIO_MMD_PMAPMD:
+		return ROCKCHIP_MMD_PMAPMD;
+	case MDIO_MMD_VEND2:
+		break;
+	default:
+		return -ENXIO;
+	}
+
+	/* Writable only on MII */
+	switch (reg) {
+	case DW_VR_MII_AN_CTRL:
+	case DW_VR_MII_AN_INTR_STS:
+	case DW_VR_MII_EEE_MCTRL0:
+	case DW_VR_MII_EEE_MCTRL1:
+	case DW_VR_MII_DIG_CTRL2:
+		return ROCKCHIP_MMD_MII;
+	default:
+		break;
+	}

[Severity: High]
Does this break the configuration for ports 1, 2, and 3? This block intercepts
writes to these specific MII vendor registers and unconditionally routes them
to MMD 7 (port 0), ignoring the port address mapping. 

If these registers are modified via the MDIO bus for ports greater than 0
during initialization, the auto-negotiation and EEE configurations might be
applied to port 0 instead, potentially breaking the network links for the
secondary ports.

[ ... ]

+static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
+{

[ ... ]

+	/*
+	 * Compute the multiplier for the EEE clock so that
+	 * clk_eee_period * (mult_fact + 1) falls within 80..120 ns.
+	 *
+	 * On RK3568, clk_xpcs_eee is muxed between gpll200 (200 MHz, 5 ns)
+	 * and cpll125 (125 MHz, 8 ns), selected by CRU_CLKSEL_CON29 bit 13.
+	 * The reset value is 0 (200 MHz), but derive the value at runtime to
+	 * stay correct if the mux is changed by a board.
+	 */
+	pxpcs->eee_mult_fact = DIV_ROUND_CLOSEST(100 * clk_get_rate(pxpcs->eee_clk),
+						 1000000000UL) - 1;

[Severity: High]
Can this calculation overflow on 32-bit architectures? clk_get_rate() returns
an unsigned long. On 32-bit architectures (like an ARM32 kernel or during a 
COMPILE_TEST build), the multiplication of 100 * 125,000,000 equals 
12,500,000,000, which exceeds the 32-bit maximum (4,294,967,295). 

This overflow could result in completely incorrect hardware timings for the
EEE configuration.

[ ... ]

+struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
+{

[ ... ]

+	pdev = of_find_device_by_node(pcs_np);
+	of_node_put(pcs_np);
+	if (!pdev)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	device_lock(&pdev->dev);
+	pxpcs = platform_get_drvdata(pdev);
+	if (!pxpcs || !pxpcs->bus) {
+		device_unlock(&pdev->dev);

[Severity: Medium]
Will this trigger a lockdep warning for recursive locking? The MAC driver's 
probe already holds its own dev->mutex via really_probe() when it invokes 
xpcs_rk_create(), and this code then attempts to acquire the XPCS device's 
dev->mutex. 

Since all default device locks share the same lockdep class, could this
un-annotated nesting cause a false-positive lockdep splat that pollutes
kernel logs (and panics if panic_on_warn is enabled)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913185849.907479-1-coiaprant@gmail.com?part=7