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

[Srirangan Madhavan <smadhavan@nvidia.com>]

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