Re: [PATCH v2 6/6] crypto: caam - populate platform devices last

[Horia Geanta]

On 10/22/2019 6:30 PM, Andrey Smirnov wrote:
> Move the call to devm_of_platform_populate() at the end of
> caam_probe(), so we won't try to add any child devices until all of
> the initialization is finished successfully.
> 
> Signed-off-by: Andrey Smirnov 
> Cc: Chris Healy 
> Cc: Lucas Stach 
> Cc: Horia Geantă 
> Cc: Herbert Xu 
> Cc: Iuliana Prodan 
> Cc: linux-cry...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH v2 3/6] crypto: caam - use devres to de-initialize the RNG

[Horia Geanta]

On 10/22/2019 6:30 PM, Andrey Smirnov wrote:
> Use devres to de-initialize the RNG and drop explicit de-initialization
> code in caam_remove().
> 
> Signed-off-by: Andrey Smirnov 
> Cc: Chris Healy 
> Cc: Lucas Stach 
> Cc: Horia Geantă 
> Cc: Herbert Xu 
> Cc: Iuliana Prodan 
> Cc: linux-cry...@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH] crypto: caam - use the same jr for rng init/exit

[Horia Geanta]

On 9/18/2019 9:01 AM, Andrey Smirnov wrote:
> On Wed, Sep 11, 2019 at 2:35 AM Horia Geanta  wrote:
>>
>> On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
>>> In order to allow caam_jr_enqueue() to lock underlying JR's
>>> device (via device_lock(), see commit that follows) we need to make
>>> sure that no code calls caam_jr_enqueue() as a part of caam_jr_probe()
>>> to avoid a deadlock. Unfortunately, current implementation of caamrng
>>> code does exactly that in caam_init_buf().
>>>
> 
> I don't think your patch addresses the above, so it can probably be
> cut out of the message.
> 
No, it does not, it addresses the issue right below.

Not sure what you mean by "cut out of the message". If you look carefully
in the original message, at some pointer there is a scissors line.

>>> Another big problem with original caamrng initialization is a
>>> circular reference in the form of:
>>>
>>>  1. caam_rng_init() aquires JR via caam_jr_alloc(). Freed only by
>>> caam_rng_exit()
>>>
>>>  2. caam_rng_exit() is only called by unregister_algs() once last JR
>>> is shut down
>>>
>>>  3. caam_jr_remove() won't call unregister_algs() for last JR
>>> until tfm_count reaches zero, which can only happen via
>>> unregister_algs() -> caam_rng_exit() call chain.
>>>
>>> To avoid all of that, convert caamrng code to a platform device driver
>>> and extend caam_probe() to create corresponding platform device.
>>>
>>> Additionally, this change also allows us to remove any access to
>>> struct caam_drv_private in caamrng.c as well as simplify resource
>>> ownership/deallocation via devres.
>>>
>> I would avoid adding platform devices that don't have
>> corresponding DT nodes.
>>
>> There's some generic advice here:
>> https://www.kernel.org/doc/html/latest/driver-api/driver-model/platform.html#legacy-drivers-device-probing
>>
>> and there's also previous experience in caam driver:
>> 6b175685b4a1 crypto: caam/qi - don't allocate an extra platform device
>>
> 
> Hmm, I am not sure how that experience relates to the case we have
> with hwrng, but OK, I'm going to assume that platform driver approach
> is a no-go.
> 
Not specific to hwrng, but platform drivers in general:
[...]
SMMU. A platform device allocated using platform_device_register_full()
is not completely set up - most importantly .dma_configure()
is not called.

Horia

Re: [PATCH v6 12/14] crypto: caam - force DMA address to 32-bit on 64-bit i.MX SoCs

[Horia Geanta]

On 8/12/2019 10:27 PM, Andrey Smirnov wrote:
> On Mon, Aug 5, 2019 at 1:23 AM Horia Geanta  wrote:
>>
>> On 7/17/2019 6:25 PM, Andrey Smirnov wrote:
>>> @@ -603,11 +603,13 @@ static int caam_probe(struct platform_device *pdev)
>>>   ret = init_clocks(dev, ctrlpriv, imx_soc_match->data);
>>>   if (ret)
>>>   return ret;
>>> +
>>> + caam_ptr_sz = sizeof(u32);
>>> + } else {
>>> + caam_ptr_sz = sizeof(dma_addr_t);
>> caam_ptr_sz should be deduced by reading MCFGR[PS] bit, i.e. decoupled
>> from dma_addr_t.
>>
> 
> MCFGR[PS] is not mentioned in i.MX8MQ SRM and MCFG_PS in CTPR_MS is
> documented as set to "0" (seems to match in real HW as well). Doesn't
> seem like a workable solution for i.MX8MQ. Is there something I am
> missing?
> 
If CTPR_MS[PS]=0, this means CAAM does not allow choosing the "pointer size"
via MCFGR[PS]. Usually in this case the RM does not document MCFGR[PS] bit,
which is identical to MCFGR[PS]=0.

Thus the logic should be smth. like:
caam_ptr_sz = CTPR_MS[PS] && MCFGR[PS] ? 64 : 32;

>> There is another configuration that should be considered
>> (even though highly unlikely):
>> caam_ptr_sz=1  - > 32-bit addresses for CAAM
>> CONFIG_ARCH_DMA_ADDR_T_64BIT=n - 32-bit dma_addr_t
>> so the logic has to be carefully evaluated.
>>
> 
> I don't understand what you mean here. 32-bit CAAM + 32-bit dma_addr_t
> should already be the case for i.MX6, etc. how is what you describe
> different?
> 
Sorry for not being clear.

caam_ptr_sz=1  - > 32-bit addresses for CAAM
should have been
caam_ptr_sz=*64*  - > 32-bit addresses for CAAM
i.e. CAAM address has "more than" (>) 32 bits (exact number of bits is
SoC / chassis specific) and thus will be represented on 8 bytes.

Thanks,
Horia

Re: [PATCH v3 06/14] crypto: caam - check assoclen

[Horia Geanta]

On 7/25/2019 4:58 PM, Iuliana Prodan wrote:
> Check assoclen to solve the extra tests that expect -EINVAL to be
> returned when the associated data size is not valid.
> 
> Validated assoclen for RFC4106 and RFC4543 which expects an assoclen
> of 16 or 20.
> Based on seqiv, IPsec ESP and RFC4543/RFC4106 the assoclen is sizeof IP
> Header (spi, seq_no, extended seq_no) and IV len. This can be 16 or 20
> bytes.
> 
> Signed-off-by: Iuliana Prodan 
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH v3 04/14] crypto: caam - check key length

[Horia Geanta]

On 7/25/2019 4:58 PM, Iuliana Prodan wrote:
> Check key length to solve the extra tests that expect -EINVAL to be
> returned when the key size is not valid.
> 
> Validated AES keylen for skcipher, ahash and aead.
> 
> Signed-off-by: Iuliana Prodan 
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH v3 03/14] crypto: caam - update IV only when crypto operation succeeds

[Horia Geanta]

On 7/25/2019 4:58 PM, Iuliana Prodan wrote:
> From: Horia Geantă 
> 
> skcipher encryption might fail and in some cases, like (invalid) input
> length smaller then block size, updating the IV would lead to panic
> due to copying from a negative offset (req->cryptlen - ivsize).
> 
In v1 I mentioned the commit message needs to change:
https://lore.kernel.org/linux-crypto/vi1pr0402mb34855e675a58e1221ace7b9498...@vi1pr0402mb3485.eurprd04.prod.outlook.com/
which happened in v2, however somehow v3 is identical to v1.

Horia

Re: [PATCH v2 11/14] crypto: caam - free resources in case caam_rng registration failed

[Horia Geanta]

On 7/19/2019 2:58 AM, Iuliana Prodan wrote:
> Check the return value of the hardware registration for caam_rng and free
> resources in case of failure.
> 
> Fixes: 6e4e603a9 ("crypto: caam - Dynamic memory allocation for caam_rng_ctx 
> object")
This should be:
Fixes: e24f7c9e87d4 ("crypto: caam - hwrng support")

since there are resources leaked (like DMA mapped buffers) due to not checking
the return code of hwrng_register() even in the initial caamrng commit.

This doesn't have much practical value, since we haven't seen this failure
in practice and we don't intend fixing previous kernel releases.

Horia

Re: [PATCH v2 09/14] crypto: caam - keep both virtual and dma key addresses

[Horia Geanta]

On 7/19/2019 2:58 AM, Iuliana Prodan wrote:
> From: Horia Geantă 
> 
> Update alginfo struct to keep both virtual and dma key addresses,
> so that descriptors have them at hand.
> One example where this is needed is in the xcbc(aes) shared descriptors,
> which are updated in current patch.
> Another example is the upcoming fix for DKP.
> 
> Signed-off-by: Horia Geantă 
> ---
>  drivers/crypto/caam/caamhash.c  | 26 --
>  drivers/crypto/caam/caamhash_desc.c |  5 ++---
>  drivers/crypto/caam/caamhash_desc.h |  2 +-
>  drivers/crypto/caam/desc_constr.h   | 10 --
>  4 files changed, 19 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
> index 2ec4bad..14fdfa1 100644
> --- a/drivers/crypto/caam/caamhash.c
> +++ b/drivers/crypto/caam/caamhash.c
[...]
> @@ -508,6 +502,10 @@ static int axcbc_setkey(struct crypto_ahash *ahash, 
> const u8 *key,
>   memcpy(ctx->key, key, keylen);
>   dma_sync_single_for_device(jrdev, ctx->key_dma, keylen, DMA_TO_DEVICE);
>   ctx->adata.keylen = keylen;
> + /* key is loaded from memory for UPDATE and FINALIZE states */
> + ctx->adata.key_dma = ctx->key_dma;
> + /* key is immediate data for INIT and INITFINAL states */
> + ctx->adata.key_virt = ctx->key;
>  
Now that it's possible to store both virtual and dma key addresses,
it would be more efficient to move these assignments .cra_init callback.

I'll submit the changes in v3.

Horia

[...]
> diff --git a/drivers/crypto/caam/desc_constr.h 
> b/drivers/crypto/caam/desc_constr.h
> index 5988a26..8154174 100644
> --- a/drivers/crypto/caam/desc_constr.h
> +++ b/drivers/crypto/caam/desc_constr.h
> @@ -457,8 +457,8 @@ do { \
>   *   functions where it is used.
>   * @keylen: length of the provided algorithm key, in bytes
>   * @keylen_pad: padded length of the provided algorithm key, in bytes
> - * @key: address where algorithm key resides; virtual address if key_inline
> - *   is true, dma (bus) address if key_inline is false.
> + * @key_dma: dma (bus) address where algorithm key resides
> + * @key_virt: virtual address where algorithm key resides
>   * @key_inline: true - key can be inlined in the descriptor; false - key is
>   *  referenced by the descriptor
>   */
> @@ -466,10 +466,8 @@ struct alginfo {
>   u32 algtype;
>   unsigned int keylen;
>   unsigned int keylen_pad;
> - union {
> - dma_addr_t key_dma;
> - const void *key_virt;
> - };
> + dma_addr_t key_dma;
> + const void *key_virt;
>   bool key_inline;
>  };
>  
> 

Re: [PATCH v4 03/16] crypto: caam - move tasklet_init() call down

[Horia Geanta]

On 7/4/2019 2:45 AM, Leonard Crestez wrote:
> On 7/3/2019 8:14 PM, Andrey Smirnov wrote:
>> On Wed, Jul 3, 2019 at 6:51 AM Leonard Crestez  
>> wrote:
>>> On 7/3/2019 11:14 AM, Andrey Smirnov wrote:
 Move tasklet_init() call further down in order to simplify error path
 cleanup. No functional change intended.

 diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
 index 4b25b2fa3d02..a7ca2bbe243f 100644
 --- a/drivers/crypto/caam/jr.c
 +++ b/drivers/crypto/caam/jr.c
 @@ -441,15 +441,13 @@ static int caam_jr_init(struct device *dev)

jrp = dev_get_drvdata(dev);

 - tasklet_init(>irqtask, caam_jr_dequeue, (unsigned long)dev);
 -
/* Connect job ring interrupt handler. */
error = request_irq(jrp->irq, caam_jr_interrupt, IRQF_SHARED,
dev_name(dev), dev);
if (error) {
dev_err(dev, "can't connect JobR %d interrupt (%d)\n",
jrp->ridx, jrp->irq);
 - goto out_kill_deq;
 + return error;
}
>>>
>>> The caam_jr_interrupt handler can schedule the tasklet so it makes sense
>>> to have it be initialized ahead of request_irq. In theory it's possible
>>> for an interrupt to be triggered immediately when request_irq is called.
>>>
>>> I'm not very familiar with the CAAM ip, can you ensure no interrupts are
>>> pending in HW at probe time? The "no functional change" part is not obvious.
>>>
Actually there is a previous report (in i.MX BSP) of CAAM job ring generating
an interrupt at probe time, between request_irq() and reset:
https://source.codeaurora.org/external/imx/linux-imx/commit/drivers/crypto/caam?h=imx_4.14.98_2.0.0_ga=aa7b3f51971ec1f60f41fe8ea71870b215376b8a

So yes, there might be cases when interrupts are pending.

>>
>> Said tasklet will use both jrp->outring and jrp->entinfo array
>> initialized after IRQ request call in both versions of the code
>> (before/after this patch). AFAICT, the only case where this patch
>> would change initialization safety of the original code is if JR was
>> scheduled somehow while ORSFx is 0 (no jobs done), which I don't think
>> is possible.
> 
> I took a second look at caam_jr_init and there is apparently a whole 
> bunch of other reset/init stuff done after request_irq. For example 
> caam_reset_hw_jr is done after request_irq and masks interrupts?
> 
> What I'd expect is that request_irq is done last after all other 
> initialization is performed. But I'm not familiar with how CAAM JRs work 
> so feel free to ignore this.
> 
There's some history here... (which is in contradiction with above-mentioned
report).

Commit 9620fd959fb1 ("crypto: caam - handle interrupt lines shared across 
rings")
moved request_irq() before JR reset:
"
- resetting a job ring triggers an interrupt, so move request_irq
prior to jr_reset to avoid 'got IRQ but nobody cared' messages.
"

but IIUC that ("resetting a job ring triggers an interrupt") was actually
due to disabling the IRQ line using disable_irq() instead of masking
the interrupt in HW using JRCFGR_LS[IMSK].

The way JR reset sequence is implemented now - in caam_reset_hw_jr() - should 
not
trigger any interrupt.

Thus, it should be safe to move request_irq() after everything is set up,
at the end of probing.

My suggestion is to move both tasklet_init() and request_irq() at the bottom
of the probe callback.
However, I'd say this is a fix that should be marked accordingly and
should be posted either separately, or at the top of the patch set.

Thanks,
Horia

Re: [PATCH v2 06/35] crypto: Use kmemdup rather than duplicating its implementation

[Horia Geanta]

On 7/3/2019 7:27 PM, Fuqian Huang wrote:
> kmemdup is introduced to duplicate a region of memory in a neat way.
> Rather than kmalloc/kzalloc + memcpy, which the programmer needs to
> write the size twice (sometimes lead to mistakes), kmemdup improves
> readability, leads to smaller code and also reduce the chances of mistakes.
> Suggestion to use kmemdup rather than using kmalloc/kzalloc + memcpy.
> 
> Signed-off-by: Fuqian Huang 
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h

[Horia Geanta]

On 6/13/2019 3:16 PM, Christophe Leroy wrote:
> 
> 
> Le 13/06/2019 à 14:13, Horia Geanta a écrit :
>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>> Next patch will require struct talitos_edesc to be defined
>>> earlier in talitos.c
>>>
>>> This patch moves it into talitos.h so that it can be used
>>> from any place in talitos.c
>>>
>>> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on 
>>> SEC1")
>>> Cc: sta...@vger.kernel.org
>>> Signed-off-by: Christophe Leroy 
>> Again, this patch does not qualify as a fix.
>>
> 
> But as I said, the following one is a fix and require that one, you told 
> me to add stable in Cc: to make it explicit it was to go into stable.
Yes, but you should remove the Fixes tag.
And probably replace "Next patch" with the commit headline.

> If someone tries to merge following one into stable with taking that one 
> first, build will fail.
This shouldn't happen, order from main tree should be preserved.

Horia

Re: [PATCH v2 3/4] crypto: talitos - eliminate unneeded 'done' functions at build time

[Horia Geanta]

On 6/11/2019 5:39 PM, Christophe Leroy wrote:
> When building for SEC1 only, talitos2_done functions are unneeded
> and should go away.
> 
> For this, use has_ftr_sec1() which will always return true when only
> SEC1 support is being built, allowing GCC to drop TALITOS2 functions.
> 
> Signed-off-by: Christophe Leroy 
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH] crypto: caam - disable some clock checks for iMX7ULP

[Horia Geanta]

On 5/31/2019 2:06 PM, Iuliana Prodan wrote:
> Disabled the check and set of 'mem' and 'emi_slow'
> clocks, since these are not available for iMX7ULP.
> 
> Signed-off-by: Iuliana Prodan 
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH v5 1/2] crypto: caam - fix pkcs1pad(rsa-caam, sha256) failure because of invalid input

[Horia Geanta]

On 5/28/2019 12:52 PM, Iuliana Prodan wrote:
> The problem is with the input data size sent to CAAM for encrypt/decrypt.
> Pkcs1pad is failing due to pkcs1 padding done in SW starting with0x01
> instead of 0x00 0x01.
> CAAM expects an input of modulus size. For this we strip the leading
> zeros in case the size is more than modulus or pad the input with zeros
> until the modulus size is reached.
> 
> Signed-off-by: Iuliana Prodan 
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH v1 08/15] crypto: talitos - Do not modify req->cryptlen on decryption.

