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

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

[Lukas Wunner]

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.

Note that for plain DOE operation, it is sufficient for the device to be
in D3hot and its parents in D0 because config space remains accessible in
D3hot.  So CMA-SPDM goes beyond the requirements of plain DOE and hence
resuming to D0 needs to (only) be done in code paths which use DOE
specifically for CMA-SPDM.

The pattern used herein for runtime resume is the best practice introduced
by commit ef8057b07c72 ("PM: runtime: Wrapper macros for ACQUIRE()/
ACQUIRE_ERR()").

Fixes: 3225f52cde56 ("PCI/TSM: Establish Secure Sessions and Link Encryption")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: stable@vger.kernel.org # v6.19+
Cc: Vivaik Balasubrawmanian <vivaik.balasubrawmanian@intel.com>
---
We're in the merge window for v7.2 and this isn't super urgent,
so it's targeting v7.3 via tsm.git/next.

Technically I'd have permission to apply myself,
but I wouldn't want to without acks from Dan and AMD!
Thanks for taking a look!

 drivers/crypto/ccp/sev-dev-tsm.c | 6 ++++++
 drivers/pci/tsm.c                | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/crypto/ccp/sev-dev-tsm.c b/drivers/crypto/ccp/sev-dev-tsm.c
index b07ae52..108204f7 100644
--- a/drivers/crypto/ccp/sev-dev-tsm.c
+++ b/drivers/crypto/ccp/sev-dev-tsm.c
@@ -7,6 +7,7 @@
 #include <linux/tsm.h>
 #include <linux/iommu.h>
 #include <linux/pci-doe.h>
+#include <linux/pm_runtime.h>
 #include <linux/bitfield.h>
 #include <linux/module.h>
 
@@ -30,6 +31,7 @@ static int sev_tio_spdm_cmd(struct tio_dsm *dsm, int ret)
 {
 	struct tsm_dsm_tio *dev_data = &dsm->data;
 	struct tsm_spdm *spdm = &dev_data->spdm;
+	int pm_ret;
 
 	/* Check the main command handler response before entering the loop */
 	if (ret == 0 && dev_data->psp_ret != SEV_RET_SUCCESS)
@@ -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;
+
 	/* ret > 0 means "SPDM requested" */
 	while (ret == PCI_DOE_FEATURE_CMA || ret == PCI_DOE_FEATURE_SSESSION) {
 		ret = pci_doe(dsm->tsm.doe_mb, PCI_VENDOR_ID_PCI_SIG, ret,
diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
index 5fdcd7f..af1817e 100644
--- a/drivers/pci/tsm.c
+++ b/drivers/pci/tsm.c
@@ -12,6 +12,7 @@
 #include <linux/pci.h>
 #include <linux/pci-doe.h>
 #include <linux/pci-tsm.h>
+#include <linux/pm_runtime.h>
 #include <linux/sysfs.h>
 #include <linux/tsm.h>
 #include <linux/xarray.h>
@@ -886,6 +887,7 @@ int pci_tsm_doe_transfer(struct pci_dev *pdev, u8 type, const void *req,
 			 size_t req_sz, void *resp, size_t resp_sz)
 {
 	struct pci_tsm_pf0 *tsm;
+	int rc;
 
 	if (!pdev->tsm || !is_pci_tsm_pf0(pdev))
 		return -ENXIO;
@@ -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;
+
 	return pci_doe(tsm->doe_mb, PCI_VENDOR_ID_PCI_SIG, type, req, req_sz,
 		       resp, resp_sz);
 }
-- 
2.53.0

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

[sashiko-bot]

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

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

[Lukas Wunner]

On Mon, Jun 15, 2026 at 01:42:52PM +0000, sashiko-bot@kernel.org wrote:
> > +++ 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?

Moving PM_RUNTIME_ACQUIRE() inside the while loop may lead to repeated
D0 -> D3hot -> D0 -> D3hot ... transitions (depending on autosuspend
settings of the device, which are user-configurable through sysfs).
It would also lead to overhead induced by runtime PM code (repeated
spinlock acquisition etc).

So I believe keeping PM_RUNTIME_ACQUIRE() outside the while loop is
the right thing to do, but I'll leave this to AMD engineers to decide.

> [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?

If the device is deleted anyway, we don't care about leaked references.
And we absolutely do not want to synchronously runtime suspend here.

> > +++ 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.

Hallucination, this code does not perform "generic DOE" exchanges, only
CMA-SPDM ones.

> [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.

We have to leave de-enumerated devices in D0 to ensure that a subsequent
rescan successfully re-enumerates them.  E.g. leaving a Downstream Port
in D3hot upon de-enumeration would leave any children inaccessible.

We also leave unbound devices in D0 for similar reasons.

Thanks,

Lukas