linux-mips-fnet
[Top] [All Lists]

RE: Missing set?

To: Harald Koerfgen <harald.koerfgen@netcologne.de>
Subject: RE: Missing set?
From: Thomas Riemer <triemer@apt4g.a3nyc.com>
Date: Fri, 16 Oct 1998 20:18:52 -0400 (EDT)
Cc: linux-mips@fnet.fr
In-reply-to: <XFMail.981016124445.harald.koerfgen@netcologne.de>
This was exactly what I needed to get stuff working...

irq_request has some very strange behavior if
it is nested within a save_flags /restore_flags
construction.

Consider the snippet:
        .....
        save_flags(flags); cli();
        request_irq(SERIAL, dz_interrupt, SA_INTERRUPT, "DZ", lines[0]);
        restore_flags(flags);
        ......

It turns out that this manages to leave the state of the status register
unchanged.

Move the restore_flags before the call to request_irq and you make 
progress - and indeed the dz.c driver code actually generates characters
across the serial line.

-Tom

-----------------------------------------------------------------------
Given enough eyeballs all bugs seem shallow.

On Fri, 16 Oct 1998, Harald Koerfgen wrote:

> Hi,
> 
> On 16-Oct-98 Thomas Riemer wrote:
> > Looking at int-handler.S the code testing the CAUSE register($12) against
> > the STATUS register($13).  It only processes if both bits in the
> > two registers are set.  Somehow the clock is being set in the STATUS 
> > register and I am getting hardware interrupts - which is a good sign.
> > 
> > After having looked for about an hour for where the STATUS register is 
> > set, I seem to be unable to find where that happens.  In the case
> > of the dz driver, I believe that the kernel is generating interrupts, but
> > because the STATUS register is not set, the kernel is ignoring these
> > interrupts. 
> > 
> > I would think the STATUS register should be set in request_irq - but
> > it isn't.
> > 
> > Where is the code that sets the STATUS bit for the clock?  and in general
> > for irqs?
> 
> This is indeed somewhat tricky. I had to find a way to manipulate the status
> register within a 
> 
>         save_and_cli(flags);
>         ...
>         restore_flags(flags);
> 
> code sequence. save_flags saves the content of the status register in flags 
> and
> restore_flags restores it, so enabling or disabling interrupts within a 
> save/restore
> pair wouldn't work.
> 
> arch/mips/dec/irq.c:
> 
>     request_irq() -> setup_dec_irq()
> 
> setup_dec_irq:
> 
>     ...
>     save_and_cli(flags);
>     *p = new;
> 
>     if (!shared) {
>         unmask_irq_flags(irq, &flags);
>     }
>     restore_flags(flags);
>     ....
> 
> and unmask_irq_flags looks basicly like (leaving the IOASIC stuff aside):
> 
> static inline void unmask_irq_flags(unsigned int irq_nr, unsigned long *flags)
> {
>     ...
>     *flags |= dec_interrupt[irq_nr].cpu_mask;
> }
> 
> That means, the bits in the status register are set (or reset) by setting (or
> resetting) bits in flags and restore_flags(flags) does the trick.
> 
> I _know_ this is dirty and if you have a better idea without adding a 
> performance
> penalty, I'd love to implement it.
> 
> People, when writing device drivers for DECstations, DO NEVER EVER use code 
> like:
> 
>     save_flags(flags);
>     [disable or enable interrupts with en/disable_irq(irq)]
>     restore(flags);
> 
> That simply doesn't work and will cause endless headache.
> ---
> Regards,
> Harald
> 

<Prev in Thread] Current Thread [Next in Thread>