[Horia Geanta]

On 5/21/2019 4:34 PM, Christophe Leroy wrote:
> For decrypt, req->cryptlen includes the size of the authentication
> part while all functions of the driver expect cryptlen to be
> the size of the encrypted data.
> 
> As it is not expected to change req->cryptlen, this patch
> implements local calculation of cryptlen.
> 
An alternative would be to restore req->cryptlen in the *_done() callback.
It would be easier to implement, though probably less intuitive.

Horia

Re: [PATCH] crypto: caam: set hwrng quality level

[Horia Geanta]

On 2/13/2019 4:49 PM, Philipp Puschmann wrote:
> When quality is set to 0 this driver is not used as randomness source by
> HWRNG framework at all. So set the quality and finally make this driver
> work. The value of the quality was discussed before
> ( see see https://patchwork.kernel.org/patch/9850669/ ) and it is still a 
> rough
> value.
> 
In the same thread you mention, the conclusion is that caamrng should be
properly configured (TRNG instead of DRBG) before claiming to be a reliable
hwrng entropy source.

Horia

Re: [PATCH 0/4] crypto: caam - add ecb mode support

[Horia Geanta]

On 2/8/2019 3:51 PM, Iuliana Prodan wrote:
> This patch set adds ecb mode support for aes, des, 3des and arc4 ciphers.
> skcipher implementation is reused, making sure to handle the no IV case.
> 
For the series:
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH 0/4] crypto: caam - add ecb mode support

[Horia Geanta]

On 2/9/2019 11:52 PM, Eric Biggers wrote:
> Do you have an actual use case for adding more DES, 3DES, and ARC4
> implementations, or are you simply adding them because the hardware happens to
> supports it?  These old ciphers are insecure, so IMO more implementations 
> should
> only be added if there is a real use case where they're absolutely needed.
> 
One legit use case is PIN encryption in Point of Sale solution.

Horia

Re: [PATCH v3] crypto: caam - add missing put_device() call

[Horia Geanta]

On 2/11/2019 2:31 PM, Wen Yang wrote:
> The of_find_device_by_node() takes a reference to the underlying device
> structure, we should release that reference.
> 
> Fixes: 35af64038623 ("crypto: caam - Check for CAAM block presence before 
> registering with crypto layer")
Fixes: b189817cf789 ("crypto: caam/qi - add ablkcipher and authenc algorithms")

> Signed-off-by: Wen Yang 
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH v2] crypto: caam - add missing put_device() call

[Horia Geanta]

On 2/9/2019 8:31 AM, Wen Yang wrote:
> The of_find_device_by_node() takes a reference to the underlying device
> structure, we should release that reference.
> 
> Fixes: 35af64038623 ("crypto: caam - Check for CAAM block presence before 
> registering with crypto layer")
> Signed-off-by: Wen Yang 
> ---
> v2->v1: This patch also fixes caamalg.c, caampkc.c and caamrng.c.
> 
caamalg_qi.c is also affected, could you please include it?

Thanks,
Horia

Re: [PATCH 7/7] crypto: caam: no need to check return value of debugfs_create functions

[Horia Geanta]

On 1/22/2019 5:14 PM, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.
> 
> Cc: "Horia Geantă" 
> Cc: Aymen Sghaier 
> Cc: Herbert Xu 
> Cc: "David S. Miller" 
> Cc: linux-cry...@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman 
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH v4 2/2] crypto: talitos - fix ablkcipher for CONFIG_VMAP_STACK

[Horia Geanta]

On 1/8/2019 8:56 AM, Christophe Leroy wrote:
> [2.364486] WARNING: CPU: 0 PID: 60 at ./arch/powerpc/include/asm/io.h:837 
> dma_nommu_map_page+0x44/0xd4
> [2.373579] CPU: 0 PID: 60 Comm: cryptomgr_test Tainted: GW
>  4.20.0-rc5-00560-g6bfb52e23a00-dirty #531
> [2.384740] NIP:  c000c540 LR: c000c584 CTR: 
> [2.389743] REGS: c95abab0 TRAP: 0700   Tainted: GW  
> (4.20.0-rc5-00560-g6bfb52e23a00-dirty)
> [2.400042] MSR:  00029032   CR: 24042204  XER: 
> [2.406669]
> [2.406669] GPR00: c02f2244 c95abb60 c6262990 c95abd80 256a 0001 
> 0001 0001
> [2.406669] GPR08:  2000 0010 0010 24042202  
> 0100 c95abd88
> [2.406669] GPR16:  c05569d4 0001 0010 c95abc88 c0615664 
> 0004 
> [2.406669] GPR24: 0010 c95abc88 c95abc88  c61ae210 c7ff6d40 
> c61ae210 3d68
> [2.441559] NIP [c000c540] dma_nommu_map_page+0x44/0xd4
> [2.446720] LR [c000c584] dma_nommu_map_page+0x88/0xd4
> [2.451762] Call Trace:
> [2.454195] [c95abb60] [82000808] 0x82000808 (unreliable)
> [2.459572] [c95abb80] [c02f2244] talitos_edesc_alloc+0xbc/0x3c8
> [2.465493] [c95abbb0] [c02f2600] ablkcipher_edesc_alloc+0x4c/0x5c
> [2.471606] [c95abbd0] [c02f4ed0] ablkcipher_encrypt+0x20/0x64
> [2.477389] [c95abbe0] [c02023b0] __test_skcipher+0x4bc/0xa08
> [2.483049] [c95abe00] [c0204b60] test_skcipher+0x2c/0xcc
> [2.488385] [c95abe20] [c0204c48] alg_test_skcipher+0x48/0xbc
> [2.494064] [c95abe40] [c0205cec] alg_test+0x164/0x2e8
> [2.499142] [c95abf00] [c0200dec] cryptomgr_test+0x48/0x50
> [2.504558] [c95abf10] [c0039ff4] kthread+0xe4/0x110
> [2.509471] [c95abf40] [c000e1d0] ret_from_kernel_thread+0x14/0x1c
> [2.515532] Instruction dump:
> [2.518468] 7c7e1b78 7c9d2378 7cbf2b78 41820054 3d20c076 8089c200 3d20c076 
> 7c84e850
> [2.526127] 8129c204 7c842e70 7f844840 419c0008 <0fe0> 2f9e 
> 54847022 7c84fa14
> [2.533960] ---[ end trace bf78d94af73fe3b8 ]---
> [2.539123] talitos ff02.crypto: master data transfer error
> [2.544775] talitos ff02.crypto: TEA error: ISR 0x2000_0040
> [2.551625] alg: skcipher: encryption failed on test 1 for 
> ecb-aes-talitos: ret=22
> 
> IV cannot be on stack when CONFIG_VMAP_STACK is selected because the stack
> cannot be DMA mapped anymore.
> 
> This patch copies the IV into the extended descriptor.
> 
> Fixes: 4de9d0b547b9 ("crypto: talitos - Add ablkcipher algorithms")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Christophe Leroy 
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH v4 1/2] crypto: talitos - reorder code in talitos_edesc_alloc()

[Horia Geanta]

On 1/8/2019 8:56 AM, Christophe Leroy wrote:
> This patch moves the mapping of IV after the kmalloc(). This
> avoids having to unmap in case kmalloc() fails.
> 
> Signed-off-by: Christophe Leroy 
Reviewed-by: Horia Geantă 

Since patch 2/2 is Cc-ing stable, this one should do the same.
Herbert, could you append the commit message before applying it?

Thanks,
Horia

Re: [PATCH v3] crypto: talitos - fix ablkcipher for CONFIG_VMAP_STACK

[Horia Geanta]

On 1/4/2019 5:17 PM, Horia Geanta wrote:
> On 12/21/2018 10:07 AM, Christophe Leroy wrote:
> [snip]
>> IV cannot be on stack when CONFIG_VMAP_STACK is selected because the stack
>> cannot be DMA mapped anymore.
>> This looks better, thanks.
> 
>> This patch copies the IV into the extended descriptor when iv is not
>> a valid linear address.
>>
> Though I am not sure the checks in place are enough.
> 
>> Fixes: 4de9d0b547b9 ("crypto: talitos - Add ablkcipher algorithms")
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Christophe Leroy 
>> ---
>>  v3: Using struct edesc buffer.
>>
>>  v2: Using per-request context.
> [snip]
>> +if (ivsize && !virt_addr_valid(iv))
>> +alloc_len += ivsize;
> [snip]
>>  
>> +if (ivsize && !virt_addr_valid(iv))
> A more precise condition would be (!is_vmalloc_addr || is_vmalloc_addr(iv))
>
Sorry for the typo, I meant:
(!virt_addr_valid(iv) || is_vmalloc_addr(iv))

> It matches the checks in debug_dma_map_single() helper, though I am not sure
> they are enough to rule out all exceptions of DMA API.

Re: [PATCH v3] crypto: talitos - fix ablkcipher for CONFIG_VMAP_STACK

[Horia Geanta]

On 12/21/2018 10:07 AM, Christophe Leroy wrote:
[snip]
> IV cannot be on stack when CONFIG_VMAP_STACK is selected because the stack
> cannot be DMA mapped anymore.
> This looks better, thanks.

> This patch copies the IV into the extended descriptor when iv is not
> a valid linear address.
> 
Though I am not sure the checks in place are enough.

> Fixes: 4de9d0b547b9 ("crypto: talitos - Add ablkcipher algorithms")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Christophe Leroy 
> ---
>  v3: Using struct edesc buffer.
> 
>  v2: Using per-request context.
[snip]
> + if (ivsize && !virt_addr_valid(iv))
> + alloc_len += ivsize;
[snip]
>  
> + if (ivsize && !virt_addr_valid(iv))
A more precise condition would be (!is_vmalloc_addr || is_vmalloc_addr(iv))

It matches the checks in debug_dma_map_single() helper, though I am not sure
they are enough to rule out all exceptions of DMA API.

> + iv = memcpy(((u8 *)edesc) + alloc_len - ivsize, iv, ivsize);
> +
>   edesc->src_nents = src_nents;
>   edesc->dst_nents = dst_nents;
> - edesc->iv_dma = iv_dma;
> + if (ivsize)
> + edesc->iv_dma = dma_map_single(dev, iv, ivsize, DMA_TO_DEVICE);
> + else
> + edesc->iv_dma = 0;
> +

Re: [PATCH] crypto: caam/qi2 - add a CRYPTO_DEV_FSL_CAAM dependency

[Horia Geanta]

