Re: [PATCH 4/5] EDAC/versalnet: Fix device_register() error handling in init_one_mc()

[Prasanna Kumar T S M]

On 22-03-2026 21:40, Borislav Petkov wrote:
> On Sun, Mar 22, 2026 at 06:11:45AM -0700, Prasanna Kumar T S M wrote:
> > When device_register() fails, it must be followed by put_device()
> > rather than kfree(), because device_register() calls
> > device_initialize() which sets up the device refcount. The matching
> > release function versal_edac_release() handles the actual kfree().
> >
> > Also reorder the dev allocation to after edac_mc_alloc() so the error
> > path no longer needs a separate err_dev_free label.
>
> Nope.
>
> edac_mc_alloc() is a lot more heavy-weight than a simple k*alloc(). Pls keep
> the ordering as it is.

Re-ordering this simplifies the error handling path. See below for details.

> > Fixes: d5fe2fec6c40d ("EDAC: Add a driver for the AMD Versal NET DDR controller")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Prasanna Kumar T S M <ptsm@xxxxxxxxxxxxxxxxxxx>
> > ---
> >   drivers/edac/versalnet_edac.c | 16 ++++++++--------
> >   1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> > index acd51b492772..6463e88ed3d3 100644
> > --- a/drivers/edac/versalnet_edac.c
> > +++ b/drivers/edac/versalnet_edac.c
> > @@ -817,24 +817,26 @@ static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i
> >   	if (!name)
> >   		return rc;
> >   
> > -	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > -	if (!dev)
> > -		goto err_name_free;
> > -
> >   	mci = edac_mc_alloc(i, ARRAY_SIZE(layers), layers, sizeof(struct mc_priv));
> >   	if (!mci) {
> >   		edac_printk(KERN_ERR, EDAC_MC, "Failed memory allocation for MC%d\n", i);
> > -		goto err_dev_free;
> > +		goto err_name_free;
> >   	}
> >   
> > +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > +	if (!dev)
> > +		goto err_mc_free;
> > +
> >   	sprintf(name, "versal-net-ddrmc5-edac-%d", i);
> >   
> >   	dev->init_name = name;
> >   	dev->release = versal_edac_release;
> >   
> >   	rc = device_register(dev);
> > -	if (rc)
> > +	if (rc) {
> > +		put_device(dev);
>
> Why here and not at the error label below?

After calling device_register(), put_device() has to be used 
irrespective of the return value. In this case, kfree(dev) will happen 
as a part of put_device().

If device_register() was not called, kfree(dev) has to be called to free 
the memory.

If kzalloc(dev) is done after edac_mc_alloc(), there is no need to 
decide between kfree(dev) or put_device(dev). This simplifies the error 
handling path. This is the reason behind re-ordering and keeping 
put_device(dev) under 'if (rc) { ... }'.

> >   		goto err_mc_free;
> > +	}
> >   
> >   	mci->pdev = dev;
> >   	mc_init(mci, dev);
> > @@ -856,8 +858,6 @@ static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i
> >   	device_unregister(mci->pdev);
> >   err_mc_free:
> >   	edac_mc_free(mci);
> > -err_dev_free:
> > -	kfree(dev);
> >   err_name_free:
> >   	kfree(name);
> >   
> > --
> > 2.49.0

Do you think it is best to do edac_mc_alloc() before kzalloc(dev)?

Thanks,
Prasanna Kumar