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

[Jitendra Vegiraju]

On Thu, Mar 19, 2026 at 1:15 AM Russell King (Oracle)
<linux@xxxxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxx> wrote:
> > >
> > > On Fri, Mar 13, 2026 at 4:01 PM Russell King (Oracle)
> > > <linux@xxxxxxxxxxxxxxx> 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.
>
Sorry, I missed the resource leak with software node on probe error path.
Your suggestion to use make use of ->init, ->exit makes the code
look cleaner. I will make the changes in v8.
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature