Skip to contentSkip to conversation

timer iomem hwrng driver

PATCH In review Alexander Clouter 2 participants 5 messages Dec 30, 2008 – Jan 21, 2009 Hardware random number generator core

lore
Share & tools
lore

Alexander Clouter

author
#0 lore raw

PATCH 1/1 timer iomem hwrng driver

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

drivers/char/hw_random/Kconfig +13 -0
index 8822eca..638e060 100644
@@ -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
drivers/char/hw_random/Makefile +1 -0
index b6effb7..e81d21a 100644
@@ -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
drivers/char/hw_random/timeriomem-rng.c new file +153 -0

153 lines not on this page · raw message

include/linux/timeriomem-rng.h new file +16 -0
new file mode 100644
index 0000000..16dd9e4
@@ -0,0 +1,16 @@
+/*
+ * linux/include/linux/timeriomem-rng.h
+ *
+ * Copyright (c) 2008 Alexander Clouter <[email protected]>
+ *
+ * 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;
+};
author
#1 lore raw
Quoted 4 lines · Alexander Clouter

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

Quoted 251 lines

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

drivers/char/hw_random/Kconfig +13 -0
index 8822eca..638e060 100644
@@ -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

drivers/char/hw_random/Makefile +0 -0
index b6effb7..e81d21a 100644
@@ -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

drivers/char/hw_random/timeriomem-rng.c new file +153 -0
new file mode 100644
index 0000000..42f2813
@@ -0,0 +1,153 @@
+/*
+ * drivers/char/hw_random/timeriomem-rng.c
+ *
+ * Copyright (C) 2008 Alexander Clouter <[email protected]>
+ *
+ * Derived from drivers/char/hw_random/omap-rng.c
+ *   Copyright 2005 (c) MontaVista Software, Inc.
+ *   Author: Deepak Saxena <[email protected]>
+ *
+ * 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 <[email protected]>");
+MODULE_DESCRIPTION("Timer IOMEM H/W RNG driver");
include/linux/timeriomem-rng.h new file +16 -0
new file mode 100644
index 0000000..16dd9e4
@@ -0,0 +1,16 @@
+/*
+ * linux/include/linux/timeriomem-rng.h
+ *
+ * Copyright (c) 2008 Alexander Clouter <[email protected]>
+ *
+ * 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;
+};
Signature · 3 lines
Alexander Clouter
.sigmonster says: It's more than magnificent -- it's mediocre.
                  		-- Sam Goldwyn
#2 lore raw
Quoted 6 lines · On Mon, 5 Jan 2009 14:30:13 +0000 Alexander Clouter <[email protected]> wrote:
Quoted 4 lines · Alexander Clouter

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.

#3 lore raw
In reply to Alexander Clouter
Quoted 28 lines · On Tue, 30 Dec 2008 14:52:10 +0000 Alexander Clouter <[email protected]> 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,
};
Quoted 2 lines

------------

questions...

Quoted 51 lines

+++ b/drivers/char/hw_random/timeriomem-rng.c

drivers/char/hw_random/timeriomem-rng.c new file +49 -0
@@ -0,0 +1,153 @@
+/*
+ * drivers/char/hw_random/timeriomem-rng.c
+ *
+ * Copyright (C) 2008 Alexander Clouter <[email protected]>
+ *
+ * Derived from drivers/char/hw_random/omap-rng.c
+ *   Copyright 2005 (c) MontaVista Software, Inc.
+ *   Author: Deepak Saxena <[email protected]>
+ *
+ * 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)

drivers/char/hw_random/timeriomem-rng.c line 59

Quoted 10 lines
drivers/char/hw_random/timeriomem-rng.c new file +10 -0
+			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.

drivers/char/hw_random/timeriomem-rng.c line 66

Quoted 6 lines
drivers/char/hw_random/timeriomem-rng.c new file +6 -0
+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()?

drivers/char/hw_random/timeriomem-rng.c line 71

Quoted 4 lines
drivers/char/hw_random/timeriomem-rng.c new file +4 -0
+	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.

drivers/char/hw_random/timeriomem-rng.c line 84

Quoted 13 lines
drivers/char/hw_random/timeriomem-rng.c new file +13 -0
+		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()?

Quoted 7 lines
drivers/char/hw_random/timeriomem-rng.c new file +7 -0
+static struct hwrng timeriomem_rng_ops = {
+	.name		= "timeriomem",
+	.data_present	= timeriomem_rng_data_present,
+	.data_read	= timeriomem_rng_data_read,
+	.priv		= 0,
+};
+

drivers/char/hw_random/timeriomem-rng.c line 102

Quoted 10 lines
drivers/char/hw_random/timeriomem-rng.c new file +10 -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?

Quoted 6 lines
drivers/char/hw_random/timeriomem-rng.c new file +6 -0
+		timeriomem_rng_ops.priv = usecs_to_jiffies(
+						timeriomem_rng_data->period);
+	}
+
+	ret = hwrng_register(&timeriomem_rng_ops);
+	if (ret) {

drivers/char/hw_random/timeriomem-rng.c line 119

Quoted 10 lines
drivers/char/hw_random/timeriomem-rng.c new file +10 -0
+		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?

drivers/char/hw_random/timeriomem-rng.c line 123

Quoted 3 lines
drivers/char/hw_random/timeriomem-rng.c new file +3 -0
+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.

Quoted 52 lines
drivers/char/hw_random/timeriomem-rng.c new file +30 -0
+	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 <[email protected]>");
+MODULE_DESCRIPTION("Timer IOMEM H/W RNG driver");
include/linux/timeriomem-rng.h new file +16 -0
new file mode 100644
index 0000000..16dd9e4
@@ -0,0 +1,16 @@
+/*
+ * linux/include/linux/timeriomem-rng.h
+ *
+ * Copyright (c) 2008 Alexander Clouter <[email protected]>
+ *
+ * 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;
+};
author
#4 lore raw

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

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

Quoted 26 lines · Andrew Morton
Quoted 13 lines
drivers/char/hw_random/timeriomem-rng.c new file +12 -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)

drivers/char/hw_random/timeriomem-rng.c line 59

Quoted 10 lines
drivers/char/hw_random/timeriomem-rng.c new file +10 -0
+			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

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

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

		schedule_timeout_uninterruptible(delay);
	}

Agreed.

Quoted 3 lines

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.

Quoted 3 lines

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

Quoted 3 lines

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.

Quoted 9 lines

drivers/char/hw_random/timeriomem-rng.c line 66

Quoted 6 lines
drivers/char/hw_random/timeriomem-rng.c new file +6 -0
+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.

Quoted 9 lines

drivers/char/hw_random/timeriomem-rng.c line 71

Quoted 4 lines
drivers/char/hw_random/timeriomem-rng.c new file +4 -0
+	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.

Quoted 7 lines
Quoted 7 lines
drivers/char/hw_random/timeriomem-rng.c new file +7 -0
+		delay = rng->priv - (delay % rng->priv);
+
+		timeriomem_rng_timer.expires = cur + delay;
+		add_timer(&timeriomem_rng_timer);
+	}
+
+	return 4;
Quoted 9 lines

drivers/char/hw_random/timeriomem-rng.c line 84

Quoted 6 lines
drivers/char/hw_random/timeriomem-rng.c new file +6 -0
+}
+
+static void timeriomem_rng_trigger(unsigned long dummy)
+{
+	del_timer(&timeriomem_rng_timer);
+}

del_timer_sync()?

Check.

Quoted 12 lines
Quoted 12 lines
drivers/char/hw_random/timeriomem-rng.c new file +12 -0
+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;
Quoted 9 lines

drivers/char/hw_random/timeriomem-rng.c line 102

Quoted 5 lines
drivers/char/hw_random/timeriomem-rng.c new file +5 -0
+
+	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.

Quoted 3 lines

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?

Quoted 7 lines

drivers/char/hw_random/timeriomem-rng.c line 123

Quoted 3 lines
drivers/char/hw_random/timeriomem-rng.c new file +3 -0
+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

Signature · 2 lines
Alexander Clouter
.sigmonster says: Don't be overly suspicious where it's not warranted.