Re: [PATCH v2 10/16] crypto: qce - Add support for lock aquire,lock release api.

Md Sadre Alam
2024-08-21T05:14:22Z

On 8/16/2024 10:08 PM, Bjorn Andersson wrote:
> On Thu, Aug 15, 2024 at 02:27:19PM GMT, Md Sadre Alam wrote:
>> Add support for lock acquire and lock release api.
>> When multiple EE's(Execution Environment) want to access
>> CE5 then there will be race condition b/w multiple EE's.
>>
>> Since each EE's having their dedicated BAM pipe, BAM allows
>> Locking and Unlocking on BAM pipe. So if one EE's requesting
>> for CE5 access then that EE's first has to LOCK the BAM pipe
>> while setting LOCK bit on command descriptor and then access
>> it. After finishing the request EE's has to UNLOCK the BAM pipe
>> so in this way we race condition will not happen.
>
> Does the lock/unlock need to happen on a dummy access before and after
> the actual sequence? Is it not sufficient to lock/unlock on the first
> and last operation?
The locking/unlocking has to happen on command descriptor only, If we
need locking/unlocking on data descriptor then we have to use dummy
command descriptor only as per Hardware Programming Guide.

Hardware Programming Guide state as below:
Pipe Locking and Unlocking should appear ONLY in Command-Descriptor.
In case a Lock is required on a Data Descriptor this can be implemented
by a dummy Command-Descriptor with Lock/Unlock bit asserted preceding/
following the Data Descriptor.
>
> Please squash this with the previous commit, if kept as explicit
> operations, please squash it with the previous patch that introduces the
> flags.
Ok
>
>>
>> Added these two API qce_bam_acquire_lock() and qce_bam_release_lock()
>> for the same.
>>
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
>> ---
>>
>> Change in [v2]
>>
>> * No chnage
>>
>> Change in [v1]
>>
>> * Added initial support for lock_acquire and lock_release
>> api.
>>
>> drivers/crypto/qce/common.c | 36 ++++++++++++++++++++++++++++++++++++
>> drivers/crypto/qce/core.h | 2 ++
>> 2 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c
>> index ff96f6ba1fc5..a8eaffe41101 100644
>> --- a/drivers/crypto/qce/common.c
>> +++ b/drivers/crypto/qce/common.c
>> @@ -617,3 +617,39 @@ void qce_get_version(struct qce_device *qce, u32 *major, u32 *minor, u32 *step)
>> *minor = (val & CORE_MINOR_REV_MASK) >> CORE_MINOR_REV_SHIFT;
>> *step = (val & CORE_STEP_REV_MASK) >> CORE_STEP_REV_SHIFT;
>> }
>> +
>> +int qce_bam_acquire_lock(struct qce_device *qce)
>> +{
>> + int ret;
>> +
>> + qce_clear_bam_transaction(qce);
>
> It's not entirely obvious that a "lock" operation will invalidate any
> pending operations.
qce_clear_bam_transaction() api is not going to invalidate any pending
thransaction. This is just an internal api which will set bam_transaction
structure to 0 before starting new bam transaction.
>
>> +
>> + /* This is just a dummy write to acquire lock on bam pipe */
>> + qce_write_reg_dma(qce, REG_AUTH_SEG_CFG, 0, 1);
>> +
>> + ret = qce_submit_cmd_desc(qce, QCE_DMA_DESC_FLAG_LOCK);
>> + if (ret) {
>> + dev_err(qce->dev, "Error in Locking cmd descriptor\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int qce_bam_release_lock(struct qce_device *qce)
>
> What would be a reasonable response from the caller if this release
> operation returns a failure? How do you expect it to recover?
If unlocking bam pipe failed means its a bam failure and we can abort
the current transaction.
>
>> +{
>> + int ret;
>> +
>> + qce_clear_bam_transaction(qce);
>> +
>
> In particularly not on "unlock".
qce_clear_bam_transaction() this is just to initialize with 0
for bam transaction structure before any new transaction start.
>
> Regards,
> Bjorn
>
>> + /* This just dummy write to release lock on bam pipe*/
>> + qce_write_reg_dma(qce, REG_AUTH_SEG_CFG, 0, 1);
>> +
>> + ret = qce_submit_cmd_desc(qce, QCE_DMA_DESC_FLAG_UNLOCK);
>> + if (ret) {
>> + dev_err(qce->dev, "Error in Un-Locking cmd descriptor\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> diff --git a/drivers/crypto/qce/core.h b/drivers/crypto/qce/core.h
>> index bf28dedd1509..d01d810b60ad 100644
>> --- a/drivers/crypto/qce/core.h
>> +++ b/drivers/crypto/qce/core.h
>> @@ -68,4 +68,6 @@ int qce_read_reg_dma(struct qce_device *qce, unsigned int offset, void *buff,
>> void qce_clear_bam_transaction(struct qce_device *qce);
>> int qce_submit_cmd_desc(struct qce_device *qce, unsigned long flags);
>> struct qce_bam_transaction *qce_alloc_bam_txn(struct qce_dma_data *dma);
>> +int qce_bam_acquire_lock(struct qce_device *qce);
>> +int qce_bam_release_lock(struct qce_device *qce);
>> #endif /* _CORE_H_ */
>> --
>> 2.34.1
>>