Re: [PATCH v10 05/12] cxl: Cache endpoint decoder settings during PCI enumeration

[Srirangan Madhavan <smadhavan@nvidia.com>]

On 8/21/26 4:33 PM, Dave Jiang wrote:
>> +}
>> +
>> +static int cxl_pci_hdm_capable(struct pci_dev *pdev)
> Function should return a bool.
> 
> cxl_pci_mem_capable() would be clearer on what this function is checking. Although do you want to check the hdm_count as well? That would make the original function name make sense.
> 
> Also a helper that returns the DVSEC offset and the cap word can be shared with cxl_dvsec_rr_decode() and cxl_reset_dvsec().
> 
> 
>> +{
>> +     u16 cap;
>> +     int dvsec;
>> +     int rc;
>> +
>> +     dvsec = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
>> +                                       PCI_DVSEC_CXL_DEVICE);
>> +     if (!dvsec)
>> +             return -ENOTTY;
>> +
>> +     rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CAP, &cap);
>> +     if (rc)
>> +             return pcibios_err_to_errno(rc);
>> +
>> +     if (!(cap & PCI_DVSEC_CXL_MEM_CAPABLE))
>> +             return -ENOTTY;
>> +
>> +     return 0;
>> +}
>> +
>> +static int cxl_pci_hdm_read_info(struct pci_dev *pdev,
>> +                              struct cxl_register_map *map,
>> +                              struct cxl_hdm_info *info)
>> +{
>> +     struct cxl_decoder_settings *settings;
>> +     void __iomem *hdm;
>> +     int decoder_count;
>> +     int rc;
>> +
>> +     rc = cxl_setup_regs(map);
>> +     if (rc)
>> +             return rc;
>> +
>> +     if (!map->component_map.hdm_decoder.valid)
>> +             return -ENODEV;
>> +
>> +     hdm = cxl_pci_hdm_map(pdev, map, info);
> I think you can setup a custom __free() for the hdm so you don't need to have the gotos.
> 
>> +     if (IS_ERR(hdm))
>> +             return PTR_ERR(hdm);
>> +
>> +     decoder_count = cxl_hdm_decoder_count(readl(hdm +
>> +                                                 CXL_HDM_DECODER_CAP_OFFSET));
>> +     if (decoder_count < 0) {
>> +             rc = decoder_count;
>> +             goto out_unmap;
>> +     }
>> +
>> +     if (decoder_count > CXL_HDM_DECODER_MAX_COUNT) {
> There's no need to check this. The register cannot decoder larger than a value of 32. So this scenario would never happen. Maybe
>          if (decoder_count > ARRAY_SIZE(info->settings))
> 
> DJ

Ack. I've addressed this in v11. The helper now returns bool, and I 
retained the cxl_pci_hdm_capable() name because it checks both CXL.mem 
capability and HDM decoder count.

   I also added a shared helper for retrieving the CXL Device DVSEC 
offset and capability word, added a custom __free() cleanup for the HDM 
mapping, and replaced the fixed decoder-count limit with an ARRAY_SIZE() 
check.

-- 
Regards,
Srirangan