Re: [PATCH v10 07/12] cxl: Validate HDM ranges before CXL reset

[Srirangan Madhavan]

On 8/25/26 1:30 PM, Dave Jiang wrote:
> > +                 range->hpa_range.end == hpa_range->end)
> I think range_contains() would work here?

 Wouldn’t range_contains() also match a subrange? Is it okay to treat a 
contained but non-identical decoder range as a duplicate as well?

> > +                     return 0;
> > +
> > +     range = kzalloc_obj(*range);
> > +     if (!range)
> > +             return -ENOMEM;
> > +
> > +     range->pdev = pdev;
> > +     range->hpa_range = *hpa_range;
> > +     list_add_tail(&range->list, &ctx->ranges);
> > +
> > +     return 0;
> > +}
> > +
> > +static int cxl_hdm_ranges_collect(struct cxl_hdm_range_context *ctx,
> > +                               struct pci_dev *pdev)
> > +{
> > +     struct cxl_hdm_info *info;
> > +     int rc;
> > +
> > +     guard(rwsem_read)(&cxl_rwsem.dpa);
> > +     info = pdev->hdm;
> > +     if (!info) {
> > +             pci_err(pdev, "CXL HDM decoder state unavailable\n");
> > +             return -ENXIO;
> > +     }
> > +
> > +     for (int i = 0; i < info->decoder_count; i++) {
> > +             struct cxl_decoder_settings *settings = &info->settings[i];
> > +
> > +             if (!(settings->flags & CXL_DECODER_F_ENABLE))
> > +                     continue;
> > +
> > +             if (settings->flags & CXL_DECODER_F_NORMALIZED_ADDRESSING) {
> > +                     pci_err(pdev,
> > +                             "CXL reset does not support normalized address decoders\n");
> > +                     return -EOPNOTSUPP;
> > +             }
> > +
> > +             rc = cxl_hdm_range_add(ctx, pdev, &settings->hpa_range);
> > +             if (rc)
> > +                     return rc;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int cxl_hdm_range_len(struct pci_dev *pdev,
> > +                          const struct range *hpa_range, u64 *len)
> > +{
> > +     if (hpa_range->end < hpa_range->start)
> > +             return -EINVAL;
> > +
> > +     if (hpa_range->start > RESOURCE_SIZE_MAX ||
> > +         hpa_range->end > RESOURCE_SIZE_MAX) {
> Given that above you established that (end >= start) couple lines above, you really only need to test end here.
>
> > +             pci_err(pdev,
> > +                     "CXL reset range [%#llx-%#llx] exceeds resource address size\n",
> > +                     hpa_range->start, hpa_range->end);
> > +             return -EOVERFLOW;
> > +     }
> > +
> > +     *len = range_len(hpa_range);
> > +     if (!*len || *len > RESOURCE_SIZE_MAX) {
> > +             pci_err(pdev,
> > +                     "CXL reset range [%#llx-%#llx] exceeds resource size\n",
> > +                     hpa_range->start, hpa_range->end);
> > +             return -EOVERFLOW;
> > +     }
> > +
> > +     if (*len > SIZE_MAX) {
> > +             pci_err(pdev,
> > +                     "CXL reset range [%#llx-%#llx] exceeds cache flush size\n",
> > +                     hpa_range->start, hpa_range->end);
> > +             return -EOVERFLOW;
> > +     }
> > +
> > +     return 0;
> > +}
> This function is doing too much. I suggest you rename it cxl_hdm_range_validate() and drop the *len parameter. And just assign len from range_len(hpa_range) once it's validated. I'll paste a diff at the end as a suggestion.

 I applied this refactor in v11: the helper is now 
cxl_hdm_range_validate(), it no longer has a len output parameter, and 
the caller assigns range_len() after validation.

> > +
> > +static int cxl_hdm_range_request(struct cxl_hdm_range *range)
> > +{
> > +     struct pci_dev *pdev = rang


--
Regards,
Srirangan