Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support

[Gary R Hook]

On 09/07/2017 05:19 PM, Brijesh Singh wrote:

Hi Boris,

On 09/07/2017 09:27 AM, Borislav Petkov wrote:

...



The commit message above reads better to me as the help text than what
you have here.

Also, in order to make it easier for the user, I think we'll need a
CONFIG_AMD_MEM_ENCRYPT_SEV or so and make that depend on CONFIG_KVM_AMD,
this above and all the other pieces that are needed. Just so that when
the user builds such a kernel, all is enabled and not her having to go
look for what else is needed.

And then put the sev code behind that config option. Depending on how
ugly it gets...



I will add more detail in the help text. I will look into adding some
depends.

...


+
+void psp_add_device(struct psp_device *psp)


That function is needlessly global and should be static, AFAICT.

Better yet, it is called only once and its body is trivial so you can
completely get rid of it and meld it into the callsite.



Agreed, will do.

.


+
+static struct psp_device *psp_alloc_struct(struct sp_device *sp)


"psp_alloc()" is enough I guess.



I was trying to adhere to the existing ccp-dev.c function naming
conversion.


I would prefer that we not shorten this. The prior incarnation,
ccp_alloc_struct(), has/had been around for a while. And there are a 
number of
similarly named allocation functions in the driver that we like to keep 
sorted.

If anything, it should be more explanatory, IMO.







static.

Please audit all your functions in the psp pile and make them static if
not needed outside of their compilation unit.



Will do.


