Re: [PATCH v1 7/8] crypto: ccp - Skip DMA coherency check for platform psp

[Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>]

On 31/01/2023 21:42, Tom Lendacky wrote:
> On 1/23/23 09:22, Jeremi Piotrowski wrote:
>> The value of device_get_dma_attr() is only relevenat for ARM64 and CCP
>> devices to configure the value of the axcache attribute used to access
>> memory by the coprocessor. None of this applies to the platform psp so
>> skip it.
>>
>> Signed-off-by: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
>> ---
>>   drivers/crypto/ccp/sp-platform.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/crypto/ccp/sp-platform.c b/drivers/crypto/ccp/sp-platform.c
>> index 281dbf6b150c..b74f16e0e963 100644
>> --- a/drivers/crypto/ccp/sp-platform.c
>> +++ b/drivers/crypto/ccp/sp-platform.c
>> @@ -29,6 +29,7 @@
>>   struct sp_platform {
>>       int coherent;
>>       unsigned int irq_count;
>> +    bool is_platform;
> 
> s/is_platform/is_platform_device/
> 

ok

>>   };
>>     #ifdef CONFIG_CRYPTO_DEV_SP_PSP
>> @@ -190,8 +191,10 @@ static int sp_platform_probe(struct platform_device *pdev)
>>       sp->dev_specific = sp_platform;
>>       sp->dev_vdata = pdev->dev.of_node ? sp_get_of_version(pdev)
>>                        : sp_get_acpi_version(pdev);
>> -    if (!sp->dev_vdata && pdev->id_entry)
>> +    if (!sp->dev_vdata && pdev->id_entry) {
>> +        sp_platform->is_platform = true;
> 
> Move this into the sp_get_plat_version() function.
> 
>>           sp->dev_vdata = sp_get_plat_version(pdev);
> 
> And I probably should have made this comment in the previous patch, but you should probably spell out platform here.
>

ok (i had done that before i got to this comment for consistency)

>> +    }
>>       if (!sp->dev_vdata) {
>>           ret = -ENODEV;
>>           dev_err(dev, "missing driver data\n");
>> @@ -205,7 +208,7 @@ static int sp_platform_probe(struct platform_device *pdev)
>>       }
>>         attr = device_get_dma_attr(dev);
>> -    if (attr == DEV_DMA_NOT_SUPPORTED) {
>> +    if (!sp_platform->is_platform && attr == DEV_DMA_NOT_SUPPORTED) {
> 
> Just a nit but I'd prefer to see this as:
> 
>     if (attr == DEV_DMA_NOT_SUPPORTED && !sp_platform->is_platform) {
> 
> The diff is easier to see that way.

makes sense

> 
> Thanks,
> Tom
> 
>>           dev_err(dev, "DMA is not supported");
>>           goto e_err;
>>       }