[alsa-devel] [PATCH] Modify mpc5200 AC97 driver to use V9 of spin_event_timeout()

[Grant Likely]

On Tue, May 26, 2009 at 6:44 PM, Jon Smirl <jonsmirl at gmail.com> wrote:
> On Tue, May 26, 2009 at 8:38 PM, Timur Tabi <timur at freescale.com> wrote:
>> On Tue, May 26, 2009 at 7:25 PM, Jon Smirl <jonsmirl at gmail.com> wrote:
>>
>>> -       spin_event_timeout(0, 10, 0, rc);
>>> +       spin_event_timeout(0, 10, 0);
>>>        out_8(&regs->op0, MPC52xx_PSC_OP_RES);
>>> -       spin_event_timeout(0, 50, 0, rc);
>>> +       spin_event_timeout(0, 50, 0);
>>
>> Jon, I'm still hoping you'll explain why you're not using udelay() here.
>
> Because Grant didn't want me doing udelay(50) just to delay things in
> order to give the AC97 controller time to initialize. Your function
> lets me loop on cpu_relax() for 50us.

No, you misunderstand.  My issue is that udelay is far too coarse
grained for what you want.  ie. If your are using udelay(1) in your
loop, and your event changes state in 1.01us, then you're still going
to burn a full 2us of time.  If you can loop over cpu_relax() instead,
then less time will get burned.

For a hard 50us delay, with no early exit condition, udelay() is the
correct function.

HOWEVER; I'm distrustful of *any* spins, and would rather see sleeps
instead of spins wherever possible.  There are two reasons for this:

First, udelay just burns time, and if the delay is too large, then the
it is wasting time that could be used for something else.  That being
said, it needs to be balanced with the context switch overhead.  If
the udelay() is less than double the context switch time, then the
overhead is greater than the time spent spinning.

Second, udelay() with IRQs disabled cause unbounded latencies.  Even
if it is less efficient, I'd rather see mutexes+msleep() than
spin_lock_irqsave()+udelay().  Of course, this only works in
non-atomic contexts.  If the code is atomic, then you have no choice.

There is no blanket rule here; but as the time delay requirement gets
larger, the greater the benefit to overall system responsiveness by
using sleep instead of delay.  I'm not going to make a hard statement
about how it should be done right now.  It's a fiddly area and it can
be tweaked after the driver is merged.

However, Timur is absolutely right.  A spin without an early exit
condition should simply be a udelay().

> I have to delay 50us because ALSA tries to access the hardware
> immediately after the function returns.

Does the code run in atomic context?

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.