Re: [PATCH] cxl/hdm: fix a warning in devm_remove_action()

[Sungwoo Kim]

On Tue, Mar 24, 2026 at 8:46 PM Alison Schofield
<alison.schofield@xxxxxxxxx> wrote:
>
> On Sun, Mar 08, 2026 at 08:08:10PM -0400, Sungwoo Kim wrote:
> > In the following race scenario, devm_remove_action() can be called
> > before devm_add_action(), triggering a warning because there is no
> > action to remove.
> >
> > To fix this, extend a critical section to embrace both
> > __cxl_dpa_reserve() and devm_add_action().
>
> The fix LGTM. I suggest tightening up the code comment. Now it
> talks through the race in detail and also cross-ref another location.
> Limit the comment to describing the ordering requirement we need to
> preserve. The commit message already captures the full race and
> rationale.
>
> Comment update suggestion below-
>
> snip
>
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -509,13 +509,20 @@ int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
> >       struct cxl_port *port = cxled_to_port(cxled);
> >       int rc;
> >
> > -     scoped_guard(rwsem_write, &cxl_rwsem.dpa)
> > -             rc = __cxl_dpa_reserve(cxled, base, len, skipped);
> > +     guard(rwsem_write)(&cxl_rwsem.dpa);
> > +     rc = __cxl_dpa_reserve(cxled, base, len, skipped);
> >
> >       if (rc)
> >               return rc;
> >
> > -     return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
> > +     /* See comments in cxl_dpa_alloc()*/
>
> Something like this:
> /* Keep reservation and devres registration ordered under dpa lock */
>
>
> snip
>
> > @@ -613,7 +620,7 @@ static int __cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, u64 size)
> >       struct resource *p, *last;
> >       int part;
> >
> > -     guard(rwsem_write)(&cxl_rwsem.dpa);
> > +     lockdep_assert_held_write(&cxl_rwsem.dpa);
> >       if (cxled->cxld.region) {
> >               dev_dbg(dev, "decoder attached to %s\n",
> >                       dev_name(&cxled->cxld.region->dev));
> > @@ -679,11 +686,28 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, u64 size)
> >       struct cxl_port *port = cxled_to_port(cxled);
> >       int rc;
> >
> > +     guard(rwsem_write)(&cxl_rwsem.dpa);
> >       rc = __cxl_dpa_alloc(cxled, size);
> >       if (rc)
> >               return rc;
> >
> > -     return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
> > +     /*
> > +      * Add the devres action while still holding cxl_rwsem.dpa to prevent
> > +      * a race with cxl_dpa_free(). Without this, a concurrent cxl_dpa_free()
> > +      * can observe dpa_res set (by __cxl_dpa_reserve()) and attempt
> > +      * devm_remove_action() before devm_add_action() has been called,
> > +      * triggering a WARN_ON in devm_remove_action().
> > +      * Also, devm_add_action_or_reset() cannot be used here because
> > +      * cxl_dpa_release() tries to hold the dpa lock that is already held,
> > +      * causing a self deadlock.
> > +      */
>
> and like this:
>
> /*
>  * Hold cxl_rwsem.dpa across allocation and devres registration
>  * so cxl_dpa_free() cannot observe dpa_res without a matching
>  * devres action.
>  */
>
> snip
>

Thank you for your review with a suggestion. Will do in V2.
Resending to the mailing list, I replied privately by mistake.

Sungwoo.