Re: [net-next,v2,2/8] net: macb: rename macb_default_usrio to at91_default_usrio as not all platforms have mii mode control in usrio

From: Jakub Kicinski <kuba@kernel.org>
To: conor@kernel.org
Cc: Jakub Kicinski <kuba@kernel.org>,
	andrew+netdev@lunn.ch, richardcochran@gmail.com,
	abin.joseph@amd.com, robh@kernel.org, edumazet@google.com,
	netdev@vger.kernel.org, theo.lebrun@bootlin.com, pjw@kernel.org,
	Valentina.FernandezAlanis@microchip.com, krzk+dt@kernel.org,
	sean.anderson@linux.dev, aou@eecs.berkeley.edu,
	linux-kernel@vger.kernel.org, alex@ghiti.fr,
	devicetree@vger.kernel.org, palmer@dabbelt.com,
	nicolas.ferre@microchip.com, vineeth.karumanchi@amd.com,
	claudiu.beznea@tuxon.dev, samuel.holland@sifive.com,
	daire.mcnamara@microchip.com, conor+dt@kernel.org,
	dave.stevenson@raspberrypi.com, linux-riscv@lists.infradead.org,
	davem@davemloft.net, conor.dooley@microchip.com,
	narmstrong@baylibre.com, pabeni@redhat.com
Subject: Re: [net-next,v2,2/8] net: macb: rename macb_default_usrio to at91_default_usrio as not all platforms have mii mode control in usrio
Date: Sat, 28 Feb 2026 15:26:00 -0800	[thread overview]
Message-ID: <20260228232600.4187398-1-kuba@kernel.org> (raw)
In-Reply-To: <20260226-enjoyer-shock-e17f9dc7cbdb@spud>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: macb: rename macb_default_usrio to at91_default_usrio as not all
platforms have mii mode control in usrio

This patch renames macb_default_usrio to at91_default_usrio to better
reflect that the usrio configuration is platform-specific, and
introduces a new capability flag (MACB_CAPS_USRIO_HAS_MII) to gate MII
mode configuration code for platforms that actually support it.

> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -779,6 +779,7 @@ #define MACB_CAPS_DMA_64B			BIT(21)
>  #define MACB_CAPS_DMA_PTP			BIT(22)
>  #define MACB_CAPS_RSC				BIT(23)
>  #define MACB_CAPS_NO_LSO			BIT(24)
> +#define MACB_CAPS_USRIO_HAS_MII			BIT(25)
>
>  /* LSO settings */
>  #define MACB_LSO_UFO_ENABLE			0x01
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -4614,13 +4614,15 @@
>
>  	if (!(bp->caps & MACB_CAPS_USRIO_DISABLED)) {
>  		val = 0;
> -		if (phy_interface_mode_is_rgmii(bp->phy_interface))
> -			val = bp->usrio->rgmii;
> -		else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
> -			 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> -			val = bp->usrio->rmii;
> -		else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> -			val = bp->usrio->mii;
> +		if (bp->caps & MACB_CAPS_USRIO_HAS_MII) {
> +			if (phy_interface_mode_is_rgmii(bp->phy_interface))
> +				val = bp->usrio->rgmii;
> +			else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
> +				 (bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> +				val = bp->usrio->rmii;
> +			else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII))
> +				val = bp->usrio->mii;
> +		}
>
>  		if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
>  			val |= bp->usrio->refclk;

Can bp->usrio be NULL here? The new MACB_CAPS_USRIO_HAS_MII capability
gates the MII-related dereferences of bp->usrio, but bp->usrio is also
dereferenced when MACB_CAPS_USRIO_HAS_CLKEN is set.

With the change to default_gem_config later in this patch:

static const struct macb_config default_gem_config = {
	...
	.usrio = NULL,
	...
};

A device that matches the generic 'cdns,macb' compatible string (without
specific .data) will get bp->usrio = NULL during probe. If the device
tree then has 'cdns,refclk-ext', macb_configure_caps() adds
MACB_CAPS_USRIO_HAS_CLKEN without checking if bp->usrio is valid:

macb_configure_caps() {
	...
	if (of_property_read_bool(np, "cdns,refclk-ext"))
		bp->caps |= MACB_CAPS_USRIO_HAS_CLKEN;
	...
}

Then in macb_init(), the code would dereference bp->usrio->refclk with
bp->usrio being NULL, causing a crash.

The MACB_CAPS_USRIO_HAS_MII gate only protects the MII-related
dereferences above, not the CLKEN dereference.

> @@ -4638,13 +4640,6 @@
>  	return 0;
>  }
>
> -static const struct macb_usrio_config macb_default_usrio = {
> -	.mii = MACB_BIT(MII),
> -	.rmii = MACB_BIT(RMII),
> -	.rgmii = GEM_BIT(RGMII),
> -	.refclk = MACB_BIT(CLKEN),
> -};
> -

[ ... ]

> @@ -5219,6 +5214,13 @@ static int eyeq5_init(struct platform_device *pdev)
>  	return ret;
>  }
>
> +static const struct macb_usrio_config at91_default_usrio = {
> +	.mii = MACB_BIT(MII),
> +	.rmii = MACB_BIT(RMII),
> +	.rgmii = GEM_BIT(RGMII),
> +	.refclk = MACB_BIT(CLKEN),
> +};
> +

[ ... ]

> @@ -5432,7 +5448,7 @@ MODULE_DEVICE_TABLE(of, macb_dt_ids);
>  	.dma_burst_length = 16,
>  	.clk_init = macb_clk_init,
>  	.init = macb_init,
> -	.usrio = &macb_default_usrio,
> +	.usrio = NULL,
>  	.jumbo_max_len = 10240,
>  };
>
-- 
pw-bot: cr

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv