Re: [PATCH v12 06/12] cxl: Add CXL Device Reset helper

[Jonathan Cameron]

On Thu, 10 Sep 2026 07:08:02 +0000
Srirangan Madhavan <smadhavan@xxxxxxxxxx> wrote:

> Add an internal CXL Device Reset helper for Type 2 functions that
> advertise CXL Reset and CXL Reset Memory Clear in the CXL Device DVSEC.
> The helper disables CXL.cache, performs cache writeback when supported,
> initiates reset with Memory Clear enabled, waits for completion, and
> re-enables CXL.cache on exit.
> 
> Leave the helper unregistered until range validation and reset-scope
> validation are in place.
> 
> Signed-off-by: Srirangan Madhavan <smadhavan@xxxxxxxxxx>
A few things inline.
> ---
>  drivers/cxl/core/resource.c   | 251 ++++++++++++++++++++++++++++++++++
>  include/cxl/cxl.h             |   7 +
>  include/uapi/linux/pci_regs.h |  14 ++
>  3 files changed, 272 insertions(+)
> 
> diff --git a/drivers/cxl/core/resource.c b/drivers/cxl/core/resource.c
> index 6d9f8fe14b16..9c1aa0800521 100644
> --- a/drivers/cxl/core/resource.c
> +++ b/drivers/cxl/core/resource.c
> @@ -8,6 +8,8 @@
>  #include <linux/export.h>
>  #include <linux/io.h>
>  #include <linux/ioport.h>
> +#include <linux/iommu.h>
> +#include <linux/jiffies.h>
>  #include <linux/kernel.h>
>  #include <linux/pci.h>
>  #include <linux/slab.h>
> @@ -492,3 +494,252 @@ void pci_cxl_hdm_init(struct pci_dev *pdev)
>  	if (rc && rc != -ENOTTY && rc != -ENODEV)
>  		pci_dbg(pdev, "CXL HDM cache init failed: %d\n", rc);
>  }
> +/*
> + * CXL r4.0 sec 9.7.2 defines the reset completion timeout encodings.
> + * Sec 9.7.3 leaves config-space access behavior undefined for 100 ms after
> + * initiating CXL Reset, then limits software to CXL Status2 access until
> + * reset completion, timeout, or error.
> + */
> +#define CXL_RESET_RRS_WAIT_MS 100
> +#define CXL_RESET_STATUS_POLL_MS 20
> +static const u32 cxl_reset_timeout_ms[] = {
> +	10, 100, 1000, 10000, 100000,
> +};
> +
> +#define CXL_CACHE_WBI_TIMEOUT_US 100000
> +#define CXL_CACHE_WBI_POLL_US 100
> +
> +static int cxl_reset_get_dvsec(struct pci_dev *pdev, u16 *cap_out)
> +{
> +	int dvsec, rc;
> +	u16 cap, ctrl;
> +
> +	dvsec = cxl_pci_get_device_dvsec_cap(pdev, 0, &cap);

That is an odd function given the control read is separate - why
should we wrap up the cap read?

I'd just drop this helper and open code the search for the extended
cap at the few callsites.  A little more code, but less odd and all
standard PCI stuff not needing an extra helper.

Probably a bit of code evolution caused this given you are up at v12!

> +	if (dvsec < 0)
> +		return dvsec;
> +
> +	if (!(cap & PCI_DVSEC_CXL_CACHE_CAPABLE) ||
> +	    !(cap & PCI_DVSEC_CXL_MEM_CAPABLE))

Why do we need them both? Sure that's type 2, but a non
class code matching type3 would I think need the same infrastructure
you are building here. That would have cxl.mem but not cxl.cache
- I think some of the CXL SSD prototypes fit in this category.

> +		return -ENOTTY;
> +
> +	if (!(cap & PCI_DVSEC_CXL_RST_CAPABLE))
> +		return -ENOTTY;
> +	if (!(cap & PCI_DVSEC_CXL_RST_MEM_CLR_CAPABLE))
> +		return -ENOTTY;
> +
> +	rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL, &ctrl);
> +	if (rc)
> +		return pcibios_err_to_errno(rc);
> +
> +	if (!(ctrl & PCI_DVSEC_CXL_CACHE_ENABLE) ||
> +	    !(ctrl & PCI_DVSEC_CXL_MEM_ENABLE))
> +		return -ENOTTY;
> +
> +	*cap_out = cap;
> +	return dvsec;
> +}
> +
> +#define CXL_RESET_CTRL2_CMD_MASK \
> +	(PCI_DVSEC_CXL_INIT_CACHE_WBI | PCI_DVSEC_CXL_INIT_CXL_RST)
> +
> +static int cxl_reset_read_ctrl2(struct pci_dev *pdev, int dvsec, u16 *ctrl2)
> +{
> +	int rc;
> +
> +	rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL2, ctrl2);
> +	if (rc)
> +		return pcibios_err_to_errno(rc);
> +
> +	*ctrl2 &= ~CXL_RESET_CTRL2_CMD_MASK;
I'd rename this. It is doing more than reading crl2.  Or push the code
inline and avoid need for any name bikeshedding.

