* 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.