IRQF_TIMER | IRQF_SHARED ?

[Andres Salomon]

On Mon, 12 Dec 2011 16:41:25 +0100

Hi Andres,
one of our customers tripped over the fact that the MFGPT driver
won't share its IRQ with anyone else. (MFGPT defaulted to same IRQ as
audio, MFGPT driver loaded first, audio fails.) *No big deal!* They
don't actually need MFGPT and will simply disable it. It just made me
wonder ...
Why would it be such a bad idea to use IRQF_TIMER | IRQF_SHARED (see
patch below)? mfgpt_tick() already does properly return IRQ_NONE when
it feels unresponsible. I tested it with either driver loaded first
and it seemed to work (well, at least audio worked, don't know how to
explicitly test cs5535-clockevt).

Just loading cs5535-clockevt should start the periodic timer. On my
XO-1, IRQ 7 starts firing immediately.

I thought about latencies of IRQ sharing being unacceptable for a
timer, but ...
- If MFGPT is loaded first there is no additional latency, is there?
Audio recieves its IRQs only as 2nd in list but that's not a problem.
- If MFGPT is loaded second - well, there is a latency, but without
sharing the IRQ the driver failed to load at all, so that's still an
improvement.
But I did not fail to notice that _none_ of the code in
drivers/clocksource/ uses IRQF_SHARED, obviously this must be
deliberate.

Hm, maybe tglx knows? For my part, I don't think it would be a
problem, but I can imagine the reason for not sharing being clock drift
or something to that effect.