+{
+unsigned int status;
+irqreturn_t ret = IRQ_HANDLED;
+struct psp_device *psp = data;


Please sort function local variables declaration in a reverse christmas
tree order:

 longest_variable_name;
 shorter_var_name;
 even_shorter;
 i;



Got it, will do



+
+/* read the interrupt status */
+status = ioread32(psp->io_regs + PSP_P2CMSG_INTSTS);
+
+/* invoke subdevice interrupt handlers */
+if (status) {
+if (psp->sev_irq_handler)
+ret = psp->sev_irq_handler(irq, psp->sev_irq_data);
+if (psp->tee_irq_handler)
+ret = psp->tee_irq_handler(irq, psp->tee_irq_data);
+}
+
+/* clear the interrupt status */
+iowrite32(status, psp->io_regs + PSP_P2CMSG_INTSTS);


We're clearing the status by writing the same value back?!? Shouldn't
that be:

iowrite32(0, psp->io_regs + PSP_P2CMSG_INTSTS);



Actually the SW should write "1" to clear the bit. To make it clear, I
can use value 1 and add comment.




Below I see

iowrite32(0x, psp->io_regs + PSP_P2CMSG_INTSTS);

which is supposed to clear IRQs. Btw, you can write that:

iowrite32(-1, psp->io_regs + PSP_P2CMSG_INTSTS);



Sure, I will do that

...

...


+
+sp_set_psp_master(sp);


So this function is called only once and declared somewhere else. You
could simply do here:

 if (sp->set_psp_master_device)
 sp->set_psp_master_device(sp);

and get rid of one more global function.



Sure I can do that.




+/* Enable interrupt */
+dev_dbg(dev, "Enabling interrupts ...\n");
+iowrite32(7, psp->io_regs + PSP_P2CMSG_INTEN);


Uh, a magic "7"! Exciting!

I wonder what that means and whether it could be a define with an
explanatory name instead. Ditto for the other values...




I will try to define some macro instead of hard coded values.




+
+int psp_dev_resume(struct sp_device *sp)
+{
+return 0;
+}
+
+int psp_dev_suspend(struct sp_device *sp, pm_message_t state)
+{
+return 0;
+}


Those last two are completely useless. Delete them pls.



We don't have any PM support, I agree will delete it.

...


+int psp_request_sev_irq(struct psp_device *psp, irq_handler_t handler,
+void *data)
+{
+psp->sev_irq_data = data;
+psp->sev_irq_handler = handler;
+
+return 0;
+}
+
+int psp_free_sev_irq(struct psp_device *psp, void *data)
+{
+if (psp->sev_irq_handler) {
+psp->sev_irq_data = NULL;
+psp->sev_irq_handler = NULL;
+}
+
+return 0;
+}


Both void. Please do not return values from functions which are simply
void functions by design.



thanks, will fix it.

...


+int psp_request_sev_irq(struct psp_device *psp, irq_handler_t handler,
+void *data);
+int psp_free_sev_irq(struct psp_device *psp, void *data);
+
+int psp_request_tee_irq(struct psp_device *psp, irq_handler_t handler,
+void *data);


Let them stick out.


okay

...




+int psp_free_tee_irq(struct psp_device *psp, void *data);
+
+struct psp_device *psp_get_master_device(void);
+
+extern const struct psp_vdata psp_entry;
+
+#endif /* __PSP_DEV_H */
diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c


So this file is called sp-dev and the other psp-dev. Confusing.

And in general, why isn't the whole thing a single psp-dev and you can
save yourself all the registering blabla and have a single driver for
the whole PSP 

Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support

[Brijesh Singh]

Hi Boris,

On 09/07/2017 09:27 AM, Borislav Petkov wrote:

...



The commit message above reads better to me as the help text than what
you have here.

Also, in order to make it easier for the user, I think we'll need a
CONFIG_AMD_MEM_ENCRYPT_SEV or so and make that depend on CONFIG_KVM_AMD,
this above and all the other pieces that are needed. Just so that when
the user builds such a kernel, all is enabled and not her having to go
look for what else is needed.

And then put the sev code behind that config option. Depending on how
ugly it gets...



I will add more detail in the help text. I will look into adding some
depends.

...


+
+void psp_add_device(struct psp_device *psp)


That function is needlessly global and should be static, AFAICT.

Better yet, it is called only once and its body is trivial so you can
completely get rid of it and meld it into the callsite.



Agreed, will do.

.


+
+static struct psp_device *psp_alloc_struct(struct sp_device *sp)


"psp_alloc()" is enough I guess.



I was trying to adhere to the existing ccp-dev.c function naming
conversion.





static.

Please audit all your functions in the psp pile and make them static if
not needed outside of their compilation unit.



Will do.


+{
+   unsigned int status;
+   irqreturn_t ret = IRQ_HANDLED;
+   struct psp_device *psp = data;


Please sort function local variables declaration in a reverse christmas
tree order:

 longest_variable_name;
 shorter_var_name;
 even_shorter;
 i;



Got it, will do



+
+   /* read the interrupt status */
+   status = ioread32(psp->io_regs + PSP_P2CMSG_INTSTS);
+
+   /* invoke subdevice interrupt handlers */
+   if (status) {
+   if (psp->sev_irq_handler)
+   ret = psp->sev_irq_handler(irq, psp->sev_irq_data);
+   if (psp->tee_irq_handler)
+   ret = psp->tee_irq_handler(irq, psp->tee_irq_data);
+   }
+
+   /* clear the interrupt status */
+   iowrite32(status, psp->io_regs + PSP_P2CMSG_INTSTS);


We're clearing the status by writing the same value back?!? Shouldn't
that be:

iowrite32(0, psp->io_regs + PSP_P2CMSG_INTSTS);



Actually the SW should write "1" to clear the bit. To make it clear, I
can use value 1 and add comment.




Below I see

iowrite32(0x, psp->io_regs + PSP_P2CMSG_INTSTS);

which is supposed to clear IRQs. Btw, you can write that:

iowrite32(-1, psp->io_regs + PSP_P2CMSG_INTSTS);



Sure, I will do that

...

...


+
+   sp_set_psp_master(sp);


So this function is called only once and declared somewhere else. You
could simply do here:

 if (sp->set_psp_master_device)
 sp->set_psp_master_device(sp);

and get rid of one more global function.



Sure I can do that.




+   /* Enable interrupt */
+   dev_dbg(dev, "Enabling interrupts ...\n");
+   iowrite32(7, psp->io_regs + PSP_P2CMSG_INTEN);


Uh, a magic "7"! Exciting!

I wonder what that means and whether it could be a define with an
explanatory name instead. Ditto for the other values...




I will try to define some macro instead of hard coded values.




+
+int psp_dev_resume(struct sp_device *sp)
+{
+   return 0;
+}
+
+int psp_dev_suspend(struct sp_device *sp, pm_message_t state)
+{
+   return 0;
+}


Those last two are completely useless. Delete them pls.



We don't have any PM support, I agree will delete it.

...


+int psp_request_sev_irq(struct psp_device *psp, irq_handler_t handler,
+   void *data)
+{
+   psp->sev_irq_data = data;
+   psp->sev_irq_handler = handler;
+
+   return 0;
+}
+
+int psp_free_sev_irq(struct psp_device *psp, void *data)
+{
+   if (psp->sev_irq_handler) {
+   psp->sev_irq_data = NULL;
+   psp->sev_irq_handler = NULL;
+   }
+
+   return 0;
+}


Both void. Please do not return values from functions which are simply
void functions by design.



thanks, will fix it.

...


+int psp_request_sev_irq(struct psp_device *psp, irq_handler_t handler,
+   void *data);
+int psp_free_sev_irq(struct psp_device *psp, void *data);
+
+int psp_request_tee_irq(struct psp_device *psp, irq_handler_t handler,
+   void *data);


Let them stick out.


okay

...




+int psp_free_tee_irq(struct psp_device *psp, void *data);
+
+struct psp_device *psp_get_master_device(void);
+
+extern const struct psp_vdata psp_entry;
+
+#endif /* __PSP_DEV_H */
diff --git a/drivers/crypto/ccp/sp-dev.c b/drivers/crypto/ccp/sp-dev.c


So this file is called sp-dev and the other psp-dev. Confusing.

And in general, why isn't the whole thing a single psp-dev and you can
save yourself all the registering blabla and have a single driver for
the whole PSP functionality?

Distros will have to enable everything anyway and the whole CCP/PSP code
is only a couple of KBs so you can just as well put it all into 

Re: [PATCH 00/12] x86/crypto: Fix RBP usage in several crypto .S files

[Ingo Molnar]

* Eric Biggers  wrote:

> On Thu, Sep 07, 2017 at 09:15:34AM +0200, Ingo Molnar wrote:
> > 
> > * Eric Biggers  wrote:
> > 
> > > Thanks for fixing these!  I don't have time to review these in detail, 
> > > but I ran
> > > the crypto self-tests on the affected algorithms, and they all pass.  I 
> > > also
> > > benchmarked them before and after; the only noticable performance 
> > > difference was
> > > that sha256-avx2 and sha512-avx2 became a few percent slower.  I don't 
> > > suppose
> > > there is a way around that?  Otherwise it's probably not a big deal.
> > 
> > Which CPU model did you use for the test?
> > 
> > Thanks,
> > 
> > Ingo
> 
> This was on Haswell, "Intel(R) Xeon(R) CPU E5-1650 v3 @ 3.50GHz".

Any chance to test this with the latest microarchitecture - any Skylake 
derivative 
Intel CPU you have access to?

Thanks,

Ingo

Re: [PATCH 00/12] x86/crypto: Fix RBP usage in several crypto .S files

[Eric Biggers]

On Thu, Sep 07, 2017 at 09:15:34AM +0200, Ingo Molnar wrote:
> 
> * Eric Biggers  wrote:
> 
> > Thanks for fixing these!  I don't have time to review these in detail, but 
> > I ran
> > the crypto self-tests on the affected algorithms, and they all pass.  I also
> > benchmarked them before and after; the only noticable performance 
> > difference was
> > that sha256-avx2 and sha512-avx2 became a few percent slower.  I don't 
> > suppose
> > there is a way around that?  Otherwise it's probably not a big deal.
> 
> Which CPU model did you use for the test?
> 
> Thanks,
> 
>   Ingo

This was on Haswell, "Intel(R) Xeon(R) CPU E5-1650 v3 @ 3.50GHz".

Eric

[PATCH v2] Staging: ccree: Prefer using BIT macro.

[Srishti Sharma]

Use BIT(x) instead of using (1<
---
Changes in v2:
 - Add tab spaces before BIT macro.

 drivers/staging/ccree/ssi_cipher.h | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/ccree/ssi_cipher.h 
b/drivers/staging/ccree/ssi_cipher.h
index 296b375..c9a83df 100644
--- a/drivers/staging/ccree/ssi_cipher.h
+++ b/drivers/staging/ccree/ssi_cipher.h
@@ -27,11 +27,11 @@
 #include "ssi_buffer_mgr.h"

 /* Crypto cipher flags */
-#define CC_CRYPTO_CIPHER_KEY_KFDE0(1 << 0)
-#define CC_CRYPTO_CIPHER_KEY_KFDE1(1 << 1)
-#define CC_CRYPTO_CIPHER_KEY_KFDE2(1 << 2)
-#define CC_CRYPTO_CIPHER_KEY_KFDE3(1 << 3)
-#define CC_CRYPTO_CIPHER_DU_SIZE_512B (1 << 4)
+#define CC_CRYPTO_CIPHER_KEY_KFDE0 BIT(0)
+#define CC_CRYPTO_CIPHER_KEY_KFDE1 BIT(1)
+#define CC_CRYPTO_CIPHER_KEY_KFDE2 BIT(2)
+#define CC_CRYPTO_CIPHER_KEY_KFDE3 BIT(3)
+#define CC_CRYPTO_CIPHER_DU_SIZE_512B  BIT(4)

 #define CC_CRYPTO_CIPHER_KEY_KFDE_MASK (CC_CRYPTO_CIPHER_KEY_KFDE0 | 
CC_CRYPTO_CIPHER_KEY_KFDE1 | CC_CRYPTO_CIPHER_KEY_KFDE2 | 
CC_CRYPTO_CIPHER_KEY_KFDE3)

--
2.7.4

Re: [PATCH] Staging: ccree: Prefer using BIT macro.

[Greg KH]

On Thu, Sep 07, 2017 at 07:17:09PM +0530, Srishti Sharma wrote:
> Use BIT(x) instead of (1< 
> Signed-off-by: Srishti Sharma 
> ---
>  drivers/staging/ccree/ssi_cipher.h | 10 +-
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/ccree/ssi_cipher.h 
> b/drivers/staging/ccree/ssi_cipher.h
> index 296b375..6fbcf9d 100644
> --- a/drivers/staging/ccree/ssi_cipher.h
> +++ b/drivers/staging/ccree/ssi_cipher.h
> @@ -27,11 +27,11 @@
>  #include "ssi_buffer_mgr.h"
> 
>  /* Crypto cipher flags */
> -#define CC_CRYPTO_CIPHER_KEY_KFDE0(1 << 0)
> -#define CC_CRYPTO_CIPHER_KEY_KFDE1(1 << 1)
> -#define CC_CRYPTO_CIPHER_KEY_KFDE2(1 << 2)
> -#define CC_CRYPTO_CIPHER_KEY_KFDE3(1 << 3)
> -#define CC_CRYPTO_CIPHER_DU_SIZE_512B (1 << 4)
> +#define CC_CRYPTO_CIPHER_KEY_KFDE0BIT(0)
> +#define CC_CRYPTO_CIPHER_KEY_KFDE1BIT(1)
> +#define CC_CRYPTO_CIPHER_KEY_KFDE2BIT(2)
> +#define CC_CRYPTO_CIPHER_KEY_KFDE3BIT(3)
> +#define CC_CRYPTO_CIPHER_DU_SIZE_512B BIT(4)

Care to use a  here as well?  I know the original didn't, but the
cleaned up code should.

thanks,

greg k-h

[PATCH] Staging: ccree: Prefer using BIT macro.

[Srishti Sharma]

Use BIT(x) instead of (1<
---
 drivers/staging/ccree/ssi_cipher.h | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/ccree/ssi_cipher.h 
b/drivers/staging/ccree/ssi_cipher.h
index 296b375..6fbcf9d 100644
--- a/drivers/staging/ccree/ssi_cipher.h
+++ b/drivers/staging/ccree/ssi_cipher.h
@@ -27,11 +27,11 @@
 #include "ssi_buffer_mgr.h"

 /* Crypto cipher flags */
-#define CC_CRYPTO_CIPHER_KEY_KFDE0(1 << 0)
-#define CC_CRYPTO_CIPHER_KEY_KFDE1(1 << 1)
-#define CC_CRYPTO_CIPHER_KEY_KFDE2(1 << 2)
-#define CC_CRYPTO_CIPHER_KEY_KFDE3(1 << 3)
-#define CC_CRYPTO_CIPHER_DU_SIZE_512B (1 << 4)
+#define CC_CRYPTO_CIPHER_KEY_KFDE0BIT(0)
+#define CC_CRYPTO_CIPHER_KEY_KFDE1BIT(1)
+#define CC_CRYPTO_CIPHER_KEY_KFDE2BIT(2)
+#define CC_CRYPTO_CIPHER_KEY_KFDE3BIT(3)
+#define CC_CRYPTO_CIPHER_DU_SIZE_512B BIT(4)

 #define CC_CRYPTO_CIPHER_KEY_KFDE_MASK (CC_CRYPTO_CIPHER_KEY_KFDE0 | 
CC_CRYPTO_CIPHER_KEY_KFDE1 | CC_CRYPTO_CIPHER_KEY_KFDE2 | 
CC_CRYPTO_CIPHER_KEY_KFDE3)

--
2.7.4

Re: [RFC Part2 PATCH v3 02/26] crypto: ccp: Add Platform Security Processor (PSP) device support

[Borislav Petkov]

On Wed, Sep 06, 2017 at 04:26:52PM -0500, Gary R Hook wrote:
> They were included in a pull request (for 4.14) from Herbert, dated Monday.

Right. I rebased the SEV pile ontop of latest upstream and now it applies much
better:

checking file drivers/crypto/ccp/Kconfig
Hunk #1 succeeded at 32 (offset 1 line).
checking file drivers/crypto/ccp/Makefile
checking file drivers/crypto/ccp/psp-dev.c
checking file drivers/crypto/ccp/psp-dev.h
checking file drivers/crypto/ccp/sp-dev.c
Hunk #3 succeeded at 225 (offset 1 line).
Hunk #4 FAILED at 243.
1 out of 4 hunks FAILED
checking file drivers/crypto/ccp/sp-dev.h
Hunk #1 succeeded at 42 with fuzz 2 (offset 1 line).
Hunk #2 succeeded at 75 (offset 1 line).
Hunk #3 succeeded at 114 (offset 1 line).
Hunk #4 succeeded at 143 (offset 1 line).
checking file drivers/crypto/ccp/sp-pci.c

The failed hunk is due to #ifdef CONFIG_PM guards but that's trivial to fix.

Thanks.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [PATCH] crypto: caam - properly set IV after {en,de}crypt

[Horia Geantă]

On 9/6/2017 1:14 PM, Gilad Ben-Yossef wrote:
> On Tue, Sep 5, 2017 at 6:33 PM, Horia Geantă  wrote:
>> On 8/14/2017 10:59 AM, Gilad Ben-Yossef wrote:
>>> Hi,
>>>
>>> On Thu, Jun 29, 2017 at 1:19 PM, Horia Geantă  wrote:
 On 6/28/2017 4:42 PM, Horia Geantă wrote:
> On 6/28/2017 4:27 PM, David Gstir wrote:
>> Certain cipher modes like CTS expect the IV (req->info) of
>> ablkcipher_request (or equivalently req->iv of skcipher_request) to
>> contain the last ciphertext block when the {en,de}crypt operation is 
>> done.
>> This is currently not the case for the CAAM driver which in turn breaks
>> e.g. cts(cbc(aes)) when the CAAM driver is enabled.
>>
>> This patch fixes the CAAM driver to properly set the IV after the
>> {en,de}crypt operation of ablkcipher finishes.
>>
>> This issue was revealed by the changes in the SW CTS mode in commit
>> 0605c41cc53ca ("crypto: cts - Convert to skcipher")
>>
>> Cc:  # 4.8+
>> Signed-off-by: David Gstir 
> Reviewed-by: Horia Geantă 
>
 Btw, instead of updating the IV in SW, CAAM engine could be programmed
 to do it - by saving the Context Register of the AES accelerator.

 Unfortunately this would require changes in quite a few places: shared
 descriptor, HW S/G generation logic, IV dma (un)mapping and maybe others.

 So it's better to have this fix now (which, considering size, is
 appropriate for -stable) and later, if needed, offload IV updating in HW.

>>>
>>> My apologies for reviving this thread from the dead, but doesn't the patch 
>>> fail
>>> for in-place decryption since we are copying from req->dst after
>>> the operation is done, and therefore it no longer contains the ciphertext?
>>>
>> You are right, thanks! Will follow up with a fix.
>> Though cts(cbc(aes)) in particular is working, see below.
>>
>>> I'm asking since I ran into a similar issue in the ccree driver and thought
>>> to deploy a similar fix but could not convince myself why this works.
>>>
>> IIUC cts(cbc(aes)) in-place decryption (with cbc(aes) offloaded to CAAM
>> engine) works since SW implementation of cts, when performing the
>> ciphertext stealing phase in cts_cbc_decrypt() does not use req->iv, but
>> a previously value, saved before staring decryption in crypto_cts_decrypt():
>>
>> if (cbc_blocks <= 1)
>> memcpy(space, req->iv, bsize);
>> else
>> scatterwalk_map_and_copy(space, req->src, offset - 2 * bsize,
>>  bsize, 0);
>>
> 
> Is that not a performance bug in software CTS than? I mean all those
> transformation
> drivers doing that extra copy and possibly malloc and free to save the
> data for the info
> and than have the CTS implementation ignore that and do its own memory copy?
> 
AFAICT, in case cbc_blocks > 1 cts saves the second from last full
block, while underlying cbc subrequest saves the last full block.

Horia

Re: [PATCH v5] Staging: ccree: Remove unused variable.

[Srishti Sharma]

On Thu, Sep 7, 2017 at 2:20 PM, Gilad Ben-Yossef  wrote:
> Hi,
>
> On Thu, Sep 7, 2017 at 10:49 AM, Srishti Sharma  wrote:
>> Remove the local variable inflight_counter as it is never used.
>>
>> Signed-off-by: Srishti Sharma 
>> ---
>
> I've been meaning to clean that for some time now and never got around to do 
> it.
> Thank you! :-)

Glad to know :)

