Message-ID: <4D8A6E38.7020203@gmail.com>
Date:	Wed, 23 Mar 2011 15:03:36 -0700
From:	David Daney <ddaney.cavm@...il.com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	linux-mips@...ux-mips.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/2] genirq: Add chip hooks for taking CPUs on/off
 line.

On 03/21/2011 02:13 PM, Thomas Gleixner wrote:
> On Mon, 21 Mar 2011, David Daney wrote:
>
>> On 03/19/2011 01:51 PM, Thomas Gleixner wrote:
>>> On Fri, 18 Mar 2011, David Daney wrote:
>>>> --- a/include/linux/irqdesc.h
>>>> +++ b/include/linux/irqdesc.h
>>>> @@ -178,6 +178,12 @@ static inline int irq_has_action(unsigned int irq)
>>>>    	return desc->action != NULL;
>>>>    }
>>>>
>>>> +/* Test to see if the irq is currently enabled */
>>>> +static inline int irq_desc_is_enabled(struct irq_desc *desc)
>>>> +{
>>>> +	return desc->depth == 0;
>>>> +}
>>>
>>> That want's to go into kernel/irq/internal.h
>>
>> I think I need to use this in my irq_chip.irq_unmask method.
>>
>> Consider this:
>>
>>
>> CPU0                   CPU1
>> handle_level_irq
>>    lock
>>    mask
>>    handle_irq_event
>>      unlock
>>      .
>>      .                  disable_irq
>>      .
>>      lock
>>    unmask
>>    unlock
>
> handle level irq does:
>
> 	if (!(desc->istate&  (IRQS_DISABLED | IRQS_ONESHOT)))
> 		unmask_irq(desc);
>
> So it does not call unmask.
>
>> I need to know in my .unmask method if the interrupt has been disabled.  If it
>> has, I will not re-enable (unmask)it.
>
> It wont be called :)
>

I missed that.  Really irq_desc_is_enabled() should just use the 
IRQS_DISABLED flag.

I will do that.


>>>
>>>>    #ifndef CONFIG_GENERIC_HARDIRQS_NO_COMPAT
>>>>    static inline int irq_balancing_disabled(unsigned int irq)
>>>>    {
>>>> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
>>>> index c9c0601..40736f7 100644
>>>> --- a/kernel/irq/chip.c
>>>> +++ b/kernel/irq/chip.c
>>>> @@ -689,3 +689,38 @@ void irq_modify_status(unsigned int irq, unsigned
>>>> long clr, unsigned long set)
>>>>
>>>>    	irq_put_desc_unlock(desc, flags);
>>>>    }
>>>> +
>>>> +void irq_cpu_online(unsigned int irq)
>>>
>>> Odd function name. It does not reflect that this is for per cpu
>>> interrupts. So something like irq_xxx_per_cpu_irq(irq)
>>> might be a bit more descriptive.
>>
>> I am using it for per cpu interrupts, but I didn't want to impose that policy
>> on others.
>
> I can't imagine any other purpose for that.

Modifying the affinity of non-per-cpu IRQs to use the new CPU?

>
>>> Though one question remains: should we just iterate over the irq space
>>> and call the online/offline callbacks when available instead of having
>>> the arch code do the iteration.
>>>
>>
>> That would be good I think, especially for sparse irqs.
>>
>> In the case of the CPU going offline, the .irq_cpu_offline() may need to
>> adjust the affinity so that the irq no longer has affinity for the off-lined
>> CPU.
>>
>> This is something needed even for non-per-cpu interrupts.  Also I would need a
>> way to call irq_set_affinity() while holding the desc->lock.
>
> Hmm. The offline fixup_irq() code is arch specific and usually calls
> desc->irq_data.chip->irq_set_affinity under desc->lock. I have not yet
> found an arch independent way to do that. Any ideas welcome.
>

There are all the new affinity callbacks, and the things shown in 
procfs?  Are those handled properly if I call chip->irq_set_affinity?  I 
think not.

David Daney

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists