Re: [PATCH v9 08/11] cxl: Restore CXL HDM state after PCI reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware PCI BARs are not restored before attempting HDM MMIO access post-reset, causing the restore to fail silently or trigger bus errors.
- [Medium] Mixing `goto`-based error handling with scope-based cleanups (`__free()` and `scoped_guard()`) in the same function.
--

--- Patch [8]: [PATCH v9 08/11] cxl: Restore CXL HDM state after PCI reset ---
commit a1cce28cbb5bb68a25127de7ce247a2950193943
Author: Srirangan Madhavan <smadhavan@nvidia.com>

cxl: Restore CXL HDM state after PCI reset

After CXL reset, restore PCI config state enough to reach HDM MMIO,
restore cached global and per-decoder HDM state, and then run the normal
PCI restore callbacks.

> diff --git a/drivers/cxl/core/reset.c b/drivers/cxl/core/reset.c
> --- a/drivers/cxl/core/reset.c
> +++ b/drivers/cxl/core/reset.c
> @@ -455,6 +531,164 @@ void pci_cxl_hdm_init(struct pci_dev *pdev)

[ ... ]

> +static int cxl_restore_hdm(struct pci_dev *pdev)
> +{
> +	struct cxl_hdm_decoder_state *decoder_state __free(kfree) = NULL;
> +	struct cxl_decoder_settings *settings __free(kfree) = NULL;

[ ... ]

> +	hdm = cxl_pci_hdm_ioremap_current(pdev, hdm_bar, hdm_offset, hdm_size);
> +	if (IS_ERR(hdm)) {
> +		first_rc = PTR_ERR(hdm);
> +		goto out_restore_command;
> +	}

[Severity: Medium]
Is it safe to mix goto-based error handling with scope-based cleanups
like __free() in this function? 

The kernel cleanup guidelines suggest converting all resources that need a 
goto cleanup to scope-based cleanup, or converting none of them, because 
mixing them creates confusing ownership semantics.

> +
> +	/*
> +	 * Restore global HDM control before per-decoder commit. PCI config
> +	 * memory decoding is enabled for MMIO access, but IOMMU reset blocks
> +	 * remain active until HDM restore completes.
> +	 */
> +	writel(global_ctrl, hdm + CXL_HDM_DECODER_CTRL_OFFSET);

[Severity: High]
Will this MMIO access fail or trigger bus errors? 

The CXL reset clears the device's PCI configuration space, which sets the 
hardware BARs to 0. Although cxl_hdm_enable_mem() sets the 
PCI_COMMAND_MEMORY bit earlier in this function, it doesn't appear to write 
the physical address back into the hardware BARs before we attempt to access 
the MMIO region here.

[ ... ]

>  int cxl_reset_function(struct pci_dev *pdev, bool probe)
>  {

[ ... ]

>  	cxl_hdm_range_context_init(&range_ctx);
>  
> +	if (!cxl_reset_scope_hdm_available(&ctx)) {
> +		rc = -ENOTTY;
> +		goto out;
> +	}
> +
> +	rc = cxl_pci_target_reset_prepare(&ctx);
> +	if (rc)
> +		goto out;
> +
>  	scoped_guard(rwsem_write, &cxl_rwsem.region) {

[Severity: Medium]
Could we also avoid mixing the legacy goto error paths with the 
scoped_guard() block in this function?

> -		rc = cxl_hdm_ranges_prepare(&range_ctx, pdev);
> +		rc = cxl_hdm_ranges_prepare(&range_ctx, &ctx);
>  		if (!rc)
>  			rc = cxl_reset_execute(pdev, dvsec);
> +		if (!rc)
> +			rc = cxl_restore_hdm_decoders(&ctx);
>  	}
> +
> +	cxl_pci_target_reset_done(&ctx);
> +out:
>  	cxl_hdm_range_context_destroy(&range_ctx);
>  	return rc;
>  }

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