>
> Acked-by: Gilad Ben-Yossef 
>
>
>> Changes in v5:
>>  - Correct the format of the subject.
>>  drivers/staging/ccree/ssi_cipher.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/staging/ccree/ssi_cipher.c 
>> b/drivers/staging/ccree/ssi_cipher.c
>> index 8d31a93..1ff3c8a 100644
>> --- a/drivers/staging/ccree/ssi_cipher.c
>> +++ b/drivers/staging/ccree/ssi_cipher.c
>> @@ -697,13 +697,10 @@ static int ssi_blkcipher_complete(struct device *dev,
>>   void __iomem *cc_base)
>>  {
>> int completion_error = 0;
>> -   u32 inflight_counter;
>> struct ablkcipher_request *req = (struct ablkcipher_request *)areq;
>>
>> ssi_buffer_mgr_unmap_blkcipher_request(dev, req_ctx, ivsize, src, 
>> dst);
>>
>> -   /*Set the inflight couter value to local variable*/
>> -   inflight_counter =  ctx_p->drvdata->inflight_counter;
>> /*Decrease the inflight counter*/
>> if (ctx_p->flow_mode == BYPASS && ctx_p->drvdata->inflight_counter > 
>> 0)
>> ctx_p->drvdata->inflight_counter--;
>> --
>> 2.7.4
>>
>
>
>
> --
> 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

Regards,
Srishti

[PATCH v3 1/8] staging: ccree: Replace kzalloc with devm_kzalloc

[Gilad Ben-Yossef]

From: Suniel Mahesh 

It is recommended to use managed function devm_kzalloc, which
simplifies driver cleanup paths and driver code.
This patch does the following:
(a) replace kzalloc with devm_kzalloc.
(b) drop kfree(), because memory allocated with devm_kzalloc() is
automatically freed on driver detach, otherwise it leads to a double
free.
(c) remove unnecessary blank lines.

Signed-off-by: Suniel Mahesh 
[gby: rebase on top of latest coding style fixes changes]
Acked-by: Gilad Ben-Yossef 
---
 drivers/staging/ccree/ssi_driver.c | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/ccree/ssi_driver.c 
b/drivers/staging/ccree/ssi_driver.c
index 9c6f120..47e0880 100644
--- a/drivers/staging/ccree/ssi_driver.c
+++ b/drivers/staging/ccree/ssi_driver.c
@@ -223,14 +223,15 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
struct resource *req_mem_cc_regs = NULL;
void __iomem *cc_base = NULL;
bool irq_registered = false;
-   struct ssi_drvdata *new_drvdata = kzalloc(sizeof(*new_drvdata),
- GFP_KERNEL);
+   struct ssi_drvdata *new_drvdata;
struct device *dev = _dev->dev;
struct device_node *np = dev->of_node;
u32 signature_val;
int rc = 0;
 
-   if (unlikely(!new_drvdata)) {
+   new_drvdata = devm_kzalloc(_dev->dev, sizeof(*new_drvdata),
+  GFP_KERNEL);
+   if (!new_drvdata) {
SSI_LOG_ERR("Failed to allocate drvdata");
rc = -ENOMEM;
goto init_cc_res_err;
@@ -435,10 +436,8 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
   resource_size(new_drvdata->res_mem));
new_drvdata->res_mem = NULL;
}
-   kfree(new_drvdata);
dev_set_drvdata(_dev->dev, NULL);
}
-
return rc;
 }
 
@@ -479,8 +478,6 @@ static void cleanup_cc_resources(struct platform_device 
*plat_dev)
drvdata->cc_base = NULL;
drvdata->res_mem = NULL;
}
-
-   kfree(drvdata);
dev_set_drvdata(_dev->dev, NULL);
 }
 
-- 
2.1.4

[PATCH v3 2/8] staging: ccree: Convert to devm_ioremap_resource for map, unmap

