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

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

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