local_irq_save not masking interrupts

local_irq_save not masking interrupts

[Alex Zeffertt]

Hi list,

I'm having a strange problem with interrupts.  My platform is the
MPC832xEMDS and the BSP I'm using (from Freescale) uses Linux-2.6.11.

In the code below I enter a critical section with local_irq_save(),
call request_irq() (from mpc832xemds_phy_interrupt_enable) 4 times,
then exit the critical section using local_irq_restore.

     /* Enable interrupts from PHYs */
     local_irq_save(flags);
     for (i = 0; i < driver_data->num_phys; i++) {
         struct atm_dev *dev = driver_data->dev_data[i]->dev;
         printk("%s/%d\n",__FUNCTION__,__LINE__);
         RETURN_ON_ERROR(mpc832xemds_phy_interrupt_enable(dev));
     }
     local_irq_restore(flags);

The problem is that I get an interrupt *before* exiting the critical
section.  This causes a problem for me, because the interrupts are
shared and the correct handler has not yet been registered, so the
interrupt never gets deasserted.

My question is: why is local_irq_save()/local_irq_restore() not
working?

TIA,

Alex

RE: local_irq_save not masking interrupts

[Liu Dave-r63238]

<snip>
> I'm having a strange problem with interrupts.  My platform is=20
> the MPC832xEMDS and the BSP I'm using (from Freescale) uses=20
> Linux-2.6.11.
>=20
> In the code below I enter a critical section with=20
> local_irq_save(), call request_irq() (from=20
> mpc832xemds_phy_interrupt_enable) 4 times, then exit the=20
> critical section using local_irq_restore.
>=20
>      /* Enable interrupts from PHYs */
>      local_irq_save(flags);
>      for (i =3D 0; i < driver_data->num_phys; i++) {
>          struct atm_dev *dev =3D driver_data->dev_data[i]->dev;
>          printk("%s/%d\n",__FUNCTION__,__LINE__);
>          RETURN_ON_ERROR(mpc832xemds_phy_interrupt_enable(dev));
>      }
>      local_irq_restore(flags);
>=20
> The problem is that I get an interrupt *before* exiting the=20
> critical section.  This causes a problem for me, because the=20
> interrupts are shared and the correct handler has not yet=20
> been registered, so the interrupt never gets deasserted.
>=20
> My question is: why is local_irq_save()/local_irq_restore()=20
> not working?

Really? local_irq_save not working?
I don't believe it.=20
Please check if exist any re-enable the interrupt in the critical
section.

-DAve

Re: local_irq_save not masking interrupts

[Alex Zeffertt]

Liu Dave-r63238 wrote:
> <snip>
>> I'm having a strange problem with interrupts.  My platform is 
>> the MPC832xEMDS and the BSP I'm using (from Freescale) uses 
>> Linux-2.6.11.
>>
>> In the code below I enter a critical section with 
>> local_irq_save(), call request_irq() (from 
>> mpc832xemds_phy_interrupt_enable) 4 times, then exit the 
>> critical section using local_irq_restore.
>>
>>      /* Enable interrupts from PHYs */
>>      local_irq_save(flags);
>>      for (i = 0; i < driver_data->num_phys; i++) {
>>          struct atm_dev *dev = driver_data->dev_data[i]->dev;
>>          printk("%s/%d\n",__FUNCTION__,__LINE__);
>>          RETURN_ON_ERROR(mpc832xemds_phy_interrupt_enable(dev));
>>      }
>>      local_irq_restore(flags);
>>
>> The problem is that I get an interrupt *before* exiting the 
>> critical section.  This causes a problem for me, because the 
>> interrupts are shared and the correct handler has not yet 
>> been registered, so the interrupt never gets deasserted.
>>
>> My question is: why is local_irq_save()/local_irq_restore() 
>> not working?
> 
> Really? local_irq_save not working?
> I don't believe it. 
> Please check if exist any re-enable the interrupt in the critical
> section.
> 
> -DAve


Well, mpc832xemds_phy_interrupt_enable() does nothing except call
request_irq(,,SA_SHIRQ,,).  I suspect that request_irq() is somehow
reenabling interrupts, but I can't see where it might be doing so.

Is this a possible?

Alex

Re: local_irq_save not masking interrupts

[Scott Wood]

Alex Zeffertt wrote:
> Well, mpc832xemds_phy_interrupt_enable() does nothing except call
> request_irq(,,SA_SHIRQ,,).  I suspect that request_irq() is somehow
> reenabling interrupts, but I can't see where it might be doing so.

One possibile way (in 2.6.18; I'm assuming 2.6.11 is similar) is that 
request_irq() calls setup_irq(), which calls register_irq_proc() and 
register_handler_proc(), both of which call proc_mkdir(), which 
eventually calls proc_create(), which calls kmalloc() with GFP_KERNEL. 
This is probably a bug, since request_irq itself uses GFP_ATOMIC, 
indicating an intent for request_irq() to be safely callable in atomic 
context.

Can you disable the interrupts at the device level until the handler is 
in place, and thus avoid the need to disable IRQs at all?

-Scott

Re: local_irq_save not masking interrupts

[Alex Zeffertt]

Hi Scott,

Thanks for your reply.  Comments below.

Scott Wood wrote:
> Alex Zeffertt wrote:
>> Well, mpc832xemds_phy_interrupt_enable() does nothing except call
>> request_irq(,,SA_SHIRQ,,).  I suspect that request_irq() is somehow
>> reenabling interrupts, but I can't see where it might be doing so.
> 
> One possibile way (in 2.6.18; I'm assuming 2.6.11 is similar) is that 
> request_irq() calls setup_irq(), which calls register_irq_proc() and 
> register_handler_proc(), both of which call proc_mkdir(), which 
> eventually calls proc_create(), which calls kmalloc() with GFP_KERNEL. 
> This is probably a bug, since request_irq itself uses GFP_ATOMIC, 
> indicating an intent for request_irq() to be safely callable in atomic 
> context.
> 

I agree this indicates an intent to make it atomic, but I don't see how
this could cause interrupts to become re-enabled during the request_irq()
call.  Also, since I am calling request_irq at insmod time, i.e. in process
context, both GFP_ flags *should* work.

> Can you disable the interrupts at the device level until the handler is 
> in place, and thus avoid the need to disable IRQs at all?
> 
> -Scott

Yup, I've come to the same conclusion.  I now, for each shared interrupt:

* initialise all the devices which share the interrupt, turning interrupt generation off

* register all handlers for each device which shares the interrupt

* turn interrupt generation back on for each device.

This avoids the original problem, but it does not explain it.  I still
can't see how I can get an interrupt on line 3 below:

1.         local_irq_save(flags);
2.         request_irq(MPC83xx_IRQ_EXT6, handler, SA_SHIRQ, "name", dev1);
3.
4.         request_irq(MPC83xx_IRQ_EXT6, handler, SA_SHIRQ, "name", dev2);
5          local_irq_restore(flags);

Regards,

Alex

Re: local_irq_save not masking interrupts

[Scott Wood]

Alex Zeffertt wrote:
> I agree this indicates an intent to make it atomic, but I don't see how
> this could cause interrupts to become re-enabled during the request_irq()
> call.  Also, since I am calling request_irq at insmod time, i.e. in process
> context, both GFP_ flags *should* work.

You're effectively not in process context when you disable IRQs (at 
least, if you want them to stay that way).  By specifying GFP_KERNEL, 
you're giving the allocator permission to go to sleep, enable IRQs, etc. 
  The IRQ enabling will happen any time cache_grow() is called with 
GFP_WAIT (which is part of GFP_KERNEL), assuming a growable slab.

-Scott

Re: local_irq_save not masking interrupts

[Alex Zeffertt]

Scott Wood wrote:
> Alex Zeffertt wrote:
>> I agree this indicates an intent to make it atomic, but I don't see how
>> this could cause interrupts to become re-enabled during the request_irq()
>> call.  Also, since I am calling request_irq at insmod time, i.e. in 
>> process
>> context, both GFP_ flags *should* work.
> 
> You're effectively not in process context when you disable IRQs (at 
> least, if you want them to stay that way).  By specifying GFP_KERNEL, 
> you're giving the allocator permission to go to sleep, enable IRQs, etc. 
>  The IRQ enabling will happen any time cache_grow() is called with 
> GFP_WAIT (which is part of GFP_KERNEL), assuming a growable slab.

Ah-ha!  This explains a lot...

One of the oddities I was seeing with this problem was that if I
did an "ifconfig down" on a completely unrelated net_device (a vlan)
the problem would *not* occur, i.e. I did not get an interrupt
during the critical section.  Now I understand why:  the "ifconfig down"
command freed some memory so that the kmalloc(,GFP_KERNEL) did not
need to grow the cache.

It follows from what you are saying that kmalloc(,GFP_KERNEL)
MUST NOT occur anywhere in the call chain during a critical section.

This must catch others out too.  Surely kmalloc/cache_grow should
return NULL rather than enable interrupts.  In fact, shouldn't it be
a BUG() if kmalloc(,GFP_KERNEL) is called with IRQs disabled?


Thanks for your explanation!

Alex

Re: local_irq_save not masking interrupts

[Scott Wood]

Alex Zeffertt wrote:
> It follows from what you are saying that kmalloc(,GFP_KERNEL)
> MUST NOT occur anywhere in the call chain during a critical section.
> 
> This must catch others out too.  Surely kmalloc/cache_grow should
> return NULL rather than enable interrupts.

The local_irq_enable() is intended to reverse a previous 
local_irq_save() done in the allocator (with the assumption that if 
GFP_WAIT is specified, the flags saved had IRQs enabled), not to handle 
the case where IRQs were disabled by kmalloc's caller.  Returning NULL 
would mean that slabs can never grow with GFP_KERNEL.

 > In fact, shouldn't it be
> a BUG() if kmalloc(,GFP_KERNEL) is called with IRQs disabled?

Yes.

-Scott

Re: local_irq_save not masking interrupts

[Esben Nielsen]

On Tue, 26 Sep 2006, Alex Zeffertt wrote:

> Hi list,
>
> I'm having a strange problem with interrupts.  My platform is the
> MPC832xEMDS and the BSP I'm using (from Freescale) uses Linux-2.6.11.
>
> In the code below I enter a critical section with local_irq_save(),
> call request_irq() (from mpc832xemds_phy_interrupt_enable) 4 times,
> then exit the critical section using local_irq_restore.
>
>     /* Enable interrupts from PHYs */
>     local_irq_save(flags);
>     for (i = 0; i < driver_data->num_phys; i++) {
>         struct atm_dev *dev = driver_data->dev_data[i]->dev;
>         printk("%s/%d\n",__FUNCTION__,__LINE__);
>         RETURN_ON_ERROR(mpc832xemds_phy_interrupt_enable(dev));
>     }
>     local_irq_restore(flags);
>

This is a pure side-comment:

Please, don't use local_irq_save() for a critical region. Use
spin_lock_irqsave(), for 3 reasons:
1) SMP.
2) Preempt-realtime is like SMP not happy about using local_irq_save().
3) Read-ability: It is more clear what data is protected when there is a 
named lock object in the code.

You might not care about 1) and 2) but 3) should matter for you. And 
remember: When CONFIG_SMP is not set spin_lock_irqsave() will just 
become a local_irq_save().

Esben