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

[Prasanna Kumar T S M <ptsm@linux.microsoft.com>]

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@vger.kernel.org
>> Signed-off-by: Prasanna Kumar T S M <ptsm@linux.microsoft.com>
>> ---
>>   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