Re: [PATCH net-next v9 2/4] net: stmmac: Integrate dw25gmac into hwif handling

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: stmmac: Integrate dw25gmac into hwif handling

Integrate dw25gmac support into stmmac hardware interface handling.
Added a new entry to the stmmac_hw table in hwif.c.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> index 511b0fd5e8343..a69f558c0db06 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> @@ -287,6 +287,26 @@ static const struct stmmac_hwif_entry {
>  		.mmc = &dwxgmac_mmc_ops,
>  		.est = &dwmac510_est_ops,
>  		.setup = dwxlgmac2_setup,
> +	}, {
> +		.core_type = DWMAC_CORE_25GMAC,
> +		.min_id = DW25GMAC_CORE_3_20,

Are there other areas where the core capabilities will be misidentified?

Throughout stmmac_main.c and stmmac_ethtool.c, hardware features branch
explicitly on core_type == DWMAC_CORE_XGMAC instead of including
DWMAC_CORE_25GMAC.

This seems to cause a few side effects:
- stmmac_ethtool_get_drvinfo() misreports the driver as MAC100_ETHTOOL_NAME
  (st_mac100).
- stmmac_main.c enforces legacy MTU limits rather than XGMAC_JUMBO_LEN.
- stmmac_dma_cap_show() debugfs output treats the 25GMAC count of multiple
  MAC address registers as a simple boolean and omits Timestamp System Time
  Source details.

Does this also break MDIO communication for the PHY?

In stmmac_mdio_register(), XGMAC MDIO accessors are assigned strictly if
priv->plat->core_type == DWMAC_CORE_XGMAC. Because 25GMAC is omitted, it
falls into the else block and assigns legacy GMAC callbacks like
stmmac_mdio_read_c22().

The GMAC accessors format the control word using GMAC bitfields (checking
MII_ADDR_GBUSY at bit 0), while the XGMAC register layout expects
MII_XGMAC_BUSY at bit 22.

Could this misconfiguration write invalid bit patterns to the register and
wait on the wrong bit?

> +		.regs = {
> +			.ptp_off = PTP_XGMAC_OFFSET,
> +			.mmc_off = MMC_XGMAC_OFFSET,
> +			.est_off = EST_XGMAC_OFFSET,

Will this misconfigure the Time-to-Output Value for EST (802.3 Qbv)?

In stmmac_est.c, est_configure() and est_irq_status() use
if (priv->plat->core_type == DWMAC_CORE_XGMAC) to differentiate XGMAC from
GMAC5.

Without 25GMAC in that check, it falls through to the GMAC5 logic, applying
the EST_GMAC5_PTOV mask (bits 31:24) instead of the EST_XGMAC_PTOV mask
(bits 31:23) to the EST control register at EST_XGMAC_OFFSET.

> +		},
> +		.desc = &dwxgmac210_desc_ops,
> +		.dma = &dw25gmac400_dma_ops,
> +		.mac = &dwxgmac210_ops,

Does adding these ops introduce a buffer overflow during ethtool register
dumps?

Since stmmac_ethtool_get_regs_len() does not match DWMAC_CORE_25GMAC, it
falls back to returning REG_SPACE_SIZE (4192 bytes, or 1048 u32 elements).

However, these ops use the dwxgmac2_dump_regs() and dwxgmac2_dma_dump_regs()
callbacks, which write far beyond the 1048-element buffer:

- dwxgmac2_dump_regs() writes up to index XGMAC_MAC_REGSIZE (1309 elements).
- dwxgmac2_dma_dump_regs() writes from XGMAC_DMA_MODE / 4 (3072) to
  XGMAC_REGSIZE (3647 elements).

Will this corrupt kernel memory when a privileged user runs ethtool -d?

> +		.vlan = &dwxgmac210_vlan_ops,
> +		.hwtimestamp = &stmmac_ptp,
> +		.ptp = &stmmac_ptp_clock_ops,
> +		.mode = NULL,
> +		.tc = &dwmac510_tc_ops,
> +		.mmc = &dwxgmac_mmc_ops,
> +		.est = &dwmac510_est_ops,
> +		.setup = dw25gmac_setup,
> +		.quirks = NULL,
>  	},
>  };