KPP: DH/ECDH -- allocation returns ENOENT

[Stephan Müller]

Hi Salvatore,

when I test 4.11-rc1 with the DH and ECDH options enabled, I get -ENOENT when 
trying to allocate the ciphers dh/dh_generic/ecdh.

Note, the dh/ecdh kernel modules are loaded.

Ciao
Stephan

Re: [PATCH v2 1/3] crypto: hw_random - Add new Exynos RNG driver

[Stephan Müller]

Am Freitag, 24. März 2017, 19:26:04 CET schrieb Krzysztof Kozlowski:

Hi Krzysztof,

> +static unsigned int exynos_rng_copy_random(struct exynos_rng_dev *rng,
> +u8 *dst, unsigned int dlen)
> +{
> + unsigned int cnt = 0;
> + int i, j;
> + u32 val;
> +
> + for (j = 0; j < EXYNOS_RNG_SEED_REGS; j++) {
> + val = exynos_rng_readl(rng, EXYNOS_RNG_OUT(j));
> +
> + for (i = 0; i < 4; i++) {
> + dst[cnt] = val & 0xff;
> + val >>= 8;
> + if (++cnt >= dlen)
> + return cnt;
> + }
> + rng->seed_save[j] = val;

Just to clarify: is this call right? Shouldn't that be removed? Any RNG that 
is given to a caller is tainted and should not serve as seed.
> + }
> +
> + /*
> +  * Engine filled all output registers, so read the remaining registers
> +  * for storing data as future seed.
> +  */
> + for (; j < EXYNOS_RNG_SEED_REGS; j++)
> + rng->seed_save[j] = exynos_rng_readl(rng, EXYNOS_RNG_OUT(j));

With this call, I guess the questioned line above could go away, right?


Ciao
Stephan

[PATCH v2 3/3] ARM: multi_v7_defconfig: Enable Exynos RNG and user-space crypto API

[Krzysztof Kozlowski]

Enable the new Exynos pseudo random number generator driver and
user-space API to access crypto algorithms and devices (not only RNG).

Signed-off-by: Krzysztof Kozlowski 
---
 arch/arm/configs/multi_v7_defconfig | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/configs/multi_v7_defconfig 
b/arch/arm/configs/multi_v7_defconfig
index 36c1b39cfb54..9bc1bd7f2afa 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -935,7 +935,13 @@ CONFIG_CPUFREQ_DT=y
 CONFIG_KEYSTONE_IRQ=y
 CONFIG_HW_RANDOM=y
 CONFIG_HW_RANDOM_ST=y
+CONFIG_CRYPTO_USER=m
+CONFIG_CRYPTO_USER_API_HASH=m
+CONFIG_CRYPTO_USER_API_SKCIPHER=m
+CONFIG_CRYPTO_USER_API_RNG=m
+CONFIG_CRYPTO_USER_API_AEAD=m
 CONFIG_CRYPTO_DEV_MARVELL_CESA=m
+CONFIG_CRYPTO_DEV_EXYNOS_RNG=m
 CONFIG_CRYPTO_DEV_S5P=m
 CONFIG_CRYPTO_DEV_SUN4I_SS=m
 CONFIG_CRYPTO_DEV_ROCKCHIP=m
-- 
2.9.3

[PATCH v2 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

Replace existing hw_ranndom/exynos-rng driver with a new, reworked one.
This is a driver for pseudo random number generator block which on
Exynos4 chipsets must be seeded with some value.  On newer Exynos5420
chipsets it might seed itself from true random number generator block
but this is not implemented yet.

New driver is a complete rework to use the crypto ALGAPI instead of
hw_random API.  Rationale for the change:
1. hw_random interface is for true RNG devices.
2. The old driver was seeding itself with jiffies which is not a
   reliable source for randomness.
3. Device generates five random numbers in each pass but old driver was
   returning only one thus its performance was reduced.

Compatibility with DeviceTree bindings is preserved.

New driver does not use runtime power management but manually enables
and disables the clock when needed.  This is preferred approach because
using runtime PM just to toggle clock is huge overhead.

Another difference is reseeding itself with generated random data
periodically and during resuming from system suspend (previously driver
was re-seeding itself again with jiffies).

Signed-off-by: Krzysztof Kozlowski 

---

The resume path is untested as my Odroid U3 fails to go online. Testing
on Trats2 or other devices would be appreciated.
---
 MAINTAINERS |   8 +
 drivers/char/hw_random/Kconfig  |  14 --
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/exynos-rng.c | 231 -
 drivers/crypto/Kconfig  |  15 ++
 drivers/crypto/Makefile |   1 +
 drivers/crypto/exynos-rng.c | 392 
 7 files changed, 416 insertions(+), 246 deletions(-)
 delete mode 100644 drivers/char/hw_random/exynos-rng.c
 create mode 100644 drivers/crypto/exynos-rng.c

diff --git a/MAINTAINERS b/MAINTAINERS
index affecc6d59f4..371fda859d43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10977,6 +10977,14 @@ L: alsa-de...@alsa-project.org (moderated for 
non-subscribers)
 S: Supported
 F: sound/soc/samsung/
 
+SAMSUNG EXYNOS PSEUDO RANDOM NUMBER GENERATOR (RNG) DRIVER
+M: Krzysztof Kozlowski 
+L: linux-crypto@vger.kernel.org
+L: linux-samsung-...@vger.kernel.org
+S: Maintained
+F: drivers/crypto/exynos-rng.c
+F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
+
 SAMSUNG FRAMEBUFFER DRIVER
 M: Jingoo Han 
 L: linux-fb...@vger.kernel.org
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 0cafe08919c9..bdae802e7154 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -294,20 +294,6 @@ config HW_RANDOM_POWERNV
 
  If unsure, say Y.
 
-config HW_RANDOM_EXYNOS
-   tristate "EXYNOS HW random number generator support"
-   depends on ARCH_EXYNOS || COMPILE_TEST
-   depends on HAS_IOMEM
-   default HW_RANDOM
-   ---help---
- This driver provides kernel-side support for the Random Number
- Generator hardware found on EXYNOS SOCs.
-
- To compile this driver as a module, choose M here: the
- module will be called exynos-rng.
-
- If unsure, say Y.
-
 config HW_RANDOM_TPM
tristate "TPM HW Random Number Generator support"
depends on TCG_TPM
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index 5f52b1e4e7be..6f1eecc2045c 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -24,7 +24,6 @@ obj-$(CONFIG_HW_RANDOM_OCTEON) += octeon-rng.o
 obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
 obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
 obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
-obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o
 obj-$(CONFIG_HW_RANDOM_HISI)   += hisi-rng.o
 obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
 obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
diff --git a/drivers/char/hw_random/exynos-rng.c 
b/drivers/char/hw_random/exynos-rng.c
deleted file mode 100644
index 23d358553b21..
--- a/drivers/char/hw_random/exynos-rng.c
+++ /dev/null
@@ -1,231 +0,0 @@
-/*
- * exynos-rng.c - Random Number Generator driver for the exynos
- *
- * Copyright (C) 2012 Samsung Electronics
- * Jonghwa Lee 
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation;
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  

[PATCH v2 2/3] ARM: exynos_defconfig: Enable Exynos RNG and user-space crypto API

[Krzysztof Kozlowski]

Enable the new Exynos pseudo random number generator driver and
user-space API to access crypto algorithms and devices (not only RNG).

Signed-off-by: Krzysztof Kozlowski 
---
 arch/arm/configs/exynos_defconfig | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/configs/exynos_defconfig 
b/arch/arm/configs/exynos_defconfig
index 6dc661c4a2c1..960d55445e05 100644
--- a/arch/arm/configs/exynos_defconfig
+++ b/arch/arm/configs/exynos_defconfig
@@ -265,6 +265,12 @@ CONFIG_DEBUG_RT_MUTEXES=y
 CONFIG_DEBUG_SPINLOCK=y
 CONFIG_DEBUG_MUTEXES=y
 CONFIG_DEBUG_USER=y
+CONFIG_CRYPTO_USER=m
+CONFIG_CRYPTO_USER_API_HASH=m
+CONFIG_CRYPTO_USER_API_SKCIPHER=m
+CONFIG_CRYPTO_USER_API_RNG=m
+CONFIG_CRYPTO_USER_API_AEAD=m
+CONFIG_CRYPTO_DEV_EXYNOS_RNG=y
 CONFIG_CRYPTO_DEV_S5P=y
 CONFIG_ARM_CRYPTO=y
 CONFIG_CRYPTO_SHA1_ARM_NEON=m
-- 
2.9.3

[PATCH v2 0/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

Hi,


This is a follow up of my questions around exynos-rng [1].

Changes since v1:
=
1. Re-work the code for seeding after system resume, following suggestions
   and review by Stephan Müller.
   The resume path was not tested.

2. Re-seed itself from time to time (every 100 ms), suggested by Stephan
   Müller.

3. Use a define for retries (Bartlomiej Zolnierkiewicz).
4. Add some docs.


Description:

The existing exynos-rng has many issues.  The most important one is that
it is a pseudo RNG device but uses hw_random interface which does not allow
proper seeding.

The RNG module on Exynos4 requires seeding.  On newer SoCs (like Exynos5420)
it can seed itself from a true RNG.  Converting the existing driver
to use TRNG would effectively drop support for Exynos4 and break
compatibility with existing users.

Instead I decided to convert it to crypto API.  In the future I hope
to add support for seeding from TRNG module.

Tested with app [2].

Patches are independent. I will take the defconfig changes (2/3 and 3/3)
through samsung-soc tree.

Best regards,
Krzysztof

[1] https://www.spinics.net/lists/arm-kernel/msg569641.html
[2] https://www.spinics.net/lists/arm-kernel/msg571184.html


Krzysztof Kozlowski (3):
  crypto: hw_random - Add new Exynos RNG driver
  ARM: exynos_defconfig: Enable Exynos RNG and user-space crypto API
  ARM: multi_v7_defconfig: Enable Exynos RNG and user-space crypto API

 MAINTAINERS |   8 +
 arch/arm/configs/exynos_defconfig   |   6 +
 arch/arm/configs/multi_v7_defconfig |   6 +
 drivers/char/hw_random/Kconfig  |  14 --
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/exynos-rng.c | 231 -
 drivers/crypto/Kconfig  |  15 ++
 drivers/crypto/Makefile |   1 +
 drivers/crypto/exynos-rng.c | 392 
 9 files changed, 428 insertions(+), 246 deletions(-)
 delete mode 100644 drivers/char/hw_random/exynos-rng.c
 create mode 100644 drivers/crypto/exynos-rng.c

-- 
2.9.3

[cryptodev:master 42/60] drivers/crypto/ccp/ccp-dev-v5.c:436:28: note: in expansion of macro 'cpu_to_le32'

[kbuild test robot]

tree:   
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
head:   796b40c6171456274b02447e1dbbea97456403fe
commit: 990672d48515ce09c76fcf1ceccee48b0dd1942b [42/60] crypto: ccp - Enable 
3DES function on v5 CCPs
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 990672d48515ce09c76fcf1ceccee48b0dd1942b
# save the attached .config to linux build tree
make.cross ARCH=arm64 

All warnings (new ones prefixed by >>):

   In file included from include/linux/byteorder/big_endian.h:4:0,
from arch/arm64/include/uapi/asm/byteorder.h:20,
from include/asm-generic/bitops/le.h:5,
from arch/arm64/include/asm/bitops.h:50,
from include/linux/bitops.h:36,
from include/linux/kernel.h:10,
from include/linux/list.h:8,
from include/linux/module.h:9,
from drivers/crypto/ccp/ccp-dev-v5.c:13:
   drivers/crypto/ccp/ccp-dev-v5.c: In function 'ccp5_perform_des3':
   include/uapi/linux/byteorder/big_endian.h:32:26: warning: large integer 
implicitly truncated to unsigned type [-Woverflow]
#define __cpu_to_le32(x) ((__force __le32)__swab32((x)))
 ^
   include/linux/byteorder/generic.h:87:21: note: in expansion of macro 
'__cpu_to_le32'
#define cpu_to_le32 __cpu_to_le32
^
>> drivers/crypto/ccp/ccp-dev-v5.c:436:28: note: in expansion of macro 
>> 'cpu_to_le32'
 CCP5_CMD_KEY_MEM() = cpu_to_le32(CCP_MEMTYPE_SB);
   ^~~

vim +/cpu_to_le32 +436 drivers/crypto/ccp/ccp-dev-v5.c

   420  CCP_DES3_MODE() = op->u.des3.mode;
   421  CCP_DES3_TYPE() = op->u.des3.type;
   422  CCP5_CMD_FUNCTION() = cpu_to_le32(function.raw);
   423  
   424  CCP5_CMD_LEN() = cpu_to_le32(op->src.u.dma.length);
   425  
   426  CCP5_CMD_SRC_LO() = 
cpu_to_le32(ccp_addr_lo(>src.u.dma));
   427  CCP5_CMD_SRC_HI() = 
cpu_to_le32(ccp_addr_hi(>src.u.dma));
   428  CCP5_CMD_SRC_MEM() = cpu_to_le32(CCP_MEMTYPE_SYSTEM);
   429  
   430  CCP5_CMD_DST_LO() = 
cpu_to_le32(ccp_addr_lo(>dst.u.dma));
   431  CCP5_CMD_DST_HI() = 
cpu_to_le32(ccp_addr_hi(>dst.u.dma));
   432  CCP5_CMD_DST_MEM() = cpu_to_le32(CCP_MEMTYPE_SYSTEM);
   433  
   434  CCP5_CMD_KEY_LO() = cpu_to_le32(lower_32_bits(key_addr));
   435  CCP5_CMD_KEY_HI() = 0;
 > 436  CCP5_CMD_KEY_MEM() = cpu_to_le32(CCP_MEMTYPE_SB);
   437  CCP5_CMD_LSB_ID() = cpu_to_le32(op->sb_ctx);
   438  
   439  return ccp5_do_cmd(, op->cmd_q);
   440  }
   441  
   442  static int ccp5_perform_rsa(struct ccp_op *op)
   443  {
   444  struct ccp5_desc desc;

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [RFC PATCH v2 15/32] x86: Add support for changing memory encryption attribute in early boot

[Borislav Petkov]

On Thu, Mar 02, 2017 at 10:15:28AM -0500, Brijesh Singh wrote:
> Some KVM-specific custom MSRs shares the guest physical address with
> hypervisor. When SEV is active, the shared physical address must be mapped
> with encryption attribute cleared so that both hypervsior and guest can
> access the data.
> 
> Add APIs to change memory encryption attribute in early boot code.
> 
> Signed-off-by: Brijesh Singh 
> ---
>  arch/x86/include/asm/mem_encrypt.h |   15 +
>  arch/x86/mm/mem_encrypt.c  |   63 
> 
>  2 files changed, 78 insertions(+)
> 
> diff --git a/arch/x86/include/asm/mem_encrypt.h 
> b/arch/x86/include/asm/mem_encrypt.h
> index 9799835..95bbe4c 100644
> --- a/arch/x86/include/asm/mem_encrypt.h
> +++ b/arch/x86/include/asm/mem_encrypt.h
> @@ -47,6 +47,9 @@ void __init sme_unmap_bootdata(char *real_mode_data);
>  
>  void __init sme_early_init(void);
>  
> +int __init early_set_memory_decrypted(void *addr, unsigned long size);
> +int __init early_set_memory_encrypted(void *addr, unsigned long size);
> +
>  /* Architecture __weak replacement functions */
>  void __init mem_encrypt_init(void);
>  
> @@ -110,6 +113,18 @@ static inline void __init sme_early_init(void)
>  {
>  }
>  
> +static inline int __init early_set_memory_decrypted(void *addr,
> + unsigned long size)
> +{
> + return 1;


return 1 when !CONFIG_AMD_MEM_ENCRYPT ?

The non-early variants return 0.

> +}
> +
> +static inline int __init early_set_memory_encrypted(void *addr,
> + unsigned long size)
> +{
> + return 1;
> +}
> +
>  #define __sme_pa __pa
>  #define __sme_pa_nodebug __pa_nodebug
>  
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 7df5f4c..567e0d8 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -15,6 +15,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -258,6 +259,68 @@ static void sme_free(struct device *dev, size_t size, 
> void *vaddr,
>   swiotlb_free_coherent(dev, size, vaddr, dma_handle);
>  }
>  
> +static unsigned long __init get_pte_flags(unsigned long address)
> +{
> + int level;
> + pte_t *pte;
> + unsigned long flags = _KERNPG_TABLE_NOENC | _PAGE_ENC;
> +
> + pte = lookup_address(address, );
> + if (!pte)
> + return flags;
> +
> + switch (level) {
> + case PG_LEVEL_4K:
> + flags = pte_flags(*pte);
> + break;
> + case PG_LEVEL_2M:
> + flags = pmd_flags(*(pmd_t *)pte);
> + break;
> + case PG_LEVEL_1G:
> + flags = pud_flags(*(pud_t *)pte);
> + break;
> + default:
> + break;
> + }
> +
> + return flags;
> +}
> +
> +int __init early_set_memory_enc_dec(void *vaddr, unsigned long size,
> + unsigned long flags)
> +{
> + unsigned long pfn, npages;
> + unsigned long addr = (unsigned long)vaddr & PAGE_MASK;
> +
> + /* We are going to change the physical page attribute from C=1 to C=0.
> +  * Flush the caches to ensure that all the data with C=1 is flushed to
> +  * memory. Any caching of the vaddr after function returns will
> +  * use C=0.
> +  */

Kernel comments style is:

/*
 * A sentence ending with a full-stop.
 * Another sentence. ...
 * More sentences. ...
 */

> + clflush_cache_range(vaddr, size);
> +
> + npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> + pfn = slow_virt_to_phys((void *)addr) >> PAGE_SHIFT;
> +
> + return kernel_map_pages_in_pgd(init_mm.pgd, pfn, addr, npages,
> + flags & ~sme_me_mask);
> +
> +}
> +
> +int __init early_set_memory_decrypted(void *vaddr, unsigned long size)
> +{
> + unsigned long flags = get_pte_flags((unsigned long)vaddr);

So this does lookup_address()...

> + return early_set_memory_enc_dec(vaddr, size, flags & ~sme_me_mask);

... and this does it too in slow_virt_to_phys(). So you do it twice per
vaddr.

So why don't you define a __slow_virt_to_phys() helper - notice
the "__" - which returns flags in its second parameter and which
slow_virt_to_phys() calls with a NULL second parameter in the other
cases?

-- 
Regards/Gruss,
Boris.

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

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Fri, Mar 24, 2017 at 05:45:41PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > > > And I think the probe might be called twice, for example in case of
> > > > mistake in DTB.
> > > 
> > > Even if this is possible resource allocation code in the driver will
> > > take take care of handling it just fine,
> > 
> > Indeed, the devm_ioremap_resource() solves the case. I can drop the
> > check then.
> 
> Looking on this a bit more it seems that devm_ioremap_resource() will
> not cover all mistakes (using compatible by mistake in some other DTB
> node).
> 
> Leave the check, I take my objection back.

Great! Thanks for feedback.

Best regards,
Krzysztof

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Bartlomiej Zolnierkiewicz]

On Friday, March 24, 2017 07:19:34 PM Krzysztof Kozlowski wrote:
> On Fri, Mar 24, 2017 at 05:11:25PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > On Friday, March 24, 2017 06:46:00 PM Krzysztof Kozlowski wrote:
> > > I really do not like global or file-scope variables. I do not like
> > > drivers using them. Actually I hate them.
> > > 
> > > From time to time I encounter a driver which was designed with that
> > > approach - static fields and hidden assumption that there will be only
> > > one instance. Usually that assumption is really hidden...
> > > 
> > > ... and then it happens that I want to use two instances which of course
> > > fails.
> > > 
> > > This code serves as a clear documentation for this assumption - only one
> > > instance is allowed. You can look at it as a self-documenting
> > > requirement.
> > 
> > For me it looks as needless case of defensive programming and when
> > I see the code like this it always raises questions about the real
> > intentions of the code. I find it puzzling and not helpful.
> 
> I do not understand what might be puzzling about check for static
> file-scope value. It is of course subjective, but for me that looks
> pretty self-explanatory.

The check should never happen given that ->probe will not happen twice.

However it seems that this is possible now with DT platform devices and
incorrect DTB.

> > > And I think the probe might be called twice, for example in case of
> > > mistake in DTB.
> > 
> > Even if this is possible resource allocation code in the driver will
> > take take care of handling it just fine,
> 
> Indeed, the devm_ioremap_resource() solves the case. I can drop the
> check then.

Looking on this a bit more it seems that devm_ioremap_resource() will
not cover all mistakes (using compatible by mistake in some other DTB
node).

Leave the check, I take my objection back.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R Institute Poland
Samsung Electronics

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Fri, Mar 24, 2017 at 05:11:25PM +0100, Bartlomiej Zolnierkiewicz wrote:
> On Friday, March 24, 2017 06:46:00 PM Krzysztof Kozlowski wrote:
> > I really do not like global or file-scope variables. I do not like
> > drivers using them. Actually I hate them.
> > 
> > From time to time I encounter a driver which was designed with that
> > approach - static fields and hidden assumption that there will be only
> > one instance. Usually that assumption is really hidden...
> > 
> > ... and then it happens that I want to use two instances which of course
> > fails.
> > 
> > This code serves as a clear documentation for this assumption - only one
> > instance is allowed. You can look at it as a self-documenting
> > requirement.
> 
> For me it looks as needless case of defensive programming and when
> I see the code like this it always raises questions about the real
> intentions of the code. I find it puzzling and not helpful.

I do not understand what might be puzzling about check for static
file-scope value. It is of course subjective, but for me that looks
pretty self-explanatory.

> 
> > And I think the probe might be called twice, for example in case of
> > mistake in DTB.
> 
> Even if this is possible resource allocation code in the driver will
> take take care of handling it just fine,

Indeed, the devm_ioremap_resource() solves the case. I can drop the
check then.

Best regards,
Krzysztof

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Bartlomiej Zolnierkiewicz]

On Friday, March 24, 2017 06:46:00 PM Krzysztof Kozlowski wrote:
> On Fri, Mar 24, 2017 at 04:26:47PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > 
> > Hi,
> > 
> > Firstly, thanks for working on this.
> > 
> > The patch looks fine overall for me, some review comments below.
> > 
> > On Friday, March 24, 2017 05:24:44 PM Krzysztof Kozlowski wrote:
> > > Replace existing hw_ranndom/exynos-rng driver with a new, reworked one.
> > > This is a driver for pseudo random number generator block which on
> > > Exynos4 chipsets must be seeded with some value.  On newer Exynos5420
> > > chipsets it might seed itself from true random number generator block
> > > but this is not implemented yet.
> > > 
> > > New driver is a complete rework to use the crypto ALGAPI instead of
> > > hw_random API.  Rationale for the change:
> > > 1. hw_random interface is for true RNG devices.
> > > 2. The old driver was seeding itself with jiffies which is not a
> > >reliable source for randomness.
> > > 3. Device generates five random numbers in each pass but old driver was
> > >returning only one thus its performance was reduced.
> > > 
> > > Compatibility with DeviceTree bindings is preserved.
> > > 
> > > New driver does not use runtime power management but manually enables
> > > and disables the clock when needed.  This is preferred approach because
> > > using runtime PM just to toggle clock is huge overhead.  Another
> > 
> > I'm not entirely convinced that the new approach is better.
> > 
> > With the old approach exynos_rng_generate() can be called more
> > than once before PM autosuspend kicks in and thus clk_prepare_enable()/
> > clk_disable()_unprepare() operations will be done only once. This
> > would give better performance on the "burst" operations.
> > 
> > [ The above assumes that clock operations are more costly than
> >   going through PM core to check the current device state. ]
> 
> I agree that we loose the "burst" mode but:
> 1. At least on Exynso4 SSS is part of TOP power domain so it will not
>help to reduce any more power consumption (on Exynos5422 it is
>mentioned in G2D... which seems incorrect).
> 2. I think the overhead of clk operations is much smaller. These are only
>two locks (prepare mutex + enable spin), simple tree traversal and
>play with few SFRs.
> 
>The power domain code in comparison to that is huge and complicated
>with inter-device links and dependencies. Also the actual runtime PM
>suspend would anyway fall back at then end to clk prepare/enable
>locks and paths.
> 
>We've been talking about this a lot with Marek Szyprowski (cc'ed) and
>he was always (AFAIR) against attempts of runtime power
>management of a single clock...

OK, thanks for explanation.

> > > +static int exynos_rng_probe(struct platform_device *pdev)
> > > +{
> > > + struct exynos_rng_dev *rng;
> > > + struct resource *res;
> > > + int ret;
> > > +
> > > + if (exynos_rng_dev)
> > > + return -EEXIST;
> > 
> > How this condition could ever happen? 
> > 
> > The probe function will never be called twice.
> 
> I really do not like global or file-scope variables. I do not like
> drivers using them. Actually I hate them.
> 
> From time to time I encounter a driver which was designed with that
> approach - static fields and hidden assumption that there will be only
> one instance. Usually that assumption is really hidden...
> 
> ... and then it happens that I want to use two instances which of course
> fails.
> 
> This code serves as a clear documentation for this assumption - only one
> instance is allowed. You can look at it as a self-documenting
> requirement.

For me it looks as needless case of defensive programming and when
I see the code like this it always raises questions about the real
intentions of the code. I find it puzzling and not helpful.

> And I think the probe might be called twice, for example in case of
> mistake in DTB.

Even if this is possible resource allocation code in the driver will
take take care of handling it just fine,

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R Institute Poland
Samsung Electronics

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Fri, Mar 24, 2017 at 04:26:47PM +0100, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> Firstly, thanks for working on this.
> 
> The patch looks fine overall for me, some review comments below.
> 
> On Friday, March 24, 2017 05:24:44 PM Krzysztof Kozlowski wrote:
> > Replace existing hw_ranndom/exynos-rng driver with a new, reworked one.
> > This is a driver for pseudo random number generator block which on
> > Exynos4 chipsets must be seeded with some value.  On newer Exynos5420
> > chipsets it might seed itself from true random number generator block
> > but this is not implemented yet.
> > 
> > New driver is a complete rework to use the crypto ALGAPI instead of
> > hw_random API.  Rationale for the change:
> > 1. hw_random interface is for true RNG devices.
> > 2. The old driver was seeding itself with jiffies which is not a
> >reliable source for randomness.
> > 3. Device generates five random numbers in each pass but old driver was
> >returning only one thus its performance was reduced.
> > 
> > Compatibility with DeviceTree bindings is preserved.
> > 
> > New driver does not use runtime power management but manually enables
> > and disables the clock when needed.  This is preferred approach because
> > using runtime PM just to toggle clock is huge overhead.  Another
> 
> I'm not entirely convinced that the new approach is better.
> 
> With the old approach exynos_rng_generate() can be called more
> than once before PM autosuspend kicks in and thus clk_prepare_enable()/
> clk_disable()_unprepare() operations will be done only once. This
> would give better performance on the "burst" operations.
> 
> [ The above assumes that clock operations are more costly than
>   going through PM core to check the current device state. ]

I agree that we loose the "burst" mode but:
1. At least on Exynso4 SSS is part of TOP power domain so it will not
   help to reduce any more power consumption (on Exynos5422 it is
   mentioned in G2D... which seems incorrect).
2. I think the overhead of clk operations is much smaller. These are only
   two locks (prepare mutex + enable spin), simple tree traversal and
   play with few SFRs.

   The power domain code in comparison to that is huge and complicated
   with inter-device links and dependencies. Also the actual runtime PM
   suspend would anyway fall back at then end to clk prepare/enable
   locks and paths.

   We've been talking about this a lot with Marek Szyprowski (cc'ed) and
   he was always (AFAIR) against attempts of runtime power
   management of a single clock...


> 
> > +static int exynos_rng_get_random(struct exynos_rng_dev *rng,
> > +u8 *dst, unsigned int dlen,
> > +unsigned int *read)
> > +{
> > +   int retry = 100;
> 
> I know that this is copied verbatim from the old driver but please
> use define for the maximum number of retries.

No problem. Number is chosen arbitrarily so there will not be any
additional information coming from the define.

> > +static int exynos_rng_probe(struct platform_device *pdev)
> > +{
> > +   struct exynos_rng_dev *rng;
> > +   struct resource *res;
> > +   int ret;
> > +
> > +   if (exynos_rng_dev)
> > +   return -EEXIST;
> 
> How this condition could ever happen? 
> 
> The probe function will never be called twice.

I really do not like global or file-scope variables. I do not like
drivers using them. Actually I hate them.

>From time to time I encounter a driver which was designed with that
approach - static fields and hidden assumption that there will be only
one instance. Usually that assumption is really hidden...

... and then it happens that I want to use two instances which of course
fails.

This code serves as a clear documentation for this assumption - only one
instance is allowed. You can look at it as a self-documenting
requirement.

And I think the probe might be called twice, for example in case of
mistake in DTB.

Best regards,
Krzysztof

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Bartlomiej Zolnierkiewicz]

Hi,

Firstly, thanks for working on this.

The patch looks fine overall for me, some review comments below.

On Friday, March 24, 2017 05:24:44 PM Krzysztof Kozlowski wrote:
> Replace existing hw_ranndom/exynos-rng driver with a new, reworked one.
> This is a driver for pseudo random number generator block which on
> Exynos4 chipsets must be seeded with some value.  On newer Exynos5420
> chipsets it might seed itself from true random number generator block
> but this is not implemented yet.
> 
> New driver is a complete rework to use the crypto ALGAPI instead of
> hw_random API.  Rationale for the change:
> 1. hw_random interface is for true RNG devices.
> 2. The old driver was seeding itself with jiffies which is not a
>reliable source for randomness.
> 3. Device generates five random numbers in each pass but old driver was
>returning only one thus its performance was reduced.
> 
> Compatibility with DeviceTree bindings is preserved.
> 
> New driver does not use runtime power management but manually enables
> and disables the clock when needed.  This is preferred approach because
> using runtime PM just to toggle clock is huge overhead.  Another

I'm not entirely convinced that the new approach is better.

With the old approach exynos_rng_generate() can be called more
than once before PM autosuspend kicks in and thus clk_prepare_enable()/
clk_disable()_unprepare() operations will be done only once. This
would give better performance on the "burst" operations.

[ The above assumes that clock operations are more costly than
  going through PM core to check the current device state. ]

> +static int exynos_rng_get_random(struct exynos_rng_dev *rng,
> +  u8 *dst, unsigned int dlen,
> +  unsigned int *read)
> +{
> + int retry = 100;

I know that this is copied verbatim from the old driver but please
use define for the maximum number of retries.

> +static int exynos_rng_probe(struct platform_device *pdev)
> +{
> + struct exynos_rng_dev *rng;
> + struct resource *res;
> + int ret;
> +
> + if (exynos_rng_dev)
> + return -EEXIST;

How this condition could ever happen? 

The probe function will never be called twice.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R Institute Poland
Samsung Electronics

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Fri, Mar 24, 2017 at 03:55:22PM +0100, Stephan Müller wrote:
> Am Freitag, 24. März 2017, 15:51:56 CET schrieb Krzysztof Kozlowski:
> 
> Hi Krzysztof,
> > 
> > I'll use the generated random numbers.
> 
> If you do that, may I propse that you update this seed field periodically? 
> E.g. with every (or every other) user request for random data, you may update 
> that field with new data.

Sounds reasonable. Thanks for feedback, it is much appreciated.

Best regards,
Krzysztof

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Stephan Müller]

Am Freitag, 24. März 2017, 15:51:56 CET schrieb Krzysztof Kozlowski:

Hi Krzysztof,
> 
> I'll use the generated random numbers.

If you do that, may I propse that you update this seed field periodically? 
E.g. with every (or every other) user request for random data, you may update 
that field with new data.

Ciao
Stephan

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Fri, Mar 24, 2017 at 03:46:44PM +0100, Stephan Müller wrote:
> Am Freitag, 24. März 2017, 15:43:48 CET schrieb Krzysztof Kozlowski:
> 
> Hi Krzysztof,
> 
> > On Fri, Mar 24, 2017 at 03:37:59PM +0100, Stephan Müller wrote:
> > > Am Freitag, 24. März 2017, 15:24:44 CET schrieb Krzysztof Kozlowski:
> > > 
> > > Hi Krzysztof,
> > > 
> > > > +
> > > > +static int exynos_rng_set_seed(struct exynos_rng_dev *rng,
> > > > +  const u8 *seed, unsigned int slen)
> > > > +{
> > > > +   int ret, i;
> > > > +   u32 val;
> > > > +
> > > > +   dev_dbg(rng->dev, "Seeding with %u bytes\n", slen);
> > > > +
> > > > +   ret = clk_prepare_enable(rng->clk);
> > > > +   if (ret)
> > > > +   return ret;
> > > > +
> > > > +   if (slen < EXYNOS_RNG_SEED_SIZE) {
> > > > +   dev_warn(rng->dev, "Seed too short (only %u bytes)\n", 
> > > > slen);
> > > > +   ret = -EINVAL;
> > > > +   goto out;
> > > > +   }
> > > > +
> > > > +   for (i = 0 ; i < EXYNOS_RNG_SEED_REGS ; i++) {
> > > > +   val = seed[i * 4] << 24;
> > > > +   val |= seed[i * 4 + 1] << 16;
> > > > +   val |= seed[i * 4 + 2] << 8;
> > > > +   val |= seed[i * 4 + 3] << 0;
> > > > +
> > > > +   exynos_rng_writel(rng, val, EXYNOS_RNG_SEED(i));
> > > > +   }
> > > > +
> > > > +   val = exynos_rng_readl(rng, EXYNOS_RNG_STATUS);
> > > > +   if (!(val & EXYNOS_RNG_STATUS_SEED_SETTING_DONE)) {
> > > > +   dev_warn(rng->dev, "Seed setting not finished\n");
> > > > +   ret = -EIO;
> > > > +   goto out;
> > > > +   }
> > > > +
> > > > +   ret = 0;
> > > > +   /* Save seed for suspend */
> > > > +   memcpy(rng->seed_save, seed, slen);
> > > 
> > > Is this really necessary? If you need to save some seed, shouldn't that be
> > > an output of the DRNG and not the real seed?
> > 
> > When suspended to RAM device will loose the contents of registers
> > (including SEED registers) so it has to be initialized with something on
> > resume.
> > 
> > The seed registers are write-only so I cannot read them and store
> > contents just before suspend.
> > 
> > I understand that real seed should not be stored... but then if I am not
> > able to re-seed it with same values, I will loose the continuous and
> > reproducible pseudo-random generation after suspend. Aren't this
> > expected out of PRNG?
> 
> An RNG has to be stateless from the perspective of the caller -- this is the 
> core implication of entropy.
> 
> Then, if you add the initial seed after the RNG lost its state implies that 
> the same sequence of random numbers starts again. I.e. where is the 
> randomness?

Ahhh, yes, you are right. The device would not continue on its random
generation because all state is lost anyway.

I'll use the generated random numbers.

> > > Besides, how do you know that slen is not larger than
> > > EXYNOS_RNG_SEED_SIZE?
> > 
> > Right, there is a overflow here. It should be sizeof(rng->seed_save);
> 
> shouldn't it be min(rng->seed_save, slen)?

Now it is irrelevant but anyway I am not allowing the seed to be less
then EXYNOS_RNG_SEED_SIZE (== sizeof(rng->seed_save)). The device
expects all five channels (registers) to be seeded.

Best regards,
Krzysztof

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Stephan Müller]

Am Freitag, 24. März 2017, 15:43:48 CET schrieb Krzysztof Kozlowski:

Hi Krzysztof,

> On Fri, Mar 24, 2017 at 03:37:59PM +0100, Stephan Müller wrote:
> > Am Freitag, 24. März 2017, 15:24:44 CET schrieb Krzysztof Kozlowski:
> > 
> > Hi Krzysztof,
> > 
> > > +
> > > +static int exynos_rng_set_seed(struct exynos_rng_dev *rng,
> > > +const u8 *seed, unsigned int slen)
> > > +{
> > > + int ret, i;
> > > + u32 val;
> > > +
> > > + dev_dbg(rng->dev, "Seeding with %u bytes\n", slen);
> > > +
> > > + ret = clk_prepare_enable(rng->clk);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (slen < EXYNOS_RNG_SEED_SIZE) {
> > > + dev_warn(rng->dev, "Seed too short (only %u bytes)\n", slen);
> > > + ret = -EINVAL;
> > > + goto out;
> > > + }
> > > +
> > > + for (i = 0 ; i < EXYNOS_RNG_SEED_REGS ; i++) {
> > > + val = seed[i * 4] << 24;
> > > + val |= seed[i * 4 + 1] << 16;
> > > + val |= seed[i * 4 + 2] << 8;
> > > + val |= seed[i * 4 + 3] << 0;
> > > +
> > > + exynos_rng_writel(rng, val, EXYNOS_RNG_SEED(i));
> > > + }
> > > +
> > > + val = exynos_rng_readl(rng, EXYNOS_RNG_STATUS);
> > > + if (!(val & EXYNOS_RNG_STATUS_SEED_SETTING_DONE)) {
> > > + dev_warn(rng->dev, "Seed setting not finished\n");
> > > + ret = -EIO;
> > > + goto out;
> > > + }
> > > +
> > > + ret = 0;
> > > + /* Save seed for suspend */
> > > + memcpy(rng->seed_save, seed, slen);
> > 
> > Is this really necessary? If you need to save some seed, shouldn't that be
> > an output of the DRNG and not the real seed?
> 
> When suspended to RAM device will loose the contents of registers
> (including SEED registers) so it has to be initialized with something on
> resume.
> 
> The seed registers are write-only so I cannot read them and store
> contents just before suspend.
> 
> I understand that real seed should not be stored... but then if I am not
> able to re-seed it with same values, I will loose the continuous and
> reproducible pseudo-random generation after suspend. Aren't this
> expected out of PRNG?

An RNG has to be stateless from the perspective of the caller -- this is the 
core implication of entropy.

Then, if you add the initial seed after the RNG lost its state implies that 
the same sequence of random numbers starts again. I.e. where is the 
randomness?
> 
> > Besides, how do you know that slen is not larger than
> > EXYNOS_RNG_SEED_SIZE?
> 
> Right, there is a overflow here. It should be sizeof(rng->seed_save);

shouldn't it be min(rng->seed_save, slen)?
> 
> Thanks for review!
> 
> Best regards,
> Krzysztof



Ciao
Stephan

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

On Fri, Mar 24, 2017 at 03:37:59PM +0100, Stephan Müller wrote:
> Am Freitag, 24. März 2017, 15:24:44 CET schrieb Krzysztof Kozlowski:
> 
> Hi Krzysztof,
> 
> > +
> > +static int exynos_rng_set_seed(struct exynos_rng_dev *rng,
> > +  const u8 *seed, unsigned int slen)
> > +{
> > +   int ret, i;
> > +   u32 val;
> > +
> > +   dev_dbg(rng->dev, "Seeding with %u bytes\n", slen);
> > +
> > +   ret = clk_prepare_enable(rng->clk);
> > +   if (ret)
> > +   return ret;
> > +
> > +   if (slen < EXYNOS_RNG_SEED_SIZE) {
> > +   dev_warn(rng->dev, "Seed too short (only %u bytes)\n", slen);
> > +   ret = -EINVAL;
> > +   goto out;
> > +   }
> > +
> > +   for (i = 0 ; i < EXYNOS_RNG_SEED_REGS ; i++) {
> > +   val = seed[i * 4] << 24;
> > +   val |= seed[i * 4 + 1] << 16;
> > +   val |= seed[i * 4 + 2] << 8;
> > +   val |= seed[i * 4 + 3] << 0;
> > +
> > +   exynos_rng_writel(rng, val, EXYNOS_RNG_SEED(i));
> > +   }
> > +
> > +   val = exynos_rng_readl(rng, EXYNOS_RNG_STATUS);
> > +   if (!(val & EXYNOS_RNG_STATUS_SEED_SETTING_DONE)) {
> > +   dev_warn(rng->dev, "Seed setting not finished\n");
> > +   ret = -EIO;
> > +   goto out;
> > +   }
> > +
> > +   ret = 0;
> > +   /* Save seed for suspend */
> > +   memcpy(rng->seed_save, seed, slen);
> 
> Is this really necessary? If you need to save some seed, shouldn't that be an 
> output of the DRNG and not the real seed?

When suspended to RAM device will loose the contents of registers
(including SEED registers) so it has to be initialized with something on
resume.

The seed registers are write-only so I cannot read them and store
contents just before suspend.

I understand that real seed should not be stored... but then if I am not
able to re-seed it with same values, I will loose the continuous and
reproducible pseudo-random generation after suspend. Aren't this
expected out of PRNG?

> Besides, how do you know that slen is not larger than EXYNOS_RNG_SEED_SIZE?

Right, there is a overflow here. It should be sizeof(rng->seed_save);

Thanks for review!

Best regards,
Krzysztof

Re: [PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Stephan Müller]

Am Freitag, 24. März 2017, 15:24:44 CET schrieb Krzysztof Kozlowski:

Hi Krzysztof,

> +
> +static int exynos_rng_set_seed(struct exynos_rng_dev *rng,
> +const u8 *seed, unsigned int slen)
> +{
> + int ret, i;
> + u32 val;
> +
> + dev_dbg(rng->dev, "Seeding with %u bytes\n", slen);
> +
> + ret = clk_prepare_enable(rng->clk);
> + if (ret)
> + return ret;
> +
> + if (slen < EXYNOS_RNG_SEED_SIZE) {
> + dev_warn(rng->dev, "Seed too short (only %u bytes)\n", slen);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + for (i = 0 ; i < EXYNOS_RNG_SEED_REGS ; i++) {
> + val = seed[i * 4] << 24;
> + val |= seed[i * 4 + 1] << 16;
> + val |= seed[i * 4 + 2] << 8;
> + val |= seed[i * 4 + 3] << 0;
> +
> + exynos_rng_writel(rng, val, EXYNOS_RNG_SEED(i));
> + }
> +
> + val = exynos_rng_readl(rng, EXYNOS_RNG_STATUS);
> + if (!(val & EXYNOS_RNG_STATUS_SEED_SETTING_DONE)) {
> + dev_warn(rng->dev, "Seed setting not finished\n");
> + ret = -EIO;
> + goto out;
> + }
> +
> + ret = 0;
> + /* Save seed for suspend */
> + memcpy(rng->seed_save, seed, slen);

Is this really necessary? If you need to save some seed, shouldn't that be an 
output of the DRNG and not the real seed?

Besides, how do you know that slen is not larger than EXYNOS_RNG_SEED_SIZE?


Ciao
Stephan

Re: next build: 208 builds: 9 failed, 199 passed, 857 errors, 444 warnings (next-20170323)

[Ralf Baechle]

On Thu, Mar 23, 2017 at 07:44:50PM +0100, Ralf Baechle wrote:

> > On Thu, Mar 23, 2017 at 6:46 AM, kernelci.org bot  wrote:
> > 
> > > acs5k_defconfig (arm) — PASS, 0 errors, 2 warnings, 0 section mismatches
> > >
> > > Warnings:
> > > :1328:2: warning: #warning syscall arch_prctl not implemented [-Wcpp]
> > > :1328:2: warning: #warning syscall arch_prctl not implemented [-Wcpp]
> > 
> > patch sent today, we don't really want this syscall except on x86
> > 
> > > allmodconfig (arm64) — FAIL, 6 errors, 5 warnings, 0 section mismatches
> > >
> > > Errors:
> > > ERROR (phandle_references): Reference to non-existent node or label 
> > > "pwm_f_clk_pins"
> > > ERROR (phandle_references): Reference to non-existent node or label 
> > > "pwm_ao_a_3_pins"
> > > ERROR: Input tree has errors, aborting (use -f to force output)
> > > ERROR (phandle_references): Reference to non-existent node or label 
> > > "pwm_f_clk_pins"
> > > ERROR (phandle_references): Reference to non-existent node or label 
> > > "pwm_ao_a_3_pins"
> > > ERROR: Input tree has errors, aborting (use -f to force output)
> > 
> > Kevin has already backed out the commit that caused this.
> > 
> > > Warnings:
> > > :1325:2: warning: #warning syscall statx not implemented [-Wcpp]
> > 
> > Should get fixed in the next few days when the patch gets picked up for 
> > arm64.
> > 
> > > drivers/misc/aspeed-lpc-ctrl.c:232:17: warning: format '%x' expects 
> > > argument of type 'unsigned int', but argument 4 has type 'resource_size_t 
> > > {aka long long unsigned int}' [-Wformat=]
> > 
> > patch sent today
> > 
> > > include/uapi/linux/byteorder/big_endian.h:32:26: warning: large integer 
> > > implicitly truncated to unsigned type [-Woverflow]
> > > include/uapi/linux/byteorder/big_endian.h:32:26: warning: large integer 
> > > implicitly truncated to unsigned type [-Woverflow]
> > 
> > I sent this one a few days ago
> > 
> > > allmodconfig (x86) — PASS, 0 errors, 11 warnings, 0 section mismatches
> > >
> > > Warnings:
> > > drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' 
> > > expects argument of type 'long int', but argument 4 has type 'long long 
> > > int' [-Wformat=]
> > > drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' 
> > > expects argument of type 'long int', but argument 5 has type 'long long 
> > > int' [-Wformat=]
> > 
> > This one too.
> > 
> > > cavium_octeon_defconfig (mips) — FAIL, 830 errors, 3 warnings, 0 section 
> > > mismatches
> > >
> > > Errors:
> > > arch/mips/include/asm/octeon/cvmx-mio-defs.h:78:3: error: expected 
> > > specifier-qualifier-list before '__BITFIELD_FIELD'
> > > arch/mips/include/asm/octeon/cvmx-mio-defs.h:95:3: error: expected 
> > > specifier-qualifier-list before '__BITFIELD_FIELD'
> > 
> > Maybe caused by 4cd156de2ca8 ("MIPS: Octeon: Remove unused MIO types
> > and macros.")
> > I have not looked in detail
> 
> Seems an #include  is missing.  I'm going to sort
> this one.

I fixed this in my branch for linux-next only to encounter another build
error so I dropped another two patches.

  Ralf

[PATCH 3/3] ARM: multi_v7_defconfig: Enable Exynos RNG and user-space crypto API

[Krzysztof Kozlowski]

Enable the new Exynos pseudo random number generator driver and
user-space API to access crypto algorithms and devices (not only RNG).

Signed-off-by: Krzysztof Kozlowski 
---
 arch/arm/configs/multi_v7_defconfig | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/configs/multi_v7_defconfig 
b/arch/arm/configs/multi_v7_defconfig
index 36c1b39cfb54..9bc1bd7f2afa 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -935,7 +935,13 @@ CONFIG_CPUFREQ_DT=y
 CONFIG_KEYSTONE_IRQ=y
 CONFIG_HW_RANDOM=y
 CONFIG_HW_RANDOM_ST=y
+CONFIG_CRYPTO_USER=m
+CONFIG_CRYPTO_USER_API_HASH=m
+CONFIG_CRYPTO_USER_API_SKCIPHER=m
+CONFIG_CRYPTO_USER_API_RNG=m
+CONFIG_CRYPTO_USER_API_AEAD=m
 CONFIG_CRYPTO_DEV_MARVELL_CESA=m
+CONFIG_CRYPTO_DEV_EXYNOS_RNG=m
 CONFIG_CRYPTO_DEV_S5P=m
 CONFIG_CRYPTO_DEV_SUN4I_SS=m
 CONFIG_CRYPTO_DEV_ROCKCHIP=m
-- 
2.9.3

[PATCH 2/3] ARM: exynos_defconfig: Enable Exynos RNG and user-space crypto API

[Krzysztof Kozlowski]

Enable the new Exynos pseudo random number generator driver and
user-space API to access crypto algorithms and devices (not only RNG).

Signed-off-by: Krzysztof Kozlowski 
---
 arch/arm/configs/exynos_defconfig | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/configs/exynos_defconfig 
b/arch/arm/configs/exynos_defconfig
index 6dc661c4a2c1..960d55445e05 100644
--- a/arch/arm/configs/exynos_defconfig
+++ b/arch/arm/configs/exynos_defconfig
@@ -265,6 +265,12 @@ CONFIG_DEBUG_RT_MUTEXES=y
 CONFIG_DEBUG_SPINLOCK=y
 CONFIG_DEBUG_MUTEXES=y
 CONFIG_DEBUG_USER=y
+CONFIG_CRYPTO_USER=m
+CONFIG_CRYPTO_USER_API_HASH=m
+CONFIG_CRYPTO_USER_API_SKCIPHER=m
+CONFIG_CRYPTO_USER_API_RNG=m
+CONFIG_CRYPTO_USER_API_AEAD=m
+CONFIG_CRYPTO_DEV_EXYNOS_RNG=y
 CONFIG_CRYPTO_DEV_S5P=y
 CONFIG_ARM_CRYPTO=y
 CONFIG_CRYPTO_SHA1_ARM_NEON=m
-- 
2.9.3

[PATCH 1/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

Replace existing hw_ranndom/exynos-rng driver with a new, reworked one.
This is a driver for pseudo random number generator block which on
Exynos4 chipsets must be seeded with some value.  On newer Exynos5420
chipsets it might seed itself from true random number generator block
but this is not implemented yet.

New driver is a complete rework to use the crypto ALGAPI instead of
hw_random API.  Rationale for the change:
1. hw_random interface is for true RNG devices.
2. The old driver was seeding itself with jiffies which is not a
   reliable source for randomness.
3. Device generates five random numbers in each pass but old driver was
   returning only one thus its performance was reduced.

Compatibility with DeviceTree bindings is preserved.

New driver does not use runtime power management but manually enables
and disables the clock when needed.  This is preferred approach because
using runtime PM just to toggle clock is huge overhead.  Another
difference is using the same seed after resuming from system suspend
(previously driver was re-seeding itself again with jiffies).

Signed-off-by: Krzysztof Kozlowski 
---
 MAINTAINERS |   8 +
 drivers/char/hw_random/Kconfig  |  14 --
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/exynos-rng.c | 231 ---
 drivers/crypto/Kconfig  |  15 ++
 drivers/crypto/Makefile |   1 +
 drivers/crypto/exynos-rng.c | 306 
 7 files changed, 330 insertions(+), 246 deletions(-)
 delete mode 100644 drivers/char/hw_random/exynos-rng.c
 create mode 100644 drivers/crypto/exynos-rng.c

diff --git a/MAINTAINERS b/MAINTAINERS
index affecc6d59f4..371fda859d43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10977,6 +10977,14 @@ L: alsa-de...@alsa-project.org (moderated for 
non-subscribers)
 S: Supported
 F: sound/soc/samsung/
 
+SAMSUNG EXYNOS PSEUDO RANDOM NUMBER GENERATOR (RNG) DRIVER
+M: Krzysztof Kozlowski 
+L: linux-crypto@vger.kernel.org
+L: linux-samsung-...@vger.kernel.org
+S: Maintained
+F: drivers/crypto/exynos-rng.c
+F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
+
 SAMSUNG FRAMEBUFFER DRIVER
 M: Jingoo Han 
 L: linux-fb...@vger.kernel.org
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 0cafe08919c9..bdae802e7154 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -294,20 +294,6 @@ config HW_RANDOM_POWERNV
 
  If unsure, say Y.
 
-config HW_RANDOM_EXYNOS
-   tristate "EXYNOS HW random number generator support"
-   depends on ARCH_EXYNOS || COMPILE_TEST
-   depends on HAS_IOMEM
-   default HW_RANDOM
-   ---help---
- This driver provides kernel-side support for the Random Number
- Generator hardware found on EXYNOS SOCs.
-
- To compile this driver as a module, choose M here: the
- module will be called exynos-rng.
-
- If unsure, say Y.
-
 config HW_RANDOM_TPM
tristate "TPM HW Random Number Generator support"
depends on TCG_TPM
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index 5f52b1e4e7be..6f1eecc2045c 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -24,7 +24,6 @@ obj-$(CONFIG_HW_RANDOM_OCTEON) += octeon-rng.o
 obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
 obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
 obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o
-obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o
 obj-$(CONFIG_HW_RANDOM_HISI)   += hisi-rng.o
 obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
 obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
diff --git a/drivers/char/hw_random/exynos-rng.c 
b/drivers/char/hw_random/exynos-rng.c
deleted file mode 100644
index 23d358553b21..
--- a/drivers/char/hw_random/exynos-rng.c
+++ /dev/null
@@ -1,231 +0,0 @@
-/*
- * exynos-rng.c - Random Number Generator driver for the exynos
- *
- * Copyright (C) 2012 Samsung Electronics
- * Jonghwa Lee 
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation;
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
- *
- */
-
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-#define EXYNOS_PRNG_STATUS_OFFSET  0x10
-#define 

[PATCH 0/3] crypto: hw_random - Add new Exynos RNG driver

[Krzysztof Kozlowski]

Hi,

This is a follow up of my questions around exynos-rng [1].

The existing exynos-rng has many issues.  The most important one is that
it is a pseudo RNG device but uses hw_random interface which does not allow
proper seeding.

The RNG module on Exynos4 requires seeding.  On newer SoCs (like Exynos5420)
it can seed itself from a true RNG.  Converting the existing driver
to use TRNG would effectively drop support for Exynos4 and break
compatibility with existing users.

Instead I decided to convert it to crypto API.  In the future I hope
to add support for seeding from TRNG module.

Tested with app [2].

Patches are independent. I will take the defconfig changes (2/3 and 3/3)
through samsung-soc tree.

Best regards,
Krzysztof

[1] https://www.spinics.net/lists/arm-kernel/msg569641.html
[2] https://www.spinics.net/lists/arm-kernel/msg571184.html

Krzysztof Kozlowski (3):
  crypto: hw_random - Add new Exynos RNG driver
  ARM: exynos_defconfig: Enable Exynos RNG and user-space crypto API
  ARM: multi_v7_defconfig: Enable Exynos RNG and user-space crypto API

 MAINTAINERS |   8 +
 arch/arm/configs/exynos_defconfig   |   6 +
 arch/arm/configs/multi_v7_defconfig |   6 +
 drivers/char/hw_random/Kconfig  |  14 --
 drivers/char/hw_random/Makefile |   1 -
 drivers/char/hw_random/exynos-rng.c | 231 ---
 drivers/crypto/Kconfig  |  15 ++
 drivers/crypto/Makefile |   1 +
 drivers/crypto/exynos-rng.c | 306 
 9 files changed, 342 insertions(+), 246 deletions(-)
 delete mode 100644 drivers/char/hw_random/exynos-rng.c
 create mode 100644 drivers/crypto/exynos-rng.c

-- 
2.9.3

Re: [PATCH] crypto: zip - add a cast for printing atomic64_t values

[Herbert Xu]

On Mon, Mar 20, 2017 at 01:39:16PM +0100, Arnd Bergmann wrote:
> kernelci.org reports a build-time regression on linux-next, with a harmless
> warning in x86 allmodconfig:
> 
> drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects 
> argument of type 'long int', but argument 7 has type 'long long int' 
> [-Wformat=]
> drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects 
> argument of type 'long int', but argument 6 has type 'long long int' 
> [-Wformat=]
> drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects 
> argument of type 'long int', but argument 5 has type 'long long int' 
> [-Wformat=]
> 
> The return type for atomic64_read() unfortunately differs between
> architectures, with some defining it as atomic_long_read() and others
> returning a 64-bit type explicitly. Fixing this in general would be nice,
> but also require changing other users of these functions, so the simpler
> workaround is to add a cast here that avoids the warnings on the default
> build.
> 
> Fixes: 09ae5d37e093 ("crypto: zip - Add Compression/Decompression statistics")
> Signed-off-by: Arnd Bergmann 

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

Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array

[Dmitry Vyukov]

On Fri, Mar 24, 2017 at 3:10 PM, Peter Zijlstra  wrote:
> On Fri, Mar 24, 2017 at 02:50:24PM +0100, Dmitry Vyukov wrote:
>> OK, I guess should not have referenced the llvm-linux page.
>> So here are reasons on our side that I am ready to vouch:
>>
>>  - clang make it possible to implement KMSAN (dynamic detection of
>> uses of uninit memory)
>
> How does GCC make this impossible?

Too complex and too difficult to implement correctly on all corner
cases. All other sanitizers were ported to gcc very quickly, but msan
wasn't. Nobody is brave enough to even approach it.

>>  - better code coverage for fuzzing
>
> How so? Why can't the same be achieved using GCC?

Same reason.

>>  - why simpler and faster development (e.g. we can port our user-space
>> hardening technologies -- CFI and SafeStack)
>
> That's just because you've already implemented this in clang, right? So
> less work for you. Not because its impossible.

I am not saying that it's impossible. It would just require
unreasonable amount of time, and then perpetual maintenance to fix
corner cases and regressions.

For background: I implemented the current fuzzing coverage (KCOV) in
gcc, and user-space tsan instrumentation in gcc.

Re: [PATCH 1/4] crypto: powerpc - Factor out the core CRC vpmsum algorithm

[Herbert Xu]

Daniel Axtens  wrote:
> The core nuts and bolts of the crc32c vpmsum algorithm will
> also work for a number of other CRC algorithms with different
> polynomials. Factor out the function into a new asm file.
> 
> To handle multiple users of the function, a user simply
> provides constants, defines the name of their CRC function,
> and then #includes the core algorithm file.
> 
> Cc: Anton Blanchard 
> Signed-off-by: Daniel Axtens 

All patches applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH V3 0/3] Support new function in new CCPs

[Herbert Xu]

On Wed, Mar 15, 2017 at 01:20:34PM -0500, Gary R Hook wrote:
> The following series implements new function in a version 5 coprocessor.
> New features are:
>  - Support for SHA-2 384-bit and 512-bit hashing
>  - Support for 3DES encryption
>  - Support for AES GCM encryption
> 
> Changes from V2:
>  - Correct a comment in the GCM support code.
>  - Ensure the patches apply to the current repo

All patches applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 0/7] crypto: caam - add Queue Interface (QI) support

[Herbert Xu]

On Fri, Mar 17, 2017 at 12:05:55PM +0200, Horia Geantă wrote:
> RFC -> v1:
> -rebased on latest cryptodev-2.6 tree
> open-code tsk_cpus_allowed() - sync with commit
> 0c98d344fe5c "sched/core: Remove the tsk_cpus_allowed() wrapper"
> -addressed Scott's feedback - removed most of the accessors
> added in soc/qman (patch 4) and instead open-coded them in caam/qi
> 
> 
> The patchset adds support for CAAM Queue Interface (QI), the additional
> interface (besides job ring) available for submitting jobs to the engine
> on platforms having DPAA (Datapath Acceleration Architecture).
> 
> Patches 1-4 are QMan dependencies.
> During RFC stage, we agreed to go with them through the crypto tree:
> https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg24105.html
> 
> Patch 5 adds a missing double inclusion guard in desc_constr.h.
> 
> Patch 6 adds the caam/qi job submission backend.
> 
> Patch 7 adds algorithms (ablkcipher and authenc) that run on top
> of caam/qi. For now, their priority is set lower than caam/jr.

All patches applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2] dt-bindings: rng: clocks property on omap_rng not always mandatory

[Rob Herring]

On Fri, Mar 24, 2017 at 09:19:05AM -0500, Rob Herring wrote:
> On Fri, Mar 24, 2017 at 08:52:00AM -0500, Rob Herring wrote:
> > On Fri, Mar 17, 2017 at 01:58:39PM +0100, Thomas Petazzoni wrote:
> > > Commit 52060836f79 ("dt-bindings: omap-rng: Document SafeXcel IP-76
> > > device variant") update the omap_rng Device Tree binding to add support
> > > for the IP-76 variation of the IP. As part of this change, a "clocks"
> > > property was added, but is indicated as "Required", without indicated
> > > it's actually only required for some compatible strings.
> > > 
> > > This commit fixes that, by explicitly stating that the clocks property
> > > is only required with the inside-secure,safexcel-eip76 compatible
> > > string.
> > > 
> > > Fixes: 52060836f79 ("dt-bindings: omap-rng: Document SafeXcel IP-76 
> > > device variant")
> > > Cc: 
> > > Signed-off-by: Thomas Petazzoni 
> > > ---
> > > Changes since v1:
> > >  - Instead of indicating the property as optional, indicate it as
> > >mandatory for the inside-secure,safexcel-eip76 compatible string, as
> > >suggested by Rob Herring.
> > > ---
> > >  Documentation/devicetree/bindings/rng/omap_rng.txt | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > Acked-by: Rob Herring 
> 
> And applied.

Ah, I see Herbert applied it, so I've dropped it.

Rob

Re: [PATCH v2] dt-bindings: rng: clocks property on omap_rng not always mandatory

[Rob Herring]

On Fri, Mar 24, 2017 at 08:52:00AM -0500, Rob Herring wrote:
> On Fri, Mar 17, 2017 at 01:58:39PM +0100, Thomas Petazzoni wrote:
> > Commit 52060836f79 ("dt-bindings: omap-rng: Document SafeXcel IP-76
> > device variant") update the omap_rng Device Tree binding to add support
> > for the IP-76 variation of the IP. As part of this change, a "clocks"
> > property was added, but is indicated as "Required", without indicated
> > it's actually only required for some compatible strings.
> > 
> > This commit fixes that, by explicitly stating that the clocks property
> > is only required with the inside-secure,safexcel-eip76 compatible
> > string.
> > 
> > Fixes: 52060836f79 ("dt-bindings: omap-rng: Document SafeXcel IP-76 device 
> > variant")
> > Cc: 
> > Signed-off-by: Thomas Petazzoni 
> > ---
> > Changes since v1:
> >  - Instead of indicating the property as optional, indicate it as
> >mandatory for the inside-secure,safexcel-eip76 compatible string, as
> >suggested by Rob Herring.
> > ---
> >  Documentation/devicetree/bindings/rng/omap_rng.txt | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Acked-by: Rob Herring 

And applied.

Rob

Re: [PATCH] crypto: ccp - Make some CCP DMA channels private

[Herbert Xu]

On Thu, Mar 23, 2017 at 12:53:30PM -0500, Gary R Hook wrote:
> The CCP registers its queues as channels capable of handling
> general DMA operations. The NTB driver will use DMA if
> directed, but as public channels can be reserved for use in
> asynchronous operations some channels should be held back
> as private. Since the public/private determination is
> handled at a device level, reserve the "other" (secondary)
> CCP channels as private.
> 
> Add a module parameter that allows for override, to be
> applied to all channels on all devices.
> 
> CC:  # 4.10.x-
> Signed-off-by: Gary R Hook 

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

Re: [PATCH] crypto: ixp4xx - Use sg_virt()

[Herbert Xu]

On Thu, Mar 23, 2017 at 09:16:30PM +0800, Geliang Tang wrote:
> Use sg_virt() instead of open-coding it.
> 
> Signed-off-by: Geliang Tang 

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

Re: [PATCH] crypto: zip - Memory corruption in zip_clear_stats()

[Herbert Xu]

On Fri, Mar 17, 2017 at 11:46:21PM +0300, Dan Carpenter wrote:
> There is a typo here.  It should be "stats" instead of "state".  The
> impact is that we clear 224 bytes instead of 80 and we zero out memory
> that we shouldn't.
> 
> Fixes: 09ae5d37e093 ("crypto: zip - Add Compression/Decompression statistics")
> Signed-off-by: Dan Carpenter 

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

Re: [PATCH] padata: avoid race in reordering

[Herbert Xu]

Jason A. Donenfeld  wrote:
> Under extremely heavy uses of padata, crashes occur, and with list
> debugging turned on, this happens instead:
> 
> [87487.298728] WARNING: CPU: 1 PID: 882 at lib/list_debug.c:33
> __list_add+0xae/0x130
> [87487.301868] list_add corruption. prev->next should be next
> (b17abfc043d0), but was 8dba70872c80. (prev=8dba70872b00).
> [87487.339011]  [] dump_stack+0x68/0xa3
> [87487.342198]  [] ? console_unlock+0x281/0x6d0
> [87487.345364]  [] __warn+0xff/0x140
> [87487.348513]  [] warn_slowpath_fmt+0x4a/0x50
> [87487.351659]  [] __list_add+0xae/0x130
> [87487.354772]  [] ? _raw_spin_lock+0x64/0x70
> [87487.357915]  [] padata_reorder+0x1e6/0x420
> [87487.361084]  [] padata_do_serial+0xa5/0x120
> 
> padata_reorder calls list_add_tail with the list to which its adding
> locked, which seems correct:
> 
> spin_lock(>serial.lock);
> list_add_tail(>list, >serial.list);
> spin_unlock(>serial.lock);
> 
> This therefore leaves only place where such inconsistency could occur:
> if padata->list is added at the same time on two different threads.
> This pdata pointer comes from the function call to
> padata_get_next(pd), which has in it the following block:
> 
> next_queue = per_cpu_ptr(pd->pqueue, cpu);
> padata = NULL;
> reorder = _queue->reorder;
> if (!list_empty(>list)) {
>   padata = list_entry(reorder->list.next,
>   struct padata_priv, list);
>   spin_lock(>lock);
>   list_del_init(>list);
>   atomic_dec(>reorder_objects);
>   spin_unlock(>lock);
> 
>   pd->processed++;
> 
>   goto out;
> }
> out:
> return padata;
> 
> I strongly suspect that the problem here is that two threads can race
> on reorder list. Even though the deletion is locked, call to
> list_entry is not locked, which means it's feasible that two threads
> pick up the same padata object and subsequently call list_add_tail on
> them at the same time. The fix is thus be hoist that lock outside of
> that block.
> 
> Signed-off-by: Jason A. Donenfeld 

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

Re: [PATCH] crypto: DRBG - initialize SGL only once

[Herbert Xu]

On Wed, Mar 22, 2017 at 03:26:36PM +0100, Stephan Müller wrote:
> An SGL to be initialized only once even when its buffers are written
> to several times.
> 
> Signed-off-by: Stephan Mueller 

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

Re: [PATCH] crypto: testmgr - mark ctr(des3_ede) as fips_allowed

[Herbert Xu]

On Mon, Mar 20, 2017 at 05:28:05PM -0300, Marcelo Henrique Cerri wrote:
> 3DES is missing the fips_allowed flag for CTR mode.
> 
> Signed-off-by: Marcelo Henrique Cerri 

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

Re: [PATCH] crypto: xts,lrw - fix out-of-bounds write after kmalloc failure

[Herbert Xu]

On Thu, Mar 23, 2017 at 01:39:46PM -0700, Eric Biggers wrote:
> From: Eric Biggers 
> 
> In the generic XTS and LRW algorithms, for input data > 128 bytes, a
> temporary buffer is allocated to hold the values to be XOR'ed with the
> data before and after encryption or decryption.  If the allocation
> fails, the fixed-size buffer embedded in the request buffer is meant to
> be used as a fallback --- resulting in more calls to the ECB algorithm,
> but still producing the correct result.  However, we weren't correctly
> limiting subreq->cryptlen in this case, resulting in pre_crypt()
> overrunning the embedded buffer.  Fix this by setting subreq->cryptlen
> correctly.
> 
> Fixes: f1c131b45410 ("crypto: xts - Convert to skcipher")
> Fixes: 700cb3f5fe75 ("crypto: lrw - Convert to skcipher")
> Cc: sta...@vger.kernel.org # v4.10+
> Reported-by: Dmitry Vyukov 
> Signed-off-by: Eric Biggers 

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

Re: [PATCH v2] dt-bindings: rng: clocks property on omap_rng not always mandatory

[Herbert Xu]

On Fri, Mar 17, 2017 at 01:58:39PM +0100, Thomas Petazzoni wrote:
> Commit 52060836f79 ("dt-bindings: omap-rng: Document SafeXcel IP-76
> device variant") update the omap_rng Device Tree binding to add support
> for the IP-76 variation of the IP. As part of this change, a "clocks"
> property was added, but is indicated as "Required", without indicated
> it's actually only required for some compatible strings.
> 
> This commit fixes that, by explicitly stating that the clocks property
> is only required with the inside-secure,safexcel-eip76 compatible
> string.
> 
> Fixes: 52060836f79 ("dt-bindings: omap-rng: Document SafeXcel IP-76 device 
> variant")
> Cc: 
> Signed-off-by: Thomas Petazzoni 

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

Re: [PATCH 0/4] crypto: s5p-sss - Fix and minor improvements

[Herbert Xu]

On Fri, Mar 17, 2017 at 04:49:18PM +0200, Krzysztof Kozlowski wrote:
> Hi,
> 
> I still did not fix the NULL pointer dereference reported by
> Nathan Royce [1], but I got some other improvements.
> 
> Testing done on Odroid U3 (Exynos4412) with tcrypt and cryptsetup.
> 
> Best regards,
> Krzysztof

Patches 1-3 applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto, x86: aesni - fix token pasting for clang

[Herbert Xu]

On Wed, Mar 15, 2017 at 03:36:00PM -0700, Michael Davidson wrote:
> aes_ctrby8_avx-x86_64.S uses the C preprocessor for token pasting
> of character sequences that are not valid preprocessor tokens.
> While this is allowed when preprocessing assembler files it exposes
> an incompatibilty between the clang and gcc preprocessors where
> clang does not strip leading white space from macro parameters,
> leading to the CONCAT(%xmm, i) macro expansion on line 96 resulting
> in a token with a space character embedded in it.
> 
> While this could be resolved by deleting the offending space character,
> the assembler is perfectly capable of doing the token pasting correctly
> for itself so we can just get rid of the preprocessor macros.
> 
> Signed-off-by: Michael Davidson 

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

Re: [PATCH] md5: remove from lib and only live in crypto

[Herbert Xu]

On Thu, Mar 16, 2017 at 03:18:57PM +0100, Jason A. Donenfeld wrote:
> The md5_transform function is no longer used any where in the tree,
> except for the crypto api's actual implementation of md5, so we can drop
> the function from lib and put it as a static function of the crypto
> file, where it belongs. There should be no new users of md5_transform,
> anyway, since there are more modern ways of doing what it once achieved.
> 
> Signed-off-by: Jason A. Donenfeld 

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

Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array

[Peter Zijlstra]

On Fri, Mar 24, 2017 at 02:50:24PM +0100, Dmitry Vyukov wrote:
> OK, I guess should not have referenced the llvm-linux page.
> So here are reasons on our side that I am ready to vouch:
> 
>  - clang make it possible to implement KMSAN (dynamic detection of
> uses of uninit memory)

How does GCC make this impossible?

>  - better code coverage for fuzzing

How so? Why can't the same be achieved using GCC?

>  - why simpler and faster development (e.g. we can port our user-space
> hardening technologies -- CFI and SafeStack)

That's just because you've already implemented this in clang, right? So
less work for you. Not because its impossible.

Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array

[Peter Zijlstra]

On Fri, Mar 24, 2017 at 02:47:15PM +0100, Dmitry Vyukov wrote:
> > Seriously, you should have taken the hack the first time that this
> > needs to be fixed.  Just because this is a fairly uncommon construct
> > in the kernel doesn't mean it is not in userspace.
> 
> There is a reason why it is fairly uncommon in kernel.

So first off; its not entirely clear that the code as it exists it
correct. From a cursory reading of it and surrounding code, there is no
actual upper limit on the variable. If I were stupid enough to make a
raid with 64 devices I'd get a huge on-stack structure.

Since you're touching it; you should check these things.

And secondly, refactor the code to not look like dog vomit. You can do
more than the absolute minimal patch to make it compile, I'm sure.

Re: [PATCH] arm64: dts: ls1012a: add crypto node

[Shawn Guo]

On Fri, Mar 24, 2017 at 08:29:17AM +, Horia Geantă wrote:
> On 3/24/2017 9:35 AM, Shawn Guo wrote:
> > On Fri, Mar 24, 2017 at 07:17:50AM +, Horia Geantă wrote:
>  +sec_mon: sec_mon@1e9 {
> >>>
> >>> Hyphen is more preferred to be used in node name than underscore.
> >>>
> >> This would imply changing the
> >> Documentation/devicetree/bindings/crypto/fsl-sec4.txt binding and
> >> dealing with all the consequences, which IIUC is probably not worth.
> > 
> > I do not care the bindings doc that much, since I'm not the maintainer
> > of it.  What are the consequences specifically, if we use a better node
> > name in dts than bindings example?
> > 
> Users relying on finding the sec_mon node will obviously stop working.
> I don't see any in-kernel users, however there could be others I am not
> aware of and DT bindings should provide for backwards compatibility.

Okay, point taken.  You can keep the node name as it is.

> I could deprecate "sec_mon" in the bindings and suggest "sec-mon"
> instead, while leaving all existing dts files as-is.
> The risk is breaking LS1012A users relying on "sec_mon".

For existing bindings, I do not care that much.  But for new ones, I do
hope that we recommend to use hyphen, as that's more idiomatic at least
for Linux kernel.

> I see that ePAPR:
> -allows both for hyphen and underline in case of node names
> -allows only for hyphen (i.e. forbids underline) in case of alias nodes
> 
> In the first case, I understand there's an (undocumented?) agreement to
> prefer hyphen over underline.

Both are valid, but hyphen is more idiomatic for Linux kernel.

> For the 2nd one, does this mean I should change alias names?

This is something I see difference between specification and DTC.

aliases {
alias-name = _name;
};

label_name: node-name {
...
};

The spec says that only hyphen is valid for alias name, but DTC works
happily with underscore too.  From my experience with DTC playing, both
hyphen and underscore are valid for alias and node name.  But for label
name, only underscore is valid.  Using hyphen in label name will cause
DTC to report syntax error.

Shawn

Re: [PATCH v2] dt-bindings: rng: clocks property on omap_rng not always mandatory

[Rob Herring]

On Fri, Mar 17, 2017 at 01:58:39PM +0100, Thomas Petazzoni wrote:
> Commit 52060836f79 ("dt-bindings: omap-rng: Document SafeXcel IP-76
> device variant") update the omap_rng Device Tree binding to add support
> for the IP-76 variation of the IP. As part of this change, a "clocks"
> property was added, but is indicated as "Required", without indicated
> it's actually only required for some compatible strings.
> 
> This commit fixes that, by explicitly stating that the clocks property
> is only required with the inside-secure,safexcel-eip76 compatible
> string.
> 
> Fixes: 52060836f79 ("dt-bindings: omap-rng: Document SafeXcel IP-76 device 
> variant")
> Cc: 
> Signed-off-by: Thomas Petazzoni 
> ---
> Changes since v1:
>  - Instead of indicating the property as optional, indicate it as
>mandatory for the inside-secure,safexcel-eip76 compatible string, as
>suggested by Rob Herring.
> ---
>  Documentation/devicetree/bindings/rng/omap_rng.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Acked-by: Rob Herring 

Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array

[Dmitry Vyukov]

On Fri, Mar 17, 2017 at 8:29 PM, Peter Zijlstra  wrote:
> On Fri, Mar 17, 2017 at 08:26:42PM +0100, Peter Zijlstra wrote:
>> On Fri, Mar 17, 2017 at 08:05:16PM +0100, Dmitry Vyukov wrote:
>> > You can also find some reasons in the Why section of LLVM-Linux project:
>> > http://llvm.linuxfoundation.org/index.php/Main_Page
>>
>> From that:
>>
>>  - LLVM/Clang is a fast moving project with many things fixed quickly
>>and features added.
>>
>> So what's the deal with that 5 year old bug you want us to work around?
>>
>> Also, clang doesn't support asm cc flags output and a few other
>> extensions last time I checked.
>>
>
> Another great one:
>
>  - BSD License (some people prefer this license to the GPL)
>
> Seems a very weak argument to make when talking about the Linux Kernel
> which is very explicitly GPLv2 (and not later).

OK, I guess should not have referenced the llvm-linux page.
So here are reasons on our side that I am ready to vouch:

 - clang make it possible to implement KMSAN (dynamic detection of
uses of uninit memory)
 - better code coverage for fuzzing
 - why simpler and faster development (e.g. we can port our user-space
hardening technologies -- CFI and SafeStack)

Michael is on a different team and has own reasons to do this.

Re: [PATCH 6/7] md/raid10, LLVM: get rid of variable length array

[Dmitry Vyukov]

On Fri, Mar 17, 2017 at 9:04 PM,   wrote:
> On March 17, 2017 12:27:46 PM PDT, Peter Zijlstra  
> wrote:
>>On Fri, Mar 17, 2017 at 11:52:01AM -0700, Michael Davidson wrote:
>>> On Fri, Mar 17, 2017 at 5:44 AM, Peter Zijlstra
>> wrote:
>>> >
>>> > Be that as it may; what you construct above is disgusting. Surely
>>the
>>> > code can be refactored to not look like dog vomit?
>>> >
>>> > Also; its not immediately obvious conf->copies is 'small' and this
>>> > doesn't blow up the stack; I feel that deserves a comment
>>somewhere.
>>> >
>>>
>>> I agree that the code is horrible.
>>>
>>> It is, in fact, exactly the same solution that was used to remove
>>> variable length arrays in structs from several of the crypto drivers
>>a
>>> few years ago - see the definition of SHASH_DESC_ON_STACK() in
>>> "crypto/hash.h" - I did not, however, hide the horrors in a macro
>>> preferring to leave the implementation visible as a warning to
>>whoever
>>> might touch the code next.
>>>
>>> I believe that the actual stack usage is exactly the same as it was
>>previously.
>>>
>>> I can certainly wrap this  up in a macro and add comments with
>>> appropriately dire warnings in it if you feel that is both necessary
>>> and sufficient.
>>
>>We got away with ugly in the past, so we should get to do it again?
>
> Seriously, you should have taken the hack the first time that this needs to 
> be fixed.  Just because this is a fairly uncommon construct in the kernel 
> doesn't mean it is not in userspace.


There is a reason why it is fairly uncommon in kernel.
Initially it was used more widely, but then there was a decision to
drop all uses of this feature. Namely:
Linus: "We should definitely drop it. The feature is an abomination".
https://lkml.org/lkml/2013/9/23/500
I really don't understand why you cling onto this last use of the
feature. Having a single use of a compiler extension on an error path
of a non-mandatory driver does not look like a great idea to me. Let's
just kill it off outside of clang discussion.

Crypto Fixes for 4.11

[Herbert Xu]

Hi Linus:

This push fixes regressions in the crypto ccp driver and the
hwrng drivers amd and geode.


Please pull from

git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git linus


Gary R Hook (1):
  crypto: ccp - Assign DMA commands to the channel's CCP

Prarit Bhargava (2):
  hwrng: amd - Revert managed API changes
  hwrng: geode - Revert managed API changes

 drivers/char/hw_random/amd-rng.c   |   42 --
 drivers/char/hw_random/geode-rng.c |   50 +---
 drivers/crypto/ccp/ccp-dev.c   |5 +++-
 drivers/crypto/ccp/ccp-dmaengine.c |1 +
 include/linux/ccp.h|2 +-
 5 files changed, 75 insertions(+), 25 deletions(-)

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

Re: [PATCH v2 2/5] crypto: stm32 - Support for STM32 CRC32 crypto module

[PrasannaKumar Muralidharan]

On 24 March 2017 at 15:26, Fabien DESSENNE  wrote:
> On 24/03/17 05:28, PrasannaKumar Muralidharan wrote:
>> On 21 March 2017 at 20:43, Fabien Dessenne  wrote:
>>> This module registers a CRC32 ("Ethernet") and a CRC32C (Castagnoli)
>>> algorithm that make use of the STMicroelectronics STM32 crypto hardware.
>>>
>>> Theses algorithms are compatible with the little-endian generic ones.
>>> Both algorithms use ~0 as default seed (key).
>>> With CRC32C the output is xored with ~0.
>>>
>>> Using TCRYPT CRC32C speed test, this shows up to 900% speedup compared
>>> to the crc32c-generic algorithm.
>> Comparing with crc3c-generic alogrithm does not sound like a good
>> metric for someone who has to decide between hw crypto or not.
>> Wouldn't it be better if the comparison is between crc32 using NEON
>> with hw crypto module? It will help in choosing between hw crypto or
>> arch optimised crc routiene.
>
> The STM32 microcontrollers are based on ARM Cortex-M7 (or older core)
> that do not have NEON support.

I was not aware of the absence of NEON support. Sorry for the noise.

Re: [PATCH v2 2/5] crypto: stm32 - Support for STM32 CRC32 crypto module

[Fabien DESSENNE]

On 24/03/17 05:28, PrasannaKumar Muralidharan wrote:
> On 21 March 2017 at 20:43, Fabien Dessenne  wrote:
>> This module registers a CRC32 ("Ethernet") and a CRC32C (Castagnoli)
>> algorithm that make use of the STMicroelectronics STM32 crypto hardware.
>>
>> Theses algorithms are compatible with the little-endian generic ones.
>> Both algorithms use ~0 as default seed (key).
>> With CRC32C the output is xored with ~0.
>>
>> Using TCRYPT CRC32C speed test, this shows up to 900% speedup compared
>> to the crc32c-generic algorithm.
> Comparing with crc3c-generic alogrithm does not sound like a good
> metric for someone who has to decide between hw crypto or not.
> Wouldn't it be better if the comparison is between crc32 using NEON
> with hw crypto module? It will help in choosing between hw crypto or
> arch optimised crc routiene.

The STM32 microcontrollers are based on ARM Cortex-M7 (or older core) 
that do not have NEON support.
Moreover, the purpose of this introduction is not to provide with a 
(full) benchmark. It just gives a hint of the HW performance.

>> Signed-off-by: Fabien Dessenne 
>> ---
>>   drivers/crypto/Kconfig |   2 +
>>   drivers/crypto/Makefile|   1 +
>>   drivers/crypto/stm32/Kconfig   |   8 +
>>   drivers/crypto/stm32/Makefile  |   2 +
>>   drivers/crypto/stm32/stm32_crc32.c | 324 
>> +
>>   5 files changed, 337 insertions(+)
>>   create mode 100644 drivers/crypto/stm32/Kconfig
>>   create mode 100644 drivers/crypto/stm32/Makefile
>>   create mode 100644 drivers/crypto/stm32/stm32_crc32.c
>>
>> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
>> index 473d312..922b323 100644
>> --- a/drivers/crypto/Kconfig
>> +++ b/drivers/crypto/Kconfig
>> @@ -619,4 +619,6 @@ config CRYPTO_DEV_BCM_SPU
>>Secure Processing Unit (SPU). The SPU driver registers ablkcipher,
>>ahash, and aead algorithms with the kernel cryptographic API.
>>
>> +source "drivers/crypto/stm32/Kconfig"
>> +
>>   endif # CRYPTO_HW
>> diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
>> index 7396094..95bf2f9 100644
>> --- a/drivers/crypto/Makefile
>> +++ b/drivers/crypto/Makefile
>> @@ -30,6 +30,7 @@ obj-$(CONFIG_CRYPTO_DEV_QCE) += qce/
>>   obj-$(CONFIG_CRYPTO_DEV_ROCKCHIP) += rockchip/
>>   obj-$(CONFIG_CRYPTO_DEV_S5P) += s5p-sss.o
>>   obj-$(CONFIG_CRYPTO_DEV_SAHARA) += sahara.o
>> +obj-$(CONFIG_CRYPTO_DEV_STM32) += stm32/
>>   obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/
>>   obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o
>>   obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/
>> diff --git a/drivers/crypto/stm32/Kconfig b/drivers/crypto/stm32/Kconfig
>> new file mode 100644
>> index 000..792335b
>> --- /dev/null
>> +++ b/drivers/crypto/stm32/Kconfig
>> @@ -0,0 +1,8 @@
>> +config CRYPTO_DEV_STM32
>> +   tristate "Support for STM32 crypto accelerators"
>> +   depends on ARCH_STM32
>> +   select CRYPTO_HASH
>> +   help
>> +  This enables support for the CRC32 hw accelerator which can be 
>> found
>> + on STMicroelectronis STM32 SOC.
>> +
>> diff --git a/drivers/crypto/stm32/Makefile b/drivers/crypto/stm32/Makefile
>> new file mode 100644
>> index 000..73b4c6e
>> --- /dev/null
>> +++ b/drivers/crypto/stm32/Makefile
>> @@ -0,0 +1,2 @@
>> +obj-$(CONFIG_CRYPTO_DEV_STM32) += stm32_cryp.o
>> +stm32_cryp-objs := stm32_crc32.o
>> diff --git a/drivers/crypto/stm32/stm32_crc32.c 
>> b/drivers/crypto/stm32/stm32_crc32.c
>> new file mode 100644
>> index 000..7652822
>> --- /dev/null
>> +++ b/drivers/crypto/stm32/stm32_crc32.c
>> @@ -0,0 +1,324 @@
>> +/*
>> + * Copyright (C) STMicroelectronics SA 2017
>> + * Author: Fabien Dessenne 
>> + * License terms:  GNU General Public License (GPL), version 2
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +
>> +#include 
>> +
>> +#define DRIVER_NAME "stm32-crc32"
>> +#define CHKSUM_DIGEST_SIZE  4
>> +#define CHKSUM_BLOCK_SIZE   1
>> +
>> +/* Registers */
>> +#define CRC_DR  0x
>> +#define CRC_CR  0x0008
>> +#define CRC_INIT0x0010
>> +#define CRC_POL 0x0014
>> +
>> +/* Registers values */
>> +#define CRC_CR_RESETBIT(0)
>> +#define CRC_CR_REVERSE  (BIT(7) | BIT(6) | BIT(5))
>> +#define CRC_INIT_DEFAULT0x
>> +
>> +/* Polynomial reversed */
>> +#define POLY_CRC32  0xEDB88320
>> +#define POLY_CRC32C 0x82F63B78
>> +
>> +struct stm32_crc {
>> +   struct list_head list;
>> +   struct device*dev;
>> +   void __iomem *regs;
>> +   struct clk   *clk;
>> +   u8   pending_data[sizeof(u32)];
>> +   size_t   nb_pending_bytes;
>> +};
>> +
>> +struct stm32_crc_list {
>> +   struct list_head dev_list;
>> +   spinlock_t   

Re: [PATCH] padata: avoid race in reordering

[Steffen Klassert]

On Thu, Mar 23, 2017 at 12:24:43PM +0100, Jason A. Donenfeld wrote:
> Under extremely heavy uses of padata, crashes occur, and with list
> debugging turned on, this happens instead:
> 
> [87487.298728] WARNING: CPU: 1 PID: 882 at lib/list_debug.c:33
> __list_add+0xae/0x130
> [87487.301868] list_add corruption. prev->next should be next
> (b17abfc043d0), but was 8dba70872c80. (prev=8dba70872b00).
> [87487.339011]  [] dump_stack+0x68/0xa3
> [87487.342198]  [] ? console_unlock+0x281/0x6d0
> [87487.345364]  [] __warn+0xff/0x140
> [87487.348513]  [] warn_slowpath_fmt+0x4a/0x50
> [87487.351659]  [] __list_add+0xae/0x130
> [87487.354772]  [] ? _raw_spin_lock+0x64/0x70
> [87487.357915]  [] padata_reorder+0x1e6/0x420
> [87487.361084]  [] padata_do_serial+0xa5/0x120
> 
> padata_reorder calls list_add_tail with the list to which its adding
> locked, which seems correct:
> 
> spin_lock(>serial.lock);
> list_add_tail(>list, >serial.list);
> spin_unlock(>serial.lock);
> 
> This therefore leaves only place where such inconsistency could occur:
> if padata->list is added at the same time on two different threads.
> This pdata pointer comes from the function call to
> padata_get_next(pd), which has in it the following block:
> 
> next_queue = per_cpu_ptr(pd->pqueue, cpu);
> padata = NULL;
> reorder = _queue->reorder;
> if (!list_empty(>list)) {
>padata = list_entry(reorder->list.next,
>struct padata_priv, list);
>spin_lock(>lock);
>list_del_init(>list);
>atomic_dec(>reorder_objects);
>spin_unlock(>lock);
> 
>pd->processed++;
> 
>goto out;
> }
> out:
> return padata;
> 
> I strongly suspect that the problem here is that two threads can race
> on reorder list. Even though the deletion is locked, call to
> list_entry is not locked, which means it's feasible that two threads
> pick up the same padata object and subsequently call list_add_tail on
> them at the same time. The fix is thus be hoist that lock outside of
> that block.
> 
> Signed-off-by: Jason A. Donenfeld 

Acked-by: Steffen Klassert 

Re: [PATCH] arm64: dts: ls1012a: add crypto node

[Horia Geantă]

On 3/24/2017 9:35 AM, Shawn Guo wrote:
> On Fri, Mar 24, 2017 at 07:17:50AM +, Horia Geantă wrote:
 +  sec_mon: sec_mon@1e9 {
>>>
>>> Hyphen is more preferred to be used in node name than underscore.
>>>
>> This would imply changing the
>> Documentation/devicetree/bindings/crypto/fsl-sec4.txt binding and
>> dealing with all the consequences, which IIUC is probably not worth.
> 
> I do not care the bindings doc that much, since I'm not the maintainer
> of it.  What are the consequences specifically, if we use a better node
> name in dts than bindings example?
> 
Users relying on finding the sec_mon node will obviously stop working.
I don't see any in-kernel users, however there could be others I am not
aware of and DT bindings should provide for backwards compatibility.

I could deprecate "sec_mon" in the bindings and suggest "sec-mon"
instead, while leaving all existing dts files as-is.
The risk is breaking LS1012A users relying on "sec_mon".

I see that ePAPR:
-allows both for hyphen and underline in case of node names
-allows only for hyphen (i.e. forbids underline) in case of alias nodes

In the first case, I understand there's an (undocumented?) agreement to
prefer hyphen over underline.
For the 2nd one, does this mean I should change alias names?

Thanks,
Horia

Re: [PATCH] arm64: dts: ls1012a: add crypto node

[Shawn Guo]

On Fri, Mar 24, 2017 at 07:17:50AM +, Horia Geantă wrote:
> >> +  sec_mon: sec_mon@1e9 {
> > 
> > Hyphen is more preferred to be used in node name than underscore.
> > 
> This would imply changing the
> Documentation/devicetree/bindings/crypto/fsl-sec4.txt binding and
> dealing with all the consequences, which IIUC is probably not worth.

I do not care the bindings doc that much, since I'm not the maintainer
of it.  What are the consequences specifically, if we use a better node
name in dts than bindings example?

Shawn

Re: [PATCH] arm64: dts: ls1012a: add crypto node

[Horia Geantă]

On 3/24/2017 3:56 AM, Shawn Guo wrote:
> On Wed, Mar 22, 2017 at 02:29:39PM +0200, Horia Geantă wrote:
>> LS1012A has a SEC v5.4 security engine.
>>
>> Signed-off-by: Horia Geantă 
>> ---
>>  arch/arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts |  9 +++
>>  arch/arm64/boot/dts/freescale/fsl-ls1012a-qds.dts  |  9 +++
>>  arch/arm64/boot/dts/freescale/fsl-ls1012a-rdb.dts  |  9 +++
>>  arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi | 91 
>> +-
>>  4 files changed, 117 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts 
>> b/arch/arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
>> index a619f6496a4c..bab9e68947e4 100644
>> --- a/arch/arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
>> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
>> @@ -49,6 +49,15 @@
>>  model = "LS1012A Freedom Board";
>>  compatible = "fsl,ls1012a-frdm", "fsl,ls1012a";
>>  
>> +aliases {
>> +crypto = 
>> +rtic_a = _a;
>> +rtic_b = _b;
>> +rtic_c = _c;
>> +rtic_d = _d;
>> +sec_mon = _mon;
>> +};
>> +
>>  sys_mclk: clock-mclk {
>>  compatible = "fixed-clock";
>>  #clock-cells = <0>;
>> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1012a-qds.dts 
>> b/arch/arm64/boot/dts/freescale/fsl-ls1012a-qds.dts
>> index 14a67f1709e7..5c4e84c7f20d 100644
>> --- a/arch/arm64/boot/dts/freescale/fsl-ls1012a-qds.dts
>> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1012a-qds.dts
>> @@ -49,6 +49,15 @@
>>  model = "LS1012A QDS Board";
>>  compatible = "fsl,ls1012a-qds", "fsl,ls1012a";
>>  
>> +aliases {
>> +crypto = 
>> +rtic_a = _a;
>> +rtic_b = _b;
>> +rtic_c = _c;
>> +rtic_d = _d;
>> +sec_mon = _mon;
>> +};
>> +
>>  sys_mclk: clock-mclk {
>>  compatible = "fixed-clock";
>>  #clock-cells = <0>;
>> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1012a-rdb.dts 
>> b/arch/arm64/boot/dts/freescale/fsl-ls1012a-rdb.dts
>> index 62c5c7123a15..ff9dd16aa65a 100644
>> --- a/arch/arm64/boot/dts/freescale/fsl-ls1012a-rdb.dts
>> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1012a-rdb.dts
>> @@ -48,6 +48,15 @@
>>  / {
>>  model = "LS1012A RDB Board";
>>  compatible = "fsl,ls1012a-rdb", "fsl,ls1012a";
>> +
>> +aliases {
>> +crypto = 
>> +rtic_a = _a;
>> +rtic_b = _b;
>> +rtic_c = _c;
>> +rtic_d = _d;
>> +sec_mon = _mon;
>> +};
> 
> What are these aliases used for?  Are they board specific?  If not, we
> should probably have them in fsl-ls1012a.dtsi, since you are adding
> them for all three fsl-ls1012a based boards.
> 
Indeed, these can be shared and thus should be moved to
fsl-ls1012a.dtsi. Will be fixed in v2.

crypto alias is used in u-boot to fixup the crypto node with a
"fsl,sec-era" property.

rtic and sec_mon aliases have been added to be in line with the PPC
device trees, I am not aware how they are used.

>>  };
>>  
>>   {
>> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi 
>> b/arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi
>> index cffebb4b3df1..68f3012ae07e 100644
>> --- a/arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi
>> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1012a.dtsi
>> @@ -42,7 +42,7 @@
>>   * OTHER DEALINGS IN THE SOFTWARE.
>>   */
>>  
>> -#include 
>> +#include 
>>  
>>  / {
>>  compatible = "fsl,ls1012a";
>> @@ -113,6 +113,95 @@
>>  big-endian;
>>  };
>>  
>> +crypto: crypto@170 {
>> +compatible = "fsl,sec-v5.4", "fsl,sec-v5.0",
>> + "fsl,sec-v4.0";
>> +fsl,sec-era = <8>;
>> +#address-cells = <1>;
>> +#size-cells = <1>;
>> +ranges = <0x0 0x00 0x170 0x10>;
>> +reg = <0x00 0x170 0x0 0x10>;
>> +interrupts = ;
>> +
>> +sec_jr0: jr@1 {
>> +compatible = "fsl,sec-v5.4-job-ring",
>> + "fsl,sec-v5.0-job-ring",
>> + "fsl,sec-v4.0-job-ring";
>> +reg= <0x1 0x1>;
>> +interrupts = ;
>> +};
>> +
>> +sec_jr1: jr@2 {
>> +compatible = "fsl,sec-v5.4-job-ring",
>> + "fsl,sec-v5.0-job-ring",
>> + "fsl,sec-v4.0-job-ring";
>> +reg= <0x2 0x1>;
>> +interrupts = ;
>> +};
>> +
>> +sec_jr2: jr@3 {
>> +compatible =