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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 22 Sep 2026 08:39:18 +0000
Srirangan Madhavan <smadhavan@nvidia.com> wrote:

> PCI capability initialization runs before BAR resources are finalized,
> while driver binding is too late for driver-independent reset support.
> Create pci_dev->hdm during pci_bus_add_device(), after PCI resource setup
> and before driver binding.
> 
> Cache BAR-relative HDM location, global control, decoder settings, and CXL
> Device DVSEC Control, then publish the completed cache under cxl_rwsem.dpa.
> Restore PCI_COMMAND after temporary MMIO access and reject decoder-count
> changes.
> 
> Signed-off-by: Srirangan Madhavan <smadhavan@nvidia.com>

A few things inline.  Maybe well overlap with Benjamin's comments!

> diff --git a/drivers/cxl/core/resource.c b/drivers/cxl/core/resource.c
> index 7eea2bc38d94..b5f8c28e613f 100644
> --- a/drivers/cxl/core/resource.c
> +++ b/drivers/cxl/core/resource.c


> +static int __cxl_pci_hdm_read_info(struct pci_dev *pdev,
> +				   struct cxl_register_map *map, int dvsec,
> +				   struct cxl_hdm_info **out)
> +{
> +	struct cxl_reg_map *hdm_map = &map->component_map.hdm_decoder;
> +	struct cxl_hdm_info *info = NULL;

Fits in with comments below, but that should not be set to NULL up
here.

> +	resource_size_t hdm_offset;
> +	void __iomem *hdm;
> +	int decoder_count;
> +	size_t required;
> +	u16 dvsec_ctrl;
> +	int rc;
> +
> +	rc = cxl_setup_regs(map);
> +	if (rc)
> +		return rc;
> +	if (!hdm_map->valid)
> +		return -ENODEV;
> +	if (hdm_map->offset > map->max_size ||
> +	    hdm_map->size > map->max_size - hdm_map->offset)
> +		return -ENXIO;

That first lot can happen before setup regs.  Does it makes sense
to do that, then setup the regs before the follow up check?

> +	if (check_add_overflow(map->bar_offset,
> +			       (resource_size_t)hdm_map->offset, &hdm_offset))
> +		return -EOVERFLOW;
> +
> +	hdm = ioremap(map->resource + hdm_map->offset, hdm_map->size);
> +	if (!hdm)
> +		return -ENOMEM;
> +
> +	decoder_count = cxl_hdm_decoder_count(readl(hdm + CXL_HDM_DECODER_CAP_OFFSET));
> +	if (decoder_count < 0) {
> +		rc = decoder_count;
> +		goto out_unmap;
> +	}
> +
> +	required = CXL_HDM_DECODER0_TL_HIGH(decoder_count - 1) +
> +		   sizeof(u32);

I'd add a comment on why this size.

> +	if (required > hdm_map->size) {
> +		pci_err(pdev,
> +			"CXL HDM decoder count exceeds mapped register block\n");
> +		rc = -ENXIO;
> +		goto out_unmap;
> +	}
> +
> +	rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL,
> +				  &dvsec_ctrl);
> +	if (rc) {
> +		rc = pcibios_err_to_errno(rc);
> +		goto out_unmap;
> +	}
> +
> +	info = kzalloc(struct_size(info, settings, decoder_count), GFP_KERNEL);

Ah. So burried in here is where info is allocated.
If you can use struct_size for the size you can use the _obj allocators instead.
Probably kzalloc_flex().



> +	if (!info) {
> +		rc = -ENOMEM;
> +		goto out_unmap;
> +	}
> +
> +	info->decoder_count = decoder_count;
> +	info->hdm_bar = map->bar;
> +	info->hdm_offset = hdm_offset;
> +	info->hdm_size = hdm_map->size;
> +	info->global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> +	info->dvsec_ctrl = dvsec_ctrl;
> +	info->dvsec_ctrl_valid = true;
> +
> +	for (int i = 0; i < decoder_count; i++) {
> +		rc = cxl_pci_hdm_read_decoder(pdev, &info->settings[i], hdm, i);
> +		if (rc)
> +			goto out_unmap;

with ordering below fixed this should goto free_info; or something like that.

> +	}
> +
> +	/*
> +	 * A changed decoder count means the snapshot no longer describes the
> +	 * complete hardware state. Reject it rather than publish a partial cache.
> +	 */
> +	rc = cxl_hdm_decoder_count(readl(hdm + CXL_HDM_DECODER_CAP_OFFSET));
> +	if (rc != decoder_count) {
> +		pci_err(pdev, "CXL HDM decoder count changed from %d to %d\n",
> +			decoder_count, rc);
> +		rc = -ENXIO;
> +		goto out_unmap;
> +	}
> +
> +	*out = info;
> +	info = NULL;

