[PATCH] [REPOST] timer iomem hwrng driver

[PATCH] [REPOST] timer iomem hwrng driver

[Alexander Clouter]

Hi,

I submitted this some time back but got no 'love' from the community[1] 
so I'm reposting it.

Some hardware platforms, the TS-7800[2] is one for example, can supply 
the kernel with an entropy source, albeit a slow one for TS-7800 users, 
by just reading a particular IO address.  This source must not be read 
above a certain rate otherwise the quality is not suitable.

The driver is then hooked into by calling
platform_device_(register|add|del) passing a structure similar to: 
------------
#define TS_RNG		(TS78XX_FPGA_REGS_VIRT_BASE | 0x044)

static struct timeriomem_rng_data ts78xx_ts_rng_data = {
	.address	= (u32 *__iomem) TS_RNG,
	.period		= 1000000, /* one second */
};

static struct platform_device ts78xx_ts_rng_device = {
	.name		= "timeriomem_rng",
	.id		= -1,
	.dev		= {
		.platform_data	= &ts78xx_ts_rng_data,
	},
	.num_resources  = 0,
};
------------

Feedback welcomed.

Cheers

Alex

[1] http://lkml.org/lkml/2008/6/15/106
[2] http://www.embeddedarm.com/products/board-detail.php?product=TS-7800

diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 8822eca..638e060 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -20,6 +20,19 @@ config HW_RANDOM
 
 	  If unsure, say Y.
 
+config HW_RANDOM_TIMERIOMEM
+	tristate "Timer IOMEM HW Random Number Generator support"
+	depends on HW_RANDOM
+	---help---
+	  This driver provides kernel-side support for a generic Random
+	  Number Generator provisioned by hardware through a 'dumb' iomem
+	  address; for example the TS-7800 is such a device.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called timeriomem-rng.
+
+	  If unsure, say Y.
+
 config HW_RANDOM_INTEL
 	tristate "Intel HW Random Number Generator support"
 	depends on HW_RANDOM && (X86 || IA64) && PCI
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index b6effb7..e81d21a 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -4,6 +4,7 @@
 
 obj-$(CONFIG_HW_RANDOM) += rng-core.o
 rng-core-y := core.o
+obj-$(CONFIG_HW_RANDOM_TIMERIOMEM) += timeriomem-rng.o
 obj-$(CONFIG_HW_RANDOM_INTEL) += intel-rng.o
 obj-$(CONFIG_HW_RANDOM_AMD) += amd-rng.o
 obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
diff --git a/drivers/char/hw_random/timeriomem-rng.c b/drivers/char/hw_random/timeriomem-rng.c
new file mode 100644
index 0000000..42f2813
--- /dev/null
+++ b/drivers/char/hw_random/timeriomem-rng.c
@@ -0,0 +1,153 @@
+/*
+ * drivers/char/hw_random/timeriomem-rng.c
+ *
+ * Copyright (C) 2008 Alexander Clouter <alex@digriz.org.uk>
+ *
+ * Derived from drivers/char/hw_random/omap-rng.c
+ *   Copyright 2005 (c) MontaVista Software, Inc.
+ *   Author: Deepak Saxena <dsaxena@plexity.net>
+ *
+ * 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.
+ *
+ * Overview:
+ *   This driver is useful for platforms that have an IO range that provides
+ *   periodic random data from a single IO memory address.  All the platform
+ *   has to do is provide the address and 'wait time' that new data becomes
+ *   available.
+ *
+ * TODO: add support for reading sizes other than 32bits and masking
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/hw_random.h>
+#include <linux/io.h>
+#include <linux/timeriomem-rng.h>
+#include <linux/jiffies.h>
+#include <linux/sched.h>
+#include <linux/timer.h>
+
+static struct timeriomem_rng_data *timeriomem_rng_data;
+
+static void timeriomem_rng_trigger(unsigned long);
+static DEFINE_TIMER(timeriomem_rng_timer, &timeriomem_rng_trigger, 0, 0);
+
+/*
+ * have data return 1, however return 0 if we have nothing
+ */
+static int timeriomem_rng_data_present(struct hwrng *rng, int wait)
+{
+	s32 delay;
+
+	if (rng->priv == 0)
+		return 1;
+
+	if (timer_pending(&timeriomem_rng_timer)) {
+		if (!wait)
+			return 0;
+
+		del_timer(&timeriomem_rng_timer);
+		delay = (long)timeriomem_rng_timer.expires - (long)jiffies;
+
+		schedule_timeout_uninterruptible(delay);
+	}
+
+	return 1;
+}
+
+static int timeriomem_rng_data_read(struct hwrng *rng, u32 *data)
+{
+	u32 cur;
+	s32 delay;
+
+	*data = *timeriomem_rng_data->address;
+
+	if (rng->priv != 0) {
+		cur = jiffies;
+
+		delay = (long)cur - (long)timeriomem_rng_timer.expires;
+		delay = rng->priv - (delay % rng->priv);
+
+		timeriomem_rng_timer.expires = cur + delay;
+		add_timer(&timeriomem_rng_timer);
+	}
+
+	return 4;
+}
+
+static void timeriomem_rng_trigger(unsigned long dummy)
+{
+	del_timer(&timeriomem_rng_timer);
+}
+
+static struct hwrng timeriomem_rng_ops = {
+	.name		= "timeriomem",
+	.data_present	= timeriomem_rng_data_present,
+	.data_read	= timeriomem_rng_data_read,
+	.priv		= 0,
+};
+
+static int __init timeriomem_rng_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	timeriomem_rng_data = pdev->dev.platform_data;
+
+	if (timeriomem_rng_data->period != 0
+		&& usecs_to_jiffies(timeriomem_rng_data->period) > 0) {
+		timeriomem_rng_timer.expires = jiffies;
+		init_timer(&timeriomem_rng_timer);
+
+		timeriomem_rng_ops.priv = usecs_to_jiffies(
+						timeriomem_rng_data->period);
+	}
+
+	ret = hwrng_register(&timeriomem_rng_ops);
+	if (ret) {
+		dev_err(&pdev->dev, "problem registering\n");
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "32bits from 0x%p @ %dus\n",
+			timeriomem_rng_data->address,
+			timeriomem_rng_data->period);
+
+	return 0;
+}
+
+static int __devexit timeriomem_rng_remove(struct platform_device *pdev)
+{
+	del_timer(&timeriomem_rng_timer);
+	hwrng_unregister(&timeriomem_rng_ops);
+
+	return 0;
+}
+
+static struct platform_driver timeriomem_rng_driver = {
+	.driver = {
+		.name		= "timeriomem_rng",
+		.owner		= THIS_MODULE,
+	},
+	.probe		= timeriomem_rng_probe,
+	.remove		= __devexit_p(timeriomem_rng_remove),
+};
+
+static int __init timeriomem_rng_init(void)
+{
+	return platform_driver_register(&timeriomem_rng_driver);
+}
+
+static void __exit timeriomem_rng_exit(void)
+{
+	platform_driver_unregister(&timeriomem_rng_driver);
+}
+
+module_init(timeriomem_rng_init);
+module_exit(timeriomem_rng_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Alexander Clouter <alex@digriz.org.uk>");
+MODULE_DESCRIPTION("Timer IOMEM H/W RNG driver");
diff --git a/include/linux/timeriomem-rng.h b/include/linux/timeriomem-rng.h
new file mode 100644
index 0000000..16dd9e4
--- /dev/null
+++ b/include/linux/timeriomem-rng.h
@@ -0,0 +1,16 @@
+/*
+ * linux/include/linux/timeriomem-rng.h
+ *
+ * Copyright (c) 2008 Alexander Clouter <alex@digriz.org.uk>
+ *
+ * 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.
+ */
+
+struct timeriomem_rng_data {
+	u32 __iomem	*address;
+
+	/* measures in usecs */
+	unsigned int	period;
+};

Re: [REPOST^3] [PATCH] timer iomem hwrng driver

[Alexander Clouter]

Alexander Clouter <alex@digriz.org.uk> wrote:
>
> I submitted this some time back but got no 'love' from the community[1] 
> so I'm reposting it.
> 
Please, can *someone* give me some feedback.  After the second time of
submitting this code still no-one has any feedback.  It really really is 
demoralising.

I understand this is more or less a patch to an orphaned subsystem but I 
have also made submissions to ARM and MTD and it's as if I have the 
'spot' :(

Cheers

Alex

> Some hardware platforms, the TS-7800[2] is one for example, can supply 
> the kernel with an entropy source, albeit a slow one for TS-7800 users, 
> by just reading a particular IO address.  This source must not be read 
> above a certain rate otherwise the quality is not suitable.
> 
> The driver is then hooked into by calling
> platform_device_(register|add|del) passing a structure similar to: 
> ------------
> #define TS_RNG          (TS78XX_FPGA_REGS_VIRT_BASE | 0x044)
> 
> static struct timeriomem_rng_data ts78xx_ts_rng_data = {
>        .address        = (u32 *__iomem) TS_RNG,
>        .period         = 1000000, /* one second */
> };
> 
> static struct platform_device ts78xx_ts_rng_device = {
>        .name           = "timeriomem_rng",
>        .id             = -1,
>        .dev            = {
>                .platform_data  = &ts78xx_ts_rng_data,
>        },
>        .num_resources  = 0,
> };
> ------------
> 
> Feedback welcomed.
> 
> Cheers
> 
> Alex
> 
> [1] http://lkml.org/lkml/2008/6/15/106
> [2] http://www.embeddedarm.com/products/board-detail.php?product=TS-7800
> 
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 8822eca..638e060 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -20,6 +20,19 @@ config HW_RANDOM
> 
>          If unsure, say Y.
> 
> +config HW_RANDOM_TIMERIOMEM
> +       tristate "Timer IOMEM HW Random Number Generator support"
> +       depends on HW_RANDOM
> +       ---help---
> +         This driver provides kernel-side support for a generic Random
> +         Number Generator provisioned by hardware through a 'dumb' iomem
> +         address; for example the TS-7800 is such a device.
> +
> +         To compile this driver as a module, choose M here: the
> +         module will be called timeriomem-rng.
> +
> +         If unsure, say Y.
> +
> config HW_RANDOM_INTEL
>        tristate "Intel HW Random Number Generator support"
>        depends on HW_RANDOM && (X86 || IA64) && PCI
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index b6effb7..e81d21a 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -4,6 +4,7 @@
> 
> obj-$(CONFIG_HW_RANDOM) += rng-core.o
> rng-core-y := core.o
> +obj-$(CONFIG_HW_RANDOM_TIMERIOMEM) += timeriomem-rng.o
> obj-$(CONFIG_HW_RANDOM_INTEL) += intel-rng.o
> obj-$(CONFIG_HW_RANDOM_AMD) += amd-rng.o
> obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
> diff --git a/drivers/char/hw_random/timeriomem-rng.c b/drivers/char/hw_random/timeriomem-rng.c
> new file mode 100644
> index 0000000..42f2813
> --- /dev/null
> +++ b/drivers/char/hw_random/timeriomem-rng.c
> @@ -0,0 +1,153 @@
> +/*
> + * drivers/char/hw_random/timeriomem-rng.c
> + *
> + * Copyright (C) 2008 Alexander Clouter <alex@digriz.org.uk>
> + *
> + * Derived from drivers/char/hw_random/omap-rng.c
> + *   Copyright 2005 (c) MontaVista Software, Inc.
> + *   Author: Deepak Saxena <dsaxena@plexity.net>
> + *
> + * 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.
> + *
> + * Overview:
> + *   This driver is useful for platforms that have an IO range that provides
> + *   periodic random data from a single IO memory address.  All the platform
> + *   has to do is provide the address and 'wait time' that new data becomes
> + *   available.
> + *
> + * TODO: add support for reading sizes other than 32bits and masking
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/hw_random.h>
> +#include <linux/io.h>
> +#include <linux/timeriomem-rng.h>
> +#include <linux/jiffies.h>
> +#include <linux/sched.h>
> +#include <linux/timer.h>
> +
> +static struct timeriomem_rng_data *timeriomem_rng_data;
> +
> +static void timeriomem_rng_trigger(unsigned long);
> +static DEFINE_TIMER(timeriomem_rng_timer, &timeriomem_rng_trigger, 0, 0);
> +
> +/*
> + * have data return 1, however return 0 if we have nothing
> + */
> +static int timeriomem_rng_data_present(struct hwrng *rng, int wait)
> +{
> +       s32 delay;
> +
> +       if (rng->priv == 0)
> +               return 1;
> +
> +       if (timer_pending(&timeriomem_rng_timer)) {
> +               if (!wait)
> +                       return 0;
> +
> +               del_timer(&timeriomem_rng_timer);
> +               delay = (long)timeriomem_rng_timer.expires - (long)jiffies;
> +
> +               schedule_timeout_uninterruptible(delay);
> +       }
> +
> +       return 1;
> +}
> +
> +static int timeriomem_rng_data_read(struct hwrng *rng, u32 *data)
> +{
> +       u32 cur;
> +       s32 delay;
> +
> +       *data = *timeriomem_rng_data->address;
> +
> +       if (rng->priv != 0) {
> +               cur = jiffies;
> +
> +               delay = (long)cur - (long)timeriomem_rng_timer.expires;
> +               delay = rng->priv - (delay % rng->priv);
> +
> +               timeriomem_rng_timer.expires = cur + delay;
> +               add_timer(&timeriomem_rng_timer);
> +       }
> +
> +       return 4;
> +}
> +
> +static void timeriomem_rng_trigger(unsigned long dummy)
> +{
> +       del_timer(&timeriomem_rng_timer);
> +}
> +
> +static struct hwrng timeriomem_rng_ops = {
> +       .name           = "timeriomem",
> +       .data_present   = timeriomem_rng_data_present,
> +       .data_read      = timeriomem_rng_data_read,
> +       .priv           = 0,
> +};
> +
> +static int __init timeriomem_rng_probe(struct platform_device *pdev)
> +{
> +       int ret;
> +
> +       timeriomem_rng_data = pdev->dev.platform_data;
> +
> +       if (timeriomem_rng_data->period != 0
> +               && usecs_to_jiffies(timeriomem_rng_data->period) > 0) {
> +               timeriomem_rng_timer.expires = jiffies;
> +               init_timer(&timeriomem_rng_timer);
> +
> +               timeriomem_rng_ops.priv = usecs_to_jiffies(
> +                                               timeriomem_rng_data->period);
> +       }
> +
> +       ret = hwrng_register(&timeriomem_rng_ops);
> +       if (ret) {
> +               dev_err(&pdev->dev, "problem registering\n");
> +               return ret;
> +       }
> +
> +       dev_info(&pdev->dev, "32bits from 0x%p @ %dus\n",
> +                       timeriomem_rng_data->address,
> +                       timeriomem_rng_data->period);
> +
> +       return 0;
> +}
> +
> +static int __devexit timeriomem_rng_remove(struct platform_device *pdev)
> +{
> +       del_timer(&timeriomem_rng_timer);
> +       hwrng_unregister(&timeriomem_rng_ops);
> +
> +       return 0;
> +}
> +
> +static struct platform_driver timeriomem_rng_driver = {
> +       .driver = {
> +               .name           = "timeriomem_rng",
> +               .owner          = THIS_MODULE,
> +       },
> +       .probe          = timeriomem_rng_probe,
> +       .remove         = __devexit_p(timeriomem_rng_remove),
> +};
> +
> +static int __init timeriomem_rng_init(void)
> +{
> +       return platform_driver_register(&timeriomem_rng_driver);
> +}
> +
> +static void __exit timeriomem_rng_exit(void)
> +{
> +       platform_driver_unregister(&timeriomem_rng_driver);
> +}
> +
> +module_init(timeriomem_rng_init);
> +module_exit(timeriomem_rng_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Alexander Clouter <alex@digriz.org.uk>");
> +MODULE_DESCRIPTION("Timer IOMEM H/W RNG driver");
> diff --git a/include/linux/timeriomem-rng.h b/include/linux/timeriomem-rng.h
> new file mode 100644
> index 0000000..16dd9e4
> --- /dev/null
> +++ b/include/linux/timeriomem-rng.h
> @@ -0,0 +1,16 @@
> +/*
> + * linux/include/linux/timeriomem-rng.h
> + *
> + * Copyright (c) 2008 Alexander Clouter <alex@digriz.org.uk>
> + *
> + * 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.
> + */
> +
> +struct timeriomem_rng_data {
> +       u32 __iomem     *address;
> +
> +       /* measures in usecs */
> +       unsigned int    period;
> +};

-- 
Alexander Clouter
.sigmonster says: It's more than magnificent -- it's mediocre.
                  		-- Sam Goldwyn

Re: [REPOST^3] [PATCH] timer iomem hwrng driver

[Andrew Morton]

On Mon, 5 Jan 2009 14:30:13 +0000 Alexander Clouter <alex@digriz.org.uk> wrote:

> Alexander Clouter <alex@digriz.org.uk> wrote:
> >
> > I submitted this some time back but got no 'love' from the community[1] 
> > so I'm reposting it.
> > 
> Please, can *someone* give me some feedback

Yup, I'll take a look.

Re: [PATCH] [REPOST] timer iomem hwrng driver

[Andrew Morton]

On Tue, 30 Dec 2008 14:52:10 +0000
Alexander Clouter <alex@digriz.org.uk> wrote:

> Hi,
> 
> I submitted this some time back but got no 'love' from the community[1] 
> so I'm reposting it.
> 
> Some hardware platforms, the TS-7800[2] is one for example, can supply 
> the kernel with an entropy source, albeit a slow one for TS-7800 users, 
> by just reading a particular IO address.  This source must not be read 
> above a certain rate otherwise the quality is not suitable.
> 
> The driver is then hooked into by calling
> platform_device_(register|add|del) passing a structure similar to: 
> ------------
> #define TS_RNG		(TS78XX_FPGA_REGS_VIRT_BASE | 0x044)
> 
> static struct timeriomem_rng_data ts78xx_ts_rng_data = {
> 	.address	= (u32 *__iomem) TS_RNG,
> 	.period		= 1000000, /* one second */
> };
> 
> static struct platform_device ts78xx_ts_rng_device = {
> 	.name		= "timeriomem_rng",
> 	.id		= -1,
> 	.dev		= {
> 		.platform_data	= &ts78xx_ts_rng_data,
> 	},
> 	.num_resources  = 0,
> };
> ------------
> 

questions...

> +++ b/drivers/char/hw_random/timeriomem-rng.c
> @@ -0,0 +1,153 @@
> +/*
> + * drivers/char/hw_random/timeriomem-rng.c
> + *
> + * Copyright (C) 2008 Alexander Clouter <alex@digriz.org.uk>
> + *
> + * Derived from drivers/char/hw_random/omap-rng.c
> + *   Copyright 2005 (c) MontaVista Software, Inc.
> + *   Author: Deepak Saxena <dsaxena@plexity.net>
> + *
> + * 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.
> + *
> + * Overview:
> + *   This driver is useful for platforms that have an IO range that provides
> + *   periodic random data from a single IO memory address.  All the platform
> + *   has to do is provide the address and 'wait time' that new data becomes
> + *   available.
> + *
> + * TODO: add support for reading sizes other than 32bits and masking
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/hw_random.h>
> +#include <linux/io.h>
> +#include <linux/timeriomem-rng.h>
> +#include <linux/jiffies.h>
> +#include <linux/sched.h>
> +#include <linux/timer.h>
> +
> +static struct timeriomem_rng_data *timeriomem_rng_data;
> +
> +static void timeriomem_rng_trigger(unsigned long);
> +static DEFINE_TIMER(timeriomem_rng_timer, &timeriomem_rng_trigger, 0, 0);
> +
> +/*
> + * have data return 1, however return 0 if we have nothing
> + */
> +static int timeriomem_rng_data_present(struct hwrng *rng, int wait)
> +{
> +	s32 delay;
> +
> +	if (rng->priv == 0)
> +		return 1;
> +
> +	if (timer_pending(&timeriomem_rng_timer)) {
> +		if (!wait)
> +			return 0;
> +
> +		del_timer(&timeriomem_rng_timer);
> +		delay = (long)timeriomem_rng_timer.expires - (long)jiffies;
> +
> +		schedule_timeout_uninterruptible(delay);
> +	}
> +
> +	return 1;
> +}

Would it be better (less racy) to do

	if (del_timer(&timeriomem_rng_timer)) {
		if (!wait)
			return 0;

		delay = (long)timeriomem_rng_timer.expires - (long)jiffies;

		schedule_timeout_uninterruptible(delay);
	}

Secondly, can `delay' be negative, if jiffies increments at just the
right (ie: wrong) time?

Thirdly, why the typecasts in the calculation of `delay'?  Both terms
already have type `unsigned long'.

Fourthly, should it use del_timer_sync()?  Bear in mind that the timer
handler might be concurrently running on another CPU.

> +static int timeriomem_rng_data_read(struct hwrng *rng, u32 *data)
> +{
> +	u32 cur;
> +	s32 delay;
> +
> +	*data = *timeriomem_rng_data->address;

This is reading from I/O memory.  It should use readl()?

> +	if (rng->priv != 0) {
> +		cur = jiffies;
> +
> +		delay = (long)cur - (long)timeriomem_rng_timer.expires;

bug: `cur' should have type `unsigned long'.  The u32 can get truncated.

Then, the casts are unneeded.

> +		delay = rng->priv - (delay % rng->priv);
> +
> +		timeriomem_rng_timer.expires = cur + delay;
> +		add_timer(&timeriomem_rng_timer);
> +	}
> +
> +	return 4;
> +}
> +
> +static void timeriomem_rng_trigger(unsigned long dummy)
> +{
> +	del_timer(&timeriomem_rng_timer);
> +}

del_timer_sync()?

> +static struct hwrng timeriomem_rng_ops = {
> +	.name		= "timeriomem",
> +	.data_present	= timeriomem_rng_data_present,
> +	.data_read	= timeriomem_rng_data_read,
> +	.priv		= 0,
> +};
> +
> +static int __init timeriomem_rng_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	timeriomem_rng_data = pdev->dev.platform_data;
> +
> +	if (timeriomem_rng_data->period != 0
> +		&& usecs_to_jiffies(timeriomem_rng_data->period) > 0) {
> +		timeriomem_rng_timer.expires = jiffies;
> +		init_timer(&timeriomem_rng_timer);

I don't think the init_timer() is needed - we already (correctly)
initialised it at compile time?

> +		timeriomem_rng_ops.priv = usecs_to_jiffies(
> +						timeriomem_rng_data->period);
> +	}
> +
> +	ret = hwrng_register(&timeriomem_rng_ops);
> +	if (ret) {
> +		dev_err(&pdev->dev, "problem registering\n");
> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "32bits from 0x%p @ %dus\n",
> +			timeriomem_rng_data->address,
> +			timeriomem_rng_data->period);
> +
> +	return 0;
> +}

What will happen if we load this driver on machines which don't
actually have the necessary hardware?  Even non-x86 hardware?

> +static int __devexit timeriomem_rng_remove(struct platform_device *pdev)
> +{
> +	del_timer(&timeriomem_rng_timer);

This should be del_timer_sync().  Otherwise the timer handler could be
running on another CPU during driver teardown.

> +	hwrng_unregister(&timeriomem_rng_ops);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver timeriomem_rng_driver = {
> +	.driver = {
> +		.name		= "timeriomem_rng",
> +		.owner		= THIS_MODULE,
> +	},
> +	.probe		= timeriomem_rng_probe,
> +	.remove		= __devexit_p(timeriomem_rng_remove),
> +};
> +
> +static int __init timeriomem_rng_init(void)
> +{
> +	return platform_driver_register(&timeriomem_rng_driver);
> +}
> +
> +static void __exit timeriomem_rng_exit(void)
> +{
> +	platform_driver_unregister(&timeriomem_rng_driver);
> +}
> +
> +module_init(timeriomem_rng_init);
> +module_exit(timeriomem_rng_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Alexander Clouter <alex@digriz.org.uk>");
> +MODULE_DESCRIPTION("Timer IOMEM H/W RNG driver");
> diff --git a/include/linux/timeriomem-rng.h b/include/linux/timeriomem-rng.h
> new file mode 100644
> index 0000000..16dd9e4
> --- /dev/null
> +++ b/include/linux/timeriomem-rng.h
> @@ -0,0 +1,16 @@
> +/*
> + * linux/include/linux/timeriomem-rng.h
> + *
> + * Copyright (c) 2008 Alexander Clouter <alex@digriz.org.uk>
> + *
> + * 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.
> + */
> +
> +struct timeriomem_rng_data {
> +	u32 __iomem	*address;
> +
> +	/* measures in usecs */
> +	unsigned int	period;
> +};

Re: [PATCH] [REPOST] timer iomem hwrng driver

[Alexander Clouter]

Sorry for the slow reply. <insert-usual-excuses/>

Thanks though for picking this up and finding the time to have a nosey 
at it.

* Andrew Morton <akpm@linux-foundation.org> [Mon, 12 Jan 2009 13:13:21 -0800]:
>
>> +/*
>> + * have data return 1, however return 0 if we have nothing
>> + */
>> +static int timeriomem_rng_data_present(struct hwrng *rng, int wait)
>> +{
>> +	s32 delay;
>> +
>> +	if (rng->priv == 0)
>> +		return 1;
>> +
>> +	if (timer_pending(&timeriomem_rng_timer)) {
>> +		if (!wait)
>> +			return 0;
>> +
>> +		del_timer(&timeriomem_rng_timer);
>> +		delay = (long)timeriomem_rng_timer.expires - (long)jiffies;
>> +
>> +		schedule_timeout_uninterruptible(delay);
>> +	}
>> +
>> +	return 1;
>> +}
>
> Would it be better (less racy) to do
>
> 	if (del_timer(&timeriomem_rng_timer)) {
> 		if (!wait)
> 			return 0;
>
> 		delay = (long)timeriomem_rng_timer.expires - (long)jiffies;
>
> 		schedule_timeout_uninterruptible(delay);
> 	}
>
Agreed.

> Secondly, can `delay' be negative, if jiffies increments at just the
> right (ie: wrong) time?
>
I thought long and hard about this when I initially put the code 
together, I thought that it could only go very large (when jiffies 
wraps) but never negative as I could not think of a case unless people 
had a source that provided new data at intervals of time where jiffies 
is larger than 2^32...hell you might as well not bother with that source 
:)

Of course I should handle the wrap case, which I have already done at 
least one (with an interval period of one second)...seemed to work fine.

> Thirdly, why the typecasts in the calculation of `delay'?  Both terms
> already have type `unsigned long'.
>
Blind following of whats going on in linux/jiffies.h.  If they can be 
dropped then that's fine with me, I thought 'jiffies' was possibly 
64bit on a platform or two (sparc64 and alpha for example).

> Fourthly, should it use del_timer_sync()?  Bear in mind that the timer
> handler might be concurrently running on another CPU.
>
Will do, had no idea about that.

>> +static int timeriomem_rng_data_read(struct hwrng *rng, u32 *data)
>> +{
>> +	u32 cur;
>> +	s32 delay;
>> +
>> +	*data = *timeriomem_rng_data->address;
>
> This is reading from I/O memory.  It should use readl()?
>
Fixed.

>> +	if (rng->priv != 0) {
>> +		cur = jiffies;
>> +
>> +		delay = (long)cur - (long)timeriomem_rng_timer.expires;
>
> bug: `cur' should have type `unsigned long'.  The u32 can get truncated.
>
> Then, the casts are unneeded.
>
Fixed.

>> +		delay = rng->priv - (delay % rng->priv);
>> +
>> +		timeriomem_rng_timer.expires = cur + delay;
>> +		add_timer(&timeriomem_rng_timer);
>> +	}
>> +
>> +	return 4;
>> +}
>> +
>> +static void timeriomem_rng_trigger(unsigned long dummy)
>> +{
>> +	del_timer(&timeriomem_rng_timer);
>> +}
>
> del_timer_sync()?
>
Check.

>> +static struct hwrng timeriomem_rng_ops = {
>> +	.name		= "timeriomem",
>> +	.data_present	= timeriomem_rng_data_present,
>> +	.data_read	= timeriomem_rng_data_read,
>> +	.priv		= 0,
>> +};
>> +
>> +static int __init timeriomem_rng_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +
>> +	timeriomem_rng_data = pdev->dev.platform_data;
>> +
>> +	if (timeriomem_rng_data->period != 0
>> +		&& usecs_to_jiffies(timeriomem_rng_data->period) > 0) {
>> +		timeriomem_rng_timer.expires = jiffies;
>> +		init_timer(&timeriomem_rng_timer);
>
> I don't think the init_timer() is needed - we already (correctly)
> initialised it at compile time?
>
Again, I had no idea, I was reading a tutorial on timers[1] and it said 
init_timer().  "Just following orders capt'in".

I have removed the init_timer() and you are right, it is unneeded.

> What will happen if we load this driver on machines which don't
> actually have the necessary hardware?  Even non-x86 hardware?
>
Cooks on my development ARM board with no problems, no idea if it works 
on x86 though :) As for not having the hardware present, it's a platform 
driver, surely it would be the fault of the crazed platform writer who 
would to do something like this?

>> +static int __devexit timeriomem_rng_remove(struct platform_device *pdev)
>> +{
>> +	del_timer(&timeriomem_rng_timer);
>
> This should be del_timer_sync().  Otherwise the timer handler could be
> running on another CPU during driver teardown.
>
Check.

Thanks again for having a look at my module.  All I need is some further 
feedback on the timer wrap bit (just ran it again now and it seems to 
work still as expected after five minutes) and then I will re-submit.

Cheers

[1] http://lwn.net/images/pdf/LDD3/ch07.pdf

-- 
Alexander Clouter
.sigmonster says: Don't be overly suspicious where it's not warranted.