Re: [PATCH 6/7] staging: ccree: add DT bus coherency detection

[Gilad Ben-Yossef]

On Thu, Jun 22, 2017 at 12:04 PM, Dan Carpenter
<dan.carpenter@xxxxxxxxxx> wrote:
> On Thu, Jun 22, 2017 at 10:07:52AM +0300, Gilad Ben-Yossef wrote:
>> The ccree driver has build time configurable support
>> to work on top of coherent (e.g. ACP) vs. none coherent bus
>> connections. Turn it to run-time configurable option
>> based on device tree.
>>
>> Signed-off-by: Gilad Ben-Yossef <gilad@xxxxxxxxxxxxx>
>> ---
>>  drivers/staging/ccree/ssi_buffer_mgr.c | 37 ++++++++++++++++++----------------
>>  drivers/staging/ccree/ssi_config.h     | 20 ------------------
>>  drivers/staging/ccree/ssi_driver.c     | 14 +++++++++----
>>  drivers/staging/ccree/ssi_driver.h     |  3 +++
>>  4 files changed, 33 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c b/drivers/staging/ccree/ssi_buffer_mgr.c
>> index 88ebda8..75810dc 100644
>> --- a/drivers/staging/ccree/ssi_buffer_mgr.c
>> +++ b/drivers/staging/ccree/ssi_buffer_mgr.c
>> @@ -627,6 +627,9 @@ void ssi_buffer_mgr_unmap_aead_request(
>>       struct aead_req_ctx *areq_ctx = aead_request_ctx(req);
>>       unsigned int hw_iv_size = areq_ctx->hw_iv_size;
>>       struct crypto_aead *tfm = crypto_aead_reqtfm(req);
>> +     struct ssi_drvdata *drvdata =
>> +             (struct ssi_drvdata *)dev_get_drvdata(dev);
>
> The cast is not needed.
>
>> +
>
> Don't put a blank in the middle of the declaration block.
>
>>       u32 dummy;
>>       bool chained;
>>       u32 size_to_unmap = 0;

Will fix.

>
> [ snip ]
>
>> @@ -981,20 +983,22 @@ static inline int ssi_buffer_mgr_prepare_aead_data_mlli(
>>                        * MAC verification upon request completion
>>                        */
>>                       if (direct == DRV_CRYPTO_DIRECTION_DECRYPT) {
>> -#if !DX_HAS_ACP
>> -                             /* In ACP platform we already copying ICV
>> -                              * for any INPLACE-DECRYPT operation, hence
>> +                             if (!drvdata->coherent) {
>> +                             /* In coherent platforms (e.g. ACP)
>> +                              * already copying ICV for any
>> +                              * INPLACE-DECRYPT operation, hence
>>                                * we must neglect this code.
>>                                */
>> -                             u32 size_to_skip = req->assoclen;
>> -                             if (areq_ctx->is_gcm4543) {
>> -                                     size_to_skip += crypto_aead_ivsize(tfm);
>> +                                     u32 skip = req->assoclen;
>> +
>> +                                     if (areq_ctx->is_gcm4543)
>> +                                             skip += crypto_aead_ivsize(tfm);
>> +
>> +                                     ssi_buffer_mgr_copy_scatterlist_portion(
>> +areq_ctx->backup_mac, req->src,
>> +(skip + req->cryptlen - areq_ctx->req_authsize),
>> +skip + req->cryptlen, SSI_SG_TO_BUF);
>
>
> Indenting is messed up.

Sigh... having a function with a name as long as
ssi_buffer_mgr_copy_scatterlist_portion()
with such a deep indentation level is what is really messed up (but
that is for another patch
to fix).

I will indent it more sanely.
>
>>                               }
>> -                             ssi_buffer_mgr_copy_scatterlist_portion(
>> -                                     areq_ctx->backup_mac, req->src,
>> -                                     size_to_skip+ req->cryptlen - areq_ctx->req_authsize,
>> -                                     size_to_skip+ req->cryptlen, SSI_SG_TO_BUF);
>> -#endif
>>                               areq_ctx->icv_virt_addr = areq_ctx->backup_mac;
>>                       } else {
>>                               areq_ctx->icv_virt_addr = areq_ctx->mac_buf;
>
> [ snip ]
>
>> @@ -533,7 +539,7 @@ int cc_clk_on(struct ssi_drvdata *drvdata)
>>       struct clk *clk = drvdata->clk;
>>
>>       if (IS_ERR(clk))
>> -     /* No all devices have a clock associated with CCREE */
>> +     /* Not all devices have a clock associated with CCREE */
>
> This is not related.  Do unrelated things in different patches.  This
> typo was introduced in an earlier patch.  There are a couple ways in git
> to edit previous patches.

Yes, this was not intended.


>
>>               goto out;
>>
>>       rc = clk_prepare_enable(clk);
>
> regards,
> dan carpenter

Thanks for your time and great review comments!

Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru