IRQF_TIMER | IRQF_SHARED ?

Quoted 10 lines · Andres Salomon

Quoted 3 lines · On Mon, 12 Dec 2011 16:41:25 +0100 Jens Rottmann…
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.

No, you can share a timer irq. The other drivers don't have the SHARED flag set because they are on exclusive irq lines, which is the sane thing to do. shared irqs suck and you figure that out once you try to use that shared timer irq on a preempt-rt enabled kernel.

Thanks,

tglx

Jens Rottmann

author

2011-12-13 15:49 UTC #5 lore raw

Quoted 5 lines · Andres Salomon

Quoted 2 lines · Jens Rottmann
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.

Hmm, no, doesn't work. :-| In /proc/interrupts IRQ0 gets increased, not (in my case) IRQ 11. And also in /proc/timer_list next_event gets increased for the pit device, not cs5535-clockevt.

But that's even without sharing enabled ... I guess I'll have to figure out first what's going wrong here and then re-test. I'll get back to you ...

Thanks and regards,
Jens

Jens Rottmann

author

2011-12-14 18:37 UTC #6 lore raw

Quoted 5 lines · Jens Rottmann

Quoted 2 lines · 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.

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.

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.

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

Cheers
Jens

Andres Salomon

2011-12-14 18:47 UTC #7 lore raw

Quoted 10 lines · On Wed, 14 Dec 2011 19:37:39 +0100 Jens Rottmann…

Quoted 5 lines · Jens Rottmann
Quoted 2 lines · 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..

Quoted 11 lines

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! ;)

Quoted 6 lines

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.

Quoted 4 lines

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

Jens Rottmann

author

2011-12-21 15:42 UTC #8 lore raw

PATCH 1/1 cs5535-clockevt: don't ignore MFGPT on SMP-capable kernels

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.

---

drivers/clocksource/cs5535-clockevt.c +0 -1

@@ -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
 };

Andres Salomon

2012-01-11 11:13 UTC #9 lore raw

Re: [PATCH] cs5535-clockevt: don't ignore MFGPT on SMP-capable kernels

Quoted 33 lines · On Wed, 21 Dec 2011 16:42:20 +0100 Jens Rottmann…

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 <[email protected]>
---
drivers/clocksource/cs5535-clockevt.c +0 -1
@@ -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
 };
 

Quoted 2 lines

_

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

Jens Rottmann

author

2012-01-19 12:57 UTC #10 lore raw

Re: [PATCH] cs5535-clockevt: don't ignore MFGPT on SMP-capable kernels

Hi Andres,

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

Quoted 34 lines · Andres Salomon

Quoted 33 lines · Jens Rottmann
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 <[email protected]>
---
drivers/clocksource/cs5535-clockevt.c +0 -1
@@ -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
 };
 

Quoted 4 lines

Quoted 1 line
_
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

clockevents.c:clockevents_register_device():

	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.

Both do work fine, but I understand cpumask=cpumask_of(0) to mean "this timer is hardware-tied to a specific CPU", whereas cpumask unset means more like "wow, we never expected anybody putting this hardware in an SMP system".

And you've said it yourself:

Quoted 2 lines

I've only ever tested it on UP kernels. [...] I hope all of those cs5535 drivers that hadn't had their locking primitives tested aren't racy!

So I see the WARN_ON as a bonus.

Cheers
Jens

Andres Salomon

Acked-by

2012-01-23 11:16 UTC #11 lore raw

Re: [PATCH] cs5535-clockevt: don't ignore MFGPT on SMP-capable kernels

Quoted 48 lines · On Thu, 19 Jan 2012 13:57:31 +0100 Jens Rottmann…

Hi Andres,
and a happy new year to you. Sorry for the delay.
Quoted 34 lines · Andres Salomon
Quoted 33 lines · Jens Rottmann
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 <[email protected]>
---
drivers/clocksource/cs5535-clockevt.c +0 -1
@@ -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
 };
 
Quoted 4 lines
Quoted 1 line
_
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

Quoted 9 lines

clockevents.c:clockevents_register_device():
	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,

Jens Rottmann

author

2011-12-21 16:37 UTC #12 lore raw

Hi Andres,

Quoted 3 lines

Quoted 3 lines
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.

Solved, hope my patch is ok.

Quoted 1 line

I didn't know SMP CS5536 boards existed.

They don't. I was talking about SMP-capable kernels, e.g. _generic_ distro kernels which were compiled with SMP support enabled, even though this feature is not needed for a UP-only Geode. Sorry if I wasn't making myself clear.

Cheers,
Jens

Jens Rottmann

author

2011-12-22 16:35 UTC #13 lore raw

PATCH 1/1 cs5535-clockevt: allow the MFGPT IRQ to be shared

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.

---

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.

Quoted 2 lines · Thomas Gleixner

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?

Quoted 2 lines

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

drivers/clocksource/cs5535-clockevt.c +1 -1

@@ -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

Acked-by on v1 1/1

2012-01-11 11:13 UTC #14 lore raw

Re: [PATCH] cs5535-clockevt: allow the MFGPT IRQ to be shared

Seems fine to me

Quoted 51 lines · On Thu, 22 Dec 2011 17:35:50 +0100 Jens Rottmann…

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 <[email protected]>
---
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.
Quoted 2 lines · Thomas Gleixner
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?
Quoted 2 lines
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
drivers/clocksource/cs5535-clockevt.c +1 -1
@@ -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,
 };
 
_

Jens Rottmann

author

2012-01-30 14:01 UTC #15 lore raw

PATCH 1/1 cs5535-clockevt: don't ignore MFGPT on SMP-capable kernels

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.

If bisecting led you here this patch may have exposed a pre-existing MFGPT problem. Configure for UP-only and re-check.

---

Hi,

could you please take this, for linux-next?

Thanks,
Jens

drivers/clocksource/cs5535-clockevt.c +0 -1

@@ -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
 };

Jens Rottmann

author

2012-01-30 14:01 UTC #16 lore raw

PATCH 1/1 cs5535-clockevt: allow the MFGPT IRQ to be shared

cs5535-clockevt: allow the MFGPT IRQ to be shared

---

Hi,

could you please take this, for linux-next?

Thanks,
Jens

drivers/clocksource/cs5535-clockevt.c +1 -1

@@ -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,
 };

Jens Rottmann

author

2012-02-06 08:27 UTC #17 lore raw

PATCH RESEND 1/1 cs5535-clockevt: don't ignore MFGPT on SMP-capable kernels

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.

If bisecting led you here this patch may have exposed a pre-existing MFGPT problem. Configure for UP-only and re-check.

---

Hi again,

could you please take this, for linux-next?

Thanks,
Jens

drivers/clocksource/cs5535-clockevt.c +0 -1

@@ -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
 };

Jens Rottmann

author

2012-02-06 08:27 UTC #18 lore raw

PATCH RESEND 1/1 cs5535-clockevt: allow the MFGPT IRQ to be shared

cs5535-clockevt: allow the MFGPT IRQ to be shared