On 2013-01-12 18:59, Gilles Chanteperdrix wrote: > On 01/12/2013 06:45 PM, Jan Kiszka wrote: > >> On 2013-01-12 18:35, Gilles Chanteperdrix wrote: >>> On 01/11/2013 08:45 AM, Jan Kiszka wrote: >>> >>>> On 2013-01-11 08:37, Jan Kiszka wrote: >>>>>> From acc3c57390d275a1b28d66f1ec88c85e0f8c0890 Mon Sep 17 00:00:00 2001 >>>>>> From: Gilles Chanteperdrix <[email protected]> >>>>>> Date: Sat, 29 Dec 2012 17:48:20 +0100 >>>>>> Subject: [PATCH] x86/ipipe: restore warning with AMD erratum >>>>>> >>>>>> --- >>>>>> arch/x86/kernel/apic/apic.c | 16 ++++++++++------ >>>>>> 1 files changed, 10 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c >>>>>> index 7f07610..91531bb 100644 >>>>>> --- a/arch/x86/kernel/apic/apic.c >>>>>> +++ b/arch/x86/kernel/apic/apic.c >>>>>> @@ -545,14 +545,18 @@ static void __cpuinit setup_APIC_timer(void) >>>>>> memcpy(levt, &lapic_clockevent, sizeof(*levt)); >>>>>> levt->cpumask = cpumask_of(smp_processor_id()); >>>>>> #ifdef CONFIG_IPIPE >>>>>> - if (!(lapic_clockevent.features & CLOCK_EVT_FEAT_DUMMY)) >>>>>> + if (!(lapic_clockevent.features & CLOCK_EVT_FEAT_DUMMY) >>>>>> + && !cpu_has_amd_erratum(amd_erratum_400)) >>>>>> levt->ipipe_timer = &__get_cpu_var(lapic_itimer); >>>>>> else { >>>>>> - printk(KERN_INFO >>>>>> - "I-pipe: cannot use LAPIC as a tick device\n"); >>>>>> - if (cpu_has_amd_erratum(amd_erratum_400)) >>>>>> - printk(KERN_INFO >>>>>> - "I-pipe: disable C1E power state in your >>>>>> BIOS\n"); >>>>>> + static atomic_t printed = ATOMIC_INIT(-1); >>>>>> + printk(KERN_INFO >>>>>> + "I-pipe: cannot use LAPIC on cpu #%d as a tick >>>>>> device\n", >>>>>> + smp_processor_id()); >>>>>> + if (cpu_has_amd_erratum(amd_erratum_400) >>>>>> + && atomic_inc_and_test(&printed)) >>>>>> + printk(KERN_INFO >>>>>> + "I-pipe: disable C1E power state in your >>>>>> BIOS\n"); >>>>> >>>>> printk_once should do the trick as well. >>>> >>>> In fact, it should actually do what you likely intended: print on first >>>> occurrence, not on second or later. ;) >>> >>> >>> Not quite, printk_once is not protected from the code running >>> concurrently on multiple cpus. >> >> OK. >> >>> So, atomic_inc_and_test is better, except >>> the ATOMIC_INIT needs to be set to 0. >> >> Nope, -1 is correct. atomic_inc_and_test first increments, then tests. I >> missed that it checks against 0, not != 0. So the logic is fine, you >> should just include the first printk as well. > > > Yes, I have just realized that looking at the x86 version of > atomic_inc_and_test, only the asm-generic version is broken, but I guess > nobody uses it: > > static inline int atomic_add_return(int i, atomic_t *v) > { > unsigned long flags; > int temp; > > flags = hard_local_irq_save(); /* Don't trace it in an irqsoff handler > */ > temp = v->counter; > temp += i; > v->counter = temp; > hard_local_irq_restore(flags); > > return temp; > } > > #define atomic_inc_and_test(v) (atomic_inc_return(v) == 0) >
Are you referring to lacking SMP support of this version? I guess that's intentional (no SMP arch should lack atomic ops). It is otherwise functionally identical to the x86 code, i.e. it *first* increments and *then* tests. Your new version of this patch will therefore never print anything due to the 0 initialization - well, until the counter overflows. Probably it's better to use test_and_set_bit here to avoid all these confusions. Jan -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 261 bytes Desc: OpenPGP digital signature URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20130113/09252461/attachment.pgp> _______________________________________________ Xenomai mailing list [email protected] http://www.xenomai.org/mailman/listinfo/xenomai
