Received-SPF: pass (google.com: domain of linux-kernel+bounces-59857-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <a9b139fe-832d-889b-3d1a-2122e9c26a11@amd.com>
Date: Fri, 9 Feb 2024 13:05:51 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH 1/2] crypto: ccp: Avoid discarding errors in
 psp_send_platform_access_msg()
Content-Language: en-US
To: Mario Limonciello <mario.limonciello@amd.com>,
 Herbert Xu <herbert@gondor.apana.org.au>
Cc: John Allen <john.allen@amd.com>,
 "open list:AMD CRYPTOGRAPHIC COPROCESSOR (CCP) DRIVER - DB..."
 <linux-crypto@vger.kernel.org>, open list <linux-kernel@vger.kernel.org>,
 Tim Van Patten <timvp@google.com>
References: <20240207215439.95871-1-mario.limonciello@amd.com>
From: Tom Lendacky <thomas.lendacky@amd.com>
In-Reply-To: <20240207215439.95871-1-mario.limonciello@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?ejU3OGdsTG9EcFZkUFE0VlF3bVhFaGh1Ymt6Y05LY0IwQTFUS0t3QThqVVhZ?=
 =?utf-8?B?cnp0WHRNdmF0d3Y0QUdOK0tBWWZpdUdyR2VXb01qaCtOdldPZXVJZUQxOVFF?=
 =?utf-8?B?REJkQTlabTdLaXQ2MGpkSUdBak5OeUk4UTVvT2lZRUJXQSsxSFlVT3REVEd4?=
 =?utf-8?B?MER1c0JqUmtoWjc2RHJscEQ0bWIzN0xGU2V5R0hRSzdMQmRDckY1Zy9sOW9T?=
 =?utf-8?B?cW5NR0lNS1NNeUFzRnBpc1haaXVRN2JTZkNKSVdvbkc1bmlzRGpWSmxBZ1JZ?=
 =?utf-8?B?TkpSSi9TNWtZTGJMZUpISlpWQW1ReEdZcWhwODhTbGRSZFl0VytLOVBjUTlF?=
 =?utf-8?B?YStnZVZSMGUxTHI3VnIvemxvRDY2SkxLWHdodXBNMUxmVzJOd2l3cnBNR3Uz?=
 =?utf-8?B?NnRUb2F5eEl4UlBMMENDUjg4RE53eXFGNHBDSE1ZZnJvVEUxWktRdkFraStw?=
 =?utf-8?B?TEFJcnV6WUtGbjREWXhST1MzWGM1c2krMkJLY2tqUUV5K0g0VHEzQzVJYXBj?=
 =?utf-8?B?RlpCUUVML01vVmJFVGdoWHU5RGxhK2pjZjNIb0lVV1MvamUrMjZ1ZUg3TTZY?=
 =?utf-8?B?bnVRdXdSdXB5N3htdjBFSXBmYUFJd2c4UE5FbUl0Qm40SEpRU2xWYU1vNmxs?=
 =?utf-8?B?NUFNOWpvakFMVkEycEhBKzBBZURHVFlnUmp3elhuRXVxTlkzbVdwWkZzTjM5?=
 =?utf-8?B?WkRyWWJQZjZWZktncnpZTXJGTzZ0a3BTR01EclJ2TGxsaGxWRFdlVUFPYlgy?=
 =?utf-8?B?NDBET2Y4eWxxL3lnbDE2MDBFeWRydVRtWTc4aUV4elFPbGpVa0Uzd3BkWTNy?=
 =?utf-8?B?Qlc5K0NxdVNTSyt0dUxhcVk0UkxUNDJyWUV1SitqaUZTUC96QTlFc01pOWZZ?=
 =?utf-8?B?RllpV2MrSklnbEVjeTM4SUJHYkpaa3k1aDZ0UzhkeThQK3R6TTVNcTFRVDZW?=
 =?utf-8?B?Wis0cWlGcjg3NTd0VmhFK29pUkdMMDVNZSsrazEzQURQRlgyT2NiK3VUUnhB?=
 =?utf-8?B?aGg0RDVXT3lTLytETktlem1TcGRXVFEvaFllZ0Z4MXYxNCs4U2pueUxML3hm?=
 =?utf-8?B?VmwzbXNrT3Z2SnY2Rlp1ek0xOG1tNW1tcERzSVVINE5HTUhOdFVMTVlMbUlU?=
 =?utf-8?B?NWxFdWlTdjhhdzNCQVFuWVk5aVk3bW94em5PM3VwNVpPMHFjU1hnTU9qaFcr?=
 =?utf-8?B?dWc1Q1Nxamh5VWd5YnB2Ykd1UlBZTlN5bTNreGNFc3BpTHVjOVJ0Q3Vtblgy?=
 =?utf-8?B?ZkFTN3FqOVhOaXBhaGd0OC9UVlhmWDRUUXgvNm9ySXR6SEt1OGFNaEhIRGJX?=
 =?utf-8?B?Rm42bDh0Wi9CNDRxYzBMbmNZWm5IbjZIWDlrM1cvdEQvSFE2RGJPd3JoSmtL?=
 =?utf-8?B?aGk0ckJkUmErR0ZiK2pOaDVSY08wb0hLR0s4TS85blRSdm5VZDBzUURYVjRi?=
 =?utf-8?B?VUhhZnJtTmJMaVMyS0tKWlhtRkd4NzZwZU51TFd1WkcyanVMR0lCUmNoZ1Jp?=
 =?utf-8?B?Q0FoK2poZ3NBN1dkSmY1N2MxWk11OEIyWnp1NDI1NkNTSUEzUnllKy8xTDhO?=
 =?utf-8?B?cWpSc2NOYzE0TmJHTTNkSXBpNEdKK3pkSWxDaWFBQnJMNldQNUtNNFJNY2dL?=
 =?utf-8?B?Z2p4QkNoRGwxT251ckRGSzlyd04yVDRUNFVjSU5OdmdCMVl0bGJEcjB0QVov?=
 =?utf-8?B?dWQ3b3RveXRCUWcvZTJkaFRUTldnalhiNFo3SVRodXBLaS9WSEpGS3JhYjgy?=
 =?utf-8?B?TjcxYklUTWcxNk1INmJBYit6elRueDErcWFNL1ZuY0RReUFGbTcrSjllR1R4?=
 =?utf-8?B?eTNGQVgyUXhkc2s5Y2gvSGZDaHVPOVlmVHhhS2V4QUxDdzJ6cElxQWpwMWh1?=
 =?utf-8?B?bmtLa1Q5QlUwQlVvUWp1NTZGd1J4aTdWMllhZnUyUGcvNFZjWGsrR3FINjBt?=
 =?utf-8?B?OGFLOUJLS3RoaEFuV3VXa0J5eStQZmZvZ2VvWHVHVTUyMmh0eHpsUTlrTUNi?=
 =?utf-8?B?RnViU2pyVU1NYTQrQTdoVmhHTGN2clBSWjdKSTNueUZ5U0Nza00yRWorZHFR?=
 =?utf-8?B?UGNwbzJ5SEdyK0pQYmRPQ25HWTlxTkxaaER4TXBWVFkwYSt2U3E3YzM2aE1T?=
 =?utf-8?Q?4CnQJ5uCihguNqp/A87rLms8T?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 527e2cca-67af-4040-071c-08dc29a223e3
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5732.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Feb 2024 19:05:54.4466
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: THmUttEaXxOhAAAOPJiZT0ucEjdcCPV5eykxC825wuuZtzrOsXkJExhap5zyW5X7wUrIuvdhzQ2mjGQg2sXngA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4251

