Re: [PATCH RFC net-next 7/9] net: stmmac: support custom XGMAC register layouts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] mm: heap OOB write in ethtool register dump with custom MAC/DMA layouts
- [High] net: stmmac: SError panic during ethtool dump over unmapped sparse MMIO gaps
- [High] net: stmmac: incorrect DMA channel offset scaling for odd/even bases
- [Medium] net: stmmac: PPS registers ignore custom timestamp_base override
- [Medium] net: stmmac: zero-initialized dwxgmac_addrs fields silently overwrite critical MAC registers
- [Low] net: stmmac: socfpga explicitly hardcodes NULL for custom address overrides
--

commit c3f13a2a3a2ef1c42853ddb7b0f741ba03e49292
Author: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

net: stmmac: support custom XGMAC register layouts

This commit allows platforms to override the MTL queues, DMA channels, and
timestamp register offsets via custom platform data, while retaining the
standard layout by default. It updates various macro definitions and driver
functions to use these configurable bases instead of hardcoded addresses.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> index 1d7f0a57d2889..331ba38abae66 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
> @@ -365,7 +365,7 @@ static int smtg_crosststamp(ktime_t *device, struct system_counterval_t *system,
>  	};
>  
>  	num_snapshot = FIELD_GET(XGMAC_TIMESTAMP_ATSNS_MASK,
> -				 readl(ioaddr + XGMAC_TIMESTAMP_STATUS));
> +				 readl(ioaddr + XGMAC_TIMESTAMP_STATUS(NULL)));

[Severity: Low]
Does explicitly passing NULL here break future compatibility? By hardcoding
the fallback state instead of using the layout configuration available via
the local priv context (e.g., priv->plat->dwxgmac_addrs), it seems this would
fail to use a custom PTP base on future socfpga hardware revisions.

>  
>  	/* Repeat until the timestamps are from the FIFO last segment */
>  	for (i = 0; i < num_snapshot; i++) {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> index f8ab347f7b5b9..22b476358a202 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> @@ -220,12 +220,21 @@
>  #define XGMAC_CT			BIT(1)
>  #define XGMAC_OB			BIT(0)
>  #define XGMAC_RSS_DATA			0x00000c8c
> -#define XGMAC_TIMESTAMP_STATUS		0x00000d20
> +
> +/* Timestamp registers */
> +#define XGMAC_TIMESTAMP_BASE_ADDR	0x00000d00
> +
> +static inline u32 xgmac_timestamp_base_addr(const struct dwxgmac_addrs *addrs)
> +{
> +	return addrs ? addrs->timestamp_base : XGMAC_TIMESTAMP_BASE_ADDR;
> +}

[Severity: Medium]
Will this return 0 if a platform supplies dwxgmac_addrs for DMA overrides but
leaves timestamp_base uninitialized? If addrs is non-NULL but timestamp_base
is 0, this inline helper doesn't fall back to XGMAC_TIMESTAMP_BASE_ADDR, which
could map critical operations to offset 0 (the MAC TX Configuration register)
and silently corrupt MAC state.

> +
> +#define XGMAC_TIMESTAMP_STATUS(a)	(xgmac_timestamp_base_addr(a) + 0x20)
>  #define XGMAC_TIMESTAMP_ATSNS_MASK	GENMASK(29, 25)
>  #define XGMAC_TXTSC			BIT(15)
> -#define XGMAC_TXTIMESTAMP_NSEC		0x00000d30
> +#define XGMAC_TXTIMESTAMP_NSEC(a)	(xgmac_timestamp_base_addr(a) + 0x30)
>  #define XGMAC_TXTSSTSLO			GENMASK(30, 0)
> -#define XGMAC_TXTIMESTAMP_SEC		0x00000d34
> +#define XGMAC_TXTIMESTAMP_SEC(a)	(xgmac_timestamp_base_addr(a) + 0x34)
>  #define XGMAC_PPS_CONTROL		0x00000d70

[Severity: Medium]
Should XGMAC_PPS_CONTROL and other PPS registers also use the custom
xgmac_timestamp_base_addr override? By remaining hardcoded to 0x0d70,
if a platform uses a custom timestamp_base, PTP PPS functionality will write
to standard hardware offsets rather than the custom relocated timestamp block.

[ ... ]

> +static inline u32
> +xgmac_mtl_chanx_base_addr(const struct dwxgmac_addrs *addrs, u32 x)
> +{
> +	return addrs ? addrs->mtl_chan_base + x * addrs->mtl_chan_offset
> +		     : XGMAC_MTL_CHAN_BASE_ADDR + x * XGMAC_MTL_CHAN_OFFSET;
> +}

[Severity: Medium]
Similar to the timestamp base helper, does this properly fall back to
XGMAC_MTL_CHAN_BASE_ADDR if addrs is provided but mtl_chan_base is 0?
This could also silently overwrite offset 0.

[ ... ]

> +static inline u32
> +xgmac_dma_chanx_base_addr(const struct dwxgmac_addrs *addrs, u32 x)
> +{
> +	if (addrs && (x & 1))
> +		return addrs->dma_odd_chan_base + x * addrs->dma_chan_offset;
> +	if (addrs)
> +		return addrs->dma_even_chan_base + x * addrs->dma_chan_offset;
> +	return XGMAC_DMA_CHAN_BASE_ADDR + x * XGMAC_DMA_CHAN_OFFSET;
> +}

[Severity: High]
Does multiplying by the absolute index x here incorrectly double the intended
physical spacing for channels using split odd/even bases? For instance,
channel 1 will map to odd_base + 1 * offset, and channel 2 to
even_base + 2 * offset. Should this scale by (x / 2) to interleave the
odd and even channels correctly?

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> index 1a88cbaed70ca..d1099a1cf9ea5 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c

[ ... ]

> @@ -292,7 +294,7 @@ static void dwxgmac2_dump_regs(struct mac_device_info *hw, u32 *reg_space)
>  	void __iomem *ioaddr = hw->pcsr;
>  	int i;
>  
> -	for (i = 0; i < XGMAC_MAC_REGSIZE; i++)
> +	for (i = 0; i < XGMAC_MAC_REGSIZE(hw->dwxgmac_addrs); i++)
>  		reg_space[i] = readl(ioaddr + i * 4);
>  }

