Re: [PATCH v13 11/15] cxl: Validate and synchronize HDM ranges around reset

[Srirangan Madhavan]

On 9/23/26 2:40 PM, Cheatham, Benjamin wrote:
> External email: Use caution opening links or attachments
>
> > +
> > +     list_for_each_entry(range, &ctx->ranges, list)
> > +             if (range->hpa_range.start == hpa_range->start &&
> > +                 range->hpa_range.end == hpa_range->end)
>
> Maybe resource_contains() instead? I don't think there'll be any overlap, but no point in reserving
> a fully contained resource. You'd have to add some extra logic to remove the smaller resource though.

Done in v14. I used range_contains() for the struct range values. 
Already covered is skipped, and a smaller collected range is removed 
when a larger one arrives.

> > +
> > +             if (!(config->flags & CXL_DECODER_F_ENABLE))
> > +                     continue;
> > +
> > +             /* A committed zero-size decoder maps no HPA. */
> > +             if (!range_len(&config->hpa_range))
> > +                     continue;
>
> Nit: these two if statements could be on a single line and just put the comment above it, it's pretty
> obvious which condition the comment is talking about.

Done

> > +
> > +out_destroy_ranges:
> > +     cxl_hdm_range_context_destroy(range_ctx);
>
> Same thing here as the last patch, but I'll let you do this one ;).

Ack. V14 splits the reset sequence into smaller helpers and removes this 
cleanup-label flow. Please let me know if it is better now.

> > +
> > +     INIT_LIST_HEAD(&range_ctx.ranges);
> > +
> > +     scoped_guard(rwsem_write, &cxl_rwsem.region) {
> > +             rc = cxl_hdm_ranges_prepare(&range_ctx, pdev);
> > +             if (!rc)
> > +                     rc = cxl_reset_execute(pdev, dvsec, cap, &range_ctx);
> > +             else
> > +                     cxl_hdm_range_context_destroy(&range_ctx);
> > +     }
>
> Probably better to just copy-paste the range context destroy call and re-order as:
>
>                  rc = cxl_hdm_ranges_prepare(&range_ctx, pdev);
>                  if (rc) {
>                          cxl_hdm_range_context_destroy(&range_ctx);
>                          return rc;
>                  }
>
>
>                  rc = cxl_reset_execute(pdev, dvsec, cap, &range_ctx);
>                  cxl_hdm_range_context_destroy(&range_ctx);
>
> instead.

Ack. In v14, cxl_reset_execute() owns the range context from preparation 
through cleanup, so this handoff between functions is gone.

--
Regards,
Srirangan