Re: [PATCH v16 15/18] PCI/CXL: Restore CXL state after PCI reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] PCI/CXL: missing __must_hold(cxl_region_quiesce) on __cxl_reset_execute()
--

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

PCI/CXL: Restore CXL state after PCI reset

This commit restores the PCI configuration needed for HDM access, cached HDM
decoder settings, and CXL Device DVSEC state after a CXL device reset. It also
handles restoration failures by disabling the device in the saved PCI command
state.

> diff --git a/drivers/pci/cxl.c b/drivers/pci/cxl.c
> index 9a298e90586da..58bbd5b47539f 100644
> --- a/drivers/pci/cxl.c
> +++ b/drivers/pci/cxl.c

[ ... ]

> @@ -232,6 +233,162 @@ void pci_cxl_hdm_cache_release(struct pci_dev *pdev)

[ ... ]

> +/* Caller holds the region lock and keeps device traffic isolated. */
> +static int pci_cxl_restore_state(struct pci_dev *pdev,
> +			  const struct cxl_hdm_info *snapshot, u16 *command)
> +	__must_hold(cxl_region_quiesce)
> +{
> +	device_lock_assert(&pdev->dev);
> +	cxl_restore_pci_state_for_hdm_restore(pdev, command);
> +	return cxl_restore_state(pdev, snapshot);
> +}
> +
>  /*
>   * 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
> @@ -412,7 +569,9 @@ static int cxl_clear_memory(struct pci_dev *pdev, int dvsec, bool initiate)
>  }
>  
>  static int __cxl_reset_execute(struct pci_dev *pdev, int dvsec, u16 cap,
> -			      struct cxl_hdm_quiesce *q)
> +			      const struct cxl_hdm_info *snapshot,
> +			      struct cxl_hdm_quiesce *q,
> +			      u16 *command)
>  {
>  	int rc, rc2;
>  
> @@ -437,10 +596,15 @@ static int __cxl_reset_execute(struct pci_dev *pdev, int dvsec, u16 cap,
>  		pci_err(pdev, "failed to clear CXL Reset Memory Clear: %d\n", rc2);
>  	rc = rc ?: rc2;
>  
> +	if (!rc)
> +		rc = pci_cxl_restore_state(pdev, snapshot, command);
> +
>  	rc2 = cxl_hdm_quiesce_flush(q);

[Severity: Low]
Does __cxl_reset_execute() also need the __must_hold(cxl_region_quiesce)
annotation to avoid compiler context-analysis (-Wthread-safety) warnings when
context analysis is enabled?

Both its caller cxl_reset_quiesced() and its callees pci_cxl_restore_state()
and cxl_hdm_quiesce_flush() are annotated with
__must_hold(cxl_region_quiesce), while __cxl_reset_execute() itself is not:

  cxl_reset_execute()
    cxl_region_quiesce_lock()
    cxl_reset_quiesced()                [__must_hold(cxl_region_quiesce)]
      __cxl_reset_execute()             [unannotated]
        pci_cxl_restore_state()         [__must_hold(cxl_region_quiesce)]
        cxl_hdm_quiesce_flush()         [__must_hold(cxl_region_quiesce)]
    cxl_region_quiesce_unlock()

Relatedly, in include/cxl/hdm.h:

  void cxl_region_quiesce_lock(void);
  void cxl_region_quiesce_unlock(void);

the prototypes omit the __acquires(cxl_region_quiesce) and
__releases(cxl_region_quiesce) annotations that are present on the definitions
in drivers/cxl/core/hdm_state.c, which also prevents context analysis from
seeing the lock acquisition in cxl_reset_execute().

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