Re: [PATCH net-next v2 2/2] net: stmmac: Add support for Allwinner A733 GMAC210

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Writing to the GMAC configuration MMIO registers before the required AHB reset is deasserted and bus clocks are enabled will cause a bus fault (synchronous external abort) and hang the system.
- [Medium] Hardware configuration programmed into the controller's glue registers will be lost if the power domain is cycled during suspend, because the driver lacks a resume callback.
--

commit b1b91e273360986b68768571246550826028aa8d
Author: Jerome Brunet <jbrunet@baylibre.com>

net: stmmac: Add support for Allwinner A733 GMAC210

This commit adds support for the A733 GMAC210 controller, introducing
a dedicated MMIO region for glue configuration registers. It handles TX
clock delays, enables per-DMA-channel interrupts, and configures TX LPI
clock gating.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun55i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun55i.c
> index 862df173d963f..3d434a52fb02d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun55i.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun55i.c
[ ... ]
> @@ -37,17 +38,99 @@
[ ... ]
> +static struct regmap *sun60i_gmac210_get_regmap(struct platform_device *pdev,
> +						struct plat_stmmacenet_data *plat)
> +{
> +	struct device *dev = &pdev->dev;
> +	void __iomem *base;
> +	int ret;
> +
> +	base = devm_platform_ioremap_resource(pdev, 1);
> +	if (IS_ERR(base)) {
> +		dev_err_probe(dev, PTR_ERR(base), "unable to get glue memory region\n");
> +		return ERR_CAST(base);
> +	}
> +
> +	if (!plat->stmmac_rst || !plat->stmmac_ahb_rst) {
> +		dev_err(dev, "missing required reset controls\n");
> +		return ERR_PTR(-EINVAL);
> +	}

[Severity: High]
Does this leave the AHB reset asserted during register access?

The driver verifies that plat->stmmac_ahb_rst is present, but it does
not appear to deassert it before returning the regmap or performing
writes.

[ ... ]
> @@ -118,11 +212,7 @@ static int sun55i_gmac200_probe(struct platform_device *pdev)
>  	if (IS_ERR(plat_dat))
>  		return PTR_ERR(plat_dat);
>  
> -	/* BSP disables it */
> -	plat_dat->flags |= STMMAC_FLAG_SPH_DISABLE;
> -	plat_dat->host_dma_width = 32;
> -
> -	ret = sun55i_gmac200_setup(pdev, plat_dat, data);

[Severity: High]
Can writing to the controller's MMIO region here trigger a synchronous
external abort?

sun55i_gmac200_setup() calls regmap_write() on the glue configuration
registers. However, at this point, the AHB reset has not been deasserted,
and the mbus clock is only enabled below this call.

Accessing an ARM SoC peripheral's MMIO registers while its reset is
asserted or its bus clocks are disabled typically results in a bus fault
and a system hang.

>  	if (ret)
>  		return ret;
>  
> @@ -138,8 +228,27 @@ static int sun55i_gmac200_probe(struct platform_device *pdev)
>  	return devm_stmmac_pltfr_probe(pdev, plat_dat, &stmmac_res);
>  }

[Severity: Medium]
Will the hardware configuration be lost after a system suspend?

The commit message notes that the glue configuration registers are in the
controller's reset domain. When the system enters suspend (e.g., S2RAM),
power is cycled and these registers will likely revert to their hardware
defaults.

Since the driver writes this configuration exactly once during probe in
sun55i_gmac200_setup(), and there is no resume callback assigned (e.g.,
via plat_dat->resume) to restore it on wake, could this lead to a
silent network failure after system resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910-allwinner-a733-gmac-support-v2-0-4f4a1e520c23@baylibre.com?part=2