> +	return 0;
> +}
> +
> +static int cxl_reset_write_ctrl2(struct pci_dev *pdev, int dvsec, u16 ctrl2)
> +{
> +	int rc;
> +
> +	rc = pci_write_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL2, ctrl2);
> +	if (rc)
> +		return pcibios_err_to_errno(rc);
This helper is also providing little value
> +
> +	return 0;
> +}
> +
> +static int cxl_reset_modify_ctrl2(struct pci_dev *pdev, int dvsec, u16 set,
> +				  u16 clear)

This needs a rename because it has that subtle mask of previous
state. What is it actually allowing you to modify?

> +{
> +	u16 ctrl2;
> +	int rc;
> +
> +	rc = cxl_reset_read_ctrl2(pdev, dvsec, &ctrl2);
> +	if (rc)
> +		return rc;
> +
> +	ctrl2 &= ~clear;
> +	ctrl2 |= set;
> +	return cxl_reset_write_ctrl2(pdev, dvsec, ctrl2);

So with them squashed inline this would just be
{
	u16 ctrl2;
	int rc;

	rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL2, ctrl2);
	if (rc)
		return pcibios_err_to_errno(rc);

	/* Comment on why this is masked */ 
	ctrl2 &= ~(PCI_DVSEC_CXL_INIT_CACHE_WBI | PCI_DVSEC_CXL_INIT_CXL_RST);
	ctrl2 &= ~clear;
	ctrl2 |= set;

	rc = pci_write_config_word(pdev, dvsec + PCI_DVSEC_CXL_CTRL2, ctrl2);
	if (rc)
		return pcibios_err_to_errno(rc);

	return 0;
}

which is if anything easier to read.

> +}
> +
> +static int cxl_reset_enable_cache(struct pci_dev *pdev, int dvsec)
> +{
> +	return cxl_reset_modify_ctrl2(pdev, dvsec, 0,
> +				      PCI_DVSEC_CXL_DISABLE_CACHING);
> +}
> +
> +static int cxl_reset_initiate(struct pci_dev *pdev, int dvsec)
> +{
> +	return cxl_reset_modify_ctrl2(pdev, dvsec,
> +				      PCI_DVSEC_CXL_INIT_CXL_RST |
> +				      PCI_DVSEC_CXL_RST_MEM_CLR_EN, 0);
> +}



> +
> +static int cxl_reset_execute(struct pci_dev *pdev, int dvsec, u16 cap)
> +{
> +	bool target_prepared = false;
> +	int rc, rc2;
> +
> +	rc = cxl_reset_disable_cache(pdev, dvsec, cap);
> +	if (rc)
> +		return rc;
> +
> +	if (!pci_wait_for_pending_transaction(pdev))
> +		pci_err(pdev, "timed out waiting for pending transactions\n");
> +
> +	rc = pci_dev_reset_iommu_prepare(pdev);
> +	if (rc)
> +		pci_err(pdev, "failed to stop IOMMU for CXL reset: %d\n", rc);

Maybe a comment on whether this is even remotely safe to continue.
My gut feeling is this sort of thing happens, just give up...


> +	else
> +		target_prepared = true;
> +
> +	if (!rc)
> +		rc = cxl_reset_initiate(pdev, dvsec);
> +	if (!rc)
> +		rc = cxl_reset_wait_done(pdev, dvsec, cap);
> +
> +	rc2 = cxl_reset_enable_cache(pdev, dvsec);
Can't we delay this until after the iommu is told things are back?
I'm not keen on the sequence not being a clean tear down then a clean setup
in the other order. I may well be missing some subtleties. This maybe
needs some documentation.

> +	if (rc2 && rc)
> +		pci_warn(pdev, "failed to re-enable CXL caching: %d\n", rc2);

What is logic about not printing if we have another failure sat in rc?
Is it not true anyway?

> +	else if (rc2)
> +		rc = rc2;

This code flow is less than ideal I'd use some gotos rather than that if (!rc)
dance.

> +
> +	if (target_prepared)
> +		pci_dev_reset_iommu_done(pdev);
> +	return rc;

	return rc ? rc : rc2; and some of the complexity above goes away.

> +}
> +
> +int cxl_reset_function(struct pci_dev *pdev, bool probe)
> +{
> +	int dvsec;
> +	u16 cap;
> +
> +	dvsec = cxl_reset_get_dvsec(pdev, &cap);
> +	if (dvsec < 0)
> +		return dvsec;
> +
> +	if (probe)
> +		return 0;
> +
> +	return cxl_reset_execute(pdev, dvsec, cap);
> +}