[Severity: High]
Can this result in a heap out-of-bounds write? The ethtool register buffer is
allocated based on stmmac_ethtool_get_regs_len(), which uses
XGMAC_REGSIZE(priv->plat->dwxgmac_addrs) * 4 (the maximum DMA channel offset).
However, dwxgmac2_dump_regs() writes up to
XGMAC_MAC_REGSIZE(hw->dwxgmac_addrs). If a platform configures a layout
where the maximum MTL offset exceeds the maximum DMA offset, the loop will
write past the end of the allocated buffer.

[Severity: High]
Could this linear read sweep also cause a synchronous external abort
(bus fault) on ARM/ARM64 devices? If a platform uses custom bases (like
dma_even_chan_base = 0x8500), it might create large unmapped MMIO gaps
between standard blocks and custom channels. Iterating through the entire
range with readl() would attempt to read from these unmapped physical
addresses.

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> index ff83858ebc1f3..65d5e2a828076 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c

[ ... ]

> @@ -118,16 +121,18 @@ static void dwxgmac2_dma_axi(void __iomem *ioaddr, struct stmmac_axi *axi)
>  static void dwxgmac2_dma_dump_regs(struct stmmac_priv *priv,
>  				   void __iomem *ioaddr, u32 *reg_space)
>  {
> +	const struct dwxgmac_addrs *addrs = priv->plat->dwxgmac_addrs;
>  	int i;
>  
> -	for (i = (XGMAC_DMA_MODE / 4); i < XGMAC_REGSIZE; i++)
> +	for (i = (XGMAC_DMA_MODE / 4); i < XGMAC_REGSIZE(addrs); i++)
>  		reg_space[i] = readl(ioaddr + i * 4);
>  }

[Severity: High]
Similar to dwxgmac2_dump_regs(), could this loop read from unmapped memory
gaps if custom DMA channel offsets are used, potentially triggering a
hardware bus fault during an ethtool dump?

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index 1cf0f8820b337..82ce81fb6a18b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -351,8 +351,9 @@ static int stmmac_ethtool_get_regs_len(struct net_device *dev)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  
> -	if (priv->plat->core_type == DWMAC_CORE_XGMAC)
> -		return XGMAC_REGSIZE * 4;
> +	if (priv->plat->core_type == DWMAC_CORE_XGMAC ||
> +	    priv->plat->core_type == DWMAC_CORE_25GMAC)
> +		return XGMAC_REGSIZE(priv->plat->dwxgmac_addrs) * 4;
>  	else if (priv->plat->core_type == DWMAC_CORE_GMAC4)
>  		return GMAC4_REG_SPACE_SIZE;
>  	return REG_SPACE_SIZE;

[Severity: High]
As mentioned above, this allocates a buffer bounded exclusively by the
maximum DMA offset, which can lead to an out-of-bounds write in
dwxgmac2_dump_regs() if the custom MTL offset is larger.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-qcom_xpcs_nord_emac-v1-0-4b1c682af70f@oss.qualcomm.com?part=7