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

[Sungwoo Kim]

On Tue, Mar 31, 2026 at 6:35 AM Dave Jiang <dave.jiang@xxxxxxxxx> wrote:

[snip]

> > ---
> > V1: https://lore.kernel.org/linux-cxl/20260309000810.2632065-2-iam@xxxxxxxxxxxx/
> > V1->V2:
> > - Let __cxl_dpa_reserve() handle devm_remove_action() to reduce
> >   duplicated patches. (Thanks to Dan for the suggestion)
> > - Make the comment concise (Reflect Dan and Alison's comments)
> > - Make a warning log in a commit message concise
> >
> >  drivers/cxl/core/hdm.c | 33 +++++++++++++--------------------
> >  1 file changed, 13 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index cb5d5a047a9d..63bd0d5f31df 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -408,7 +408,13 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
> >
> >       port->hdm_end++;
> >       get_device(&cxled->cxld.dev);
> > -     return 0;
> > +
> > +     /*
> > +      * Perform devres registration while holding cxl_rwsem.dpa so
> > +      * cxl_dpa_free() cannot observe dpa_res without a matching devres
> > +      * action.
> > +      */
> > +     return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
>
> With this function added the devm_add_action_or_reset(), I think it needs to be renamed to __devm_cxl_add_dpa_reserve().
>
> DJ
>

Sounds reasonable to me. I will do this in v3.
Thank you for your review.


Sungwoo.

> >  }
> >
> >  static int add_dpa_res(struct device *dev, struct resource *parent,
> > @@ -499,16 +505,8 @@ int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
> >                               resource_size_t base, resource_size_t len,
> >                               resource_size_t skipped)
> >  {
> > -     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);
> > -
> > -     if (rc)
> > -             return rc;
> > -
> > -     return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
> > +     guard(rwsem_write)(&cxl_rwsem.dpa);
> > +     return __cxl_dpa_reserve(cxled, base, len, skipped);
> >  }
> >  EXPORT_SYMBOL_NS_GPL(devm_cxl_dpa_reserve, "CXL");
> >
> > @@ -606,7 +604,8 @@ 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));
> > @@ -669,14 +668,8 @@ static int __cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, u64 size)
> >
> >  int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, u64 size)
> >  {
> > -     struct cxl_port *port = cxled_to_port(cxled);
> > -     int rc;
> > -
> > -     rc = __cxl_dpa_alloc(cxled, size);
> > -     if (rc)
> > -             return rc;
> > -
> > -     return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
> > +     guard(rwsem_write)(&cxl_rwsem.dpa);
> > +     return __cxl_dpa_alloc(cxled, size);
> >  }
> >
> >  static void cxld_set_interleave(struct cxl_decoder *cxld, u32 *ctrl)
>
>