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

[Srirangan Madhavan]

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