Re: [PATCH v12 04/12] cxl: Cache decoder settings on PCI devices

[Srirangan Madhavan <smadhavan@nvidia.com>]

On 9/11/26 5:22 PM, Jonathan Cameron wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 10 Sep 2026 07:08:00 +0000
> Srirangan Madhavan <smadhavan@nvidia.com> wrote:
> 
>> Add CXL core plumbing to refresh a PCI device HDM decoder cache when
>> decoders are enumerated, committed, or reset. PCI reset paths can use
>> this snapshot to restore HDM programming without walking CXL topology
>> during reset recovery.
>>
>> The cache is populated by PCI-side discovery in a follow-on patch. Until
>> then, the CXL core update path is a no-op when no PCI HDM cache is
>> present.
>>
>> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>
> Various comments inline
> 
> Thanks,
> 
> Jonathan
>> ---
>>   drivers/cxl/core/hdm.c | 68 +++++++++++++++++++++++++++++++++++++++++-
>>   include/cxl/cxl.h      | 12 ++++++++
>>   include/linux/pci.h    |  6 ++++
>>   3 files changed, 85 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>> index d621d827f59f..0927036aed27 100644
>> --- a/drivers/cxl/core/hdm.c
>> +++ b/drivers/cxl/core/hdm.c
>> @@ -16,6 +16,9 @@
>>    * for enumerating these registers and capabilities.
>>
>>
>> +static bool __cxl_pci_hdm_decoder_count_match(struct pci_dev *pdev,
>> +                                           int decoder_count)
>> +{
>> +     struct cxl_hdm_info *info;
>> +     bool match = true;
>> +
>> +     down_read(&cxl_rwsem.dpa);
> 
> guard()
>> +     info = pdev->hdm;
>> +     if (info) {
> 
> If !info fails the number of decoders definitely didn't match - so maybe
> print something in that path too.
> 
> so far, the local info variable is less readable than pdev->hdm.
> Maybe it becomes more useful later in series.
> 
>> +             if (info->decoder_count != decoder_count) {
>> +                     pci_warn(pdev,
>> +                              "CXL HDM cache decoder count mismatch: cached=%d hdm=%d\n",
>> +                              info->decoder_count, decoder_count);
>> +                     match = false;
> 
>> +             }
>> +     }
>> +     up_read(&cxl_rwsem.dpa);
>> +
>> +     return match;
>> +}
>> +
>> +static bool cxl_pci_hdm_decoder_count_match(struct cxl_hdm *cxlhdm)
>> +{
>> +     struct pci_dev *pdev __free(pci_dev_put) =
>> +             cxl_port_get_uport_pci_dev(cxlhdm->port);
>> +
>> +     if (!pdev)
>> +             return true;
>> +
>> +     return __cxl_pci_hdm_decoder_count_match(pdev, cxlhdm->decoder_count);
> 
> I went looking and seems like this is the only call. Just bring the implementation
> inline here.
> 
>> +}
>> +
>> +static void cxl_hdm_save_decoder_info(struct cxl_hdm *cxlhdm,
>> +                                   struct cxl_decoder *cxld)
>> +{
>> +     struct pci_dev *pdev __free(pci_dev_put) =
>> +             cxl_port_get_uport_pci_dev(cxlhdm->port);
>> +     struct cxl_decoder_settings *settings;
>> +     struct cxl_hdm_info *info;
>> +
>> +     if (!pdev)
>> +             return;
>> +
>> +     guard(rwsem_write)(&cxl_rwsem.dpa);
>> +     info = pdev->hdm;
>> +     if (!info || cxld->id >= info->decoder_count)
>> +             return;
>> +
>> +     settings = &info->settings[cxld->id];
>> +     *settings = (struct cxl_decoder_settings) {
>> +             .id = cxld->id,
>> +     };
>> +     if (cxld->flags & CXL_DECODER_F_ENABLE)
> 
> Why is it bad to snapshot a non enabled decoder?  Is it pointless
> or harmful.  Add a comment.
> 
>> +             cxl_decoder_snapshot(cxld, settings);
>> +}
> 
>> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
>> index c09492af8fbd..ed5237df510f 100644
>> --- a/include/cxl/cxl.h
>> +++ b/include/cxl/cxl.h
>> @@ -133,6 +133,18 @@ struct cxl_regs {
>>        );
>>   };
>>
>> +#define CXL_HDM_DECODER_MAX_COUNT 32
>> +
>> +/**
>> + * struct cxl_hdm_info - PCI device HDM decoder programming cache
>> + * @decoder_count: number of decoder settings entries
>> + * @settings: cached per-decoder programming state
>> + */
>> +struct cxl_hdm_info {
>> +     int decoder_count;
>> +     struct cxl_decoder_settings settings[CXL_HDM_DECODER_MAX_COUNT];
>> +};
> 
> This is quite a big structure. Can't we do
>          int decoder_count;
>          struct cxl_decodet setting settings[] __counted_by(decoder_count);
> and allocate the necessary size using struct_size()?
> 
> I doubt that many devices will do all 32 decoders - particularly as
> the spec used to allow a smaller number IIRC.
> 
>> +
>>   struct cxl_reg_map {
>>        bool valid;
>>        int id;
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index d31a8d107b1e..84d058b1b492 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -339,6 +339,9 @@ struct pcie_link_state;
>>   struct pci_sriov;
>>   struct pci_p2pdma;
>>   struct rcec_ea;
>> +#ifdef CONFIG_CXL_RESET
>> +struct cxl_hdm_info;
>> +#endif
>>
>>   /* struct pci_dev - describes a PCI device
>>    *
>> @@ -566,6 +569,9 @@ struct pci_dev {
>>   #ifdef CONFIG_PCI_DOE
>>        struct xarray   doe_mbs;        /* Data Object Exchange mailboxes */
>>   #endif
>> +#ifdef CONFIG_CXL_RESET
>> +     struct cxl_hdm_info *hdm;       /* CXL HDM decoder reset state */
>> +#endif
>>   #ifdef CONFIG_PCI_NPEM
>>        struct npem     *npem;          /* Native PCIe Enclosure Management */
>>   #endif
> 

I addressed these comments in v13 patch 8:

   - replaced the manual locking with guard();
   - removed the single-use count-match wrapper;
   - added the explanation for not snapshotting stale state from a 
disabled decoder; and
   - changed cxl_hdm_info to use a flexible array allocated with 
struct_size().

For the missing pdev->hdm case, I did not report a decoder-count 
mismatch. With the revised flow, no cache means there is no state to 
refresh; reset will subsequently be rejected because no snapshot is 
available. A mismatch is reported only when a cache exists and its 
decoder count differs.

https://lore.kernel.org/linux-cxl/20260922083924.2451158-9-smadhavan@nvidia.com/

-- 
Regards,
Srirangan