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

[Jonathan Cameron <jic23@kernel.org>]

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