Re: Q for a new API for the random device driver

[PrasannaKumar Muralidharan]

Hi Herald,

On 6 June 2018 at 18:18, Harald Freudenberger  wrote:
> Hello Theodore, hi Linux Community
>
> my patch for the s390 arch_get_random_seed* implementation is about to
> be integrated with the current merge window for kernel 4.18.
>
> So I'd like to start a discussion about a new API for the random.c
> device driver. The current s390 hardware comes with a true hardware
> random generator. This great random source provides high quality
> entropy (100% according to our hardware guys) but is relatively slow.
>
> I want to provide this entropy source to the random pool of the
> random device driver. The only possibility is the implementation of
> the arch_get_random* and arch_get_random_seed* API. So my first attempt
> was to implement the arch_get_random* functions and directly call the
> TRNG. This code made it into the 4.12 kernel and slowed down the
> /dev/urandom performance remarkable. So the first rework removed the
> s390 arch_get_random* implementation and instead provided the
> arch_*_seed functions. The idea was that your good but slow TRNG is
> ideal for providing entropy as seed for pseudo random
> generators. However, as arch_get_random_seed_long() is called very
> frequently in interrupt context and this limits the amount of irqs
> per cpu remarkable (e.g. on heavy network load). As a result my latest
> fix introduces some kind of buffer concept in combination with filling
> the buffer asynchronous with a cascaded TRNG/PRNG.
>
> I am still searching for a way to provide our good hardware entropy
> source to the random pool in the random device driver. So I'd like to
> have a new arch interface which is called when the random pool finds
> out that it is running out of entropy. My feeling is that it should
> not be the only source but should still be mixed with other sources
> of entropy. It should not be able to dominate or occupy the random
> pool but contribute to a significant part.
>
> As nowadays true random generators provide conditioned data usually
> with some kind of hashing algorithm a granularity of 4 or 8 bytes is
> waste of random entropy. The s390 TRNG uses SHA512 and can provide
> 64 bytes entropy with each invocation. Other TRNGs may use sha1 or
> sha256 and so provide 20 or 32 bytes of random. However, the API
> could be something like:
>
>   int arch_get_entropy(void *buf, int bufsize);
>
> and the function returns:
>
>   < 0 - error, negative errno value
>   0...bufsize - amount of entropy bytes written into the buffer
> may be 0 if there is (currently) no random entropy
> available. No need to fill the buffer, any data in the
> range of 0 up to bufsize is welcome.
>
> random.c could call this api with a buffer of 64 bytes when the
> computed entropy within the pool drops below a threshold limit.
>
> The call should not be done within a userspace process context or
> 'foreign' kernel context (like irqs) but with an own kernel thread
> or workqueue or similar. The data returned should be mixed into
> the pool and counted as entropy.
>
> I think, other architectures could also benefit from such a new
> interface. Power and even x86 have or will have true random entropy
> source hardware and may also like to contribute to the linux random
> device driver.
>
> Does this sound reasonable? If yes, I'll start some investigation and
> try to develop something for random.c. Though this may take some time.
>
> regards and thanks for your time
> Harald Freudenberger
>

hw_rng framework provides entropy to the kernel's entropy pool
already. I am wondering why hw_random interface can't be used for
this.

Regards,
PrasannaKumar

Re: [PATCH v3 3/4] crypto: jz4780-rng: Add RNG node to jz4780.dtsi

[PrasannaKumar Muralidharan]

Hi Rob,

On 6 March 2018 at 19:25, Rob Herring <robh...@kernel.org> wrote:
> On Tue, Mar 6, 2018 at 3:32 AM, James Hogan <jho...@kernel.org> wrote:
>> On Mon, Sep 18, 2017 at 07:32:40PM +0530, PrasannaKumar Muralidharan wrote:
>>> Add RNG node to jz4780 dtsi. This driver uses registers that are part of
>>> the register set used by Ingenic CGU driver. Use regmap in RNG driver to
>>> access its register. Create 'simple-bus' node, make CGU and RNG node as
>>> child of it so that both the nodes are visible without changing CGU
>>> driver code.
>
> The goal should be to avoid changing the DT (because the h/w hasn't
> changed), not avoid changing a driver.

Please have a look at the discussion happened at
https://patchwork.linux-mips.org/patch/14094/. Looks like there is a
difference in though process between you and mips folks. I am not an
expert in DT so please suggest me the correct way to go about this.

>>>
>>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>>
>> Better late than never:
>> Acked-by: James Hogan <jho...@kernel.org>
>>
>> (I presume its okay for the reg ranges to overlap, ISTR that being an
>> issue a few years ago, but maybe thats fixed now).
>
> No, that should be avoided. It does work because we have existing
> cases that have to be supported.

I am sorry but I require guidance here. Do you have any suggestion on
how this should be.

>>
>> Cheers
>> James

Thanks and regards,
PrasannaKumar

Re: [PATCH v3 3/4] crypto: jz4780-rng: Add RNG node to jz4780.dtsi

[PrasannaKumar Muralidharan]

Hi Paul,

On 7 March 2018 at 04:31, Paul Cercueil <p...@crapouillou.net> wrote:
> Le 2018-03-06 10:32, James Hogan a écrit :
>>
>> On Mon, Sep 18, 2017 at 07:32:40PM +0530, PrasannaKumar Muralidharan
>> wrote:
>>>
>>> Add RNG node to jz4780 dtsi. This driver uses registers that are part of
>>> the register set used by Ingenic CGU driver. Use regmap in RNG driver to
>>> access its register. Create 'simple-bus' node, make CGU and RNG node as
>>> child of it so that both the nodes are visible without changing CGU
>>> driver code.
>>>
>>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>>
>>
>> Better late than never:
>> Acked-by: James Hogan <jho...@kernel.org>
>>
>> (I presume its okay for the reg ranges to overlap, ISTR that being an
>> issue a few years ago, but maybe thats fixed now).
>>
>> Cheers
>> James
>
>
> What bothers me is that the CGU code has not been modified to use regmap, so
> the
> registers area is actually mapped twice (once in the CGU driver, once with
> regmap).

One of my previous versions changed CGU code to use regmap. I got a
review comment saying that is not required
(https://patchwork.kernel.org/patch/9906889/). The points in the
comment were valid so I reverted the change. Please have a look at the
discussion.

> Besides, regmap would be useful if the RNG registers were actually located
> in the
> middle of the register area used by the CGU driver, which is not the case
> here.
> The CGU block does have some registers after the RNG ones on the X1000 SoC,
> but
> I don't think they will ever be used (and if they are it won't be by the CGU
> driver).
>
> Regards,
> -Paul

Ingenic M200 SoC's CGU has clock and power related registers after the
RNG registers. Paul Burton suggested using regmap to expose registers
to CGU and RNG drivers
(https://patchwork.linux-mips.org/patch/14094/).

Regards,
PrasannaKumar

Re: [PATCH v2] tpm: Move Linux RNG connection to hwrng

[PrasannaKumar Muralidharan]

Hi Jarkko,

On 17 November 2017 at 19:27, Jarkko Sakkinen
 wrote:
> On Fri, Nov 17, 2017 at 03:28:53PM +0200, Jarkko Sakkinen wrote:
>
> At least signed-off-by from PrassanaKumar is missing from the 2nd
> commit. I'll add it.

I had the impression that my signed-off-by will be present in this
change. But it is missing in [1]. Is it supposed to be that way?

1. 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=6e592a065d51d26f9d62b8b7501a5114076af8b4

Thanks,
PrasannaKumar

Re: [PATCH v3] tpm: use struct tpm_chip for tpm_chip_find_get()

[PrasannaKumar Muralidharan]

Hi Jarkko,

On 18 January 2018 at 21:50, Jarkko Sakkinen
<jarkko.sakki...@linux.intel.com> wrote:
> On Wed, Jan 17, 2018 at 07:43:51PM +0530, PrasannaKumar Muralidharan wrote:
>> Hi Jarkko,
>>
>> On 14 November 2017 at 20:02, Jarkko Sakkinen
>> <jarkko.sakki...@linux.intel.com> wrote:
>> > On Sun, Nov 12, 2017 at 10:53:35AM +0530, PrasannaKumar Muralidharan wrote:
>> >> Did basic check on tpm rng patch, it works fine. As it depends on this
>> >> patch this should be working fine too.
>> >>
>> >> Tested-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> >>
>> >> Regards,
>> >> PrasannaKumar
>> >
>> > Thank you.
>> >
>> > /Jarkko
>>
>> Wondering what happened to this and tpm rng patch. Is there something
>> more to do for this to get merged?
>>
>> Thanks,
>> PrasannaKumar
>
> Was part of 4.6 PR.
>
> /Jarkko

Ah, I see.

PS: I think you meant 4.16.

Thanks,
PrasannaKumar

Re: [PATCH v3] tpm: use struct tpm_chip for tpm_chip_find_get()

[PrasannaKumar Muralidharan]

Hi Jarkko,

On 14 November 2017 at 20:02, Jarkko Sakkinen
<jarkko.sakki...@linux.intel.com> wrote:
> On Sun, Nov 12, 2017 at 10:53:35AM +0530, PrasannaKumar Muralidharan wrote:
>> Did basic check on tpm rng patch, it works fine. As it depends on this
>> patch this should be working fine too.
>>
>> Tested-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>>
>> Regards,
>> PrasannaKumar
>
> Thank you.
>
> /Jarkko

Wondering what happened to this and tpm rng patch. Is there something
more to do for this to get merged?

Thanks,
PrasannaKumar

Re: [PATCH] [RFT] crypto: aes-generic - turn off -ftree-pre and -ftree-sra

[PrasannaKumar Muralidharan]

Hi Ard,

On 21 December 2017 at 17:52, Ard Biesheuvel  wrote:
> On 21 December 2017 at 10:20, Arnd Bergmann  wrote:
>> On Wed, Dec 20, 2017 at 10:46 PM, Jakub Jelinek  wrote:
>>> On Wed, Dec 20, 2017 at 09:52:05PM +0100, Arnd Bergmann wrote:
 diff --git a/crypto/aes_generic.c b/crypto/aes_generic.c
 index ca554d57d01e..35f973ba9878 100644
 --- a/crypto/aes_generic.c
 +++ b/crypto/aes_generic.c
 @@ -1331,6 +1331,20 @@ EXPORT_SYMBOL_GPL(crypto_aes_set_key);
   f_rl(bo, bi, 3, k); \
  } while (0)

 +#if __GNUC__ >= 7
 +/*
 + * Newer compilers try to optimize integer arithmetic more aggressively,
 + * which generally improves code quality a lot, but in this specific case
 + * ends up hurting more than it helps, in some configurations drastically
 + * so. This turns off two optimization steps that have been shown to
 + * lead to rather badly optimized code with gcc-7.
 + *
 + * See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83356
 + */
 +#pragma GCC optimize("-fno-tree-pre")
 +#pragma GCC optimize("-fno-tree-sra")
>>>
>>> So do it only when UBSAN is enabled?  GCC doesn't have a particular
>>> predefined macro for those (only for asan and tsan), but either the kernel
>>> does have something already, or could have something added in the
>>> corresponding Makefile.
>>
>> My original interpretation of the resulting object code suggested that 
>> disabling
>> those two optimizations produced better results for this particular
>> file even without
>> UBSAN, on both gcc-7 and gcc-8 (but not gcc-6), so my patch might have
>> been better, but I did some measurements now as Ard suggested, showing
>> cycles/byte for AES256/CBC with 8KB blocks:
>>
>>
>>default  ubsan patchedpatched+ubsan
>> gcc-4.3.614.9   14.9 
>> gcc-4.6.415.0   15.8 
>> gcc-4.9.415.520.7   15.9 20.9
>> gcc-5.5.015.647.3   86.4 48.8
>> gcc-6.3.114.649.4   94.3 50.9
>> gcc-7.1.113.554.6   15.2 52.0
>> gcc-7.2.116.8   124.7   92.0 52.2
>> gcc-8.0.015.0  no boot  15.3no boot
>>
>> I checked that there are actually three significant digits on the 
>> measurements,
>> detailed output is available at https://pastebin.com/eFsWYjQp
>>
>> It seems that I was wrong about the interpretation that disabling
>> the optimization would be a win on gcc-7 and higher, it almost
>> always makes things worse even with UBSAN. Making that
>> check "#if __GNUC__ == 7 && IS_ENABLED(CONFIG_UBSAN_SANITIZE_ALL)"
>> would help here, or we could list the file as an exception for
>> UBSAN and never sanitize it.
>>
>> Looking at the 'default' column, I wonder if anyone would be interested
>> in looking at why the throughput regressed with gcc-7.2 and gcc-8.
>>
>
> Thanks for the elaborate benchmarks. Looking at the bugzilla entry, it
> appears the UBSAN code inserts runtime checks to ensure that certain
> u8 variables don't assume values <0 or >255, which seems like a rather
> pointless exercise to me. But even if it didn't, I think it is
> justified to disable UBSAN on all of the low-level cipher
> implementations, given that they are hardly ever modified, and
> typically don't suffer from the issues UBSAN tries to identify.
>
> So my vote is to disable UBSAN for all such cipher implementations:
> aes_generic, but also aes_ti, which has a similar 256 byte lookup
> table [although it does not seem to be affected by the same issue as
> aes_generic], and possibly others as well.
>
> Perhaps it makes sense to move core cipher code into a separate
> sub-directory, and disable UBSAN at the directory level?
>
> It would involve the following files
>
> crypto/aes_generic.c
> crypto/aes_ti.c
> crypto/anubis.c
> crypto/arc4.c
> crypto/blowfish_generic.c
> crypto/camellia_generic.c
> crypto/cast5_generic.c
> crypto/cast6_generic.c
> crypto/des_generic.c
> crypto/fcrypt.c
> crypto/khazad.c
> crypto/seed.c
> crypto/serpent_generic.c
> crypto/tea.c
> crypto/twofish_generic.c

As *SAN is enabled only on developer setup, is such a change required?
Looks like I am missing something here. Can you explain what value it
provides?

Regards,
PrasannaKumar

Re: [PATCH v2] hwrng: Clean up RNG list when last hwrng is unregistered

[PrasannaKumar Muralidharan]

On 17 December 2017 at 14:53, PrasannaKumar Muralidharan
<prasannatsmku...@gmail.com> wrote:
> Hi Gary,
>
> Some minor comments below.
>
> On 16 December 2017 at 01:25, Gary R Hook <gary.h...@amd.com> wrote:
>>
>> Commit 142a27f0a731 added support for a "best" RNG, and in doing so
>> introduced a hang from rmmod/modprobe -r when the last RNG on the list
>> was unloaded.
>
> Nice catch. Thanks for fixing this.
>
>> When the hwrng list is depleted, return the global variables to their
>> original state and decrement all references to the object.
>>
>> Fixes: 142a27f0a731 ("hwrng: core - Reset user selected rng by writing "" to 
>> rng_current")
>
> Please cc the commit author (in this case its me) so that this patch
> gets noticed easily.
>
>> Signed-off-by: Gary R Hook <gary.h...@amd.com>
>> ---
>>
>> Changes since v1: fix misspelled word in subject
>>
>>  drivers/char/hw_random/core.c |4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
>> index 657b8770b6b9..91bb98c42a1c 100644
>> --- a/drivers/char/hw_random/core.c
>> +++ b/drivers/char/hw_random/core.c
>> @@ -306,6 +306,10 @@ static int enable_best_rng(void)
>> ret = ((new_rng == current_rng) ? 0 : 
>> set_current_rng(new_rng));
>> if (!ret)
>> cur_rng_set_by_user = 0;
>> +   } else {
>> +   drop_current_rng();
>
> When the hwrng list is empty just set current_rng = NULL instead of
> calling drop_current_rng().
>
>> +   cur_rng_set_by_user = 0;
>> +   ret = 0;
>> }
>>
>> return ret;
>>
>
> Regards,
> PrasannaKumar

I am fine with the code as is.

Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Regards,
PrasannaKumar

Re: [PATCH v2] hwrng: Clean up RNG list when last hwrng is unregistered

[PrasannaKumar Muralidharan]

Hi Gary,

Some minor comments below.

On 16 December 2017 at 01:25, Gary R Hook  wrote:
>
> Commit 142a27f0a731 added support for a "best" RNG, and in doing so
> introduced a hang from rmmod/modprobe -r when the last RNG on the list
> was unloaded.

Nice catch. Thanks for fixing this.

> When the hwrng list is depleted, return the global variables to their
> original state and decrement all references to the object.
>
> Fixes: 142a27f0a731 ("hwrng: core - Reset user selected rng by writing "" to 
> rng_current")

Please cc the commit author (in this case its me) so that this patch
gets noticed easily.

> Signed-off-by: Gary R Hook 
> ---
>
> Changes since v1: fix misspelled word in subject
>
>  drivers/char/hw_random/core.c |4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 657b8770b6b9..91bb98c42a1c 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -306,6 +306,10 @@ static int enable_best_rng(void)
> ret = ((new_rng == current_rng) ? 0 : 
> set_current_rng(new_rng));
> if (!ret)
> cur_rng_set_by_user = 0;
> +   } else {
> +   drop_current_rng();

When the hwrng list is empty just set current_rng = NULL instead of
calling drop_current_rng().

> +   cur_rng_set_by_user = 0;
> +   ret = 0;
> }
>
> return ret;
>

Regards,
PrasannaKumar

Re: [PATCH v2 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

[PrasannaKumar Muralidharan]

;
> +
> +   return 0;
> +}
> +
> +static int __maybe_unused exynos_trng_suspend(struct device *dev)
> +{
> +   pm_runtime_put_sync(dev);
> +
> +   return 0;
> +}
> +
> +static int __maybe_unused exynos_trng_resume(struct device *dev)
> +{
> +   int ret;
> +
> +   ret = pm_runtime_get_sync(dev);
> +   if (ret < 0) {
> +   dev_err(dev, "Could not get runtime PM.\n");
> +   pm_runtime_put_noidle(dev);
> +   return ret;
> +   }
> +
> +   return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(exynos_trng_pm_ops, exynos_trng_suspend,
> +exynos_trng_resume);
> +
> +static const struct of_device_id exynos_trng_dt_match[] = {
> +   {
> +   .compatible = "samsung,exynos5250-trng",
> +   },
> +   { },
> +};
> +MODULE_DEVICE_TABLE(of, exynos_rng_dt_match);
> +
> +static struct platform_driver exynos_trng_driver = {
> +   .driver = {
> +   .name = "exynos-trng",
> +   .pm = _trng_pm_ops,
> +   .of_match_table = exynos_trng_dt_match,
> +   },
> +   .probe = exynos_trng_probe,
> +   .remove = exynos_trng_remove,
> +};
> +
> +module_platform_driver(exynos_trng_driver);
> +MODULE_AUTHOR("Łukasz Stelmach");
> +MODULE_DESCRIPTION("H/W TRNG driver for Exynos chips");
> +MODULE_LICENSE("GPL");
> --
> 2.11.0
>

Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Regards,
PrasannaKumar

Re: [PATCH v3 0/4] crypto: Add driver for JZ4780 PRNG

[PrasannaKumar Muralidharan]

Hi Ralf,

On 12 October 2017 at 22:59, PrasannaKumar Muralidharan
<prasannatsmku...@gmail.com> wrote:
> Hi Herbert,
>
> On 12 October 2017 at 20:00, Herbert Xu <herb...@gondor.apana.org.au> wrote:
>> On Mon, Sep 18, 2017 at 07:32:37PM +0530, PrasannaKumar Muralidharan wrote:
>>> This patch series adds support of pseudo random number generator found
>>> in Ingenic's JZ4780 and X1000 SoC.
>>>
>>> Create cgublock node which has CGU and RNG node as its children. The
>>> cgublock node uses "simple-bus" compatible which helps in exposing CGU
>>> and RNG nodes without changing CGU driver. Add 'syscon' compatible in
>>> CGU node in jz4780.dtsi. The jz4780-rng driver uses regmap exposed via
>>> syscon interface to access the RNG registers. CGU driver is not
>>> modified in this patch set as registers used by CGU driver and this
>>> driver are different.
>>>
>>> PrasannaKumar Muralidharan (4):
>>>   crypto: jz4780-rng: Add JZ4780 PRNG devicetree binding documentation
>>>   crypto: jz4780-rng: Add Ingenic JZ4780 hardware PRNG driver
>>>   crypto: jz4780-rng: Add RNG node to jz4780.dtsi
>>>   crypto: jz4780-rng: Enable PRNG support in CI20 defconfig
>>
>> Please indicate which patches are intended to go through the crypto
>> trees.
>
> From https://patchwork.linux-mips.org/patch/17162/ I expect the same.
> Either all patches go via crypto tree or via mips tree.
> The dtsi changes is not yet acked by MIPS / JZ4780 maintainer. Let's
> wait for it.
>
> Thanks,
> PrasannaKumar

Should I do anything more for this series?

Thanks,
PrasannaKumar

Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

[PrasannaKumar Muralidharan]

On 24 November 2017 at 20:55, PrasannaKumar Muralidharan
<prasannatsmku...@gmail.com> wrote:
> Hi Lukasz,
>
> Some minor comments below.
>
> On 23 November 2017 at 20:39, Łukasz Stelmach <l.stelm...@samsung.com> wrote:
>> Add support for True Random Number Generator found in Samsung Exynos
>> 5250+ SoCs.
>>
>> Signed-off-by: Łukasz Stelmach <l.stelm...@samsung.com>
>> ---
>>  MAINTAINERS  |   7 +
>>  drivers/char/hw_random/Kconfig   |  12 ++
>>  drivers/char/hw_random/Makefile  |   1 +
>>  drivers/char/hw_random/exynos-trng.c | 256 
>> +++
>>  4 files changed, 276 insertions(+)
>>  create mode 100644 drivers/char/hw_random/exynos-trng.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 2811a211632c..992074cca612 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11780,6 +11780,13 @@ S: Maintained
>>  F: drivers/crypto/exynos-rng.c
>>  F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
>>
>> +SAMSUNG EXYNOS TRUE RANDOM NUMBER GENERATOR (TRNG) DRIVER
>> +M: Łukasz Stelmach <l.stelm...@samsung.com>
>> +L: linux-samsung-...@vger.kernel.org
>> +S: Maintained
>> +F: drivers/char/hw_random/exynos-trng.c
>> +F: Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
>> +
>>  SAMSUNG FRAMEBUFFER DRIVER
>>  M: Jingoo Han <jingooh...@gmail.com>
>>  L: linux-fb...@vger.kernel.org
>> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
>> index 95a031e9eced..a788ac07081b 100644
>> --- a/drivers/char/hw_random/Kconfig
>> +++ b/drivers/char/hw_random/Kconfig
>> @@ -449,6 +449,18 @@ config HW_RANDOM_S390
>>
>>   If unsure, say Y.
>>
>> +config HW_RANDOM_EXYNOS
>> +   tristate "Samsung Exynos True Random Number Generator support"
>> +   depends on ARCH_EXYNOS || COMPILE_TEST
>> +   default HW_RANDOM
>> +   ---help---
>> + This driver provides support for the True Random Number
>> + Generator available in Exynos SoCs.
>> +
>> +To compile this driver as a module, choose M here: the module
>> +will be called exynos-trng.
>> +
>> +If unsure, say Y.
>>  endif # HW_RANDOM
>>
>>  config UML_RANDOM
>> diff --git a/drivers/char/hw_random/Makefile 
>> b/drivers/char/hw_random/Makefile
>> index f3728d008fff..5595df97da7a 100644
>> --- a/drivers/char/hw_random/Makefile
>> +++ b/drivers/char/hw_random/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
>>  obj-$(CONFIG_HW_RANDOM_N2RNG) += n2-rng.o
>>  n2-rng-y := n2-drv.o n2-asm.o
>>  obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
>> +obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-trng.o
>>  obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
>>  obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
>>  obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
>> diff --git a/drivers/char/hw_random/exynos-trng.c 
>> b/drivers/char/hw_random/exynos-trng.c
>> new file mode 100644
>> index ..340e106cae24
>> --- /dev/null
>> +++ b/drivers/char/hw_random/exynos-trng.c
>> @@ -0,0 +1,256 @@
>> +/*
>> + * RNG driver for Exynos TRNGs
>> + *
>> + * Author: Łukasz Stelmach <l.stelm...@samsung.com>
>> + *
>> + * Copyright 2017 (c) Samsung Electronics Software, Inc.
>> + *
>> + * Based on the Exynos PRNG driver drivers/crypto/exynos-rng by
>> + * Krzysztof Kozłowski <k...@kernel.org>
>> + *
>> + * 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.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define EXYNOS_TRNG_CLKDIV (0x0)
>> +#define EXYNOS_TRNG_CTRL   (0x20)
>> +#define EXYNOS_TRNG_POST_CTRL  (0x30)
>> +#define EXYNOS_TRNG_ONLINE_CTRL(0x40)
>> +#define EXYNOS_TRNG_ONLINE_STAT(0x44)
>> +#define EXY

Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver

[PrasannaKumar Muralidharan]

Hi Lukasz,

Some minor comments below.

On 23 November 2017 at 20:39, Łukasz Stelmach  wrote:
> Add support for True Random Number Generator found in Samsung Exynos
> 5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach 
> ---
>  MAINTAINERS  |   7 +
>  drivers/char/hw_random/Kconfig   |  12 ++
>  drivers/char/hw_random/Makefile  |   1 +
>  drivers/char/hw_random/exynos-trng.c | 256 
> +++
>  4 files changed, 276 insertions(+)
>  create mode 100644 drivers/char/hw_random/exynos-trng.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2811a211632c..992074cca612 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11780,6 +11780,13 @@ S: Maintained
>  F: drivers/crypto/exynos-rng.c
>  F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
>
> +SAMSUNG EXYNOS TRUE RANDOM NUMBER GENERATOR (TRNG) DRIVER
> +M: Łukasz Stelmach 
> +L: linux-samsung-...@vger.kernel.org
> +S: Maintained
> +F: drivers/char/hw_random/exynos-trng.c
> +F: Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.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 95a031e9eced..a788ac07081b 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -449,6 +449,18 @@ config HW_RANDOM_S390
>
>   If unsure, say Y.
>
> +config HW_RANDOM_EXYNOS
> +   tristate "Samsung Exynos True Random Number Generator support"
> +   depends on ARCH_EXYNOS || COMPILE_TEST
> +   default HW_RANDOM
> +   ---help---
> + This driver provides support for the True Random Number
> + Generator available in Exynos SoCs.
> +
> +To compile this driver as a module, choose M here: the module
> +will be called exynos-trng.
> +
> +If unsure, say Y.
>  endif # HW_RANDOM
>
>  config UML_RANDOM
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index f3728d008fff..5595df97da7a 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
>  obj-$(CONFIG_HW_RANDOM_N2RNG) += n2-rng.o
>  n2-rng-y := n2-drv.o n2-asm.o
>  obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
> +obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-trng.o
>  obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
>  obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
>  obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
> diff --git a/drivers/char/hw_random/exynos-trng.c 
> b/drivers/char/hw_random/exynos-trng.c
> new file mode 100644
> index ..340e106cae24
> --- /dev/null
> +++ b/drivers/char/hw_random/exynos-trng.c
> @@ -0,0 +1,256 @@
> +/*
> + * RNG driver for Exynos TRNGs
> + *
> + * Author: Łukasz Stelmach 
> + *
> + * Copyright 2017 (c) Samsung Electronics Software, Inc.
> + *
> + * Based on the Exynos PRNG driver drivers/crypto/exynos-rng by
> + * Krzysztof Kozłowski 
> + *
> + * 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.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define EXYNOS_TRNG_CLKDIV (0x0)
> +#define EXYNOS_TRNG_CTRL   (0x20)
> +#define EXYNOS_TRNG_POST_CTRL  (0x30)
> +#define EXYNOS_TRNG_ONLINE_CTRL(0x40)
> +#define EXYNOS_TRNG_ONLINE_STAT(0x44)
> +#define EXYNOS_TRNG_ONLINE_MAXCHI2 (0x48)
> +#define EXYNOS_TRNG_FIFO_CTRL  (0x50)
> +#define EXYNOS_TRNG_FIFO_0 (0x80)
> +#define EXYNOS_TRNG_FIFO_1 (0x84)
> +#define EXYNOS_TRNG_FIFO_2 (0x88)
> +#define EXYNOS_TRNG_FIFO_3 (0x8c)
> +#define EXYNOS_TRNG_FIFO_4 (0x90)
> +#define EXYNOS_TRNG_FIFO_5 (0x94)
> +#define EXYNOS_TRNG_FIFO_6 (0x98)
> +#define EXYNOS_TRNG_FIFO_7 (0x9c)
> +#define EXYNOS_TRNG_FIFO_LEN   (8)
> +#define EXYNOS_TRNG_CLOCK_RATE (50)
> +
> +struct exynos_trng_dev {
> +   struct device*dev;
> +   void __iomem *mem;
> +   struct clk   *clk;
> +   struct hwrng rng;
> +};
> +
> +struct exynos_trng_dev *exynos_trng_dev;
> +
> +static inline void exynos_trng_set_reg(struct exynos_trng_dev *trng, u16 reg,
> +  u32 val)
> +{
> +   /* Check range of reg? */
> +   __raw_writel(val, trng->mem + 

Re: [PATCH] crypto: hifn_795x - Fix a memory leak in the error handling path of 'hifn_probe()'

[PrasannaKumar Muralidharan]

Hi Christophe,

On 18 November 2017 at 19:15, Christophe JAILLET
<christophe.jail...@wanadoo.fr> wrote:
> 'dev' is leaking in the error handling path of 'hifn_probe()'.
>
> Add a 'kfree(dev)' to match the code in 'hifn_remove()'
>
> Signed-off-by: Christophe JAILLET <christophe.jail...@wanadoo.fr>
> ---
>  drivers/crypto/hifn_795x.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/crypto/hifn_795x.c b/drivers/crypto/hifn_795x.c
> index e09d4055b19e..a5a36fe7bf2c 100644
> --- a/drivers/crypto/hifn_795x.c
> +++ b/drivers/crypto/hifn_795x.c
> @@ -2579,6 +2579,7 @@ static int hifn_probe(struct pci_dev *pdev, const 
> struct pci_device_id *id)
> for (i = 0; i < 3; ++i)
> if (dev->bar[i])
> iounmap(dev->bar[i]);
> +   kfree(dev);
>
>  err_out_free_regions:
>     pci_release_regions(pdev);
> --
> 2.14.1
>

Nice catch.

Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Regards,
PrasannaKumar

Re: [PATCH] Fix NULL pointer deref. on no default_rng

[PrasannaKumar Muralidharan]

Hi Pierre,

On 12 November 2017 at 19:54, Pierre Ducroquet <pina...@pinaraf.info> wrote:
> If crypto_get_default_rng returns an error, the
> function ecc_gen_privkey should return an error.
> Instead, it currently tries to use the default_rng
> nevertheless, thus creating a kernel panic with a
> NULL pointer dereference.
> Returning the error directly, as was supposedly
> intended when looking at the code, fixes this.
>
> Signed-off-by: Pierre Ducroquet <pina...@pinaraf.info>
> ---
>  crypto/ecc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/crypto/ecc.c b/crypto/ecc.c
> index 633a9bcdc574..18f32f2a5e1c 100644
> --- a/crypto/ecc.c
> +++ b/crypto/ecc.c
> @@ -964,7 +964,7 @@ int ecc_gen_privkey(unsigned int curve_id, unsigned int 
> ndigits, u64 *privkey)
>  * DRBG with a security strength of 256.
>  */
> if (crypto_get_default_rng())
> -   err = -EFAULT;
> +   return -EFAULT;
>
> err = crypto_rng_get_bytes(crypto_default_rng, (u8 *)priv, nbytes);
> crypto_put_default_rng();
> --
> 2.15.0
>

Looks good to me.

Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Regards,
PrasannaKumar

Re: PATCH : Fix NULL pointer dereference on no default_rng

[PrasannaKumar Muralidharan]

Would you mind sending the patch using 'git send-email'? Kernel
community does not accept attachments.

Regards,
PrasannaKumar

Re: [PATCH v2] tpm: Move Linux RNG connection to hwrng

[PrasannaKumar Muralidharan]

Hi Jason,

On 9 November 2017 at 21:59, Jason Gunthorpe <j...@ziepe.ca> wrote:
> On Thu, Nov 09, 2017 at 09:49:33PM +0530, PrasannaKumar Muralidharan wrote:
>> Hi Jason,
>>
>> On 7 November 2017 at 21:34, Jason Gunthorpe <j...@ziepe.ca> wrote:
>> > On Tue, Nov 07, 2017 at 08:50:44AM +0530, PrasannaKumar Muralidharan wrote:
>> >
>> >> I am assuming you are talking about the following patches - using
>> >> struct tpm_chip instead of chip number and this patch.
>> >
>> > yes
>> >
>> >> I won't be able to test if struct tpm_chip usage as I don't have
>> >> multiple tpm hw in one machine. In case of tpm rng changes I can test
>> >> only the lifecycle of tpm rng device. Is that enough? I feel my test
>> >> will be limited. Please provide your thoughts on this.
>> >
>> > That is certainly better than no testing.
>>
>> The struct tpm_chip patch partially applied on linux next. I had to
>> manually change the code. In qemu tpm rng device did not show up on
>> loading tpm module. My laptop has tpm hw but Linux next did not work
>> properly in that. All my console were getting spammed with some USB
>> log message and I could not do anything. X did not start either. I
>> could not debug the issue as the logs were printing infinitely. Will
>> get little more time this weekend. Will do a proper test and provide
>> you the result.
>
> Test against 4.15-rc, here are the two patches
>
> https://github.com/jgunthorpe/linux/tree/tpm
>
> Pull from here and merge the latest rc and you will probably have a
> bootable system.
>
> Jason

Applied this patch on v4.14-rc4. Able to get data from tpm rng
(/dev/hwrng with tpm as the chosen rng). This patch works fine. Its
just a basic test though.

Tested-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Regards,
PrasannaKumar

Re: [PATCH v3] tpm: use struct tpm_chip for tpm_chip_find_get()

[PrasannaKumar Muralidharan]

Did basic check on tpm rng patch, it works fine. As it depends on this
patch this should be working fine too.

Tested-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

Regards,
PrasannaKumar

Re: [PATCH v2] tpm: Move Linux RNG connection to hwrng

[PrasannaKumar Muralidharan]

Hi Jason,

On 7 November 2017 at 21:34, Jason Gunthorpe <j...@ziepe.ca> wrote:
> On Tue, Nov 07, 2017 at 08:50:44AM +0530, PrasannaKumar Muralidharan wrote:
>
>> I am assuming you are talking about the following patches - using
>> struct tpm_chip instead of chip number and this patch.
>
> yes
>
>> I won't be able to test if struct tpm_chip usage as I don't have
>> multiple tpm hw in one machine. In case of tpm rng changes I can test
>> only the lifecycle of tpm rng device. Is that enough? I feel my test
>> will be limited. Please provide your thoughts on this.
>
> That is certainly better than no testing.

The struct tpm_chip patch partially applied on linux next. I had to
manually change the code. In qemu tpm rng device did not show up on
loading tpm module. My laptop has tpm hw but Linux next did not work
properly in that. All my console were getting spammed with some USB
log message and I could not do anything. X did not start either. I
could not debug the issue as the logs were printing infinitely. Will
get little more time this weekend. Will do a proper test and provide
you the result.

Regards,
PrasannaKumar

Re: [PATCH v2] tpm: Move Linux RNG connection to hwrng

[PrasannaKumar Muralidharan]

Hi Jason,

On 6 November 2017 at 07:57, Jason Gunthorpe  wrote:
> On Sun, Nov 05, 2017 at 01:05:06PM +0200, Jarkko Sakkinen wrote:
>
>> I asked to create a series for a reason. Now this doesn't apply because I
>> don't have an ancestor in my git history.
>
> It would be unusual for me to put your patch into a series unless I am
> also adopting it. eg what happens if there are more comments on it?
>
> Also, I wasn't sure what branch your patch was against since my tree
> didn't have history for it either..
>
> Sometimes the maintainer has to sort stuff like this out... :)
>
>> Please resend as series together with my patch. I can apply neither yet
>> because they have zero tested-by's.
>
> Hopefully PrasannaKumar can test both patches.

I am assuming you are talking about the following patches - using
struct tpm_chip instead of chip number and this patch.

I won't be able to test if struct tpm_chip usage as I don't have
multiple tpm hw in one machine. In case of tpm rng changes I can test
only the lifecycle of tpm rng device. Is that enough? I feel my test
will be limited. Please provide your thoughts on this.

Regards,
PrasannaKumar

Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume

[PrasannaKumar Muralidharan]

Hi Herbert,

On 6 November 2017 at 12:39, Herbert Xu  wrote:
> On Fri, Nov 03, 2017 at 09:57:21AM +, Jim Quigley wrote:
>> moved the call to hwrng_register() out of the probe routine into the scan
>> routine. We need to call hwrng_register() after a suspend/restore cycle
>> to re-register the device, but the scan function is not invoked for the
>> restore. Add the call to hwrng_register() to virtio_restore().
>
> Patch applied.  Thanks.

My rb tag is missing in the commit.

Regards,
PrasannaKumar

Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume

[PrasannaKumar Muralidharan]

Hi Jim,

Have second thoughts on this.

On 3 November 2017 at 20:55, PrasannaKumar Muralidharan
<prasannatsmku...@gmail.com> wrote:
>>
>> It would be cleaner to just get rid of probe_common() altogether in that
>> case, and do whatever
>> needs to be done in virtrng_probe()/virtrng_restore() respectively, but
>
> That's correct.
>
>> I didn't want to change code
>> affecting the normal probe path as well as suspend/resume. Is it OK to
>> leave it that way to avoid
>> the more extensive changes ?
>
> Personally I would prefer to do the cleaner way. It is up to the
> virtio and hwrng maintainer.

You are trying to restore the status quo of the driver that was before
the commit 5c06273401. It is really important and valid. The driver's
suspend/resume code is not in best shape but that is not what you are
trying to solve. I am fine with this change.

Regards,
PrasannaKumar

Fwd: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume

[PrasannaKumar Muralidharan]

Did reply instead of reply all. Forwarding my previous message.

-- Forwarded message --
From: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
Date: 3 November 2017 at 20:19
Subject: Re: virtio:rng: Virtio RNG devices need to be re-registered
after suspend/resume
To: Jim Quigley <jim.quig...@oracle.com>


Hi Jim,

On 3 November 2017 at 20:11, Jim Quigley <jim.quig...@oracle.com> wrote:
>
>
> On 03/11/2017 13:06, PrasannaKumar Muralidharan wrote:
>>
>> Hi Jim,
>>
>> On 3 November 2017 at 15:27, Jim Quigley <jim.quig...@oracle.com> wrote:
>>>
>>> The patch for
>>>
>>> commit: 5c06273401f2eb7b290cadbae18ee00f8f65e893
>>> Author: Amit Shah <amit.s...@redhat.com>
>>> Date:   Sun Jul 27 07:34:01 2014 +0930
>>>
>>>  virtio: rng: delay hwrng_register() till driver is ready
>>>
>>> moved the call to hwrng_register() out of the probe routine into the scan
>>> routine. We need to call hwrng_register() after a suspend/restore cycle
>>> to re-register the device, but the scan function is not invoked for the
>>> restore. Add the call to hwrng_register() to virtio_restore().
>>>
>>> Reviewed-by: Liam Merwick <liam.merw...@oracle.com>
>>> Signed-off-by: Jim Quigley <jim.quig...@oracle.com>
>>> ---
>>>   drivers/char/hw_random/virtio-rng.c | 21 -
>>>   1 file changed, 20 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/char/hw_random/virtio-rng.c
>>> b/drivers/char/hw_random/virtio-rng.c
>>> index 3fa2f8a..b89df66 100644
>>> --- a/drivers/char/hw_random/virtio-rng.c
>>> +++ b/drivers/char/hw_random/virtio-rng.c
>>> @@ -184,7 +184,26 @@ static int virtrng_freeze(struct virtio_device
>>> *vdev)
>>>
>>>   static int virtrng_restore(struct virtio_device *vdev)
>>>   {
>>> -   return probe_common(vdev);
>>> +   int err;
>>> +
>>> +   err = probe_common(vdev);
>>> +   if (!err) {
>>> +   struct virtrng_info *vi = vdev->priv;
>>> +
>>> +   /*
>>> +* Set hwrng_removed to ensure that virtio_read()
>>> +* does not block waiting for data before the
>>> +* registration is complete.
>>> +*/
>>> +   vi->hwrng_removed = true;
>>> +   err = hwrng_register(>hwrng);
>>> +   if (!err) {
>>> +   vi->hwrng_register_done = true;
>>> +   vi->hwrng_removed = false;
>>> +   }
>>> +   }
>>> +
>>> +   return err;
>>>   }
>>>   #endif
>>>
>>> --
>>> 1.8.3.1
>>>
>> This patch makes me wonder why hwrng_unregister is required in
>> virtrng_freeze. Looks strange and unusual. May be that is not required
>> and it can be removed. If it is required can you please add a comment
>> on why it is required?
>
>
> The reason it's required is because the virtrng_restore() uses
> probe_common() which allocates
> a new virtrng_info struct, changing  the devices private pointer . This
> virtrng struct is used in
> hwrng_register() to set the current RNG etc.  If we don't
> unregister/re-register then we would
> need to split probe_common() to avoid
>
> vi = kzalloc(sizeof(struct virtrng_info), GFP_KERNEL);
>
> overwriting vdev->priv on a restore.

True. As restore uses probe_common calling hwrng_register is required.

But I think it is not correct way to do.

>
> It would be cleaner to just get rid of probe_common() altogether in that
> case, and do whatever
> needs to be done in virtrng_probe()/virtrng_restore() respectively, but

That's correct.

> I didn't want to change code
> affecting the normal probe path as well as suspend/resume. Is it OK to
> leave it that way to avoid
> the more extensive changes ?

Personally I would prefer to do the cleaner way. It is up to the
virtio and hwrng maintainer.

Regards,
PrasannaKumar

Re: virtio:rng: Virtio RNG devices need to be re-registered after suspend/resume

[PrasannaKumar Muralidharan]

Hi Jim,

On 3 November 2017 at 15:27, Jim Quigley  wrote:
> The patch for
>
> commit: 5c06273401f2eb7b290cadbae18ee00f8f65e893
> Author: Amit Shah 
> Date:   Sun Jul 27 07:34:01 2014 +0930
>
> virtio: rng: delay hwrng_register() till driver is ready
>
> moved the call to hwrng_register() out of the probe routine into the scan
> routine. We need to call hwrng_register() after a suspend/restore cycle
> to re-register the device, but the scan function is not invoked for the
> restore. Add the call to hwrng_register() to virtio_restore().
>
> Reviewed-by: Liam Merwick 
> Signed-off-by: Jim Quigley 
> ---
>  drivers/char/hw_random/virtio-rng.c | 21 -
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/hw_random/virtio-rng.c 
> b/drivers/char/hw_random/virtio-rng.c
> index 3fa2f8a..b89df66 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -184,7 +184,26 @@ static int virtrng_freeze(struct virtio_device *vdev)
>
>  static int virtrng_restore(struct virtio_device *vdev)
>  {
> -   return probe_common(vdev);
> +   int err;
> +
> +   err = probe_common(vdev);
> +   if (!err) {
> +   struct virtrng_info *vi = vdev->priv;
> +
> +   /*
> +* Set hwrng_removed to ensure that virtio_read()
> +* does not block waiting for data before the
> +* registration is complete.
> +*/
> +   vi->hwrng_removed = true;
> +   err = hwrng_register(>hwrng);
> +   if (!err) {
> +   vi->hwrng_register_done = true;
> +   vi->hwrng_removed = false;
> +   }
> +   }
> +
> +   return err;
>  }
>  #endif
>
> --
> 1.8.3.1
>

This patch makes me wonder why hwrng_unregister is required in
virtrng_freeze. Looks strange and unusual. May be that is not required
and it can be removed. If it is required can you please add a comment
on why it is required?

Thanks,
PrasannaKumar

Re: [PATCH] hw_random: core: Reset user selected rng by writing "" to rng_current

[PrasannaKumar Muralidharan]

Hi Harald,

On 30 October 2017 at 18:58, Harald Freudenberger
<fre...@linux.vnet.ibm.com> wrote:
> On 10/27/2017 07:04 PM, PrasannaKumar Muralidharan wrote:
>> User is able to select a chosen rng by writing its name to rng_current
>> but there is no way to reset it without unbinding the rng. Let user
>> write "" to rng_current and delesect the chosen rng.
>>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> ---
>>  drivers/char/hw_random/core.c | 51 
>> +++
>>  1 file changed, 32 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
>> index 9701ac7..be03024 100644
>> --- a/drivers/char/hw_random/core.c
>> +++ b/drivers/char/hw_random/core.c
>> @@ -292,26 +292,48 @@ static struct miscdevice rng_miscdev = {
>>   .groups = rng_dev_groups,
>>  };
>>
>> +static int enable_best_rng(void)
>> +{
>> + int ret = -ENODEV;
>> +
>> + BUG_ON(!mutex_is_locked(_mutex));
>> +
>> + /* rng_list is sorted by quality, use the best (=first) one */
>> + if (!list_empty(_list)) {
>> + struct hwrng *new_rng;
>> +
>> + new_rng = list_entry(rng_list.next, struct hwrng, list);
>> + ret = ((new_rng == current_rng) ? 0 : 
>> set_current_rng(new_rng));
>> + if (!ret)
>> + cur_rng_set_by_user = 0;
>> + }
>> +
>> + return ret;
>> +}
>> +
>>  static ssize_t hwrng_attr_current_store(struct device *dev,
>>   struct device_attribute *attr,
>>   const char *buf, size_t len)
>>  {
>> - int err;
>> + int err = -ENODEV;
> Two lines later err is assigned the return value of 
> mutex_lock_interruptible().
> So the -ENODEV value assignment does nothing.

Indeed true. Will change this when I make a v2.

I am even thinking if it makes sense to return error when user writes
"" to rng_current and there is no rng device registered. I think
returning 0 should be fine.

>>   struct hwrng *rng;
>>
>>   err = mutex_lock_interruptible(_mutex);
>>   if (err)
>>   return -ERESTARTSYS;
>> - err = -ENODEV;
>> - list_for_each_entry(rng, _list, list) {
>> - if (sysfs_streq(rng->name, buf)) {
>> - err = 0;
>> - cur_rng_set_by_user = 1;
>> - if (rng != current_rng)
>> +
>> + if (sysfs_streq(buf, "")) {
>> + err = enable_best_rng();
>> + } else {
>> + list_for_each_entry(rng, _list, list) {
>> + if (sysfs_streq(rng->name, buf)) {
>> + cur_rng_set_by_user = 1;
>>   err = set_current_rng(rng);
>> - break;
>> + break;
>> + }
>>   }
>>   }
>> +
>>   mutex_unlock(_mutex);
>>
>>   return err ? : len;
>> @@ -493,17 +515,8 @@ void hwrng_unregister(struct hwrng *rng)
>>   mutex_lock(_mutex);
>>
>>   list_del(>list);
>> - if (current_rng == rng) {
>> - drop_current_rng();
>> - cur_rng_set_by_user = 0;
>> - /* rng_list is sorted by quality, use the best (=first) one */
>> - if (!list_empty(_list)) {
>> - struct hwrng *new_rng;
>> -
>> - new_rng = list_entry(rng_list.next, struct hwrng, 
>> list);
>> - set_current_rng(new_rng);
>> - }
>> - }
>> + if (current_rng == rng)
>> + enable_best_rng();
>>
>>   if (list_empty(_list)) {
>>   mutex_unlock(_mutex);
> looks like for me.
> reviewed-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>
>

Thank you.

Thanks,
PrasannaKumar

Re: [PATCH] hw_random: core: Remove unnecessary new line in MODULE_PARM_DESC

[PrasannaKumar Muralidharan]

Hi Herbert,

On 30 October 2017 at 12:23, Herbert Xu <herb...@gondor.apana.org.au> wrote:
> On Thu, Oct 26, 2017 at 10:00:29PM +0530, PrasannaKumar Muralidharan wrote:
>> While using MODULE_PARM_DESC there is a new line which is not required.
>> Remove it.
>>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>
> Please stop sending pointless patches like this.

Okay.

Regards,
PrasannaKumar

Re: [PATCH] hw_random: Include device.h instead of declaring struct device

[PrasannaKumar Muralidharan]

Hi Herbert,

On 30 October 2017 at 12:22, Herbert Xu <herb...@gondor.apana.org.au> wrote:
> On Thu, Oct 26, 2017 at 07:12:08PM +0530, PrasannaKumar Muralidharan wrote:
>> Include linux/device.h instead of declaring struct device.
>>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>
> Nack.  We should not include a header file when a simple forward
> declaration is enough.

No issues.

Regards,
PrasannaKumar

Re: [PATCH] hw_random: core: Reset user selected rng by writing "" to rng_current

[PrasannaKumar Muralidharan]

Hi Harald,

On 30 October 2017 at 13:40, Harald Freudenberger
 wrote:
> That's a really good idea. I also thought about something like that.

Do you mind reviewing the code?

Thanks,
PrasannaKumar

[PATCH] hw_random: core: Reset user selected rng by writing "" to rng_current

[PrasannaKumar Muralidharan]

User is able to select a chosen rng by writing its name to rng_current
but there is no way to reset it without unbinding the rng. Let user
write "" to rng_current and delesect the chosen rng.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 drivers/char/hw_random/core.c | 51 +++
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 9701ac7..be03024 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -292,26 +292,48 @@ static struct miscdevice rng_miscdev = {
.groups = rng_dev_groups,
 };
 
+static int enable_best_rng(void)
+{
+   int ret = -ENODEV;
+
+   BUG_ON(!mutex_is_locked(_mutex));
+
+   /* rng_list is sorted by quality, use the best (=first) one */
+   if (!list_empty(_list)) {
+   struct hwrng *new_rng;
+
+   new_rng = list_entry(rng_list.next, struct hwrng, list);
+   ret = ((new_rng == current_rng) ? 0 : set_current_rng(new_rng));
+   if (!ret)
+   cur_rng_set_by_user = 0;
+   }
+
+   return ret;
+}
+
 static ssize_t hwrng_attr_current_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t len)
 {
-   int err;
+   int err = -ENODEV;
struct hwrng *rng;
 
err = mutex_lock_interruptible(_mutex);
if (err)
return -ERESTARTSYS;
-   err = -ENODEV;
-   list_for_each_entry(rng, _list, list) {
-   if (sysfs_streq(rng->name, buf)) {
-   err = 0;
-   cur_rng_set_by_user = 1;
-   if (rng != current_rng)
+
+   if (sysfs_streq(buf, "")) {
+   err = enable_best_rng();
+   } else {
+   list_for_each_entry(rng, _list, list) {
+   if (sysfs_streq(rng->name, buf)) {
+   cur_rng_set_by_user = 1;
err = set_current_rng(rng);
-   break;
+   break;
+   }
}
}
+
mutex_unlock(_mutex);
 
return err ? : len;
@@ -493,17 +515,8 @@ void hwrng_unregister(struct hwrng *rng)
mutex_lock(_mutex);
 
list_del(>list);
-   if (current_rng == rng) {
-   drop_current_rng();
-   cur_rng_set_by_user = 0;
-   /* rng_list is sorted by quality, use the best (=first) one */
-   if (!list_empty(_list)) {
-   struct hwrng *new_rng;
-
-   new_rng = list_entry(rng_list.next, struct hwrng, list);
-   set_current_rng(new_rng);
-   }
-   }
+   if (current_rng == rng)
+   enable_best_rng();
 
if (list_empty(_list)) {
mutex_unlock(_mutex);
-- 
2.10.0

Re: [PATCH 0/4] Rearrange functions to remove forward declarations

[PrasannaKumar Muralidharan]

Hi David,

On 26 October 2017 at 22:18, David Daney <dda...@caviumnetworks.com> wrote:
> On 10/26/2017 08:34 AM, PrasannaKumar Muralidharan wrote:
>>
>> This patch series rearranges functions such that forward declarations
>> becomes unnecessary. Remove those forward declaration.
>
>
> Why?
>
> The code churn and increase in difficulty of attribution are big drawbacks
> to this sort of patch set.

Completely agreed. I did understand there is little value in this
change. Wasn't sure whether such a patch is encouraged so gave a try.
I am fine with not taking the patch.

Regards,
PrasannaKumar

[PATCH] hw_random: core: Remove unnecessary new line in MODULE_PARM_DESC

[PrasannaKumar Muralidharan]

While using MODULE_PARM_DESC there is a new line which is not required.
Remove it.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 drivers/char/hw_random/core.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 328d065..323f04c 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -43,11 +43,10 @@ static unsigned short current_quality;
 static unsigned short default_quality; /* = 0; default to "off" */
 
 module_param(current_quality, ushort, 0644);
-MODULE_PARM_DESC(current_quality,
-"current hwrng entropy estimation per mill");
+MODULE_PARM_DESC(current_quality, "current hwrng entropy estimation per mill");
+
 module_param(default_quality, ushort, 0644);
-MODULE_PARM_DESC(default_quality,
-"default entropy content of hwrng per mill");
+MODULE_PARM_DESC(default_quality, "default entropy content of hwrng per mill");
 
 static size_t rng_buffer_size(void)
 {
-- 
2.10.0

Re: [PATCH v3] tpm: use struct tpm_chip for tpm_chip_find_get()

[PrasannaKumar Muralidharan]

On 26 October 2017 at 21:39, Jarkko Sakkinen
<jarkko.sakki...@linux.intel.com> wrote:
> On Thu, Oct 26, 2017 at 07:40:49PM +0530, PrasannaKumar Muralidharan wrote:
>> Hi Jarkko,
>>
>> On 26 October 2017 at 19:24, Jarkko Sakkinen
>> <jarkko.sakki...@linux.intel.com> wrote:
>> > Device number (the character device index) is not a stable identifier
>> > for a TPM chip. That is the reason why every call site passes
>> > TPM_ANY_NUM to tpm_chip_find_get().
>> >
>> > This commit changes the API in a way that instead a struct tpm_chip
>> > instance is given and NULL means the default chip. In addition, this
>> > commit refines the documentation to be up to date with the
>> > implementation.
>> >
>> > Suggested-by: Jason Gunthorpe <jguntho...@obsidianresearch.com> (@chip_num 
>> > -> @chip)
>> > Signed-off-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
>> > ---
>> > v3:
>> > * If chip != NULL was given to tpm_chip_find_get(), it tried to reserve 
>> > chip
>> >   associated with the device number 0 instead of trying to reserve the 
>> > given
>> >   chip instance.
>> > v2:
>> > * Further defined function documentation.
>> > * Changed @chip_num to @chip instead of removing the parameter as 
>> > suggested by
>> >   Jason Gunthorpe.
>> >  drivers/char/hw_random/tpm-rng.c|   2 +-
>> >  drivers/char/tpm/tpm-chip.c |  24 ---
>> >  drivers/char/tpm/tpm-interface.c| 135 
>> > +++-
>> >  drivers/char/tpm/tpm.h  |   2 +-
>> >  include/linux/tpm.h |  38 +-
>> >  security/integrity/ima/ima_crypto.c |   2 +-
>> >  security/integrity/ima/ima_init.c   |   2 +-
>> >  security/integrity/ima/ima_queue.c  |   2 +-
>> >  security/keys/trusted.c |  35 +-
>> >  9 files changed, 126 insertions(+), 116 deletions(-)
>> >
>> > diff --git a/drivers/char/hw_random/tpm-rng.c 
>> > b/drivers/char/hw_random/tpm-rng.c
>> > index d6d448266f07..c5e363825af0 100644
>> > --- a/drivers/char/hw_random/tpm-rng.c
>> > +++ b/drivers/char/hw_random/tpm-rng.c
>> > @@ -25,7 +25,7 @@
>> >
>> >  static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool 
>> > wait)
>> >  {
>> > -   return tpm_get_random(TPM_ANY_NUM, data, max);
>> > +   return tpm_get_random(NULL, data, max);
>> >  }
>> >
>> >  static struct hwrng tpm_rng = {
>> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> > index a114e8f7fb90..bab9c14e040c 100644
>> > --- a/drivers/char/tpm/tpm-chip.c
>> > +++ b/drivers/char/tpm/tpm-chip.c
>> > @@ -81,21 +81,26 @@ void tpm_put_ops(struct tpm_chip *chip)
>> >  EXPORT_SYMBOL_GPL(tpm_put_ops);
>> >
>> >  /**
>> > - * tpm_chip_find_get() - return tpm_chip for a given chip number
>> > - * @chip_num: id to find
>> > + * tpm_chip_find_get() - find and reserve a TPM chip
>> > + * @chip:  a  tpm_chip instance, %NULL for the default chip
>> >   *
>> > - * The return'd chip has been tpm_try_get_ops'd and must be released via
>> > - * tpm_put_ops
>> > + * Finds a TPM chip and reserves its class device and operations. The 
>> > chip must
>> > + * be released with tpm_chip_put_ops() after use.
>> > + *
>> > + * Return:
>> > + * A reserved  tpm_chip instance.
>> > + * %NULL if a chip is not found.
>> > + * %NULL if the chip is not available.
>> >   */
>> > -struct tpm_chip *tpm_chip_find_get(int chip_num)
>> > +struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
>> >  {
>> > -   struct tpm_chip *chip, *res = NULL;
>> > +   struct tpm_chip *res = NULL;
>> > +   int chip_num = 0;
>> > int chip_prev;
>> >
>> > mutex_lock(_lock);
>> >
>> > -   if (chip_num == TPM_ANY_NUM) {
>> > -   chip_num = 0;
>> > +   if (!chip) {
>> > do {
>> > chip_prev = chip_num;
>> > chip = idr_get_next(_nums_idr, _num);
>> > @@ -105,8 +110,7 @@ struct tpm_chip *tpm_chip_find_get(int chip_num)
>> > }
>> > } while (chip_prev != chip_num);
>> > } else {
>> > -  

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

[PrasannaKumar Muralidharan]

On 26 October 2017 at 00:41, Jarkko Sakkinen
<jarkko.sakki...@linux.intel.com> wrote:
> On Wed, Oct 25, 2017 at 08:21:16PM +0530, PrasannaKumar Muralidharan wrote:
>> >> > 2. Moving struct tpm_rng to the TPM client is architecturally
>> >> >uacceptable.
>> >>
>> >> As there was no response to the patch there is no way to know whether
>> >> it is acceptable or not.
>> >
>> > I like the idea of removing the tpm rng driver as discussed in other
>> > emails in this thread.
>>
>> Thank you.
>
> No, thank you.
>
> I didn't first understand the big idea and only looked at the code
> change per se. I apologize for that.

No need for that. I missed mentioning the reason for the patch and it
is not obvious from code change. Its my fault.

> The problem that you went to solve was real and it led to a properly
> implemented solution. You were not late from the party. Jason's code
> change is derivative work of your code change. That's why his code
> change has also your signed-off-by.
>
> Thanks for doing awesome work :-)

Its really nice to hear such words :-) :-D.

>
> /Jarkko

Thanks and regards,
PrasannaKumar

[PATCH 4/4] hw_random: core: Remove forward declaration of hwrng_init

[PrasannaKumar Muralidharan]

Rearrange set_current_rng such that hwrng_init's forward declaration can
be removed.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 drivers/char/hw_random/core.c | 34 --
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 534c2ae..328d065 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -49,8 +49,6 @@ module_param(default_quality, ushort, 0644);
 MODULE_PARM_DESC(default_quality,
 "default entropy content of hwrng per mill");
 
-static int hwrng_init(struct hwrng *rng);
-
 static size_t rng_buffer_size(void)
 {
return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES;
@@ -108,22 +106,6 @@ static void drop_current_rng(void)
current_rng = NULL;
 }
 
-static int set_current_rng(struct hwrng *rng)
-{
-   int err;
-
-   BUG_ON(!mutex_is_locked(_mutex));
-
-   err = hwrng_init(rng);
-   if (err)
-   return err;
-
-   drop_current_rng();
-   current_rng = rng;
-
-   return 0;
-}
-
 /* Returns ERR_PTR(), NULL or refcounted hwrng */
 static struct hwrng *get_current_rng(void)
 {
@@ -220,6 +202,22 @@ static int hwrng_init(struct hwrng *rng)
return 0;
 }
 
+static int set_current_rng(struct hwrng *rng)
+{
+   int err;
+
+   BUG_ON(!mutex_is_locked(_mutex));
+
+   err = hwrng_init(rng);
+   if (err)
+   return err;
+
+   drop_current_rng();
+   current_rng = rng;
+
+   return 0;
+}
+
 static int rng_dev_open(struct inode *inode, struct file *filp)
 {
/* enforce read-only access to this chrdev */
-- 
2.10.0

[PATCH 2/4] hw_random: core: Rearranging rng_get_data to remove forward declaration

[PrasannaKumar Muralidharan]

Rearrange rng_get_data such that its forward declaration is not
required.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 drivers/char/hw_random/core.c | 41 +++--
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 8e1f43c..7e2b1a7 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -52,14 +52,30 @@ MODULE_PARM_DESC(default_quality,
 static int hwrng_init(struct hwrng *rng);
 static void start_khwrngd(void);
 
-static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size,
-  int wait);
-
 static size_t rng_buffer_size(void)
 {
return SMP_CACHE_BYTES < 32 ? 32 : SMP_CACHE_BYTES;
 }
 
+static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size,
+   int wait) {
+   int present;
+
+   BUG_ON(!mutex_is_locked(_mutex));
+   if (rng->read)
+   return rng->read(rng, (void *)buffer, size, wait);
+
+   if (rng->data_present)
+   present = rng->data_present(rng, wait);
+   else
+   present = 1;
+
+   if (present)
+   return rng->data_read(rng, (u32 *)buffer);
+
+   return 0;
+}
+
 static void add_early_randomness(struct hwrng *rng)
 {
int bytes_read;
@@ -178,25 +194,6 @@ static int rng_dev_open(struct inode *inode, struct file 
*filp)
return 0;
 }
 
-static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size,
-   int wait) {
-   int present;
-
-   BUG_ON(!mutex_is_locked(_mutex));
-   if (rng->read)
-   return rng->read(rng, (void *)buffer, size, wait);
-
-   if (rng->data_present)
-   present = rng->data_present(rng, wait);
-   else
-   present = 1;
-
-   if (present)
-   return rng->data_read(rng, (u32 *)buffer);
-
-   return 0;
-}
-
 static ssize_t rng_dev_read(struct file *filp, char __user *buf,
size_t size, loff_t *offp)
 {
-- 
2.10.0

[PATCH 1/4] hw_random: core: Remove forward declaration by rearranging code

[PrasannaKumar Muralidharan]

Rearrange drop_current_rng such that its forward declaration is not
required.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 drivers/char/hw_random/core.c | 23 +++
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 9701ac7..8e1f43c 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -49,7 +49,6 @@ module_param(default_quality, ushort, 0644);
 MODULE_PARM_DESC(default_quality,
 "default entropy content of hwrng per mill");
 
-static void drop_current_rng(void);
 static int hwrng_init(struct hwrng *rng);
 static void start_khwrngd(void);
 
@@ -83,6 +82,17 @@ static inline void cleanup_rng(struct kref *kref)
complete(>cleanup_done);
 }
 
+static void drop_current_rng(void)
+{
+   BUG_ON(!mutex_is_locked(_mutex));
+   if (!current_rng)
+   return;
+
+   /* decrease last reference for triggering the cleanup */
+   kref_put(_rng->ref, cleanup_rng);
+   current_rng = NULL;
+}
+
 static int set_current_rng(struct hwrng *rng)
 {
int err;
@@ -99,17 +109,6 @@ static int set_current_rng(struct hwrng *rng)
return 0;
 }
 
-static void drop_current_rng(void)
-{
-   BUG_ON(!mutex_is_locked(_mutex));
-   if (!current_rng)
-   return;
-
-   /* decrease last reference for triggering the cleanup */
-   kref_put(_rng->ref, cleanup_rng);
-   current_rng = NULL;
-}
-
 /* Returns ERR_PTR(), NULL or refcounted hwrng */
 static struct hwrng *get_current_rng(void)
 {
-- 
2.10.0

[PATCH 3/4] hw_random: core: Rearranging start_khwrngd to remove forward declaration

[PrasannaKumar Muralidharan]

Rearrange start_khwrngd such that its forward declaration is not
required.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 drivers/char/hw_random/core.c | 75 +--
 1 file changed, 37 insertions(+), 38 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 7e2b1a7..534c2ae 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -50,7 +50,6 @@ MODULE_PARM_DESC(default_quality,
 "default entropy content of hwrng per mill");
 
 static int hwrng_init(struct hwrng *rng);
-static void start_khwrngd(void);
 
 static size_t rng_buffer_size(void)
 {
@@ -153,6 +152,43 @@ static void put_rng(struct hwrng *rng)
mutex_unlock(_mutex);
 }
 
+static int hwrng_fillfn(void *unused)
+{
+   long rc;
+
+   while (!kthread_should_stop()) {
+   struct hwrng *rng;
+
+   rng = get_current_rng();
+   if (IS_ERR(rng) || !rng)
+   break;
+   mutex_lock(_mutex);
+   rc = rng_get_data(rng, rng_fillbuf,
+ rng_buffer_size(), 1);
+   mutex_unlock(_mutex);
+   put_rng(rng);
+   if (rc <= 0) {
+   pr_warn("hwrng: no data available\n");
+   msleep_interruptible(1);
+   continue;
+   }
+   /* Outside lock, sure, but y'know: randomness. */
+   add_hwgenerator_randomness((void *)rng_fillbuf, rc,
+  rc * current_quality * 8 >> 10);
+   }
+   hwrng_fill = NULL;
+   return 0;
+}
+
+static void start_khwrngd(void)
+{
+   hwrng_fill = kthread_run(hwrng_fillfn, NULL, "hwrng");
+   if (IS_ERR(hwrng_fill)) {
+   pr_err("hwrng_fill thread creation failed");
+   hwrng_fill = NULL;
+   }
+}
+
 static int hwrng_init(struct hwrng *rng)
 {
if (kref_get_unless_zero(>ref))
@@ -387,43 +423,6 @@ static int __init register_miscdev(void)
return misc_register(_miscdev);
 }
 
-static int hwrng_fillfn(void *unused)
-{
-   long rc;
-
-   while (!kthread_should_stop()) {
-   struct hwrng *rng;
-
-   rng = get_current_rng();
-   if (IS_ERR(rng) || !rng)
-   break;
-   mutex_lock(_mutex);
-   rc = rng_get_data(rng, rng_fillbuf,
- rng_buffer_size(), 1);
-   mutex_unlock(_mutex);
-   put_rng(rng);
-   if (rc <= 0) {
-   pr_warn("hwrng: no data available\n");
-   msleep_interruptible(1);
-   continue;
-   }
-   /* Outside lock, sure, but y'know: randomness. */
-   add_hwgenerator_randomness((void *)rng_fillbuf, rc,
-  rc * current_quality * 8 >> 10);
-   }
-   hwrng_fill = NULL;
-   return 0;
-}
-
-static void start_khwrngd(void)
-{
-   hwrng_fill = kthread_run(hwrng_fillfn, NULL, "hwrng");
-   if (IS_ERR(hwrng_fill)) {
-   pr_err("hwrng_fill thread creation failed");
-   hwrng_fill = NULL;
-   }
-}
-
 int hwrng_register(struct hwrng *rng)
 {
int err = -EINVAL;
-- 
2.10.0

[PATCH 0/4] Rearrange functions to remove forward declarations

[PrasannaKumar Muralidharan]

This patch series rearranges functions such that forward declarations
becomes unnecessary. Remove those forward declaration.

This patch series is boot tested without user space in qemu with
CONFIG_HW_RANDOM=y.

PrasannaKumar Muralidharan (4):
  hw_random: core: Remove forward declaration by rearranging code
  hw_random: core: Rearranging rng_get_data to remove forward
declaration
  hw_random: core: Rearranging start_khwrngd to remove forward
declaration
  hw_random: core: Remove forward declaration of hwrng_init

 drivers/char/hw_random/core.c | 151 --
 1 file changed, 72 insertions(+), 79 deletions(-)

-- 
2.10.0

Re: [PATCH v3] tpm: use struct tpm_chip for tpm_chip_find_get()

[PrasannaKumar Muralidharan]

,13 +355,12 @@ static int TSS_checkhmac2(unsigned char *buffer,
>   * For key specific tpm requests, we will generate and send our
>   * own TPM command packets using the drivers send function.
>   */
> -static int trusted_tpm_send(const u32 chip_num, unsigned char *cmd,
> -   size_t buflen)
> +static int trusted_tpm_send(unsigned char *cmd, size_t buflen)
>  {
> int rc;
>
> dump_tpm_buf(cmd);
> -   rc = tpm_send(chip_num, cmd, buflen);
> +   rc = tpm_send(NULL, cmd, buflen);
> dump_tpm_buf(cmd);
> if (rc > 0)
> /* Can't return positive return codes values to keyctl */
> @@ -382,10 +381,10 @@ static int pcrlock(const int pcrnum)
>
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
> -   ret = tpm_get_random(TPM_ANY_NUM, hash, SHA1_DIGEST_SIZE);
> +   ret = tpm_get_random(NULL, hash, SHA1_DIGEST_SIZE);
> if (ret != SHA1_DIGEST_SIZE)
> return ret;
> -   return tpm_pcr_extend(TPM_ANY_NUM, pcrnum, hash) ? -EINVAL : 0;
> +   return tpm_pcr_extend(NULL, pcrnum, hash) ? -EINVAL : 0;
>  }
>
>  /*
> @@ -398,7 +397,7 @@ static int osap(struct tpm_buf *tb, struct osapsess *s,
> unsigned char ononce[TPM_NONCE_SIZE];
> int ret;
>
> -   ret = tpm_get_random(TPM_ANY_NUM, ononce, TPM_NONCE_SIZE);
> +   ret = tpm_get_random(NULL, ononce, TPM_NONCE_SIZE);
> if (ret != TPM_NONCE_SIZE)
> return ret;
>
> @@ -410,7 +409,7 @@ static int osap(struct tpm_buf *tb, struct osapsess *s,
> store32(tb, handle);
> storebytes(tb, ononce, TPM_NONCE_SIZE);
>
> -   ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, MAX_BUF_SIZE);
> +   ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
> if (ret < 0)
> return ret;
>
> @@ -434,7 +433,7 @@ static int oiap(struct tpm_buf *tb, uint32_t *handle, 
> unsigned char *nonce)
> store16(tb, TPM_TAG_RQU_COMMAND);
> store32(tb, TPM_OIAP_SIZE);
> store32(tb, TPM_ORD_OIAP);
> -   ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, MAX_BUF_SIZE);
> +   ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
> if (ret < 0)
> return ret;
>
> @@ -493,7 +492,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> if (ret < 0)
> goto out;
>
> -   ret = tpm_get_random(TPM_ANY_NUM, td->nonceodd, TPM_NONCE_SIZE);
> +   ret = tpm_get_random(NULL, td->nonceodd, TPM_NONCE_SIZE);
> if (ret != TPM_NONCE_SIZE)
> goto out;
> ordinal = htonl(TPM_ORD_SEAL);
> @@ -542,7 +541,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> store8(tb, cont);
> storebytes(tb, td->pubauth, SHA1_DIGEST_SIZE);
>
> -   ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, MAX_BUF_SIZE);
> +   ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
> if (ret < 0)
> goto out;
>
> @@ -603,7 +602,7 @@ static int tpm_unseal(struct tpm_buf *tb,
>
> ordinal = htonl(TPM_ORD_UNSEAL);
> keyhndl = htonl(SRKHANDLE);
> -   ret = tpm_get_random(TPM_ANY_NUM, nonceodd, TPM_NONCE_SIZE);
> +   ret = tpm_get_random(NULL, nonceodd, TPM_NONCE_SIZE);
> if (ret != TPM_NONCE_SIZE) {
> pr_info("trusted_key: tpm_get_random failed (%d)\n", ret);
> return ret;
> @@ -635,7 +634,7 @@ static int tpm_unseal(struct tpm_buf *tb,
> store8(tb, cont);
> storebytes(tb, authdata2, SHA1_DIGEST_SIZE);
>
> -   ret = trusted_tpm_send(TPM_ANY_NUM, tb->data, MAX_BUF_SIZE);
> +   ret = trusted_tpm_send(tb->data, MAX_BUF_SIZE);
> if (ret < 0) {
> pr_info("trusted_key: authhmac failed (%d)\n", ret);
> return ret;
> @@ -748,7 +747,7 @@ static int getoptions(char *c, struct trusted_key_payload 
> *pay,
> int i;
> int tpm2;
>
> -   tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
> +   tpm2 = tpm_is_tpm2(NULL);
> if (tpm2 < 0)
> return tpm2;
>
> @@ -917,7 +916,7 @@ static struct trusted_key_options 
> *trusted_options_alloc(void)
> struct trusted_key_options *options;
> int tpm2;
>
> -   tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
> +   tpm2 = tpm_is_tpm2(NULL);
> if (tpm2 < 0)
> return NULL;
>
> @@ -967,7 +966,7 @@ static int trusted_instantiate(struct key *key,
> size_t key_len;
> int tpm2;
>
> -   tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
> +   tpm2 = tpm_is_tpm2(NULL);
> if (tpm2 < 0)
> return tpm2;
>
> @@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
> switch (key_cmd) {
> case Opt_load:
> if (tpm2)
> -   ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, 
> options);
> +   ret = tpm_unseal_trusted(NULL, payload, options);
> else
> ret = key_unseal(payload, options);
> dump_payload(payload);
> @@ -1018,13 +1017,13 @@ static int trusted_instantiate(struct key *key,
> break;
> case Opt_new:
> key_len = payload->key_len;
> -   ret = tpm_get_random(TPM_ANY_NUM, payload->key, key_len);
> +   ret = tpm_get_random(NULL, payload->key, key_len);
> if (ret != key_len) {
> pr_info("trusted_key: key_create failed (%d)\n", ret);
> goto out;
> }
> if (tpm2)
> -   ret = tpm_seal_trusted(TPM_ANY_NUM, payload, options);
> +   ret = tpm_seal_trusted(NULL, payload, options);
> else
> ret = key_seal(payload, options);
> if (ret < 0)
> --
> 2.14.1
>

Looks good to me. FWIW, Reviewed-by: PrasannaKumar Muralidharan
<prasannatsmku...@gmail.com>.

Regards,
PrasannaKumar

[PATCH] hw_random: Include device.h instead of declaring struct device

[PrasannaKumar Muralidharan]

Include linux/device.h instead of declaring struct device.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 include/linux/hw_random.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index bee0827..2ec9af7 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -13,6 +13,7 @@
 #define LINUX_HWRANDOM_H_
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -51,8 +52,6 @@ struct hwrng {
struct completion cleanup_done;
 };
 
-struct device;
-
 /** Register a new Hardware Random Number Generator driver. */
 extern int hwrng_register(struct hwrng *rng);
 extern int devm_hwrng_register(struct device *dev, struct hwrng *rng);
-- 
2.10.0

Re: [PATCH] tpm: Move Linux RNG connection to hwrng

[PrasannaKumar Muralidharan]

Hi Jason,

On 25 October 2017 at 20:48, Jason Gunthorpe
<jguntho...@obsidianresearch.com> wrote:
> On Wed, Oct 25, 2017 at 08:15:09PM +0530, PrasannaKumar Muralidharan
> wrote:
>
>> > +static int tpm_add_hwrng(struct tpm_chip *chip)
>> > +{
>> > +   if (!IS_ENABLED(CONFIG_HW_RANDOM_TPM))
>> > +   return 0;
>>
>> Can #ifndef CONFIG_HW_RANDOM_TPM be used instead? That way an if
>> condition can be avoided.
>
> Generally speaking IS_ENABLED is prefered over #ifdef as it reduces the
> set of compilation combinations.

Oh okay. No issues then.

Regards,
PrasannaKumar

Re: [PATCH v2] tpm: use struct tpm_chip for tpm_chip_find_get()

[PrasannaKumar Muralidharan]

Hi Jarkko,

On 25 October 2017 at 17:25, Jarkko Sakkinen
 wrote:
> Device number (the character device index) is not a stable identifier
> for a TPM chip. That is the reason why every call site passes
> TPM_ANY_NUM to tpm_chip_find_get().
>
> This commit changes the API in a way that instead a struct tpm_chip
> instance is given and NULL means the default chip. In addition, this
> commit refines the documentation to be up to date with the
> implementation.
>
> Suggested-by: Jason Gunthorpe  (@chip_num -> 
> @chip)
> Signed-off-by: Jarkko Sakkinen 
> ---
> v2:
> * Further defined function documentation.
> * Changed @chip_num to @chip instead of removing the parameter as suggested by
>   Jason Gunthorpe.
>  drivers/char/hw_random/tpm-rng.c|   2 +-
>  drivers/char/tpm/tpm-chip.c |  21 +++---
>  drivers/char/tpm/tpm-interface.c| 135 
> +++-
>  drivers/char/tpm/tpm.h  |   2 +-
>  include/linux/tpm.h |  38 +-
>  security/integrity/ima/ima_crypto.c |   2 +-
>  security/integrity/ima/ima_init.c   |   2 +-
>  security/integrity/ima/ima_queue.c  |   2 +-
>  security/keys/trusted.c |  35 +-
>  9 files changed, 125 insertions(+), 114 deletions(-)
>
> diff --git a/drivers/char/hw_random/tpm-rng.c 
> b/drivers/char/hw_random/tpm-rng.c
> index d6d448266f07..c5e363825af0 100644
> --- a/drivers/char/hw_random/tpm-rng.c
> +++ b/drivers/char/hw_random/tpm-rng.c
> @@ -25,7 +25,7 @@
>
>  static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
>  {
> -   return tpm_get_random(TPM_ANY_NUM, data, max);
> +   return tpm_get_random(NULL, data, max);
>  }
>
>  static struct hwrng tpm_rng = {
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index a114e8f7fb90..c7a4e7fb424d 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -81,21 +81,26 @@ void tpm_put_ops(struct tpm_chip *chip)
>  EXPORT_SYMBOL_GPL(tpm_put_ops);
>
>  /**
> - * tpm_chip_find_get() - return tpm_chip for a given chip number
> - * @chip_num: id to find
> + * tpm_chip_find_get() - find and reserve a TPM chip
> + * @chip:  a  tpm_chip instance, %NULL for the default chip
>   *
> - * The return'd chip has been tpm_try_get_ops'd and must be released via
> - * tpm_put_ops
> + * Finds a TPM chip and reserves its class device and operations. The chip 
> must
> + * be released with tpm_chip_put_ops() after use.
> + *
> + * Return:
> + * A reserved  tpm_chip instance.
> + * %NULL if a chip is not found.
> + * %NULL if the chip is not available.
>   */
> -struct tpm_chip *tpm_chip_find_get(int chip_num)
> +struct tpm_chip *tpm_chip_find_get(struct tpm_chip *chip)
>  {
> -   struct tpm_chip *chip, *res = NULL;
> +   struct tpm_chip *res = NULL;
> +   int chip_num = 0;
> int chip_prev;
>
> mutex_lock(_lock);
>
> -   if (chip_num == TPM_ANY_NUM) {
> -   chip_num = 0;
> +   if (!chip) {
> do {
> chip_prev = chip_num;
> chip = idr_get_next(_nums_idr, _num);

When chip is not NULL just do tpm_try_get_ops(chip). Current code does
more things which are not required.

> diff --git a/drivers/char/tpm/tpm-interface.c 
> b/drivers/char/tpm/tpm-interface.c
> index ebe0a1d36d8c..19f820f775b5 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -809,19 +809,20 @@ int tpm_pcr_read_dev(struct tpm_chip *chip, int 
> pcr_idx, u8 *res_buf)
>  }
>
>  /**
> - * tpm_is_tpm2 - is the chip a TPM2 chip?
> - * @chip_num:  tpm idx # or ANY
> + * tpm_is_tpm2 - do we a have a TPM2 chip?
> + * @chip:  a  tpm_chip instance, %NULL for the default chip
>   *
> - * Returns < 0 on error, and 1 or 0 on success depending whether the chip
> - * is a TPM2 chip.
> + * Return:
> + * 1 if we have a TPM2 chip.
> + * 0 if we don't have a TPM2 chip.
> + * A negative number for system errors (errno).
>   */
> -int tpm_is_tpm2(u32 chip_num)
> +int tpm_is_tpm2(struct tpm_chip *chip)
>  {
> -   struct tpm_chip *chip;
> int rc;
>
> -   chip = tpm_chip_find_get(chip_num);
> -   if (chip == NULL)
> +   chip = tpm_chip_find_get(chip);
> +   if (!chip)
> return -ENODEV;
>
> rc = (chip->flags & TPM_CHIP_FLAG_TPM2) != 0;
> @@ -833,23 +834,19 @@ int tpm_is_tpm2(u32 chip_num)
>  EXPORT_SYMBOL_GPL(tpm_is_tpm2);
>
>  /**
> - * tpm_pcr_read - read a pcr value
> - * @chip_num:  tpm idx # or ANY
> - * @pcr_idx:   pcr idx to retrieve
> - * @res_buf:   TPM_PCR value
> - * size of res_buf is 20 bytes (or NULL if you don't care)
> + * tpm_pcr_read - read a PCR value from SHA1 bank
> + * @chip:  a  tpm_chip instance, %NULL for the default chip
> + * @pcr_idx:   the PCR to be retrieved
> + * @res_buf:   the value of the PCR
>   *
> - * 

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

[PrasannaKumar Muralidharan]

Hi Jarkko,

On 24 October 2017 at 23:52, Jarkko Sakkinen
<jarkko.sakki...@linux.intel.com> wrote:
> On Tue, Oct 24, 2017 at 10:05:20PM +0530, PrasannaKumar Muralidharan wrote:
>> > 1. Every user in the kernel is using TPM_ANY_NUM, which means there are
>> >no other users.
>>
>> Completely agree that there is no in kernel users yet.
>
> And should never be. It's a bogus parameter that makes no sense.

I understood that after seeing latest patch that uses struct tpm_chip.
Sorry for the noise.

>> > 2. Moving struct tpm_rng to the TPM client is architecturally
>> >uacceptable.
>>
>> As there was no response to the patch there is no way to know whether
>> it is acceptable or not.
>
> I like the idea of removing the tpm rng driver as discussed in other
> emails in this thread.

Thank you.

>> > 3. Using zero deos not give you any better guarantees on anything than
>> >just using TPM_ANY_NUM.
>>
>> Chip id is used, not zero.
>
> Sorry I misread the patch first time. Anyway it's not any kind of ID to
> be trusted.

Okay.

>> > Why this patch is not CC'd to linux-integrity? It modifies the TPM
>> > driver. And in the worst way.
>>
>> TPM list is moderated and the moderator has not approved it yet.
>> get_maintainer script did not say about linux-integrity mailing list.
>>
>> It could be doing things in worst way but it is not known until some
>> one says. If no one tells it is the case I don't think it is possible
>> to fix. Which is what happened.
>
> Understood. We've moved to linux-integr...@vger.kernel.org. MAINTAINERS
> update is in the queue for the next kernel release.

Sorry I never knew this.

>> > Implementing the ideas that Jason explained is the senseful way to
>> > get stable access. modules.dep makes sure that the modules are loaded
>> > in the correct order.
>>
>> If that is sensible then it is the way to go.
>>
>> There must be a reason to believe what is sensible and what is not.
>> Looks like this RFC has helped in judging that.
>>
>> Regards,
>> PrasannaKumar
>
> Would you be interested to work on patch set that would remove the
> existing tpm rng driver and make the TPM driver the customer? It's not
> that far away from the work you've been doing already.
>
> /Jarkko

I am late to the party. Jason has sent a patch doing that by the time
I read this email.

Thanks and regards,
PrasannaKumar

Re: [PATCH] tpm: Move Linux RNG connection to hwrng

[PrasannaKumar Muralidharan]

Hi Jason,

Nice to see this patch. Some minor comments below.

On 25 October 2017 at 00:12, Jason Gunthorpe
<jguntho...@obsidianresearch.com> wrote:
> The tpm-rng.c approach is completely inconsistent with how the kernel
> handles hotplug. Instead manage a hwrng device for each TPM. This will
> cause the kernel to read entropy from the TPM when it is plugged in,
> and allow access to the TPM rng via /dev/hwrng.
>
> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
> Signed-off-by: Jason Gunthorpe <jguntho...@obsidianresearch.com>
> Reviewed-by: Jason Gunthorpe <jguntho...@obsidianresearch.com>
> ---
>  drivers/char/hw_random/Kconfig   | 13 ---
>  drivers/char/hw_random/Makefile  |  1 -
>  drivers/char/hw_random/tpm-rng.c | 50 
> 
>  drivers/char/tpm/Kconfig | 13 +++
>  drivers/char/tpm/tpm-chip.c  | 41 
>  drivers/char/tpm/tpm-interface.c | 37 -
>  drivers/char/tpm/tpm.h   |  5 
>  7 files changed, 76 insertions(+), 84 deletions(-)
>  delete mode 100644 drivers/char/hw_random/tpm-rng.c
>
> All,
>
> It is such a good idea, I took PrasannaKumar's patch, reviewed and
> revised it to the level it could be merged.

Thank you for this.

> This is compile tested only.
>
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 95a031e9eced07..a20fed182cbcce 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -306,19 +306,6 @@ config HW_RANDOM_POWERNV
>
>   If unsure, say Y.
>
> -config HW_RANDOM_TPM
> -   tristate "TPM HW Random Number Generator support"
> -   depends on TCG_TPM
> -   default HW_RANDOM
> -   ---help---
> - This driver provides kernel-side support for the Random Number
> - Generator in the Trusted Platform Module
> -
> - To compile this driver as a module, choose M here: the
> - module will be called tpm-rng.
> -
> - If unsure, say Y.
> -
>  config HW_RANDOM_HISI
> tristate "Hisilicon Random Number Generator support"
> depends on HW_RANDOM && ARCH_HISI
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index 39a67defac67cb..91cb8e8213e7c1 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -26,7 +26,6 @@ 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_HISI)   += hisi-rng.o
> -obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
>  obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
>  obj-$(CONFIG_HW_RANDOM_IPROC_RNG200) += iproc-rng200.o
>  obj-$(CONFIG_HW_RANDOM_MSM) += msm-rng.o
> diff --git a/drivers/char/hw_random/tpm-rng.c 
> b/drivers/char/hw_random/tpm-rng.c
> deleted file mode 100644
> index d6d448266f0752..00
> --- a/drivers/char/hw_random/tpm-rng.c
> +++ /dev/null
> @@ -1,50 +0,0 @@
> -/*
> - * Copyright (C) 2012 Kent Yoder IBM Corporation
> - *
> - * HWRNG interfaces to pull RNG data from a TPM
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 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 
> -
> -#define MODULE_NAME "tpm-rng"
> -
> -static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> -{
> -   return tpm_get_random(TPM_ANY_NUM, data, max);
> -}
> -
> -static struct hwrng tpm_rng = {
> -   .name = MODULE_NAME,
> -   .read = tpm_rng_read,
> -};
> -
> -static int __init rng_init(void)
> -{
> -   return hwrng_register(_rng);
> -}
> -module_init(rng_init);
> -
> -static void __exit rng_exit(void)
> -{
> -   hwrng_unregister(_rng);
> -}
> -module_exit(rng_exit);
> -
> -MODULE_LICENSE("GPL v2");
> -MODULE_AUTHOR("Kent Yoder <k...@linux.vnet.ibm.com>");
> -MODULE_DESCRI

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

[PrasannaKumar Muralidharan]

Hi Jason,

On 24 October 2017 at 23:16, Jason Gunthorpe
<jguntho...@obsidianresearch.com> wrote:
> On Tue, Oct 24, 2017 at 09:44:30PM +0530, PrasannaKumar Muralidharan wrote:
>
>> I am wondering why it is wrong. Isn't the chip id valid till it is
>> unregistered? If so the rfc is correct. Please explain, may be I am
>> missing something.
>
> The lifetime is a bit complicated, but the general rule in the kernel
> for things like this it to use pointers, not ids, and certainly not
> string ids.
>
> For that patch it could just use container_of to get the chip..
>
> Jason

hwrng requires a unique name for every device. In that patch
"tpm-rng-" is used. chip_num is nothing but dev->dev_num.
This way more than 1 tpm chip can be used as rng provider.
tpm_get_random uses chip_num as its parameter. This is why
chip->dev_num was used.

Is that reasoning correct?

Please feel free to correct me if I am wrong.

Thanks,
PrasannaKumar

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

[PrasannaKumar Muralidharan]

On 24 October 2017 at 23:07, Jason Gunthorpe
 wrote:
> On Tue, Oct 24, 2017 at 10:02:00AM -0700, Dmitry Torokhov wrote:
>> tpm-rng is abomination that should be kicked out as soon as possible.
>> It wrecks havoc with the power management (TPM chip drivers may go
>> into suspend state, but tpm_rng does not do any power management and
>> happily forwards requests to suspended hardware) and may be available
>> when there is no TPM at all yet (the drivers have not been probed yet,
>> or have gotten a deferral, etc).
>
> Makes sense
>
>> TPM core should register HWRNGs when chips are ready.
>
> The main thing I've wanted from the TPM RNG is
> 'add_early_randomness'..
>
> We can certainly provide a TPM interface to hwrng, it seems
> reasonable.
>
> Excep that we already have a user api in /dev/tpm to access the
> tpm RNG, is the duplication a problem?

I tried to do that via the rfc we discussed previously. It may not be
the right way but I wanted to start the discussion via the rfc.

Thanks,
PrasannaKumar

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

[PrasannaKumar Muralidharan]

On 24 October 2017 at 21:53, Jarkko Sakkinen
<jarkko.sakki...@linux.intel.com> wrote:
> On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
>> On 24 October 2017 at 21:14, Jarkko Sakkinen
>> <jarkko.sakki...@linux.intel.com> wrote:
>> > On Mon, Oct 23, 2017 at 10:31:39AM -0600, Jason Gunthorpe wrote:
>> >> On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
>> >>
>> >> > >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
>> >> > >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
>> >> > >  {
>> >> >
>> >> >
>> >> > I think every kernel internal TPM driver API should be called with the
>> >> > tpm_chip as a parameter. This is in foresight of namespacing of IMA 
>> >> > where we
>> >> > want to provide the flexibility of passing a dedicated vTPM to each
>> >> > namespace and IMA would use the chip as a parameter to all of these
>> >> > functions to talk to the right tpm_vtpm_proxy instance. From that
>> >> > perspective this patch goes into the wrong direction.
>> >>
>> >> Yes, we should ultimately try and get to there.. Someday the
>> >> tpm_chip_find_get() will need to become namespace aware.
>> >>
>> >> But this patch is along the right path, eliminating the chip_num is
>> >> the right thing to do..
>> >>
>> >> > >-  tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
>> >> > >+  tpm2 = tpm_is_tpm2();
>> >> > >   if (tpm2 < 0)
>> >> > >   return tpm2;
>> >> > >
>> >> > >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
>> >> > >   switch (key_cmd) {
>> >> > >   case Opt_load:
>> >> > >   if (tpm2)
>> >> > >-  ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, 
>> >> > >options);
>> >> > >+  ret = tpm_unseal_trusted(payload, options);
>> >>
>> >> Sequences like this are sketchy.
>> >>
>> >> It should be
>> >>
>> >> struct tpm_chip *tpm;
>> >>
>> >> tpm = tpm_chip_find_get()
>> >> tpm2 = tpm_is_tpm2(tpm);
>> >>
>> >> [..]
>> >>
>> >> if (tpm2)
>> >>  ret = tpm_unseal_trusted(tpm, payload, options);
>> >>
>> >> [..]
>> >>
>> >> tpm_put_chip(tpm);
>> >>
>> >> As hot plug could alter the 'tpm' between the two tpm calls.
>> >>
>> >> Jason
>> >
>> > This patch just removes bunch of dead code. It does not change existing
>> > semantics. What you are saying should be done after the dead code has
>> > been removed. This commit is first step to that direction.
>> >
>> > /Jarkko
>>
>> Please check the RFC [1]. It does use chip id. The rfc has issues and
>> has to be fixed but still there could be users of the API.
>>
>> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>>
>> Regards,
>> PrasannaKumar
>
> 1. Every user in the kernel is using TPM_ANY_NUM, which means there are
>no other users.

Completely agree that there is no in kernel users yet.

> 2. Moving struct tpm_rng to the TPM client is architecturally
>uacceptable.

As there was no response to the patch there is no way to know whether
it is acceptable or not.

> 3. Using zero deos not give you any better guarantees on anything than
>just using TPM_ANY_NUM.

Chip id is used, not zero.

> Why this patch is not CC'd to linux-integrity? It modifies the TPM
> driver. And in the worst way.

TPM list is moderated and the moderator has not approved it yet.
get_maintainer script did not say about linux-integrity mailing list.

It could be doing things in worst way but it is not known until some
one says. If no one tells it is the case I don't think it is possible
to fix. Which is what happened.

> Implementing the ideas that Jason explained is the senseful way to
> get stable access. modules.dep makes sure that the modules are loaded
> in the correct order.

If that is sensible then it is the way to go.

There must be a reason to believe what is sensible and what is not.
Looks like this RFC has helped in judging that.

Regards,
PrasannaKumar

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

[PrasannaKumar Muralidharan]

On 24 October 2017 at 21:41, Jason Gunthorpe
<jguntho...@obsidianresearch.com> wrote:
> On Tue, Oct 24, 2017 at 09:37:33PM +0530, PrasannaKumar Muralidharan wrote:
>> Hi Jason,
>>
>> On 24 October 2017 at 21:25, Jason Gunthorpe
>> <jguntho...@obsidianresearch.com> wrote:
>> > On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
>> >
>> >> Please check the RFC [1]. It does use chip id. The rfc has issues and
>> >> has to be fixed but still there could be users of the API.
>> >>
>> >> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>> >
>> > That patch isn't safe at all. You need to store a kref to th chip in
>> > the hwrng, not parse a string.
>>
>> The drivers/char/hw_random/tpm-rng.c module does not store the chip
>> reference so I guess the usage is safe.
>
> It is using the default TPM, it is always safe to use the default tpm.
>
>> The RFC is just a sample use case of the API.
>
> Well, a wrong example not to be emulated, and I think, further shows
> how Jarkko's direction is the right one.

I am wondering why it is wrong. Isn't the chip id valid till it is
unregistered? If so the rfc is correct. Please explain, may be I am
missing something.

Thanks,
PrasannaKumar

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

[PrasannaKumar Muralidharan]

Hi Jason,

On 24 October 2017 at 21:25, Jason Gunthorpe
<jguntho...@obsidianresearch.com> wrote:
> On Tue, Oct 24, 2017 at 09:21:15PM +0530, PrasannaKumar Muralidharan wrote:
>
>> Please check the RFC [1]. It does use chip id. The rfc has issues and
>> has to be fixed but still there could be users of the API.
>>
>> 1. https://www.spinics.net/lists/linux-crypto/msg28282.html
>
> That patch isn't safe at all. You need to store a kref to th chip in
> the hwrng, not parse a string.

The drivers/char/hw_random/tpm-rng.c module does not store the chip
reference so I guess the usage is safe. The RFC is just a sample use
case of the API.

Regards,
PrasannaKumar

Re: [tpmdd-devel] [PATCH] tpm: remove chip_num parameter from in-kernel API

[PrasannaKumar Muralidharan]

On 24 October 2017 at 21:14, Jarkko Sakkinen
 wrote:
> On Mon, Oct 23, 2017 at 10:31:39AM -0600, Jason Gunthorpe wrote:
>> On Mon, Oct 23, 2017 at 10:07:31AM -0400, Stefan Berger wrote:
>>
>> > >-int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
>> > >+int tpm_pcr_extend(int pcr_idx, const u8 *hash)
>> > >  {
>> >
>> >
>> > I think every kernel internal TPM driver API should be called with the
>> > tpm_chip as a parameter. This is in foresight of namespacing of IMA where 
>> > we
>> > want to provide the flexibility of passing a dedicated vTPM to each
>> > namespace and IMA would use the chip as a parameter to all of these
>> > functions to talk to the right tpm_vtpm_proxy instance. From that
>> > perspective this patch goes into the wrong direction.
>>
>> Yes, we should ultimately try and get to there.. Someday the
>> tpm_chip_find_get() will need to become namespace aware.
>>
>> But this patch is along the right path, eliminating the chip_num is
>> the right thing to do..
>>
>> > >-  tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
>> > >+  tpm2 = tpm_is_tpm2();
>> > >   if (tpm2 < 0)
>> > >   return tpm2;
>> > >
>> > >@@ -1008,7 +1007,7 @@ static int trusted_instantiate(struct key *key,
>> > >   switch (key_cmd) {
>> > >   case Opt_load:
>> > >   if (tpm2)
>> > >-  ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, 
>> > >options);
>> > >+  ret = tpm_unseal_trusted(payload, options);
>>
>> Sequences like this are sketchy.
>>
>> It should be
>>
>> struct tpm_chip *tpm;
>>
>> tpm = tpm_chip_find_get()
>> tpm2 = tpm_is_tpm2(tpm);
>>
>> [..]
>>
>> if (tpm2)
>>  ret = tpm_unseal_trusted(tpm, payload, options);
>>
>> [..]
>>
>> tpm_put_chip(tpm);
>>
>> As hot plug could alter the 'tpm' between the two tpm calls.
>>
>> Jason
>
> This patch just removes bunch of dead code. It does not change existing
> semantics. What you are saying should be done after the dead code has
> been removed. This commit is first step to that direction.
>
> /Jarkko

Please check the RFC [1]. It does use chip id. The rfc has issues and
has to be fixed but still there could be users of the API.

1. https://www.spinics.net/lists/linux-crypto/msg28282.html

Regards,
PrasannaKumar

Re: [PATCH v3 0/4] crypto: Add driver for JZ4780 PRNG

[PrasannaKumar Muralidharan]

Hi Herbert,

On 12 October 2017 at 20:00, Herbert Xu <herb...@gondor.apana.org.au> wrote:
> On Mon, Sep 18, 2017 at 07:32:37PM +0530, PrasannaKumar Muralidharan wrote:
>> This patch series adds support of pseudo random number generator found
>> in Ingenic's JZ4780 and X1000 SoC.
>>
>> Create cgublock node which has CGU and RNG node as its children. The
>> cgublock node uses "simple-bus" compatible which helps in exposing CGU
>> and RNG nodes without changing CGU driver. Add 'syscon' compatible in
>> CGU node in jz4780.dtsi. The jz4780-rng driver uses regmap exposed via
>> syscon interface to access the RNG registers. CGU driver is not
>> modified in this patch set as registers used by CGU driver and this
>> driver are different.
>>
>> PrasannaKumar Muralidharan (4):
>>   crypto: jz4780-rng: Add JZ4780 PRNG devicetree binding documentation
>>   crypto: jz4780-rng: Add Ingenic JZ4780 hardware PRNG driver
>>   crypto: jz4780-rng: Add RNG node to jz4780.dtsi
>>   crypto: jz4780-rng: Enable PRNG support in CI20 defconfig
>
> Please indicate which patches are intended to go through the crypto
> trees.

>From https://patchwork.linux-mips.org/patch/17162/ I expect the same.
Either all patches go via crypto tree or via mips tree.
The dtsi changes is not yet acked by MIPS / JZ4780 maintainer. Let's
wait for it.

Thanks,
PrasannaKumar

Re: [RFC PATCH] crypto: make the seed() function optional

[PrasannaKumar Muralidharan]

Hi Herbert,

On 7 October 2017 at 09:03, Herbert Xu  wrote:
> Mathieu Malaterre  wrote:
>> This makes it simplier for driver author to not provide the seed() function
>> in case of a pseudo RNG where the seed operation is a no-op.
>>
>> Document that the seed() function pointer is optional in header.
>>
>> Signed-off-by: Mathieu Malaterre 
>> ---
>> The PRNG as found on Ingenic JZ4780 is one such example. This is found on a
>> MIPS Creator CI20 SoC.
>
> So how does it seed itself? This also contradicts with the JZ4780
> driver that's currently in the patch queue as it does contain a
> seed function.

The current version of JZ4780 driver in the patch queue indeed has
seed function. But when Mathieu sent this email based on v2 of the
driver. V2 did not have seed callback. Using v2 resulted in a NULL
pointer in kernel. This patch prevents that NULL pointer access.

Regardless of what JZ4780 driver has this patch makes sense.

Currently crypto framework does not mandate seed callback's presence.
If mandatory, crypto framework should error out if seed is not
implemented while registering the PRNG.

Thanks,
PrasannaKumar

Re: [PATCH] Documentation: hw_random: Fix issue related to feeding entropy pool

[PrasannaKumar Muralidharan]

Hi Herbert,

On 22 September 2017 at 14:47, Herbert Xu <herb...@gondor.apana.org.au> wrote:
> PrasannaKumar Muralidharan <prasannatsmku...@gmail.com> wrote:
>> There is no need to use rng-tools for feeding random data into kernel
>> entropy pool as hw_random core handles it. Documentation suggested that
>> rng-tools is required which is incorrect. So remove it.
>>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>
> Hmm, the tool still exists and can still be used to feed /dev/random.
>
> Feeding /dev/random directly from the kernel is just one way of
> using hwrng.

But it is the default way.

> So rather than removing it perhaps you can modify it to state that
> the kernel can now feed /dev/random as an option.

As it is not required I think stating it is going to create confusion.
Users may not know whether rng-tools is optional or necessary for
feeding kernel entropy pool.

Thanks,
PrasannaKumar

[PATCH v3 3/4] crypto: jz4780-rng: Add RNG node to jz4780.dtsi

[PrasannaKumar Muralidharan]

Add RNG node to jz4780 dtsi. This driver uses registers that are part of
the register set used by Ingenic CGU driver. Use regmap in RNG driver to
access its register. Create 'simple-bus' node, make CGU and RNG node as
child of it so that both the nodes are visible without changing CGU
driver code.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
Changes in v3:
* Create a cgublock node with "simple-bus" compatible
* Make CGU and RNG node as children of cgublock node.

Changes in v2:  
 
* Add "syscon" in CGU node's compatible section 
 
* Make RNG child node of CGU.   
 

 arch/mips/boot/dts/ingenic/jz4780.dtsi | 25 -
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi 
b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 4853ef6..5953b97 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -34,14 +34,29 @@
clock-frequency = <32768>;
};
 
-   cgu: jz4780-cgu@1000 {
-   compatible = "ingenic,jz4780-cgu";
+   cgublock {
+   compatible = "simple-bus";
+
+   #address-cells = <1>;
+   #size-cells = <1>;
+
reg = <0x1000 0x100>;
+   ranges;
 
-   clocks = <>, <>;
-   clock-names = "ext", "rtc";
+   cgu: jz4780-cgu@0 {
+   compatible = "ingenic,jz4780-cgu";
+   reg = <0x1000 0x100>;
 
-   #clock-cells = <1>;
+   clocks = <>, <>;
+   clock-names = "ext", "rtc";
+
+   #clock-cells = <1>;
+   };
+
+   rng: rng@d8 {
+   compatible = "ingenic,jz4780-rng";
+   reg = <0x10d8 0x8>;
+   };
};
 
pinctrl: pin-controller@1001 {
-- 
2.10.0

[PATCH v3 4/4] crypto: jz4780-rng: Enable PRNG support in CI20 defconfig

[PrasannaKumar Muralidharan]

Enable PRNG driver support in MIPS Creator CI20 default config.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
No changes in v3

No changes in v2

 arch/mips/configs/ci20_defconfig | 5 +
 1 file changed, 5 insertions(+)

diff --git a/arch/mips/configs/ci20_defconfig b/arch/mips/configs/ci20_defconfig
index b42cfa7..9f48f2c 100644
--- a/arch/mips/configs/ci20_defconfig
+++ b/arch/mips/configs/ci20_defconfig
@@ -88,6 +88,11 @@ CONFIG_SERIAL_8250_RUNTIME_UARTS=5
 CONFIG_SERIAL_8250_INGENIC=y
 CONFIG_SERIAL_OF_PLATFORM=y
 # CONFIG_HW_RANDOM is not set
+CONFIG_CRYPTO_USER=y
+CONFIG_CRYPTO_USER_API=y
+CONFIG_CRYPTO_USER_API_RNG=y
+CONFIG_CRYPTO_HW=y
+CONFIG_CRYPTO_DEV_JZ4780_RNG=y
 CONFIG_I2C=y
 CONFIG_I2C_JZ4780=y
 CONFIG_GPIO_SYSFS=y
-- 
2.10.0

[PATCH v3 0/4] crypto: Add driver for JZ4780 PRNG

[PrasannaKumar Muralidharan]

This patch series adds support of pseudo random number generator found
in Ingenic's JZ4780 and X1000 SoC.

Create cgublock node which has CGU and RNG node as its children. The
cgublock node uses "simple-bus" compatible which helps in exposing CGU
and RNG nodes without changing CGU driver. Add 'syscon' compatible in
CGU node in jz4780.dtsi. The jz4780-rng driver uses regmap exposed via
syscon interface to access the RNG registers. CGU driver is not
modified in this patch set as registers used by CGU driver and this
driver are different.

PrasannaKumar Muralidharan (4):
  crypto: jz4780-rng: Add JZ4780 PRNG devicetree binding documentation
  crypto: jz4780-rng: Add Ingenic JZ4780 hardware PRNG driver
  crypto: jz4780-rng: Add RNG node to jz4780.dtsi
  crypto: jz4780-rng: Enable PRNG support in CI20 defconfig

 .../devicetree/bindings/rng/ingenic,jz4780-rng.txt |  21 +++
 MAINTAINERS|   7 +
 arch/mips/boot/dts/ingenic/jz4780.dtsi |  25 ++-
 arch/mips/configs/ci20_defconfig   |   5 +
 drivers/crypto/Kconfig |  19 ++
 drivers/crypto/Makefile|   1 +
 drivers/crypto/jz4780-rng.c| 193 +
 7 files changed, 266 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
 create mode 100644 drivers/crypto/jz4780-rng.c

-- 
2.10.0

[PATCH v3 2/4] crypto: jz4780-rng: Add Ingenic JZ4780 hardware PRNG driver

[PrasannaKumar Muralidharan]

JZ4780 SoC pseudo random number generator driver using crypto framework.

Adding a delay before reading RNG data and disabling RNG after reading
data was suggested by Jeffery Walton.

Tested-by: Mathieu Malaterre <ma...@debian.org>
Suggested-by: Jeffrey Walton <noloa...@gmail.com>
Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
Changes in v3:
* Add seeding support
* Reduce delay

Changes in v2:  

* Fixed buffer overflow in generate function pointed out in Stephan's review

* Fold patch that had only MAINTAINERS file change with this patch  

* Removed unnecessary comment in code   


 MAINTAINERS |   7 ++
 drivers/crypto/Kconfig  |  19 +
 drivers/crypto/Makefile |   1 +
 drivers/crypto/jz4780-rng.c | 193 
 4 files changed, 220 insertions(+)
 create mode 100644 drivers/crypto/jz4780-rng.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 2093060..d2341a7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6783,6 +6783,13 @@ L:   linux-...@lists.infradead.org
 S: Maintained
 F: drivers/mtd/nand/jz4780_*
 
+INGENIC JZ4780 PRNG DRIVER
+M: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
+L: linux-crypto@vger.kernel.org
+S: Maintained
+F: drivers/crypto/jz4780-rng.c
+F: Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
+
 INOTIFY
 M: Jan Kara <j...@suse.cz>
 R: Amir Goldstein <amir7...@gmail.com>
diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index fe33c19..f3ac1cd 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -613,6 +613,25 @@ config CRYPTO_DEV_IMGTEC_HASH
  hardware hash accelerator. Supporting MD5/SHA1/SHA224/SHA256
  hashing algorithms.
 
+config CRYPTO_DEV_JZ4780_RNG
+   tristate "JZ4780 HW pseudo random number generator support"
+   depends on MACH_JZ4780 || COMPILE_TEST
+   depends on HAS_IOMEM
+   select CRYPTO_RNG
+   select REGMAP
+   select SYSCON
+   select MFD_SYSCON
+   ---help---
+ This driver provides kernel-side support through the
+ cryptographic API for the pseudo random number generator
+ hardware found in ingenic JZ4780 and X1000 SoC. MIPS
+ Creator CI20 uses JZ4780 SoC.
+
+ To compile this driver as a module, choose M here: the
+ module will be called jz4780-rng.
+
+ If unsure, say Y.
+
 config CRYPTO_DEV_SUN4I_SS
tristate "Support for Allwinner Security System cryptographic 
accelerator"
depends on ARCH_SUNXI && !64BIT
diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
index 808432b..a09d9f4 100644
--- a/drivers/crypto/Makefile
+++ b/drivers/crypto/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_CRYPTO_DEV_GEODE) += geode-aes.o
 obj-$(CONFIG_CRYPTO_DEV_HIFN_795X) += hifn_795x.o
 obj-$(CONFIG_CRYPTO_DEV_IMGTEC_HASH) += img-hash.o
 obj-$(CONFIG_CRYPTO_DEV_IXP4XX) += ixp4xx_crypto.o
+obj-$(CONFIG_CRYPTO_DEV_JZ4780_RNG) += jz4780-rng.o
 obj-$(CONFIG_CRYPTO_DEV_MV_CESA) += mv_cesa.o
 obj-$(CONFIG_CRYPTO_DEV_MARVELL_CESA) += marvell/
 obj-$(CONFIG_CRYPTO_DEV_MEDIATEK) += mediatek/
diff --git a/drivers/crypto/jz4780-rng.c b/drivers/crypto/jz4780-rng.c
new file mode 100644
index 000..918ba94
--- /dev/null
+++ b/drivers/crypto/jz4780-rng.c
@@ -0,0 +1,193 @@
+/*
+ * jz4780-rng.c - Random Number Generator driver for the jz4780
+ *
+ * Copyright (c) 2017 PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
+ *
+ * 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.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#define REG_RNG_CTRL   0xD8
+#define REG_RNG_DATA   0xDC
+
+/* Context for crypto */
+struct jz4780_rng_ctx {
+   struct jz4780_rng *rng;
+};
+
+/* Device associated memory */
+struct jz4780_rng {
+   struct device *dev;
+   struct regmap *regmap;
+   u32 seed;
+};
+
+static struct jz4780_rng *jz4780_rng;
+
+static int jz4780_rng_readl(struct jz4780_rng *rng, u32 offset)
+{
+   u32 val = 0;
+   int ret;
+
+   ret = regmap_read(rng->regmap, offset, );
+   if (!ret)
+   return val;
+
+   return ret;
+}
+
+static int jz4780_rng_writel(struct jz4780_rng *rng, u32 val, u32 offset)
+{
+   return regmap_write(rng->regmap, offset, val);
+}
+
+static int jz4780_rng_seed(struct crypto_rng *tfm, con

[PATCH v3 1/4] crypto: jz4780-rng: Add JZ4780 PRNG devicetree binding documentation

[PrasannaKumar Muralidharan]

Add devicetree bindings for hardware pseudo random number generator
present in Ingenic JZ4780 SoC.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
Changes in v3:
* Create a cgublock node with "simple-bus" compatible
* Make CGU and RNG node as children of cgublock node.

Changes in v2:  
 
* Add "syscon" in CGU node's compatible section 
 
* Make RNG child node of CGU.   
 

 .../devicetree/bindings/rng/ingenic,jz4780-rng.txt  | 21 +
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt

diff --git a/Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt 
b/Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
new file mode 100644
index 000..765df9c
--- /dev/null
+++ b/Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
@@ -0,0 +1,21 @@
+Ingenic jz4780 RNG driver
+
+Required properties:
+- compatible : Should be "ingenic,jz4780-rng"
+
+Example:
+
+cgublock {
+   compatible = "simple-bus";
+
+   #address-cells = <1>;
+   #size-cells = <1>;
+
+   reg = <0x1000 0x100>;
+   ranges;
+
+   rng: rng@d8 {
+   compatible = "ingenic,jz4780-rng";
+   reg = <0x10d8 0x8>;
+   };
+};
-- 
2.10.0

[RFC] tpm: Register RNG device only on tpm chip init

[PrasannaKumar Muralidharan]

RNG device is registered as soon as tpm-rng module is loaded even if
there are no TPM chip available. Call hwrng_register once tpm chip has
registered.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 drivers/char/hw_random/tpm-rng.c | 50 
 drivers/char/tpm/tpm-chip.c  | 24 +++
 drivers/char/tpm/tpm.h   |  3 +++
 3 files changed, 27 insertions(+), 50 deletions(-)
 delete mode 100644 drivers/char/hw_random/tpm-rng.c

diff --git a/drivers/char/hw_random/tpm-rng.c b/drivers/char/hw_random/tpm-rng.c
deleted file mode 100644
index d6d4482..000
--- a/drivers/char/hw_random/tpm-rng.c
+++ /dev/null
@@ -1,50 +0,0 @@
-/*
- * Copyright (C) 2012 Kent Yoder IBM Corporation
- *
- * HWRNG interfaces to pull RNG data from a TPM
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 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 
-
-#define MODULE_NAME "tpm-rng"
-
-static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
-{
-   return tpm_get_random(TPM_ANY_NUM, data, max);
-}
-
-static struct hwrng tpm_rng = {
-   .name = MODULE_NAME,
-   .read = tpm_rng_read,
-};
-
-static int __init rng_init(void)
-{
-   return hwrng_register(_rng);
-}
-module_init(rng_init);
-
-static void __exit rng_exit(void)
-{
-   hwrng_unregister(_rng);
-}
-module_exit(rng_exit);
-
-MODULE_LICENSE("GPL v2");
-MODULE_AUTHOR("Kent Yoder <k...@linux.vnet.ibm.com>");
-MODULE_DESCRIPTION("RNG driver for TPM devices");
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 0eca20c..bf9c2ad 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "tpm.h"
 #include "tpm_eventlog.h"
 
@@ -387,6 +388,15 @@ static int tpm_add_legacy_sysfs(struct tpm_chip *chip)
 
return 0;
 }
+
+static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
+{
+   int chip_num = 0;
+
+   kstrtoint(>name[8], 10, _num);
+   return tpm_get_random(chip_num, data, max);
+}
+
 /*
  * tpm_chip_register() - create a character device for the TPM chip
  * @chip: TPM chip to use.
@@ -401,6 +411,7 @@ static int tpm_add_legacy_sysfs(struct tpm_chip *chip)
 int tpm_chip_register(struct tpm_chip *chip)
 {
int rc;
+   char *tpm_rng_name;
 
if (chip->ops->flags & TPM_OPS_AUTO_STARTUP) {
if (chip->flags & TPM_CHIP_FLAG_TPM2)
@@ -431,6 +442,14 @@ int tpm_chip_register(struct tpm_chip *chip)
return rc;
}
 
+   tpm_rng_name = kmalloc(64, GFP_KERNEL);
+
+   if (tpm_rng_name) {
+   sprintf(tpm_rng_name, "tpm-rng-%d", chip->dev_num);
+   tpm_rng.name = tpm_rng_name;
+   tpm_rng.read = tpm_rng_read;
+   hwrng_register(_rng);
+   }
return 0;
 }
 EXPORT_SYMBOL_GPL(tpm_chip_register);
@@ -450,6 +469,11 @@ EXPORT_SYMBOL_GPL(tpm_chip_register);
  */
 void tpm_chip_unregister(struct tpm_chip *chip)
 {
+   if (chip->tpm_rng.name[0] != 0) {
+   hwrng_unregister(_rng);
+   kfree(chip->tpm_rng.name);
+   chip->tpm_rng.name[0] = 0;
+   }
tpm_del_legacy_sysfs(chip);
tpm_bios_log_teardown(chip);
if (chip->flags & TPM_CHIP_FLAG_TPM2)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index b50e92f..ca042f7 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -210,6 +211,8 @@ struct tpm_chip {
int dev_num;/* /dev/tpm# */
unsigned long is_open;  /* only one allowed */
 
+   struct hwrng tpm_rng;
+
struct mutex tpm_mutex; /* tpm is processing */
 
unsigned long timeout_a; /* jiffies */
-- 
2.10.0

Re: HWRNGs without quality attribute - are they used or not?

[PrasannaKumar Muralidharan]

Hi Peter,

On 5 September 2017 7:24:02 PM IST, Peter Huewe  wrote:
>Hi,
>
>while reading through the analysis of the Linux RNG by the BSI [1][2],
>I was suprised by the lack of reflecting on the usage of HWRNGs except
>RDRAND/RDSEED.
>
>In the paper it was mentioned that if the quality attribute of a
>regular hwrng is not set
>(or specified via the module parameter default_quality for all hwrng)
>the HWRNG is not used at all.
>Only very few set this attribute explictly, and the default is 0,
>so even if we enable these HWRNGs in the kernel config, they are not
>used?
>
>I read through the hw_random code and came to the same conclusion - but
>is this correct and desired?

If the quality of RNG source is not set or set to 0 then hwrng does not use the 
data to seed /dev/random.

>Specifically since you can set the 'default value' only for every hwrng
>but not overwrite the quality for a specific device.
>e.g. the quite good tpm hwrng of my ifx tpm would not be used and 
>if I set the default_quality to something other than 0, other devices
>would be used, which I would not want to.

Device with high quality will be used by default. But user space can tell hwrng 
to use a particular device even if it is not the best RNG source. But it won't 
be used for seeing /dev/random even in this case.

Every instance of hw_random structure represents a RNG device. If you can use 
different instance for every TPM chip then setting different quality value is 
possible.

I have an untested patch that allocates different hw_random structure instance 
for every TPM chip and registers with hwrng on tpm_chip_register. I made the 
change for fixing some other issue but did not get time to test it. If that 
patch will be helpful I can find it out and post the patch as RFC tomorrow.

>
>Is this understanding correct?
>
>
>Thanks,
>Peter
>(tpm maintainer)
>
>
>
>[1]
>https://www.bsi.bund.de/DE/Publikationen/Studien/LinuxRNG/index_htm.html
>[2]
>https://www.bsi.bund.de/SharedDocs/Downloads/DE/BSI/Publikationen/Studien/LinuxRNG/LinuxRNG_EN.pdf?__blob=publicationFile=5

Regards,
PrasannaKumar
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v2 0/4] crypto: Add driver for JZ4780 PRNG

[PrasannaKumar Muralidharan]

On 23 August 2017 at 08:27, PrasannaKumar Muralidharan
<prasannatsmku...@gmail.com> wrote:
> This patch series adds support of pseudo random number generator found
> in Ingenic's JZ4780 and X1000 SoC.
>
> Based on Paul's review comments, add 'syscon' compatible in CGU node in
> jz4780.dtsi. jz4780-rng driver uses regmap exposed via syscon interface
> to access the RNG registers. CGU driver is not modified in this patch
> set as registers used by CGU driver and this driver are different.
>
> PrasannaKumar Muralidharan (4):
>   crypto: jz4780-rng: Add JZ4780 PRNG devicetree binding documentation
>   crypto: jz4780-rng: Add Ingenic JZ4780 hardware PRNG driver
>   crypto: jz4780-rng: Add RNG node to jz4780.dtsi
>   crypto: jz4780-rng: Enable PRNG support in CI20 defconfig
>
>  .../bindings/crypto/ingenic,jz4780-rng.txt |  20 +++
>  MAINTAINERS|   5 +
>  arch/mips/boot/dts/ingenic/jz4780.dtsi |   6 +-
>  arch/mips/configs/ci20_defconfig   |   5 +
>  drivers/crypto/Kconfig |  19 +++
>  drivers/crypto/Makefile|   1 +
>  drivers/crypto/jz4780-rng.c| 168 
> +
>  7 files changed, 223 insertions(+), 1 deletion(-)
>  create mode 100644 
> Documentation/devicetree/bindings/crypto/ingenic,jz4780-rng.txt
>  create mode 100644 drivers/crypto/jz4780-rng.c
>
> --
> 2.10.0
>

The rng node which is the child node of CGU is ignored so the driver's
probe is not called at all. Realised that the tests were using crng
instead of this. Don't know how to get this working. Will submit a new
version once the issue is fixed.

Please do not take this patch series.

Thanks,
PrasannaKumar

Re: [PATCH v2 1/4] crypto: jz4780-rng: Add JZ4780 PRNG devicetree binding documentation

[PrasannaKumar Muralidharan]

Hi Rob,

On 26 August 2017 at 03:27, Rob Herring <r...@kernel.org> wrote:
> On Wed, Aug 23, 2017 at 08:27:04AM +0530, PrasannaKumar Muralidharan wrote:
>> Add devicetree bindings for hardware pseudo random number generator
>> present in Ingenic JZ4780 SoC.
>>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> ---
>> Changes in v2:
>> * Add "syscon" in CGU node's compatible section
>> * Make RNG child node of CGU.
>>
>>  .../bindings/crypto/ingenic,jz4780-rng.txt   | 20 
>> 
>
> bindings/rng/ for RNG h/w.

There are two subsystem for dealing with RNG hw. Hw_random subsystem
for true RNG (driver/char/hw_random) and crypto framework for pseudo
RNG (crypto/ and drviers/crypto). This HW is pseudo RNG so I have
placed the dt bindings in bindings/crypto as the driver itself is in
drivers/crypto folder. I am wondering if there is any relation between
driver folder and bindings folder. Can you please explain the folder
relation? Should this be put in bindings/rng or bindings/crypto?

>
>>  1 file changed, 20 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/crypto/ingenic,jz4780-rng.txt
>>
>> diff --git a/Documentation/devicetree/bindings/crypto/ingenic,jz4780-rng.txt 
>> b/Documentation/devicetree/bindings/crypto/ingenic,jz4780-rng.txt
>> new file mode 100644
>> index 000..a0c18e5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/crypto/ingenic,jz4780-rng.txt
>> @@ -0,0 +1,20 @@
>> +Ingenic jz4780 RNG driver
>> +
>> +Required properties:
>> +- compatible : Should be "ingenic,jz4780-rng"
>> +
>> +Example:
>> +
>> +cgu: jz4780-cgu@1000 {
>> + compatible = "ingenic,jz4780-cgu", "syscon";
>> + reg = <0x1000 0x100>;
>> +
>> + clocks = <>, <>;
>> + clock-names = "ext", "rtc";
>> +
>> + #clock-cells = <1>;
>> +
>> + rng: rng@d8 {
>
> unit-address requires reg property.

The driver uses regmap to access the registers. In this case reg
property is not useful. Is reg property still needed? If not, how
should the node be declared?

>
>> + compatible = "ingenic,jz480-rng";
>> + };
>> +};
>> --
>> 2.10.0
>>

Thanks,
PrasannaKumar

Re: [PATCH v2] dt/bindings: exynos-rng: Move dt binding documentation to bindings/crypto

[PrasannaKumar Muralidharan]

On 23 August 2017 at 22:24, Krzysztof Kozlowski <k...@kernel.org> wrote:
> On Wed, Aug 23, 2017 at 10:14:29PM +0530, PrasannaKumar Muralidharan wrote:
>> Hi Krzysztof,
>>
>> On 23 August 2017 at 21:42, Krzysztof Kozlowski <k...@kernel.org> wrote:
>> > On Wed, Aug 23, 2017 at 08:34:43PM +0530, PrasannaKumar Muralidharan wrote:
>> >> Samsung exynos PRNG driver is using crypto framework instead of
>> >> hw_random framework. So move the devicetree binding to crypto folder.
>> >>
>> >> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> >> ---
>> >> Changes in v2:
>> >> * Modify MAINTAINERS file to reflect file rename
>> >>
>> >>  .../devicetree/bindings/{rng => crypto}/samsung,exynos-rng4.txt 
>> >> | 0
>> >>  MAINTAINERS 
>> >> | 2 +-
>> >>  2 files changed, 1 insertion(+), 1 deletion(-)
>> >>  rename Documentation/devicetree/bindings/{rng => 
>> >> crypto}/samsung,exynos-rng4.txt (100%)
>> >>
>> >
>> > Patch is okay but CC list is still incomplete. I do not know how you
>> > could get Mauro for this patch... just use get_maintainers.pl.
>>
>> I used get_maintainer.pl. Rechecked again, it says Mauro and couple of
>> others who may not be interested in this. It did not give your or
>> crypto mailing list. Its strange.
>
> Hm, you're right. Aparently get_maintainer.pl does handle moved files
> thus he printed only entries for MAINTAINERS.
>
> You will get full entry with:
> scripts/get_maintainer.pl -f 
> Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt

Will keep this in mind.

> Krzysztof Kozlowski <k...@kernel.org> (maintainer:SAMSUNG EXYNOS PSEUDO 
> RANDOM NUMBER GENERATOR (...)
> Herbert Xu <herb...@gondor.apana.org.au> (maintainer:CRYPTO API)
> "David S. Miller" <da...@davemloft.net> (maintainer:CRYPTO API)
> Rob Herring <robh...@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED 
> DEVICE TREE BINDINGS)
> Mark Rutland <mark.rutl...@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED 
> DEVICE TREE BINDINGS)
> Kukjin Kim <kg...@kernel.org> (maintainer:ARM/SAMSUNG EXYNOS ARM 
> ARCHITECTURES)
> linux-crypto@vger.kernel.org (open list:SAMSUNG EXYNOS PSEUDO RANDOM NUMBER 
> GENERATOR (...)
> linux-samsung-...@vger.kernel.org (open list:SAMSUNG EXYNOS PSEUDO RANDOM 
> NUMBER GENERATOR (...)
> devicet...@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE 
> BINDINGS)
> linux-arm-ker...@lists.infradead.org (moderated list:ARM/SAMSUNG EXYNOS ARM 
> ARCHITECTURES)
> linux-ker...@vger.kernel.org (open list)
>
> Best regards,
> Krzysztof
>

Thanks,
PrasannaKumar

Re: [PATCH v2] dt/bindings: exynos-rng: Move dt binding documentation to bindings/crypto

[PrasannaKumar Muralidharan]

Hi Krzysztof,

On 23 August 2017 at 21:42, Krzysztof Kozlowski <k...@kernel.org> wrote:
> On Wed, Aug 23, 2017 at 08:34:43PM +0530, PrasannaKumar Muralidharan wrote:
>> Samsung exynos PRNG driver is using crypto framework instead of
>> hw_random framework. So move the devicetree binding to crypto folder.
>>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> ---
>> Changes in v2:
>> * Modify MAINTAINERS file to reflect file rename
>>
>>  .../devicetree/bindings/{rng => crypto}/samsung,exynos-rng4.txt | 0
>>  MAINTAINERS | 2 
>> +-
>>  2 files changed, 1 insertion(+), 1 deletion(-)
>>  rename Documentation/devicetree/bindings/{rng => 
>> crypto}/samsung,exynos-rng4.txt (100%)
>>
>
> Patch is okay but CC list is still incomplete. I do not know how you
> could get Mauro for this patch... just use get_maintainers.pl.

I used get_maintainer.pl. Rechecked again, it says Mauro and couple of
others who may not be interested in this. It did not give your or
crypto mailing list. Its strange.

> Reviewed-by: Krzysztof Kozlowski <k...@kernel.org>

Thanks for the review.

> Best regards,
> Krzysztof
>

Re: [PATCH v2 4/4] crypto: jz4780-rng: Enable PRNG support in CI20 defconfig

[PrasannaKumar Muralidharan]

Hi Harvey,

On 23 August 2017 at 20:21, Harvey Hunt <harvey.h...@imgtec.com> wrote:
> Hi PrasannaKumar,
>
>
> On 23/08/17 15:50, PrasannaKumar Muralidharan wrote:
>>
>> Hi Harvey,
>>
>> On 23 August 2017 at 14:39, Harvey Hunt <harvey.h...@imgtec.com> wrote:
>>>
>>> Hi PrasannaKumar,
>>>
>>> On 23/08/17 03:57, PrasannaKumar Muralidharan wrote:
>>>>
>>>>
>>>> Enable PRNG driver support in MIPS Creator CI20 default config.
>>>>
>>>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>>>> ---
>>>> No changes in v2
>>>>
>>>>arch/mips/configs/ci20_defconfig | 5 +
>>>>1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/arch/mips/configs/ci20_defconfig
>>>> b/arch/mips/configs/ci20_defconfig
>>>> index b42cfa7..9f48f2c 100644
>>>> --- a/arch/mips/configs/ci20_defconfig
>>>> +++ b/arch/mips/configs/ci20_defconfig
>>>> @@ -88,6 +88,11 @@ CONFIG_SERIAL_8250_RUNTIME_UARTS=5
>>>>CONFIG_SERIAL_8250_INGENIC=y
>>>>CONFIG_SERIAL_OF_PLATFORM=y
>>>># CONFIG_HW_RANDOM is not set
>>>> +CONFIG_CRYPTO_USER=y
>>>> +CONFIG_CRYPTO_USER_API=y
>>>> +CONFIG_CRYPTO_USER_API_RNG=y
>>>> +CONFIG_CRYPTO_HW=y
>>>> +CONFIG_CRYPTO_DEV_JZ4780_RNG=y
>>>>CONFIG_I2C=y
>>>>CONFIG_I2C_JZ4780=y
>>>>CONFIG_GPIO_SYSFS=y
>>>>
>>>
>>> You need to regenerate your defconfig as it is missing CONFIG_MFD_SYSCON.
>>>
>>> Thanks,
>>>
>>> Harvey
>>
>>
>> CONFIG_MFD_SYSCON gets selected when CONFIG_CRYPTO_DEV_JZ4780_RNG is
>> selected. Please see the Kconfig changes. Given that should I add it
>> in ci20_defconfig? If it is required I will add and send a new
>> version.
>
>
> Oops, I hadn't noticed that - just skimmed the patches before my morning
> coffee. :-)
>
> It's fine as is, excuse the noise.

No issues.

>
>>
>> Thanks,
>> PrasannaKumar
>>
>
> Thanks,
>
> Harvey
>

Regards,
PrasannaKumar

[PATCH v2] dt/bindings: exynos-rng: Move dt binding documentation to bindings/crypto

[PrasannaKumar Muralidharan]

Samsung exynos PRNG driver is using crypto framework instead of
hw_random framework. So move the devicetree binding to crypto folder.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
Changes in v2:
* Modify MAINTAINERS file to reflect file rename

 .../devicetree/bindings/{rng => crypto}/samsung,exynos-rng4.txt | 0
 MAINTAINERS | 2 +-
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename Documentation/devicetree/bindings/{rng => 
crypto}/samsung,exynos-rng4.txt (100%)

diff --git a/Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt 
b/Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
similarity index 100%
rename from Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
rename to Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
diff --git a/MAINTAINERS b/MAINTAINERS
index 4d284c7..3e0b822 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11500,7 +11500,7 @@ 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
+F: Documentation/devicetree/bindings/crypto/samsung,exynos-rng4.txt
 
 SAMSUNG FRAMEBUFFER DRIVER
 M: Jingoo Han <jingooh...@gmail.com>
-- 
2.10.0

Re: [PATCH v2 4/4] crypto: jz4780-rng: Enable PRNG support in CI20 defconfig

[PrasannaKumar Muralidharan]

Hi Harvey,

On 23 August 2017 at 14:39, Harvey Hunt <harvey.h...@imgtec.com> wrote:
> Hi PrasannaKumar,
>
> On 23/08/17 03:57, PrasannaKumar Muralidharan wrote:
>>
>> Enable PRNG driver support in MIPS Creator CI20 default config.
>>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> ---
>> No changes in v2
>>
>>   arch/mips/configs/ci20_defconfig | 5 +
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/mips/configs/ci20_defconfig
>> b/arch/mips/configs/ci20_defconfig
>> index b42cfa7..9f48f2c 100644
>> --- a/arch/mips/configs/ci20_defconfig
>> +++ b/arch/mips/configs/ci20_defconfig
>> @@ -88,6 +88,11 @@ CONFIG_SERIAL_8250_RUNTIME_UARTS=5
>>   CONFIG_SERIAL_8250_INGENIC=y
>>   CONFIG_SERIAL_OF_PLATFORM=y
>>   # CONFIG_HW_RANDOM is not set
>> +CONFIG_CRYPTO_USER=y
>> +CONFIG_CRYPTO_USER_API=y
>> +CONFIG_CRYPTO_USER_API_RNG=y
>> +CONFIG_CRYPTO_HW=y
>> +CONFIG_CRYPTO_DEV_JZ4780_RNG=y
>>   CONFIG_I2C=y
>>   CONFIG_I2C_JZ4780=y
>>   CONFIG_GPIO_SYSFS=y
>>
>
> You need to regenerate your defconfig as it is missing CONFIG_MFD_SYSCON.
>
> Thanks,
>
> Harvey

CONFIG_MFD_SYSCON gets selected when CONFIG_CRYPTO_DEV_JZ4780_RNG is
selected. Please see the Kconfig changes. Given that should I add it
in ci20_defconfig? If it is required I will add and send a new
version.

Thanks,
PrasannaKumar

Re: [PATCH] hw_random: timeriomem-rng: Remove 'max < 4' condition check

[PrasannaKumar Muralidharan]

Hi Rick,

On 22 August 2017 at 22:23, Rick Altherr <ralth...@google.com> wrote:
> On Tue, Aug 22, 2017 at 9:22 AM, PrasannaKumar Muralidharan
> <prasannatsmku...@gmail.com> wrote:
>>
>> In read routiene max is always >= 4. The check whether 'max < 4' is not
>> necessary. Remove it.
>
> Missed that in the header.

It was added recently so you could have not missed it.

>
> Acked-By: Rick Altherr <ralth...@google.com>
>
>>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> ---
>>  drivers/char/hw_random/timeriomem-rng.c | 7 ---
>>  1 file changed, 7 deletions(-)
>>
>> diff --git a/drivers/char/hw_random/timeriomem-rng.c 
>> b/drivers/char/hw_random/timeriomem-rng.c
>> index 03ff548..f615684 100644
>> --- a/drivers/char/hw_random/timeriomem-rng.c
>> +++ b/drivers/char/hw_random/timeriomem-rng.c
>> @@ -53,13 +53,6 @@ static int timeriomem_rng_read(struct hwrng *hwrng, void 
>> *data,
>> int period_us = ktime_to_us(priv->period);
>>
>> /*
>> -* The RNG provides 32-bits per read.  Ensure there is enough space 
>> for
>> -* at minimum one read.
>> -*/
>> -   if (max < sizeof(u32))
>> -   return 0;
>> -
>> -   /*
>>  * There may not have been enough time for new data to be generated
>>  * since the last request.  If the caller doesn't want to wait, let 
>> them
>>  * bail out.  Otherwise, wait for the completion.  If the new data 
>> has
>> --
>> 2.10.0
>>

Regards,
PrasannaKumar

[PATCH v2 4/4] crypto: jz4780-rng: Enable PRNG support in CI20 defconfig

[PrasannaKumar Muralidharan]

Enable PRNG driver support in MIPS Creator CI20 default config.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
No changes in v2

 arch/mips/configs/ci20_defconfig | 5 +
 1 file changed, 5 insertions(+)

diff --git a/arch/mips/configs/ci20_defconfig b/arch/mips/configs/ci20_defconfig
index b42cfa7..9f48f2c 100644
--- a/arch/mips/configs/ci20_defconfig
+++ b/arch/mips/configs/ci20_defconfig
@@ -88,6 +88,11 @@ CONFIG_SERIAL_8250_RUNTIME_UARTS=5
 CONFIG_SERIAL_8250_INGENIC=y
 CONFIG_SERIAL_OF_PLATFORM=y
 # CONFIG_HW_RANDOM is not set
+CONFIG_CRYPTO_USER=y
+CONFIG_CRYPTO_USER_API=y
+CONFIG_CRYPTO_USER_API_RNG=y
+CONFIG_CRYPTO_HW=y
+CONFIG_CRYPTO_DEV_JZ4780_RNG=y
 CONFIG_I2C=y
 CONFIG_I2C_JZ4780=y
 CONFIG_GPIO_SYSFS=y
-- 
2.10.0

[PATCH v2 3/4] crypto: jz4780-rng: Add RNG node to jz4780.dtsi

[PrasannaKumar Muralidharan]

Add RNG node to jz4780 dtsi. This driver uses registers that are part of
the register set used by Ingenic CGU driver. Make RNG node as child of
CGU node.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
Changes in v2:
* Add "syscon" in CGU node's compatible section
* Make RNG child node of CGU.

 arch/mips/boot/dts/ingenic/jz4780.dtsi | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi 
b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 4853ef6..411e16c 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -35,13 +35,17 @@
};
 
cgu: jz4780-cgu@1000 {
-   compatible = "ingenic,jz4780-cgu";
+   compatible = "ingenic,jz4780-cgu", "syscon";
reg = <0x1000 0x100>;
 
clocks = <>, <>;
clock-names = "ext", "rtc";
 
#clock-cells = <1>;
+
+   rng: rng@d8 {
+   compatible = "ingenic,jz480-rng";
+   };
};
 
pinctrl: pin-controller@1001 {
-- 
2.10.0

[PATCH v2 1/4] crypto: jz4780-rng: Add JZ4780 PRNG devicetree binding documentation

[PrasannaKumar Muralidharan]

Add devicetree bindings for hardware pseudo random number generator
present in Ingenic JZ4780 SoC.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
Changes in v2:
* Add "syscon" in CGU node's compatible section
* Make RNG child node of CGU.

 .../bindings/crypto/ingenic,jz4780-rng.txt   | 20 
 1 file changed, 20 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/crypto/ingenic,jz4780-rng.txt

diff --git a/Documentation/devicetree/bindings/crypto/ingenic,jz4780-rng.txt 
b/Documentation/devicetree/bindings/crypto/ingenic,jz4780-rng.txt
new file mode 100644
index 000..a0c18e5
--- /dev/null
+++ b/Documentation/devicetree/bindings/crypto/ingenic,jz4780-rng.txt
@@ -0,0 +1,20 @@
+Ingenic jz4780 RNG driver
+
+Required properties:
+- compatible : Should be "ingenic,jz4780-rng"
+
+Example:
+
+cgu: jz4780-cgu@1000 {
+   compatible = "ingenic,jz4780-cgu", "syscon";
+   reg = <0x1000 0x100>;
+
+   clocks = <>, <>;
+   clock-names = "ext", "rtc";
+
+   #clock-cells = <1>;
+
+   rng: rng@d8 {
+   compatible = "ingenic,jz480-rng";
+   };
+};
-- 
2.10.0

[PATCH v2 2/4] crypto: jz4780-rng: Add Ingenic JZ4780 hardware PRNG driver

[PrasannaKumar Muralidharan]

JZ4780 SoC pseudo random number generator driver using crypto framework.

Adding a delay before reading RNG data and disabling RNG after reading
data was suggested by Jeffery Walton.

Tested-by: Mathieu Malaterre <ma...@debian.org>
Suggested-by: Jeffrey Walton <noloa...@gmail.com>
Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
Changes in v2:
* Fixed buffer overflow in generate function pointed out in Stephan's review
* Fold patch that had only MAINTAINERS file change with this patch
* Removed unnecessary comment in code

 MAINTAINERS |   5 ++
 drivers/crypto/Kconfig  |  19 +
 drivers/crypto/Makefile |   1 +
 drivers/crypto/jz4780-rng.c | 168 
 4 files changed, 193 insertions(+)
 create mode 100644 drivers/crypto/jz4780-rng.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 1c3feff..f662a70 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6672,6 +6672,11 @@ L:   linux-...@lists.infradead.org
 S: Maintained
 F: drivers/mtd/nand/jz4780_*
 
+INGENIC JZ4780 PRNG DRIVER
+M: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
+S: Maintained
+F: drivers/crypto/jz4780-rng.c
+
 INOTIFY
 M: John McCutchan <j...@johnmccutchan.com>
 M: Robert Love <rl...@rlove.org>
diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 4b75084..de2459f 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -599,6 +599,25 @@ config CRYPTO_DEV_IMGTEC_HASH
  hardware hash accelerator. Supporting MD5/SHA1/SHA224/SHA256
  hashing algorithms.
 
+config CRYPTO_DEV_JZ4780_RNG
+   tristate "JZ4780 HW pseudo random number generator support"
+   depends on MACH_JZ4780 || COMPILE_TEST
+   depends on HAS_IOMEM
+   select CRYPTO_RNG
+   select REGMAP
+   select SYSCON
+   select MFD_SYSCON
+   ---help---
+ This driver provides kernel-side support through the
+ cryptographic API for the pseudo random number generator
+ hardware found in ingenic JZ4780 and X1000 SoC. MIPS
+ Creator CI20 uses JZ4780 SoC.
+
+ To compile this driver as a module, choose M here: the
+ module will be called jz4780-rng.
+
+ If unsure, say Y.
+
 config CRYPTO_DEV_SUN4I_SS
tristate "Support for Allwinner Security System cryptographic 
accelerator"
depends on ARCH_SUNXI && !64BIT
diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
index 2c555a3..1df48f9 100644
--- a/drivers/crypto/Makefile
+++ b/drivers/crypto/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_CRYPTO_DEV_GEODE) += geode-aes.o
 obj-$(CONFIG_CRYPTO_DEV_HIFN_795X) += hifn_795x.o
 obj-$(CONFIG_CRYPTO_DEV_IMGTEC_HASH) += img-hash.o
 obj-$(CONFIG_CRYPTO_DEV_IXP4XX) += ixp4xx_crypto.o
+obj-$(CONFIG_CRYPTO_DEV_JZ4780_RNG) += jz4780-rng.o
 obj-$(CONFIG_CRYPTO_DEV_MV_CESA) += mv_cesa.o
 obj-$(CONFIG_CRYPTO_DEV_MARVELL_CESA) += marvell/
 obj-$(CONFIG_CRYPTO_DEV_MEDIATEK) += mediatek/
diff --git a/drivers/crypto/jz4780-rng.c b/drivers/crypto/jz4780-rng.c
new file mode 100644
index 000..682
--- /dev/null
+++ b/drivers/crypto/jz4780-rng.c
@@ -0,0 +1,168 @@
+/*
+ * jz4780-rng.c - Random Number Generator driver for the jz4780
+ *
+ * Copyright (c) 2017 PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
+ *
+ * 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.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#define REG_RNG_CTRL   0xD8
+#define REG_RNG_DATA   0xDC
+
+/* Context for crypto */
+struct jz4780_rng_ctx {
+   struct jz4780_rng *rng;
+};
+
+/* Device associated memory */
+struct jz4780_rng {
+   struct device *dev;
+   struct regmap *regmap;
+};
+
+static struct jz4780_rng *jz4780_rng;
+
+static int jz4780_rng_readl(struct jz4780_rng *rng, u32 offset)
+{
+   u32 val = 0;
+   int ret;
+
+   ret = regmap_read(rng->regmap, offset, );
+   if (!ret)
+   return val;
+
+   return ret;
+}
+
+static int jz4780_rng_writel(struct jz4780_rng *rng, u32 val, u32 offset)
+{
+   return regmap_write(rng->regmap, offset, val);
+}
+
+static int jz4780_rng_generate(struct crypto_rng *tfm,
+  const u8 *src, unsigned int slen,
+  u8 *dst, unsigned int dlen)
+{
+   struct jz4780_rng_ctx *ctx = crypto_rng_ctx(tfm);
+   struct jz4780_rng *rng = ctx->rng;
+   u32 data;
+
+   /*
+* A delay is required so that the current RNG dat

[PATCH v2 0/4] crypto: Add driver for JZ4780 PRNG

[PrasannaKumar Muralidharan]

This patch series adds support of pseudo random number generator found
in Ingenic's JZ4780 and X1000 SoC.

Based on Paul's review comments, add 'syscon' compatible in CGU node in
jz4780.dtsi. jz4780-rng driver uses regmap exposed via syscon interface
to access the RNG registers. CGU driver is not modified in this patch
set as registers used by CGU driver and this driver are different.

PrasannaKumar Muralidharan (4):
  crypto: jz4780-rng: Add JZ4780 PRNG devicetree binding documentation
  crypto: jz4780-rng: Add Ingenic JZ4780 hardware PRNG driver
  crypto: jz4780-rng: Add RNG node to jz4780.dtsi
  crypto: jz4780-rng: Enable PRNG support in CI20 defconfig

 .../bindings/crypto/ingenic,jz4780-rng.txt |  20 +++
 MAINTAINERS|   5 +
 arch/mips/boot/dts/ingenic/jz4780.dtsi |   6 +-
 arch/mips/configs/ci20_defconfig   |   5 +
 drivers/crypto/Kconfig |  19 +++
 drivers/crypto/Makefile|   1 +
 drivers/crypto/jz4780-rng.c| 168 +
 7 files changed, 223 insertions(+), 1 deletion(-)
 create mode 100644 
Documentation/devicetree/bindings/crypto/ingenic,jz4780-rng.txt
 create mode 100644 drivers/crypto/jz4780-rng.c

-- 
2.10.0

[PATCH] hw_random: timeriomem-rng: Remove 'max < 4' condition check

[PrasannaKumar Muralidharan]

In read routiene max is always >= 4. The check whether 'max < 4' is not
necessary. Remove it.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 drivers/char/hw_random/timeriomem-rng.c | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/drivers/char/hw_random/timeriomem-rng.c 
b/drivers/char/hw_random/timeriomem-rng.c
index 03ff548..f615684 100644
--- a/drivers/char/hw_random/timeriomem-rng.c
+++ b/drivers/char/hw_random/timeriomem-rng.c
@@ -53,13 +53,6 @@ static int timeriomem_rng_read(struct hwrng *hwrng, void 
*data,
int period_us = ktime_to_us(priv->period);
 
/*
-* The RNG provides 32-bits per read.  Ensure there is enough space for
-* at minimum one read.
-*/
-   if (max < sizeof(u32))
-   return 0;
-
-   /*
 * There may not have been enough time for new data to be generated
 * since the last request.  If the caller doesn't want to wait, let them
 * bail out.  Otherwise, wait for the completion.  If the new data has
-- 
2.10.0

[PATCH] Documentation: hw_random: Fix issue related to feeding entropy pool

[PrasannaKumar Muralidharan]

There is no need to use rng-tools for feeding random data into kernel
entropy pool as hw_random core handles it. Documentation suggested that
rng-tools is required which is incorrect. So remove it.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 Documentation/hw_random.txt | 4 
 1 file changed, 4 deletions(-)

diff --git a/Documentation/hw_random.txt b/Documentation/hw_random.txt
index 121de96..46b2480 100644
--- a/Documentation/hw_random.txt
+++ b/Documentation/hw_random.txt
@@ -19,10 +19,6 @@ hw_random driver's official Web site:
 
http://sourceforge.net/projects/gkernel/
 
-Those tools use /dev/hwrng to fill the kernel entropy pool,
-which is used internally and exported by the /dev/urandom and
-/dev/random special files.
-
 Theory of operation
 ===
 
-- 
2.10.0

Re: [PATCH 2/6] crypto: jz4780-rng: Make ingenic CGU driver use syscon

[PrasannaKumar Muralidharan]

Hi Paul,

Thanks for your review.

On 19 August 2017 at 02:18, Paul Burton <paul.bur...@imgtec.com> wrote:
> Hi PrasannaKumar,
>
> On Thursday, 17 August 2017 11:25:16 PDT PrasannaKumar Muralidharan wrote:
>> Ingenic PRNG registers are a part of the same hardware block as clock
>> and power stuff. It is handled by CGU driver. Ingenic M200 SoC has power
>> related registers that are after the PRNG registers. So instead of
>> shortening the register range use syscon interface to expose regmap.
>> This regmap is used by the PRNG driver.
>>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> ---
>>  arch/mips/boot/dts/ingenic/jz4740.dtsi | 14 +++
>>  arch/mips/boot/dts/ingenic/jz4780.dtsi | 14 +++
>>  drivers/clk/ingenic/cgu.c  | 46
>> +- drivers/clk/ingenic/cgu.h  |
>>  9 +--
>>  drivers/clk/ingenic/jz4740-cgu.c   | 30 +++---
>>  drivers/clk/ingenic/jz4780-cgu.c   | 10 
>>  6 files changed, 73 insertions(+), 50 deletions(-)
>>
>> diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>> b/arch/mips/boot/dts/ingenic/jz4740.dtsi index 2ca7ce7..ada5e67 100644
>> --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>> +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>> @@ -34,14 +34,18 @@
>>   clock-frequency = <32768>;
>>   };
>>
>> - cgu: jz4740-cgu@1000 {
>> - compatible = "ingenic,jz4740-cgu";
>> + cgu_registers {
>> + compatible = "simple-mfd", "syscon";
>>   reg = <0x1000 0x100>;
>>
>> - clocks = <>, <>;
>> - clock-names = "ext", "rtc";
>> + cgu: jz4780-cgu@1000 {
>> + compatible = "ingenic,jz4740-cgu";
>>
>> - #clock-cells = <1>;
>> + clocks = <>, <>;
>> + clock-names = "ext", "rtc";
>> +
>> + #clock-cells = <1>;
>> + };
>>   };
>>
>>   rtc_dev: rtc@10003000 {
>> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> b/arch/mips/boot/dts/ingenic/jz4780.dtsi index 4853ef6..1301694 100644
>> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> @@ -34,14 +34,18 @@
>>   clock-frequency = <32768>;
>>   };
>>
>> - cgu: jz4780-cgu@1000 {
>> - compatible = "ingenic,jz4780-cgu";
>> + cgu_registers {
>> + compatible = "simple-mfd", "syscon";
>>   reg = <0x1000 0x100>;
>>
>> - clocks = <>, <>;
>> - clock-names = "ext", "rtc";
>> + cgu: jz4780-cgu@1000 {
>> + compatible = "ingenic,jz4780-cgu";
>>
>> - #clock-cells = <1>;
>> + clocks = <>, <>;
>> + clock-names = "ext", "rtc";
>> +
>> + #clock-cells = <1>;
>> + };
>>   };
>>
>>   pinctrl: pin-controller@1001 {
>> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
>> index e8248f9..2f12c7c 100644
>> --- a/drivers/clk/ingenic/cgu.c
>> +++ b/drivers/clk/ingenic/cgu.c
>> @@ -29,6 +29,18 @@
>>
>>  #define MHZ (1000 * 1000)
>>
>> +unsigned int cgu_readl(struct ingenic_cgu *cgu, unsigned int reg)
>> +{
>> + unsigned int val = 0;
>> + regmap_read(cgu->regmap, reg, );
>> + return val;
>> +}
>> +
>> +int cgu_writel(struct ingenic_cgu *cgu, unsigned int val, unsigned int reg)
>> +{
>> + return regmap_write(cgu->regmap, reg, val);
>> +}
>
> This is going to introduce a lot of overhead to the CGU driver - each
> regmap_read() or regmap_write() call will not only add overhead from the
> indirection but also acquire a lock which we really don't need.
>

Indeed.

> Could you instead perhaps:
>
>  - Just add "syscon" as a second compatible string to the CGU node in the
>device tree, but otherwise leave it as-is without the extra cgu_registers
>node.
>
>  - Have your RNG device node as a child of the CGU node, which should let it
>pick up the regmap via syscon_node_to_regmap() as it already does.
>
>  - Leave the CGU driver as-is, so it can continue accessing memory directly
>rather than via regmap.
>

As per my understanding, CGU driver and syscon will map the same
register set. Is that fine?

Thanks,
PrasannaKumar

Re: [PATCH 3/6] crypto: jz4780-rng: Add Ingenic JZ4780 hardware PRNG driver

[PrasannaKumar Muralidharan]

Hi Stephan,

On 18 August 2017 at 00:22, Stephan Mueller <smuel...@chronox.de> wrote:
> Am Donnerstag, 17. August 2017, 20:25:17 CEST schrieb PrasannaKumar
> Muralidharan:
>
> Hi PrasannaKumar,
>
>> +
>> +static int jz4780_rng_generate(struct crypto_rng *tfm,
>> +const u8 *src, unsigned int slen,
>> +u8 *dst, unsigned int dlen)
>> +{
>> + struct jz4780_rng_ctx *ctx = crypto_rng_ctx(tfm);
>> + struct jz4780_rng *rng = ctx->rng;
>> + u32 data;
>> +
>> + /*
>> +  * JZ4780 Programmers manual says the RNG should not run continuously
>> +  * for more than 1s. So enable RNG, read data and disable it.
>> +  * NOTE: No issue was observed with MIPS creator CI20 board even when
>> +  * RNG ran continuously for longer periods. This is just a precaution.
>> +  *
>> +  * A delay is required so that the current RNG data is not bit shifted
>> +  * version of previous RNG data which could happen if random data is
>> +  * read continuously from this device.
>> +  */
>> + jz4780_rng_writel(rng, 1, REG_RNG_CTRL);
>> + do {
>> + data = jz4780_rng_readl(rng, REG_RNG_DATA);
>> + memcpy((void *)dst, (void *), 4);
>
> How do you know that dst is a multiple of 4 bytes? When dlen is only 3, you
> overflow the buffer.

You are right. I initially used hw_random framework for this driver.
But later realised that PRNG driver should use crypto framework. When
I started using crypto I reused most of the code. This was because of
porting. Will change it and send next version.

>
>> + dlen -= 4;
>> + dst += 4;
>> + udelay(20);
>> + } while (dlen >= 4);
>> +
>> + if (dlen > 0) {
>> + data = jz4780_rng_readl(rng, REG_RNG_DATA);
>> + memcpy((void *)dst, (void *), dlen);
>> + }
>> + jz4780_rng_writel(rng, 0, REG_RNG_CTRL);
>> +
>> + return 0;
>> +}
>
> Ciao
> Stephan

Thanks,
PrasannaKumar

[PATCH 6/6] crypto: jz4780-rng: Enable PRNG support in CI20 defconfig

[PrasannaKumar Muralidharan]

Enable PRNG driver support in MIPS Creator CI20 default config.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 arch/mips/configs/ci20_defconfig | 5 +
 1 file changed, 5 insertions(+)

diff --git a/arch/mips/configs/ci20_defconfig b/arch/mips/configs/ci20_defconfig
index b42cfa7..9f48f2c 100644
--- a/arch/mips/configs/ci20_defconfig
+++ b/arch/mips/configs/ci20_defconfig
@@ -88,6 +88,11 @@ CONFIG_SERIAL_8250_RUNTIME_UARTS=5
 CONFIG_SERIAL_8250_INGENIC=y
 CONFIG_SERIAL_OF_PLATFORM=y
 # CONFIG_HW_RANDOM is not set
+CONFIG_CRYPTO_USER=y
+CONFIG_CRYPTO_USER_API=y
+CONFIG_CRYPTO_USER_API_RNG=y
+CONFIG_CRYPTO_HW=y
+CONFIG_CRYPTO_DEV_JZ4780_RNG=y
 CONFIG_I2C=y
 CONFIG_I2C_JZ4780=y
 CONFIG_GPIO_SYSFS=y
-- 
2.10.0

[PATCH 5/6] crypto: jz4780-rng: Add myself as mainatainer for JZ4780 PRNG driver

[PrasannaKumar Muralidharan]

Add myself as the maintainer of JZ4780 SoC's PRNG drvier.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 MAINTAINERS | 5 +
 1 file changed, 5 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 45ec467..ee8c6f6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6733,6 +6733,11 @@ L:   linux-...@lists.infradead.org
 S: Maintained
 F: drivers/mtd/nand/jz4780_*
 
+INGENIC JZ4780 PRNG DRIVER
+M: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
+S: Maintained
+F: drivers/crypto/jz4780-rng.c
+
 INOTIFY
 M: John McCutchan <j...@johnmccutchan.com>
 M: Robert Love <rl...@rlove.org>
-- 
2.10.0

[PATCH 3/6] crypto: jz4780-rng: Add Ingenic JZ4780 hardware PRNG driver

[PrasannaKumar Muralidharan]

JZ4780 SoC pseudo random number generator driver using crypto framework.

Adding a delay before reading RNG data and disabling RNG after reading
data was suggested by Jeffery Walton.

Tested-by: Mathieu Malaterre <ma...@debian.org>
Suggested-by: Jeffrey Walton <noloa...@gmail.com>
Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 drivers/crypto/Kconfig  |  19 +
 drivers/crypto/Makefile |   1 +
 drivers/crypto/jz4780-rng.c | 173 
 3 files changed, 193 insertions(+)
 create mode 100644 drivers/crypto/jz4780-rng.c

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index 8fa7e72..8263622 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig
@@ -614,6 +614,25 @@ config CRYPTO_DEV_IMGTEC_HASH
  hardware hash accelerator. Supporting MD5/SHA1/SHA224/SHA256
  hashing algorithms.
 
+config CRYPTO_DEV_JZ4780_RNG
+   tristate "JZ4780 HW pseudo random number generator support"
+   depends on MACH_JZ4780 || COMPILE_TEST
+   depends on HAS_IOMEM
+   select CRYPTO_RNG
+   select REGMAP
+   select SYSCON
+   select MFD_SYSCON
+   ---help---
+ This driver provides kernel-side support through the
+ cryptographic API for the pseudo random number generator
+ hardware found in ingenic JZ4780 and X1000 SoC. MIPS
+ Creator CI20 uses JZ4780 SoC.
+
+ To compile this driver as a module, choose M here: the
+ module will be called jz4780-rng.
+
+ If unsure, say Y.
+
 config CRYPTO_DEV_SUN4I_SS
tristate "Support for Allwinner Security System cryptographic 
accelerator"
depends on ARCH_SUNXI && !64BIT
diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
index b12eb3c..3a3d970 100644
--- a/drivers/crypto/Makefile
+++ b/drivers/crypto/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_CRYPTO_DEV_GEODE) += geode-aes.o
 obj-$(CONFIG_CRYPTO_DEV_HIFN_795X) += hifn_795x.o
 obj-$(CONFIG_CRYPTO_DEV_IMGTEC_HASH) += img-hash.o
 obj-$(CONFIG_CRYPTO_DEV_IXP4XX) += ixp4xx_crypto.o
+obj-$(CONFIG_CRYPTO_DEV_JZ4780_RNG) += jz4780-rng.o
 obj-$(CONFIG_CRYPTO_DEV_MV_CESA) += mv_cesa.o
 obj-$(CONFIG_CRYPTO_DEV_MARVELL_CESA) += marvell/
 obj-$(CONFIG_CRYPTO_DEV_MEDIATEK) += mediatek/
diff --git a/drivers/crypto/jz4780-rng.c b/drivers/crypto/jz4780-rng.c
new file mode 100644
index 000..a03f2a0
--- /dev/null
+++ b/drivers/crypto/jz4780-rng.c
@@ -0,0 +1,173 @@
+/*
+ * jz4780-rng.c - Random Number Generator driver for the jz4780
+ *
+ * Copyright (c) 2017 PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
+ *
+ * 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.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#define REG_RNG_CTRL   0xD8
+#define REG_RNG_DATA   0xDC
+
+/* Context for crypto */
+struct jz4780_rng_ctx {
+   struct jz4780_rng *rng;
+};
+
+/* Device associated memory */
+struct jz4780_rng {
+   struct device *dev;
+   struct regmap *regmap;
+};
+
+static struct jz4780_rng *jz4780_rng;
+
+static int jz4780_rng_readl(struct jz4780_rng *rng, u32 offset)
+{
+   u32 val = 0;
+   int ret;
+
+   ret = regmap_read(rng->regmap, offset, );
+   if (!ret)
+   return val;
+
+   return ret;
+}
+
+static int jz4780_rng_writel(struct jz4780_rng *rng, u32 val, u32 offset)
+{
+   return regmap_write(rng->regmap, offset, val);
+}
+
+static int jz4780_rng_generate(struct crypto_rng *tfm,
+  const u8 *src, unsigned int slen,
+  u8 *dst, unsigned int dlen)
+{
+   struct jz4780_rng_ctx *ctx = crypto_rng_ctx(tfm);
+   struct jz4780_rng *rng = ctx->rng;
+   u32 data;
+
+   /*
+* JZ4780 Programmers manual says the RNG should not run continuously
+* for more than 1s. So enable RNG, read data and disable it.
+* NOTE: No issue was observed with MIPS creator CI20 board even when
+* RNG ran continuously for longer periods. This is just a precaution.
+*
+* A delay is required so that the current RNG data is not bit shifted
+* version of previous RNG data which could happen if random data is
+* read continuously from this device.
+*/
+   jz4780_rng_writel(rng, 1, REG_RNG_CTRL);
+   do {
+   data = jz4780_rng_readl(rng, REG_RNG_DATA);
+   memcpy((void *)dst, (void *), 4);
+   dlen -= 4;
+   dst += 4;
+   ud

[PATCH 4/6] crypto: jz4780-rng: Add RNG node to jz4780.dtsi

[PrasannaKumar Muralidharan]

This patch adds RNG node to jz4780.dtsi.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 4 
 1 file changed, 4 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi 
b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 1301694..865b9bf 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -46,6 +46,10 @@
 
#clock-cells = <1>;
};
+
+   rng: rng@d8 {
+   compatible = "ingenic,jz480-rng";
+   };
};
 
pinctrl: pin-controller@1001 {
-- 
2.10.0

[PATCH 2/6] crypto: jz4780-rng: Make ingenic CGU driver use syscon

[PrasannaKumar Muralidharan]

Ingenic PRNG registers are a part of the same hardware block as clock
and power stuff. It is handled by CGU driver. Ingenic M200 SoC has power
related registers that are after the PRNG registers. So instead of
shortening the register range use syscon interface to expose regmap.
This regmap is used by the PRNG driver.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 arch/mips/boot/dts/ingenic/jz4740.dtsi | 14 +++
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 14 +++
 drivers/clk/ingenic/cgu.c  | 46 +-
 drivers/clk/ingenic/cgu.h  |  9 +--
 drivers/clk/ingenic/jz4740-cgu.c   | 30 +++---
 drivers/clk/ingenic/jz4780-cgu.c   | 10 
 6 files changed, 73 insertions(+), 50 deletions(-)

diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi 
b/arch/mips/boot/dts/ingenic/jz4740.dtsi
index 2ca7ce7..ada5e67 100644
--- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
@@ -34,14 +34,18 @@
clock-frequency = <32768>;
};
 
-   cgu: jz4740-cgu@1000 {
-   compatible = "ingenic,jz4740-cgu";
+   cgu_registers {
+   compatible = "simple-mfd", "syscon";
reg = <0x1000 0x100>;
 
-   clocks = <>, <>;
-   clock-names = "ext", "rtc";
+   cgu: jz4780-cgu@1000 {
+   compatible = "ingenic,jz4740-cgu";
 
-   #clock-cells = <1>;
+   clocks = <>, <>;
+   clock-names = "ext", "rtc";
+
+   #clock-cells = <1>;
+   };
};
 
rtc_dev: rtc@10003000 {
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi 
b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 4853ef6..1301694 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -34,14 +34,18 @@
clock-frequency = <32768>;
};
 
-   cgu: jz4780-cgu@1000 {
-   compatible = "ingenic,jz4780-cgu";
+   cgu_registers {
+   compatible = "simple-mfd", "syscon";
reg = <0x1000 0x100>;
 
-   clocks = <>, <>;
-   clock-names = "ext", "rtc";
+   cgu: jz4780-cgu@1000 {
+   compatible = "ingenic,jz4780-cgu";
 
-   #clock-cells = <1>;
+   clocks = <>, <>;
+   clock-names = "ext", "rtc";
+
+   #clock-cells = <1>;
+   };
};
 
pinctrl: pin-controller@1001 {
diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
index e8248f9..2f12c7c 100644
--- a/drivers/clk/ingenic/cgu.c
+++ b/drivers/clk/ingenic/cgu.c
@@ -29,6 +29,18 @@
 
 #define MHZ (1000 * 1000)
 
+unsigned int cgu_readl(struct ingenic_cgu *cgu, unsigned int reg)
+{
+   unsigned int val = 0;
+   regmap_read(cgu->regmap, reg, );
+   return val;
+}
+
+int cgu_writel(struct ingenic_cgu *cgu, unsigned int val, unsigned int reg)
+{
+   return regmap_write(cgu->regmap, reg, val);
+}
+
 /**
  * ingenic_cgu_gate_get() - get the value of clock gate register bit
  * @cgu: reference to the CGU whose registers should be read
@@ -43,7 +55,7 @@ static inline bool
 ingenic_cgu_gate_get(struct ingenic_cgu *cgu,
 const struct ingenic_cgu_gate_info *info)
 {
-   return readl(cgu->base + info->reg) & BIT(info->bit);
+   return cgu_readl(cgu, info->reg) & BIT(info->bit);
 }
 
 /**
@@ -60,14 +72,14 @@ static inline void
 ingenic_cgu_gate_set(struct ingenic_cgu *cgu,
 const struct ingenic_cgu_gate_info *info, bool val)
 {
-   u32 clkgr = readl(cgu->base + info->reg);
+   u32 clkgr = cgu_readl(cgu, info->reg);
 
if (val)
clkgr |= BIT(info->bit);
else
clkgr &= ~BIT(info->bit);
 
-   writel(clkgr, cgu->base + info->reg);
+   cgu_writel(cgu, clkgr, info->reg);
 }
 
 /*
@@ -91,7 +103,7 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long 
parent_rate)
pll_info = _info->pll;
 
spin_lock_irqsave(>lock, flags);
-   ctl = readl(cgu->base + pll_info->reg);
+   ctl = cgu_readl(cgu, pll_info->reg);
spin_unlock_irqrestore(>lock, flags);
 
m = (ctl >> pll_info->m_shift) & GENMASK(pll_info->m_bits - 1, 0);
@@ -190,7 +202,7 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned long 
req_rate,
clk_info->name, req_rate, rate);
 
spin_lock_irqsave(>lock, flags)

[PATCH 0/6] crypto: Add driver for JZ4780 PRNG

[PrasannaKumar Muralidharan]

This patch series adds support of pseudo random number generator found
in Ingenic's JZ4780 and X1000 SoC.

The PRNG hardware block registers are a part of same hardware block
that has clock and power registers which is handled by CGU driver.
Ingenic M200 SoC contains power related registers that are present
after the PRNG registers. So instead of reducing the register range,
syscon interface is used to expose a register map that is used by both
CGU driver and this driver. Changes made to jz4740-cgu.c is only compile
tested.

PrasannaKumar Muralidharan (6):
  crypto: jz4780-rng: Add devicetree bindings for RNG in JZ4780 SoC
  crypto: jz4780-rng: Make ingenic CGU driver use syscon
  crypto: jz4780-rng: Add Ingenic JZ4780 hardware PRNG driver
  crypto: jz4780-rng: Add RNG node to jz4780.dtsi
  crypto: jz4780-rng: Add myself as mainatainer for JZ4780 PRNG driver
  crypto: jz4780-rng: Enable PRNG support in CI20 defconfig

 .../devicetree/bindings/rng/ingenic,jz4780-rng.txt |  24 +++
 MAINTAINERS|   5 +
 arch/mips/boot/dts/ingenic/jz4740.dtsi |  14 +-
 arch/mips/boot/dts/ingenic/jz4780.dtsi |  18 ++-
 arch/mips/configs/ci20_defconfig   |   5 +
 drivers/clk/ingenic/cgu.c  |  46 +++---
 drivers/clk/ingenic/cgu.h  |   9 +-
 drivers/clk/ingenic/jz4740-cgu.c   |  30 ++--
 drivers/clk/ingenic/jz4780-cgu.c   |  10 +-
 drivers/crypto/Kconfig |  19 +++
 drivers/crypto/Makefile|   1 +
 drivers/crypto/jz4780-rng.c| 173 +
 12 files changed, 304 insertions(+), 50 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
 create mode 100644 drivers/crypto/jz4780-rng.c

-- 
2.10.0

[PATCH 1/6] crypto: jz4780-rng: Add devicetree bindings for RNG in JZ4780 SoC

[PrasannaKumar Muralidharan]

Add devicetree bindings for hardware pseudo random number generator
present in Ingenic JZ4780 SoC.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 .../devicetree/bindings/rng/ingenic,jz4780-rng.txt | 24 ++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt

diff --git a/Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt 
b/Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
new file mode 100644
index 000..88a0a92
--- /dev/null
+++ b/Documentation/devicetree/bindings/rng/ingenic,jz4780-rng.txt
@@ -0,0 +1,24 @@
+Ingenic jz4780 RNG driver
+
+Required properties:
+- compatible : Should be "ingenic,jz4780-rng"
+
+Example:
+
+cgu_registers {
+   compatible = "simple-mfd", "syscon";
+   reg = <0x1000 0x100>;
+
+   cgu: jz4780-cgu@1000 {
+   compatible = "ingenic,jz4780-cgu";
+
+   clocks = <>, <>;
+   clock-names = "ext", "rtc";
+
+   #clock-cells = <1>;
+   };
+
+   rng: rng@d8 {
+   compatible = "ingenic,jz480-rng";
+   };
+};
-- 
2.10.0

[PATCH] crypto: Kconfig: Correct help text about feeding entropy pool

[PrasannaKumar Muralidharan]

Modify Kconfig help text to reflect the fact that random data from hwrng
is fed into kernel random number generator's entropy pool.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
---
 drivers/char/hw_random/Kconfig | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 1b223c3..2f45e15 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -13,10 +13,8 @@ menuconfig HW_RANDOM
  that's usually called /dev/hwrng, and which exposes one
  of possibly several hardware random number generators.
 
- These hardware random number generators do not feed directly
- into the kernel's random number generator.  That is usually
- handled by the "rngd" daemon.  Documentation/hw_random.txt
- has more information.
+ These hardware random number generators do feed into the
+ kernel's random number generator entropy pool.
 
  If unsure, say Y.
 
-- 
2.10.0

Re: [PATCH v4 3/3] hwrng: mxc-fsl - add support for Freescale RNGC

[PrasannaKumar Muralidharan]

  /*
> +* clearing the interrupt will also clear the error register
> +* read error and status before clearing
> +*/
> +   status = readl(rngc->base + RNGC_STATUS);
> +   rngc->err_reg = readl(rngc->base + RNGC_ERROR);
> +
> +   mxc_rngc_irq_mask_clear(rngc);
> +
> +   if (status & RNGC_STATUS_SEED_DONE)
> +   complete(>rng_seed_done);
> +
> +   if (status & RNGC_STATUS_ST_DONE)
> +   complete(>rng_self_testing);
> +
> +   return IRQ_HANDLED;
> +}
> +
> +static int mxc_rngc_init(struct hwrng *rng)
> +{
> +   struct mxc_rngc *rngc = container_of(rng, struct mxc_rngc, rng);
> +   u32 cmd;
> +   int ret;
> +
> +   /* clear error */
> +   cmd = readl(rngc->base + RNGC_COMMAND);
> +   writel(cmd | RNGC_CMD_CLR_ERR, rngc->base + RNGC_COMMAND);
> +
> +   /* create seed, repeat while there is some statistical error */
> +   do {
> +   mxc_rngc_irq_unmask(rngc);
> +
> +   /* seed creation */
> +   cmd = readl(rngc->base + RNGC_COMMAND);
> +   writel(cmd | RNGC_CMD_SEED, rngc->base + RNGC_COMMAND);
> +
> +   ret = wait_for_completion_timeout(>rng_seed_done,
> +   RNGC_TIMEOUT);
> +
> +   if (!ret) {
> +   mxc_rngc_irq_mask_clear(rngc);
> +   return -ETIMEDOUT;
> +   }
> +
> +   } while (rngc->err_reg == RNGC_ERROR_STATUS_STAT_ERR);
> +
> +   return rngc->err_reg ? -EIO : 0;
> +}
> +
> +static int mxc_rngc_probe(struct platform_device *pdev)
> +{
> +   struct mxc_rngc *rngc;
> +   struct resource *res;
> +   int ret;
> +   int irq;
> +
> +   rngc = devm_kzalloc(>dev, sizeof(*rngc), GFP_KERNEL);
> +   if (!rngc)
> +   return -ENOMEM;
> +
> +   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +   rngc->base = devm_ioremap_resource(>dev, res);
> +   if (IS_ERR(rngc->base))
> +   return PTR_ERR(rngc->base);
> +
> +   rngc->clk = devm_clk_get(>dev, NULL);
> +   if (IS_ERR(rngc->clk)) {
> +   dev_err(>dev, "Can not get rng_clk\n");
> +   return PTR_ERR(rngc->clk);
> +   }
> +
> +   irq = platform_get_irq(pdev, 0);
> +   if (irq <= 0) {
> +   dev_err(>dev, "Couldn't get irq %d\n", irq);
> +   return irq;
> +   }
> +
> +   ret = clk_prepare_enable(rngc->clk);
> +   if (ret)
> +   return ret;
> +
> +   ret = devm_request_irq(>dev,
> +   irq, mxc_rngc_irq, 0, pdev->name, (void *)rngc);
> +   if (ret) {
> +   dev_err(rngc->dev, "Can't get interrupt working.\n");
> +   goto err;
> +   }
> +
> +   init_completion(>rng_self_testing);
> +   init_completion(>rng_seed_done);
> +
> +   rngc->rng.name = pdev->name;
> +   rngc->rng.init = mxc_rngc_init;
> +   rngc->rng.read = mxc_rngc_read;
> +
> +   rngc->dev = >dev;
> +   platform_set_drvdata(pdev, rngc);
> +
> +   mxc_rngc_irq_mask_clear(rngc);
> +
> +   if (self_test) {
> +   ret = mxc_rngc_self_test(rngc);
> +   if (ret) {
> +   dev_err(rngc->dev, "FSL RNGC self test failed.\n");
> +   goto err;
> +   }
> +   }
> +
> +   ret = hwrng_register(>rng);
> +   if (ret) {
> +   dev_err(>dev, "FSL RNGC registering failed (%d)\n", 
> ret);
> +   goto err;
> +   }
> +
> +   dev_info(>dev, "Freescale RNGC registered.\n");
> +   return 0;
> +
> +err:
> +   clk_disable_unprepare(rngc->clk);
> +
> +   return ret;
> +}
> +
> +static int __exit mxc_rngc_remove(struct platform_device *pdev)
> +{
> +   struct mxc_rngc *rngc = platform_get_drvdata(pdev);
> +
> +   hwrng_unregister(>rng);
> +
> +   clk_disable_unprepare(rngc->clk);
> +
> +   return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int mxc_rngc_suspend(struct device *dev)
> +{
> +   struct mxc_rngc *rngc = dev_get_drvdata(dev);
> +
> +   clk_disable_unprepare(rngc->clk);
> +
> +   return 0;
> +}
> +
> +static int mxc_rngc_resume(struct device *dev)
> +{
> +   struct mxc_rngc *rngc = dev_get_drvdata(dev);
> +
> +   clk_prepare_enable(rngc->clk);
> +
> +   return 0;
> +}
> +
> +static const struct dev_pm_ops mxc_rngc_pm_ops = {
> +   .suspend= mxc_rngc_suspend,
> +   .resume = mxc_rngc_resume,
> +};
> +#endif
> +
> +static const struct of_device_id mxc_rngc_dt_ids[] = {
> +   { .compatible = "fsl,imx25-rng", .data = NULL, },
> +   { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mxc_rngc_dt_ids);
> +
> +static struct platform_driver mxc_rngc_driver = {
> +   .driver = {
> +   .name = "mxc_rngc",
> +#ifdef CONFIG_PM
> +   .pm = _rngc_pm_ops,
> +#endif
> +   .of_match_table = mxc_rngc_dt_ids,
> +   },
> +   .remove = __exit_p(mxc_rngc_remove),
> +};
> +
> +module_platform_driver_probe(mxc_rngc_driver, mxc_rngc_probe);
> +
> +MODULE_AUTHOR("Freescale Semiconductor, Inc.");
> +MODULE_DESCRIPTION("H/W RNGC driver for i.MX");
> +MODULE_LICENSE("GPL");
> --
> 2.1.4
>

Regardless of the minor suggestion, code looks good to me as is.
Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>.

Note: Add my reviewed by tag if you are going for next version.

Regards,
PrasannaKumar

Re: [PATCH 3/3] hwrng: mxc-fsl - add support for Freescale RNGC

[PrasannaKumar Muralidharan]

Hi Martin,

On 18 July 2017 at 02:46, Martin Kaiser  wrote:
> From: Steffen Trumtrar 
>
> The driver is ported from Freescales Linux git and can be
> found in the
>
> vendor/freescale/imx_2.6.35_maintain
>
> branch.
>
> According to that code, the RNGC is found on Freescales i.MX3/5 SoCs.
> The i.MX2x actually has an RNGB, which has no driver implementation
> in Freescales kernel. However as it turns out, the driver for the RNGC
> works fine on the (at least) i.MX25. So, they seem to be somewhat
> compatible.
>
> Signed-off-by: Steffen Trumtrar 
> Signed-off-by: Martin Kaiser 
> ---
> Changes in v3:
>- use pdev->dev to request the irq, rngc->dev is not yet initialized
>- remove unused defines for registers and fields
>- use module_platform_driver_probe()
>- clean up the error handling in the probe function,
>  disable the clock if necessary
>- self-test must succeed in the first run
>- check for errors after seeding, exit for errors unrelated to
>  statistics
>- set a timeout when waiting for a completion
>
> Changes in v2:
>   - remove irq variable from private struct
>   - move devm_request_irq from mxc_rngc_init to probe
>   - return irq in case of error
>   - handle irq 0 as error
>
>  drivers/char/hw_random/Kconfig|  13 ++
>  drivers/char/hw_random/Makefile   |   1 +
>  drivers/char/hw_random/mxc-rngc.c | 351 
> ++
>  3 files changed, 365 insertions(+)
>  create mode 100644 drivers/char/hw_random/mxc-rngc.c
>
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 1b223c3..ef057b7 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -255,6 +255,19 @@ config HW_RANDOM_MXC_RNGA
>
>   If unsure, say Y.
>
> +config HW_RANDOM_MXC_RNGC
> +   tristate "Freescale i.MX RNGC Random Number Generator"
> +   depends on ARCH_MXC
> +   default HW_RANDOM
> +   ---help---
> + This driver provides kernel-side support for the Random Number
> + Generator hardware found on some Freescale i.MX processors.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called mxc-rngc.
> +
> + If unsure, say Y.
> +
>  config HW_RANDOM_NOMADIK
> tristate "ST-Ericsson Nomadik Random Number Generator support"
> depends on ARCH_NOMADIK
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index b085975..043b71d 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_HW_RANDOM_PASEMI) += pasemi-rng.o
>  obj-$(CONFIG_HW_RANDOM_VIRTIO) += virtio-rng.o
>  obj-$(CONFIG_HW_RANDOM_TX4939) += tx4939-rng.o
>  obj-$(CONFIG_HW_RANDOM_MXC_RNGA) += mxc-rnga.o
> +obj-$(CONFIG_HW_RANDOM_MXC_RNGC) += mxc-rngc.o
>  obj-$(CONFIG_HW_RANDOM_OCTEON) += octeon-rng.o
>  obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
>  obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
> diff --git a/drivers/char/hw_random/mxc-rngc.c 
> b/drivers/char/hw_random/mxc-rngc.c
> new file mode 100644
> index 000..56175b1
> --- /dev/null
> +++ b/drivers/char/hw_random/mxc-rngc.c
> @@ -0,0 +1,351 @@
> +/*
> + * RNG driver for Freescale RNGC
> + *
> + * Copyright (C) 2008-2012 Freescale Semiconductor, Inc.
> + */
> +
> +/*
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */

Please combine above 2 comments.

> +
> +/*
> + * Hardware driver for the Intel/AMD/VIA Random Number Generators (RNG)
> + * (c) Copyright 2003 Red Hat Inc 
> + *
> + * derived from
> + *
> + * Hardware driver for the AMD 768 Random Number Generator (RNG)
> + * (c) Copyright 2001 Red Hat Inc 
> + *
> + * derived from
> + *
> + * Hardware driver for Intel i810 Random Number Generator (RNG)
> + * Copyright 2000,2001 Jeff Garzik 
> + * Copyright 2000,2001 Philipp Rumpf 
> + *
> + * This file is licensed under  the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */

I feel this comment is because of copy paste. If that's the case please remove.

> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define RNGC_COMMAND   0x0004
> +#define RNGC_CONTROL   0x0008
> +#define RNGC_STATUS0x000C
> +#define RNGC_ERROR 0x0010
> +#define RNGC_FIFO   

Re: [PATCH] crypto: change hwrng device default permissions to 0444

[PrasannaKumar Muralidharan]

Hi Herbert,

On 12 July 2017 at 15:43, Herbert Xu  wrote:
> Hmm, one usage scenario for /dev/hwrng is to feed rngd which then
> feeds into /dev/random.  In that case it may not be desirable to
> allow arbitrary access to hwrgn since it may cause the rate of
> entropy going into /dev/random to go down.
>
> In any case, as you noted userspace can change this anyway so I
> don't see why we need to make this policy change in the kernel.

Looking at the comment in
https://android.googlesource.com/platform/frameworks/base/+/master/services/core/java/com/android/server/EntropyMixer.java#145
I am wondering whether your concern is a problem. I do not know
whether the comment in Android source is valid so please ignore my
ignorance.

Regards,
PrasannaKumar

Re: [PATCH 3/3] crypto: hwrng add sysfs attribute to show user selected rng

[PrasannaKumar Muralidharan]

Hi Harald,

> Hello PrasannaKumar
>
> here is now a version which combines patch 2 and patch 3 together
> according to your suggestions:
>
> == cut ==
>
> From: Harald Freudenberger <fre...@linux.vnet.ibm.com>
> Date: Fri, 30 Jun 2017 17:06:40 +0200
> Subject: [PATCH 2/2] crypto: hwrng remember rng chosen by user
>
> When a user chooses a rng source via sysfs attribute
> this rng should be sticky, even when other sources
> with better quality to register. This patch introduces
> a simple way to remember the user's choice. This is
> reflected by a new sysfs attribute file 'rng_selected'
> which shows if the current rng has been chosen by
> userspace. The new attribute file shows '1' for user
> selected rng and '0' otherwise.
>
> Signed-off-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>
> ---
>  drivers/char/hw_random/core.c | 21 +++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index e9dda16..9701ac7 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -28,6 +28,8 @@
>  #define RNG_MODULE_NAME"hw_random"
>
>  static struct hwrng *current_rng;
> +/* the current rng has been explicitly chosen by user via sysfs */
> +static int cur_rng_set_by_user;
>  static struct task_struct *hwrng_fill;
>  /* list of registered rngs, sorted decending by quality */
>  static LIST_HEAD(rng_list);
> @@ -304,6 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device 
> *dev,
>  list_for_each_entry(rng, _list, list) {
>  if (sysfs_streq(rng->name, buf)) {
>  err = 0;
> +cur_rng_set_by_user = 1;
>  if (rng != current_rng)
>  err = set_current_rng(rng);
>  break;
> @@ -352,16 +355,27 @@ static ssize_t hwrng_attr_available_show(struct device 
> *dev,
>  return strlen(buf);
>  }
>
> +static ssize_t hwrng_attr_selected_show(struct device *dev,
> +struct device_attribute *attr,
> +char *buf)
> +{
> +return snprintf(buf, PAGE_SIZE, "%d\n", cur_rng_set_by_user);
> +}
> +
>  static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
> hwrng_attr_current_show,
> hwrng_attr_current_store);
>  static DEVICE_ATTR(rng_available, S_IRUGO,
> hwrng_attr_available_show,
> NULL);
> +static DEVICE_ATTR(rng_selected, S_IRUGO,
> +   hwrng_attr_selected_show,
> +   NULL);
>
>  static struct attribute *rng_dev_attrs[] = {
>  _attr_rng_current.attr,
>  _attr_rng_available.attr,
> +_attr_rng_selected.attr,
>  NULL
>  };
>
> @@ -444,10 +458,12 @@ int hwrng_register(struct hwrng *rng)
>
>  old_rng = current_rng;
>  err = 0;
> -if (!old_rng || (rng->quality > old_rng->quality)) {
> +if (!old_rng ||
> +(!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
>  /*
>   * Set new rng as current as the new rng source
> - * provides better entropy quality.
> + * provides better entropy quality and was not
> + * chosen by userspace.
>   */
>  err = set_current_rng(rng);
>  if (err)
> @@ -479,6 +495,7 @@ void hwrng_unregister(struct hwrng *rng)
>  list_del(>list);
>  if (current_rng == rng) {
>  drop_current_rng();
> +cur_rng_set_by_user = 0;
>  /* rng_list is sorted by quality, use the best (=first) one */
>  if (!list_empty(_list)) {
>  struct hwrng *new_rng;
> --
> 2.7.4
>
> == cut ==
>

Good work, patch looks good to me. Reviewed-by: PrasannaKumar
Muralidharan <prasannatsmku...@gmail.com>.
I am wondering if you want to make a v2 of the whole series. You can
just put your patch 1 and 2 and send it again with description.
Requiring a v2 depends on how Herbert prefers to take patches in.

Herbert, Do you prefer a v2 of the patch series or you can take this
version of patch 2?

Note: If you are re-posting this patch as v2 please add my reviewed by
tag for both the patches.

Regards,
PrasannaKumar

Re: [PATCH 3/3] crypto: hwrng add sysfs attribute to show user selected rng

[PrasannaKumar Muralidharan]

Hi Harald,

> Here is an updated version with just showing 0 or 1 in the new sysfs
> attribute file:
> == cut ==
> From: Harald Freudenberger 
> Date: Mon, 3 Jul 2017 10:19:22 +0200
> Subject: [PATCH 3/3] crypto: hwrng add sysfs attribute to show user selected
>  rng
>
> This patch introduces a new sysfs attribute file 'rng_selected'
> which shows if the current rng has been chosen by userspace.
>
> If a rng source is chosen by user via echo some valid string
> to rng_current there should be a way to signal this choice to
> userspace. The new attribute file 'rng_selected' shows '1' for
> user selecte rng and '0' otherwise.
>
> Signed-off-by: Harald Freudenberger 
> ---
>  drivers/char/hw_random/core.c | 21 -
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index ffd4e36..2b4c7f6 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -28,8 +28,8 @@
>  #define RNG_MODULE_NAME"hw_random"
>
>  static struct hwrng *current_rng;
> -/* the current rng has been explicitly chosen by user via sysfs */
> -static int cur_rng_set_by_user;
> +/* the rng explicitly selected by user via sysfs */
> +static struct hwrng *selected_rng;
>  static struct task_struct *hwrng_fill;
>  /* list of registered rngs, sorted decending by quality */
>  static LIST_HEAD(rng_list);
> @@ -306,7 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device 
> *dev,
>  list_for_each_entry(rng, _list, list) {
>  if (sysfs_streq(rng->name, buf)) {
>  err = 0;
> -cur_rng_set_by_user = 1;
> +selected_rng = rng;
>  if (rng != current_rng)
>  err = set_current_rng(rng);
>  break;
> @@ -355,16 +355,27 @@ static ssize_t hwrng_attr_available_show(struct device 
> *dev,
>  return strlen(buf);
>  }
>
> +static ssize_t hwrng_attr_selected_show(struct device *dev,
> +struct device_attribute *attr,
> +char *buf)
> +{
> +return snprintf(buf, PAGE_SIZE, "%d\n", selected_rng ? 1 : 0);
> +}
> +
>  static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
> hwrng_attr_current_show,
> hwrng_attr_current_store);
>  static DEVICE_ATTR(rng_available, S_IRUGO,
> hwrng_attr_available_show,
> NULL);
> +static DEVICE_ATTR(rng_selected, S_IRUGO,
> +   hwrng_attr_selected_show,
> +   NULL);
>
>  static struct attribute *rng_dev_attrs[] = {
>  _attr_rng_current.attr,
>  _attr_rng_available.attr,
> +_attr_rng_selected.attr,
>  NULL
>  };
>
> @@ -448,7 +459,7 @@ int hwrng_register(struct hwrng *rng)
>  old_rng = current_rng;
>  err = 0;
>  if (!old_rng ||
> -(!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
> +(!selected_rng && rng->quality > old_rng->quality)) {
>  /*
>   * Set new rng as current as the new rng source
>   * provides better entropy quality and was not
> @@ -484,7 +495,7 @@ void hwrng_unregister(struct hwrng *rng)
>  list_del(>list);
>  if (current_rng == rng) {
>  drop_current_rng();
> -cur_rng_set_by_user = 0;
> +selected_rng = NULL;
>  /* rng_list is sorted by quality, use the best (=first) one */
>  if (!list_empty(_list)) {
>  struct hwrng *new_rng;
> --
> 2.7.4
> == cut ==
>

Nice to see quick turn around time. Why not just use the
cur_rng_set_by_user instead of selected_rng? This way patch 3 won't
change things that patch 2 introduced and patch 3 will become much
smaller. Also I feel folding patch 3 and 2 will make sense as both are
related to user selected rng.

Regards,
PrasannaKumar

Re: [PATCH 2/3] crypto: hwrng remember rng chosen by user

[PrasannaKumar Muralidharan]

Hi Harald,

On 3 July 2017 at 15:33, Harald Freudenberger <fre...@linux.vnet.ibm.com> wrote:
> When a user chooses a rng source via sysfs
> attribute this rng should be sticky, even
> when other sources with better quality to
> register. This patch introduces a simple way
> to remember the user's choise.
>
> Signed-off-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>
> ---
>  drivers/char/hw_random/core.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index e9dda16..ffd4e36 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -28,6 +28,8 @@
>  #define RNG_MODULE_NAME"hw_random"
>
>  static struct hwrng *current_rng;
> +/* the current rng has been explicitly chosen by user via sysfs */
> +static int cur_rng_set_by_user;
>  static struct task_struct *hwrng_fill;
>  /* list of registered rngs, sorted decending by quality */
>  static LIST_HEAD(rng_list);
> @@ -304,6 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device 
> *dev,
> list_for_each_entry(rng, _list, list) {
> if (sysfs_streq(rng->name, buf)) {
> err = 0;
> +   cur_rng_set_by_user = 1;
> if (rng != current_rng)
> err = set_current_rng(rng);
> break;
> @@ -444,10 +447,12 @@ int hwrng_register(struct hwrng *rng)
>
> old_rng = current_rng;
> err = 0;
> -   if (!old_rng || (rng->quality > old_rng->quality)) {
> +   if (!old_rng ||
> +   (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
> /*
>  * Set new rng as current as the new rng source
> -* provides better entropy quality.
> +* provides better entropy quality and was not
> +* chosen by userspace.
>  */
> err = set_current_rng(rng);
> if (err)
> @@ -479,6 +484,7 @@ void hwrng_unregister(struct hwrng *rng)
> list_del(>list);
> if (current_rng == rng) {
> drop_current_rng();
> +   cur_rng_set_by_user = 0;
> /* rng_list is sorted by quality, use the best (=first) one */
> if (!list_empty(_list)) {
> struct hwrng *new_rng;
> --
> 2.7.4
>

Looks good to me. Reviewed-by: PrasannaKumar Muralidharan
<prasannatsmku...@gmail.com>.

Regards,
PrasannaKumar

Re: [PATCH 1/3] crypto: hwrng use rng source with best quality

[PrasannaKumar Muralidharan]

On 3 July 2017 at 15:33, Harald Freudenberger <fre...@linux.vnet.ibm.com> wrote:
> This patch rewoks the hwrng to always use the
> rng source with best entropy quality.
>
> On registation and unregistration the hwrng now
> tries to choose the best (= highest quality value)
> rng source. The handling of the internal list
> of registered rng sources is now always sorted
> by quality and the top most rng chosen.
>
> Signed-off-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>
> ---
>  drivers/char/hw_random/core.c | 25 +++--
>  1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 503a41d..e9dda16 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -29,6 +29,7 @@
>
>  static struct hwrng *current_rng;
>  static struct task_struct *hwrng_fill;
> +/* list of registered rngs, sorted decending by quality */
>  static LIST_HEAD(rng_list);
>  /* Protects rng_list and current_rng */
>  static DEFINE_MUTEX(rng_mutex);
> @@ -417,6 +418,7 @@ int hwrng_register(struct hwrng *rng)
>  {
> int err = -EINVAL;
> struct hwrng *old_rng, *tmp;
> +   struct list_head *rng_list_ptr;
>
> if (!rng->name || (!rng->data_read && !rng->read))
> goto out;
> @@ -432,14 +434,25 @@ int hwrng_register(struct hwrng *rng)
> init_completion(>cleanup_done);
> complete(>cleanup_done);
>
> +   /* rng_list is sorted by decreasing quality */
> +   list_for_each(rng_list_ptr, _list) {
> +   tmp = list_entry(rng_list_ptr, struct hwrng, list);
> +   if (tmp->quality < rng->quality)
> +   break;
> +   }
> +   list_add_tail(>list, rng_list_ptr);
> +
> old_rng = current_rng;
> err = 0;
> -   if (!old_rng) {
> +   if (!old_rng || (rng->quality > old_rng->quality)) {
> +   /*
> +* Set new rng as current as the new rng source
> +* provides better entropy quality.
> +*/
> err = set_current_rng(rng);
> if (err)
> goto out_unlock;
> }
> -   list_add_tail(>list, _list);
>
> if (old_rng && !rng->init) {
> /*
> @@ -466,12 +479,12 @@ void hwrng_unregister(struct hwrng *rng)
> list_del(>list);
> if (current_rng == rng) {
> drop_current_rng();
> +   /* rng_list is sorted by quality, use the best (=first) one */
> if (!list_empty(_list)) {
> -   struct hwrng *tail;
> -
> -   tail = list_entry(rng_list.prev, struct hwrng, list);
> +   struct hwrng *new_rng;
>
> -   set_current_rng(tail);
> +   new_rng = list_entry(rng_list.next, struct hwrng, 
> list);
> +   set_current_rng(new_rng);
> }
> }
>
> --
> 2.7.4
>

Looks good to me.
Reviewed-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>.

Regards,
PrasannaKumar

Re: [PATCH 3/3] crypto: hwrng add sysfs attribute to show user selected rng

[PrasannaKumar Muralidharan]

On 3 July 2017 at 15:33, Harald Freudenberger  wrote:
> This patch introduces a new sysfs attribute file 'rng_selected'
> which shows the the rng chosen by userspace.
>
> If a rng source is chosen by user via echo some valid string
> to rng_current there should be a way to signal this choice to
> userspace. The new attribute file 'rng_selected' shows either
> the name of the rng chosen or 'none'.
>
> Signed-off-by: Harald Freudenberger 
> ---
>  drivers/char/hw_random/core.c | 22 +-
>  1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index ffd4e36..6a6276a 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -28,8 +28,8 @@
>  #define RNG_MODULE_NAME"hw_random"
>
>  static struct hwrng *current_rng;
> -/* the current rng has been explicitly chosen by user via sysfs */
> -static int cur_rng_set_by_user;
> +/* the rng explicitly selected by user via sysfs */
> +static struct hwrng *selected_rng;
>  static struct task_struct *hwrng_fill;
>  /* list of registered rngs, sorted decending by quality */
>  static LIST_HEAD(rng_list);
> @@ -306,7 +306,7 @@ static ssize_t hwrng_attr_current_store(struct device 
> *dev,
> list_for_each_entry(rng, _list, list) {
> if (sysfs_streq(rng->name, buf)) {
> err = 0;
> -   cur_rng_set_by_user = 1;
> +   selected_rng = rng;
> if (rng != current_rng)
> err = set_current_rng(rng);
> break;
> @@ -355,16 +355,28 @@ static ssize_t hwrng_attr_available_show(struct device 
> *dev,
> return strlen(buf);
>  }
>
> +static ssize_t hwrng_attr_selected_show(struct device *dev,
> +   struct device_attribute *attr,
> +   char *buf)
> +{
> +   return snprintf(buf, PAGE_SIZE, "%s\n",
> +   selected_rng ? selected_rng->name : "none");
> +}
> +
>  static DEVICE_ATTR(rng_current, S_IRUGO | S_IWUSR,
>hwrng_attr_current_show,
>hwrng_attr_current_store);
>  static DEVICE_ATTR(rng_available, S_IRUGO,
>hwrng_attr_available_show,
>NULL);
> +static DEVICE_ATTR(rng_selected, S_IRUGO,
> +  hwrng_attr_selected_show,
> +  NULL);
>
>  static struct attribute *rng_dev_attrs[] = {
> _attr_rng_current.attr,
> _attr_rng_available.attr,
> +   _attr_rng_selected.attr,
> NULL
>  };
>
> @@ -448,7 +460,7 @@ int hwrng_register(struct hwrng *rng)
> old_rng = current_rng;
> err = 0;
> if (!old_rng ||
> -   (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
> +   (!selected_rng && rng->quality > old_rng->quality)) {
> /*
>  * Set new rng as current as the new rng source
>  * provides better entropy quality and was not
> @@ -484,7 +496,7 @@ void hwrng_unregister(struct hwrng *rng)
> list_del(>list);
> if (current_rng == rng) {
> drop_current_rng();
> -   cur_rng_set_by_user = 0;
> +   selected_rng = NULL;
> /* rng_list is sorted by quality, use the best (=first) one */
> if (!list_empty(_list)) {
> struct hwrng *new_rng;
> --
> 2.7.4
>

The current_rng sysfs attribute shows currently selected rng. So this
new attribute can contain 1 to indicate user's choice, 0 otherwise.

Regards,
PrasannaKumar

Re: [PATCH 1/2] crypto: Make hwrng choose rng source by quality.

[PrasannaKumar Muralidharan]

Hi Harald,

Can you split this patch into two? One patch to choose rng based on
the quality and another for not overriding user decided rng.

Some more minor comments below.

On 29 June 2017 at 15:33, Harald Freudenberger
<fre...@linux.vnet.ibm.com> wrote:
> The hwrng core implementation currently doesn't consider the
> quality field of the struct hwrng. So the first registered rng
> is the winner and further rng sources even with much better
> quality are ignored.
>
> The behavior should be that always the best rng with the highest
> quality rate should be used as current rng source. Only if the
> user explicitly chooses a rng source (via writing a rng name
> to /sys/class/misc/hw_random) the decision for the best quality
> should be suppressed.
>
> This patch makes hwrng always hold a list of registered rng
> sources sorted decreasing by quality. On registration of a new
> hwrng source the list is updated and if the current rng source
> was not chosen by user and the new rng provides better quality
> set as new current rng source. Similar on unregistration of an
> rng, if it was the current used rng source the one with the
> next highest quality is used. If a rng source has been set via
> sysfs from userland as long as this one doesn't unregister
> it is kept as current rng regardless of registration of 'better'
> rng sources.

Nice to see the patch. This is indeed required.

> Signed-off-by: Harald Freudenberger <fre...@linux.vnet.ibm.com>
> ---
>  drivers/char/hw_random/core.c | 31 +--
>  1 file changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 503a41d..7fe47f8 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -28,7 +28,10 @@
>  #define RNG_MODULE_NAME"hw_random"
>
>  static struct hwrng *current_rng;
> +/* the current rng has been explicitly chosen by user via sysfs */
> +static int cur_rng_set_by_user;

Letting the user know that the current rng was selected based on user
input would be a good option I guess. Any thoughts on this?

>  static struct task_struct *hwrng_fill;
> +/* list of registered rngs, sorted decending by quality */
>  static LIST_HEAD(rng_list);
>  /* Protects rng_list and current_rng */
>  static DEFINE_MUTEX(rng_mutex);
> @@ -308,6 +311,8 @@ static ssize_t hwrng_attr_current_store(struct device 
> *dev,
> break;
> }
> }
> +   if (!err)
> +   cur_rng_set_by_user = 1;

This can be put inside the loop. The if condition will go away in that case.

> mutex_unlock(_mutex);
>
> return err ? : len;
> @@ -417,6 +422,7 @@ int hwrng_register(struct hwrng *rng)
>  {
> int err = -EINVAL;
> struct hwrng *old_rng, *tmp;
> +   struct list_head *ptr;

Any better name instead of ptr?

> if (!rng->name || (!rng->data_read && !rng->read))
> goto out;
> @@ -432,14 +438,26 @@ int hwrng_register(struct hwrng *rng)
> init_completion(>cleanup_done);
> complete(>cleanup_done);
>
> +   /* rng_list is sorted by decreasing quality */
> +   list_for_each(ptr, _list) {
> +   tmp = list_entry(ptr, struct hwrng, list);
> +   if (tmp->quality < rng->quality)
> +   break;
> +   }
> +   list_add_tail(>list, ptr);
> +
> old_rng = current_rng;
> err = 0;
> -   if (!old_rng) {
> +   if (!old_rng ||
> +   (!cur_rng_set_by_user && rng->quality > old_rng->quality)) {
> +   /*
> +* Set new rng as current if no current rng or rng was
> +* not chosen by user and the new one has better quality.
> +*/
> err = set_current_rng(rng);
> if (err)
> goto out_unlock;
> }
> -   list_add_tail(>list, _list);
>
> if (old_rng && !rng->init) {
> /*
> @@ -466,12 +484,13 @@ void hwrng_unregister(struct hwrng *rng)
> list_del(>list);
> if (current_rng == rng) {
> drop_current_rng();
> +   cur_rng_set_by_user = 0;
> +   /* rng_list is sorted by quality, use the best (=first) one */
> if (!list_empty(_list)) {
> -   struct hwrng *tail;
> -
> -   tail = list_entry(rng_list.prev, struct hwrng, list);
> +   struct hwrng *new_rng;
>
> -   set_current_rng(tail);
> +   new_rng = list_entry(rng_list.next, struct hwrng, 
> list);
> +   set_current_rng(new_rng);
> }
> }
>
> --
> 2.7.4
>

This patch looks good. I am fine with this patch as is. Reviewed-by:
PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>

If this patch is split into please go ahead and my reviewed-by tag.

Regards,
PrasannaKumar

Re: [PATCH] hwrng: do not warn when there are no devices

[PrasannaKumar Muralidharan]

On 20 June 2017 at 00:33, Mike Frysinger <vap...@chromium.org> wrote:
> On Mon, Jun 19, 2017 at 2:43 AM, PrasannaKumar Muralidharan wrote:
>> On 19 June 2017 at 11:51, Herbert Xu wrote:
>>> On Sun, Jun 18, 2017 at 10:00:17PM -0700, Mike Frysinger wrote:
>>>>
>>>> in order to make tpm-rng react in the way you're implying, the TPM
>>>> subsystem would need to add a notification chain for transitions from
>>>> none<->some devices, then tpm-rng could subscribe to that, and during
>>>> those transition points, it would call hwrng_register/hwrng_unregister
>>>> to make itself visible accordingly to the hwrng subsystem.  maybe
>>>> someone on the TPM side would be interested in writing all that logic,
>>>> but it sounds excessive for this minor usage.  the current tpm-rng
>>>> driver is *extremely* simple -- it's 3 funcs, each of which are 1
>>>> line.
>>>
>>> It's simple and it's broken, as far as the way it hooks into the
>>> hwrng is concerned.
>>
>> *
>> diff --git a/drivers/char/hw_random/tpm-rng.c 
>> b/drivers/char/hw_random/tpm-rng.c
>> index d6d4482..4861b35 100644
>> --- a/drivers/char/hw_random/tpm-rng.c
>> +++ b/drivers/char/hw_random/tpm-rng.c
>> @@ -22,6 +22,10 @@
>>  #include 
>>
>>  #define MODULE_NAME "tpm-rng"
>> +#define MAX_RETRIES 30
>> +
>> +static struct delayed_work check_tpm_work;
>> +static int retry_count;
>>
>>  static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool 
>> wait)
>>  {
>> @@ -33,9 +37,27 @@ static struct hwrng tpm_rng = {
>> .read = tpm_rng_read,
>>  };
>>
>> +static void check_tpm_presence(struct work_struct *work)
>> +{
>> +   u8 data = 0;
>> +   if (tpm_get_random(TPM_ANY_NUM, , 1) > 0) {
>> +   hwrng_register(_rng);
>> +   } else {
>> +   if (retry_count < MAX_RETRIES) {
>> +   retry_count++;
>> +   schedule_delayed_work(_tpm_work, HZ * 10);
>> +   } else {
>> +   pr_err("Could not find any TPM chip, not
>> registering rng");
>> +   }
>> +   }
>> +}
>> +
>>  static int __init rng_init(void)
>>  {
>> -   return hwrng_register(_rng);
>> +   INIT_DELAYED_WORK(_tpm_work, check_tpm_presence);
>> +   check_tpm_presence(NULL);
>> +
>> +   return 0;
>>  }
>>  module_init(rng_init);
>> *
>>
>> Why not something like this? Patch is completely untested. If this
>> idea seems useful I can clean the code but would require help in
>> testing.
>
> first, that's not how deferred device probing works in the kernel.
> drivers shouldn't be doing their own sleeping.  but we can ignore that
> because no amount of delay/retries will work -- TPMs can come & go at
> anytime via hotplugging or module loading/unloading.  so the only way
> to pull it off would be to do something like what i described --
> extending the tpm framework so that it can signal children to come
> up/go down.

If TPM can come and go then notification or callback is the correct
way to handle this case.

> imo, standing all of that up is over-engineering and not worth the
> effort, so i'm not going to do it.  but maybe you can convince some of
> the TPM maintainers it's worthwhile.

Okay.

Thanks,
PrasannaKumar

Re: [PATCH] hwrng: do not warn when there are no devices

[PrasannaKumar Muralidharan]

On 19 June 2017 at 11:51, Herbert Xu  wrote:
> On Sun, Jun 18, 2017 at 10:00:17PM -0700, Mike Frysinger wrote:
>>
>> in order to make tpm-rng react in the way you're implying, the TPM
>> subsystem would need to add a notification chain for transitions from
>> none<->some devices, then tpm-rng could subscribe to that, and during
>> those transition points, it would call hwrng_register/hwrng_unregister
>> to make itself visible accordingly to the hwrng subsystem.  maybe
>> someone on the TPM side would be interested in writing all that logic,
>> but it sounds excessive for this minor usage.  the current tpm-rng
>> driver is *extremely* simple -- it's 3 funcs, each of which are 1
>> line.
>
> It's simple and it's broken, as far as the way it hooks into the
> hwrng is concerned.

*
diff --git a/drivers/char/hw_random/tpm-rng.c b/drivers/char/hw_random/tpm-rng.c
index d6d4482..4861b35 100644
--- a/drivers/char/hw_random/tpm-rng.c
+++ b/drivers/char/hw_random/tpm-rng.c
@@ -22,6 +22,10 @@
 #include 

 #define MODULE_NAME "tpm-rng"
+#define MAX_RETRIES 30
+
+static struct delayed_work check_tpm_work;
+static int retry_count;

 static int tpm_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
@@ -33,9 +37,27 @@ static struct hwrng tpm_rng = {
.read = tpm_rng_read,
 };

+static void check_tpm_presence(struct work_struct *work)
+{
+   u8 data = 0;
+   if (tpm_get_random(TPM_ANY_NUM, , 1) > 0) {
+   hwrng_register(_rng);
+   } else {
+   if (retry_count < MAX_RETRIES) {
+   retry_count++;
+   schedule_delayed_work(_tpm_work, HZ * 10);
+   } else {
+   pr_err("Could not find any TPM chip, not
registering rng");
+   }
+   }
+}
+
 static int __init rng_init(void)
 {
-   return hwrng_register(_rng);
+   INIT_DELAYED_WORK(_tpm_work, check_tpm_presence);
+   check_tpm_presence(NULL);
+
+   return 0;
 }
 module_init(rng_init);
*

Why not something like this? Patch is completely untested. If this
idea seems useful I can clean the code but would require help in
testing.

Regards,
PrasannaKumar

Re: [PATCH v6 4/5] crypto: inside-secure: add SafeXcel EIP197 crypto engine driver

[PrasannaKumar Muralidharan]

On 29 May 2017 at 14:51, Antoine Tenart
 wrote:
>> As you have got help from other people for testing, wouldn't it be
>> nice to add tested-by tag?
>
> Well, they're listed as authors of the driver: not only they helped to
> test it but they developed parts of it. The tested-by tag doesn't apply
> here.

Makes sense.

Regards,
PrasannaKumar

Re: [PATCH v6 4/5] crypto: inside-secure: add SafeXcel EIP197 crypto engine driver

[PrasannaKumar Muralidharan]

Hi Antonie,

On 24 May 2017 at 19:40, Antoine Tenart
 wrote:
> Add support for Inside Secure SafeXcel EIP197 cryptographic engine,
> which can be found on Marvell Armada 7k and 8k boards. This driver
> currently implements: ecb(aes), cbc(aes), sha1, sha224, sha256 and
> hmac(sah1) algorithms.
>
> Two firmwares are needed for this engine to work. Their are mostly used
> for more advanced operations than the ones supported (as of now), but we
> still need them to pass the data to the internal cryptographic engine.
>
> Signed-off-by: Antoine Tenart 

As you have got help from other people for testing, wouldn't it be
nice to add tested-by tag?

Regards,
PrasannaKumar

Re: [PATCH] crypto: exynoes-rng: Set cra_ctxsize to 0

[PrasannaKumar Muralidharan]

On 21 May 2017 at 12:44, Krzysztof Kozlowski <k...@kernel.org> wrote:
> On Sun, May 21, 2017 at 9:11 AM, PrasannaKumar Muralidharan
> <prasannatsmku...@gmail.com> wrote:
>> Hi Krzysztof
>>
>> On 21 May 2017 at 11:56, Krzysztof Kozlowski <k...@kernel.org> wrote:
>>> On Sun, May 21, 2017 at 8:09 AM, PrasannaKumar Muralidharan
>>> <prasannatsmku...@gmail.com> wrote:
>>>> As cra_ctxsize is set but the allocated space is not used, set it 0.
>>>
>>> Why do you think it is not used? Did you test our change on hardware?
>>
>> Had a look at the crypto rng code. I think the additional size is used
>> to store driver private data. But this driver does not store any
>> private data in the crypto_tfm structure so I think the 'cra_ctxsize'
>> can be safely set to 0.
>
> Then from where does crypto_tfm_ctx() get its memory?

Ah, yes. Thanks for pointing out. I overlooked this. My patch is
completely wrong.

>> I do not have access to the hardware, did not test the change. Sorry I
>> forgot to mention that.
>
> That is quite important... By default everything must be tested so if
> you are skipping this step then please mark the patch respectively so
> others will provide testing.

Sure. Will keep that in mind. Instead of marking it as a [PATCH]
should I use something else for this? Will it make things easier?

Thanks,
PrasannaKumar

Re: [PATCH] crypto: exynoes-rng: Set cra_ctxsize to 0

[PrasannaKumar Muralidharan]

Hi Krzysztof

On 21 May 2017 at 11:56, Krzysztof Kozlowski <k...@kernel.org> wrote:
> On Sun, May 21, 2017 at 8:09 AM, PrasannaKumar Muralidharan
> <prasannatsmku...@gmail.com> wrote:
>> As cra_ctxsize is set but the allocated space is not used, set it 0.
>
> Why do you think it is not used? Did you test our change on hardware?

Had a look at the crypto rng code. I think the additional size is used
to store driver private data. But this driver does not store any
private data in the crypto_tfm structure so I think the 'cra_ctxsize'
can be safely set to 0.

I do not have access to the hardware, did not test the change. Sorry I
forgot to mention that.

> Best regards,
> Krzysztof
>
>>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmku...@gmail.com>
>> ---
>>  drivers/crypto/exynos-rng.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/crypto/exynos-rng.c b/drivers/crypto/exynos-rng.c
>> index 451620b..d009df6 100644
>> --- a/drivers/crypto/exynos-rng.c
>> +++ b/drivers/crypto/exynos-rng.c
>> @@ -260,7 +260,7 @@ static int exynos_rng_kcapi_init(struct crypto_tfm *tfm)
>> .cra_name   = "stdrng",
>> .cra_driver_name= "exynos_rng",
>> .cra_priority   = 100,
>> -   .cra_ctxsize= sizeof(struct exynos_rng_ctx),
>> +   .cra_ctxsize= 0,
>> .cra_module = THIS_MODULE,
>> .cra_init   = exynos_rng_kcapi_init,
>> }
>> --
>> 1.8.5.6
>>

Regards,
PrasannaKumar