Re: [PATCH 12/13] crypto: hisilicon/zip - Use temporary sqe when doing work

[Zhou Wang <wangzhou1@hisilicon.com>]

On 2020/5/9 11:42, Song Bao Hua wrote:
>> -----Original Message-----
>> From: linux-crypto-owner@vger.kernel.org [mailto:linux-crypto-owner@vger.kernel.org] On Behalf Of Shukun Tan
>> Sent: Friday, May 8, 2020 6:58 PM
>> To: herbert@gondor.apana.org.au; davem@davemloft.net
>> Cc: linux-crypto@vger.kernel.org; Xu Zaibo <xuzaibo@huawei.com>; Wangzhou (B) <wangzhou1@hisilicon.com>
>> Subject: [PATCH 12/13] crypto: hisilicon/zip - Use temporary sqe when doing work
> 
>> From: Zhou Wang <wangzhou1@hisilicon.com>
> 
>> Currently zip sqe is stored in hisi_zip_qp_ctx, which will bring corruption with multiple parallel users of the crypto tfm.
> 
>> This patch removes the zip_sqe in hisi_zip_qp_ctx and uses a temporary sqe instead.
> 
> This looks like a quite correct fix as in the old code, the 2nd request will overwrite the zip_sqe of the 1st request if we send the 2nd request before the 1st one is completed.
> So this will lead to the mistakes of both request1 and request2.

Yes.

> 
> unfortunately, it seems the hang issue can still be reproduced with this patch applied if we ask multi-threads running on multi-cores to send requests in parallel. Maybe something more needs fix?

Currently we do not support multi-threads using one crypto_acomp in this driver.
If you tested like this, it may go wrong, as we do not protect queue which is in zip ctx.

Best,
Zhou

> 
>> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Signed-off-by: Shukun Tan <tanshukun1@huawei.com>
>> ---
>>  drivers/crypto/hisilicon/zip/zip_crypto.c | 11 +++++------
>>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
>> diff --git a/drivers/crypto/hisilicon/zip/zip_crypto.c b/drivers/crypto/hisilicon/zip/zip_crypto.c
>> index 369ec32..5fb9d4b 100644
>> --- a/drivers/crypto/hisilicon/zip/zip_crypto.c
>> +++ b/drivers/crypto/hisilicon/zip/zip_crypto.c
>> @@ -64,7 +64,6 @@ struct hisi_zip_req_q {
>>  
>>  struct hisi_zip_qp_ctx {
>>  	struct hisi_qp *qp;
>> -	struct hisi_zip_sqe zip_sqe;
>>  	struct hisi_zip_req_q req_q;
>>  	struct hisi_acc_sgl_pool *sgl_pool;
>>  	struct hisi_zip *zip_dev;
>> @@ -484,11 +483,11 @@ static struct hisi_zip_req *hisi_zip_create_req(struct acomp_req *req,  static int hisi_zip_do_work(struct hisi_zip_req *req,
>>  			    struct hisi_zip_qp_ctx *qp_ctx)
>>  {
>> -	struct hisi_zip_sqe *zip_sqe = &qp_ctx->zip_sqe;
>>  	struct acomp_req *a_req = req->req;
>>  	struct hisi_qp *qp = qp_ctx->qp;
>>  	struct device *dev = &qp->qm->pdev->dev;
>>  	struct hisi_acc_sgl_pool *pool = qp_ctx->sgl_pool;
>> +	struct hisi_zip_sqe zip_sqe;
>>  	dma_addr_t input;
>>  	dma_addr_t output;
>>  	int ret;
>> @@ -511,13 +510,13 @@ static int hisi_zip_do_work(struct hisi_zip_req *req,
>>  	}
>>  	req->dma_dst = output;
>>  
>> -	hisi_zip_fill_sqe(zip_sqe, qp->req_type, input, output, a_req->slen,
>> +	hisi_zip_fill_sqe(&zip_sqe, qp->req_type, input, output, a_req->slen,
>>  			  a_req->dlen, req->sskip, req->dskip);
>> -	hisi_zip_config_buf_type(zip_sqe, HZIP_SGL);
>> -	hisi_zip_config_tag(zip_sqe, req->req_id);
>> +	hisi_zip_config_buf_type(&zip_sqe, HZIP_SGL);
>> +	hisi_zip_config_tag(&zip_sqe, req->req_id);
>>  
>>  	/* send command to start a task */
>> -	ret = hisi_qp_send(qp, zip_sqe);
>> +	ret = hisi_qp_send(qp, &zip_sqe);
>>  	if (ret < 0)
>>  		goto err_unmap_output;
>>  
> 
> Best Regards
> Barry
> 
> .
>