So, what's so bad about IRQF_TIMER | IRQF_SHARED?
Any education would be welcome, even if combined with flame. :-)
Thanks and best regards,
Jens
--- linux-3.2-rc4/drivers/clocksource/cs5535-clockevt.c
+++ shared_mfgpt_irq/drivers/clocksource/cs5535-clockevt.c
@@ -133,7 +133,7 @@ static irqreturn_t mfgpt_tick(int irq, v
static struct irqaction mfgptirq = {
.handler = mfgpt_tick,
- .flags = IRQF_DISABLED | IRQF_NOBALANCING | IRQF_TIMER,
+ .flags = IRQF_DISABLED | IRQF_NOBALANCING | IRQF_TIMER |
IRQF_SHARED, .name = DRV_NAME,
};
_

[Andres Salomon]

On Mon, 12 Dec 2011 23:00:01 +0200

12. joulukuuta 2011 22.31 Andres Salomon <dilinger at queued.net>

Post by Andres Salomon
On Mon, 12 Dec 2011 16:41:25 +0100

one of our customers tripped over the fact that the MFGPT driver
won't share its IRQ with anyone else. (MFGPT defaulted to same IRQ
as audio, MFGPT driver loaded first, audio fails.) *No big deal!*
They don't actually need MFGPT and will simply disable it. It just
made me wonder ...
Why would it be such a bad idea to use IRQF_TIMER | IRQF_SHARED
(see patch below)? mfgpt_tick() already does properly return
IRQ_NONE when it feels unresponsible. I tested it with either
driver loaded first and it seemed to work (well, at least audio
worked, don't know how to explicitly test cs5535-clockevt).

Just loading cs5535-clockevt should start the periodic timer. ?On my
XO-1, IRQ 7 starts firing immediately.

Could it be a good idea to inform udev maintainers of this?

It *would* be nice to get it auto-loading..

[Andres Salomon]

On Wed, 14 Dec 2011 19:37:39 +0100

Post by Andres Salomon
Just loading cs5535-clockevt should start the periodic timer.
On my XO-1, IRQ 7 starts firing immediately.

Hmm, no, doesn't work. :-| In /proc/interrupts IRQ 0 gets
increased, not (in my case) IRQ 11.

Update: I found that SMP-enabled kernels (like the generic one I was
using) do load cs5535-clockevt fine but ignore it and keep using the
pit timer instead.

Note that there are two subsystems at work here; clockevents and
clocksource. cs5535-clockevt uses clockevents (despite being in
drivers/clocksource/). For you to switch away from pit (via,
/sys/devices/system/clocksource/clocksource0/current_clocksource), we'd
have to implement cs5535_clocksource. Something that would be worth
doing, but I haven't looked into it..

Responsible seems kernel/time/tick-common.c: tick_check_new_device()
/*
* If the cpu affinity of the device interrupt can not
* be set, ignore it.
*/
if (!irq_can_set_affinity(newdev->irq))
goto out_bc;
Looks like it has been that way for quite some time, maybe
cs5535-clockevt hasn't ever worked on SMP kernels.

I've only ever tested it on UP kernels. I didn't know SMP CS5536
boards existed. Gosh, I hope all of those cs5535 drivers that hadn't
had their locking primitives tested aren't racy! ;)

If I turn off SMP, cs5535-clockevt replaces the pit timer and the
MFGPT IRQ starts firing just as you said - at least on 3.0.9. 3.2-rc5
prefers to hang instead, looks like no timer events get generated.
Sigh.

3.2-rc4 hung on my XO-1 due to a bug that's been fixed on rc5, but I
haven't tested rc5 yet.

I'll try to narrow it down tomorrow, but now I'm calling it a day.
Cheers
Jens

[Andres Salomon]

Seems fine to me

Acked-by: Andres Salomon <dilinger at queued.net>

On Thu, 22 Dec 2011 17:35:50 +0100

cs5535-clockevt: allow the MFGPT IRQ to be shared
Shared timer IRQs are not a good solution, however the Geode platform
has no APIC, IRQs are a scarce resource and there is no technical
reason to forbid it rightaway. Increased latencies and overhead due
to sharing are still better than a driver refusing to load.
Signed-off-by: Jens Rottmann <JRottmann at LiPPERTEmbedded.de>
---
Hi,
I tested this a bit longer, this time with MFGPT IRQ actually being
triggered, with cs5535-clockevt driver loaded first or second when
sharing the IRQ, with some CPU load or without.
I didn't encounter any negative effects of this change. I did have
suspend problems with cs5535-clockevt, but that was in no way
different from before I applied this patch, it's an unrelated BIOS
issue.

No, you can share a timer irq. The other drivers don't have the
SHARED flag set because they are on exclusive irq lines, ...

Is this an ACK?

shared irqs suck and you figure that out once you try to
use that shared timer irq on a preempt-rt enabled kernel.

Or a NACK? :-| (I did address this in the commit log.)
The kernel I tested with was 3.2-rc6, CONFIG_SMP=y and
CONFIG_PREEMPT=y. As I said, I didn't notice anything bad happening.
Thanks and have a nice christmas holiday,
Jens
--- linux-3.2-rc6/drivers/clocksource/cs5535-clockevt.c
+++ allow_shared_mfgpt_irq/drivers/clocksource/cs5535-clockevt.c
@@ -133,7 +133,7 @@ static irqreturn_t mfgpt_tick(int irq, v
static struct irqaction mfgptirq = {
.handler = mfgpt_tick,
- .flags = IRQF_DISABLED | IRQF_NOBALANCING | IRQF_TIMER,
+ .flags = IRQF_DISABLED | IRQF_NOBALANCING | IRQF_TIMER |
IRQF_SHARED, .name = DRV_NAME,
};
_

[Andres Salomon]

On Wed, 21 Dec 2011 16:42:20 +0100

cs5535-clockevt: don't ignore MFGPT on SMP-capable kernels
On SMP-capable kernels (e.g. generic distro kernel) the
cs5535-clockevt driver loads but is not actually used.
Setting cpumask to cpu_all_mask works for UP-only kernels, but if
compiled for SMP - though still running on the same UP hardware -
kernel/time/tick-common.c:tick_check_new_device() reads this as
"non-cpu-local" and silently ignores the device.
If we leave cpumask unset clockevents_register_device() will
initialize it and the cs5535-clockevt driver will be used no matter
how the kernel was compiled. Should anyone ever manage to stick a
CS553x in an SMP system (is this even possible?) then a warning will
be printed. This is fine as the cs5535-clockevt driver was never
written/tested for SMP.
If bisecting led you here this patch may have exposed a pre-existing
MFGPT problem. Configure for UP-only and re-check.
Signed-off-by: Jens Rottmann <JRottmann at LiPPERTEmbedded.de>
---
--- linux-3.2-rc6/drivers/clocksource/cs5535-clockevt.c
+++ use_mfgpt_on_smp_kernels/drivers/clocksource/cs5535-clockevt.c
@@ -100,7 +100,6 @@ static struct clock_event_device cs5535_
.set_mode = mfgpt_set_mode,
.set_next_event = mfgpt_next_event,
.rating = 250,
- .cpumask = cpu_all_mask,
.shift = 32
};
_

Hm, have you tried setting cpumask to cpumask_of(0), like a bunch of
the other clock_event_device drivers do?

[Andres Salomon]

On Thu, 19 Jan 2012 13:57:31 +0100

Hi Andres,
and a happy new year to you. Sorry for the delay.

Post by Andres Salomon

cs5535-clockevt: don't ignore MFGPT on SMP-capable kernels
On SMP-capable kernels (e.g. generic distro kernel) the
cs5535-clockevt driver loads but is not actually used.
Setting cpumask to cpu_all_mask works for UP-only kernels, but if
compiled for SMP - though still running on the same UP hardware -
kernel/time/tick-common.c:tick_check_new_device() reads this as
"non-cpu-local" and silently ignores the device.
If we leave cpumask unset clockevents_register_device() will
initialize it and the cs5535-clockevt driver will be used no matter
how the kernel was compiled. Should anyone ever manage to stick a
CS553x in an SMP system (is this even possible?) then a warning
will be printed. This is fine as the cs5535-clockevt driver was
never written/tested for SMP.
If bisecting led you here this patch may have exposed a
pre-existing MFGPT problem. Configure for UP-only and re-check.
Signed-off-by: Jens Rottmann <JRottmann at LiPPERTEmbedded.de>
---
--- linux-3.2-rc6/drivers/clocksource/cs5535-clockevt.c
+++ use_mfgpt_on_smp_kernels/drivers/clocksource/cs5535-clockevt.c
@@ -100,7 +100,6 @@ static struct clock_event_device cs5535_
.set_mode = mfgpt_set_mode,
.set_next_event = mfgpt_next_event,
.rating = 250,
- .cpumask = cpu_all_mask,
.shift = 32
};
_

Hm, have you tried setting cpumask to cpumask_of(0), like a bunch of
the other clock_event_device drivers do?

Yes, I've seen that, and it was my initial approach. cpumask_of(0)
isn't regarded constant, so moved it from the struct init to
cs5535_mfgpt_init() ... but then I saw
if (!dev->cpumask) {
WARN_ON(num_possible_cpus() > 1);
dev->cpumask = cpumask_of(smp_processor_id());
}
Looks to me like meant exactly for this purpose. This will set
cpumask to cpumask_of(0), i.e. does exactly what you're suggesting.

Ah, great! In that case,

Acked-by: Andres Salomon <dilinger at queued.net>