diff mbox series

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

Message ID 20240815085725.2740390-11-quic_mdalam@quicinc.com (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show
Series Add cmd descriptor support | expand

Commit Message

Md Sadre Alam Aug. 15, 2024, 8:57 a.m. UTC
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.

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(+)

Comments

Bjorn Andersson Aug. 16, 2024, 4:38 p.m. UTC | #1
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?

Please squash this with the previous commit, if kept as explicit
operations, please squash it with the previous patch that introduces the
flags.

> 
> 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.

> +
> +	/* 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?

> +{
> +	int ret;
> +
> +	qce_clear_bam_transaction(qce);
> +

In particularly not on "unlock".

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
>
Md Sadre Alam Aug. 21, 2024, 5:14 a.m. UTC | #2
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
>>
diff mbox series

Patch

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);
+
+	/* 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)
+{
+	int ret;
+
+	qce_clear_bam_transaction(qce);
+
+	/* 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_ */