Don't use this pattern of papering over stuff before then
running your error path.  Given no __free magic for iounmap just
have two exit paths and duplicate the iounmap().

> +	rc = 0;
> +
> +out_unmap:
> +	kfree(info);
> +	iounmap(hdm);

FWIW these are in the wrong order. You should unwind in reverse of
setup.

> +	return rc;
> +}
> +
> +static int cxl_pci_hdm_read_info(struct pci_dev *pdev,
> +				 struct cxl_register_map *map, int dvsec,
> +				 struct cxl_hdm_info **out)
> +{
> +	bool restore_command = false;
> +	u16 command;
> +	int rc, rc2;
> +
> +	guard(pci_dev)(pdev);
> +
> +	rc = pci_read_config_word(pdev, PCI_COMMAND, &command);
> +	if (rc)
> +		return pcibios_err_to_errno(rc);
> +
> +	if (!(command & PCI_COMMAND_MEMORY)) {
> +		rc = pci_write_config_word(pdev, PCI_COMMAND,
> +					   command | PCI_COMMAND_MEMORY);
> +		if (rc)
> +			return pcibios_err_to_errno(rc);
> +		restore_command = true;
> +	}
> +
> +	rc = __cxl_pci_hdm_read_info(pdev, map, dvsec, out);

To keep ownership and lifetimes clear I'd have this helper return "out"
and use a PTR_ERR() for errors.  Then I'd hand over to a __free()
in here so that you can then explicitly hand ownership on by again
returning out (with a ptr_no_free() or similar).  Basically
we want that flow of ownership to be obvious.  That may lead to dances
where you allocate then pass the ptr on via a ptr_no_free() in two
lines but that is fine.

> +	if (!restore_command)
> +		return rc;
> +
> +	if (!rc) {

This looks like a good path?  If so have the bad path out of line
not this.

> +		rc = pci_write_config_word(pdev, PCI_COMMAND, command);
> +		return pcibios_err_to_errno(rc);
> +	}
> +
> +	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;
> +}
> +
> +static int __pci_cxl_hdm_cache_init(struct pci_dev *pdev)
> +{
> +	struct cxl_hdm_info *info __free(kfree) = NULL;

See cleanup.h documentation.  I have no idea where the = NULL
at the top pattern is coming from, but there has been strong
push back from Linus and others on that in the kernel because
it introduces ordering problems. 

Here I have no idea where it is getting allocated.
If possible make that allocation function return info.


> +	struct cxl_register_map map = { };
> +	struct resource *bar;
> +	int dvsec;
> +	int rc;
> +
> +	scoped_guard(rwsem_read, &cxl_rwsem.dpa)
> +		if (pdev->hdm)
> +			return 0;

Add a comment on why just taking the lock briefly for this
check is useful. I'd expect something to need hdm to stick around
though perhaps lifetimes are fine and that isn't an issue.

> +
> +	dvsec = cxl_pci_hdm_dvsec(pdev);
> +	if (dvsec < 0)
> +		return dvsec;
> +
> +	rc = cxl_find_regblock(pdev, CXL_REGLOC_RBI_COMPONENT, &map);
> +	if (rc)
> +		return rc;
> +	if (map.bar < 0 || map.bar >= PCI_STD_NUM_BARS)
> +		return -ENODEV;
> +
> +	bar = &pdev->resource[map.bar];
> +	if (!pci_resource_len(pdev, map.bar) ||
> +	    (bar->flags & (IORESOURCE_UNSET | IORESOURCE_DISABLED)) ||
> +	    resource_type(bar) != IORESOURCE_MEM || !bar->start || !bar->end)
> +		return -ENODEV;
> +
> +	rc = cxl_pci_hdm_read_info(pdev, &map, dvsec, &info);
> +	if (rc)
> +		return rc;
> +
> +	guard(rwsem_write)(&cxl_rwsem.dpa);
> +	if (!pdev->hdm)

Needs a comment. I'm going to guess race given this pairs
with the one above?

> +		pdev->hdm = no_free_ptr(info);
> +
> +	return 0;
> +}

> +
> +void pci_cxl_hdm_cache_release(struct pci_dev *pdev)
> +{
> +	struct cxl_hdm_info *info;
> +
> +	guard(rwsem_write)(&cxl_rwsem.dpa);
> +	info = pdev->hdm;
> +	pdev->hdm = NULL;

I'd like a comment on why we need to set this NULL.
I'd kind of not really expect there to ever be a path by
which this is checked after release but maybe there is?

> +	kfree(info);
> +}