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

[Alexey Kardashevskiy <aik@amd.com>]

On 17/6/26 03:34, Dan Williams (nvidia) wrote:
> Lukas Wunner wrote:
>> 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!
> 
> Thanks, Lukas. A few questions:
> 
> This says Fixes, but I assume it is based on inspection and not a
> report?
> 
> There are no upstream usages of pci_tsm_doe_transfer() yet, but the ones

I am not convinced this is a very useful helper but I'll post a patch to use that instead of calling pci_doe() directly :)

> in flight would suffer from the "D0 -> D3hot -> D0 -> D3hot" bounce that
> you described to sashiko. I.e. the runtime acquire should be done at a
> higher level.
> 
> I think the natural place to add PM_RUNTIME_ACQUIRE() that covers all
> cases is withing pci_tsm_connect() and pci_tsm_disconnect().

Agree, although the AMD chunk of the patch would work too.


> I also think failure to power manage the device in the disconnect path
> should not be fatal to performing the rest of the cleanup.

+1.


-- 
Alexey