Re: [PATCH v13 09/15] cxl: Cache endpoint HDM state during PCI enumeration

[Srirangan Madhavan <smadhavan@nvidia.com>]

On 9/23/26 2:40 PM, Cheatham, Benjamin wrote:
> External email: Use caution opening links or attachments
> 
>> +static int cxl_pci_hdm_dvsec(struct pci_dev *pdev)
>> +{
>> +     u16 cap;
>> +     int dvsec;
>> +     int rc;
>> +
>> +     dvsec = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
>> +                                       PCI_DVSEC_CXL_DEVICE);
>> +     if (!dvsec)
>> +             return -ENOTTY;
> 
> Why -ENOTTY here and below? I would've expected -ENXIO for a missing capability.
>

Agreed. Fixed this in v14 for this and other similar cases.

>> +     rc2 = pci_write_config_word(pdev, PCI_COMMAND, command);
>> +     if (rc2)
>> +             pci_err(pdev,
>> +                     "failed to restore PCI_COMMAND after CXL HDM cache init: %d\n",
>> +                     pcibios_err_to_errno(rc2));
>> +
>> +     return rc;
> 
> 
> I think the above would flow better as:
> 
> rc = __cxl_pci_hdm_read_info(pdev, map, dvsec, out);
> if (rc) {
>          if (!restore_command)
>                  return rc;
> 
>          rc2 = pci_write_config_word(pdev, PCI_COMMAND, command);
>          if (rc2)
>                  ...
> } else {
>          rc = pci_write_config_word(pdev, PCI_COMMAND, command);
>          return pcibios_err_to_errno(rc);
> }
> 
> return rc;
> 

I reworked this flow in v14. Sashiko pointed out a small issue with 
error log. I’ll also fix that missing restore-error log in the next 
revision.

>> +}
>> +
>> +static int __pci_cxl_hdm_cache_init(struct pci_dev *pdev)
>> +{
>> +     struct cxl_hdm_info *info __free(kfree) = NULL;
>> +     struct cxl_register_map map = { };
>> +     struct resource *bar;
>> +     int dvsec;
>> +     int rc;
>> +
>> +     scoped_guard(rwsem_read, &cxl_rwsem.dpa)
>> +             if (pdev->hdm)
>> +                     return 0;
>> +
>> +     dvsec = cxl_pci_hdm_dvsec(pdev);
>> +     if (dvsec < 0)
>> +             return dvsec;
> 
> Might be better to just return 0 here. If the device doesn't have a CXL capability
> then I wouldn't consider that an error from the perspective of the PCI subsystem.
> 

Fixed this in v14. pci_cxl_hdm_cache_init() returns void; a missing CXL 
capability does not fail PCI enumeration now.

>> +
>> +     rc = __pci_cxl_hdm_cache_init(pdev);
>> +     if (rc && rc != -ENOTTY && rc != -ENODEV)
>> +             pci_dbg(pdev, "CXL HDM cache init failed: %d\n", rc);
> 
> I don't know if it's worth filtering out -ENOTTY and -ENODEV for a debug message,
> but that's more of a question for the PCI people.
>

V14 logs all nonzero cache initialization results at debug level; the 
error filtering is gone.

>>   cxl_core-y := $(CXL_CORE_SRC)/port.o
>>   cxl_core-y += $(CXL_CORE_SRC)/pmem.o
>> -cxl_core-y += $(CXL_CORE_SRC)/regs.o
> 
> Did you run CXL test with this change? I don't think there are any test symbols in regs.c, but you should
> make sure this doesn't break anything there. You may not need to even fix anything if it does, but just
> make a note of the regression.
> 

The test build is not broken. For v14, make M=tools/testing/cxl/ passed 
after this change and at every patch in the series. I haven't tested the 
runtime yet.

-- 
Regards,
Srirangan