* Re: [Xenomai] [PATCH] x86/ipipe: restore warning with AMD erratum @ 2013-01-11 7:37 Jan Kiszka 2013-01-11 7:45 ` Jan Kiszka 0 siblings, 1 reply; 10+ messages in thread From: Jan Kiszka @ 2013-01-11 7:37 UTC (permalink / raw) To: Gilles Chanteperdrix; +Cc: Xenomai > From acc3c57390d275a1b28d66f1ec88c85e0f8c0890 Mon Sep 17 00:00:00 2001 > From: Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org> > 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. > } > #endif /* CONFIG_IPIPE */ > > -- > 1.7.3.4 I can also carry an updated version in my queue if you like. Did you already request a merge of your queue? Then I would rebase mine on top these days. Jan PS: ipipe-jki.git works now, thanks again. -------------- 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/20130111/6d1ee428/attachment.pgp> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xenomai] [PATCH] x86/ipipe: restore warning with AMD erratum 2013-01-11 7:37 [Xenomai] [PATCH] x86/ipipe: restore warning with AMD erratum Jan Kiszka @ 2013-01-11 7:45 ` Jan Kiszka 2013-01-11 7:49 ` Gilles Chanteperdrix 2013-01-12 17:35 ` Gilles Chanteperdrix 0 siblings, 2 replies; 10+ messages in thread From: Jan Kiszka @ 2013-01-11 7:45 UTC (permalink / raw) To: Gilles Chanteperdrix; +Cc: Xenomai On 2013-01-11 08:37, Jan Kiszka wrote: >> From acc3c57390d275a1b28d66f1ec88c85e0f8c0890 Mon Sep 17 00:00:00 2001 >> From: Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org> >> 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. ;) 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/20130111/0c53e5cc/attachment.pgp> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xenomai] [PATCH] x86/ipipe: restore warning with AMD erratum 2013-01-11 7:45 ` Jan Kiszka @ 2013-01-11 7:49 ` Gilles Chanteperdrix 2013-01-11 7:52 ` Jan Kiszka 2013-01-12 17:35 ` Gilles Chanteperdrix 1 sibling, 1 reply; 10+ messages in thread From: Gilles Chanteperdrix @ 2013-01-11 7:49 UTC (permalink / raw) To: Jan Kiszka; +Cc: Xenomai 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 <gilles.chanteperdrix@xenomai.org> >>> 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. ;) printed is initialized at -1, so, the printk happens on first occurence, but yes printk_once is good, and we can use printk_once for the first printk too, these two tests (feat_dummy and amd_erratum_400) will be true for all cpus, so, printing the message for all cpus has no added value. > > Jan > -- Gilles. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xenomai] [PATCH] x86/ipipe: restore warning with AMD erratum 2013-01-11 7:49 ` Gilles Chanteperdrix @ 2013-01-11 7:52 ` Jan Kiszka 2013-01-11 7:55 ` Gilles Chanteperdrix 0 siblings, 1 reply; 10+ messages in thread From: Jan Kiszka @ 2013-01-11 7:52 UTC (permalink / raw) To: Gilles Chanteperdrix; +Cc: Xenomai On 2013-01-11 08:49, 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 <gilles.chanteperdrix@xenomai.org> >>>> 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. ;) > > > printed is initialized at -1, so, the printk happens on first occurence, inc_and_test increments first and then returns the result. > but yes printk_once is good, and we can use printk_once for the first > printk too, these two tests (feat_dummy and amd_erratum_400) will be > true for all cpus, so, printing the message for all cpus has no added value. Perfect. 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/20130111/320ba77b/attachment.pgp> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xenomai] [PATCH] x86/ipipe: restore warning with AMD erratum 2013-01-11 7:52 ` Jan Kiszka @ 2013-01-11 7:55 ` Gilles Chanteperdrix 0 siblings, 0 replies; 10+ messages in thread From: Gilles Chanteperdrix @ 2013-01-11 7:55 UTC (permalink / raw) To: Jan Kiszka; +Cc: Xenomai On 01/11/2013 08:52 AM, Jan Kiszka wrote: > On 2013-01-11 08:49, 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 <gilles.chanteperdrix@xenomai.org> >>>>> 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. ;) >> >> >> printed is initialized at -1, so, the printk happens on first occurence, > > inc_and_test increments first and then returns the result. Yes, the other way around, the result used is the value before incrementation, so printed should be initialized to 0, right... -- Gilles. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xenomai] [PATCH] x86/ipipe: restore warning with AMD erratum 2013-01-11 7:45 ` Jan Kiszka 2013-01-11 7:49 ` Gilles Chanteperdrix @ 2013-01-12 17:35 ` Gilles Chanteperdrix 2013-01-12 17:45 ` Jan Kiszka 1 sibling, 1 reply; 10+ messages in thread From: Gilles Chanteperdrix @ 2013-01-12 17:35 UTC (permalink / raw) To: Jan Kiszka; +Cc: Xenomai 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 <gilles.chanteperdrix@xenomai.org> >>> 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. So, atomic_inc_and_test is better, except the ATOMIC_INIT needs to be set to 0. > > Jan > -- Gilles. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xenomai] [PATCH] x86/ipipe: restore warning with AMD erratum 2013-01-12 17:35 ` Gilles Chanteperdrix @ 2013-01-12 17:45 ` Jan Kiszka 2013-01-12 17:59 ` Gilles Chanteperdrix 0 siblings, 1 reply; 10+ messages in thread From: Jan Kiszka @ 2013-01-12 17:45 UTC (permalink / raw) To: Gilles Chanteperdrix; +Cc: Xenomai 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 <gilles.chanteperdrix@xenomai.org> >>>> 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. 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/20130112/9e2ee820/attachment.pgp> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xenomai] [PATCH] x86/ipipe: restore warning with AMD erratum 2013-01-12 17:45 ` Jan Kiszka @ 2013-01-12 17:59 ` Gilles Chanteperdrix 2013-01-13 12:28 ` Jan Kiszka 0 siblings, 1 reply; 10+ messages in thread From: Gilles Chanteperdrix @ 2013-01-12 17:59 UTC (permalink / raw) To: Jan Kiszka; +Cc: Xenomai 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 <gilles.chanteperdrix@xenomai.org> >>>>> 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) -- Gilles. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xenomai] [PATCH] x86/ipipe: restore warning with AMD erratum 2013-01-12 17:59 ` Gilles Chanteperdrix @ 2013-01-13 12:28 ` Jan Kiszka 2013-01-13 12:39 ` Gilles Chanteperdrix 0 siblings, 1 reply; 10+ messages in thread From: Jan Kiszka @ 2013-01-13 12:28 UTC (permalink / raw) To: Gilles Chanteperdrix; +Cc: Xenomai 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 <gilles.chanteperdrix@xenomai.org> >>>>>> 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xenomai] [PATCH] x86/ipipe: restore warning with AMD erratum 2013-01-13 12:28 ` Jan Kiszka @ 2013-01-13 12:39 ` Gilles Chanteperdrix 0 siblings, 0 replies; 10+ messages in thread From: Gilles Chanteperdrix @ 2013-01-13 12:39 UTC (permalink / raw) To: Jan Kiszka; +Cc: Xenomai On 01/13/2013 01:28 PM, Jan Kiszka wrote: > 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 <gilles.chanteperdrix@xenomai.org> >>>>>>> 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. Sorry, fixed, thanks. -- Gilles. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-01-13 12:39 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-11 7:37 [Xenomai] [PATCH] x86/ipipe: restore warning with AMD erratum Jan Kiszka 2013-01-11 7:45 ` Jan Kiszka 2013-01-11 7:49 ` Gilles Chanteperdrix 2013-01-11 7:52 ` Jan Kiszka 2013-01-11 7:55 ` Gilles Chanteperdrix 2013-01-12 17:35 ` Gilles Chanteperdrix 2013-01-12 17:45 ` Jan Kiszka 2013-01-12 17:59 ` Gilles Chanteperdrix 2013-01-13 12:28 ` Jan Kiszka 2013-01-13 12:39 ` Gilles Chanteperdrix
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.