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

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@xxxxxxxxxxxxxxxxxxxx> [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.

--
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/

Quantcast