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

[Srirangan Madhavan]

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