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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The PCI_DVSEC_CXL_RST_MEM_CLR_EN bit is not cleared on error paths, causing subsequent resets to inadvertently perform a full Memory Clear.
- [Medium] Missing final status check after timeout in cache WBI polling can cause a false timeout.
--

commit e65504bb1432889a2f2835801327341b00b21dfc
Author: Srirangan Madhavan <smadhavan@nvidia.com>

cxl: Add CXL Device Reset helper

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.

> diff --git a/drivers/cxl/core/resource.c b/drivers/cxl/core/resource.c
> index 6d9f8fe14b16e..9c1aa0800521b 100644
> --- a/drivers/cxl/core/resource.c
> +++ b/drivers/cxl/core/resource.c

[ ... ]

> +#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;

[Severity: High]
Does this mask inadvertently exclude the memory clear enable bit, preserving it
on error paths?

If cxl_reset_initiate() sets PCI_DVSEC_CXL_RST_MEM_CLR_EN and the reset
subsequently fails, the error path calls cxl_reset_enable_cache(). That calls
cxl_reset_modify_ctrl2(), which relies on cxl_reset_read_ctrl2() to fetch the
current register state.

Because CXL_RESET_CTRL2_CMD_MASK does not include PCI_DVSEC_CXL_RST_MEM_CLR_EN,
the stale bit is read, preserved, and written back. Could this cause subsequent
resets to inadvertently perform a full Memory Clear?

[ ... ]

> +static int cxl_reset_wait_cache_wbi(struct pci_dev *pdev, int dvsec)
> +{
> +	unsigned long deadline;
> +	u16 status2;
> +	int rc;
> +
> +	rc = cxl_reset_modify_ctrl2(pdev, dvsec,
> +				    PCI_DVSEC_CXL_INIT_CACHE_WBI, 0);
> +	if (rc)
> +		return rc;
> +
> +	deadline = jiffies + usecs_to_jiffies(CXL_CACHE_WBI_TIMEOUT_US);
> +	do {
> +		usleep_range(CXL_CACHE_WBI_POLL_US, CXL_CACHE_WBI_POLL_US + 1);
> +
> +		rc = pci_read_config_word(pdev, dvsec + PCI_DVSEC_CXL_STATUS2,
> +					  &status2);
> +		if (rc)
> +			return pcibios_err_to_errno(rc);
> +		if (status2 != U16_MAX && (status2 & PCI_DVSEC_CXL_CACHE_INV))
> +			return 0;
> +	} while (time_before(jiffies, deadline));
> +
> +	return -ETIMEDOUT;

[Severity: Medium]
Does this loop miss a final status check after the timeout expires?

If the thread is preempted or delayed between the register read and the time
check, time_before(jiffies, deadline) can fail even if the cache invalidation
succeeded just as the timeout expired.

Standard kernel polling macros (like readx_poll_timeout) avoid false timeouts
by evaluating the status one final time after the timeout expires.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910070808.1444264-1-smadhavan@nvidia.com?part=6