Re: [PATCH net-next v7 4/5] net: stmmac: Add PCI driver support for BCM8958x

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

On Wed, Mar 18, 2026 at 11:12:23PM -0700, Jitendra Vegiraju wrote:
> Hi Russell,
> On Mon, Mar 16, 2026 at 1:34 PM Jitendra Vegiraju
> <jitendra.vegiraju@broadcom.com> wrote:
> >
> > On Fri, Mar 13, 2026 at 4:01 PM Russell King (Oracle)
> > <linux@armlinux.org.uk> wrote:
> > >
> > > > +
> > > > +     plat->suspend           = stmmac_pci_plat_suspend;
> > > > +     plat->resume            = brcm_pci_resume;
> > > > +     plat->bsp_priv = brcm_priv;
> > >
> > > Populating suspend/resume means that plat->init and plat->exit
> > > will only be called on driver probe (former), probe failure (latter)
> > > or remove (latter). Please consider using these to ensure that
> > > all appropriate resources are properly cleaned up in all cases.
> > >
> >
> > Thanks for pointing this out. I will check resource cleanup more closely.
> After reviewing the need for  plat->init and plat-exit, I don't think we need
> these handlers as this driver with fixed-link doesn't need to restore any device
> specific state such as clocks.

Huh?

plat->init and plat->exit have nothing to do with "restoring" anything.

plat->init is for platform specific initialisation.

plat->exit is for reversing the effects of plat->init once plat->init
has suceeded, and will be called should the probe fail or on device
removal.

So, where you have:

static int foo_probe()
{
	do init stuff();

	ret = stmmac_dvr_probe();
	if (ret)
		goto cleanup;

	return 0;

cleanup:
	do cleanup stuff();

	return ret;
}

static void foo_remove()
{
	stmmac_dvr_remove();
	do cleanup stuff();
}

Using ->init for "do init stuff()" and ->exit for "do cleanup stuff()"
will simplify the code, and actually make things more correct.

Currently, you have this in your remove path:

+       pci_free_irq_vectors(pdev);
+       device_set_node(&pdev->dev, NULL);
+       software_node_unregister_node_group(brcm_swnodes);

but in your probe error path, you have failure paths that leave
the swnode connected to the device, and you don't call
software_node_unregister_node_group(). Thus, it seems to me that
your cleanup path is buggy.

My suggestion of using ->init and ->exit means you have slightly
less to think about when stmmac_dvr_probe() fails - although if
you still have to do appropriate cleanup within ->init if it
partially fails.

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