Re: [PATCH v6 7/9] cxl/pci: Orchestrate CXL reset for affected memdevs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] cxl_reset_disable_memdevs() fails to update cxlds->media_ready to false when disabling CXL.mem.
- [High] Missing wait for pending DMA transactions before blocking the IOMMU for sibling PCI functions.
- [High] Early PCI config state restore re-enables bus mastering while the IOMMU is still blocked.
- [Medium] Mixing goto-based error handling and scope-based cleanup helpers in the same function.
--

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

cxl/pci: Orchestrate CXL reset for affected memdevs

This commit adds a reset flow for CXL devices that coordinates the target
function, sibling functions, and active memdevs. It collects regions,
flushes caches, saves and disables functions, and then executes the CXL
reset before restoring PCI config space and re-enabling CXL.mem.

> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index c755c18c8d84a..486c447e98f3f 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c

[ ... ]

> +static int cxl_pci_functions_reset_prepare(struct cxl_reset_context *ctx)
> +{
> +	int rc, i;
> +
> +	ctx->nr_siblings_prepared = 0;
> +
> +	for (i = 0; i < ctx->nr_siblings_locked; i++) {
> +		struct pci_dev *sibling = ctx->siblings[i];
> +
>  		pci_dev_save_and_disable(sibling);
>  		rc = pci_dev_reset_iommu_prepare(sibling);

[Severity: High]
Is it safe to block the IOMMU here without waiting for in-flight DMA
transactions to drain?

pci_dev_save_and_disable() disables bus mastering, but any transactions
already in flight might be dropped by the IOMMU and raise AERs (Advanced
Error Reporting faults) if we don't wait for them.

The target device correctly drains its transactions using
pci_wait_for_pending_transaction(), could we do the same for the sibling
functions here?

[ ... ]

> +static void cxl_pci_functions_restore_state(struct cxl_reset_context *ctx)
> +{
> +	int i;
> +
> +	for (i = ctx->nr_siblings_prepared - 1; i >= 0; i--)
> +		pci_restore_state(ctx->siblings[i]);
> +}

[Severity: High]
Does restoring the PCI config space here re-enable bus mastering while the
IOMMU is still actively blocking requests?

pci_restore_state() restores the entire saved PCI config space, including
the PCI_COMMAND register with the PCI_COMMAND_MASTER bit set.

Since the IOMMU is not unblocked until pci_dev_reset_iommu_done() runs
later during cleanup, could this allow the device to issue DMAs (or
MSI/MSI-X interrupts) that will trigger IOMMU faults and AERs?

[ ... ]

> +static void cxl_reset_disable_memdevs(struct cxl_reset_context *ctx)
> +{
> +	int rc, i;
> +
> +	for (i = ctx->nr_memdevs - 1; i >= 0; i--) {
> +		struct cxl_memdev *cxlmd = ctx->memdevs[i].cxlmd;
> +
> +		if (!ctx->memdevs[i].active)
> +			continue;
> +
> +		rc = cxl_set_mem_enable(cxlmd->cxlds, 0);

[Severity: High]
Could this leave the device state indicating the media is active when
physical memory decode has been disabled?

cxl_set_mem_enable() disables CXL.mem on the device, but
cxlds->media_ready isn't updated to false here. If the system subsequently
attempts to access this memory, could it result in unhandled unsupported
requests (UR) and trigger fatal bus errors?

[ ... ]

> +static int __maybe_unused cxl_do_reset(struct pci_dev *pdev, bool mem_clear)
> +{
> +	struct cxl_reset_context ctx = {
> +		.target = pdev,
> +	};
> +	int rc;
> +
> +	/*
> +	 * Snapshot the CXL r3.2 9.7 device reset scope before taking
> +	 * cxl_rwsem.region. Hot-added functions after this point are not
> +	 * coordinated by this reset operation.
> +	 */
> +	rc = cxl_reset_collect_siblings(&ctx);
> +	if (rc)
> +		goto out;
> +
> +	rc = cxl_reset_collect_memdevs(&ctx);
> +	if (rc)
> +		goto out;
> +
> +	scoped_guard(rwsem_write, &cxl_rwsem.region)
> +		rc = cxl_do_reset_locked(&ctx, mem_clear);
> +
> +out:
> +	cxl_reset_context_destroy(&ctx);
>  	return rc;
>  }

[Severity: Medium]
Is it intended to mix goto-based error handling and scope-based cleanup
helpers in the same function?

This function uses "goto out" for early error paths, but then uses
scoped_guard() below. The cleanup subsystem guidelines in
include/linux/cleanup.h state that goto and cleanup helpers should never be
mixed in the same function, as it can create confusing ownership semantics
and increase the risk of double-free or resource leak bugs.

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