Re: [PATCH] PCI/TSM: Resume device to D0 for CMA-SPDM operation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Asynchronous PM runtime put during device removal path leaves hardware stuck in D0.
- [Medium] The patch unconditionally resumes the device to D0 for any DOE transfer type, contradicting the commit message's stated power management requirement.
--

commit ef77945b44ebbc6f448bd2fb94eac66fe80392e2
Author: Lukas Wunner <lukas@wunner.de>

PCI/TSM: Resume device to D0 for CMA-SPDM operation

Per PCIe r7.0 sec 6.31.3, CMA-SPDM operation in non-D0 states is optional.
The spec does not define a way to determine if it's supported, so resume
to D0 unconditionally for the duration of a CMA-SPDM exchange. Vivaik has
talked to Windows engineers and they said that Windows does the same.

> diff --git a/drivers/crypto/ccp/sev-dev-tsm.c b/drivers/crypto/ccp/sev-dev-tsm.c
> index 46f2539d2d5ab..d99b835d2b7c6 100644
> --- a/drivers/crypto/ccp/sev-dev-tsm.c
> +++ b/drivers/crypto/ccp/sev-dev-tsm.c

[ ... ]

> @@ -38,6 +40,10 @@ static int sev_tio_spdm_cmd(struct tio_dsm *dsm, int ret)
>  	if (ret <= 0)
>  		return ret;
>  
> +	PM_RUNTIME_ACQUIRE(&dsm->tsm.base_tsm.pdev->dev, pm);
> +	if ((pm_ret = PM_RUNTIME_ACQUIRE_ERR(&pm)))
> +		return pm_ret;
> +

[Severity: Medium]
The commit message mentions that "resuming to D0 needs to (only) be done in
code paths which use DOE specifically for CMA-SPDM", but does this code
unconditionally resume the device to D0 regardless of the operation?

Looking at sev_tio_spdm_cmd(), the resume happens before the loop checks if
the operation is actually PCI_DOE_FEATURE_CMA or PCI_DOE_FEATURE_SSESSION.
Could this cause unnecessary power transitions for generic operations where
accessing the config space in D3hot would be sufficient?

[Severity: High]
Does using PM_RUNTIME_ACQUIRE() here risk leaving the hardware permanently
stuck in D0 during the device removal path?

The PM_RUNTIME_ACQUIRE() macro queues an asynchronous pm_runtime_put() when
dropping the reference. If sev_tio_spdm_cmd() is called during teardown
sequences, the pending asynchronous suspend can be cancelled by a subsequent
device_del() call, which disables runtime PM. 

Should a synchronous runtime put be used here to ensure the device actually
suspends before the removal completes?

>  	/* ret > 0 means "SPDM requested" */
>  	while (ret == PCI_DOE_FEATURE_CMA || ret == PCI_DOE_FEATURE_SSESSION) {

[ ... ]

> diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> index 5fdcd7f2e8207..af1817e86b30d 100644
> --- a/drivers/pci/tsm.c
> +++ b/drivers/pci/tsm.c

[ ... ]

> @@ -894,6 +896,10 @@ int pci_tsm_doe_transfer(struct pci_dev *pdev, u8 type, const void *req,
>  	if (!tsm->doe_mb)
>  		return -ENXIO;
>  
> +	PM_RUNTIME_ACQUIRE(&pdev->dev, pm);
> +	if ((rc = PM_RUNTIME_ACQUIRE_ERR(&pm)))
> +		return rc;
> +

[Severity: Medium]
Similarly to sev_tio_spdm_cmd(), does this code unconditionally resume the
device to D0, ignoring the type parameter?

If a generic DOE transfer is requested that does not strictly require D0, it
appears this will still force a transition to D0, contradicting the commit
message's stated power management requirements.

[Severity: High]
Is this exported API also susceptible to the same asynchronous put regression
mentioned above?

If pci_tsm_doe_transfer() is invoked during a teardown sequence, the async
idle and suspend queued by the macro might get canceled by device_del(),
leaving the device in D0.

>  	return pci_doe(tsm->doe_mb, PCI_VENDOR_ID_PCI_SIG, type, req, req_sz,
>  		       resp, resp_sz);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/7bdfaf14d7e5a466f3f650150c688a60e947a7a9.1781527060.git.lukas@wunner.de?part=1