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

[Lukas Wunner <lukas@wunner.de>]

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