On 12/20/2018 5:28 PM, Arnd Bergmann wrote:
> My previous bugfix was incomplete, we still have a broken kernel
> with CRYPTO_DEV_FSL_CAAM=m and CRYPTO_DEV_FSL_DPAA2_CAAM=y:
> 
> drivers/crypto/caam/caamalg_desc.o: In function `cnstr_shdsc_aead_null_encap':
> caamalg_desc.c:(.text+0x14): undefined reference to `caam_little_end'
> drivers/crypto/caam/caamalg_desc.o: In function `cnstr_shdsc_aead_null_encap':
> caamalg_desc.c:(.text+0x310): undefined reference to `caam_imx'
> caamalg_desc.c:(.text+0x4a8): undefined reference to `caam_little_end'
> drivers/crypto/caam/caamalg_desc.o: In function `cnstr_shdsc_aead_null_encap':
> caamalg_desc.c:(.text+0x664): undefined reference to `caam_imx'
> 
> Everything is fine for the other combinations: if both are loadable
> modules, or both are built-in, the flags work as expected, also if
> only one of the two is enabled.
> 
> Add a dependency to enforce using one of the working configurations.
> Overall, I'm still not happy with that dependency, but for now
> it documents what the code requires.
> 
Thanks Arnd.
This indeed solves the issue, but as you mentioned won't allow for
CRYPTO_DEV_FSL_DPAA2_CAAM=y when CRYPTO_DEV_FSL_CAAM=m.

I think a better solution is the following:

-- >8 --
Subject: [PATCH] crypto: caam - move shared symbols in a common location

There are several issues with symbols shared b/w:
-caam/jr and caam/qi drivers on one hand
-caam/qi2 driver on the other hand

Commit 52813ab24959 ("crypto: caam/qi2 - avoid double export") fixed
some of them, however compilation still fails for CRYPTO_DEV_FSL_CAAM=m
and CRYPTO_DEV_FSL_DPAA2_CAAM=y.

Another issue is related to dependency cycles reported by depmod when
CRYPTO_DEV_FSL_CAAM=n and CRYPTO_DEV_FSL_DPAA2_CAAM=m, as mentioned in
82c7b351be3f ("Revert "arm64: defconfig: Enable FSL_MC_BUS and FSL_MC_DPIO"")

To fix all these, move the symbols shared by these drivers in a common
location. The only existing possibility is error.c file (note that naming
doesn't help and should eventually change).

Fixes: 52813ab24959 ("crypto: caam/qi2 - avoid double export")
Reported-by: Arnd Bergmann 
Signed-off-by: Horia Geantă 
---
 drivers/crypto/caam/caamalg_qi2.c | 7 ---
 drivers/crypto/caam/ctrl.c| 4 
 drivers/crypto/caam/error.c   | 6 ++
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/crypto/caam/caamalg_qi2.c 
b/drivers/crypto/caam/caamalg_qi2.c
index 425d5d974613..cc59814afd29 100644
--- a/drivers/crypto/caam/caamalg_qi2.c
+++ b/drivers/crypto/caam/caamalg_qi2.c
@@ -25,13 +25,6 @@
 #define CAAM_MAX_KEY_SIZE  (AES_MAX_KEY_SIZE + CTR_RFC3686_NONCE_SIZE + \
 SHA512_DIGEST_SIZE * 2)

-#if !IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM)
-bool caam_little_end;
-EXPORT_SYMBOL(caam_little_end);
-bool caam_imx;
-EXPORT_SYMBOL(caam_imx);
-#endif
-
 /*
  * This is a a cache of buffers, from which the users of CAAM QI driver
  * can allocate short buffers. It's speedier than doing kmalloc on the hotpath.
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index 16bbc72f041a..14fb09223156 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -18,12 +18,8 @@
 #include "desc_constr.h"
 #include "ctrl.h"

-bool caam_little_end;
-EXPORT_SYMBOL(caam_little_end);
 bool caam_dpaa2;
 EXPORT_SYMBOL(caam_dpaa2);
-bool caam_imx;
-EXPORT_SYMBOL(caam_imx);

 #ifdef CONFIG_CAAM_QI
 #include "qi.h"
diff --git a/drivers/crypto/caam/error.c b/drivers/crypto/caam/error.c
index 7e8d690f2827..21a70fd32f5d 100644
--- a/drivers/crypto/caam/error.c
+++ b/drivers/crypto/caam/error.c
@@ -50,6 +50,12 @@ void caam_dump_sg(const char *level, const char *prefix_str,
int prefix_type,
 #endif /* DEBUG */
 EXPORT_SYMBOL(caam_dump_sg);

+bool caam_little_end;
+EXPORT_SYMBOL(caam_little_end);
+
+bool caam_imx;
+EXPORT_SYMBOL(caam_imx);
+
 static const struct {
u8 value;
const char *error_text;
-- 
2.16.2

Re: [PATCH] Revert "arm64: defconfig: Enable FSL_MC_BUS and FSL_MC_DPIO"

[Horia Geanta]

On 12/20/2018 5:28 PM, Arnd Bergmann wrote:
> On Wed, Dec 19, 2018 at 11:18 AM Horia Geantă  wrote:
>>
>> This reverts commit d9678adbe733a770428a98651beaa2817d503ed3.
>>
>> Received below report from Stefan.
>> Revert the commit until CAAM driver dependency cycles are fixed.
>>
>> this patch in next-20181214 breaks "make modules_install" for
>> arm64/defconfig on my Ubuntu machine:
>>
>> DEPMOD  4.20.0-rc6-next-20181214
>> depmod: ERROR: Found 6 modules in dependency cycles!
>> depmod: ERROR: Cycle detected: caamalg_desc -> dpaa2_caam -> authenc
>> depmod: ERROR: Cycle detected: caamalg_desc -> dpaa2_caam -> fsl_mc_dpio
>> depmod: ERROR: Cycle detected: dpaa2_caam -> caamhash_desc -> dpaa2_caam
>> depmod: ERROR: Cycle detected: caamalg_desc -> dpaa2_caam -> caamhash_desc 
>> -> error
>> depmod: ERROR: Cycle detected: caamalg_desc -> dpaa2_caam -> caamhash_desc 
>> -> caamalg_desc
>>
>> Reported-by: Stefan Wahren 
>> Signed-off-by: Horia Geantă 
> 
> I've applied the revert, but I think there is still a problem, since
> anyone could manually enable those options and should not
> see those cycles in the module dependencies.
> 
Agree, root cause has to be fixed.

> Horia, can you have a look at what caused that and how to fix it?
> It's probably a commit in the caam drivers. I have also created
> a patch to address a problem with that driver but forgot
> to send that out, it might fix this one as well, see my follow-up
> patch "crypto: caam/qi2 - add a CRYPTO_DEV_FSL_CAAM
> dependency".
> 
Yes, will continue discussion there.

Thanks,
Horia

Re: [PATCH v2] crypto: talitos - fix ablkcipher for CONFIG_VMAP_STACK

[Horia Geanta]

On 12/13/2018 9:34 AM, Christophe Leroy wrote:
> [2.364486] WARNING: CPU: 0 PID: 60 at ./arch/powerpc/include/asm/io.h:837 
> dma_nommu_map_page+0x44/0xd4
> [2.373579] CPU: 0 PID: 60 Comm: cryptomgr_test Tainted: GW
>  4.20.0-rc5-00560-g6bfb52e23a00-dirty #531
> [2.384740] NIP:  c000c540 LR: c000c584 CTR: 
> [2.389743] REGS: c95abab0 TRAP: 0700   Tainted: GW  
> (4.20.0-rc5-00560-g6bfb52e23a00-dirty)
> [2.400042] MSR:  00029032   CR: 24042204  XER: 
> [2.406669]
> [2.406669] GPR00: c02f2244 c95abb60 c6262990 c95abd80 256a 0001 
> 0001 0001
> [2.406669] GPR08:  2000 0010 0010 24042202  
> 0100 c95abd88
> [2.406669] GPR16:  c05569d4 0001 0010 c95abc88 c0615664 
> 0004 
> [2.406669] GPR24: 0010 c95abc88 c95abc88  c61ae210 c7ff6d40 
> c61ae210 3d68
> [2.441559] NIP [c000c540] dma_nommu_map_page+0x44/0xd4
> [2.446720] LR [c000c584] dma_nommu_map_page+0x88/0xd4
> [2.451762] Call Trace:
> [2.454195] [c95abb60] [82000808] 0x82000808 (unreliable)
> [2.459572] [c95abb80] [c02f2244] talitos_edesc_alloc+0xbc/0x3c8
> [2.465493] [c95abbb0] [c02f2600] ablkcipher_edesc_alloc+0x4c/0x5c
> [2.471606] [c95abbd0] [c02f4ed0] ablkcipher_encrypt+0x20/0x64
> [2.477389] [c95abbe0] [c02023b0] __test_skcipher+0x4bc/0xa08
> [2.483049] [c95abe00] [c0204b60] test_skcipher+0x2c/0xcc
> [2.488385] [c95abe20] [c0204c48] alg_test_skcipher+0x48/0xbc
> [2.494064] [c95abe40] [c0205cec] alg_test+0x164/0x2e8
> [2.499142] [c95abf00] [c0200dec] cryptomgr_test+0x48/0x50
> [2.504558] [c95abf10] [c0039ff4] kthread+0xe4/0x110
> [2.509471] [c95abf40] [c000e1d0] ret_from_kernel_thread+0x14/0x1c
> [2.515532] Instruction dump:
> [2.518468] 7c7e1b78 7c9d2378 7cbf2b78 41820054 3d20c076 8089c200 3d20c076 
> 7c84e850
> [2.526127] 8129c204 7c842e70 7f844840 419c0008 <0fe0> 2f9e 
> 54847022 7c84fa14
> [2.533960] ---[ end trace bf78d94af73fe3b8 ]---
> [2.539123] talitos ff02.crypto: master data transfer error
> [2.544775] talitos ff02.crypto: TEA error: ISR 0x2000_0040
> [2.551625] alg: skcipher: encryption failed on test 1 for 
> ecb-aes-talitos: ret=22
> 
> IV cannot be on stack when CONFIG_VMAP_STACK is selected because the stack
> cannot be DMA mapped anymore.
> 
Same failure could happen for aead.

> This patch copies the IV from areq->info into the request context.
> 
There is already a per-request structure - talitos_edesc - that should be used
to save the IV.

The best approach to fix the issue (both for ablkcipher and aead) would be to
update talitos_edesc_alloc().

Thanks,
Horia

Re: [PATCH v3 7/9] arm64: defconfig: Enable FSL_MC_BUS and FSL_MC_DPIO

[Horia Geanta]

On 12/15/2018 11:44 PM, Stefan Wahren wrote:
> Hi,
> 
>> Olof Johansson  hat am 13. Dezember 2018 um 07:48 
>> geschrieben:
>>
>>
>> On Fri, Nov 09, 2018 at 06:05:24AM +, Horia Geanta wrote:
>>> On 11/9/2018 3:11 AM, Marc Gonzalez wrote:
>>>> Commit e8342cc7954e ("enable CAAM crypto engine on QorIQ DPAA2 SoCs")
>>>> enabled CRYPTO_DEV_FSL_DPAA2_CAAM, which depends on FSL_MC_DPIO,
>>>> which is not set. Enable FSL_MC_BUS, and build FSL_MC_DPIO and
>>>> CRYPTO_DEV_FSL_DPAA2_CAAM as modules.
>>>>
>>>> Signed-off-by: Marc Gonzalez 
>>> Reviewed-by: Horia Geantă 
>>
>> I had to redo this one, and when I did I noticed that there's also an 
>> ethernet
>> driver. Should that be enabled as well?
>>
> 
> this patch in next-20181214 breaks "make modules_install" for arm64/defconfig 
> on my Ubuntu machine:
> 
> DEPMOD  4.20.0-rc6-next-20181214
> depmod: ERROR: Found 6 modules in dependency cycles!
> depmod: ERROR: Cycle detected: caamalg_desc -> dpaa2_caam -> authenc
> depmod: ERROR: Cycle detected: caamalg_desc -> dpaa2_caam -> fsl_mc_dpio
> depmod: ERROR: Cycle detected: dpaa2_caam -> caamhash_desc -> dpaa2_caam
> depmod: ERROR: Cycle detected: caamalg_desc -> dpaa2_caam -> caamhash_desc -> 
> error
> depmod: ERROR: Cycle detected: caamalg_desc -> dpaa2_caam -> caamhash_desc -> 
> caamalg_desc
> 
> After reverting of this patch the issue disappeared.
> 
Seems there's a dependency cycle b/w dpaa2_caam and caam{alg,hash}_desc, as 
follows:

A->B
---
dpaa2_caam needs cnstr_* exported by caam{alg,hash}_desc

B->A
---
caam{alg,hash}_desc need caam_imx, caam_little_end:
caam{hash,alg}_desc.c
--> desc_constr.h (included for descriptors generation functions)
--> regs.h (included for endianness helpers)
--> extern bool caam_imx, caam_little_end
caam_imx, caam_little_end are exported by dpaa2_caam - caamalg_qi2.c (when
CONFIG_CRYPTO_DEV_FSL_CAAM=n)

Could we drop this patch, until CAAM driver gets a proper fix (which is not
straightforward)?

Thanks,
Horia

Re: [PATCH 4/5] soc: fsl: dpio: add a device_link at dpaa2_io_service_register

[Horia Geanta]

On 12/10/2018 6:50 PM, Ioana Ciornei wrote:
> Automatically add a device link between the actual device requesting the
> dpaa2_io_service_register and the underlying dpaa2_io used. This link
> will ensure that when a DPIO device, which is indirectly used by other
> devices, is unbound any consumer devices will be also unbound from their
> drivers.
> 
> For example, any DPNI, bound to the dpaa2-eth driver, which is using
> DPIO devices will be unbound before its supplier device.
> 
> Also, add a new parameter to the dpaa2_io_service_[de]register functions
> to specify the requesting device (ie the consumer).
> 
> Signed-off-by: Ioana Ciornei 
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH v3 7/9] arm64: defconfig: Enable FSL_MC_BUS and FSL_MC_DPIO

[Horia Geanta]

On 11/9/2018 3:11 AM, Marc Gonzalez wrote:
> Commit e8342cc7954e ("enable CAAM crypto engine on QorIQ DPAA2 SoCs")
> enabled CRYPTO_DEV_FSL_DPAA2_CAAM, which depends on FSL_MC_DPIO,
> which is not set. Enable FSL_MC_BUS, and build FSL_MC_DPIO and
> CRYPTO_DEV_FSL_DPAA2_CAAM as modules.
> 
> Signed-off-by: Marc Gonzalez 
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH v3 7/9] arm64: defconfig: Enable FSL_MC_BUS and FSL_MC_DPIO

[Horia Geanta]

On 11/9/2018 3:11 AM, Marc Gonzalez wrote:
> Commit e8342cc7954e ("enable CAAM crypto engine on QorIQ DPAA2 SoCs")
> enabled CRYPTO_DEV_FSL_DPAA2_CAAM, which depends on FSL_MC_DPIO,
> which is not set. Enable FSL_MC_BUS, and build FSL_MC_DPIO and
> CRYPTO_DEV_FSL_DPAA2_CAAM as modules.
> 
> Signed-off-by: Marc Gonzalez 
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH 7/9] arm64: defconfig: Drop CRYPTO_DEV_FSL_DPAA2_CAAM

[Horia Geanta]

On 11/8/2018 3:23 PM, Marc Gonzalez wrote:
> 
> 
> On November 8, 2018 12:38:10 PM GMT+01:00, Horia Geanta 
>  wrote:
>> On 11/8/2018 1:23 PM, Horia Geanta wrote:
>>> On 11/7/2018 10:49 PM, Marc Gonzalez wrote:
>>>> [ Add interested parties ]
>>>>
>>>> On 07/11/2018 21:18, Marc Gonzalez wrote:
>>>>
>>>>> Commit e8342cc7954e ("enable CAAM crypto engine on QorIQ DPAA2
>> SoCs")
>>>>> enabled CRYPTO_DEV_FSL_DPAA2_CAAM, which depends on FSL_MC_DPIO,
>>>>> which is not set.
>>>>>
>>> Thanks Marc.
>>>
>>> Though I would prefer enabling the dependencies:
>>> dpaa2-caam driver -> dpio driver (FSL_MC_DPIO) -> fsl-mc driver
>> (FSL_MC_BUS).
>>>
>> Could you instead incorporate below patch in your series?
>> (I am pretty sure it won't apply clean)
>>
>> Thanks,
>> Horia
>>
>> -- >8 --
>>
>> Subject: [PATCH] arm64: defconfig: enable DPAA2 fsl-mc bus and DPIO
>> driver
>>
>> Enable fsl-mc bus and DPIO (Datapath I/O) driver, which are needed on
>> Layerscape SoCs with QorIQ Data Path Acceleration Architecture (DPAA)
>> v2.
>>
>> Suggested-by: Marc Gonzalez 
>> Signed-off-by: Horia Geantă 
>> ---
>> arch/arm64/configs/defconfig | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm64/configs/defconfig
>> b/arch/arm64/configs/defconfig
>> index c9a57d11330b..7c3785e61ef9 100644
>> --- a/arch/arm64/configs/defconfig
>> +++ b/arch/arm64/configs/defconfig
>> @@ -181,6 +181,7 @@ CONFIG_DMA_CMA=y
>> CONFIG_CMA_SIZE_MBYTES=32
>> CONFIG_HISILICON_LPC=y
>> CONFIG_SIMPLE_PM_BUS=y
>> +CONFIG_FSL_MC_BUS=y
>> CONFIG_MTD=y
>> CONFIG_MTD_BLOCK=y
>> CONFIG_MTD_M25P80=y
>> @@ -615,6 +616,7 @@ CONFIG_QCOM_IOMMU=y
>> CONFIG_RPMSG_QCOM_GLINK_RPM=y
>> CONFIG_RPMSG_QCOM_SMD=y
>> CONFIG_RASPBERRYPI_POWER=y
>> +CONFIG_FSL_MC_DPIO=y
>> CONFIG_QCOM_SMEM=y
>> CONFIG_QCOM_SMD_RPM=y
>> CONFIG_QCOM_SMP2P=y
>> -- 
>> 2.16.2
> 
> Are CRYPTO_DEV_FSL_DPAA2_CAAM and CONFIG_FSL_MC_DPIO required to boot the 
> platform? If not, we will build them as modules. I can send a v3 tonight with 
> that change, as well as the various Acks.
> 
No, the platform is able to boot without them, so I am ok with:
+CONFIG_FSL_MC_BUS=y
+CONFIG_FSL_MC_DPIO=m
-CONFIG_CRYPTO_DEV_FSL_DPAA2_CAAM=y
+CONFIG_CRYPTO_DEV_FSL_DPAA2_CAAM=m

Thanks,
Horia

Re: [PATCH 7/9] arm64: defconfig: Drop CRYPTO_DEV_FSL_DPAA2_CAAM

[Horia Geanta]

On 11/8/2018 3:23 PM, Marc Gonzalez wrote:
> 
> 
> On November 8, 2018 12:38:10 PM GMT+01:00, Horia Geanta 
>  wrote:
>> On 11/8/2018 1:23 PM, Horia Geanta wrote:
>>> On 11/7/2018 10:49 PM, Marc Gonzalez wrote:
>>>> [ Add interested parties ]
>>>>
>>>> On 07/11/2018 21:18, Marc Gonzalez wrote:
>>>>
>>>>> Commit e8342cc7954e ("enable CAAM crypto engine on QorIQ DPAA2
>> SoCs")
>>>>> enabled CRYPTO_DEV_FSL_DPAA2_CAAM, which depends on FSL_MC_DPIO,
>>>>> which is not set.
>>>>>
>>> Thanks Marc.
>>>
>>> Though I would prefer enabling the dependencies:
>>> dpaa2-caam driver -> dpio driver (FSL_MC_DPIO) -> fsl-mc driver
>> (FSL_MC_BUS).
>>>
>> Could you instead incorporate below patch in your series?
>> (I am pretty sure it won't apply clean)
>>
>> Thanks,
>> Horia
>>
>> -- >8 --
>>
>> Subject: [PATCH] arm64: defconfig: enable DPAA2 fsl-mc bus and DPIO
>> driver
>>
>> Enable fsl-mc bus and DPIO (Datapath I/O) driver, which are needed on
>> Layerscape SoCs with QorIQ Data Path Acceleration Architecture (DPAA)
>> v2.
>>
>> Suggested-by: Marc Gonzalez 
>> Signed-off-by: Horia Geantă 
>> ---
>> arch/arm64/configs/defconfig | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm64/configs/defconfig
>> b/arch/arm64/configs/defconfig
>> index c9a57d11330b..7c3785e61ef9 100644
>> --- a/arch/arm64/configs/defconfig
>> +++ b/arch/arm64/configs/defconfig
>> @@ -181,6 +181,7 @@ CONFIG_DMA_CMA=y
>> CONFIG_CMA_SIZE_MBYTES=32
>> CONFIG_HISILICON_LPC=y
>> CONFIG_SIMPLE_PM_BUS=y
>> +CONFIG_FSL_MC_BUS=y
>> CONFIG_MTD=y
>> CONFIG_MTD_BLOCK=y
>> CONFIG_MTD_M25P80=y
>> @@ -615,6 +616,7 @@ CONFIG_QCOM_IOMMU=y
>> CONFIG_RPMSG_QCOM_GLINK_RPM=y
>> CONFIG_RPMSG_QCOM_SMD=y
>> CONFIG_RASPBERRYPI_POWER=y
>> +CONFIG_FSL_MC_DPIO=y
>> CONFIG_QCOM_SMEM=y
>> CONFIG_QCOM_SMD_RPM=y
>> CONFIG_QCOM_SMP2P=y
>> -- 
>> 2.16.2
> 
> Are CRYPTO_DEV_FSL_DPAA2_CAAM and CONFIG_FSL_MC_DPIO required to boot the 
> platform? If not, we will build them as modules. I can send a v3 tonight with 
> that change, as well as the various Acks.
> 
No, the platform is able to boot without them, so I am ok with:
+CONFIG_FSL_MC_BUS=y
+CONFIG_FSL_MC_DPIO=m
-CONFIG_CRYPTO_DEV_FSL_DPAA2_CAAM=y
+CONFIG_CRYPTO_DEV_FSL_DPAA2_CAAM=m

Thanks,
Horia

Re: [PATCH 7/9] arm64: defconfig: Drop CRYPTO_DEV_FSL_DPAA2_CAAM

[Horia Geanta]

On 11/8/2018 1:23 PM, Horia Geanta wrote:
> On 11/7/2018 10:49 PM, Marc Gonzalez wrote:
>> [ Add interested parties ]
>>
>> On 07/11/2018 21:18, Marc Gonzalez wrote:
>>
>>> Commit e8342cc7954e ("enable CAAM crypto engine on QorIQ DPAA2 SoCs")
>>> enabled CRYPTO_DEV_FSL_DPAA2_CAAM, which depends on FSL_MC_DPIO,
>>> which is not set.
>>>
> Thanks Marc.
> 
> Though I would prefer enabling the dependencies:
> dpaa2-caam driver -> dpio driver (FSL_MC_DPIO) -> fsl-mc driver (FSL_MC_BUS).
> 
Could you instead incorporate below patch in your series?
(I am pretty sure it won't apply clean)

Thanks,
Horia

-- >8 --

Subject: [PATCH] arm64: defconfig: enable DPAA2 fsl-mc bus and DPIO driver

Enable fsl-mc bus and DPIO (Datapath I/O) driver, which are needed on
Layerscape SoCs with QorIQ Data Path Acceleration Architecture (DPAA) v2.

Suggested-by: Marc Gonzalez 
Signed-off-by: Horia Geantă 
---
 arch/arm64/configs/defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index c9a57d11330b..7c3785e61ef9 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -181,6 +181,7 @@ CONFIG_DMA_CMA=y
 CONFIG_CMA_SIZE_MBYTES=32
 CONFIG_HISILICON_LPC=y
 CONFIG_SIMPLE_PM_BUS=y
+CONFIG_FSL_MC_BUS=y
 CONFIG_MTD=y
 CONFIG_MTD_BLOCK=y
 CONFIG_MTD_M25P80=y
@@ -615,6 +616,7 @@ CONFIG_QCOM_IOMMU=y
 CONFIG_RPMSG_QCOM_GLINK_RPM=y
 CONFIG_RPMSG_QCOM_SMD=y
 CONFIG_RASPBERRYPI_POWER=y
+CONFIG_FSL_MC_DPIO=y
 CONFIG_QCOM_SMEM=y
 CONFIG_QCOM_SMD_RPM=y
 CONFIG_QCOM_SMP2P=y
-- 
2.16.2

Re: [PATCH 7/9] arm64: defconfig: Drop CRYPTO_DEV_FSL_DPAA2_CAAM

[Horia Geanta]

On 11/8/2018 1:23 PM, Horia Geanta wrote:
> On 11/7/2018 10:49 PM, Marc Gonzalez wrote:
>> [ Add interested parties ]
>>
>> On 07/11/2018 21:18, Marc Gonzalez wrote:
>>
>>> Commit e8342cc7954e ("enable CAAM crypto engine on QorIQ DPAA2 SoCs")
>>> enabled CRYPTO_DEV_FSL_DPAA2_CAAM, which depends on FSL_MC_DPIO,
>>> which is not set.
>>>
> Thanks Marc.
> 
> Though I would prefer enabling the dependencies:
> dpaa2-caam driver -> dpio driver (FSL_MC_DPIO) -> fsl-mc driver (FSL_MC_BUS).
> 
Could you instead incorporate below patch in your series?
(I am pretty sure it won't apply clean)

Thanks,
Horia

-- >8 --

Subject: [PATCH] arm64: defconfig: enable DPAA2 fsl-mc bus and DPIO driver

Enable fsl-mc bus and DPIO (Datapath I/O) driver, which are needed on
Layerscape SoCs with QorIQ Data Path Acceleration Architecture (DPAA) v2.

Suggested-by: Marc Gonzalez 
Signed-off-by: Horia Geantă 
---
 arch/arm64/configs/defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index c9a57d11330b..7c3785e61ef9 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -181,6 +181,7 @@ CONFIG_DMA_CMA=y
 CONFIG_CMA_SIZE_MBYTES=32
 CONFIG_HISILICON_LPC=y
 CONFIG_SIMPLE_PM_BUS=y
+CONFIG_FSL_MC_BUS=y
 CONFIG_MTD=y
 CONFIG_MTD_BLOCK=y
 CONFIG_MTD_M25P80=y
@@ -615,6 +616,7 @@ CONFIG_QCOM_IOMMU=y
 CONFIG_RPMSG_QCOM_GLINK_RPM=y
 CONFIG_RPMSG_QCOM_SMD=y
 CONFIG_RASPBERRYPI_POWER=y
+CONFIG_FSL_MC_DPIO=y
 CONFIG_QCOM_SMEM=y
 CONFIG_QCOM_SMD_RPM=y
 CONFIG_QCOM_SMP2P=y
-- 
2.16.2

Re: [PATCH 7/9] arm64: defconfig: Drop CRYPTO_DEV_FSL_DPAA2_CAAM

[Horia Geanta]

On 11/7/2018 10:49 PM, Marc Gonzalez wrote:
> [ Add interested parties ]
> 
> On 07/11/2018 21:18, Marc Gonzalez wrote:
> 
>> Commit e8342cc7954e ("enable CAAM crypto engine on QorIQ DPAA2 SoCs")
>> enabled CRYPTO_DEV_FSL_DPAA2_CAAM, which depends on FSL_MC_DPIO,
>> which is not set.
>>
Thanks Marc.

Though I would prefer enabling the dependencies:
dpaa2-caam driver -> dpio driver (FSL_MC_DPIO) -> fsl-mc driver (FSL_MC_BUS).

Horia

Re: [PATCH 7/9] arm64: defconfig: Drop CRYPTO_DEV_FSL_DPAA2_CAAM

[Horia Geanta]

On 11/7/2018 10:49 PM, Marc Gonzalez wrote:
> [ Add interested parties ]
> 
> On 07/11/2018 21:18, Marc Gonzalez wrote:
> 
>> Commit e8342cc7954e ("enable CAAM crypto engine on QorIQ DPAA2 SoCs")
>> enabled CRYPTO_DEV_FSL_DPAA2_CAAM, which depends on FSL_MC_DPIO,
>> which is not set.
>>
Thanks Marc.

Though I would prefer enabling the dependencies:
dpaa2-caam driver -> dpio driver (FSL_MC_DPIO) -> fsl-mc driver (FSL_MC_BUS).

Horia

Re: [PATCH v1 1/2] bus: mc-bus: Add support for mapping shareable portals

[Horia Geanta]

On 10/30/2018 10:31 PM, Roy Pledge wrote:
> Starting with v5 of NXP QBMan devices the hardware supports using
> regular cacheable/shareable memory as the backing store for the
> portals.
> 
> This patch adds support for the new portal mode by switching to
> use the DPRC get object region v2 command which returns both
> a base address and offset for the portal memory. The new portal
> region is identified as shareable through the addition of a new
> flag.
> 
> Signed-off-by: Roy Pledge 
> ---
>  drivers/bus/fsl-mc/dprc.c   |  3 ++-
>  drivers/bus/fsl-mc/fsl-mc-bus.c | 14 --
>  drivers/bus/fsl-mc/fsl-mc-private.h | 17 ++---
>  3 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/bus/fsl-mc/dprc.c b/drivers/bus/fsl-mc/dprc.c
> index 1c3f621..bde856d 100644
> --- a/drivers/bus/fsl-mc/dprc.c
> +++ b/drivers/bus/fsl-mc/dprc.c
> @@ -461,8 +461,9 @@ int dprc_get_obj_region(struct fsl_mc_io *mc_io,
>  
>   /* retrieve response parameters */
>   rsp_params = (struct dprc_rsp_get_obj_region *)cmd.params;
> - region_desc->base_offset = le64_to_cpu(rsp_params->base_addr);
> + region_desc->base_offset = le64_to_cpu(rsp_params->base_offset);
>   region_desc->size = le32_to_cpu(rsp_params->size);
> + region_desc->base_address = le64_to_cpu(rsp_params->base_addr);
>  
>   return 0;
>  }
> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
> index f0404c6..25ad422 100644
> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> @@ -487,10 +487,18 @@ static int fsl_mc_device_get_mmio_regions(struct 
> fsl_mc_device *mc_dev,
>   "dprc_get_obj_region() failed: %d\n", error);
>   goto error_cleanup_regions;
>   }
> -
> - error = translate_mc_addr(mc_dev, mc_region_type,
> + /* Older MC only returned region offset and no base address
Nitpick: comment style is not consistent with existing code in fsl-mc.

> +  * If base address is in the region_desc use it otherwise
> +  * revert to old mechanism
> +  */
> + if (region_desc.base_address)
> + regions[i].start = region_desc.base_address +
> + region_desc.base_offset;
> + else
> + error = translate_mc_addr(mc_dev, mc_region_type,
> region_desc.base_offset,
> [i].start);
> +
>   if (error < 0) {
>   dev_err(parent_dev,
>   "Invalid MC offset: %#x (for %s.%d\'s region 
> %d)\n",
> @@ -504,6 +512,8 @@ static int fsl_mc_device_get_mmio_regions(struct 
> fsl_mc_device *mc_dev,
>   regions[i].flags = IORESOURCE_IO;
>   if (region_desc.flags & DPRC_REGION_CACHEABLE)
>   regions[i].flags |= IORESOURCE_CACHEABLE;
> + if (region_desc.flags & DPRC_REGION_SHAREABLE)
> + regions[i].flags |= IORESOURCE_MEM;
>   }
>  
>   mc_dev->regions = regions;
> diff --git a/drivers/bus/fsl-mc/fsl-mc-private.h 
> b/drivers/bus/fsl-mc/fsl-mc-private.h
> index ea11b4f..28e40d1 100644
> --- a/drivers/bus/fsl-mc/fsl-mc-private.h
> +++ b/drivers/bus/fsl-mc/fsl-mc-private.h
> @@ -79,9 +79,11 @@ int dpmcp_reset(struct fsl_mc_io *mc_io,
>  
>  /* DPRC command versioning */
>  #define DPRC_CMD_BASE_VERSION1
> +#define DPRC_CMD_2ND_VERSION 2
>  #define DPRC_CMD_ID_OFFSET   4
>  
>  #define DPRC_CMD(id) (((id) << DPRC_CMD_ID_OFFSET) | DPRC_CMD_BASE_VERSION)
> +#define DPRC_CMD_V2(id)  (((id) << DPRC_CMD_ID_OFFSET) | 
> DPRC_CMD_2ND_VERSION)
>  
>  /* DPRC command IDs */
>  #define DPRC_CMDID_CLOSEDPRC_CMD(0x800)
> @@ -99,7 +101,7 @@ int dpmcp_reset(struct fsl_mc_io *mc_io,
>  #define DPRC_CMDID_GET_CONT_ID  DPRC_CMD(0x830)
>  #define DPRC_CMDID_GET_OBJ_COUNTDPRC_CMD(0x159)
>  #define DPRC_CMDID_GET_OBJ  DPRC_CMD(0x15A)
> -#define DPRC_CMDID_GET_OBJ_REG  DPRC_CMD(0x15E)
> +#define DPRC_CMDID_GET_OBJ_REG  DPRC_CMD_V2(0x15E)
>  #define DPRC_CMDID_SET_OBJ_IRQ  DPRC_CMD(0x15F)
>  
>  struct dprc_cmd_open {
> @@ -199,9 +201,15 @@ struct dprc_rsp_get_obj_region {
>   /* response word 0 */
>   __le64 pad;
>   /* response word 1 */
> - __le64 base_addr;
> + __le64 base_offset;
>   /* response word 2 */
>   __le32 size;
> + u8 pad2[3];
Padding size (3B) is incorrect.

Either add 4B of padding:
__le32 pad2;

or expose the "type" field:
u8 type;
u8 pad2[3];

> + /* response word 3 */
> + __le32 flags;
> + __le32 pad3;
> + /* response word 4 */
> + __le64 base_addr;
>  };
>  
>  struct dprc_cmd_set_obj_irq {
> @@ -334,6 

Re: [PATCH v1 1/2] bus: mc-bus: Add support for mapping shareable portals

[Horia Geanta]

On 10/30/2018 10:31 PM, Roy Pledge wrote:
> Starting with v5 of NXP QBMan devices the hardware supports using
> regular cacheable/shareable memory as the backing store for the
> portals.
> 
> This patch adds support for the new portal mode by switching to
> use the DPRC get object region v2 command which returns both
> a base address and offset for the portal memory. The new portal
> region is identified as shareable through the addition of a new
> flag.
> 
> Signed-off-by: Roy Pledge 
> ---
>  drivers/bus/fsl-mc/dprc.c   |  3 ++-
>  drivers/bus/fsl-mc/fsl-mc-bus.c | 14 --
>  drivers/bus/fsl-mc/fsl-mc-private.h | 17 ++---
>  3 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/bus/fsl-mc/dprc.c b/drivers/bus/fsl-mc/dprc.c
> index 1c3f621..bde856d 100644
> --- a/drivers/bus/fsl-mc/dprc.c
> +++ b/drivers/bus/fsl-mc/dprc.c
> @@ -461,8 +461,9 @@ int dprc_get_obj_region(struct fsl_mc_io *mc_io,
>  
>   /* retrieve response parameters */
>   rsp_params = (struct dprc_rsp_get_obj_region *)cmd.params;
> - region_desc->base_offset = le64_to_cpu(rsp_params->base_addr);
> + region_desc->base_offset = le64_to_cpu(rsp_params->base_offset);
>   region_desc->size = le32_to_cpu(rsp_params->size);
> + region_desc->base_address = le64_to_cpu(rsp_params->base_addr);
>  
>   return 0;
>  }
> diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
> index f0404c6..25ad422 100644
> --- a/drivers/bus/fsl-mc/fsl-mc-bus.c
> +++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
> @@ -487,10 +487,18 @@ static int fsl_mc_device_get_mmio_regions(struct 
> fsl_mc_device *mc_dev,
>   "dprc_get_obj_region() failed: %d\n", error);
>   goto error_cleanup_regions;
>   }
> -
> - error = translate_mc_addr(mc_dev, mc_region_type,
> + /* Older MC only returned region offset and no base address
Nitpick: comment style is not consistent with existing code in fsl-mc.

> +  * If base address is in the region_desc use it otherwise
> +  * revert to old mechanism
> +  */
> + if (region_desc.base_address)
> + regions[i].start = region_desc.base_address +
> + region_desc.base_offset;
> + else
> + error = translate_mc_addr(mc_dev, mc_region_type,
> region_desc.base_offset,
> [i].start);
> +
>   if (error < 0) {
>   dev_err(parent_dev,
>   "Invalid MC offset: %#x (for %s.%d\'s region 
> %d)\n",
> @@ -504,6 +512,8 @@ static int fsl_mc_device_get_mmio_regions(struct 
> fsl_mc_device *mc_dev,
>   regions[i].flags = IORESOURCE_IO;
>   if (region_desc.flags & DPRC_REGION_CACHEABLE)
>   regions[i].flags |= IORESOURCE_CACHEABLE;
> + if (region_desc.flags & DPRC_REGION_SHAREABLE)
> + regions[i].flags |= IORESOURCE_MEM;
>   }
>  
>   mc_dev->regions = regions;
> diff --git a/drivers/bus/fsl-mc/fsl-mc-private.h 
> b/drivers/bus/fsl-mc/fsl-mc-private.h
> index ea11b4f..28e40d1 100644
> --- a/drivers/bus/fsl-mc/fsl-mc-private.h
> +++ b/drivers/bus/fsl-mc/fsl-mc-private.h
> @@ -79,9 +79,11 @@ int dpmcp_reset(struct fsl_mc_io *mc_io,
>  
>  /* DPRC command versioning */
>  #define DPRC_CMD_BASE_VERSION1
> +#define DPRC_CMD_2ND_VERSION 2
>  #define DPRC_CMD_ID_OFFSET   4
>  
>  #define DPRC_CMD(id) (((id) << DPRC_CMD_ID_OFFSET) | DPRC_CMD_BASE_VERSION)
> +#define DPRC_CMD_V2(id)  (((id) << DPRC_CMD_ID_OFFSET) | 
> DPRC_CMD_2ND_VERSION)
>  
>  /* DPRC command IDs */
>  #define DPRC_CMDID_CLOSEDPRC_CMD(0x800)
> @@ -99,7 +101,7 @@ int dpmcp_reset(struct fsl_mc_io *mc_io,
>  #define DPRC_CMDID_GET_CONT_ID  DPRC_CMD(0x830)
>  #define DPRC_CMDID_GET_OBJ_COUNTDPRC_CMD(0x159)
>  #define DPRC_CMDID_GET_OBJ  DPRC_CMD(0x15A)
> -#define DPRC_CMDID_GET_OBJ_REG  DPRC_CMD(0x15E)
> +#define DPRC_CMDID_GET_OBJ_REG  DPRC_CMD_V2(0x15E)
>  #define DPRC_CMDID_SET_OBJ_IRQ  DPRC_CMD(0x15F)
>  
>  struct dprc_cmd_open {
> @@ -199,9 +201,15 @@ struct dprc_rsp_get_obj_region {
>   /* response word 0 */
>   __le64 pad;
>   /* response word 1 */
> - __le64 base_addr;
> + __le64 base_offset;
>   /* response word 2 */
>   __le32 size;
> + u8 pad2[3];
Padding size (3B) is incorrect.

Either add 4B of padding:
__le32 pad2;

or expose the "type" field:
u8 type;
u8 pad2[3];

> + /* response word 3 */
> + __le32 flags;
> + __le32 pad3;
> + /* response word 4 */
> + __le64 base_addr;
>  };
>  
>  struct dprc_cmd_set_obj_irq {
> @@ -334,6 

Re: [PATCH 5/5] arm64: dts: add LX2160ARDB board support

[Horia Geanta]

On 8/29/2018 3:31 AM, Scott Wood wrote:
> On Tue, 2018-08-21 at 15:45 -0500, Rob Herring wrote:
>> On Mon, Aug 20, 2018 at 1:52 PM Vabhav Sharma  wrote:
>>> +/ {
>>> +   model = "NXP Layerscape LX2160ARDB";
>>> +   compatible = "fsl,lx2160a-rdb", "fsl,lx2160a";
>>> +
>>> +   aliases {
>>> +   crypto = 
>>
>> Drop this. Aliases should be numbered, and this is not a standard
>> alias name either.
> 
> Is this a new rule?  In any case, U-Boot looks for a "crypto" alias.
> 
(Replying here, I did not see a follow-up).

Indeed, U-boot relies on the "crypto" alias.
This is true for all SoCs with CAAM crypto engine, a pretty lengthy list.

Could you please clarify?
Also: Is numbering needed even when there is a single instance of the block?

Looking at a recent discussion
https://lore.kernel.org/patchwork/patch/991718
I see the proposal is for the ID to be optional:
> Alias names are often suffixed with a numeric ID, especially when there may
> be multiple instances of the same type. The ID typically corresponds to the
[...]

Thanks,
Horia

Re: [PATCH 5/5] arm64: dts: add LX2160ARDB board support

[Horia Geanta]

On 8/29/2018 3:31 AM, Scott Wood wrote:
> On Tue, 2018-08-21 at 15:45 -0500, Rob Herring wrote:
>> On Mon, Aug 20, 2018 at 1:52 PM Vabhav Sharma  wrote:
>>> +/ {
>>> +   model = "NXP Layerscape LX2160ARDB";
>>> +   compatible = "fsl,lx2160a-rdb", "fsl,lx2160a";
>>> +
>>> +   aliases {
>>> +   crypto = 
>>
>> Drop this. Aliases should be numbered, and this is not a standard
>> alias name either.
> 
> Is this a new rule?  In any case, U-Boot looks for a "crypto" alias.
> 
(Replying here, I did not see a follow-up).

Indeed, U-boot relies on the "crypto" alias.
This is true for all SoCs with CAAM crypto engine, a pretty lengthy list.

Could you please clarify?
Also: Is numbering needed even when there is a single instance of the block?

Looking at a recent discussion
https://lore.kernel.org/patchwork/patch/991718
I see the proposal is for the ID to be optional:
> Alias names are often suffixed with a numeric ID, especially when there may
> be multiple instances of the same type. The ID typically corresponds to the
[...]

Thanks,
Horia

Re: [PATCH v3 0/4] staging/fsl-mc/bus: Move DPIO from staging to drivers/soc/fsl

[Horia Geanta]

On 7/24/2018 5:21 PM, Roy Pledge wrote:
> Move the NXP DPIO (Datapath I/O driver) from the staging/fsl-mc/bus/dpio
> directory to the drivers/soc/fsl directory.
> 
> The DPIO driver enables access to the Queue and Buffer Managemer (QBMAN)
> hardware of NXP DPAA2 devices. This is a prerequiste for moving the DPAA2
> Ethernet device driver from the staging directory.
> 
For the series:
Reviewed-by: Horia Geantă 

Thanks,
Horia

> Horia Geantă (1):
>   staging: fsl-dpaa2/eth: move generic FD defines to DPIO
> 
> Roy Pledge (3):
>   staging:fsl-mc: Move DPIO from staging to drivers/soc/fsl
>   drivers/staging: Remove fsl-mc driver from staging
>   drivers/soc/fsl/dpio: Convert DPIO documentation to .rst
> 
>  .../networking/dpaa2/dpio-driver.rst   | 29 
> +++---
>  Documentation/networking/dpaa2/index.rst   |  1 +
>  MAINTAINERS|  2 +-
>  drivers/crypto/caam/sg_sw_qm2.h|  2 +-
>  drivers/crypto/caam/sg_sw_sec4.h   |  2 +-
>  drivers/soc/fsl/Kconfig| 10 
>  drivers/soc/fsl/Makefile   |  1 +
>  .../{staging/fsl-mc/bus => soc/fsl}/dpio/Makefile  |  0
>  .../fsl-mc/bus => soc/fsl}/dpio/dpio-cmd.h |  0
>  .../fsl-mc/bus => soc/fsl}/dpio/dpio-driver.c  |  2 +-
>  .../fsl-mc/bus => soc/fsl}/dpio/dpio-service.c |  2 +-
>  .../{staging/fsl-mc/bus => soc/fsl}/dpio/dpio.c|  0
>  .../{staging/fsl-mc/bus => soc/fsl}/dpio/dpio.h|  0
>  .../fsl-mc/bus => soc/fsl}/dpio/qbman-portal.c |  2 +-
>  .../fsl-mc/bus => soc/fsl}/dpio/qbman-portal.h |  2 +-
>  drivers/staging/Kconfig|  2 --
>  drivers/staging/Makefile   |  1 -
>  drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c |  4 +--
>  drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h | 22 ++--
>  drivers/staging/fsl-mc/Kconfig |  2 --
>  drivers/staging/fsl-mc/Makefile|  3 ---
>  drivers/staging/fsl-mc/bus/Kconfig | 16 
>  drivers/staging/fsl-mc/bus/Makefile|  9 ---
>  .../fsl-mc/include => include/soc/fsl}/dpaa2-fd.h  | 12 +
>  .../include => include/soc/fsl}/dpaa2-global.h |  0
>  .../fsl-mc/include => include/soc/fsl}/dpaa2-io.h  |  0
>  26 files changed, 66 insertions(+), 60 deletions(-)
>  rename drivers/staging/fsl-mc/bus/dpio/dpio-driver.txt => 
> Documentation/networking/dpaa2/dpio-driver.rst (95%)
>  rename drivers/{staging/fsl-mc/bus => soc/fsl}/dpio/Makefile (100%)
>  rename drivers/{staging/fsl-mc/bus => soc/fsl}/dpio/dpio-cmd.h (100%)
>  rename drivers/{staging/fsl-mc/bus => soc/fsl}/dpio/dpio-driver.c (99%)
>  rename drivers/{staging/fsl-mc/bus => soc/fsl}/dpio/dpio-service.c (99%)
>  rename drivers/{staging/fsl-mc/bus => soc/fsl}/dpio/dpio.c (100%)
>  rename drivers/{staging/fsl-mc/bus => soc/fsl}/dpio/dpio.h (100%)
>  rename drivers/{staging/fsl-mc/bus => soc/fsl}/dpio/qbman-portal.c (99%)
>  rename drivers/{staging/fsl-mc/bus => soc/fsl}/dpio/qbman-portal.h (99%)
>  delete mode 100644 drivers/staging/fsl-mc/Kconfig
>  delete mode 100644 drivers/staging/fsl-mc/Makefile
>  delete mode 100644 drivers/staging/fsl-mc/bus/Kconfig
>  delete mode 100644 drivers/staging/fsl-mc/bus/Makefile
>  rename {drivers/staging/fsl-mc/include => include/soc/fsl}/dpaa2-fd.h (97%)
>  rename {drivers/staging/fsl-mc/include => include/soc/fsl}/dpaa2-global.h 
> (100%)
>  rename {drivers/staging/fsl-mc/include => include/soc/fsl}/dpaa2-io.h (100%)
> 

Re: [PATCH v3 0/4] staging/fsl-mc/bus: Move DPIO from staging to drivers/soc/fsl

[Horia Geanta]

On 7/24/2018 5:21 PM, Roy Pledge wrote:
> Move the NXP DPIO (Datapath I/O driver) from the staging/fsl-mc/bus/dpio
> directory to the drivers/soc/fsl directory.
> 
> The DPIO driver enables access to the Queue and Buffer Managemer (QBMAN)
> hardware of NXP DPAA2 devices. This is a prerequiste for moving the DPAA2
> Ethernet device driver from the staging directory.
> 
For the series:
Reviewed-by: Horia Geantă 

Thanks,
Horia

> Horia Geantă (1):
>   staging: fsl-dpaa2/eth: move generic FD defines to DPIO
> 
> Roy Pledge (3):
>   staging:fsl-mc: Move DPIO from staging to drivers/soc/fsl
>   drivers/staging: Remove fsl-mc driver from staging
>   drivers/soc/fsl/dpio: Convert DPIO documentation to .rst
> 
>  .../networking/dpaa2/dpio-driver.rst   | 29 
> +++---
>  Documentation/networking/dpaa2/index.rst   |  1 +
>  MAINTAINERS|  2 +-
>  drivers/crypto/caam/sg_sw_qm2.h|  2 +-
>  drivers/crypto/caam/sg_sw_sec4.h   |  2 +-
>  drivers/soc/fsl/Kconfig| 10 
>  drivers/soc/fsl/Makefile   |  1 +
>  .../{staging/fsl-mc/bus => soc/fsl}/dpio/Makefile  |  0
>  .../fsl-mc/bus => soc/fsl}/dpio/dpio-cmd.h |  0
>  .../fsl-mc/bus => soc/fsl}/dpio/dpio-driver.c  |  2 +-
>  .../fsl-mc/bus => soc/fsl}/dpio/dpio-service.c |  2 +-
>  .../{staging/fsl-mc/bus => soc/fsl}/dpio/dpio.c|  0
>  .../{staging/fsl-mc/bus => soc/fsl}/dpio/dpio.h|  0
>  .../fsl-mc/bus => soc/fsl}/dpio/qbman-portal.c |  2 +-
>  .../fsl-mc/bus => soc/fsl}/dpio/qbman-portal.h |  2 +-
>  drivers/staging/Kconfig|  2 --
>  drivers/staging/Makefile   |  1 -
>  drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c |  4 +--
>  drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h | 22 ++--
>  drivers/staging/fsl-mc/Kconfig |  2 --
>  drivers/staging/fsl-mc/Makefile|  3 ---
>  drivers/staging/fsl-mc/bus/Kconfig | 16 
>  drivers/staging/fsl-mc/bus/Makefile|  9 ---
>  .../fsl-mc/include => include/soc/fsl}/dpaa2-fd.h  | 12 +
>  .../include => include/soc/fsl}/dpaa2-global.h |  0
>  .../fsl-mc/include => include/soc/fsl}/dpaa2-io.h  |  0
>  26 files changed, 66 insertions(+), 60 deletions(-)
>  rename drivers/staging/fsl-mc/bus/dpio/dpio-driver.txt => 
> Documentation/networking/dpaa2/dpio-driver.rst (95%)
>  rename drivers/{staging/fsl-mc/bus => soc/fsl}/dpio/Makefile (100%)
>  rename drivers/{staging/fsl-mc/bus => soc/fsl}/dpio/dpio-cmd.h (100%)
>  rename drivers/{staging/fsl-mc/bus => soc/fsl}/dpio/dpio-driver.c (99%)
>  rename drivers/{staging/fsl-mc/bus => soc/fsl}/dpio/dpio-service.c (99%)
>  rename drivers/{staging/fsl-mc/bus => soc/fsl}/dpio/dpio.c (100%)
>  rename drivers/{staging/fsl-mc/bus => soc/fsl}/dpio/dpio.h (100%)
>  rename drivers/{staging/fsl-mc/bus => soc/fsl}/dpio/qbman-portal.c (99%)
>  rename drivers/{staging/fsl-mc/bus => soc/fsl}/dpio/qbman-portal.h (99%)
>  delete mode 100644 drivers/staging/fsl-mc/Kconfig
>  delete mode 100644 drivers/staging/fsl-mc/Makefile
>  delete mode 100644 drivers/staging/fsl-mc/bus/Kconfig
>  delete mode 100644 drivers/staging/fsl-mc/bus/Makefile
>  rename {drivers/staging/fsl-mc/include => include/soc/fsl}/dpaa2-fd.h (97%)
>  rename {drivers/staging/fsl-mc/include => include/soc/fsl}/dpaa2-global.h 
> (100%)
>  rename {drivers/staging/fsl-mc/include => include/soc/fsl}/dpaa2-io.h (100%)
> 

Re: [PATCH v2 4/4] drivers/soc/fsl/dpio: Convert DPIO documentation to .rst

[Horia Geanta]

On 7/23/2018 8:00 PM, Roy Pledge wrote:
> Convert the Datapath I/O documentation to .rst format
> and move to the Documation/networking/dpaa2 directory
typo: ^^^ Documentation

> 
> Signed-off-by: Roy Pledge 
> ---
>  .../networking/dpaa2/dpio-driver.rst   | 30 
> +++---
>  Documentation/networking/dpaa2/index.rst   |  1 +
>  2 files changed, 28 insertions(+), 3 deletions(-)
>  rename drivers/soc/fsl/dpio/dpio-driver.txt => 
> Documentation/networking/dpaa2/dpio-driver.rst (94%)
> 
> diff --git a/drivers/soc/fsl/dpio/dpio-driver.txt 
> b/Documentation/networking/dpaa2/dpio-driver.rst
> similarity index 94%
> rename from drivers/soc/fsl/dpio/dpio-driver.txt
> rename to Documentation/networking/dpaa2/dpio-driver.rst
> index 72ba9da..3a33a33 100644
> --- a/drivers/soc/fsl/dpio/dpio-driver.txt
> +++ b/Documentation/networking/dpaa2/dpio-driver.rst
> @@ -1,7 +1,16 @@
> -Copyright 2016 NXP
> +.. include:: 
> +
> +DPAA2 DPIO (Data Path I/O) Overview
> +===
> +
> +:Copyright: |copy| 2015 Freescale Semiconductor Inc.
> +:Copyright: |copy| 2018 NXP
> +
Existing copyright should be kept.
2018 could be appended if needed.

Regards,
Horia

Re: [PATCH v2 4/4] drivers/soc/fsl/dpio: Convert DPIO documentation to .rst

[Horia Geanta]

On 7/23/2018 8:00 PM, Roy Pledge wrote:
> Convert the Datapath I/O documentation to .rst format
> and move to the Documation/networking/dpaa2 directory
typo: ^^^ Documentation

> 
> Signed-off-by: Roy Pledge 
> ---
>  .../networking/dpaa2/dpio-driver.rst   | 30 
> +++---
>  Documentation/networking/dpaa2/index.rst   |  1 +
>  2 files changed, 28 insertions(+), 3 deletions(-)
>  rename drivers/soc/fsl/dpio/dpio-driver.txt => 
> Documentation/networking/dpaa2/dpio-driver.rst (94%)
> 
> diff --git a/drivers/soc/fsl/dpio/dpio-driver.txt 
> b/Documentation/networking/dpaa2/dpio-driver.rst
> similarity index 94%
> rename from drivers/soc/fsl/dpio/dpio-driver.txt
> rename to Documentation/networking/dpaa2/dpio-driver.rst
> index 72ba9da..3a33a33 100644
> --- a/drivers/soc/fsl/dpio/dpio-driver.txt
> +++ b/Documentation/networking/dpaa2/dpio-driver.rst
> @@ -1,7 +1,16 @@
> -Copyright 2016 NXP
> +.. include:: 
> +
> +DPAA2 DPIO (Data Path I/O) Overview
> +===
> +
> +:Copyright: |copy| 2015 Freescale Semiconductor Inc.
> +:Copyright: |copy| 2018 NXP
> +
Existing copyright should be kept.
2018 could be appended if needed.

Regards,
Horia

Re: [PATCH 0/2] staging/fsl-mc/bus: Move DPIO from staging to drivers/soc/fsl

[Horia Geanta]

On 7/5/2018 10:41 PM, Roy Pledge wrote:
> Move the NXP DPIO (Datapath I/O driver) from the staging/fsl-mc/bus/dpio
> directory to the drivers/soc/fsl directory.
> 
> The DPIO driver enables access to the Queue and Buffer Managemer (QBMAN)
> hardware of NXP DPAA2 devices. This is a prerequiste for moving the DPAA2
> Ethernet device driver from the staging directory.
> 
Roy, Ioana,

Would it be ok to add the following patch on top of the series?
It's a dependency for dpseci object.
If not added now I fear it won't be accepted until dpaa2-ethernet
moves out of staging, thus gating dpseci upstreaming.

Thanks,
Horia

--->8---
Previous commits:
6e2387e8f19ed ("staging: fsl-dpaa2/eth: Add Freescale DPAA2 Ethernet driver")
39163c0ce0f48 ("staging: fsl-dpaa2/eth: Errors checking update")
added bits that are not specific to the WRIOP accelerator.

Move these where they belong (in DPIO) such that other accelerators
can make use of them.

Signed-off-by: Horia Geantă 
---
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c |  4 ++--
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h | 18 +-
 drivers/staging/fsl-mc/include/dpaa2-fd.h  | 12 
 3 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c 
b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
index 396371728aa1..d5f0ac5c2d1f 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
@@ -455,7 +455,7 @@ static int build_sg_fd(struct dpaa2_eth_priv *priv,
dpaa2_fd_set_format(fd, dpaa2_fd_sg);
dpaa2_fd_set_addr(fd, addr);
dpaa2_fd_set_len(fd, skb->len);
-   dpaa2_fd_set_ctrl(fd, DPAA2_FD_CTRL_PTA | DPAA2_FD_CTRL_PTV1);
+   dpaa2_fd_set_ctrl(fd, FD_CTRL_PTA | FD_CTRL_PTV1);

if (priv->tx_tstamp && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
enable_tx_tstamp(fd, sgt_buf);
@@ -508,7 +508,7 @@ static int build_single_fd(struct dpaa2_eth_priv *priv,
dpaa2_fd_set_offset(fd, (u16)(skb->data - buffer_start));
dpaa2_fd_set_len(fd, skb->len);
dpaa2_fd_set_format(fd, dpaa2_fd_single);
-   dpaa2_fd_set_ctrl(fd, DPAA2_FD_CTRL_PTA | DPAA2_FD_CTRL_PTV1);
+   dpaa2_fd_set_ctrl(fd, FD_CTRL_PTA | FD_CTRL_PTV1);

if (priv->tx_tstamp && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
enable_tx_tstamp(fd, buffer_start);
diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h 
b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h
index 905a4e6be8fa..9269cb05a84b 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h
@@ -124,21 +124,13 @@ struct dpaa2_eth_swa {
 #define DPAA2_FD_FRC_FAICFDV   0x0400

 /* Error bits in FD CTRL */
-#define DPAA2_FD_CTRL_UFD  0x0004
-#define DPAA2_FD_CTRL_SBE  0x0008
-#define DPAA2_FD_CTRL_FSE  0x0020
-#define DPAA2_FD_CTRL_FAERR0x0040
-
-#define DPAA2_FD_RX_ERR_MASK   (DPAA2_FD_CTRL_SBE  | \
-DPAA2_FD_CTRL_FAERR)
-#define DPAA2_FD_TX_ERR_MASK   (DPAA2_FD_CTRL_UFD  | \
-DPAA2_FD_CTRL_SBE  | \
-DPAA2_FD_CTRL_FSE  | \
-DPAA2_FD_CTRL_FAERR)
+#define DPAA2_FD_RX_ERR_MASK   (FD_CTRL_SBE | FD_CTRL_FAERR)
+#define DPAA2_FD_TX_ERR_MASK   (FD_CTRL_UFD| \
+FD_CTRL_SBE| \
+FD_CTRL_FSE| \
+FD_CTRL_FAERR)

 /* Annotation bits in FD CTRL */
-#define DPAA2_FD_CTRL_PTA  0x0080
-#define DPAA2_FD_CTRL_PTV1 0x0040
 #define DPAA2_FD_CTRL_ASAL 0x0002  /* ASAL = 128B */

 /* Frame annotation status */
diff --git a/drivers/staging/fsl-mc/include/dpaa2-fd.h 
b/drivers/staging/fsl-mc/include/dpaa2-fd.h
index b55b89ba4eda..2576abaa7779 100644
--- a/drivers/staging/fsl-mc/include/dpaa2-fd.h
+++ b/drivers/staging/fsl-mc/include/dpaa2-fd.h
@@ -67,6 +67,18 @@ struct dpaa2_fd {
 #define SG_FINAL_FLAG_MASK 0x1
 #define SG_FINAL_FLAG_SHIFT15

+/* Error bits in FD CTRL */
+#define FD_CTRL_ERR_MASK   0x00FF
+#define FD_CTRL_UFD0x0004
+#define FD_CTRL_SBE0x0008
+#define FD_CTRL_FLC0x0010
+#define FD_CTRL_FSE0x0020
+#define FD_CTRL_FAERR  0x0040
+
+/* Annotation bits in FD CTRL */
+#define FD_CTRL_PTA0x0080
+#define FD_CTRL_PTV1   0x0040
+
 enum dpaa2_fd_format {
dpaa2_fd_single = 0,
dpaa2_fd_list,
-- 
2.16.2

Re: [PATCH 0/2] staging/fsl-mc/bus: Move DPIO from staging to drivers/soc/fsl

[Horia Geanta]

On 7/5/2018 10:41 PM, Roy Pledge wrote:
> Move the NXP DPIO (Datapath I/O driver) from the staging/fsl-mc/bus/dpio
> directory to the drivers/soc/fsl directory.
> 
> The DPIO driver enables access to the Queue and Buffer Managemer (QBMAN)
> hardware of NXP DPAA2 devices. This is a prerequiste for moving the DPAA2
> Ethernet device driver from the staging directory.
> 
Roy, Ioana,

Would it be ok to add the following patch on top of the series?
It's a dependency for dpseci object.
If not added now I fear it won't be accepted until dpaa2-ethernet
moves out of staging, thus gating dpseci upstreaming.

Thanks,
Horia

--->8---
Previous commits:
6e2387e8f19ed ("staging: fsl-dpaa2/eth: Add Freescale DPAA2 Ethernet driver")
39163c0ce0f48 ("staging: fsl-dpaa2/eth: Errors checking update")
added bits that are not specific to the WRIOP accelerator.

Move these where they belong (in DPIO) such that other accelerators
can make use of them.

Signed-off-by: Horia Geantă 
---
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c |  4 ++--
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h | 18 +-
 drivers/staging/fsl-mc/include/dpaa2-fd.h  | 12 
 3 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c 
b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
index 396371728aa1..d5f0ac5c2d1f 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
@@ -455,7 +455,7 @@ static int build_sg_fd(struct dpaa2_eth_priv *priv,
dpaa2_fd_set_format(fd, dpaa2_fd_sg);
dpaa2_fd_set_addr(fd, addr);
dpaa2_fd_set_len(fd, skb->len);
-   dpaa2_fd_set_ctrl(fd, DPAA2_FD_CTRL_PTA | DPAA2_FD_CTRL_PTV1);
+   dpaa2_fd_set_ctrl(fd, FD_CTRL_PTA | FD_CTRL_PTV1);

if (priv->tx_tstamp && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
enable_tx_tstamp(fd, sgt_buf);
@@ -508,7 +508,7 @@ static int build_single_fd(struct dpaa2_eth_priv *priv,
dpaa2_fd_set_offset(fd, (u16)(skb->data - buffer_start));
dpaa2_fd_set_len(fd, skb->len);
dpaa2_fd_set_format(fd, dpaa2_fd_single);
-   dpaa2_fd_set_ctrl(fd, DPAA2_FD_CTRL_PTA | DPAA2_FD_CTRL_PTV1);
+   dpaa2_fd_set_ctrl(fd, FD_CTRL_PTA | FD_CTRL_PTV1);

if (priv->tx_tstamp && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)
enable_tx_tstamp(fd, buffer_start);
diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h 
b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h
index 905a4e6be8fa..9269cb05a84b 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.h
@@ -124,21 +124,13 @@ struct dpaa2_eth_swa {
 #define DPAA2_FD_FRC_FAICFDV   0x0400

 /* Error bits in FD CTRL */
-#define DPAA2_FD_CTRL_UFD  0x0004
-#define DPAA2_FD_CTRL_SBE  0x0008
-#define DPAA2_FD_CTRL_FSE  0x0020
-#define DPAA2_FD_CTRL_FAERR0x0040
-
-#define DPAA2_FD_RX_ERR_MASK   (DPAA2_FD_CTRL_SBE  | \
-DPAA2_FD_CTRL_FAERR)
-#define DPAA2_FD_TX_ERR_MASK   (DPAA2_FD_CTRL_UFD  | \
-DPAA2_FD_CTRL_SBE  | \
-DPAA2_FD_CTRL_FSE  | \
-DPAA2_FD_CTRL_FAERR)
+#define DPAA2_FD_RX_ERR_MASK   (FD_CTRL_SBE | FD_CTRL_FAERR)
+#define DPAA2_FD_TX_ERR_MASK   (FD_CTRL_UFD| \
+FD_CTRL_SBE| \
+FD_CTRL_FSE| \
+FD_CTRL_FAERR)

 /* Annotation bits in FD CTRL */
-#define DPAA2_FD_CTRL_PTA  0x0080
-#define DPAA2_FD_CTRL_PTV1 0x0040
 #define DPAA2_FD_CTRL_ASAL 0x0002  /* ASAL = 128B */

 /* Frame annotation status */
diff --git a/drivers/staging/fsl-mc/include/dpaa2-fd.h 
b/drivers/staging/fsl-mc/include/dpaa2-fd.h
index b55b89ba4eda..2576abaa7779 100644
--- a/drivers/staging/fsl-mc/include/dpaa2-fd.h
+++ b/drivers/staging/fsl-mc/include/dpaa2-fd.h
@@ -67,6 +67,18 @@ struct dpaa2_fd {
 #define SG_FINAL_FLAG_MASK 0x1
 #define SG_FINAL_FLAG_SHIFT15

+/* Error bits in FD CTRL */
+#define FD_CTRL_ERR_MASK   0x00FF
+#define FD_CTRL_UFD0x0004
+#define FD_CTRL_SBE0x0008
+#define FD_CTRL_FLC0x0010
+#define FD_CTRL_FSE0x0020
+#define FD_CTRL_FAERR  0x0040
+
+/* Annotation bits in FD CTRL */
+#define FD_CTRL_PTA0x0080
+#define FD_CTRL_PTV1   0x0040
+
 enum dpaa2_fd_format {
dpaa2_fd_single = 0,
dpaa2_fd_list,
-- 
2.16.2

Re: [PATCH] crypto: caam: do not register AES-XTS mode on LP units

[Horia Geanta Neag]

On 11/5/2016 1:17 AM, Sven Ebenfeld wrote:
> When using AES-XTS on a Wandboard, we receive a Mode error:
> caam_jr 2102000.jr1: 20001311: CCB: desc idx 19: AES: Mode error.
> 
> Due to the Security Reference Manual, the Low Power AES units
s/Due to/According to

> of the i.MX6 do not support the XTS mode. Therefore we should
> try to provide them them in the API.
> 
Rephrase: Therefore we mustn't register XTS implementations to Crypto
API in this case.

> Signed-off-by: Sven Ebenfeld 
Reviewed-by: Horia Geantă 

Please send the patch to -stable and mention the offending commit:
Cc:  # 4.4+
Fixes: c6415a6016bf "crypto: caam - add support for acipher xts(aes)"

Thanks,
Horia

> ---
>  drivers/crypto/caam/caamalg.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
> index 156aad1..f5a63ba 100644
> --- a/drivers/crypto/caam/caamalg.c
> +++ b/drivers/crypto/caam/caamalg.c
> @@ -4583,6 +4583,15 @@ static int __init caam_algapi_init(void)
>   if (!aes_inst && (alg_sel == OP_ALG_ALGSEL_AES))
>   continue;
>  
> + /*
> +  * Check support for AES modes not available
> +  * on LP devices.
> +  */
> + if ((cha_vid & CHA_ID_LS_AES_MASK) == CHA_ID_LS_AES_LP)
> + if ((alg->class1_alg_type & OP_ALG_AAI_MASK) ==
> +  OP_ALG_AAI_XTS)
> + continue;
> +
>   t_alg = caam_alg_alloc(alg);
>   if (IS_ERR(t_alg)) {
>   err = PTR_ERR(t_alg);
> 

Re: [PATCH] crypto: caam: do not register AES-XTS mode on LP units

[Horia Geanta Neag]

On 11/5/2016 1:17 AM, Sven Ebenfeld wrote:
> When using AES-XTS on a Wandboard, we receive a Mode error:
> caam_jr 2102000.jr1: 20001311: CCB: desc idx 19: AES: Mode error.
> 
> Due to the Security Reference Manual, the Low Power AES units
s/Due to/According to

> of the i.MX6 do not support the XTS mode. Therefore we should
> try to provide them them in the API.
> 
Rephrase: Therefore we mustn't register XTS implementations to Crypto
API in this case.

> Signed-off-by: Sven Ebenfeld 
Reviewed-by: Horia Geantă 

Please send the patch to -stable and mention the offending commit:
Cc:  # 4.4+
Fixes: c6415a6016bf "crypto: caam - add support for acipher xts(aes)"

Thanks,
Horia

> ---
>  drivers/crypto/caam/caamalg.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
> index 156aad1..f5a63ba 100644
> --- a/drivers/crypto/caam/caamalg.c
> +++ b/drivers/crypto/caam/caamalg.c
> @@ -4583,6 +4583,15 @@ static int __init caam_algapi_init(void)
>   if (!aes_inst && (alg_sel == OP_ALG_ALGSEL_AES))
>   continue;
>  
> + /*
> +  * Check support for AES modes not available
> +  * on LP devices.
> +  */
> + if ((cha_vid & CHA_ID_LS_AES_MASK) == CHA_ID_LS_AES_LP)
> + if ((alg->class1_alg_type & OP_ALG_AAI_MASK) ==
> +  OP_ALG_AAI_XTS)
> + continue;
> +
>   t_alg = caam_alg_alloc(alg);
>   if (IS_ERR(t_alg)) {
>   err = PTR_ERR(t_alg);
> 

Re: [PATCH] crypto: caam: fix type mismatch warning

[Horia Geanta Neag]

On 10/26/2016 12:29 AM, Arnd Bergmann wrote:
> Building the caam driver on arm64 produces a harmless warning:
> 
> drivers/crypto/caam/caamalg.c:140:139: warning: comparison of distinct 
> pointer types lacks a cast
> 
> We can use min_t to tell the compiler which type we want it to use
> here.
> 
> Fixes: 5ecf8ef9103c ("crypto: caam - fix sg dump")
> Signed-off-by: Arnd Bergmann 
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH] crypto: caam: fix type mismatch warning

[Horia Geanta Neag]

On 10/26/2016 12:29 AM, Arnd Bergmann wrote:
> Building the caam driver on arm64 produces a harmless warning:
> 
> drivers/crypto/caam/caamalg.c:140:139: warning: comparison of distinct 
> pointer types lacks a cast
> 
> We can use min_t to tell the compiler which type we want it to use
> here.
> 
> Fixes: 5ecf8ef9103c ("crypto: caam - fix sg dump")
> Signed-off-by: Arnd Bergmann 
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH] crypto: caam: add support for iMX6UL

[Horia Geanta Neag]

On 10/4/2016 10:33 AM, Marcus Folkesson wrote:
> i.MX6UL does only require three clocks to enable CAAM module.
> 
> Signed-off-by: Marcus Folkesson 
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH] crypto: caam: add support for iMX6UL

[Horia Geanta Neag]

On 10/4/2016 10:33 AM, Marcus Folkesson wrote:
> i.MX6UL does only require three clocks to enable CAAM module.
> 
> Signed-off-by: Marcus Folkesson 
Reviewed-by: Horia Geantă 

Thanks,
Horia

Re: [PATCH 1/6] crypto-caamhash: Use kmalloc_array() in ahash_setkey()

[Horia Geanta Neag]

On 9/15/2016 5:43 PM, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Thu, 15 Sep 2016 11:20:09 +0200
> 
> * A multiplication for the size determination of a memory allocation
>   indicated that an array data structure should be processed.
>   Thus use the corresponding function "kmalloc_array".
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Replace the specification of a data type by a pointer dereference
>   to make the corresponding size determination a bit safer according to
>   the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring 
> ---
>  drivers/crypto/caam/caamhash.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
> index 9d7fc9e..f19df8f 100644
> --- a/drivers/crypto/caam/caamhash.c
> +++ b/drivers/crypto/caam/caamhash.c
> @@ -525,8 +525,9 @@ static int ahash_setkey(struct crypto_ahash *ahash,
>  #endif
>  
>   if (keylen > blocksize) {
> - hashed_key = kmalloc(sizeof(u8) * digestsize, GFP_KERNEL |
> -  GFP_DMA);
> + hashed_key = kmalloc_array(digestsize,
> +sizeof(*hashed_key),
> +GFP_KERNEL | GFP_DMA);
While correct, instead I would go with kmalloc() and get rid of sizeof(u8).

Horia

Re: [PATCH 1/6] crypto-caamhash: Use kmalloc_array() in ahash_setkey()

[Horia Geanta Neag]

On 9/15/2016 5:43 PM, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Thu, 15 Sep 2016 11:20:09 +0200
> 
> * A multiplication for the size determination of a memory allocation
>   indicated that an array data structure should be processed.
>   Thus use the corresponding function "kmalloc_array".
> 
>   This issue was detected by using the Coccinelle software.
> 
> * Replace the specification of a data type by a pointer dereference
>   to make the corresponding size determination a bit safer according to
>   the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring 
> ---
>  drivers/crypto/caam/caamhash.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
> index 9d7fc9e..f19df8f 100644
> --- a/drivers/crypto/caam/caamhash.c
> +++ b/drivers/crypto/caam/caamhash.c
> @@ -525,8 +525,9 @@ static int ahash_setkey(struct crypto_ahash *ahash,
>  #endif
>  
>   if (keylen > blocksize) {
> - hashed_key = kmalloc(sizeof(u8) * digestsize, GFP_KERNEL |
> -  GFP_DMA);
> + hashed_key = kmalloc_array(digestsize,
> +sizeof(*hashed_key),
> +GFP_KERNEL | GFP_DMA);
While correct, instead I would go with kmalloc() and get rid of sizeof(u8).

Horia

Re: crypto-caamhash: Fine-tuning for several function implementations

[Horia Geanta Neag]

On 9/15/2016 5:37 PM, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Thu, 15 Sep 2016 16:27:23 +0200
> 
> Some update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (6):
>   Use kmalloc_array() in ahash_setkey()
>   Rename jump labels in ahash_setkey()
>   Rename a jump label in five functions
>   Return a value directly in caam_hash_cra_init()
>   Delete an unnecessary initialisation in seven functions
>   Move common error handling code in two functions
> 
>  drivers/crypto/caam/caamhash.c | 111 
> +++--
>  1 file changed, 52 insertions(+), 59 deletions(-)
> 
Thanks Markus!

For patches 2-6:
Reviewed-by: Horia Geantă 

though headline prefix should be changed to "crypto: caam -"

Horia

Re: crypto-caamhash: Fine-tuning for several function implementations

[Horia Geanta Neag]

On 9/15/2016 5:37 PM, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Thu, 15 Sep 2016 16:27:23 +0200
> 
> Some update suggestions were taken into account
> from static source code analysis.
> 
> Markus Elfring (6):
>   Use kmalloc_array() in ahash_setkey()
>   Rename jump labels in ahash_setkey()
>   Rename a jump label in five functions
>   Return a value directly in caam_hash_cra_init()
>   Delete an unnecessary initialisation in seven functions
>   Move common error handling code in two functions
> 
>  drivers/crypto/caam/caamhash.c | 111 
> +++--
>  1 file changed, 52 insertions(+), 59 deletions(-)
> 
Thanks Markus!

For patches 2-6:
Reviewed-by: Horia Geantă 

though headline prefix should be changed to "crypto: caam -"

Horia

[PATCH] scripts/kernel-doc: handle object-like macros

[Horia Geanta]

Object-like macros are different than function-like macros:
https://gcc.gnu.org/onlinedocs/cpp/Object-like-Macros.html
https://gcc.gnu.org/onlinedocs/cpp/Function-like-Macros.html

They are not parsed correctly, generating invalid intermediate
files (xmls) for cases like:
#define BIT_MASK(0xFF << BIT_SHIFT)
where "OxFF <<" is considered to be parameter type.

When parsing, we can differentiate beween these two types of macros by
checking whether there is at least one whitespace b/w "#define" and
first opening paranthesis.

Signed-off-by: Horia Geanta 
---
 scripts/kernel-doc | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index da058da413e7..ffd35c9d616a 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -2073,6 +2073,7 @@ sub check_return_section {
 sub dump_function($$) {
 my $prototype = shift;
 my $file = shift;
+my $noret = 0;
 
 $prototype =~ s/^static +//;
 $prototype =~ s/^extern +//;
@@ -2086,7 +2087,7 @@ sub dump_function($$) {
 $prototype =~ s/__init_or_module +//;
 $prototype =~ s/__must_check +//;
 $prototype =~ s/__weak +//;
-$prototype =~ s/^#\s*define\s+//; #ak added
+my $define = $prototype =~ s/^#\s*define\s+//; #ak added
 $prototype =~ s/__attribute__\s*\(\([a-z,]*\)\)//;
 
 # Yes, this truly is vile.  We are looking for:
@@ -2105,7 +2106,15 @@ sub dump_function($$) {
 # - atomic_set (macro)
 # - pci_match_device, __copy_to_user (long return type)
 
-if ($prototype =~ m/^()([a-zA-Z0-9_~:]+)\s*\(([^\(]*)\)/ ||
+if ($define && $prototype =~ m/^()([a-zA-Z0-9_~:]+)\s+/) {
+# This is an object-like macro, it has no return type and no parameter
+# list.
+# Function-like macros are not allowed to have spaces between
+# declaration_name and opening paranthesis (notice the \s+).
+$return_type = $1;
+$declaration_name = $2;
+$noret = 1;
+} elsif ($prototype =~ m/^()([a-zA-Z0-9_~:]+)\s*\(([^\(]*)\)/ ||
$prototype =~ m/^(\w+)\s+([a-zA-Z0-9_~:]+)\s*\(([^\(]*)\)/ ||
$prototype =~ m/^(\w+\s*\*)\s*([a-zA-Z0-9_~:]+)\s*\(([^\(]*)\)/ ||
$prototype =~ m/^(\w+\s+\w+)\s+([a-zA-Z0-9_~:]+)\s*\(([^\(]*)\)/ ||
@@ -2140,7 +2149,7 @@ sub dump_function($$) {
 # of warnings goes sufficiently down, the check is only performed in
 # verbose mode.
 # TODO: always perform the check.
-if ($verbose) {
+if ($verbose && !$noret) {
 check_return_section($file, $declaration_name, $return_type);
 }
 
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] scripts/kernel-doc: handle object-like macros

[Horia Geanta]

Object-like macros are different than function-like macros:
https://gcc.gnu.org/onlinedocs/cpp/Object-like-Macros.html
https://gcc.gnu.org/onlinedocs/cpp/Function-like-Macros.html

They are not parsed correctly, generating invalid intermediate
files (xmls) for cases like:
#define BIT_MASK(0xFF  BIT_SHIFT)
where OxFF  is considered to be parameter type.

When parsing, we can differentiate beween these two types of macros by
checking whether there is at least one whitespace b/w #define and
first opening paranthesis.

Signed-off-by: Horia Geanta horia.gea...@freescale.com
---
 scripts/kernel-doc | 15 ---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index da058da413e7..ffd35c9d616a 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -2073,6 +2073,7 @@ sub check_return_section {
 sub dump_function($$) {
 my $prototype = shift;
 my $file = shift;
+my $noret = 0;
 
 $prototype =~ s/^static +//;
 $prototype =~ s/^extern +//;
@@ -2086,7 +2087,7 @@ sub dump_function($$) {
 $prototype =~ s/__init_or_module +//;
 $prototype =~ s/__must_check +//;
 $prototype =~ s/__weak +//;
-$prototype =~ s/^#\s*define\s+//; #ak added
+my $define = $prototype =~ s/^#\s*define\s+//; #ak added
 $prototype =~ s/__attribute__\s*\(\([a-z,]*\)\)//;
 
 # Yes, this truly is vile.  We are looking for:
@@ -2105,7 +2106,15 @@ sub dump_function($$) {
 # - atomic_set (macro)
 # - pci_match_device, __copy_to_user (long return type)
 
-if ($prototype =~ m/^()([a-zA-Z0-9_~:]+)\s*\(([^\(]*)\)/ ||
+if ($define  $prototype =~ m/^()([a-zA-Z0-9_~:]+)\s+/) {
+# This is an object-like macro, it has no return type and no parameter
+# list.
+# Function-like macros are not allowed to have spaces between
+# declaration_name and opening paranthesis (notice the \s+).
+$return_type = $1;
+$declaration_name = $2;
+$noret = 1;
+} elsif ($prototype =~ m/^()([a-zA-Z0-9_~:]+)\s*\(([^\(]*)\)/ ||
$prototype =~ m/^(\w+)\s+([a-zA-Z0-9_~:]+)\s*\(([^\(]*)\)/ ||
$prototype =~ m/^(\w+\s*\*)\s*([a-zA-Z0-9_~:]+)\s*\(([^\(]*)\)/ ||
$prototype =~ m/^(\w+\s+\w+)\s+([a-zA-Z0-9_~:]+)\s*\(([^\(]*)\)/ ||
@@ -2140,7 +2149,7 @@ sub dump_function($$) {
 # of warnings goes sufficiently down, the check is only performed in
 # verbose mode.
 # TODO: always perform the check.
-if ($verbose) {
+if ($verbose  !$noret) {
 check_return_section($file, $declaration_name, $return_type);
 }
 
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC ipsec-next] xfrm: make sha256 icv truncation length RFC-compliant

[Horia Geanta]

From: Lei Xu 

Currently the sha256 icv truncation length is set to 96bit
while the length is defined as 128bit in RFC4868.
This may result in somer errors when working with other IPsec devices
with the standard truncation length.
Thus, change the sha256 truncation length from 96bit to 128bit.

Signed-off-by: Lei Xu 
Signed-off-by: Horia Geanta 
---
 net/xfrm/xfrm_algo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_algo.c b/net/xfrm/xfrm_algo.c
index debe733386f8..ca21ba7a0716 100644
--- a/net/xfrm/xfrm_algo.c
+++ b/net/xfrm/xfrm_algo.c
@@ -220,7 +220,7 @@ static struct xfrm_algo_desc aalg_list[] = {
 
.uinfo = {
.auth = {
-   .icv_truncbits = 96,
+   .icv_truncbits = 128,
.icv_fullbits = 256,
}
},
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC ipsec-next] Non-standard sha256 ICV truncation size

[Horia Geanta]

Hi,

I am sending this patch as RFC, since the change is likely to create
interoperability issues.
I'd like to get some comments on what should be the correct approach in this
standard vs. de facto setup.

Note that an older commit (which is no longer in git's history, but can be found
here https://archive.org/details/git-history-of-linux) changed truncated ICV 
size
from 128bit to 96bit:

commit 627d1054055f465603bcfb7ef5c9adc5db9b2a53
Author: Michal Ludvig 
Date:   Fri Jan 9 04:55:59 2004 -0800

[XFRM]: SHA2-256 should be truncated to 96 bits, not 128.

It seems that initially the sha256 ICV was truncated to 128 bits, but later
changed to 96 bits due to interoperability issues - see comment here:
http://oss.sgi.com/archives/netdev/2004-01/msg00824.html

Note that commit above is from 2004, long before RFC4868 was released (2007).

Thanks,
Horia

Lei Xu (1):
  xfrm: make sha256 icv truncation length RFC-compliant

 net/xfrm/xfrm_algo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC ipsec-next] xfrm: make sha256 icv truncation length RFC-compliant

[Horia Geanta]

From: Lei Xu lei...@freescale.com

Currently the sha256 icv truncation length is set to 96bit
while the length is defined as 128bit in RFC4868.
This may result in somer errors when working with other IPsec devices
with the standard truncation length.
Thus, change the sha256 truncation length from 96bit to 128bit.

Signed-off-by: Lei Xu lei...@freescale.com
Signed-off-by: Horia Geanta horia.gea...@freescale.com
---
 net/xfrm/xfrm_algo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_algo.c b/net/xfrm/xfrm_algo.c
index debe733386f8..ca21ba7a0716 100644
--- a/net/xfrm/xfrm_algo.c
+++ b/net/xfrm/xfrm_algo.c
@@ -220,7 +220,7 @@ static struct xfrm_algo_desc aalg_list[] = {
 
.uinfo = {
.auth = {
-   .icv_truncbits = 96,
+   .icv_truncbits = 128,
.icv_fullbits = 256,
}
},
-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC ipsec-next] Non-standard sha256 ICV truncation size

[Horia Geanta]

Hi,

I am sending this patch as RFC, since the change is likely to create
interoperability issues.
I'd like to get some comments on what should be the correct approach in this
standard vs. de facto setup.

Note that an older commit (which is no longer in git's history, but can be found
here https://archive.org/details/git-history-of-linux) changed truncated ICV 
size
from 128bit to 96bit:

commit 627d1054055f465603bcfb7ef5c9adc5db9b2a53
Author: Michal Ludvig mic...@logix.cz
Date:   Fri Jan 9 04:55:59 2004 -0800

[XFRM]: SHA2-256 should be truncated to 96 bits, not 128.

It seems that initially the sha256 ICV was truncated to 128 bits, but later
changed to 96 bits due to interoperability issues - see comment here:
http://oss.sgi.com/archives/netdev/2004-01/msg00824.html

Note that commit above is from 2004, long before RFC4868 was released (2007).

Thanks,
Horia

Lei Xu (1):
  xfrm: make sha256 icv truncation length RFC-compliant

 net/xfrm/xfrm_algo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
1.8.3.1

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/