Hello!
I made a patch and I would like feedback.
If you can test the patch.. please let me know.
Background:
There are busy loops that time out after 8 million repetitions.
I made these improvements:
* Sleep during the busy loops
* Time out is based on elapsed time
If this patch is accepted, AZT_TIMEOUT can be removed.
Best regards,
Daniel Marjam?ki
diff -up a/drivers/cdrom/aztcd.c b/drivers/cdrom/aztcd.c
--- a/drivers/cdrom/aztcd.c 2005-11-17 11:39:53.000000000 +0100
+++ b/drivers/cdrom/aztcd.c 2005-11-19 17:14:58.000000000 +0100
@@ -179,6 +179,7 @@
#include <linux/ioport.h>
#include <linux/string.h>
#include <linux/major.h>
+#include <linux/jiffies.h>
#include <linux/init.h>
@@ -308,7 +309,7 @@ static char aztDiskChanged = 1;
static char aztTocUpToDate = 0;
static unsigned char aztIndatum;
-static unsigned long aztTimeOutCount;
+static unsigned long aztTimeOutCount, aztTimeOut;
static int aztCmd = 0;
static DEFINE_SPINLOCK(aztSpin);
@@ -361,14 +362,14 @@ static int azt_bcd2bin(unsigned char bcd
# define OP_OK op_ok()
static void op_ok(void)
{
- aztTimeOutCount = 0;
+ aztTimeOut = jiffies + 2;
do {
aztIndatum = inb(DATA_PORT);
- aztTimeOutCount++;
- if (aztTimeOutCount >= AZT_TIMEOUT) {
+ if (time_after(jiffies, aztTimeOut)) {
printk("aztcd: Error Wait OP_OK\n");
break;
}
+ schedule_timeout_interruptible(1);
} while (aztIndatum != AFL_OP_OK);
}
@@ -377,14 +378,14 @@ static void op_ok(void)
# define PA_OK pa_ok()
static void pa_ok(void)
{
- aztTimeOutCount = 0;
+ aztTimeOut = jiffies + 2;
do {
aztIndatum = inb(DATA_PORT);
- aztTimeOutCount++;
- if (aztTimeOutCount >= AZT_TIMEOUT) {
+ if (time_after(jiffies, aztTimeOut)) {
printk("aztcd: Error Wait PA_OK\n");
break;
}
+ schedule_timeout_interruptible(1);
} while (aztIndatum != AFL_PA_OK);
}
#endif
@@ -393,17 +394,17 @@ static void pa_ok(void)
# define STEN_LOW sten_low()
static void sten_low(void)
{
- aztTimeOutCount = 0;
+ aztTimeOut = jiffies + 2;
do {
aztIndatum = inb(STATUS_PORT);
- aztTimeOutCount++;
- if (aztTimeOutCount >= AZT_TIMEOUT) {
+ if (time_after(jiffies, aztTimeOut)) {
if (azt_init_end)
printk
("aztcd: Error Wait STEN_LOW commands:%x\n",
aztCmd);
break;
}
+ schedule_timeout_interruptible(1);
} while (aztIndatum & AFL_STATUS);
}
@@ -411,14 +412,14 @@ static void sten_low(void)
# define DTEN_LOW dten_low()
static void dten_low(void)
{
- aztTimeOutCount = 0;
+ aztTimeOut = jiffies + 2;
do {
aztIndatum = inb(STATUS_PORT);
- aztTimeOutCount++;
- if (aztTimeOutCount >= AZT_TIMEOUT) {
+ if (time_after(jiffies, aztTimeOut)) {
printk("aztcd: Error Wait DTEN_OK\n");
break;
}
+ schedule_timeout_interruptible(1);
} while (aztIndatum & AFL_DATA);
}
> static void op_ok(void)
> {
> - aztTimeOutCount = 0;
> + aztTimeOut = jiffies + 2;
> do {
> aztIndatum = inb(DATA_PORT);
> - aztTimeOutCount++;
> - if (aztTimeOutCount >= AZT_TIMEOUT) {
> + if (time_after(jiffies, aztTimeOut)) {
> printk("aztcd: Error Wait OP_OK\n");
> break;
> }
> + schedule_timeout_interruptible(1);
this I think is not quite right; schedule_timeout_*() doesn't do
anything unless you set current->state to something. And at that point
you might as well start using msleep()!
but what you're doing is generally a good idea; busy waits as the
original code did is quite wrong...
On Sun, 2005-11-20 at 16:38 +0100, Arjan van de Ven wrote: > }
> > + schedule_timeout_interruptible(1);
>
> this I think is not quite right; schedule_timeout_*() doesn't do
> anything unless you set current->state to something.
Err, schedule_timeout_(un)interruptible sets current->state.
tglx
Daniel Marjam?ki <[email protected]> wrote:
> - aztTimeOutCount = 0;
> + aztTimeOut = jiffies + 2;
Different timeout based on HZ seems wrong.
--
Ich danke GMX daf?r, die Verwendung meiner Adressen mittels per SPF
verbreiteten L?gen zu sabotieren.
On 11/20/05, Arjan van de Ven <[email protected]> wrote:
> > static void op_ok(void)
> > {
> > - aztTimeOutCount = 0;
> > + aztTimeOut = jiffies + 2;
> > do {
> > aztIndatum = inb(DATA_PORT);
> > - aztTimeOutCount++;
> > - if (aztTimeOutCount >= AZT_TIMEOUT) {
> > + if (time_after(jiffies, aztTimeOut)) {
> > printk("aztcd: Error Wait OP_OK\n");
> > break;
> > }
> > + schedule_timeout_interruptible(1);
>
> this I think is not quite right; schedule_timeout_*() doesn't do
> anything unless you set current->state to something. And at that point
> you might as well start using msleep()!
Not true, as Thomas points out. You are right for schedule_timeout(),
but that's why we introduced the _interruptible() and
_uninterruptible(). And there are reasons to use schedule_timeout_*()
instead of msleep() [not necessarily in this case, but in general],
specifically the presence of wait-queues.
> but what you're doing is generally a good idea; busy waits as the
> original code did is quite wrong...
I agree, and I recommend Daniel post to LKML to get some testing / see
if anyone actually uses this driver :)
Thanks,
Nish
On 11/20/05, Bodo Eggert <[email protected]> wrote:
> Daniel Marjam?ki <[email protected]> wrote:
>
> > - aztTimeOutCount = 0;
> > + aztTimeOut = jiffies + 2;
>
> Different timeout based on HZ seems wrong.
True; I'm trying to think of a good way to emulate 8000000 iterations
of loop, though. Really, this is not terrible to use 2 jiffies of
offset, as we try to sleep 1 jiffy each time. As long as we don't get
a signal *right* away, we'll sleep probably for 2 loops. Not sure,
though, may be useful to see what happens in practice and then debug
further for the right value.
May also want to use time_after_eq() not time_after().
Thanks,
Nish
Bodo Eggert wrote:
> Daniel Marjam?ki <[email protected]> wrote:
>
>
>>- aztTimeOutCount = 0;
>>+ aztTimeOut = jiffies + 2;
>
>
> Different timeout based on HZ seems wrong.
Yes, but..
If I'd say "HZ/100", then all systems that uses my driver must have HZ>=200.
The way I do it:
All systems will give me a delay for at least a few ms.
I get the shortest timeout possible on each computer.
On Mon, 21 Nov 2005 19:09, Daniel Marjam?ki wrote:
> Bodo Eggert wrote:
> > Daniel Marjam?ki <[email protected]> wrote:
> >>- aztTimeOutCount = 0;
> >>+ aztTimeOut = jiffies + 2;
> >
> > Different timeout based on HZ seems wrong.
>
> Yes, but..
>
> If I'd say "HZ/100", then all systems that uses my driver must have
> HZ>=200.
>
> The way I do it:
> All systems will give me a delay for at least a few ms.
> I get the shortest timeout possible on each computer.
Convention in the kernel would be
aztTimeOut = HZ / 100 ? : 1;
to be at least one tick (works for HZ even below 100) and is at least 10ms. If
you wanted 2 ms then use
aztTimeOut = HZ / 500 ? : 1;
which would give you at least 2ms
Cheers,
Con
Con Kolivas wrote:
>
> Convention in the kernel would be
> aztTimeOut = HZ / 100 ? : 1;
> to be at least one tick (works for HZ even below 100) and is at least 10ms. If
> you wanted 2 ms then use
> aztTimeOut = HZ / 500 ? : 1;
> which would give you at least 2ms
>
Thank you Con for the feedback.
Hmm.. The minimum value should be 2, right?
Otherwise the loop could time out after only a few nanoseconds.. since the loop will then timeout immediately on a clock tick.
Or am I wrong?
Best regards,
Daniel Marjam?ki
On Mon, 21 Nov 2005 19:42, Daniel Marjam?ki wrote:
> Con Kolivas wrote:
> > Convention in the kernel would be
> > aztTimeOut = HZ / 100 ? : 1;
> > to be at least one tick (works for HZ even below 100) and is at least
> > 10ms. If you wanted 2 ms then use
> > aztTimeOut = HZ / 500 ? : 1;
> > which would give you at least 2ms
>
> Thank you Con for the feedback.
>
> Hmm.. The minimum value should be 2, right?
> Otherwise the loop could time out after only a few nanoseconds.. since the
> loop will then timeout immediately on a clock tick. Or am I wrong?
aztTimeOut = HZ / 500 ? : 1;
Would give you a 2ms timeout on 1000Hz and 500Hz
It would give you 5ms on 250Hz and 10ms on 100Hz
ie the absolute minimum it would be would be 2ms, but it would always be at
least one timer tick which is longer than 2ms at low HZ values.
Cheers,
Con
Con Kolivas wrote:
> On Mon, 21 Nov 2005 19:42, Daniel Marjam?ki wrote:
>>Hmm.. The minimum value should be 2, right?
>>Otherwise the loop could time out after only a few nanoseconds.. since the
>>loop will then timeout immediately on a clock tick. Or am I wrong?
>
>
> aztTimeOut = HZ / 500 ? : 1;
> Would give you a 2ms timeout on 1000Hz and 500Hz
> It would give you 5ms on 250Hz and 10ms on 100Hz
>
> ie the absolute minimum it would be would be 2ms, but it would always be at
> least one timer tick which is longer than 2ms at low HZ values.
>
Not true. From 'now', the next timer interrupt is somewhere
between epsilon and 1/HZ seconds away.
Luckily, time_after is < rather than <=, so your aztTimeOut would
actually make Daniel's code wait until a minimum of *two* timer
ticks have elapsed since reading jiffies. So it would manage to
scrape by the values of HZ you quoted.
OTOH, if HZ were between 500 and 1000, it would again be too short
due to truncation.
Better I think would be to use the proper interfaces for converting
msecs to jiffies:
aztTimeOut = jiffies + msecs_to_jiffies(2);
Nick
--
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com
On 11/21/05, Nick Piggin <[email protected]> wrote:
> Con Kolivas wrote:
> > On Mon, 21 Nov 2005 19:42, Daniel Marjam?ki wrote:
>
> >>Hmm.. The minimum value should be 2, right?
> >>Otherwise the loop could time out after only a few nanoseconds.. since the
> >>loop will then timeout immediately on a clock tick. Or am I wrong?
> >
> >
> > aztTimeOut = HZ / 500 ? : 1;
> > Would give you a 2ms timeout on 1000Hz and 500Hz
> > It would give you 5ms on 250Hz and 10ms on 100Hz
> >
> > ie the absolute minimum it would be would be 2ms, but it would always be at
> > least one timer tick which is longer than 2ms at low HZ values.
> >
>
> Not true. From 'now', the next timer interrupt is somewhere
> between epsilon and 1/HZ seconds away.
>
> Luckily, time_after is < rather than <=, so your aztTimeOut would
> actually make Daniel's code wait until a minimum of *two* timer
> ticks have elapsed since reading jiffies. So it would manage to
> scrape by the values of HZ you quoted.
>
> OTOH, if HZ were between 500 and 1000, it would again be too short
> due to truncation.
>
> Better I think would be to use the proper interfaces for converting
> msecs to jiffies:
>
> aztTimeOut = jiffies + msecs_to_jiffies(2);
That's what I get for sleeping. Yes, Nick is right, don't do this
conversion yourself, please, use the interfaces designed exactly to
avoid any confusion / HZ-issues (and if those interfaces have
limitations, as Willy discovered earlier, please fix them).
Thanks,
Nish
On Mon, 21 Nov 2005, Con Kolivas wrote:
> On Mon, 21 Nov 2005 19:09, Daniel Marjam?ki wrote:
> > The way I do it:
> > All systems will give me a delay for at least a few ms.
> > I get the shortest timeout possible on each computer.
>
> Convention in the kernel would be
> aztTimeOut = HZ / 100 ? : 1;
> to be at least one tick (works for HZ even below 100) and is at least 10ms.
You have to add one to the timeout, since you might be just before the end
of the timeslice when the delay starts. aztTimeOut = (HZ / 100 ? : 1) + 1;
--
Top 100 things you don't want the sysadmin to say:
11. Can you get VMS for this Sparc thingy?