Hi Jean,
Many thanks for clarifying your review, I only have one question left,
see below.
Jean Delvare wrote:
> Hi Hans,
>
> Please answer to the list rather than just me.
>
Oops, sorry. I'm not used to the write to list and CC the author kinda
use of mailinglist. Right now I'm only writing this to the list, is this
ok, or do you always want a direct CC?
>>>> static int abituguru_wait(struct abituguru_data *data, u8 state)
>>>> {
>>>> int timeout = ABIT_UGURU_WAIT_TIMEOUT;
>>>>
>>>> while (inb_p(data->addr + ABIT_UGURU_DATA) != state) {
>>>> timeout--;
>>>> if (timeout == 0)
>>>> return -EBUSY;
>>>> }
>>>> return 0;
>>>> }
>>> Hm, this is a busy waiting loop. Not recommended. Why don't you msleep()
>>> between retries? Or at the very least yield()? In both cases, the
>>> timeout should be a duration rather than a number of tries.
>>>
>> Usually this succeeds within 100 - 150 reads (max 250) which assuming
>> the isa bus is the bottleneck and tacking the _p into account translates
>> to 300 isa cycles which is 37.5 usec, since the timing can vary I could
>> complicate the code by first during a small usleep which will translate
>> to a busy wait
>
> No, by definition, this isn't a busy wait if you are sleeping.
>
My bad, the kernel doesn't have a usleep function, because sleeping for
such short amounts is not supported, its called udelay, which translates
to a busy wait, see arch/xxx/lib/delay.c
>> (...) and then do some isa bus busy waiting, but that
>> complicates stuff without winning anything.
>
> What I don't get is why you need to do ISA bus waiting at all. Why
> don't you just sleep-wait? 10 us sleeps shoud do. Please keep in mind
> that your device isn't the only user of the ISA bus
> (check /proc/ioports).
> Given that all uGuru registers are read at once
> by design, if you saturate the ISA bus, other devices may experience
> increased latency.
>
I only do an update once every second, thus reducing the ISA bus load.
During an update the wait function gets called 147 times, resulting in
circa 5 ms ISA-bus usage.
But the real point is that using delay functions doesn't win all that
much since we still keep the CPU occupied, thus although it will keep
the ISA bus free there will be no CPU to use it and if we get preempted
we'll also stop using the ISA bus. The only exception to this is SMP
systems, but SMP systems really shouldn't rely on the ISA-bus.
I could change the code to use delay functions if you insist, but I'd
rather not as the current version has already been tested thoroughly by
a small team of beta testers I've build up and thus I'd rather not touch
this piece of code.
An other option to be nicer to other processes would be to yield between
reads in the update function, this way the code-path of a single read
doesn't get any changes, so I'm pretty sure this won't cause any
problems. Then again if something else wants the CPU, we would get
preempted anyways, so I don't know if this really helps.
> As a side note, I don't think the _p has any effect on modern systems,
> does it?
>
It still does, it causes a read of isa address 0x80, see
include/asm/io.h, thus causing a one isa cycle delay.
Regards,
Hans
