Re: [net-next,v2,06/22] net: stmmac: rk: add SoC specific ->init() method

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Jan 27, 2026 at 12:51:45AM +0000, Russell King (Oracle) wrote:
> On Mon, Jan 26, 2026 at 04:40:20PM -0800, Jakub Kicinski wrote:
> > 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: stmmac: rk: add SoC specific ->init() method
> > 
> > This commit adds infrastructure for SoC-specific initialization by adding
> > an init callback to rk_gmac_ops and calling it from rk_gmac_setup().
> > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > > index 7f8ffd1549bc..4117f6863ff0 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > 
> > [ ... ]
> > 
> > > @@ -1618,6 +1619,14 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
> > >
> > >  	bsp_priv->dev = dev;
> > >
> > > +	if (ops->init) {
> > > +		ret = ops->init(bsp_priv);
> > > +		if (ret) {
> > > +			dev_err_probe(dev, ret, "failed to init BSP\n");
> > > +			return ERR_PTR(ret);
> >                         ^^^^^^^^^^^^^^^
> > 
> > If ops->init() fails here, does bsp_priv->phy_reset need to be released?
> > 
> > Earlier in rk_gmac_setup(), at line 1610, the code calls:
> > 
> >     bsp_priv->phy_reset = of_reset_control_get(plat->phy_node, NULL);
> > 
> > of_reset_control_get() is not a devm-managed function, so the reset control
> > would need to be explicitly released with reset_control_put() on error paths.
> > 
> > This new error return happens after phy_reset could have been acquired (when
> > the device has an integrated PHY), but before the function successfully
> > returns. If this path is taken, it appears the reset control would be leaked.
> > 
> > Currently no SoC sets ops->init so this path cannot trigger, but when a
> > future SoC implements this callback, the leak would occur on init failure
> > for devices with integrated PHYs.
> 
> However, the driver does not release this resource when cleaning up, so
> that's already a bug as the driver currently stands. I think this could
> be converted to devm_reset_control_get(), which would resolve both
> leakages, but not sure.

Note that fixing this is going to add yet another patch to the series,
because this is a pre-existing bug in the driver. It can't be replaced
with devm_reset_control_get(), because this driver is getting resources
for a foreign device (we don't have the struct device pointer.)

So, it isn't going to be a simple patch to fix this.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!