lkml.org 
[lkml]   [2009]   [Jan]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] [REPOST] timer iomem hwrng driver
    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;
    > +};



    \
     
     \ /
      Last update: 2009-01-12 22:17    [W:0.048 / U:29.872 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site