[PATCH 0/9] EDAC/versalnet: Fix error handling, teardown, and robustness

[PATCH 0/9] EDAC/versalnet: Fix error handling, teardown, and robustness

[Shubhrajyoti Datta]

    This series addresses several correctness and robustness issues in
    the VersalNet EDAC driver

    The main fixes are:

    * Robustness fixes
      - Add NULL checks for mci in handle_error() and remove_one_mc()
        to prevent NULL pointer dereferences when a controller has not
        been fully initialized.
      - Add bounds validation in rpmsg_cb().

    * Teardown improvements
      - Fix teardown ordering in mc_remove() and remove_one_mc() to
        prevent use-after-free conditions.

    * Probe and initialization cleanup
      - Move platform_set_drvdata() from init_one_mc() to mc_probe().
      - Reorder probe flow to initialize MCDI before RPMsg registration.
      - Remove unused pdev parameters from internal initialization
        functions.

    * Resource management fixes
      - Fix error handling in init_one_mc() to avoid a potential
        double-free by reordering device_register() and edac_mc_alloc().
      - Replace sprintf() with init_name by dev_set_name() to avoid
        use-after-free issues caused by deferred kobject cleanup.

    * Code cleanup
      - Use a designated initializer for rpmsg_channel_info.



Prasanna Kumar T S M (1):
  EDAC/versalnet: Fix device_register() error handling in init_one_mc()

Shubhrajyoti Datta (8):
  EDAC/versalnet: Add NULL check for mci in handle_error()
  EDAC/versalnet: Add NULL check for mci in remove_one_mc()
  EDAC/versalnet: Move platform_set_drvdata() to mc_probe()
  EDAC/versalnet: Use dev_set_name() instead of sprintf with init_name
  EDAC/versalnet: Initialize MCDI before RPMsg registration
  EDAC/versalnet: Add bounds validation in rpmsg_cb()
  EDAC/versalnet: Fix use-after-free in remove_one_mc()
  EDAC/versalnet: Use designated initializer for rpmsg_channel_info

 drivers/edac/versalnet_edac.c | 95 +++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 44 deletions(-)

-- 
2.34.1

[PATCH 1/9] EDAC/versalnet: Add NULL check for mci in handle_error()

[Shubhrajyoti Datta]

Add a NULL pointer check for mci before use in handle_error() to
prevent a potential NULL dereference when the memory controller
instance is not initialized for a given controller number.

This is added as a defensive check. In our current firmware
the event will not be generated.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index d1af5e175f7e..316f8f79c4d8 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -439,6 +439,8 @@ static void handle_error(struct mc_priv  *priv, struct ecc_status *stat,
 		return;
 
 	mci = priv->mci[ctl_num];
+	if (!mci)
+		return;
 
 	if (stat->error_type == MC5_ERR_TYPE_CE) {
 		pinf = stat->ceinfo[stat->channel];
-- 
2.34.1

Re: [PATCH 1/9] EDAC/versalnet: Add NULL check for mci in handle_error()

[Borislav Petkov]

On Fri, Jul 24, 2026 at 10:49:37PM +0530, Shubhrajyoti Datta wrote:
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index d1af5e175f7e..316f8f79c4d8 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -439,6 +439,8 @@ static void handle_error(struct mc_priv  *priv, struct ecc_status *stat,
>  		return;
>  
>  	mci = priv->mci[ctl_num];
> +	if (!mci)
> +		return;

You have a WARN_ON_ONCE right before that line which checks against
NUM_CONTROLLERS and init_versalnet() unwinds all the setup the moment
init_one_mc() fails for one of the MCs.

So why are we adding dead code?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/9] EDAC/versalnet: Add NULL check for mci in handle_error()

[Pandey, Radhey Shyam]

On 7/27/2026 5:22 AM, Borislav Petkov wrote:
> On Fri, Jul 24, 2026 at 10:49:37PM +0530, Shubhrajyoti Datta wrote:
>> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
>> index d1af5e175f7e..316f8f79c4d8 100644
>> --- a/drivers/edac/versalnet_edac.c
>> +++ b/drivers/edac/versalnet_edac.c
>> @@ -439,6 +439,8 @@ static void handle_error(struct mc_priv  *priv, struct ecc_status *stat,
>>   		return;
>>   
>>   	mci = priv->mci[ctl_num];
>> +	if (!mci)
>> +		return;
> 
> You have a WARN_ON_ONCE right before that line which checks against
> NUM_CONTROLLERS and init_versalnet() unwinds all the setup the moment
> init_one_mc() fails for one of the MCs.

The gap in the driver is that init_one_mc() returns success without
setting priv->mci[i] when the bus width decodes to DEV_UNKNOWN, while
buggy firmware / future firmware rpmsg error path can still calls
handle_error().

Happy to drop the check if you prefer otherwise we can keep it as a
cheap guard for a firmware/driver mismatch.

Shubrajyoti: Please feel free to add/correct. I'm still ramping up on
this driver and may have overlooked details.

> 
> So why are we adding dead code?
> 

Re: [PATCH 1/9] EDAC/versalnet: Add NULL check for mci in handle_error()

[Borislav Petkov]

On Mon, Jul 27, 2026 at 12:18:48PM +0530, Pandey, Radhey Shyam wrote:
> The gap in the driver is that init_one_mc() returns success without
> setting priv->mci[i] when the bus width decodes to DEV_UNKNOWN, while
> buggy firmware / future firmware rpmsg error path can still calls
> handle_error().

Aha, right, so I missed that. So the right fix is to always return an error if
the function fails. So that the proper unwinding in init_versalnet() works.

> Happy to drop the check if you prefer otherwise we can keep it as a
> cheap guard for a firmware/driver mismatch.

You already have that at the beginning of handle_error().

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/9] EDAC/versalnet: Add NULL check for mci in handle_error()

[Pandey, Radhey Shyam]

On 7/28/2026 7:07 AM, Borislav Petkov wrote:
> On Mon, Jul 27, 2026 at 12:18:48PM +0530, Pandey, Radhey Shyam wrote:
>> The gap in the driver is that init_one_mc() returns success without
>> setting priv->mci[i] when the bus width decodes to DEV_UNKNOWN, while
>> buggy firmware / future firmware rpmsg error path can still calls
>> handle_error().
> 
> Aha, right, so I missed that. So the right fix is to always return an error if
> the function fails. So that the proper unwinding in init_versalnet() works.

Thanks Boris.
One clarification on DEV_UNKNOWN: on Versal NET, the driver exposes 8
MC5 controller slots, but a given platform may not have all of them
configured. The existing path treats unrecognized bus width as a skip
(continue), and the refactor kept that as return 0 without setting
priv->mci[i]. For that case, returning -EINVAL would fail probe on
designs with fewer than 8 active controllers. Actual init failures
(alloc, device_add, edac_mc_add_mc) already return an error and
init_versalnet() unwinds as intended.

The NULL guards in handle_error() cover the skip path where priv->mci[i]
is intentionally unset. It can also be a check in rpmsg_cb() before
calling handle_error().

Thanks,
Radhey


> 
>> Happy to drop the check if you prefer otherwise we can keep it as a
>> cheap guard for a firmware/driver mismatch.
> 
> You already have that at the beginning of handle_error().
> 
> Thx.
> 

Re: [PATCH 1/9] EDAC/versalnet: Add NULL check for mci in handle_error()

[Borislav Petkov]

On Tue, Jul 28, 2026 at 11:57:28PM +0530, Pandey, Radhey Shyam wrote:
> One clarification on DEV_UNKNOWN: on Versal NET, the driver exposes 8
> MC5 controller slots, but a given platform may not have all of them
> configured.

Are you basically saying that you can have non-contiguous controller slots
present on a system?

If so, the whole unwinding path needs to be reworked so that it doesn't remove
the properly configured ones but simply probes each one, if it finds one, it
initializes it and if it doesn't, simply continues.

Which then begs the question should that driver even load if one of the
instances fail probing?

We don't allow that in amd64_edac, see:

        for (i = 0; i < amd_nb_num(); i++) {
                err = probe_one_instance(i);
                if (err) {
                        /* unwind properly */
                        while (--i >= 0)
                                remove_one_instance(i);

                        goto err_pci;
                }
        }

But I'm willing to be persuaded that it can make sense for your driver.

Whatever it is, the code needs to be made to handle that configuration
properly and that needs to be stated somewhere prominently why is it done so.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/9] EDAC/versalnet: Add NULL check for mci in handle_error()

[Pandey, Radhey Shyam]

On 7/29/2026 3:03 AM, Borislav Petkov wrote:
> On Tue, Jul 28, 2026 at 11:57:28PM +0530, Pandey, Radhey Shyam wrote:
>> One clarification on DEV_UNKNOWN: on Versal NET, the driver exposes 8
>> MC5 controller slots, but a given platform may not have all of them
>> configured.
> 
> Are you basically saying that you can have non-contiguous controller slots
> present on a system?
> 

In the Versal NET designs I've examined so far, configuration with fewer
than eight MC5 controllers uses a contiguous set starting at slot 0
(for example, slots 0-3 populated and slots 4-7 absent).

I will let Shubhrajyoti comment if non-contiguous controller slots
are possible on any supported platform?

Agreed for real init failures if initialization of a controller that
should be present fails, probe should fail and unwind all instances
registered so far, same as amd64_edac.

For v2, my proposal is (only if contiguous slot are supported):
-Take the supported controller count from the design/DT rather than
always iterating hardcoded NUM_CONTROLLERS(8). Though it has dependency
on XSA support(hardware description metadata archive exported from
Vivado) and DT binding getting accepted.
-Run init/remove only for that range.
-Treat DEV_UNKNOWN as a probe error with full unwind, not as a silent
skip. Indices beyond the supported count are not probed at all.
-Document this model in the driver.
-The existing WARN_ON_ONCE() check in handle_error() should be
sufficient so the current patch can be dropped.

Shubhrajyoti: please chime in if I've missed anything or if you
have a different view on this approach.

Thanks,
Radhey

Re: [PATCH 1/9] EDAC/versalnet: Add NULL check for mci in handle_error()

[Shubhrajyoti Datta]

On Wed, Jul 29, 2026 at 10:10 PM Pandey, Radhey Shyam <radheys@amd.com> wrote:
>
> On 7/29/2026 3:03 AM, Borislav Petkov wrote:
> > On Tue, Jul 28, 2026 at 11:57:28PM +0530, Pandey, Radhey Shyam wrote:
> >> One clarification on DEV_UNKNOWN: on Versal NET, the driver exposes 8
> >> MC5 controller slots, but a given platform may not have all of them
> >> configured.
> >
> > Are you basically saying that you can have non-contiguous controller slots
> > present on a system?
> >
>
> In the Versal NET designs I've examined so far, configuration with fewer
> than eight MC5 controllers uses a contiguous set starting at slot 0
> (for example, slots 0-3 populated and slots 4-7 absent).
>
> I will let Shubhrajyoti comment if non-contiguous controller slots
> are possible on any supported platform?

The controllers cannot be non-contiguous.
>
> Agreed for real init failures if initialization of a controller that
> should be present fails, probe should fail and unwind all instances
> registered so far, same as amd64_edac.
>
> For v2, my proposal is (only if contiguous slot are supported):
> -Take the supported controller count from the design/DT rather than
> always iterating hardcoded NUM_CONTROLLERS(8). Though it has dependency
> on XSA support(hardware description metadata archive exported from
> Vivado) and DT binding getting accepted.
> -Run init/remove only for that range.
> -Treat DEV_UNKNOWN as a probe error with full unwind, not as a silent
> skip. Indices beyond the supported count are not probed at all.
> -Document this model in the driver.
> -The existing WARN_ON_ONCE() check in handle_error() should be
> sufficient so the current patch can be dropped.
>
> Shubhrajyoti: please chime in if I've missed anything or if you
> have a different view on this approach.
I agree we can have the controllers in the device-tree and this patch can be
dropped.

>
> Thanks,
> Radhey

[PATCH 2/9] EDAC/versalnet: Add NULL check for mci in remove_one_mc()

[Shubhrajyoti Datta]

If a controller's configuration specifies an unrecognized bus width,
init_one_mc() returns 0 but skips allocating priv->mci[ctl_num], leaving
it NULL. On module unload or probe failure, remove_one_mc() is called for
all controllers and unconditionally dereferences priv->mci[i], causing a
kernel panic.

Add a NULL pointer check for mci before dereferencing it.

 Unable to handle kernel NULL pointer dereference at virtual address 0000000000000390
 Internal error: Oops: 0000000096000004 [#1] SMP
 Hardware name: Xilinx Versal NET VNX (DT)
 pc : mc_remove+0x34/0x88
 lr : mc_remove+0x4c/0x88
 Call trace:
  mc_remove+0x34/0x88
  platform_remove+0x2c/0x70
  device_remove+0x48/0x7c
  device_release_driver_internal+0x1c8/0x224
  device_driver_detach+0x18/0x28
  unbind_store+0xb4/0xb8

Fixes: 62a9fc50e8d9 ("EDAC/versalnet: Refactor memory controller initialization and cleanup")
Cc: stable@vger.kernel.org
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 316f8f79c4d8..05dc34504cc2 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -769,6 +769,9 @@ static void remove_one_mc(struct mc_priv *priv, int i)
 	struct mem_ctl_info *mci;
 
 	mci = priv->mci[i];
+	if (!mci)
+		return;
+
 	device_unregister(mci->pdev);
 	edac_mc_del_mc(mci->pdev);
 	edac_mc_free(mci);
-- 
2.34.1

Re: [PATCH 2/9] EDAC/versalnet: Add NULL check for mci in remove_one_mc()

[Pandey, Radhey Shyam]

On 7/24/2026 10:49 PM, Shubhrajyoti Datta wrote:
> If a controller's configuration specifies an unrecognized bus width,
> init_one_mc() returns 0 but skips allocating priv->mci[ctl_num], leaving
> it NULL. On module unload or probe failure, remove_one_mc() is called for
> all controllers and unconditionally dereferences priv->mci[i], causing a
> kernel panic.
> 
> Add a NULL pointer check for mci before dereferencing it.
> 
>   Unable to handle kernel NULL pointer dereference at virtual address 0000000000000390
>   Internal error: Oops: 0000000096000004 [#1] SMP
>   Hardware name: Xilinx Versal NET VNX (DT)
>   pc : mc_remove+0x34/0x88
>   lr : mc_remove+0x4c/0x88
>   Call trace:
>    mc_remove+0x34/0x88
>    platform_remove+0x2c/0x70
>    device_remove+0x48/0x7c
>    device_release_driver_internal+0x1c8/0x224
>    device_driver_detach+0x18/0x28
>    unbind_store+0xb4/0xb8
> 
> Fixes: 62a9fc50e8d9 ("EDAC/versalnet: Refactor memory controller initialization and cleanup")

Confirm on fixes tag. The bug likely goes back to the original driver.
> Cc: stable@vger.kernel.org

There is no CC to stable kernel in email.

Below changes looks fine to me. With above tag fixed.

Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Thanks!

> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> ---
> 
>   drivers/edac/versalnet_edac.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index 316f8f79c4d8..05dc34504cc2 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -769,6 +769,9 @@ static void remove_one_mc(struct mc_priv *priv, int i)
>   	struct mem_ctl_info *mci;
>   
>   	mci = priv->mci[i];
> +	if (!mci)
> +		return;
> +
>   	device_unregister(mci->pdev);
>   	edac_mc_del_mc(mci->pdev);
>   	edac_mc_free(mci);

[PATCH 3/9] EDAC/versalnet: Move platform_set_drvdata() to mc_probe()

[Shubhrajyoti Datta]

Move platform_set_drvdata() out of init_one_mc() and into mc_probe()
so that the driver data is set once during probe rather than being
redundantly set on each memory controller initialization.

The pdev parameter in init_one_mc() and init_versalnet() is no longer
referenced. Remove it from both function signatures.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 05dc34504cc2..03b6e0958f17 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -777,7 +777,7 @@ static void remove_one_mc(struct mc_priv *priv, int i)
 	edac_mc_free(mci);
 }
 
-static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i)
+static int init_one_mc(struct mc_priv *priv, int i)
 {
 	u32 num_chans, rank, dwidth, config;
 	struct edac_mc_layer layers[2];
@@ -849,8 +849,6 @@ static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i
 	priv->mci[i] = mci;
 	priv->dwidth = dt;
 
-	platform_set_drvdata(pdev, priv);
-
 	return 0;
 
 err_unreg:
@@ -863,12 +861,12 @@ static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i
 	return rc;
 }
 
-static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
+static int init_versalnet(struct mc_priv *priv)
 {
 	int rc, i;
 
 	for (i = 0; i < NUM_CONTROLLERS; i++) {
-		rc = init_one_mc(priv, pdev, i);
+		rc = init_one_mc(priv, i);
 		if (rc) {
 			while (i--)
 				remove_one_mc(priv, i);
@@ -914,6 +912,7 @@ static int mc_probe(struct platform_device *pdev)
 		goto err_alloc;
 	}
 
+	platform_set_drvdata(pdev, priv);
 	amd_rpmsg_id_table[0].driver_data = (kernel_ulong_t)priv;
 
 	rc = register_rpmsg_driver(&amd_rpmsg_driver);
@@ -928,7 +927,7 @@ static int mc_probe(struct platform_device *pdev)
 
 	priv->mcdi->r5_rproc = rp;
 
-	rc = init_versalnet(priv, pdev);
+	rc = init_versalnet(priv);
 	if (rc)
 		goto err_init;
 
-- 
2.34.1

Re: [PATCH 3/9] EDAC/versalnet: Move platform_set_drvdata() to mc_probe()

[Pandey, Radhey Shyam]

On 7/24/2026 10:49 PM, Shubhrajyoti Datta wrote:
> Move platform_set_drvdata() out of init_one_mc() and into mc_probe()
> so that the driver data is set once during probe rather than being
> redundantly set on each memory controller initialization.
> 
> The pdev parameter in init_one_mc() and init_versalnet() is no longer
> referenced. Remove it from both function signatures.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>

Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Thanks!
> ---
> 
>   drivers/edac/versalnet_edac.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index 05dc34504cc2..03b6e0958f17 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -777,7 +777,7 @@ static void remove_one_mc(struct mc_priv *priv, int i)
>   	edac_mc_free(mci);
>   }
>   
> -static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i)
> +static int init_one_mc(struct mc_priv *priv, int i)
>   {
>   	u32 num_chans, rank, dwidth, config;
>   	struct edac_mc_layer layers[2];
> @@ -849,8 +849,6 @@ static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i
>   	priv->mci[i] = mci;
>   	priv->dwidth = dt;
>   
> -	platform_set_drvdata(pdev, priv);
> -
>   	return 0;
>   
>   err_unreg:
> @@ -863,12 +861,12 @@ static int init_one_mc(struct mc_priv *priv, struct platform_device *pdev, int i
>   	return rc;
>   }
>   
> -static int init_versalnet(struct mc_priv *priv, struct platform_device *pdev)
> +static int init_versalnet(struct mc_priv *priv)
>   {
>   	int rc, i;
>   
>   	for (i = 0; i < NUM_CONTROLLERS; i++) {
> -		rc = init_one_mc(priv, pdev, i);
> +		rc = init_one_mc(priv, i);
>   		if (rc) {
>   			while (i--)
>   				remove_one_mc(priv, i);
> @@ -914,6 +912,7 @@ static int mc_probe(struct platform_device *pdev)
>   		goto err_alloc;
>   	}
>   
> +	platform_set_drvdata(pdev, priv);
>   	amd_rpmsg_id_table[0].driver_data = (kernel_ulong_t)priv;
>   
>   	rc = register_rpmsg_driver(&amd_rpmsg_driver);
> @@ -928,7 +927,7 @@ static int mc_probe(struct platform_device *pdev)
>   
>   	priv->mcdi->r5_rproc = rp;
>   
> -	rc = init_versalnet(priv, pdev);
> +	rc = init_versalnet(priv);
>   	if (rc)
>   		goto err_init;
>   

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

[Shubhrajyoti Datta]

From: Prasanna Kumar T S M <ptsm@linux.microsoft.com>

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().

To simplify error handling and avoid complex unwinding, split
device_register() into device_initialize() and device_add().
Initialize the device early so put_device() can be used in all
error paths.

Fixes: d5fe2fec6c40 ("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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 03b6e0958f17..3c9eaea5a106 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -785,7 +785,7 @@ static int init_one_mc(struct mc_priv *priv, int i)
 	char name[MC_NAME_LEN];
 	struct device *dev;
 	enum dev_type dt;
-	int rc;
+	int rc = -ENOMEM;
 
 	config = priv->adec[CONF + i * ADEC_NUM];
 	num_chans = FIELD_GET(MC5_NUM_CHANS_MASK, config);
@@ -817,23 +817,23 @@ static int init_one_mc(struct mc_priv *priv, int i)
 	layers[1].size = num_chans;
 	layers[1].is_virt_csrow = false;
 
-	rc = -ENOMEM;
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev)
 		return rc;
 
-	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;
-	}
-
 	sprintf(name, "versal-net-ddrmc5-edac-%d", i);
 
 	dev->init_name = name;
 	dev->release = versal_edac_release;
+	device_initialize(dev);
 
-	rc = device_register(dev);
+	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_put_dev;
+	}
+
+	rc = device_add(dev);
 	if (rc)
 		goto err_mc_free;
 
@@ -843,7 +843,7 @@ static int init_one_mc(struct mc_priv *priv, int i)
 	rc = edac_mc_add_mc(mci);
 	if (rc) {
 		edac_printk(KERN_ERR, EDAC_MC, "Failed to register MC%d with EDAC core\n", i);
-		goto err_unreg;
+		goto err_dev_del;
 	}
 
 	priv->mci[i] = mci;
@@ -851,12 +851,12 @@ static int init_one_mc(struct mc_priv *priv, int i)
 
 	return 0;
 
-err_unreg:
-	device_unregister(mci->pdev);
+err_dev_del:
+	device_del(dev);
 err_mc_free:
 	edac_mc_free(mci);
-err_dev_free:
-	kfree(dev);
+err_put_dev:
+	put_device(dev);
 
 	return rc;
 }
-- 
2.34.1

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

[Pandey, Radhey Shyam]

On 7/24/2026 10:49 PM, Shubhrajyoti Datta wrote:
> From: Prasanna Kumar T S M <ptsm@linux.microsoft.com>
> 
> 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().
> 
> To simplify error handling and avoid complex unwinding, split
> device_register() into device_initialize() and device_add().
> Initialize the device early so put_device() can be used in all
> error paths.
> 
> Fixes: d5fe2fec6c40 ("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>
> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
checkpatch reports - warn.

WARNING: Non-standard signature: Co-authored-by:
#20:
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>

Nit - this is Co-developed-by: candidate as you did changes on top.

> ---
> 
>   drivers/edac/versalnet_edac.c | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index 03b6e0958f17..3c9eaea5a106 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -785,7 +785,7 @@ static int init_one_mc(struct mc_priv *priv, int i)
>   	char name[MC_NAME_LEN];
>   	struct device *dev;
>   	enum dev_type dt;
> -	int rc;
> +	int rc = -ENOMEM;
>   
>   	config = priv->adec[CONF + i * ADEC_NUM];
>   	num_chans = FIELD_GET(MC5_NUM_CHANS_MASK, config);
> @@ -817,23 +817,23 @@ static int init_one_mc(struct mc_priv *priv, int i)
>   	layers[1].size = num_chans;
>   	layers[1].is_virt_csrow = false;
>   
> -	rc = -ENOMEM;
>   	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>   	if (!dev)
>   		return rc;
>   
> -	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;
> -	}
> -
>   	sprintf(name, "versal-net-ddrmc5-edac-%d", i);
>   
>   	dev->init_name = name;
>   	dev->release = versal_edac_release;
> +	device_initialize(dev);
>   

There was a comment earlier from sashiko: After splitting
device_register(), the edac_mc_alloc() failure path calls
put_device() with dev->init_name still pointing at a stack
buffer before device_add() copies it. That's unsafe in
principle (dev_name() would follow init_name) and worse with
CONFIG_DEBUG_KOBJECT_RELEASE deferral.


> -	rc = device_register(dev);
> +	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_put_dev;
> +	}
> +
> +	rc = device_add(dev);
>   	if (rc)
>   		goto err_mc_free;
>   
> @@ -843,7 +843,7 @@ static int init_one_mc(struct mc_priv *priv, int i)
>   	rc = edac_mc_add_mc(mci);
>   	if (rc) {
>   		edac_printk(KERN_ERR, EDAC_MC, "Failed to register MC%d with EDAC core\n", i);
> -		goto err_unreg;
> +		goto err_dev_del;
>   	}
>   
>   	priv->mci[i] = mci;
> @@ -851,12 +851,12 @@ static int init_one_mc(struct mc_priv *priv, int i)
>   
>   	return 0;
>   
> -err_unreg:
> -	device_unregister(mci->pdev);
> +err_dev_del:
> +	device_del(dev);
>   err_mc_free:
>   	edac_mc_free(mci);
> -err_dev_free:
> -	kfree(dev);
> +err_put_dev:
> +	put_device(dev);
>   
>   	return rc;
>   }

[PATCH 5/9] EDAC/versalnet: Use dev_set_name() instead of sprintf with init_name

[Shubhrajyoti Datta]

The previous code used sprintf() to format a device name into a local
stack buffer and then assigned it to dev->init_name. Since kobject
cleanup can be deferred asynchronously (e.g. when
CONFIG_DEBUG_KOBJECT_RELEASE is enabled), dev_name(dev) could be
accessed after init_one_mc() returns and the stack frame containing the
name buffer is gone, resulting in a use-after-free.

This is fixed by switching to dev_set_name(), which dynamically
allocates and manages the name string internally, but the now-unused
local char name[MC_NAME_LEN] buffer and the MC_NAME_LEN macro are
not needed so remove them.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 3c9eaea5a106..1caaba653fc0 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -70,8 +70,6 @@
 #define XDDR5_BUS_WIDTH_32		1
 #define XDDR5_BUS_WIDTH_16		2
 
-#define MC_NAME_LEN			32
-
 /**
  * struct ecc_error_info - ECC error log information.
  * @burstpos:		Burst position.
@@ -782,7 +780,6 @@ static int init_one_mc(struct mc_priv *priv, int i)
 	u32 num_chans, rank, dwidth, config;
 	struct edac_mc_layer layers[2];
 	struct mem_ctl_info *mci;
-	char name[MC_NAME_LEN];
 	struct device *dev;
 	enum dev_type dt;
 	int rc = -ENOMEM;
@@ -821,9 +818,7 @@ static int init_one_mc(struct mc_priv *priv, int i)
 	if (!dev)
 		return rc;
 
-	sprintf(name, "versal-net-ddrmc5-edac-%d", i);
 
-	dev->init_name = name;
 	dev->release = versal_edac_release;
 	device_initialize(dev);
 
@@ -833,6 +828,10 @@ static int init_one_mc(struct mc_priv *priv, int i)
 		goto err_put_dev;
 	}
 
+	rc = dev_set_name(dev, "versal-net-ddrmc5-edac-%d", i);
+	if (rc)
+		goto err_mc_free;
+
 	rc = device_add(dev);
 	if (rc)
 		goto err_mc_free;
-- 
2.34.1

Re: [PATCH 5/9] EDAC/versalnet: Use dev_set_name() instead of sprintf with init_name

[Pandey, Radhey Shyam]

On 7/24/2026 10:49 PM, Shubhrajyoti Datta wrote:
> The previous code used sprintf() to format a device name into a local
> stack buffer and then assigned it to dev->init_name. Since kobject
> cleanup can be deferred asynchronously (e.g. when
> CONFIG_DEBUG_KOBJECT_RELEASE is enabled), dev_name(dev) could be
> accessed after init_one_mc() returns and the stack frame containing the
> name buffer is gone, resulting in a use-after-free.
> 
> This is fixed by switching to dev_set_name(), which dynamically
> allocates and manages the name string internally, but the now-unused
> local char name[MC_NAME_LEN] buffer and the MC_NAME_LEN macro are
> not needed so remove them.

If previous commit introduced regression then this has to be merged
to previous commit. Each patch should be correct on its own.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> ---
> 
>   drivers/edac/versalnet_edac.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index 3c9eaea5a106..1caaba653fc0 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -70,8 +70,6 @@
>   #define XDDR5_BUS_WIDTH_32		1
>   #define XDDR5_BUS_WIDTH_16		2
>   
> -#define MC_NAME_LEN			32
> -
>   /**
>    * struct ecc_error_info - ECC error log information.
>    * @burstpos:		Burst position.
> @@ -782,7 +780,6 @@ static int init_one_mc(struct mc_priv *priv, int i)
>   	u32 num_chans, rank, dwidth, config;
>   	struct edac_mc_layer layers[2];
>   	struct mem_ctl_info *mci;
> -	char name[MC_NAME_LEN];
>   	struct device *dev;
>   	enum dev_type dt;
>   	int rc = -ENOMEM;
> @@ -821,9 +818,7 @@ static int init_one_mc(struct mc_priv *priv, int i)
>   	if (!dev)
>   		return rc;
>   
> -	sprintf(name, "versal-net-ddrmc5-edac-%d", i);
>   
> -	dev->init_name = name;
>   	dev->release = versal_edac_release;
>   	device_initialize(dev);
>   
> @@ -833,6 +828,10 @@ static int init_one_mc(struct mc_priv *priv, int i)
>   		goto err_put_dev;
>   	}
>   
> +	rc = dev_set_name(dev, "versal-net-ddrmc5-edac-%d", i);
> +	if (rc)
> +		goto err_mc_free;
> +
>   	rc = device_add(dev);
>   	if (rc)
>   		goto err_mc_free;

[PATCH 6/9] EDAC/versalnet: Initialize MCDI before RPMsg registration

[Shubhrajyoti Datta]

setup_mcdi() currently allocates the MCDI instance, assigns the RPMsg
endpoint, and retrieves DDR configuration data. The DDR configuration
path invokes cdx_mcdi_rpc(), which requires a functional RPMsg endpoint.

Split setup_mcdi() so that only MCDI allocation and initialization are
performed before RPMsg registration. Move endpoint assignment and DDR
configuration retrieval until after register_rpmsg_driver() succeeds.
Update the error paths to match the new initialization order, ensuring
resources are released in reverse order of acquisition.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index 1caaba653fc0..e9561242f292 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -559,7 +559,7 @@ static void get_ddr_config(u32 index, u32 *buffer, struct cdx_mcdi *amd_mcdi)
 static int setup_mcdi(struct mc_priv *mc_priv)
 {
 	struct cdx_mcdi *amd_mcdi;
-	int ret, i;
+	int ret;
 
 	amd_mcdi = kzalloc_obj(*amd_mcdi);
 	if (!amd_mcdi)
@@ -572,12 +572,7 @@ static int setup_mcdi(struct mc_priv *mc_priv)
 		return ret;
 	}
 
-	amd_mcdi->ept = mc_priv->ept;
 	mc_priv->mcdi = amd_mcdi;
-
-	for (i = 0; i < NUM_CONTROLLERS; i++)
-		get_ddr_config(i, &mc_priv->adec[ADEC_NUM * i], amd_mcdi);
-
 	return 0;
 }
 
@@ -886,7 +881,7 @@ static int mc_probe(struct platform_device *pdev)
 {
 	struct mc_priv *priv;
 	struct rproc *rp;
-	int rc;
+	int rc, i;
 
 	struct device_node *r5_core_node __free(device_node) =
 		of_parse_phandle(pdev->dev.of_node, "amd,rproc", 0);
@@ -914,18 +909,22 @@ static int mc_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, priv);
 	amd_rpmsg_id_table[0].driver_data = (kernel_ulong_t)priv;
 
+	rc = setup_mcdi(priv);
+	if (rc)
+		goto err_alloc;
+
 	rc = register_rpmsg_driver(&amd_rpmsg_driver);
 	if (rc) {
 		edac_printk(KERN_ERR, EDAC_MC, "Failed to register RPMsg driver: %d\n", rc);
-		goto err_alloc;
-	}
-
-	rc = setup_mcdi(priv);
-	if (rc)
 		goto err_unreg;
+	}
 
+	priv->mcdi->ept = priv->ept;
 	priv->mcdi->r5_rproc = rp;
 
+	for (i = 0; i < NUM_CONTROLLERS; i++)
+		get_ddr_config(i, &priv->adec[ADEC_NUM * i], priv->mcdi);
+
 	rc = init_versalnet(priv);
 	if (rc)
 		goto err_init;
@@ -933,11 +932,11 @@ static int mc_probe(struct platform_device *pdev)
 	return 0;
 
 err_init:
-	cdx_mcdi_finish(priv->mcdi);
-	kfree(priv->mcdi);
+	unregister_rpmsg_driver(&amd_rpmsg_driver);
 
 err_unreg:
-	unregister_rpmsg_driver(&amd_rpmsg_driver);
+	cdx_mcdi_finish(priv->mcdi);
+	kfree(priv->mcdi);
 
 err_alloc:
 	rproc_shutdown(rp);
-- 
2.34.1

Re: [PATCH 6/9] EDAC/versalnet: Initialize MCDI before RPMsg registration

[Pandey, Radhey Shyam]

On 7/24/2026 10:49 PM, Shubhrajyoti Datta wrote:
> setup_mcdi() currently allocates the MCDI instance, assigns the RPMsg
> endpoint, and retrieves DDR configuration data. The DDR configuration
> path invokes cdx_mcdi_rpc(), which requires a functional RPMsg endpoint.
> 
> Split setup_mcdi() so that only MCDI allocation and initialization are
> performed before RPMsg registration. Move endpoint assignment and DDR
> configuration retrieval until after register_rpmsg_driver() succeeds.
> Update the error paths to match the new initialization order, ensuring
> resources are released in reverse order of acquisition.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> ---
> 
>   drivers/edac/versalnet_edac.c | 29 ++++++++++++++---------------
>   1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index 1caaba653fc0..e9561242f292 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -559,7 +559,7 @@ static void get_ddr_config(u32 index, u32 *buffer, struct cdx_mcdi *amd_mcdi)
>   static int setup_mcdi(struct mc_priv *mc_priv)
>   {
>   	struct cdx_mcdi *amd_mcdi;
> -	int ret, i;
> +	int ret;
>   
>   	amd_mcdi = kzalloc_obj(*amd_mcdi);
>   	if (!amd_mcdi)
> @@ -572,12 +572,7 @@ static int setup_mcdi(struct mc_priv *mc_priv)
>   		return ret;
>   	}
>   
> -	amd_mcdi->ept = mc_priv->ept;
>   	mc_priv->mcdi = amd_mcdi;
> -
> -	for (i = 0; i < NUM_CONTROLLERS; i++)
> -		get_ddr_config(i, &mc_priv->adec[ADEC_NUM * i], amd_mcdi);
> -
>   	return 0;
>   }
>   
> @@ -886,7 +881,7 @@ static int mc_probe(struct platform_device *pdev)
>   {
>   	struct mc_priv *priv;
>   	struct rproc *rp;
> -	int rc;
> +	int rc, i;
>   
>   	struct device_node *r5_core_node __free(device_node) =
>   		of_parse_phandle(pdev->dev.of_node, "amd,rproc", 0);
> @@ -914,18 +909,22 @@ static int mc_probe(struct platform_device *pdev)
>   	platform_set_drvdata(pdev, priv);
>   	amd_rpmsg_id_table[0].driver_data = (kernel_ulong_t)priv;
>   
> +	rc = setup_mcdi(priv);
> +	if (rc)
> +		goto err_alloc;
> +
>   	rc = register_rpmsg_driver(&amd_rpmsg_driver);
>   	if (rc) {
>   		edac_printk(KERN_ERR, EDAC_MC, "Failed to register RPMsg driver: %d\n", rc);
> -		goto err_alloc;
> -	}
> -
> -	rc = setup_mcdi(priv);
> -	if (rc)
>   		goto err_unreg;
> +	}
>   
> +	priv->mcdi->ept = priv->ept;
>   	priv->mcdi->r5_rproc = rp;
>   
> +	for (i = 0; i < NUM_CONTROLLERS; i++)
> +		get_ddr_config(i, &priv->adec[ADEC_NUM * i], priv->mcdi);
> +
>   	rc = init_versalnet(priv);
>   	if (rc)
>   		goto err_init;
> @@ -933,11 +932,11 @@ static int mc_probe(struct platform_device *pdev)
>   	return 0;
>   
>   err_init:
> -	cdx_mcdi_finish(priv->mcdi);
> -	kfree(priv->mcdi);
> +	unregister_rpmsg_driver(&amd_rpmsg_driver);
>   
>   err_unreg:
> -	unregister_rpmsg_driver(&amd_rpmsg_driver);
> +	cdx_mcdi_finish(priv->mcdi);
> +	kfree(priv->mcdi);

Nit - considering renaming the labels. Rest changes
looks fine to me.

>   
>   err_alloc:
>   	rproc_shutdown(rp);

[PATCH 7/9] EDAC/versalnet: Add bounds validation in rpmsg_cb()

[Shubhrajyoti Datta]

The firmware-supplied offset and length values from the RPMsg payload
are used without validation to index into mc_priv->regs[] (REG_MAX=152
entries). A malformed or buggy firmware message could write past the end
of the array, corrupting adjacent structure members and the kernel heap.
Add check for the same.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index e9561242f292..baca90f44c58 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -602,6 +602,9 @@ static int rpmsg_cb(struct rpmsg_device *rpdev, void *data,
 	length = result[MSG_ERR_LENGTH];
 	offset = result[MSG_ERR_OFFSET];
 
+	if (offset + length > REG_MAX)
+		return -EINVAL;
+
 	/*
 	 * The data can come in two stretches. Construct the regs from two
 	 * messages. The offset indicates the offset from which the data is to
-- 
2.34.1

Re: [PATCH 7/9] EDAC/versalnet: Add bounds validation in rpmsg_cb()

[Pandey, Radhey Shyam]

On 7/24/2026 10:49 PM, Shubhrajyoti Datta wrote:
> The firmware-supplied offset and length values from the RPMsg payload
> are used without validation to index into mc_priv->regs[] (REG_MAX=152
> entries). A malformed or buggy firmware message could write past the end
> of the array, corrupting adjacent structure members and the kernel heap.
> Add check for the same.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> ---
> 
>   drivers/edac/versalnet_edac.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index e9561242f292..baca90f44c58 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -602,6 +602,9 @@ static int rpmsg_cb(struct rpmsg_device *rpdev, void *data,
>   	length = result[MSG_ERR_LENGTH];
>   	offset = result[MSG_ERR_OFFSET];
>   
> +	if (offset + length > REG_MAX)
> +		return -EINVAL;
> +

Nit - Integer overflow on offset + length

>   	/*
>   	 * The data can come in two stretches. Construct the regs from two
>   	 * messages. The offset indicates the offset from which the data is to

[PATCH 8/9] EDAC/versalnet: Fix use-after-free in remove_one_mc()

[Shubhrajyoti Datta]

device_unregister() drops the last reference on the device and invokes
versal_edac_release() which calls kfree(dev). The subsequent call to
edac_mc_del_mc(mci->pdev) then dereferences the freed pointer.

Fix by saving the device pointer, calling edac_mc_del_mc() and
edac_mc_free() first, then device_unregister() last so the device
is freed only after all users are done with it.

Fixes: 62a9fc50e8d9 ("EDAC/versalnet: Refactor memory controller initialization and cleanup")
Cc: stable@vger.kernel.org
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index baca90f44c58..ba295714d972 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -763,14 +763,16 @@ static void versal_edac_release(struct device *dev)
 static void remove_one_mc(struct mc_priv *priv, int i)
 {
 	struct mem_ctl_info *mci;
+	struct device *dev;
 
 	mci = priv->mci[i];
 	if (!mci)
 		return;
 
-	device_unregister(mci->pdev);
-	edac_mc_del_mc(mci->pdev);
+	dev = mci->pdev;
+	edac_mc_del_mc(dev);
 	edac_mc_free(mci);
+	device_unregister(dev);
 }
 
 static int init_one_mc(struct mc_priv *priv, int i)
-- 
2.34.1

Re: [PATCH 8/9] EDAC/versalnet: Fix use-after-free in remove_one_mc()

[Pandey, Radhey Shyam]

On 7/24/2026 10:49 PM, Shubhrajyoti Datta wrote:
> device_unregister() drops the last reference on the device and invokes
> versal_edac_release() which calls kfree(dev). The subsequent call to
> edac_mc_del_mc(mci->pdev) then dereferences the freed pointer.
> 
> Fix by saving the device pointer, calling edac_mc_del_mc() and
> edac_mc_free() first, then device_unregister() last so the device
> is freed only after all users are done with it.
> 
> Fixes: 62a9fc50e8d9 ("EDAC/versalnet: Refactor memory controller initialization and cleanup")
> Cc: stable@vger.kernel.org
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>


Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
Thanks!> ---
> 
>   drivers/edac/versalnet_edac.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index baca90f44c58..ba295714d972 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -763,14 +763,16 @@ static void versal_edac_release(struct device *dev)
>   static void remove_one_mc(struct mc_priv *priv, int i)
>   {
>   	struct mem_ctl_info *mci;
> +	struct device *dev;
>   
>   	mci = priv->mci[i];
>   	if (!mci)
>   		return;
>   
> -	device_unregister(mci->pdev);
> -	edac_mc_del_mc(mci->pdev);
> +	dev = mci->pdev;
> +	edac_mc_del_mc(dev);
>   	edac_mc_free(mci);
> +	device_unregister(dev);
>   }
>   
>   static int init_one_mc(struct mc_priv *priv, int i)

[PATCH 9/9] EDAC/versalnet: Use designated initializer for rpmsg_channel_info

[Shubhrajyoti Datta]

Replace field-by-field assignment of struct rpmsg_channel_info with a
designated initializer. This also fixes the incorrect use of strscpy
with strlen which could lead to silent truncation of the channel name.

Fixes: d5fe2fec6c40 ("EDAC: Add a driver for the AMD Versal NET DDR controller")
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 drivers/edac/versalnet_edac.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
index ba295714d972..88b24eec4206 100644
--- a/drivers/edac/versalnet_edac.c
+++ b/drivers/edac/versalnet_edac.c
@@ -720,14 +720,14 @@ MODULE_DEVICE_TABLE(rpmsg, amd_rpmsg_id_table);
 
 static int rpmsg_probe(struct rpmsg_device *rpdev)
 {
-	struct rpmsg_channel_info chinfo;
 	struct mc_priv *pg;
+	struct rpmsg_channel_info chinfo = {
+		.src = RPMSG_ADDR_ANY,
+		.dst = rpdev->dst,
+		.name = "error_ipc",
+	};
 
 	pg = (struct mc_priv *)amd_rpmsg_id_table[0].driver_data;
-	chinfo.src = RPMSG_ADDR_ANY;
-	chinfo.dst = rpdev->dst;
-	strscpy(chinfo.name, amd_rpmsg_id_table[0].name,
-		strlen(amd_rpmsg_id_table[0].name));
 
 	pg->ept = rpmsg_create_ept(rpdev, rpmsg_cb, NULL, chinfo);
 	if (!pg->ept)
-- 
2.34.1

Re: [PATCH 9/9] EDAC/versalnet: Use designated initializer for rpmsg_channel_info

[Pandey, Radhey Shyam]

On 7/24/2026 10:49 PM, Shubhrajyoti Datta wrote:
> Replace field-by-field assignment of struct rpmsg_channel_info with a
> designated initializer. This also fixes the incorrect use of strscpy
> with strlen which could lead to silent truncation of the channel name.
> 
> Fixes: d5fe2fec6c40 ("EDAC: Add a driver for the AMD Versal NET DDR controller")
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> ---
> 
>   drivers/edac/versalnet_edac.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/edac/versalnet_edac.c b/drivers/edac/versalnet_edac.c
> index ba295714d972..88b24eec4206 100644
> --- a/drivers/edac/versalnet_edac.c
> +++ b/drivers/edac/versalnet_edac.c
> @@ -720,14 +720,14 @@ MODULE_DEVICE_TABLE(rpmsg, amd_rpmsg_id_table);
>   
>   static int rpmsg_probe(struct rpmsg_device *rpdev)
>   {
> -	struct rpmsg_channel_info chinfo;
>   	struct mc_priv *pg;
> +	struct rpmsg_channel_info chinfo = {
> +		.src = RPMSG_ADDR_ANY,
> +		.dst = rpdev->dst,
> +		.name = "error_ipc",
> +	};

Nit - have a define for channel name and use it in both places.
Also swap above declaration order to align with reverse xmas style.

>   
>   	pg = (struct mc_priv *)amd_rpmsg_id_table[0].driver_data;
> -	chinfo.src = RPMSG_ADDR_ANY;
> -	chinfo.dst = rpdev->dst;
> -	strscpy(chinfo.name, amd_rpmsg_id_table[0].name,
> -		strlen(amd_rpmsg_id_table[0].name));
>   
>   	pg->ept = rpmsg_create_ept(rpdev, rpmsg_cb, NULL, chinfo);
>   	if (!pg->ept)