Re: [PATCH v2 1/3] EDAC/versalnet: Fix teardown ordering in mc_remove()

Prasanna Kumar T S M
2026-04-06T05:26:23Z

On 03-04-2026 16:04, Borislav Petkov wrote:
> On Wed, Apr 01, 2026 at 04:18:36AM -0700, Prasanna Kumar T S M wrote:
>> The teardown sequence in mc_remove() does not mirror the reverse of the
>> initialization order in mc_probe(). In particular,
>> unregister_rpmsg_driver() is called before remove_versalnet(), and
>> cdx_mcdi_finish() is called after rproc_shutdown().
>>
>> Reorder mc_remove() to reverse the probe initialization sequence,
>> consistent with the probe error-unwind paths.
>>
>> The rproc reference acquired via rproc_get_by_phandle() during probe
>> is not released in mc_remove(), causing a reference count leak. Add
>> the missing rproc_put() call.
>>
>> Fixes: d5fe2fec6c40 ("EDAC: Add a driver for the AMD Versal NET DDR controller")
>> Signed-off-by: Prasanna Kumar T S M <ptsm@linux.microsoft.com>
>> ---
>> drivers/edac/versalnet_edac.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> Sashiko has found things, pls addres them:
>
> https://sashiko.dev/#/patchset/20260401111836.2342918-1-ptsm%40linux.microsoft.com
>

I asked AI to validate Sashiko's comment. This is its output
------------------------------------------------------------

Analysis

The review comment is not valid (false positive). Here's the detailed
reasoning:

Data structure hierarchy

mc_priv
└── mcdi: struct cdx_mcdi * ← kfree'd at step 6 (LAST)
├── mcdi: struct cdx_mcdi_data * ← kfree'd by
cdx_mcdi_finish() at step 2
│ └── iface: struct cdx_mcdi_iface (mutex, cmd list, etc.)
├── r5_rproc
└── ...

New mc_remove() order (after commit)

(1) remove_versalnet(priv); // tear down EDAC
(2) cdx_mcdi_finish(priv->mcdi); // wait_for_cleanup →
kfree(cdx->mcdi) → cdx->mcdi = NULL
(3) unregister_rpmsg_driver(&amd_rpmsg_driver); // destroys rpmsg
endpoint
(4) rproc_shutdown(priv->mcdi->r5_rproc);
(5) rproc_put(priv->mcdi->r5_rproc);
(6) kfree(priv->mcdi); // frees outer struct

Why the concern doesn't hold

The reviewer's premise is wrong. The comment says "after priv->mcdi
is freed" — but priv->mcdi (the struct cdx_mcdi * pointer passed to
cdx_mcdi_process_cmd) is not freed
until step 6, well after unregister_rpmsg_driver() at step 3.

What IS freed at step 2 is the inner cdx->mcdi (struct cdx_mcdi_data
*). But cdx_mcdi_finish() also sets cdx->mcdi = NULL. If rpmsg_cb()
fires between steps 2 and 3:

1. mc_priv->mcdi → still points to valid struct cdx_mcdi (not freed
until step 6)
2. cdx_mcdi_process_cmd(mc_priv->mcdi, ...) is called with a valid
pointer
3. Inside, cdx_mcdi_if(cdx) checks cdx->mcdi → NULL → returns NULL
4. if (!mcdi) return; → exits early, safe

Additionally, cdx_mcdi_wait_for_cleanup() inside cdx_mcdi_finish()
ensures all pending MCDI commands have completed before freeing, so no
legitimate MCDI responses should
arrive afterward.

This ordering exactly matches the probe error unwind path (err_init:
does cdx_mcdi_finish → kfree → unregister_rpmsg_driver → rproc_shutdown
→ rproc_put), which the
commit explicitly states it's aligning with.

Verdict: False positive. The NULL guard in cdx_mcdi_if() /
cdx_mcdi_process_cmd() protects the window, and the outer struct
priv->mcdi remains valid throughout.

------------------------------------------------------------

So the comment looks like a false positive. It will be great if someone
from AMD verifies this.

Can you please look at the other 2 patches in this series? They are
independent of this change.

Thanks,
Prasanna Kumar