On Fri, 2 Sep 2011, Geert Uytterhoeven wrote:

> On Fri, Sep 2, 2011 at 14:14, Finn Thain <fthain@telegraphics.com.au> wrote:
> > On Sun, 28 Aug 2011, I wrote:
> > However, I found another problem. pmac_zilog oopses when its TTY is closed
> > (see below). And macsonic does the same when the NIC is closed. The trace
> > says that irq_shutdown() died trying to call the chip irq_mask routine,
> > when desc->irq_data.chip (in a0) was NULL. Any ideas?
> 
> > Unable to handle kernel NULL pointer dereference at virtual address   (null)
> > Oops: 00000000
> > Modules linked in:
> > PC: [<00000000>]   (null)
> 
> No, it calls a function pointer that's NULL.
> 
> void irq_shutdown(struct irq_desc *desc)
> {
>         irq_state_set_disabled(desc);
>         desc->depth = 1;
>         if (desc->irq_data.chip->irq_shutdown)
>                 desc->irq_data.chip->irq_shutdown(&desc->irq_data);
>         if (desc->irq_data.chip->irq_disable)
>                 desc->irq_data.chip->irq_disable(&desc->irq_data);
>         else
>                 desc->irq_data.chip->irq_mask(&desc->irq_data);
>         irq_state_set_masked(desc);
> }
> 
> Oops, seems I misread the code and assumed the second "if" was an "else if",

I think that was supposed to be an "else if". Lemme look at the code
before the overhaul to be sure.

> as I wrote down in my notes that it calls only one of the 3 functions :-(
> But it does need either .irq_disable() or .irq_mask().
> 
> The following irq_chip structs have both .irq_disable() and .irq_mask() NULL:
>   - arch/m68k/apollo/dn_ints.c:apollo_irq_chip
>   - arch/m68k/kernel/ints.c:auto_irq_chip
>   - arch/m68k/kernel/ints.c:user_irq_chip
> 
> Since this is on Mac and mac_irq_chip is OK, I guess this is a non-PSC Mac,
> i.e. the IRQ number is IRQ_MAC_SCC == IRQ_AUTO_4?
> 
> Thomas: As we cannot disable or mask autovector interrupts at the
> autovector level,
> IMHO it doesn't make much sense do add a dummy .irq_mask() function.
> Can we add an extra check in irq_shutdown() to not call .irq_mask() if
> it's NULL?

Yes.

Thanks,

	tglx