From: David Daney <ddaney@caviumnetworks.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-mips@linux-mips.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 1/2] genirq: Add chip hooks for taking CPUs on/off line.
Date: Mon, 21 Mar 2011 11:26:49 -0700 [thread overview]
Message-ID: <4D879869.8060405@caviumnetworks.com> (raw)
In-Reply-To: <alpine.LFD.2.00.1103192050400.2787@localhost6.localdomain6>
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
I need to know in my .unmask method if the interrupt has been disabled.
If it has, I will not re-enable (unmask)it.
>
>> #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.
>
>> +{
>
> So that's called on the cpu which goes online, right?
>
Yes.
> I wonder whether we can add any sanity check to verify this.
>
> Though I would not worry too much about it. Calling that from a cpu
> which is not going offline should have enough nasty side effects that
> it's noticed during development. :)
>
>> + unsigned long flags;
>> + struct irq_chip *chip;
>> + struct irq_desc *desc = irq_to_desc(irq);
>
> Needs to check !desc
OK.
>
>> + raw_spin_lock_irqsave(&desc->lock, flags);
>> +
>> + chip = irq_data_get_irq_chip(&desc->irq_data);
>> +
>> + if (chip&& chip->irq_cpu_online)
>> + chip->irq_cpu_online(&desc->irq_data,
>> + irq_desc_is_enabled(desc));
>> +
>> + raw_spin_unlock_irqrestore(&desc->lock, flags);
>> +}
>> +
>> +void irq_cpu_offline(unsigned int irq)
>> +{
>> + unsigned long flags;
>> + struct irq_chip *chip;
>> + struct irq_desc *desc = irq_to_desc(irq);
>
> See above.
>
> Style nit: I prefer ordering:
>
> + struct irq_desc *desc = irq_to_desc(irq);
> + struct irq_chip *chip;
> + unsigned long flags;
>
> For some reason, probably because I'm used to it, that's easier to
> parse. But don't worry about that, I'll turn it around before sticking
> it into git. :)
>
> Otherwise I'm fine with the approach itself.
>
> 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.
David Daney
next prev parent reply other threads:[~2011-03-21 18:26 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-03-18 21:48 [RFC PATCH 0/2] Add IRQ chip hooks for taking CPUs on/off line David Daney
2011-03-18 21:48 ` [RFC PATCH 1/2] genirq: Add " David Daney
2011-03-19 20:51 ` Thomas Gleixner
2011-03-21 18:26 ` David Daney [this message]
2011-03-21 21:13 ` Thomas Gleixner
2011-03-23 22:03 ` David Daney
2011-03-23 22:09 ` Thomas Gleixner
2011-03-18 21:48 ` [RFC PATCH 2/2] MIPS: Octeon: Rewrite interrupt handling code David Daney
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4D879869.8060405@caviumnetworks.com \
--to=ddaney@caviumnetworks.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@linux-mips.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).