[Gilad Ben-Yossef]

From: Suniel Mahesh 

It is recommended to use managed function devm_ioremap_resource(),
which simplifies driver cleanup paths and driver code.
This patch does the following:
(a) replace request_mem_region(), ioremap() and corresponding error
handling with devm_ioremap_resource().
(b) remove struct resource pointer(res_mem) in struct ssi_drvdata as it
seems redundant, use struct resource pointer which is defined locally and
adjust return value of platform_get_resource() accordingly.
(c) release_mem_region() and iounmap() are dropped, since devm_ioremap_
resource() releases and unmaps mem region on driver detach.
(d) adjust log messages accordingly and remove any blank lines.

Signed-off-by: Suniel Mahesh 
[gby: rebase on top of latest coding style fixes changes]
Acked-by: Gilad Ben-Yossef 
---
 drivers/staging/ccree/ssi_driver.c | 60 ++
 drivers/staging/ccree/ssi_driver.h |  1 -
 2 files changed, 15 insertions(+), 46 deletions(-)

diff --git a/drivers/staging/ccree/ssi_driver.c 
b/drivers/staging/ccree/ssi_driver.c
index 47e0880..cbe9a03 100644
--- a/drivers/staging/ccree/ssi_driver.c
+++ b/drivers/staging/ccree/ssi_driver.c
@@ -246,35 +246,21 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
dev_set_drvdata(_dev->dev, new_drvdata);
/* Get device resources */
/* First CC registers space */
-   new_drvdata->res_mem = platform_get_resource(plat_dev, IORESOURCE_MEM, 
0);
-   if (unlikely(!new_drvdata->res_mem)) {
-   SSI_LOG_ERR("Failed getting IO memory resource\n");
-   rc = -ENODEV;
-   goto init_cc_res_err;
-   }
-   SSI_LOG_DEBUG("Got MEM resource (%s): start=%pad end=%pad\n",
- new_drvdata->res_mem->name,
- new_drvdata->res_mem->start,
- new_drvdata->res_mem->end);
+   req_mem_cc_regs = platform_get_resource(plat_dev, IORESOURCE_MEM, 0);
/* Map registers space */
-   req_mem_cc_regs = request_mem_region(new_drvdata->res_mem->start, 
resource_size(new_drvdata->res_mem), "arm_cc7x_regs");
-   if (unlikely(!req_mem_cc_regs)) {
-   SSI_LOG_ERR("Couldn't allocate registers memory region at "
-"0x%08X\n", (unsigned 
int)new_drvdata->res_mem->start);
-   rc = -EBUSY;
-   goto init_cc_res_err;
-   }
-   cc_base = ioremap(new_drvdata->res_mem->start, 
resource_size(new_drvdata->res_mem));
-   if (unlikely(!cc_base)) {
-   SSI_LOG_ERR("ioremap[CC](0x%08X,0x%08X) failed\n",
-   (unsigned int)new_drvdata->res_mem->start,
-   (unsigned int)resource_size(new_drvdata->res_mem));
-   rc = -ENOMEM;
+   new_drvdata->cc_base = devm_ioremap_resource(_dev->dev,
+req_mem_cc_regs);
+   if (IS_ERR(new_drvdata->cc_base)) {
+   rc = PTR_ERR(new_drvdata->cc_base);
goto init_cc_res_err;
}
-   SSI_LOG_DEBUG("CC registers mapped from %pa to 0x%p\n", 
_drvdata->res_mem->start, cc_base);
-   new_drvdata->cc_base = cc_base;
-
+   SSI_LOG_DEBUG("Got MEM resource (%s): start=%pad end=%pad\n",
+ req_mem_cc_regs->name,
+ req_mem_cc_regs->start,
+ req_mem_cc_regs->end);
+   SSI_LOG_DEBUG("CC registers mapped from %pa to 0x%p\n",
+ _mem_cc_regs->start, new_drvdata->cc_base);
+   cc_base = new_drvdata->cc_base;
/* Then IRQ */
new_drvdata->res_irq = platform_get_resource(plat_dev, IORESOURCE_IRQ, 
0);
if (unlikely(!new_drvdata->res_irq)) {
@@ -424,17 +410,9 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
 #ifdef ENABLE_CC_SYSFS
ssi_sysfs_fini();
 #endif
-
-   if (req_mem_cc_regs) {
-   if (irq_registered) {
-   free_irq(new_drvdata->res_irq->start, 
new_drvdata);
-   new_drvdata->res_irq = NULL;
-   iounmap(cc_base);
-   new_drvdata->cc_base = NULL;
-   }
-   release_mem_region(new_drvdata->res_mem->start,
-  resource_size(new_drvdata->res_mem));
-   new_drvdata->res_mem = NULL;
+   if (irq_registered) {
+   free_irq(new_drvdata->res_irq->start, new_drvdata);
+   new_drvdata->res_irq = NULL;
}
dev_set_drvdata(_dev->dev, NULL);
}
@@ -470,14 +448,6 @@ static void cleanup_cc_resources(struct platform_device 
*plat_dev)
cc_clk_off(drvdata);
free_irq(drvdata->res_irq->start, drvdata);
drvdata->res_irq = NULL;
-
-   if 

[PATCH v3 3/8] staging: ccree: Use platform_get_irq and devm_request_irq

[Gilad Ben-Yossef]

From: Suniel Mahesh 

It is recommended to use managed function devm_request_irq(),
which simplifies driver cleanup paths and driver code.
This patch does the following:
(a) replace platform_get_resource(), request_irq() and corresponding
error handling with platform_get_irq() and devm_request_irq().
(b) remove struct resource pointer(res_irq) in struct ssi_drvdata as
it seems redundant.
(c) change type of member irq in struct ssi_drvdata from unsigned int
to int, as return type of platform_get_irq is int and can be used in
error handling.
(d) remove irq_registered variable from driver probe as it seems
redundant.
(e) free_irq is not required any more, devm_request_irq() free's it
on driver detach.
(f) adjust log messages accordingly and remove any blank lines.

Signed-off-by: Suniel Mahesh 
Acked-by: Gilad Ben-Yossef 
---
 drivers/staging/ccree/ssi_driver.c | 30 +-
 drivers/staging/ccree/ssi_driver.h |  3 +--
 2 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/ccree/ssi_driver.c 
b/drivers/staging/ccree/ssi_driver.c
index cbe9a03..3e7193d 100644
--- a/drivers/staging/ccree/ssi_driver.c
+++ b/drivers/staging/ccree/ssi_driver.c
@@ -222,7 +222,6 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
 {
struct resource *req_mem_cc_regs = NULL;
void __iomem *cc_base = NULL;
-   bool irq_registered = false;
struct ssi_drvdata *new_drvdata;
struct device *dev = _dev->dev;
struct device_node *np = dev->of_node;
@@ -262,26 +261,22 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
  _mem_cc_regs->start, new_drvdata->cc_base);
cc_base = new_drvdata->cc_base;
/* Then IRQ */
-   new_drvdata->res_irq = platform_get_resource(plat_dev, IORESOURCE_IRQ, 
0);
-   if (unlikely(!new_drvdata->res_irq)) {
+   new_drvdata->irq = platform_get_irq(plat_dev, 0);
+   if (new_drvdata->irq < 0) {
SSI_LOG_ERR("Failed getting IRQ resource\n");
-   rc = -ENODEV;
+   rc = new_drvdata->irq;
goto init_cc_res_err;
}
-   rc = request_irq(new_drvdata->res_irq->start, cc_isr,
-IRQF_SHARED, "arm_cc7x", new_drvdata);
-   if (unlikely(rc != 0)) {
-   SSI_LOG_ERR("Could not register to interrupt %llu\n",
-   (unsigned long long)new_drvdata->res_irq->start);
+   rc = devm_request_irq(_dev->dev, new_drvdata->irq, cc_isr,
+ IRQF_SHARED, "arm_cc7x", new_drvdata);
+   if (rc) {
+   SSI_LOG_ERR("Could not register to interrupt %d\n",
+   new_drvdata->irq);
goto init_cc_res_err;
}
init_completion(_drvdata->icache_setup_completion);
 
-   irq_registered = true;
-   SSI_LOG_DEBUG("Registered to IRQ (%s) %llu\n",
- new_drvdata->res_irq->name,
- (unsigned long long)new_drvdata->res_irq->start);
-
+   SSI_LOG_DEBUG("Registered to IRQ: %d\n", new_drvdata->irq);
new_drvdata->plat_dev = plat_dev;
 
rc = cc_clk_on(new_drvdata);
@@ -410,10 +405,6 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
 #ifdef ENABLE_CC_SYSFS
ssi_sysfs_fini();
 #endif
-   if (irq_registered) {
-   free_irq(new_drvdata->res_irq->start, new_drvdata);
-   new_drvdata->res_irq = NULL;
-   }
dev_set_drvdata(_dev->dev, NULL);
}
return rc;
@@ -443,11 +434,8 @@ static void cleanup_cc_resources(struct platform_device 
*plat_dev)
 #ifdef ENABLE_CC_SYSFS
ssi_sysfs_fini();
 #endif
-
fini_cc_regs(drvdata);
cc_clk_off(drvdata);
-   free_irq(drvdata->res_irq->start, drvdata);
-   drvdata->res_irq = NULL;
dev_set_drvdata(_dev->dev, NULL);
 }
 
diff --git a/drivers/staging/ccree/ssi_driver.h 
b/drivers/staging/ccree/ssi_driver.h
index 518c0bf..88ef370 100644
--- a/drivers/staging/ccree/ssi_driver.h
+++ b/drivers/staging/ccree/ssi_driver.h
@@ -128,9 +128,8 @@ struct ssi_crypto_req {
  * @fw_ver:SeP loaded firmware version
  */
 struct ssi_drvdata {
-   struct resource *res_irq;
void __iomem *cc_base;
-   unsigned int irq;
+   int irq;
u32 irq_mask;
u32 fw_ver;
/* Calibration time of start/stop
-- 
2.1.4

[PATCH v3 5/8] staging: ccree: remove unused completion

[Gilad Ben-Yossef]

icache_setup_completion is no longer used. Remove it.

Signed-off-by: Gilad Ben-Yossef 
---
 drivers/staging/ccree/ssi_driver.c | 2 --
 drivers/staging/ccree/ssi_driver.h | 1 -
 2 files changed, 3 deletions(-)

diff --git a/drivers/staging/ccree/ssi_driver.c 
b/drivers/staging/ccree/ssi_driver.c
index dc22f13..8f1d7af 100644
--- a/drivers/staging/ccree/ssi_driver.c
+++ b/drivers/staging/ccree/ssi_driver.c
@@ -279,8 +279,6 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
}
SSI_LOG_DEBUG("Registered to IRQ: %d\n", new_drvdata->irq);
 
-   init_completion(_drvdata->icache_setup_completion);
-
rc = cc_clk_on(new_drvdata);
if (rc)
goto post_drvdata_err;
diff --git a/drivers/staging/ccree/ssi_driver.h 
b/drivers/staging/ccree/ssi_driver.h
index 88ef370..9b6476d 100644
--- a/drivers/staging/ccree/ssi_driver.h
+++ b/drivers/staging/ccree/ssi_driver.h
@@ -138,7 +138,6 @@ struct ssi_drvdata {
u32 monitor_null_cycles;
struct platform_device *plat_dev;
ssi_sram_addr_t mlli_sram_addr;
-   struct completion icache_setup_completion;
void *buff_mgr_handle;
void *hash_handle;
void *aead_handle;
-- 
2.1.4

[PATCH v3 4/8] staging: ccree: simplify resource release on error

[Gilad Ben-Yossef]

The resource release on probe/init error was being handled
in an awkward manner and possibly leaking memory on certain
(unlikely) error path.

Fix it by simplifying the error resource release and making
it easier to track.

Reported-by: Dan Carpenter 
Signed-off-by: Gilad Ben-Yossef 
---
 drivers/staging/ccree/ssi_aead.c   |   3 +-
 drivers/staging/ccree/ssi_cipher.c |   3 +-
 drivers/staging/ccree/ssi_driver.c | 102 -
 drivers/staging/ccree/ssi_hash.c   |   3 +-
 4 files changed, 59 insertions(+), 52 deletions(-)

diff --git a/drivers/staging/ccree/ssi_aead.c b/drivers/staging/ccree/ssi_aead.c
index 5abe6b2..8191ec4 100644
--- a/drivers/staging/ccree/ssi_aead.c
+++ b/drivers/staging/ccree/ssi_aead.c
@@ -2720,6 +2720,7 @@ int ssi_aead_alloc(struct ssi_drvdata *drvdata)
goto fail0;
}
 
+   INIT_LIST_HEAD(_handle->aead_list);
drvdata->aead_handle = aead_handle;
 
aead_handle->sram_workspace_addr = ssi_sram_mgr_alloc(
@@ -2730,8 +2731,6 @@ int ssi_aead_alloc(struct ssi_drvdata *drvdata)
goto fail1;
}
 
-   INIT_LIST_HEAD(_handle->aead_list);
-
/* Linux crypto */
for (alg = 0; alg < ARRAY_SIZE(aead_algs); alg++) {
t_alg = ssi_aead_create_alg(_algs[alg]);
diff --git a/drivers/staging/ccree/ssi_cipher.c 
b/drivers/staging/ccree/ssi_cipher.c
index 1ff3c8a..ab0349f 100644
--- a/drivers/staging/ccree/ssi_cipher.c
+++ b/drivers/staging/ccree/ssi_cipher.c
@@ -1312,9 +1312,8 @@ int ssi_ablkcipher_alloc(struct ssi_drvdata *drvdata)
if (!ablkcipher_handle)
return -ENOMEM;
 
-   drvdata->blkcipher_handle = ablkcipher_handle;
-
INIT_LIST_HEAD(_handle->blkcipher_alg_list);
+   drvdata->blkcipher_handle = ablkcipher_handle;
 
/* Linux crypto */
SSI_LOG_DEBUG("Number of algorithms = %zu\n", 
ARRAY_SIZE(blkcipher_algs));
diff --git a/drivers/staging/ccree/ssi_driver.c 
b/drivers/staging/ccree/ssi_driver.c
index 3e7193d..dc22f13 100644
--- a/drivers/staging/ccree/ssi_driver.c
+++ b/drivers/staging/ccree/ssi_driver.c
@@ -233,16 +233,14 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
if (!new_drvdata) {
SSI_LOG_ERR("Failed to allocate drvdata");
rc = -ENOMEM;
-   goto init_cc_res_err;
+   goto post_drvdata_err;
}
+   dev_set_drvdata(_dev->dev, new_drvdata);
+   new_drvdata->plat_dev = plat_dev;
 
new_drvdata->clk = of_clk_get(np, 0);
new_drvdata->coherent = of_dma_is_coherent(np);
 
-   /*Initialize inflight counter used in dx_ablkcipher_secure_complete 
used for count of BYSPASS blocks operations*/
-   new_drvdata->inflight_counter = 0;
-
-   dev_set_drvdata(_dev->dev, new_drvdata);
/* Get device resources */
/* First CC registers space */
req_mem_cc_regs = platform_get_resource(plat_dev, IORESOURCE_MEM, 0);
@@ -250,38 +248,42 @@ static int init_cc_resources(struct platform_device 
*plat_dev)
new_drvdata->cc_base = devm_ioremap_resource(_dev->dev,
 req_mem_cc_regs);
if (IS_ERR(new_drvdata->cc_base)) {
+   SSI_LOG_ERR("Failed to ioremap registers");
rc = PTR_ERR(new_drvdata->cc_base);
-   goto init_cc_res_err;
+   goto post_drvdata_err;
}
+
SSI_LOG_DEBUG("Got MEM resource (%s): start=%pad end=%pad\n",
  req_mem_cc_regs->name,
  req_mem_cc_regs->start,
  req_mem_cc_regs->end);
SSI_LOG_DEBUG("CC registers mapped from %pa to 0x%p\n",
  _mem_cc_regs->start, new_drvdata->cc_base);
+
cc_base = new_drvdata->cc_base;
+
/* Then IRQ */
new_drvdata->irq = platform_get_irq(plat_dev, 0);
if (new_drvdata->irq < 0) {
SSI_LOG_ERR("Failed getting IRQ resource\n");
rc = new_drvdata->irq;
-   goto init_cc_res_err;
+   goto post_drvdata_err;
}
+
rc = devm_request_irq(_dev->dev, new_drvdata->irq, cc_isr,
  IRQF_SHARED, "arm_cc7x", new_drvdata);
if (rc) {
SSI_LOG_ERR("Could not register to interrupt %d\n",
new_drvdata->irq);
-   goto init_cc_res_err;
+   goto post_drvdata_err;
}
-   init_completion(_drvdata->icache_setup_completion);
-
SSI_LOG_DEBUG("Registered to IRQ: %d\n", new_drvdata->irq);
-   new_drvdata->plat_dev = plat_dev;
+
+   init_completion(_drvdata->icache_setup_completion);
 
rc = cc_clk_on(new_drvdata);
if (rc)
-   goto init_cc_res_err;
+   goto post_drvdata_err;
 
if (!new_drvdata->plat_dev->dev.dma_mask)

[PATCH v3 6/8] staging: ccree: move over to BIT macro for bit defines

[Gilad Ben-Yossef]

Use BIT macro for bit definitions where needed.

Signed-off-by: Gilad Ben-Yossef 
---
 drivers/staging/ccree/ssi_cipher.h | 10 +-
 drivers/staging/ccree/ssi_driver.c |  3 ++-
 drivers/staging/ccree/ssi_driver.h |  6 +++---
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/ccree/ssi_cipher.h 
b/drivers/staging/ccree/ssi_cipher.h
index 296b375..c9a83df 100644
--- a/drivers/staging/ccree/ssi_cipher.h
+++ b/drivers/staging/ccree/ssi_cipher.h
@@ -27,11 +27,11 @@
 #include "ssi_buffer_mgr.h"
 
 /* Crypto cipher flags */
-#define CC_CRYPTO_CIPHER_KEY_KFDE0(1 << 0)
-#define CC_CRYPTO_CIPHER_KEY_KFDE1(1 << 1)
-#define CC_CRYPTO_CIPHER_KEY_KFDE2(1 << 2)
-#define CC_CRYPTO_CIPHER_KEY_KFDE3(1 << 3)
-#define CC_CRYPTO_CIPHER_DU_SIZE_512B (1 << 4)
+#define CC_CRYPTO_CIPHER_KEY_KFDE0 BIT(0)
+#define CC_CRYPTO_CIPHER_KEY_KFDE1 BIT(1)
+#define CC_CRYPTO_CIPHER_KEY_KFDE2 BIT(2)
+#define CC_CRYPTO_CIPHER_KEY_KFDE3 BIT(3)
+#define CC_CRYPTO_CIPHER_DU_SIZE_512B  BIT(4)
 
 #define CC_CRYPTO_CIPHER_KEY_KFDE_MASK (CC_CRYPTO_CIPHER_KEY_KFDE0 | 
CC_CRYPTO_CIPHER_KEY_KFDE1 | CC_CRYPTO_CIPHER_KEY_KFDE2 | 
CC_CRYPTO_CIPHER_KEY_KFDE3)
 
diff --git a/drivers/staging/ccree/ssi_driver.c 
b/drivers/staging/ccree/ssi_driver.c
index 8f1d7af..6d16220 100644
--- a/drivers/staging/ccree/ssi_driver.c
+++ b/drivers/staging/ccree/ssi_driver.c
@@ -185,7 +185,8 @@ int init_cc_regs(struct ssi_drvdata *drvdata, bool is_probe)
CC_HAL_WRITE_REGISTER(CC_REG_OFFSET(HOST_RGF, HOST_ICR), val);
 
/* Unmask relevant interrupt cause */
-   val = (~(SSI_COMP_IRQ_MASK | SSI_AXI_ERR_IRQ_MASK | SSI_GPR0_IRQ_MASK));
+   val = (unsigned int)(~(SSI_COMP_IRQ_MASK | SSI_AXI_ERR_IRQ_MASK |
+  SSI_GPR0_IRQ_MASK));
CC_HAL_WRITE_REGISTER(CC_REG_OFFSET(HOST_RGF, HOST_IMR), val);
 
 #ifdef DX_HOST_IRQ_TIMER_INIT_VAL_REG_OFFSET
diff --git a/drivers/staging/ccree/ssi_driver.h 
b/drivers/staging/ccree/ssi_driver.h
index 9b6476d..06a3c48 100644
--- a/drivers/staging/ccree/ssi_driver.h
+++ b/drivers/staging/ccree/ssi_driver.h
@@ -68,12 +68,12 @@
 #define SSI_AXI_IRQ_MASK ((1 << DX_AXIM_CFG_BRESPMASK_BIT_SHIFT) | (1 << 
DX_AXIM_CFG_RRESPMASK_BIT_SHIFT) |\
(1 << DX_AXIM_CFG_INFLTMASK_BIT_SHIFT) | (1 << 
DX_AXIM_CFG_COMPMASK_BIT_SHIFT))
 
-#define SSI_AXI_ERR_IRQ_MASK (1 << DX_HOST_IRR_AXI_ERR_INT_BIT_SHIFT)
+#define SSI_AXI_ERR_IRQ_MASK BIT(DX_HOST_IRR_AXI_ERR_INT_BIT_SHIFT)
 
-#define SSI_COMP_IRQ_MASK (1 << DX_HOST_IRR_AXIM_COMP_INT_BIT_SHIFT)
+#define SSI_COMP_IRQ_MASK BIT(DX_HOST_IRR_AXIM_COMP_INT_BIT_SHIFT)
 
 /* TEE FIPS status interrupt */
-#define SSI_GPR0_IRQ_MASK (1 << DX_HOST_IRR_GPR0_BIT_SHIFT)
+#define SSI_GPR0_IRQ_MASK BIT(DX_HOST_IRR_GPR0_BIT_SHIFT)
 
 #define SSI_CRA_PRIO 3000
 
-- 
2.1.4

[PATCH v3 8/8] staging: ccree: remove BUG macro usage

[Gilad Ben-Yossef]

Replace BUG() macro usage that crash the kernel with alternatives
that signal error and/or try to recover.

Signed-off-by: Gilad Ben-Yossef 
---
 drivers/staging/ccree/ssi_buffer_mgr.c  | 18 ++
 drivers/staging/ccree/ssi_cipher.c  |  1 -
 drivers/staging/ccree/ssi_pm.c  |  3 ++-
 drivers/staging/ccree/ssi_request_mgr.c | 22 --
 4 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c 
b/drivers/staging/ccree/ssi_buffer_mgr.c
index 6393609..67c3296 100644
--- a/drivers/staging/ccree/ssi_buffer_mgr.c
+++ b/drivers/staging/ccree/ssi_buffer_mgr.c
@@ -81,11 +81,6 @@ static unsigned int ssi_buffer_mgr_get_sgl_nents(
unsigned int nents = 0;
 
while (nbytes != 0) {
-   if (sg_is_chain(sg_list)) {
-   SSI_LOG_ERR("Unexpected chained entry "
-  "in sg (entry =0x%X)\n", nents);
-   BUG();
-   }
if (sg_list->length != 0) {
nents++;
/* get the number of bytes in the last entry */
@@ -854,7 +849,7 @@ static inline int ssi_buffer_mgr_aead_chain_assoc(
//if have reached the end of the sgl, then this is 
unexpected
if (!current_sg) {
SSI_LOG_ERR("reached end of sg list. 
unexpected\n");
-   BUG();
+   return -EINVAL;
}
sg_index += current_sg->length;
mapped_nents++;
@@ -1134,10 +1129,9 @@ static inline int ssi_buffer_mgr_aead_chain_data(
 
offset = size_to_skip;
 
-   if (!sg_data) {
-   rc = -EINVAL;
-   goto chain_data_exit;
-   }
+   if (!sg_data)
+   return -EINVAL;
+
areq_ctx->src_sgl = req->src;
areq_ctx->dst_sgl = req->dst;
 
@@ -1154,7 +1148,7 @@ static inline int ssi_buffer_mgr_aead_chain_data(
//if have reached the end of the sgl, then this is unexpected
if (!areq_ctx->src_sgl) {
SSI_LOG_ERR("reached end of sg list. unexpected\n");
-   BUG();
+   return -EINVAL;
}
sg_index += areq_ctx->src_sgl->length;
src_mapped_nents--;
@@ -1198,7 +1192,7 @@ static inline int ssi_buffer_mgr_aead_chain_data(
//if have reached the end of the sgl, then this is unexpected
if (!areq_ctx->dst_sgl) {
SSI_LOG_ERR("reached end of sg list. unexpected\n");
-   BUG();
+   return -EINVAL;
}
sg_index += areq_ctx->dst_sgl->length;
dst_mapped_nents--;
diff --git a/drivers/staging/ccree/ssi_cipher.c 
b/drivers/staging/ccree/ssi_cipher.c
index ab0349f..a462075 100644
--- a/drivers/staging/ccree/ssi_cipher.c
+++ b/drivers/staging/ccree/ssi_cipher.c
@@ -541,7 +541,6 @@ ssi_blkcipher_create_setup_desc(
break;
default:
SSI_LOG_ERR("Unsupported cipher mode (%d)\n", cipher_mode);
-   BUG();
}
 }
 
diff --git a/drivers/staging/ccree/ssi_pm.c b/drivers/staging/ccree/ssi_pm.c
index 31325e6..a50671a 100644
--- a/drivers/staging/ccree/ssi_pm.c
+++ b/drivers/staging/ccree/ssi_pm.c
@@ -109,7 +109,8 @@ int ssi_power_mgr_runtime_put_suspend(struct device *dev)
rc = pm_runtime_put_autosuspend(dev);
} else {
/* Something wrong happens*/
-   BUG();
+   SSI_LOG_ERR("request to suspend already suspended queue");
+   rc = -EBUSY;
}
return rc;
 }
diff --git a/drivers/staging/ccree/ssi_request_mgr.c 
b/drivers/staging/ccree/ssi_request_mgr.c
index e5c2f92..97c2359 100644
--- a/drivers/staging/ccree/ssi_request_mgr.c
+++ b/drivers/staging/ccree/ssi_request_mgr.c
@@ -369,11 +369,16 @@ int send_request(
enqueue_seq(cc_base, _mgr_h->compl_desc, (is_dout ? 0 : 1));
 
if (unlikely(req_mgr_h->q_free_slots < total_seq_len)) {
-   /*This means that there was a problem with the resume*/
-   BUG();
+   /* This situation should never occur. Maybe indicating problem
+* with resuming power. Set the free slot count to 0 and hope
+* for the best.
+*/
+   SSI_LOG_ERR("HW free slot count mismatch.");
+   req_mgr_h->q_free_slots = 0;
+   } else {
+   /* Update the free slots in HW queue */
+   req_mgr_h->q_free_slots -= total_seq_len;
}
-   /* Update the free slots in HW queue */
-   req_mgr_h->q_free_slots -= total_seq_len;
 
spin_unlock_bh(_mgr_h->hw_lock);
 
@@ -460,8 +465,13 @@ static void proc_completions(struct 

[PATCH v3 7/8] staging: ccree: replace noop macro with inline

[Gilad Ben-Yossef]

Replace noop macro with a noop inline function

Signed-off-by: Gilad Ben-Yossef 
---
 drivers/staging/ccree/ssi_driver.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/ccree/ssi_driver.h 
b/drivers/staging/ccree/ssi_driver.h
index 06a3c48..81ba827 100644
--- a/drivers/staging/ccree/ssi_driver.h
+++ b/drivers/staging/ccree/ssi_driver.h
@@ -187,8 +187,8 @@ struct async_gen_req_ctx {
 #ifdef DX_DUMP_BYTES
 void dump_byte_array(const char *name, const u8 *the_array, unsigned long 
size);
 #else
-#define dump_byte_array(name, array, size) do {\
-} while (0);
+static inline void dump_byte_array(const char *name, const u8 *the_array,
+  unsigned long size) {};
 #endif
 
 int init_cc_regs(struct ssi_drvdata *drvdata, bool is_probe);
-- 
2.1.4

[PATCH v3 0/8] staging: ccree: more cleanup work for 4.15

[Gilad Ben-Yossef]

More cleanup work from Sunil and myself.

I've previously sent some of these as part of a larger patch set.
I've decided to split the patch set to smaller chunks to make it
more manageable.

This patch set applies on top of commit 28eb51f7468a
("staging:rtl8188eu:core Fix remove unneccessary else block").

Changes from v2:
- Cleanup return after error in BUG replacement patch
  as suggested by Dan Carpenter

Changes from v1:
- Fix kbuild reported error of "label at end of compound statement"

Gilad Ben-Yossef (5):
  staging: ccree: simplify resource release on error
  staging: ccree: remove unused completion
  staging: ccree: move over to BIT macro for bit defines
  staging: ccree: replace noop macro with inline
  staging: ccree: remove BUG macro usage

Suniel Mahesh (3):
  staging: ccree: Replace kzalloc with devm_kzalloc
  staging: ccree: Convert to devm_ioremap_resource for map, unmap
  staging: ccree: Use platform_get_irq and devm_request_irq

 drivers/staging/ccree/ssi_aead.c|   3 +-
 drivers/staging/ccree/ssi_buffer_mgr.c  |  18 +--
 drivers/staging/ccree/ssi_cipher.c  |   4 +-
 drivers/staging/ccree/ssi_cipher.h  |  10 +-
 drivers/staging/ccree/ssi_driver.c  | 196 +---
 drivers/staging/ccree/ssi_driver.h  |  15 +--
 drivers/staging/ccree/ssi_hash.c|   3 +-
 drivers/staging/ccree/ssi_pm.c  |   3 +-
 drivers/staging/ccree/ssi_request_mgr.c |  22 +++-
 9 files changed, 118 insertions(+), 156 deletions(-)

-- 
2.1.4

Re: [PATCH 8/8] staging: ccree: remove BUG macro usage

[Gilad Ben-Yossef]

On Wed, Sep 6, 2017 at 10:28 PM, Dan Carpenter  wrote:
> On Sun, Sep 03, 2017 at 11:56:50AM +0300, Gilad Ben-Yossef wrote:
>> @@ -1154,7 +1150,8 @@ static inline int ssi_buffer_mgr_aead_chain_data(
>>   //if have reached the end of the sgl, then this is unexpected
>>   if (!areq_ctx->src_sgl) {
>>   SSI_LOG_ERR("reached end of sg list. unexpected\n");
>> - BUG();
>> + return -EINVAL;
>> + goto chain_data_exit;
>
> You've got a direct return followed by a goto.

Yes, that is silly. Thank you for spotting that.

>
> It's a do-nothing goto that just returns rc.  I hate those.  I've tried
> to review locking bugs to see if single returns prevent future
> programmers from introducing new error paths which don't unlock.  They
> don't really help...


I've replaced the error handling from goto to return as you suggested
for the changes
introduced by this patch.

Still need to clean up a lot of the other code doing this though.

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

Re: [PATCH v5] Staging: ccree: Remove unused variable.

[Gilad Ben-Yossef]

Hi,

On Thu, Sep 7, 2017 at 10:49 AM, Srishti Sharma  wrote:
> Remove the local variable inflight_counter as it is never used.
>
> Signed-off-by: Srishti Sharma 
> ---

I've been meaning to clean that for some time now and never got around to do it.
Thank you! :-)

Acked-by: Gilad Ben-Yossef 


> Changes in v5:
>  - Correct the format of the subject.
>  drivers/staging/ccree/ssi_cipher.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/staging/ccree/ssi_cipher.c 
> b/drivers/staging/ccree/ssi_cipher.c
> index 8d31a93..1ff3c8a 100644
> --- a/drivers/staging/ccree/ssi_cipher.c
> +++ b/drivers/staging/ccree/ssi_cipher.c
> @@ -697,13 +697,10 @@ static int ssi_blkcipher_complete(struct device *dev,
>   void __iomem *cc_base)
>  {
> int completion_error = 0;
> -   u32 inflight_counter;
> struct ablkcipher_request *req = (struct ablkcipher_request *)areq;
>
> ssi_buffer_mgr_unmap_blkcipher_request(dev, req_ctx, ivsize, src, 
> dst);
>
> -   /*Set the inflight couter value to local variable*/
> -   inflight_counter =  ctx_p->drvdata->inflight_counter;
> /*Decrease the inflight counter*/
> if (ctx_p->flow_mode == BYPASS && ctx_p->drvdata->inflight_counter > 
> 0)
> ctx_p->drvdata->inflight_counter--;
> --
> 2.7.4
>



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

Re: [PATCH RESEND] X.509: Recognize the legacy OID 1.3.14.3.2.29 (sha1WithRSASignature)

[Carlo Caione]

On Mon, Aug 21, 2017 at 9:27 AM, Carlo Caione  wrote:
> On Mon, Aug 7, 2017 at 10:01 AM, Carlo Caione  wrote:
>> From: Carlo Caione 
>>
>> We have found some ACER laptops shipping with certificates signed using
>> the 1.3.14.3.2.29 OID. This is causing the message
> /cut
>
> Ping on this (literally) two lines patch.

any feedback on this small patch?

-- 
Carlo Caione

Re: [Outreachy kernel] [PATCH v5] Staging: ccree: Remove unused variable.

[Julia Lawall]

On Thu, 7 Sep 2017, Srishti Sharma wrote:

> Remove the local variable inflight_counter as it is never used.
>
> Signed-off-by: Srishti Sharma 

Acked-by: Julia Lawall 

> ---
> Changes in v5:
>  - Correct the format of the subject.
>  drivers/staging/ccree/ssi_cipher.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/staging/ccree/ssi_cipher.c 
> b/drivers/staging/ccree/ssi_cipher.c
> index 8d31a93..1ff3c8a 100644
> --- a/drivers/staging/ccree/ssi_cipher.c
> +++ b/drivers/staging/ccree/ssi_cipher.c
> @@ -697,13 +697,10 @@ static int ssi_blkcipher_complete(struct device *dev,
> void __iomem *cc_base)
>  {
>   int completion_error = 0;
> - u32 inflight_counter;
>   struct ablkcipher_request *req = (struct ablkcipher_request *)areq;
>
>   ssi_buffer_mgr_unmap_blkcipher_request(dev, req_ctx, ivsize, src, dst);
>
> - /*Set the inflight couter value to local variable*/
> - inflight_counter =  ctx_p->drvdata->inflight_counter;
>   /*Decrease the inflight counter*/
>   if (ctx_p->flow_mode == BYPASS && ctx_p->drvdata->inflight_counter > 0)
>   ctx_p->drvdata->inflight_counter--;
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups 
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/1504770561-5554-1-git-send-email-srishtishar%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [PATCH v5] Staging: ccree: Remove unused variable.

[Dan Carpenter]

Looks good.  Thanks!

regards,
dan carpenter

[PATCH v5] Staging: ccree: Remove unused variable.

[Srishti Sharma]

Remove the local variable inflight_counter as it is never used.

Signed-off-by: Srishti Sharma 
---
Changes in v5:
 - Correct the format of the subject.
 drivers/staging/ccree/ssi_cipher.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/ccree/ssi_cipher.c 
b/drivers/staging/ccree/ssi_cipher.c
index 8d31a93..1ff3c8a 100644
--- a/drivers/staging/ccree/ssi_cipher.c
+++ b/drivers/staging/ccree/ssi_cipher.c
@@ -697,13 +697,10 @@ static int ssi_blkcipher_complete(struct device *dev,
  void __iomem *cc_base)
 {
int completion_error = 0;
-   u32 inflight_counter;
struct ablkcipher_request *req = (struct ablkcipher_request *)areq;

ssi_buffer_mgr_unmap_blkcipher_request(dev, req_ctx, ivsize, src, dst);

-   /*Set the inflight couter value to local variable*/
-   inflight_counter =  ctx_p->drvdata->inflight_counter;
/*Decrease the inflight counter*/
if (ctx_p->flow_mode == BYPASS && ctx_p->drvdata->inflight_counter > 0)
ctx_p->drvdata->inflight_counter--;
--
2.7.4

Re: [PATCH 00/12] x86/crypto: Fix RBP usage in several crypto .S files

[Ingo Molnar]

* Eric Biggers  wrote:

> Thanks for fixing these!  I don't have time to review these in detail, but I 
> ran
> the crypto self-tests on the affected algorithms, and they all pass.  I also
> benchmarked them before and after; the only noticable performance difference 
> was
> that sha256-avx2 and sha512-avx2 became a few percent slower.  I don't suppose
> there is a way around that?  Otherwise it's probably not a big deal.

Which CPU model did you use for the test?

Thanks,

Ingo

Re: [Outreachy kernel] [PATCH v4] Staging: ccree: ssi_cipher.c: Remove unused variable.

[Srishti Sharma]

On Thu, Sep 7, 2017 at 12:24 PM, Julia Lawall  wrote:
> One last detail.  The subject line above is not in the same format as that
> of all of the other subject lines one commits affecting this file.  You
> can use git log --oneline to see what others have done.  It is not really
> possible to guess correctly.  However, one does not generally include the
> extension on a filename.
>

Yes, they have not used the filename in the subject line . So I'll change it to
the required format .

Thanks ,
Srishti

> julia
>
> On Thu, 7 Sep 2017, Srishti Sharma wrote:
>
>> Remove local variable inflight_counter, as it is never used.
>>
>> Signed-off-by: Srishti Sharma 
>> ---
>> Change in v4:
>>  -There is no longer a need to make the comment more comprehensible, as I
>>   have deleted the variable associated with it. The v3 of this patch was
>>   not based on the original patch, hence v4.
>>  drivers/staging/ccree/ssi_cipher.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/staging/ccree/ssi_cipher.c 
>> b/drivers/staging/ccree/ssi_cipher.c
>> index 8d31a93..1ff3c8a 100644
>> --- a/drivers/staging/ccree/ssi_cipher.c
>> +++ b/drivers/staging/ccree/ssi_cipher.c
>> @@ -697,13 +697,10 @@ static int ssi_blkcipher_complete(struct device *dev,
>> void __iomem *cc_base)
>>  {
>>   int completion_error = 0;
>> - u32 inflight_counter;
>>   struct ablkcipher_request *req = (struct ablkcipher_request *)areq;
>>
>>   ssi_buffer_mgr_unmap_blkcipher_request(dev, req_ctx, ivsize, src, dst);
>>
>> - /*Set the inflight couter value to local variable*/
>> - inflight_counter =  ctx_p->drvdata->inflight_counter;
>>   /*Decrease the inflight counter*/
>>   if (ctx_p->flow_mode == BYPASS && ctx_p->drvdata->inflight_counter > 0)
>>   ctx_p->drvdata->inflight_counter--;
>> --
>> 2.7.4
>>
>> --
>> You received this message because you are subscribed to the Google Groups 
>> "outreachy-kernel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an 
>> email to outreachy-kernel+unsubscr...@googlegroups.com.
>> To post to this group, send email to outreachy-ker...@googlegroups.com.
>> To view this discussion on the web visit 
>> https://groups.google.com/d/msgid/outreachy-kernel/1504735553-13753-1-git-send-email-srishtishar%40gmail.com.
>> For more options, visit https://groups.google.com/d/optout.
>>

Re: [Outreachy kernel] [PATCH v4] Staging: ccree: ssi_cipher.c: Remove unused variable.

[Julia Lawall]

One last detail.  The subject line above is not in the same format as that
of all of the other subject lines one commits affecting this file.  You
can use git log --oneline to see what others have done.  It is not really
possible to guess correctly.  However, one does not generally include the
extension on a filename.

julia

On Thu, 7 Sep 2017, Srishti Sharma wrote:

> Remove local variable inflight_counter, as it is never used.
>
> Signed-off-by: Srishti Sharma 
> ---
> Change in v4:
>  -There is no longer a need to make the comment more comprehensible, as I
>   have deleted the variable associated with it. The v3 of this patch was
>   not based on the original patch, hence v4.
>  drivers/staging/ccree/ssi_cipher.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/staging/ccree/ssi_cipher.c 
> b/drivers/staging/ccree/ssi_cipher.c
> index 8d31a93..1ff3c8a 100644
> --- a/drivers/staging/ccree/ssi_cipher.c
> +++ b/drivers/staging/ccree/ssi_cipher.c
> @@ -697,13 +697,10 @@ static int ssi_blkcipher_complete(struct device *dev,
> void __iomem *cc_base)
>  {
>   int completion_error = 0;
> - u32 inflight_counter;
>   struct ablkcipher_request *req = (struct ablkcipher_request *)areq;
>
>   ssi_buffer_mgr_unmap_blkcipher_request(dev, req_ctx, ivsize, src, dst);
>
> - /*Set the inflight couter value to local variable*/
> - inflight_counter =  ctx_p->drvdata->inflight_counter;
>   /*Decrease the inflight counter*/
>   if (ctx_p->flow_mode == BYPASS && ctx_p->drvdata->inflight_counter > 0)
>   ctx_p->drvdata->inflight_counter--;
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups 
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/1504735553-13753-1-git-send-email-srishtishar%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [PATCH v3] Staging: ccree: ssi_cipher.c: Remove unused variable.

[Joe Perches]

On Thu, 2017-09-07 at 00:32 +0300, Dan Carpenter wrote:
> Always compile your patches.
> 
>   CC [M]  drivers/staging/ccree/ssi_cipher.o
> drivers/staging/ccree/ssi_cipher.c: In function ‘ssi_blkcipher_complete’:
> drivers/staging/ccree/ssi_cipher.c:700:6: warning: unused variable 
> ‘inflight_counter’ [-Wunused-variable]
>   u32 inflight_counter;
>   ^~~~
> 
> You need to delete the declaration as well.
> 
> Don't be in a rush to resend patches.  I normally write them then let
> them sit in my outbox overnight and send them in the morning.  The extra
> delay helps me to calm down a bit and focus better.  Even though I've
> sent thousands of patches, it sometimes still stresses me out.  It's
> like you're disagreeing with the original author and the reviewers are
> disagreeing with you and everyone's trying to be nice about it but
> patches are fundamentally points of disagreement and that's stress.

True, and you shouldn't add a blank link either.
there's already one above the block you deleted.

Re: [PATCH] crypto: authenc - cryptlen must be at least AAD len

[Herbert Xu]

On Thu, Sep 07, 2017 at 07:48:53AM +0200, Stephan Müller wrote:
>
> There is already such check:
> 
> static inline int crypto_aead_decrypt(struct aead_request *req)
> {
> struct crypto_aead *aead = crypto_aead_reqtfm(req);
> 
> if (req->cryptlen < crypto_aead_authsize(aead))
> return -EINVAL;
> ...

That doesn't check assoclen, does it?

> > Perhaps we can simply
> > truncate assoclen in aead_request_set_ad.
> 
> I am not sure that would work because at the time we set the AAD len, we may 
> not yet have cryptlen. I.e. aead_request_set_ad may be called before 
> aead_request_set_crypt.

We can add the truncation in both places.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt