* Re: [PATCH V2] tick-broadcast: Register for hrtimer based broadcast as the fallback broadcast mode
From: Preeti U Murthy @ 2014-12-08 12:02 UTC (permalink / raw)
To: Mark Rutland
Cc: Lorenzo Pieralisi, peterz@infradead.org, fweisbec@gmail.com,
rafael.j.wysocki@intel.com, Will Deacon,
linux-kernel@vger.kernel.org, jingchang.lu@freescale.com,
tglx@linutronix.de, shawn.guo@linaro.org,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20141208104828.GA21680@leverpostej>
On 12/08/2014 04:18 PM, Mark Rutland wrote:
> Hi Preeti,
>
> On Mon, Dec 08, 2014 at 06:55:43AM +0000, Preeti U Murthy wrote:
>> Commit 5d1638acb9f6 ('tick: Introduce hrtimer based broadcast') added a
>> hrtimer based broadcast mode for those platforms in which local timers stop
>> when CPUs enter deep idle states. The commit expected the platforms to
>> register for this mode explicitly when they lacked a better external device
>> to wake up CPUs in deep idle. Given that more platforms are beginning to use
>> this mode, we can avoid the call to set it up on every platform that requires
>> it, by registering for the hrtimer based broadcast mode in the core code if
>> no better broadcast device is available.
>>
>> This commit also helps detect cases where the platform fails to register for
>> a broadcast device but invokes the help of one when entering deep idle states.
>> Currently we do not handle this situation at all and call the broadcast clock
>> device without checking for its existence. This patch will handle such buggy
>> cases properly.
>>
>> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>
> I've just given this a go on an arm64 platform (Juno) without any
> system-wide clock_event_devices registered, and everything works well
> with CPUs entering and exiting idle states where the cpu-local timers
> lose state. So:
>
> Tested-by: Mark Rutland <mark.rutland@arm.com>
Thanks!
>
> One minor thing I noticed when testing was that
> /sys/devices/system/clockevents/broadcast/name contained "(null)",
> because we never set the name field on the clock_event_device. It's
> always been that way, but now might be a good time to change that to
> something like "broadcast_hrtimer".
You mean /sys/devices/system/clockevents/broadcast/current_device right?
>
> [...]
>
>> diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
>> index 2e4cb67..91754b0 100644
>> --- a/include/linux/clockchips.h
>> +++ b/include/linux/clockchips.h
>> @@ -187,11 +187,11 @@ extern int tick_receive_broadcast(void);
>> #endif
>>
>> #if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT)
>> -extern void tick_setup_hrtimer_broadcast(void);
>> +extern int __init tick_setup_hrtimer_broadcast(void);
>> extern int tick_check_broadcast_expired(void);
>> #else
>> static inline int tick_check_broadcast_expired(void) { return 0; }
>> -static inline void tick_setup_hrtimer_broadcast(void) {};
>> +static inline int __init tick_setup_hrtimer_broadcast(void) { return 0; }
>> #endif
>>
>> #ifdef CONFIG_GENERIC_CLOCKEVENTS
>> @@ -207,7 +207,7 @@ static inline void clockevents_resume(void) {}
>>
>> static inline int clockevents_notify(unsigned long reason, void *arg) { return 0; }
>> static inline int tick_check_broadcast_expired(void) { return 0; }
>> -static inline void tick_setup_hrtimer_broadcast(void) {};
>> +static inline int __init tick_setup_hrtimer_broadcast(void) { return 0; }
>
> With the initcall moved to the driver we have no external users of
> tick_setup_hrtimer_broadcast, so I think we can remove the prototype
> entirely from clockchips.h...
>
>> #endif
>>
>> diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
>> index eb682d5..5c35995 100644
>> --- a/kernel/time/tick-broadcast-hrtimer.c
>> +++ b/kernel/time/tick-broadcast-hrtimer.c
>> @@ -98,9 +98,11 @@ static enum hrtimer_restart bc_handler(struct hrtimer *t)
>> return HRTIMER_RESTART;
>> }
>>
>> -void tick_setup_hrtimer_broadcast(void)
>> +int __init tick_setup_hrtimer_broadcast(void)
>
> ...and make it static here.
Yep will do. Sorry I overlooked this.
>
>> {
>> hrtimer_init(&bctimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
>> bctimer.function = bc_handler;
>> clockevents_register_device(&ce_broadcast_hrtimer);
>> + return 0;
>> }
>> +early_initcall(tick_setup_hrtimer_broadcast);
>
> Otherwise this looks good to me, thanks for putting this together!
Thanks a lot for the review! Will send out the patch with the above
corrections.
Regards
Preeti U Murthy
>
> Mark.
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
^ permalink raw reply
* Re: [PATCH V2] tick-broadcast: Register for hrtimer based broadcast as the fallback broadcast mode
From: Mark Rutland @ 2014-12-08 12:11 UTC (permalink / raw)
To: Preeti U Murthy
Cc: Lorenzo Pieralisi, peterz@infradead.org, fweisbec@gmail.com,
rafael.j.wysocki@intel.com, Will Deacon,
linux-kernel@vger.kernel.org, jingchang.lu@freescale.com,
tglx@linutronix.de, shawn.guo@linaro.org,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <5485936F.20901@linux.vnet.ibm.com>
On Mon, Dec 08, 2014 at 12:02:55PM +0000, Preeti U Murthy wrote:
> On 12/08/2014 04:18 PM, Mark Rutland wrote:
> > Hi Preeti,
> >
> > On Mon, Dec 08, 2014 at 06:55:43AM +0000, Preeti U Murthy wrote:
> >> Commit 5d1638acb9f6 ('tick: Introduce hrtimer based broadcast') added a
> >> hrtimer based broadcast mode for those platforms in which local timers stop
> >> when CPUs enter deep idle states. The commit expected the platforms to
> >> register for this mode explicitly when they lacked a better external device
> >> to wake up CPUs in deep idle. Given that more platforms are beginning to use
> >> this mode, we can avoid the call to set it up on every platform that requires
> >> it, by registering for the hrtimer based broadcast mode in the core code if
> >> no better broadcast device is available.
> >>
> >> This commit also helps detect cases where the platform fails to register for
> >> a broadcast device but invokes the help of one when entering deep idle states.
> >> Currently we do not handle this situation at all and call the broadcast clock
> >> device without checking for its existence. This patch will handle such buggy
> >> cases properly.
> >>
> >> Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> >
> > I've just given this a go on an arm64 platform (Juno) without any
> > system-wide clock_event_devices registered, and everything works well
> > with CPUs entering and exiting idle states where the cpu-local timers
> > lose state. So:
> >
> > Tested-by: Mark Rutland <mark.rutland@arm.com>
>
> Thanks!
>
> >
> > One minor thing I noticed when testing was that
> > /sys/devices/system/clockevents/broadcast/name contained "(null)",
> > because we never set the name field on the clock_event_device. It's
> > always been that way, but now might be a good time to change that to
> > something like "broadcast_hrtimer".
>
> You mean /sys/devices/system/clockevents/broadcast/current_device right?
Whoops, yes I did.
> > [...]
> >
> >> diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
> >> index 2e4cb67..91754b0 100644
> >> --- a/include/linux/clockchips.h
> >> +++ b/include/linux/clockchips.h
> >> @@ -187,11 +187,11 @@ extern int tick_receive_broadcast(void);
> >> #endif
> >>
> >> #if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT)
> >> -extern void tick_setup_hrtimer_broadcast(void);
> >> +extern int __init tick_setup_hrtimer_broadcast(void);
> >> extern int tick_check_broadcast_expired(void);
> >> #else
> >> static inline int tick_check_broadcast_expired(void) { return 0; }
> >> -static inline void tick_setup_hrtimer_broadcast(void) {};
> >> +static inline int __init tick_setup_hrtimer_broadcast(void) { return 0; }
> >> #endif
> >>
> >> #ifdef CONFIG_GENERIC_CLOCKEVENTS
> >> @@ -207,7 +207,7 @@ static inline void clockevents_resume(void) {}
> >>
> >> static inline int clockevents_notify(unsigned long reason, void *arg) { return 0; }
> >> static inline int tick_check_broadcast_expired(void) { return 0; }
> >> -static inline void tick_setup_hrtimer_broadcast(void) {};
> >> +static inline int __init tick_setup_hrtimer_broadcast(void) { return 0; }
> >
> > With the initcall moved to the driver we have no external users of
> > tick_setup_hrtimer_broadcast, so I think we can remove the prototype
> > entirely from clockchips.h...
> >
> >> #endif
> >>
> >> diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
> >> index eb682d5..5c35995 100644
> >> --- a/kernel/time/tick-broadcast-hrtimer.c
> >> +++ b/kernel/time/tick-broadcast-hrtimer.c
> >> @@ -98,9 +98,11 @@ static enum hrtimer_restart bc_handler(struct hrtimer *t)
> >> return HRTIMER_RESTART;
> >> }
> >>
> >> -void tick_setup_hrtimer_broadcast(void)
> >> +int __init tick_setup_hrtimer_broadcast(void)
> >
> > ...and make it static here.
>
> Yep will do. Sorry I overlooked this.
>
> >
> >> {
> >> hrtimer_init(&bctimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
> >> bctimer.function = bc_handler;
> >> clockevents_register_device(&ce_broadcast_hrtimer);
> >> + return 0;
> >> }
> >> +early_initcall(tick_setup_hrtimer_broadcast);
> >
> > Otherwise this looks good to me, thanks for putting this together!
>
> Thanks a lot for the review! Will send out the patch with the above
> corrections.
Cheers!
Thanks,
Mark.
^ permalink raw reply
* [PATCH V3] tick-broadcast: Register for hrtimer based broadcast as the fallback broadcast mode
From: Preeti U Murthy @ 2014-12-08 12:23 UTC (permalink / raw)
To: mark.rutland, tglx
Cc: lorenzo.pieralisi, peterz, rafael.j.wysocki, will.deacon,
linux-kernel, jingchang.lu, linux-arm-kernel, fweisbec, shawn.guo,
linuxppc-dev
Commit 5d1638acb9f6 ('tick: Introduce hrtimer based broadcast') added a
hrtimer based broadcast mode for those platforms in which local timers stop
when CPUs enter deep idle states. The commit expected the platforms to
register for this mode explicitly when they lacked a better external device
to wake up CPUs in deep idle. Given that more platforms are beginning to use
this mode, we can avoid the call to set it up on every platform that requires
it, by registering for the hrtimer based broadcast mode in the core code if
no better broadcast device is available.
This commit also helps detect cases where the platform fails to register for
a broadcast device but invokes the help of one when entering deep idle states.
Currently we do not handle this situation at all and call the broadcast clock
device without checking for its existence. This patch will handle such buggy
cases properly. While at it, give a name to this mode of broadcast which was
missing all along.
Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
---
Changes from V1: https://lkml.org/lkml/2014/12/5/261
1.Moved registering the hrtimer based broadcast from timekeeping code
to an early_initcall.
Changes from V2: https://lkml.org/lkml/2014/12/8/57
1.Added the 'name' param to hrtimer broadcast mode and removed the prototype
to setup this mode since there are no external callers of it.
arch/arm64/kernel/time.c | 2 --
arch/powerpc/kernel/time.c | 1 -
include/linux/clockchips.h | 3 ---
kernel/time/tick-broadcast-hrtimer.c | 5 ++++-
4 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
index 1a7125c..47baaa8 100644
--- a/arch/arm64/kernel/time.c
+++ b/arch/arm64/kernel/time.c
@@ -70,8 +70,6 @@ void __init time_init(void)
of_clk_init(NULL);
clocksource_of_init();
- tick_setup_hrtimer_broadcast();
-
arch_timer_rate = arch_timer_get_rate();
if (!arch_timer_rate)
panic("Unable to initialise architected timer.\n");
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 7505599..51433a8 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -942,7 +942,6 @@ void __init time_init(void)
clocksource_init();
init_decrementer_clockevent();
- tick_setup_hrtimer_broadcast();
}
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 2e4cb67..c362143 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -187,11 +187,9 @@ extern int tick_receive_broadcast(void);
#endif
#if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT)
-extern void tick_setup_hrtimer_broadcast(void);
extern int tick_check_broadcast_expired(void);
#else
static inline int tick_check_broadcast_expired(void) { return 0; }
-static inline void tick_setup_hrtimer_broadcast(void) {};
#endif
#ifdef CONFIG_GENERIC_CLOCKEVENTS
@@ -207,7 +205,6 @@ static inline void clockevents_resume(void) {}
static inline int clockevents_notify(unsigned long reason, void *arg) { return 0; }
static inline int tick_check_broadcast_expired(void) { return 0; }
-static inline void tick_setup_hrtimer_broadcast(void) {};
#endif
diff --git a/kernel/time/tick-broadcast-hrtimer.c b/kernel/time/tick-broadcast-hrtimer.c
index eb682d5..1f6bc6a 100644
--- a/kernel/time/tick-broadcast-hrtimer.c
+++ b/kernel/time/tick-broadcast-hrtimer.c
@@ -72,6 +72,7 @@ static int bc_set_next(ktime_t expires, struct clock_event_device *bc)
}
static struct clock_event_device ce_broadcast_hrtimer = {
+ .name = "broadcast_hrtimer",
.set_mode = bc_set_mode,
.set_next_ktime = bc_set_next,
.features = CLOCK_EVT_FEAT_ONESHOT |
@@ -98,9 +99,11 @@ static enum hrtimer_restart bc_handler(struct hrtimer *t)
return HRTIMER_RESTART;
}
-void tick_setup_hrtimer_broadcast(void)
+static int __init tick_setup_hrtimer_broadcast(void)
{
hrtimer_init(&bctimer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
bctimer.function = bc_handler;
clockevents_register_device(&ce_broadcast_hrtimer);
+ return 0;
}
+early_initcall(tick_setup_hrtimer_broadcast);
^ permalink raw reply related
* Re: [PATCH] kthread: kthread_bind fails to enforce CPU affinity (fixes kernel BUG at kernel/smpboot.c:134!)
From: Steven Rostedt @ 2014-12-08 13:54 UTC (permalink / raw)
To: Anton Blanchard
Cc: yuyang.du, computersforpeace, peterz, lkp, rafael.j.wysocki,
yuanhan.liu, linux-kernel, bsegall, linuxppc-dev, mingo, sp,
daniel, tj, subbaram, akpm, fengguang.wu, torvalds, tglx, pjt
In-Reply-To: <1418009221-12719-1-git-send-email-anton@samba.org>
On Mon, 8 Dec 2014 14:27:01 +1100
Anton Blanchard <anton@samba.org> wrote:
> I have a busy ppc64le KVM box where guests sometimes hit the infamous
> "kernel BUG at kernel/smpboot.c:134!" issue during boot:
>
> BUG_ON(td->cpu != smp_processor_id());
>
> Basically a per CPU hotplug thread scheduled on the wrong CPU. The oops
> output confirms it:
>
> CPU: 0
> Comm: watchdog/130
>
> The issue is in kthread_bind where we set the cpus_allowed mask, but do
> not touch task_thread_info(p)->cpu. The scheduler assumes the previously
> scheduled CPU is in the cpus_allowed mask, but in this case we are
> moving a thread to another CPU so it is not.
>
Does this happen always on boot up, and always with the watchdog thread?
I followed the logic that starts the watchdog threads.
watchdog_enable_all_cpus()
smpboot_register_percpu-thread() {
for_each_online_cpu(cpu) { ... }
Where watchdog_enable_all_cpus() can be called by
lockup_detector_init() before SMP is started, but also by
proc_dowatchdog() which is called by the sysctl commands (after SMP is
up and running).
I noticed there's no "get_online_cpus()" anywhere, although the
unregister_percpu_thread() has it. Is it possible that we created a
thread on a CPU that wasn't fully online yet?
Perhaps the following patch is needed? Even if this isn't the solution
to this bug, it is probably needed as watchdog_enable_all_cpus() can be
called after boot up too.
-- Steve
diff --git a/kernel/smpboot.c b/kernel/smpboot.c
index eb89e1807408..60d35ac5d3f1 100644
--- a/kernel/smpboot.c
+++ b/kernel/smpboot.c
@@ -279,6 +279,7 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread)
unsigned int cpu;
int ret = 0;
+ get_online_cpus();
mutex_lock(&smpboot_threads_lock);
for_each_online_cpu(cpu) {
ret = __smpboot_create_thread(plug_thread, cpu);
@@ -291,6 +292,7 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread)
list_add(&plug_thread->list, &hotplug_threads);
out:
mutex_unlock(&smpboot_threads_lock);
+ put_online_cpus();
return ret;
}
EXPORT_SYMBOL_GPL(smpboot_register_percpu_thread);
^ permalink raw reply related
* [PATCH] powerpc32: missing accessors to pgprot_t objects
From: Christophe Leroy @ 2014-12-08 14:16 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
scottwood
Cc: linuxppc-dev, linux-kernel
Compilation with #define STRICT_MM_TYPECHECKS in arch/powerpc/include/asm/page.h
fails due to missing use of pgprot_val() when using pgprot_t objects.
arch/powerpc/mm/pgtable_32.c: In function '__ioremap_caller':
arch/powerpc/mm/pgtable_32.c:185:9: error: invalid operands to binary | (have 'long unsigned int' and 'pgprot_t')
flags |= PAGE_KERNEL;
^
arch/powerpc/mm/pgtable_32.c: In function '__mapin_ram_chunk':
arch/powerpc/mm/pgtable_32.c:320:5: error: incompatible types when assigning to type 'long unsigned int' from type 'pgprot_t'
f = ktext ? PAGE_KERNEL_TEXT : PAGE_KERNEL;
^
arch/powerpc/mm/ppc_mmu_32.c:95:2: error: incompatible type for argument 5 of 'setbat'
setbat(2, PAGE_OFFSET, 0, bl, PAGE_KERNEL_X);
^
In file included from arch/powerpc/mm/ppc_mmu_32.c:35:0:
arch/powerpc/mm/mmu_decl.h:98:13: note: expected 'int' but argument is of type 'pgprot_t'
extern void setbat(int index, unsigned long virt, phys_addr_t phys,
^
arch/powerpc/mm/ppc_mmu_32.c:103:3: error: incompatible type for argument 5 of 'setbat'
setbat(3, PAGE_OFFSET+done, done, bl, PAGE_KERNEL_X);
^
In file included from arch/powerpc/mm/ppc_mmu_32.c:35:0:
arch/powerpc/mm/mmu_decl.h:98:13: note: expected 'int' but argument is of type 'pgprot_t'
extern void setbat(int index, unsigned long virt, phys_addr_t phys,
^
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/mm/dma-noncoherent.c | 2 +-
arch/powerpc/mm/pgtable_32.c | 4 ++--
arch/powerpc/mm/ppc_mmu_32.c | 5 +++--
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/mm/dma-noncoherent.c b/arch/powerpc/mm/dma-noncoherent.c
index d85e86a..169aba4 100644
--- a/arch/powerpc/mm/dma-noncoherent.c
+++ b/arch/powerpc/mm/dma-noncoherent.c
@@ -228,7 +228,7 @@ __dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *handle, gfp_t
do {
SetPageReserved(page);
map_page(vaddr, page_to_phys(page),
- pgprot_noncached(PAGE_KERNEL));
+ pgprot_val(pgprot_noncached(PAGE_KERNEL)));
page++;
vaddr += PAGE_SIZE;
} while (size -= PAGE_SIZE);
diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index cf11342..a349089 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -182,7 +182,7 @@ __ioremap_caller(phys_addr_t addr, unsigned long size, unsigned long flags,
/* Make sure we have the base flags */
if ((flags & _PAGE_PRESENT) == 0)
- flags |= PAGE_KERNEL;
+ flags |= pgprot_val(PAGE_KERNEL);
/* Non-cacheable page cannot be coherent */
if (flags & _PAGE_NO_CACHE)
@@ -317,7 +317,7 @@ void __init __mapin_ram_chunk(unsigned long offset, unsigned long top)
p = memstart_addr + s;
for (; s < top; s += PAGE_SIZE) {
ktext = ((char *) v >= _stext && (char *) v < etext);
- f = ktext ? PAGE_KERNEL_TEXT : PAGE_KERNEL;
+ f = pgprot_val(ktext ? PAGE_KERNEL_TEXT : PAGE_KERNEL);
map_page(v, p, f);
#ifdef CONFIG_PPC_STD_MMU_32
if (ktext)
diff --git a/arch/powerpc/mm/ppc_mmu_32.c b/arch/powerpc/mm/ppc_mmu_32.c
index 5029dc1..dc710d4 100644
--- a/arch/powerpc/mm/ppc_mmu_32.c
+++ b/arch/powerpc/mm/ppc_mmu_32.c
@@ -92,7 +92,7 @@ unsigned long __init mmu_mapin_ram(unsigned long top)
break;
}
- setbat(2, PAGE_OFFSET, 0, bl, PAGE_KERNEL_X);
+ setbat(2, PAGE_OFFSET, 0, bl, pgprot_val(PAGE_KERNEL_X));
done = (unsigned long)bat_addrs[2].limit - PAGE_OFFSET + 1;
if ((done < tot) && !bat_addrs[3].limit) {
/* use BAT3 to cover a bit more */
@@ -100,7 +100,8 @@ unsigned long __init mmu_mapin_ram(unsigned long top)
for (bl = 128<<10; bl < max_size; bl <<= 1)
if (bl * 2 > tot)
break;
- setbat(3, PAGE_OFFSET+done, done, bl, PAGE_KERNEL_X);
+ setbat(3, PAGE_OFFSET+done, done, bl,
+ pgprot_val(PAGE_KERNEL_X));
done = (unsigned long)bat_addrs[3].limit - PAGE_OFFSET + 1;
}
--
2.1.0
^ permalink raw reply related
* [PATCH] powerpc32/chrp: fix section mismatch warning
From: Christophe Leroy @ 2014-12-08 14:16 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
scottwood
Cc: linuxppc-dev, linux-kernel
This patch fixes a section mismatch warning
WARNING: vmlinux.o(.text+0x213b6): Section mismatch in reference from the function chrp_init_early() to the variable .init.data:boot_command_line
The function chrp_init_early() references
the variable __initdata boot_command_line.
This is often because chrp_init_early lacks a __initdata
annotation or the annotation of boot_command_line is wrong.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/platforms/chrp/setup.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/chrp/setup.c b/arch/powerpc/platforms/chrp/setup.c
index 5b77b19..d5d1ea5 100644
--- a/arch/powerpc/platforms/chrp/setup.c
+++ b/arch/powerpc/platforms/chrp/setup.c
@@ -253,7 +253,7 @@ static void briq_restart(char *cmd)
* But unfortunately, the firmware does not connect /chosen/{stdin,stdout}
* the the built-in serial node. Instead, a /failsafe node is created.
*/
-static void chrp_init_early(void)
+static __init void chrp_init_early(void)
{
struct device_node *node;
const char *property;
--
2.1.0
^ permalink raw reply related
* Re: Right location in sysfs for dlpar file
From: Nathan Fontenot @ 2014-12-08 15:21 UTC (permalink / raw)
To: Greg KH; +Cc: linuxppc-dev
In-Reply-To: <20141204043102.GA21724@kroah.com>
On 12/03/2014 10:31 PM, Greg KH wrote:
> On Wed, Dec 03, 2014 at 09:07:27PM -0600, Nathan Fontenot wrote:
>> On 12/01/2014 10:26 PM, Greg KH wrote:
>>> On Mon, Dec 01, 2014 at 09:41:03AM -0600, Nathan Fontenot wrote:
>>>> On 11/26/2014 09:12 PM, Benjamin Herrenschmidt wrote:
>>>>> Hi Greg,
>>>>>
>>>>> So Nathan is working on a patch series to cleanup and improve our
>>>>> "DLPAR" infrastructure which is basically our hotplug mechanism when
>>>>> running under the PowerVM (aka pHyp) and KVM hypervisors.
>>>>
>>>> The cleanup to the dlpar infrastructure will move the entire operation
>>>> of hotplugging a device to the kernel instead of doing it partially in
>>>> userspace and partially in the kernel as is currently done.
>>>>
>>>>>
>>>>> I'll let Nathan give you a bit more details/background and answer
>>>>> subsequent question you might have as this is really his area of
>>>>> expertise.
>>>>>
>>>>> To cut a long story short, we need a sysfs file that allows our
>>>>> userspace tools to notify the kernel of hotplug events coming from
>>>>> the management console (which talks to userspace daemons using a
>>>>> proprietary protocol) to "initiate" the hotplug operations, which in
>>>>> turn get dispatched internally in the kernel to the right subsystem
>>>>> (memory, cpu, pci, ...) based on the resource type.
>>>>>
>>>>> On IRC, Greg suggested /sys/firmware and /sys/hypervisor which both
>>>>> look like a reasonable option to me, probably better than dlpar...
>>>>
>>>> For PowerVM systems we need this sysfs file to deliver what is
>>>> essentially a binary blob (specifically a rtas error log) to the
>>>> kernel. The current patch set is creating /sys/kernel/dlpar. As Ben
>>>> mentioned we would like your input on what would be the proper place
>>>> to create this file.
>>>
>>> And what is the kernel supposed to do with such a binary blob? Parse
>>> it? Or pass it to something else?
>>
>> The kernel will parse it and perform the requested hotplug operation.
>
> Oh I was hoping you would not say that :(
Heh! hint taken. I won't cc you.
>
> Seriously? Parsing binary blobs from userspace? Don't do that, you
> know better.
Yes, not ideal. One thing to note here is that the code to parse these
binary blobs already exists in the kernel for powerpc. The binary blob
we need to pass in is the same format as rtas error logs that we can be
handed by firmware on powerpc systems.
-Nathan
^ permalink raw reply
* [PATCH V2] powerpc: add little endian flag to syscall_get_arch()
From: Richard Guy Briggs @ 2014-12-08 17:58 UTC (permalink / raw)
To: linux-audit, linux-kernel, linuxppc-dev, linux-api
Cc: tonyj, Richard Guy Briggs, pmoore, anton, eparis, sgrubb
Since both ppc and ppc64 have LE variants which are now reported by uname, add
that flag (__AUDIT_ARCH_LE) to syscall_get_arch() and add AUDIT_ARCH_PPC*LE
variants.
Without this, perf trace and auditctl fail.
Mainline kernel reports ppc64le (per a058801) but there is no matching
AUDIT_ARCH_PPC64LE.
See:
https://www.redhat.com/archives/linux-audit/2014-August/msg00082.html
https://www.redhat.com/archives/linux-audit/2014-December/msg00004.html
v1 -> v2:
Added ";" at the end of the #ifdef-protected line so it actually compiles
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
arch/powerpc/include/asm/syscall.h | 6 +++++-
include/uapi/linux/audit.h | 2 ++
2 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
index 6fa2708..d1934e5 100644
--- a/arch/powerpc/include/asm/syscall.h
+++ b/arch/powerpc/include/asm/syscall.h
@@ -90,6 +90,10 @@ static inline void syscall_set_arguments(struct task_struct *task,
static inline int syscall_get_arch(void)
{
- return is_32bit_task() ? AUDIT_ARCH_PPC : AUDIT_ARCH_PPC64;
+ int arch = is_32bit_task() ? AUDIT_ARCH_PPC : AUDIT_ARCH_PPC64;
+#ifdef __LITTLE_ENDIAN__
+ arch |= __AUDIT_ARCH_LE;
+#endif
+ return arch;
}
#endif /* _ASM_SYSCALL_H */
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 4d100c8..fe29a99 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -364,7 +364,9 @@ enum {
#define AUDIT_ARCH_PARISC (EM_PARISC)
#define AUDIT_ARCH_PARISC64 (EM_PARISC|__AUDIT_ARCH_64BIT)
#define AUDIT_ARCH_PPC (EM_PPC)
+#define AUDIT_ARCH_PPCLE (EM_PPC|__AUDIT_ARCH_LE)
#define AUDIT_ARCH_PPC64 (EM_PPC64|__AUDIT_ARCH_64BIT)
+#define AUDIT_ARCH_PPC64LE (EM_PPC64|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE)
#define AUDIT_ARCH_S390 (EM_S390)
#define AUDIT_ARCH_S390X (EM_S390|__AUDIT_ARCH_64BIT)
#define AUDIT_ARCH_SH (EM_SH)
--
1.7.1
^ permalink raw reply related
* [PATCH V3] powerpc: add little endian flag to syscall_get_arch()
From: Richard Guy Briggs @ 2014-12-08 17:59 UTC (permalink / raw)
To: linux-audit, linux-kernel, linuxppc-dev, linux-api
Cc: tonyj, Richard Guy Briggs, pmoore, anton, eparis, sgrubb
Since both ppc and ppc64 have LE variants which are now reported by uname, add
that flag (__AUDIT_ARCH_LE) to syscall_get_arch() and add AUDIT_ARCH_PPC*LE
variants.
Without this, perf trace and auditctl fail.
Mainline kernel reports ppc64le (per a058801) but there is no matching
AUDIT_ARCH_PPC64LE.
Since 32-bit PPC LE is not supported, throw a compiler error rather than return
a bogus architecture to audit.
See:
https://www.redhat.com/archives/linux-audit/2014-August/msg00082.html
https://www.redhat.com/archives/linux-audit/2014-December/msg00004.html
v2 -> v3:
Throw a compiler error on 32-bit LE.
v1 -> v2:
Added ";" at the end of the #ifdef-protected line so it actually compiles
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
arch/powerpc/include/asm/syscall.h | 7 +++++++
include/uapi/linux/audit.h | 1 +
2 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
index 6fa2708..cf7fcab 100644
--- a/arch/powerpc/include/asm/syscall.h
+++ b/arch/powerpc/include/asm/syscall.h
@@ -90,6 +90,13 @@ static inline void syscall_set_arguments(struct task_struct *task,
static inline int syscall_get_arch(void)
{
+#ifdef __LITTLE_ENDIAN__
+ return AUDIT_ARCH_PPC64LE;
+#ifndef CONFIG_64BIT
+#error PPC 32-bit Little Endian architecture not supported.
+#endif /* CONFIG_64BIT */
+#else /* __LITTLE_ENDIAN__ */
return is_32bit_task() ? AUDIT_ARCH_PPC : AUDIT_ARCH_PPC64;
+#endif /* __LITTLE_ENDIAN__ */
}
#endif /* _ASM_SYSCALL_H */
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 4d100c8..fa2a6af 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -365,6 +365,7 @@ enum {
#define AUDIT_ARCH_PARISC64 (EM_PARISC|__AUDIT_ARCH_64BIT)
#define AUDIT_ARCH_PPC (EM_PPC)
#define AUDIT_ARCH_PPC64 (EM_PPC64|__AUDIT_ARCH_64BIT)
+#define AUDIT_ARCH_PPC64LE (EM_PPC64|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE)
#define AUDIT_ARCH_S390 (EM_S390)
#define AUDIT_ARCH_S390X (EM_S390|__AUDIT_ARCH_64BIT)
#define AUDIT_ARCH_SH (EM_SH)
--
1.7.1
^ permalink raw reply related
* Re: [PATCH V3] powerpc: add little endian flag to syscall_get_arch()
From: Paul Moore @ 2014-12-08 19:16 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: tonyj, linux-api, linux-kernel, linux-audit, anton, eparis,
sgrubb, linuxppc-dev
In-Reply-To: <c1adfab2633f57a745f3e2e1b13b5a8f283c888a.1418055157.git.rgb@redhat.com>
On Monday, December 08, 2014 12:59:32 PM Richard Guy Briggs wrote:
> Since both ppc and ppc64 have LE variants which are now reported by uname,
> add that flag (__AUDIT_ARCH_LE) to syscall_get_arch() and add
> AUDIT_ARCH_PPC*LE variants.
>
> Without this, perf trace and auditctl fail.
>
> Mainline kernel reports ppc64le (per a058801) but there is no matching
> AUDIT_ARCH_PPC64LE.
>
> Since 32-bit PPC LE is not supported, throw a compiler error rather than
> return a bogus architecture to audit.
>
> See:
> https://www.redhat.com/archives/linux-audit/2014-August/msg00082.html
> https://www.redhat.com/archives/linux-audit/2014-December/msg00004.html
>
> v2 -> v3:
> Throw a compiler error on 32-bit LE.
>
> v1 -> v2:
> Added ";" at the end of the #ifdef-protected line so it actually compiles
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> arch/powerpc/include/asm/syscall.h | 7 +++++++
> include/uapi/linux/audit.h | 1 +
> 2 files changed, 8 insertions(+), 0 deletions(-)
Looks reasonable to me from an audit perspective, but I'll let the ppc folks
merge this patch into their tree.
Acked-by: Paul Moore <paul@paul-moore.com>
> diff --git a/arch/powerpc/include/asm/syscall.h
> b/arch/powerpc/include/asm/syscall.h index 6fa2708..cf7fcab 100644
> --- a/arch/powerpc/include/asm/syscall.h
> +++ b/arch/powerpc/include/asm/syscall.h
> @@ -90,6 +90,13 @@ static inline void syscall_set_arguments(struct
> task_struct *task,
>
> static inline int syscall_get_arch(void)
> {
> +#ifdef __LITTLE_ENDIAN__
> + return AUDIT_ARCH_PPC64LE;
> +#ifndef CONFIG_64BIT
> +#error PPC 32-bit Little Endian architecture not supported.
> +#endif /* CONFIG_64BIT */
> +#else /* __LITTLE_ENDIAN__ */
> return is_32bit_task() ? AUDIT_ARCH_PPC : AUDIT_ARCH_PPC64;
> +#endif /* __LITTLE_ENDIAN__ */
> }
> #endif /* _ASM_SYSCALL_H */
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 4d100c8..fa2a6af 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -365,6 +365,7 @@ enum {
> #define AUDIT_ARCH_PARISC64 (EM_PARISC|__AUDIT_ARCH_64BIT)
> #define AUDIT_ARCH_PPC (EM_PPC)
> #define AUDIT_ARCH_PPC64 (EM_PPC64|__AUDIT_ARCH_64BIT)
> +#define AUDIT_ARCH_PPC64LE (EM_PPC64|__AUDIT_ARCH_64BIT|__AUDIT_ARCH_LE)
> #define AUDIT_ARCH_S390 (EM_S390)
> #define AUDIT_ARCH_S390X (EM_S390|__AUDIT_ARCH_64BIT)
> #define AUDIT_ARCH_SH (EM_SH)
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: Right location in sysfs for dlpar file
From: Benjamin Herrenschmidt @ 2014-12-08 20:08 UTC (permalink / raw)
To: Greg KH, Nathan Fontenot; +Cc: linuxppc-dev
In-Reply-To: <5485C1F9.9000401@linux.vnet.ibm.com>
On Mon, 2014-12-08 at 09:21 -0600, Nathan Fontenot wrote:
> > Oh I was hoping you would not say that :(
>
> Heh! hint taken. I won't cc you.
>
> >
> > Seriously? Parsing binary blobs from userspace? Don't do that, you
> > know better.
>
> Yes, not ideal. One thing to note here is that the code to parse these
> binary blobs already exists in the kernel for powerpc. The binary blob
> we need to pass in is the same format as rtas error logs that we can be
> handed by firmware on powerpc systems.
We should clarify that it's a small blob, it's an error log entry as defined
by our firmware interfaces. We already obtain these from the FW and parse them
(it's really just mapping the right structure based on the type in the header)
under various circumstances.
What happens is on KVM, we used that existing path to signal the kernel of an
hotplug event. On PowerVM (aka pHyp), we want to use the same kernel code path
by passing an equivalent event to the kernel.
Nathan, if Greg still strongly objects, we could have the sysfs interface be
something like a simple action + resource ID string, we don't *need* it to
be encapsulated in an error log, we would just pipe into the same infrastructure
in the kernel but at a slightly lower level...
Ben.
^ permalink raw reply
* Re: [PATCH v3] i2c: Driver to expose PowerNV platform i2c busses
From: Benjamin Herrenschmidt @ 2014-12-08 20:13 UTC (permalink / raw)
To: Neelesh Gupta; +Cc: linuxppc-dev, wsa, linux-i2c, devicetree
In-Reply-To: <20141208063039.14356.34770.stgit@localhost.localdomain>
On Mon, 2014-12-08 at 12:06 +0530, Neelesh Gupta wrote:
> The patch exposes the available i2c busses on the PowerNV platform
> to the kernel and implements the bus driver to support i2c and
> smbus commands.
> The driver uses the platform device infrastructure to probe the busses
> on the platform and registers them with the i2c driver framework.
Wolfram, what are you remaining objections here ? We need that in
distros ASAP ...
I still maintain that it's not reasonable to hold driver for the
additions of multi-byte smbus offsets. This is a new feature that will
require changes to a number of existing bus and device drivers, so a
very pervasive change, and which will be visible to user space, which
means that drivers will need to continue supporting the "old" way at
least for a while anyway...
Ben.
> Signed-off-by: Neelesh Gupta <neelegup@linux.vnet.ibm.com>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>
> v2 -> v3:
> - Added the device tree binding documentation for the driver.
> - Sorted the ordering of this new driver added in Makefile.
> - Removed populating the superfluous .owner field in 'struct driver'.
>
> Documentation/devicetree/bindings/i2c/i2c-opal.txt | 37 +++
> arch/powerpc/include/asm/opal.h | 29 ++
> arch/powerpc/platforms/powernv/opal-wrappers.S | 1
> arch/powerpc/platforms/powernv/opal.c | 11 +
> drivers/i2c/busses/Kconfig | 11 +
> drivers/i2c/busses/Makefile | 1
> drivers/i2c/busses/i2c-opal.c | 294 ++++++++++++++++++++
> 7 files changed, 384 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-opal.txt
> create mode 100644 drivers/i2c/busses/i2c-opal.c
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-opal.txt b/Documentation/devicetree/bindings/i2c/i2c-opal.txt
> new file mode 100644
> index 0000000..12bc614
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-opal.txt
> @@ -0,0 +1,37 @@
> +Device-tree bindings for I2C OPAL driver
> +----------------------------------------
> +
> +Most of the device node and properties layout is specific to the firmware and
> +used by the firmware itself for configuring the port. From the linux
> +perspective, the properties of use are "ibm,port-name" and "ibm,opal-id".
> +
> +Required properties:
> +
> +- reg: Port-id within a given master
> +- compatible: must be "ibm,opal-i2c"
> +- ibm,opal-id: Refers to a specific bus and used to identify it when calling
> + the relevant OPAL functions.
> +- bus-frequency: Operating frequency of the i2c bus (in HZ). Informational for
> + linux, used by the FW though.
> +
> +Optional properties:
> +- ibm,port-name: Firmware provides this name that uniquely identifies the i2c
> + port.
> +
> +The node contains a number of other properties that are used by the FW itself
> +and depend on the specific hardware implementation. The example below depicts
> +a P8 on-chip bus.
> +
> +Example:
> +
> +i2c-bus@0 {
> + reg = <0x0>;
> + bus-frequency = <0x61a80>;
> + compatible = "ibm,power8-i2c-port", "ibm,opal-i2c";
> + ibm,opal-id = <0x1>;
> + ibm,port-name = "p8_00000000_e1p0";
> + #address-cells = <0x1>;
> + phandle = <0x10000006>;
> + #size-cells = <0x0>;
> + linux,phandle = <0x10000006>;
> +};
> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> index 9124b0e..537807b 100644
> --- a/arch/powerpc/include/asm/opal.h
> +++ b/arch/powerpc/include/asm/opal.h
> @@ -56,6 +56,14 @@ struct opal_sg_list {
> #define OPAL_HARDWARE_FROZEN -13
> #define OPAL_WRONG_STATE -14
> #define OPAL_ASYNC_COMPLETION -15
> +#define OPAL_I2C_TIMEOUT -17
> +#define OPAL_I2C_INVALID_CMD -18
> +#define OPAL_I2C_LBUS_PARITY -19
> +#define OPAL_I2C_BKEND_OVERRUN -20
> +#define OPAL_I2C_BKEND_ACCESS -21
> +#define OPAL_I2C_ARBT_LOST -22
> +#define OPAL_I2C_NACK_RCVD -23
> +#define OPAL_I2C_STOP_ERR -24
>
> /* API Tokens (in r0) */
> #define OPAL_INVALID_CALL -1
> @@ -154,6 +162,7 @@ struct opal_sg_list {
> #define OPAL_HANDLE_HMI 98
> #define OPAL_REGISTER_DUMP_REGION 101
> #define OPAL_UNREGISTER_DUMP_REGION 102
> +#define OPAL_I2C_REQUEST 109
>
> #ifndef __ASSEMBLY__
>
> @@ -801,6 +810,24 @@ typedef struct oppanel_line {
> uint64_t line_len;
> } oppanel_line_t;
>
> +/* OPAL I2C request */
> +struct opal_i2c_request {
> + uint8_t type;
> +#define OPAL_I2C_RAW_READ 0
> +#define OPAL_I2C_RAW_WRITE 1
> +#define OPAL_I2C_SM_READ 2
> +#define OPAL_I2C_SM_WRITE 3
> + uint8_t flags;
> +#define OPAL_I2C_ADDR_10 0x01 /* Not supported yet */
> + uint8_t subaddr_sz; /* Max 4 */
> + uint8_t reserved;
> + __be16 addr; /* 7 or 10 bit address */
> + __be16 reserved2;
> + __be32 subaddr; /* Sub-address if any */
> + __be32 size; /* Data size */
> + __be64 buffer_ra; /* Buffer real address */
> +};
> +
> /* /sys/firmware/opal */
> extern struct kobject *opal_kobj;
>
> @@ -963,6 +990,8 @@ int64_t opal_handle_hmi(void);
> int64_t opal_register_dump_region(uint32_t id, uint64_t start, uint64_t end);
> int64_t opal_unregister_dump_region(uint32_t id);
> int64_t opal_pci_set_phb_cxl_mode(uint64_t phb_id, uint64_t mode, uint64_t pe_number);
> +int64_t opal_i2c_request(uint64_t async_token, uint32_t bus_id,
> + struct opal_i2c_request *oreq);
>
> /* Internal functions */
> extern int early_init_dt_scan_opal(unsigned long node, const char *uname,
> diff --git a/arch/powerpc/platforms/powernv/opal-wrappers.S b/arch/powerpc/platforms/powernv/opal-wrappers.S
> index feb549a..4673c02 100644
> --- a/arch/powerpc/platforms/powernv/opal-wrappers.S
> +++ b/arch/powerpc/platforms/powernv/opal-wrappers.S
> @@ -250,3 +250,4 @@ OPAL_CALL(opal_handle_hmi, OPAL_HANDLE_HMI);
> OPAL_CALL(opal_register_dump_region, OPAL_REGISTER_DUMP_REGION);
> OPAL_CALL(opal_unregister_dump_region, OPAL_UNREGISTER_DUMP_REGION);
> OPAL_CALL(opal_pci_set_phb_cxl_mode, OPAL_PCI_SET_PHB_CXL_MODE);
> +OPAL_CALL(opal_i2c_request, OPAL_I2C_REQUEST);
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index bb90e6e..994975e 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -667,6 +667,14 @@ static void opal_console_create_devs(void)
>
> }
>
> +static void opal_i2c_create_devs(void)
> +{
> + struct device_node *np;
> +
> + for_each_compatible_node(np, NULL, "ibm,opal-i2c")
> + of_platform_device_create(np, NULL, NULL);
> +}
> +
> static void opal_request_interrupts(void)
> {
> const __be32 *irqs;
> @@ -732,6 +740,9 @@ static int __init opal_init(void)
> /* Create console platform devices */
> opal_console_create_devs();
>
> + /* Create i2c platform devices */
> + opal_i2c_create_devs();
> +
> /* Register OPAL interrupts */
> opal_request_interrupts();
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 917c358..71ad6e1 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1044,4 +1044,15 @@ config SCx200_ACB
> This support is also available as a module. If so, the module
> will be called scx200_acb.
>
> +config I2C_OPAL
> + tristate "IBM OPAL I2C driver"
> + depends on PPC_POWERNV
> + default y
> + help
> + This exposes the PowerNV platform i2c busses to the linux i2c layer,
> + the driver is based on the OPAL interfaces.
> +
> + This driver can also be built as a module. If so, the module will be
> + called as i2c-opal.
> +
> endmenu
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 78d56c5..e23ec81 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -99,6 +99,7 @@ obj-$(CONFIG_I2C_ACORN) += i2c-acorn.o
> obj-$(CONFIG_I2C_BCM_KONA) += i2c-bcm-kona.o
> obj-$(CONFIG_I2C_CROS_EC_TUNNEL) += i2c-cros-ec-tunnel.o
> obj-$(CONFIG_I2C_ELEKTOR) += i2c-elektor.o
> +obj-$(CONFIG_I2C_OPAL) += i2c-opal.o
> obj-$(CONFIG_I2C_PCA_ISA) += i2c-pca-isa.o
> obj-$(CONFIG_I2C_SIBYTE) += i2c-sibyte.o
> obj-$(CONFIG_SCx200_ACB) += scx200_acb.o
> diff --git a/drivers/i2c/busses/i2c-opal.c b/drivers/i2c/busses/i2c-opal.c
> new file mode 100644
> index 0000000..16f90b1
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-opal.c
> @@ -0,0 +1,294 @@
> +/*
> + * IBM OPAL I2C driver
> + * Copyright (C) 2014 IBM
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <asm/firmware.h>
> +#include <asm/opal.h>
> +
> +static int i2c_opal_translate_error(int rc)
> +{
> + switch (rc) {
> + case OPAL_NO_MEM:
> + return -ENOMEM;
> + case OPAL_PARAMETER:
> + return -EINVAL;
> + case OPAL_I2C_ARBT_LOST:
> + return -EAGAIN;
> + case OPAL_I2C_TIMEOUT:
> + return -ETIMEDOUT;
> + case OPAL_I2C_NACK_RCVD:
> + return -ENXIO;
> + case OPAL_I2C_STOP_ERR:
> + return -EBUSY;
> + default:
> + return -EIO;
> + }
> +}
> +
> +static int i2c_opal_send_request(u32 bus_id, struct opal_i2c_request *req)
> +{
> + struct opal_msg msg;
> + int token, rc;
> +
> + token = opal_async_get_token_interruptible();
> + if (token < 0) {
> + if (token != -ERESTARTSYS)
> + pr_err("Failed to get the async token\n");
> +
> + return token;
> + }
> +
> + rc = opal_i2c_request(token, bus_id, req);
> + if (rc != OPAL_ASYNC_COMPLETION) {
> + rc = i2c_opal_translate_error(rc);
> + goto exit;
> + }
> +
> + rc = opal_async_wait_response(token, &msg);
> + if (rc)
> + goto exit;
> +
> + rc = be64_to_cpu(msg.params[1]);
> + if (rc != OPAL_SUCCESS) {
> + rc = i2c_opal_translate_error(rc);
> + goto exit;
> + }
> +
> +exit:
> + opal_async_release_token(token);
> + return rc;
> +}
> +
> +static int i2c_opal_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
> + int num)
> +{
> + unsigned long opal_id = (unsigned long)adap->algo_data;
> + struct opal_i2c_request req;
> + int rc, i;
> +
> + /* We only support fairly simple combinations here of one
> + * or two messages
> + */
> + memset(&req, 0, sizeof(req));
> + switch(num) {
> + case 0:
> + return 0;
> + case 1:
> + req.type = (msgs[0].flags & I2C_M_RD) ?
> + OPAL_I2C_RAW_READ : OPAL_I2C_RAW_WRITE;
> + req.addr = cpu_to_be16(msgs[0].addr);
> + req.size = cpu_to_be32(msgs[0].len);
> + req.buffer_ra = cpu_to_be64(__pa(msgs[0].buf));
> + break;
> + case 2:
> + /* For two messages, we basically support only simple
> + * smbus transactions of a write plus a read. We might
> + * want to allow also two writes but we'd have to bounce
> + * the data into a single buffer.
> + */
> + if ((msgs[0].flags & I2C_M_RD) || !(msgs[1].flags & I2C_M_RD))
> + return -EOPNOTSUPP;
> + if (msgs[0].len > 4)
> + return -EOPNOTSUPP;
> + if (msgs[0].addr != msgs[1].addr)
> + return -EOPNOTSUPP;
> + req.type = OPAL_I2C_SM_READ;
> + req.addr = cpu_to_be16(msgs[0].addr);
> + req.subaddr_sz = msgs[0].len;
> + for (i = 0; i < msgs[0].len; i++)
> + req.subaddr = (req.subaddr << 8) | msgs[0].buf[i];
> + req.subaddr = cpu_to_be32(req.subaddr);
> + req.size = cpu_to_be32(msgs[1].len);
> + req.buffer_ra = cpu_to_be64(__pa(msgs[1].buf));
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + rc = i2c_opal_send_request(opal_id, &req);
> + if (rc)
> + return rc;
> +
> + return num;
> +}
> +
> +static int i2c_opal_smbus_xfer(struct i2c_adapter *adap, u16 addr,
> + unsigned short flags, char read_write,
> + u8 command, int size, union i2c_smbus_data *data)
> +{
> + unsigned long opal_id = (unsigned long)adap->algo_data;
> + struct opal_i2c_request req;
> + u8 local[2];
> + int rc;
> +
> + memset(&req, 0, sizeof(req));
> +
> + req.addr = cpu_to_be16(addr);
> + switch (size) {
> + case I2C_SMBUS_BYTE:
> + req.buffer_ra = cpu_to_be64(__pa(&data->byte));
> + req.size = cpu_to_be32(1);
> + /* Fall through */
> + case I2C_SMBUS_QUICK:
> + req.type = (read_write == I2C_SMBUS_READ) ?
> + OPAL_I2C_RAW_READ : OPAL_I2C_RAW_WRITE;
> + break;
> + case I2C_SMBUS_BYTE_DATA:
> + req.buffer_ra = cpu_to_be64(__pa(&data->byte));
> + req.size = cpu_to_be32(1);
> + req.subaddr = cpu_to_be32(command);
> + req.subaddr_sz = 1;
> + req.type = (read_write == I2C_SMBUS_READ) ?
> + OPAL_I2C_SM_READ : OPAL_I2C_SM_WRITE;
> + break;
> + case I2C_SMBUS_WORD_DATA:
> + if (!read_write) {
> + local[0] = data->word & 0xff;
> + local[1] = (data->word >> 8) & 0xff;
> + }
> + req.buffer_ra = cpu_to_be64(__pa(local));
> + req.size = cpu_to_be32(2);
> + req.subaddr = cpu_to_be32(command);
> + req.subaddr_sz = 1;
> + req.type = (read_write == I2C_SMBUS_READ) ?
> + OPAL_I2C_SM_READ : OPAL_I2C_SM_WRITE;
> + break;
> + case I2C_SMBUS_I2C_BLOCK_DATA:
> + req.buffer_ra = cpu_to_be64(__pa(&data->block[1]));
> + req.size = cpu_to_be32(data->block[0]);
> + req.subaddr = cpu_to_be32(command);
> + req.subaddr_sz = 1;
> + req.type = (read_write == I2C_SMBUS_READ) ?
> + OPAL_I2C_SM_READ : OPAL_I2C_SM_WRITE;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + rc = i2c_opal_send_request(opal_id, &req);
> + if (!rc && read_write && size == I2C_SMBUS_WORD_DATA) {
> + data->word = ((u16)local[1]) << 8;
> + data->word |= local[0];
> + }
> +
> + return rc;
> +}
> +
> +static u32 i2c_opal_func(struct i2c_adapter *adapter)
> +{
> + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
> + I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
> + I2C_FUNC_SMBUS_I2C_BLOCK;
> +}
> +
> +static const struct i2c_algorithm i2c_opal_algo = {
> + .master_xfer = i2c_opal_master_xfer,
> + .smbus_xfer = i2c_opal_smbus_xfer,
> + .functionality = i2c_opal_func,
> +};
> +
> +static int i2c_opal_probe(struct platform_device *pdev)
> +{
> + struct i2c_adapter *adapter;
> + const char *pname;
> + u32 opal_id;
> + int rc;
> +
> + if (!pdev->dev.of_node)
> + return -ENODEV;
> +
> + rc = of_property_read_u32(pdev->dev.of_node, "ibm,opal-id", &opal_id);
> + if (rc) {
> + dev_err(&pdev->dev, "Missing ibm,opal-id property !\n");
> + return -EIO;
> + }
> +
> + adapter = devm_kzalloc(&pdev->dev, sizeof(*adapter), GFP_KERNEL);
> + if (!adapter)
> + return -ENOMEM;
> +
> + adapter->algo = &i2c_opal_algo;
> + adapter->algo_data = (void *)(unsigned long)opal_id;
> + adapter->dev.parent = &pdev->dev;
> + adapter->dev.of_node = of_node_get(pdev->dev.of_node);
> + pname = of_get_property(pdev->dev.of_node, "ibm,port-name", NULL);
> + if (pname)
> + strlcpy(adapter->name, pname, sizeof(adapter->name));
> + else
> + strlcpy(adapter->name, "opal", sizeof(adapter->name));
> +
> + platform_set_drvdata(pdev, adapter);
> + rc = i2c_add_adapter(adapter);
> + if (rc)
> + dev_err(&pdev->dev, "Failed to register the i2c adapter\n");
> +
> + return rc;
> +}
> +
> +static int i2c_opal_remove(struct platform_device *pdev)
> +{
> + struct i2c_adapter *adapter = platform_get_drvdata(pdev);
> +
> + i2c_del_adapter(adapter);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id i2c_opal_of_match[] = {
> + {
> + .compatible = "ibm,opal-i2c",
> + },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, i2c_opal_of_match);
> +
> +static struct platform_driver i2c_opal_driver = {
> + .probe = i2c_opal_probe,
> + .remove = i2c_opal_remove,
> + .driver = {
> + .name = "i2c-opal",
> + .of_match_table = i2c_opal_of_match,
> + },
> +};
> +
> +static int __init i2c_opal_init(void)
> +{
> + if (!firmware_has_feature(FW_FEATURE_OPAL))
> + return -ENODEV;
> +
> + return platform_driver_register(&i2c_opal_driver);
> +}
> +module_init(i2c_opal_init);
> +
> +static void __exit i2c_opal_exit(void)
> +{
> + return platform_driver_unregister(&i2c_opal_driver);
> +}
> +module_exit(i2c_opal_exit);
> +
> +MODULE_AUTHOR("Neelesh Gupta <neelegup@linux.vnet.ibm.com>");
> +MODULE_DESCRIPTION("IBM OPAL I2C driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply
* Re: [PATCH v3] i2c: Driver to expose PowerNV platform i2c busses
From: Wolfram Sang @ 2014-12-08 20:55 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Neelesh Gupta, linuxppc-dev, linux-i2c, devicetree
In-Reply-To: <1418069595.4827.43.camel@kernel.crashing.org>
[-- Attachment #1: Type: text/plain, Size: 1556 bytes --]
On Tue, Dec 09, 2014 at 07:13:15AM +1100, Benjamin Herrenschmidt wrote:
> On Mon, 2014-12-08 at 12:06 +0530, Neelesh Gupta wrote:
> > The patch exposes the available i2c busses on the PowerNV platform
> > to the kernel and implements the bus driver to support i2c and
> > smbus commands.
> > The driver uses the platform device infrastructure to probe the busses
> > on the platform and registers them with the i2c driver framework.
>
> Wolfram, what are you remaining objections here ? We need that in
> distros ASAP ...
Oh, I thought we agreed that you take it via powerpc. I still think this
is the best solution.
> I still maintain that it's not reasonable to hold driver for the
> additions of multi-byte smbus offsets. This is a new feature that will
> require changes to a number of existing bus and device drivers, so a
> very pervasive change, and which will be visible to user space, which
> means that drivers will need to continue supporting the "old" way at
> least for a while anyway...
Yeah, I agree on that. I am still unsure about the port-name binding,
but well, if it is needed to fit your PowerNV scheme...
> > Signed-off-by: Neelesh Gupta <neelegup@linux.vnet.ibm.com>
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> >
> > v2 -> v3:
> > - Added the device tree binding documentation for the driver.
> > - Sorted the ordering of this new driver added in Makefile.
> > - Removed populating the superfluous .owner field in 'struct driver'.
Thanks for the updates!
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v3 4/4] powernv: powerpc: Add winkle support for offline cpus
From: Shreyas B Prabhu @ 2014-12-08 21:54 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20141208055217.GC4437@drongo>
On Monday 08 December 2014 11:22 AM, Paul Mackerras wrote:
> On Thu, Dec 04, 2014 at 12:58:23PM +0530, Shreyas B. Prabhu wrote:
>> Winkle is a deep idle state supported in power8 chips. A core enters
>> winkle when all the threads of the core enter winkle. In this state
>> power supply to the entire chiplet i.e core, private L2 and private L3
>> is turned off. As a result it gives higher powersavings compared to
>> sleep.
>>
>> But entering winkle results in a total hypervisor state loss. Hence the
>> hypervisor context has to be preserved before entering winkle and
>> restored upon wake up.
>>
>> Power-on Reset Engine (PORE) is a dedicated engine which is responsible
>> for powering on the chiplet during wake up. It can be programmed to
>> restore the register contests of a few specific registers. This patch
>> uses PORE to restore register state wherever possible and uses stack to
>> save and restore rest of the necessary registers.
>>
>> With hypervisor state restore things fall under three categories-
>> per-core state, per-subcore state and per-thread state. To manage this,
>> extend the infrastructure introduced for sleep. Mainly we add a paca
>> variable subcore_sibling_mask. Using this and the core_idle_state we can
>> distingush first thread in core and subcore.
>
> Comments below...
>
>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
>> index 7637889..2b9b5fb 100644
>> --- a/arch/powerpc/kernel/exceptions-64s.S
>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>> @@ -102,9 +102,7 @@ system_reset_pSeries:
>> #ifdef CONFIG_PPC_P7_NAP
>> BEGIN_FTR_SECTION
>> /* Running native on arch 2.06 or later, check if we are
>> - * waking up from nap. We only handle no state loss and
>> - * supervisor state loss. We do -not- handle hypervisor
>> - * state loss at this time.
>> + * waking up from nap/sleep/winkle.
>> */
>> mfspr r13,SPRN_SRR1
>> rlwinm. r13,r13,47-31,30,31
>> @@ -112,7 +110,17 @@ BEGIN_FTR_SECTION
>>
>> cmpwi cr3,r13,2
>>
>> - GET_PACA(r13)
>> + /* Check if last bit of HSPGR0 is set. This indicates whether we are
>> + * waking up from winkle */
>> + li r3,1
>> + mfspr r4,SPRN_HSPRG0
>> + and r5,r4,r3
>> + cmpwi cr4,r5,1 /* Store result in cr4 for later use */
>> +
>> + andc r4,r4,r3
>> + mtspr SPRN_HSPRG0,r4
>> +
>> + mr r13,r4
>
> This seems unnecessarily convoluted. How about:
>
> GET_PACA(r13)
> clrldi r5,r13,63
> clrrdi r13,r13,1
> cmpwi cr4,r5,1
> mtspr SPRN_HSPRG0,r13
>
Yes, makes more sense. I'll use this.
>> diff --git a/arch/powerpc/kernel/idle_power7.S b/arch/powerpc/kernel/idle_power7.S
>> index 8c3a1f4..8102075 100644
>> --- a/arch/powerpc/kernel/idle_power7.S
>> +++ b/arch/powerpc/kernel/idle_power7.S
>> @@ -19,8 +19,24 @@
>> #include <asm/kvm_book3s_asm.h>
>> #include <asm/opal.h>
>> #include <asm/cpuidle.h>
>> +#include <asm/mmu-hash64.h>
>>
>> #undef DEBUG
>> +/*
>> + * Use unused space in the interrupt stack to save and restore
>> + * registers for winkle support.
>> + */
>> +#define _SDR1 GPR3
>> +#define _RPR GPR4
>> +#define _SPURR GPR5
>> +#define _PURR GPR6
>> +#define _TSCR GPR7
>> +#define _DSCR GPR8
>> +#define _AMOR GPR9
>> +#define _PMC5 GPR10
>> +#define _PMC6 GPR11
>
> Why only PMC5 and PMC6 out of all the PMU registers? What about
> PMC1-PMC4 and the MMCR registers? I assume they're lost during winkle
> state also, aren't they? If we're not saving them, what's the point
> of saving and restoring PMC5 and PMC6?
>
Yes all PMC and MMCR contents are lost. Using __restore_cpu_power8, the
MMCR registers are initialized to 0. The reasoning behind specifically
restoring PMC5 and PMC6 was the fact that they are not programmable and
count cycles/instructions by default. We suspected that there might be a
userspace program which relied on PMC5/PMC6 always increasing.
But now on closer look, since these counters are 32 bit and cycles/
instruction counts are bound to exceed it, I doubt such userspace programs
exist. I'll drop PMC5 and PMC6 in the next version.
>> +#define _WORT GPR12
>> +#define _WORC GPR13
>>
>> /* Idle state entry routines */
>>
>> @@ -124,8 +140,8 @@ power7_enter_nap_mode:
>> stb r4,HSTATE_HWTHREAD_STATE(r13)
>> #endif
>> stb r3,PACA_THREAD_IDLE_STATE(r13)
>> - cmpwi cr1,r3,PNV_THREAD_SLEEP
>> - bge cr1,2f
>> + cmpwi cr3,r3,PNV_THREAD_SLEEP
>> + bge cr3,2f
>> IDLE_STATE_ENTER_SEQ(PPC_NAP)
>> /* No return */
>> 2:
>> @@ -154,7 +170,8 @@ pnv_fastsleep_workaround_at_entry:
>> isync
>> bne- lwarx_loop1
>>
>> -common_enter: /* common code for all the threads entering sleep */
>> +common_enter: /* common code for all the threads entering sleep or winkle */
>> + bgt cr3,enter_winkle
>> IDLE_STATE_ENTER_SEQ(PPC_SLEEP)
>>
>> fastsleep_workaround_at_entry:
>> @@ -175,6 +192,34 @@ fastsleep_workaround_at_entry:
>> stw r0,0(r14)
>> b common_enter
>>
>> +enter_winkle:
>> + /*
>> + * Note all register i.e per-core, per-subcore or per-thread is saved
>> + * here since any thread in the core might wake up first
>> + */
>> + mfspr r3,SPRN_SDR1
>> + std r3,_SDR1(r1)
>> + mfspr r3,SPRN_RPR
>> + std r3,_RPR(r1)
>> + mfspr r3,SPRN_SPURR
>> + std r3,_SPURR(r1)
>> + mfspr r3,SPRN_PURR
>> + std r3,_PURR(r1)
>> + mfspr r3,SPRN_TSCR
>> + std r3,_TSCR(r1)
>> + mfspr r3,SPRN_DSCR
>> + std r3,_DSCR(r1)
>> + mfspr r3,SPRN_AMOR
>> + std r3,_AMOR(r1)
>> + mfspr r3,SPRN_PMC5
>> + std r3,_PMC5(r1)
>> + mfspr r3,SPRN_PMC6
>> + std r3,_PMC6(r1)
>> + mfspr r3,SPRN_WORT
>> + std r3,_WORT(r1)
>> + mfspr r3,SPRN_WORC
>> + std r3,_WORC(r1)
>> + IDLE_STATE_ENTER_SEQ(PPC_WINKLE)
>>
>> _GLOBAL(power7_idle)
>> /* Now check if user or arch enabled NAP mode */
>> @@ -197,6 +242,12 @@ _GLOBAL(power7_sleep)
>> b power7_powersave_common
>> /* No return */
>>
>> +_GLOBAL(power7_winkle)
>> + li r3,3
>> + li r4,1
>> + b power7_powersave_common
>> + /* No return */
>> +
>> #define CHECK_HMI_INTERRUPT \
>> mfspr r0,SPRN_SRR1; \
>> BEGIN_FTR_SECTION_NESTED(66); \
>> @@ -238,11 +289,23 @@ lwarx_loop2:
>> bne core_idle_lock_held
>>
>> cmpwi cr2,r15,0
>> + lbz r4,PACA_SUBCORE_SIBLING_MASK(r13)
>> + and r4,r4,r15
>> + cmpwi cr1,r4,0 /* Check if first in subcore */
>> +
>> + /*
>> + * At this stage
>> + * cr1 - 10 if first thread to wakeup in subcore
>> + * cr2 - 10 if first thread to wakeup in core
>> + * cr3- 01 if waking up from sleep or winkle
>> + * cr4 - 10 if waking up from winkle
>> + */
>
> What do "10" and "01" mean in this comment? (If they were CR field
> values in binary they would need to be 3 or 4 bits, not 2.)
>
I'll fix this.
Thanks,
Shreyas
^ permalink raw reply
* Re: [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode at boot time
From: Scott Wood @ 2014-12-08 22:39 UTC (permalink / raw)
To: Greg Kurz; +Cc: Michael Ellerman, linuxppc-dev
In-Reply-To: <20141208092321.673a0be3@bahia.local>
On Mon, 2014-12-08 at 09:23 +0100, Greg Kurz wrote:
> On Fri, 5 Dec 2014 12:52:45 -0600
> Scott Wood <scottwood@freescale.com> wrote:
>
> > On Fri, 2014-12-05 at 16:14 +0100, Greg Kurz wrote:
> > > The smt-enabled kernel parameter basically leaves unwanted cpus executing
> > > in firmware or wherever they happen to be. The very same applies to the
> > > ibm,smt-enabled DT property which is no more used by anything known. These
> > > are hacks that shoudn't be used in a production environment.
> > >
> > > Quoting mpe, "there are better ways for firmware to disable SMT".
> >
>
> Hi Scott,
>
> > Those "better ways" don't apply to Freescale chips, where the OS enables
> > (or not) SMT without any interaction with firmware. I don't care about
> > the ibm,smt-enabled property, but can we please keep the smt-enabled
> > boot option?
> >
>
> Fair enough for the firmware side, what about CPU hot(un)plug then ?
Not yet supported in mainline for e6500 (or maybe it works with
generic_mach_cpu_die which would not be helpful).
Plus, it's more complicated (both to use and how it works internally)
and doesn't avoid having the secondary thread ever run. Sometimes it's
useful to ensure that the second thread has never run when debugging a
problem.
> > > It also has an evil side effect on the split-core feature for powernv. The
> > > code needs all the cpus to participate to the split mode update: it relies
> > > on smp_send_reschedule() to get offline ones to do so. This doesn't work with
> > > cpus that haven't come up... The consequence is a kernel hang on powernv when
> > > trying to limit the number of hw threads at boot time (e.g. smt-enabled to
> > > anything but 8 on POWER8).
> >
> > In that case could you disable the option only on that hardware?
> >
>
> The fact it breaks only powernv doesn't mean it is a powernv only issue.
> The smt-enabled feature is a hack because it leaves some cpus in a undefined
> state from a kernel POV.
I'm aware of an issue where per-cpu threads get created for these CPUs,
which seems like a bug if they were never marked online (it's on my todo
list to investigate further). Are there other issues? It seems like
there ought to be some way to do this right.
> Moreover it drags about 80 lines of code and sits entirely in common
> ppc64 code. I would reverse the question then ? Why not moving
> smt-enabled code to freescale only ?
I'm fine with making it Freescale-only.
-Scott
^ permalink raw reply
* Re: [PATCH v3] i2c: Driver to expose PowerNV platform i2c busses
From: Benjamin Herrenschmidt @ 2014-12-08 21:43 UTC (permalink / raw)
To: Wolfram Sang; +Cc: Neelesh Gupta, linuxppc-dev, linux-i2c, devicetree
In-Reply-To: <20141208205520.GA12715@katana>
On Mon, 2014-12-08 at 21:55 +0100, Wolfram Sang wrote:
> On Tue, Dec 09, 2014 at 07:13:15AM +1100, Benjamin Herrenschmidt wrote:
> > On Mon, 2014-12-08 at 12:06 +0530, Neelesh Gupta wrote:
> > > The patch exposes the available i2c busses on the PowerNV platform
> > > to the kernel and implements the bus driver to support i2c and
> > > smbus commands.
> > > The driver uses the platform device infrastructure to probe the busses
> > > on the platform and registers them with the i2c driver framework.
> >
> > Wolfram, what are you remaining objections here ? We need that in
> > distros ASAP ...
>
> Oh, I thought we agreed that you take it via powerpc. I still think this
> is the best solution.
I threatened to do that :-) I don't remember you replying, did I miss
it ? If you are ok with the driver and are happy for me to take it,
please send an Ack.
> > I still maintain that it's not reasonable to hold driver for the
> > additions of multi-byte smbus offsets. This is a new feature that will
> > require changes to a number of existing bus and device drivers, so a
> > very pervasive change, and which will be visible to user space, which
> > means that drivers will need to continue supporting the "old" way at
> > least for a while anyway...
>
> Yeah, I agree on that. I am still unsure about the port-name binding,
> but well, if it is needed to fit your PowerNV scheme...
>From a binding perspective, it's just a piece of additional info that
the firmware provides for convenience. That we use it as the i2c port
name in Linux makes sense, it means that when listing the i2c ports,
it's immediately clear to the user which is which, it's not used
functionally by any driver or piece of code, but it's handy for people
doing things like manufacturing of machines, to know what bus to poke to
program a VPD EEPROM or test if a chip responds for example.
Cheers,
Ben.
^ permalink raw reply
* [PATCH] powerpc: secondary CPUs signal to master before setting active and online (fixes kernel BUG at kernel/smpboot.c:134!)
From: Anton Blanchard @ 2014-12-08 23:58 UTC (permalink / raw)
To: Ingo Molnar
Cc: peterz, yuanhan.liu, bsegall, paulus, rafael.j.wysocki, torvalds,
mingo, pjt, yuyang.du, daniel, rostedt, subbaram, tglx,
fengguang.wu, linuxppc-dev, linux-kernel, sp, tj, akpm,
computersforpeace, lkp
In-Reply-To: <20141208211859.6e81ec81@kryten>
Hi Ingo,
> At that point I thought the previous task_cpu() was somewhat ingrained
> in the scheduler and came up with the patch. If not, we could go on a
> hunt to see what else needs fixing.
I had another look. The scheduled does indeed make assumptions about the
previous task_cpu, but we have a hammer to fix it up called
select_fallback_rq.
I annotated select_fallback_rq, and did hit a case where the CPU was
not active. ppc64 patch below.
I think x86 have a similar (although harder to hit) issue. While it
does wait for the cpu_online bit to be set:
while (!cpu_online(cpu)) {
cpu_relax();
touch_nmi_watchdog();
}
The cpu_active bit is set after the cpu_online bit:
void set_cpu_online(unsigned int cpu, bool online)
{
if (online) {
cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
cpumask_set_cpu(cpu, to_cpumask(cpu_active_bits));
If the CPU got delayed between the two stores (eg a KVM guest had the CPU
scheduled out), then we'd end up with cpu_active unset and hit the same
issue in select_fallback_rq.
Anton
--
I have a busy ppc64le KVM box where guests sometimes hit the infamous
"kernel BUG at kernel/smpboot.c:134!" issue during boot:
BUG_ON(td->cpu != smp_processor_id());
Basically a per CPU hotplug thread scheduled on the wrong CPU. The oops
output confirms it:
CPU: 0
Comm: watchdog/130
The problem is that we aren't ensuring the CPU active and online bits are set
before allowing the master to continue on. The master unparks the secondary
CPUs kthreads and the scheduler looks for a CPU to run on. It calls
select_task_rq and realises the suggested CPU is not in the cpus_allowed
mask. It then ends up in select_fallback_rq, and since the active and
online bits aren't set we choose some other CPU to run on.
Cc: stable@vger.kernel.org
Signed-off-by: Anton Blanchard <anton@samba.org>
---
arch/powerpc/kernel/smp.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 71e186d..d40e46e 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -700,7 +700,6 @@ void start_secondary(void *unused)
smp_store_cpu_info(cpu);
set_dec(tb_ticks_per_jiffy);
preempt_disable();
- cpu_callin_map[cpu] = 1;
if (smp_ops->setup_cpu)
smp_ops->setup_cpu(cpu);
@@ -739,6 +738,14 @@ void start_secondary(void *unused)
notify_cpu_starting(cpu);
set_cpu_online(cpu, true);
+ /*
+ * CPU must be marked active and online before we signal back to the
+ * master, because the scheduler needs to see the cpu_online and
+ * cpu_active bits set.
+ */
+ smp_wmb();
+ cpu_callin_map[cpu] = 1;
+
local_irq_enable();
cpu_startup_entry(CPUHP_ONLINE);
--
2.1.0
^ permalink raw reply related
* Re: [PATCH v3 1/3] Revert "clk: ppc-corenet: Fix Section mismatch warning"
From: Kevin Hao @ 2014-12-09 1:34 UTC (permalink / raw)
To: Scott Wood, Mike Turquette
Cc: Yuantian Tang, Gerhard Sittig, linuxppc-dev, Jingchang Lu
In-Reply-To: <1417751519.334.9.camel@freescale.com>
[-- Attachment #1: Type: text/plain, Size: 579 bytes --]
On Thu, Dec 04, 2014 at 09:51:59PM -0600, Scott Wood wrote:
> This patch is going to conflict with commit a4ae8f3b0f7ac6ab3 "clk: drop
> owner assignment from platform_drivers" in linux-next -- or rather,
> you've based this on that patch, but it's not in mpe's next branch, so I
> get a merge conflict and there'd be another merge conflict later on to
> get back to the newer base.
>
> I really think this should go via the clock tree. That's where the
> breakage was introduced in the first place...
Hi Mike,
Could you pick up these patches?
Thanks,
Kevin
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [PATCH] kthread: kthread_bind fails to enforce CPU affinity (fixes kernel BUG at kernel/smpboot.c:134!)
From: Lai Jiangshan @ 2014-12-09 2:24 UTC (permalink / raw)
To: Steven Rostedt
Cc: yuyang.du, computersforpeace, peterz, lkp, rafael.j.wysocki,
yuanhan.liu, linux-kernel, bsegall, linuxppc-dev, mingo,
Anton Blanchard, sp, daniel, tj, subbaram, akpm, fengguang.wu,
torvalds, tglx, pjt
In-Reply-To: <20141208085405.730577a3@gandalf.local.home>
On 12/08/2014 09:54 PM, Steven Rostedt wrote:
> On Mon, 8 Dec 2014 14:27:01 +1100
> Anton Blanchard <anton@samba.org> wrote:
>
>> I have a busy ppc64le KVM box where guests sometimes hit the infamous
>> "kernel BUG at kernel/smpboot.c:134!" issue during boot:
>>
>> BUG_ON(td->cpu != smp_processor_id());
>>
>> Basically a per CPU hotplug thread scheduled on the wrong CPU. The oops
>> output confirms it:
>>
>> CPU: 0
>> Comm: watchdog/130
>>
>> The issue is in kthread_bind where we set the cpus_allowed mask, but do
>> not touch task_thread_info(p)->cpu. The scheduler assumes the previously
>> scheduled CPU is in the cpus_allowed mask, but in this case we are
>> moving a thread to another CPU so it is not.
>>
>
> Does this happen always on boot up, and always with the watchdog thread?
>
> I followed the logic that starts the watchdog threads.
>
> watchdog_enable_all_cpus()
> smpboot_register_percpu-thread() {
>
> for_each_online_cpu(cpu) { ... }
>
> Where watchdog_enable_all_cpus() can be called by
> lockup_detector_init() before SMP is started, but also by
> proc_dowatchdog() which is called by the sysctl commands (after SMP is
> up and running).
>
> I noticed there's no "get_online_cpus()" anywhere, although the
> unregister_percpu_thread() has it. Is it possible that we created a
> thread on a CPU that wasn't fully online yet?
>
> Perhaps the following patch is needed? Even if this isn't the solution
> to this bug, it is probably needed as watchdog_enable_all_cpus() can be
> called after boot up too.
>
> -- Steve
Hi, Steven, tglx
See this https://lkml.org/lkml/2014/7/30/804
"[PATCH] smpboot: add missing get_online_cpus() when register"
Thanks,
Lai
>
> diff --git a/kernel/smpboot.c b/kernel/smpboot.c
> index eb89e1807408..60d35ac5d3f1 100644
> --- a/kernel/smpboot.c
> +++ b/kernel/smpboot.c
> @@ -279,6 +279,7 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread)
> unsigned int cpu;
> int ret = 0;
>
> + get_online_cpus();
> mutex_lock(&smpboot_threads_lock);
> for_each_online_cpu(cpu) {
> ret = __smpboot_create_thread(plug_thread, cpu);
> @@ -291,6 +292,7 @@ int smpboot_register_percpu_thread(struct smp_hotplug_thread *plug_thread)
> list_add(&plug_thread->list, &hotplug_threads);
> out:
> mutex_unlock(&smpboot_threads_lock);
> + put_online_cpus();
> return ret;
> }
> EXPORT_SYMBOL_GPL(smpboot_register_percpu_thread);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
> .
>
^ permalink raw reply
* Re: [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode at boot time
From: Michael Ellerman @ 2014-12-09 4:11 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, Greg Kurz
In-Reply-To: <1417805565.334.15.camel@freescale.com>
On Fri, 2014-12-05 at 12:52 -0600, Scott Wood wrote:
> On Fri, 2014-12-05 at 16:14 +0100, Greg Kurz wrote:
> > The smt-enabled kernel parameter basically leaves unwanted cpus executing
> > in firmware or wherever they happen to be. The very same applies to the
> > ibm,smt-enabled DT property which is no more used by anything known. These
> > are hacks that shoudn't be used in a production environment.
> >
> > Quoting mpe, "there are better ways for firmware to disable SMT".
>
> Those "better ways" don't apply to Freescale chips, where the OS enables
> (or not) SMT without any interaction with firmware.
But how does it know there even are SMT threads? From the device tree? So
just don't present the threads in the device tree?
cheers
^ permalink raw reply
* Re: [PATCH] arch: powerpc: kernel: vio.c: Remove unused function
From: Michael Ellerman @ 2014-12-09 5:29 UTC (permalink / raw)
To: Rickard Strandqvist
Cc: devicetree, Li Zhong, linux-kernel, Rob Herring, Paul Mackerras,
Daniel Walter, Alistair Popple, Grant Likely, Andrew Morton,
linuxppc-dev
In-Reply-To: <1417991502-20223-1-git-send-email-rickard_strandqvist@spectrumdigital.se>
On Sun, 2014-12-07 at 23:31 +0100, Rickard Strandqvist wrote:
> Remove the function cmo_high_show() that is not used anywhere.
>
> This was partially found by using a static code analysis program called cppcheck.
>
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
> arch/powerpc/kernel/vio.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/arch/powerpc/kernel/vio.c b/arch/powerpc/kernel/vio.c
> index 5bfdab9..11657d2 100644
> --- a/arch/powerpc/kernel/vio.c
> +++ b/arch/powerpc/kernel/vio.c
> @@ -1032,11 +1032,6 @@ viobus_cmo_pool_rd_attr(reserve, size);
> viobus_cmo_pool_rd_attr(excess, size);
> viobus_cmo_pool_rd_attr(excess, free);
>
> -static ssize_t cmo_high_show(struct bus_type *bt, char *buf)
> -{
> - return sprintf(buf, "%lu\n", vio_cmo.high);
> -}
from ../arch/powerpc/kernel/vio.c:17:
../arch/powerpc/kernel/vio.c:1046:20: error: 'cmo_high_show' undeclared here (not in a function)
static BUS_ATTR_RW(cmo_high);
^
../include/linux/sysfs.h:76:10: note: in definition of macro '__ATTR'
.show = _show, \
^
../include/linux/device.h:53:42: note: in expansion of macro '__ATTR_RW'
struct bus_attribute bus_attr_##_name = __ATTR_RW(_name)
^
../arch/powerpc/kernel/vio.c:1046:8: note: in expansion of macro 'BUS_ATTR_RW'
static BUS_ATTR_RW(cmo_high);
^
cheers
^ permalink raw reply
* Re: [PATCH 1/7] CXL: Change contexts_lock to a mutex to fix sleep while atomic bug
From: Ian Munsie @ 2014-12-09 5:30 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1418026681-14787-1-git-send-email-imunsie@au.ibm.com>
This patch will definitely need to go to stable - we've run into issues
a couple of times when something has gone wrong on an AFU and ended up
taking down the whole system as a result of this bug.
Cheers,
-Ian
^ permalink raw reply
* Re: [PATCH 2/7] CXL: Add timeout to process element commands
From: Ian Munsie @ 2014-12-09 5:31 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1418026681-14787-2-git-send-email-imunsie@au.ibm.com>
This one needs to go to stable - I've hit it a couple of times while
testing bad AFUs and it results in an unkillable process.
Cheers,
-Ian
^ permalink raw reply
* Re: [PATCH 3/7] CXL: Fix leaking interrupts if attach process fails
From: Ian Munsie @ 2014-12-09 5:34 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1418026681-14787-3-git-send-email-imunsie@au.ibm.com>
This one would be nice to go to stable, but I'm not sure if it's
critical enough to justify it since it only reduces the number of
available interrupts (and therefore, contexts) that can be used by the
card (so, maybe you can only run 507 contexts simultaneously instead of
509)...
Cheers,
-Ian
^ permalink raw reply
* Re: [PATCH 5/7] CXL: Disable AFU debug flag
From: Ian Munsie @ 2014-12-09 5:39 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1418026681-14787-5-git-send-email-imunsie@au.ibm.com>
This one would be nice to go to stable, but I'm not sure it really meets
the rules. It could be a problem for userspace error paths checking the
result of MMIO reads, but only if the AFU has actually been unexpectedly
disabled somehow yet the PSL is still responding...
I don't think this is a high priority to go to stable unless it starts
causing problems for someone.
Cheers,
-Ian
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox