Re: [PATCH] [REPOST] timer iomem hwrng driver
- From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
- Date: Mon, 12 Jan 2009 13:13:21 -0800
On Tue, 30 Dec 2008 14:52:10 +0000
Alexander Clouter <alex@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
+ *
+ * Derived from drivers/char/hw_random/omap-rng.c
+ * Copyright 2005 (c) MontaVista Software, Inc.
+ * Author: Deepak Saxena <dsaxena@xxxxxxxxxxx>
+ *
+ * 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@xxxxxxxxxxxxx>");
+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@xxxxxxxxxxxxx>
+ *
+ * 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;
+};
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
- Follow-Ups:
- Re: [PATCH] [REPOST] timer iomem hwrng driver
- From: Alexander Clouter
- Re: [PATCH] [REPOST] timer iomem hwrng driver
- Prev by Date: Re: Data corruption issue with splice() on 2.6.27.10
- Next by Date: Re: ACPI hotplug panic with current git head
- Previous by thread: [WATCHDOG] v2.6.29-rc patches
- Next by thread: Re: [PATCH] [REPOST] timer iomem hwrng driver
- Index(es):
Relevant Pages
|