On 2/7/24 15:54, Mario Limonciello wrote:
> When the PSP_CMDRESP_STS field has an error set, the details of the
> error are in `req->header->status`, and the caller will want to look
> at them. But if there is no error set then caller may want to check
> `req->header->status` separately.
> 
> Stop discarding these errors.

This needs a more thorough commit message. It took me a bit to understand 
that the value in the request header could be set to something specific to 
the request even though the command response status comes back as zero.

Which also indicates that the code below should have a nice comment about 
why you only set req->header.status when PSP_CMDRESP_STS is non-zero (in 
other words the existing comment needs to be re-worded).

Thanks,
Tom

> 
> Reported-by: Tim Van Patten <timvp@google.com>
> Fixes: 7ccc4f4e2e50 ("crypto: ccp - Add support for an interface for platform features")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/crypto/ccp/platform-access.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/ccp/platform-access.c b/drivers/crypto/ccp/platform-access.c
> index 94367bc49e35..792ae8d5b11a 100644
> --- a/drivers/crypto/ccp/platform-access.c
> +++ b/drivers/crypto/ccp/platform-access.c
> @@ -120,7 +120,8 @@ int psp_send_platform_access_msg(enum psp_platform_access_msg msg,
>   
>   	/* Store the status in request header for caller to investigate */
>   	cmd_reg = ioread32(cmd);
> -	req->header.status = FIELD_GET(PSP_CMDRESP_STS, cmd_reg);
> +	if (FIELD_GET(PSP_CMDRESP_STS, cmd_reg))
> +		req->header.status = FIELD_GET(PSP_CMDRESP_STS, cmd_reg);
>   	if (req->header.status) {
>   		ret = -EIO;
>   		goto unlock;