* Re: [PATCH][RFC] preempt_count corruption across H_CEDE call with CONFIG_PREEMPT on pseries
From: Darren Hart @ 2010-08-31 7:12 UTC (permalink / raw)
To: Ankita Garg
Cc: Stephen Rothwell, Gautham R Shenoy, Josh Triplett, Steven Rostedt,
linuxppc-dev, Will Schmidt, Paul Mackerras, Thomas Gleixner
In-Reply-To: <20100819155824.GD2690@in.ibm.com>
On 08/19/2010 08:58 AM, Ankita Garg wrote:
> Hi Darren,
>
> On Thu, Jul 22, 2010 at 11:24:13AM -0700, Darren Hart wrote:
>>
>> With some instrumentation we were able to determine that the
>> preempt_count() appears to change across the extended_cede_processor()
>> call. Specifically across the plpar_hcall_norets(H_CEDE) call. On
>> PREEMPT_RT we call this with preempt_count=1 and return with
>> preempt_count=0xffffffff. On mainline with CONFIG_PREEMPT=y, the value
>> is different (0x65) but is still incorrect.
>
> I was trying to reproduce this issue on a 2.6.33.7-rt29 kernel. I could
> easily reproduce this on the RT kernel and not the non-RT kernel.
This matches my experience.
> However, I hit it every single time I did a cpu online operation. I also
> noticed that the issue persists even when I disable H_CEDE by passing
> the "cede_offline=0" kernel commandline parameter. Could you pl confirm
> if you observe the same in your setup ?
Yes, this is my experience as well.
>
> However, the issue still remains. Will spend few cycles looking into
> this issue.
>
I've spent today trying to collect some useful traces. I consistently
see the preempt_count() change to 0xffffffff across the H_CEDE call, but
the irq and sched events (to ftrace) do not indicate anything else
running on the CPU that could change that.
<idle>-0 1d.h1. 11408us : irq_handler_entry: irq=16 name=IPI
<idle>-0 1d.h1. 11411us : irq_handler_exit: irq=16 ret=handled
... <other cpus>
<idle>-0 1d.... 22101us : .pseries_mach_cpu_die: start
<idle>-0 1d.... 22108us : .pseries_mach_cpu_die: cpu 1 (hwid 1) ceding for offline with hint 2
... <other cpus>
<idle>-0 1d.Hff. 33804us : .pseries_mach_cpu_die: returned from cede with pcnt: ffffffff
<idle>-0 1d.Hff. 33805us : .pseries_mach_cpu_die: forcing pcnt to 0
<idle>-0 1d.... 33807us : .pseries_mach_cpu_die: cpu 1 (hwid 1) returned from cede.
<idle>-0 1d.... 33808us : .pseries_mach_cpu_die: Decrementer value = 7fa49474 Timebase value = baefee6c88113
<idle>-0 1d.... 33809us : .pseries_mach_cpu_die: cpu 1 (hwid 1) got prodded to go online
<idle>-0 1d.... 33816us : .pseries_mach_cpu_die: calling start_seconday, pcnt: 0
<idle>-0 1d.... 33816us : .pseries_mach_cpu_die: forcing pcnt to 0
---------------------------------
Modules linked in: autofs4 binfmt_misc dm_mirror dm_region_hash dm_log [last unloaded: scsi_wait_scan]
NIP: c00000000006aa50 LR: c00000000006ac40 CTR: c00000000006ac14
REGS: c00000008e79ffc0 TRAP: 0300 Not tainted (2.6.33-rt-dvhrt16-02358-g61223dd-dirty)
MSR: 8000000000001032 <ME,IR,DR> CR: 28000022 XER: 00000000
DAR: c000018000ba44c0, DSISR: 0000000040000000
TASK = c00000010e6de040[0] 'swapper' THREAD: c00000008e7a0000 CPU: 1
GPR00: 0000018000000040 c00000008e7a0240 c000000000b55008 c00000010e6de040
GPR04: c000000085d800c0 0000000000000000 0000000000000000 000000000000000f
GPR08: 0000018000000000 c00000008e7a0000 c000000000ba4480 c000000000a32c80
GPR12: 8000000000009032 c000000000ba4680 c00000008e7a08f0 0000000000000001
GPR16: fffffffffffff2fa c00000010e6de040 0000000000000000 7fffffffffffffff
GPR20: 0000000000000000 0000000000000001 0000000000000001 c000000000f42c80
GPR24: c0000000850b7638 0000000000000000 0000000000000000 0000000000000001
GPR28: c000000000f42c80 c00000010e6de040 c000000000ad7698 c00000008e7a0240
NIP [c00000000006aa50] .resched_task+0x48/0xd8
LR [c00000000006ac40] .check_preempt_curr_idle+0x2c/0x44
Call Trace:
Instruction dump:
f821ff71 7c3f0b78 ebc2ab28 7c7d1b78 60000000 60000000 e95e8008 e97e8000
e93d0008 81090010 79084da4 38080040 <7d4a002a> 7c0b502e 7c000074 7800d182
---[ end trace 6d3f1cddaa17382c ]---
Kernel panic - not syncing: Attempted to kill the idle task!
Call Trace:
Rebooting in 180 seconds..
When running with the function plugin I had to stop the trace
immediately before entering start_secondary after an online or my traces
would not include the pseries_mach_cpu_die function, nor the tracing I
added there (possibly buffer size, I am using 2048). The following trace
was collected using "trace-cmd record -p function -e irq -e sched" and
has been filtered to only show CPU [001] (the CPU undergoing the
offline/online test, and the one seeing preempt_count (pcnt) go to
ffffffff after cede. The function tracer does not indicate anything
running on the CPU other than the HCALL - unless the __trace_hcall*
commands might be to blame. Can a POWER guru comment?
<idle>-0 [001] 417.625286: function: .cpu_die
<idle>-0 [001] 417.625287: function: .pseries_mach_cpu_die
<idle>-0 [001] 417.625290: bprint: .pseries_mach_cpu_die : start
<idle>-0 [001] 417.625291: function: .idle_task_exit
<idle>-0 [001] 417.625292: function: .switch_slb
<idle>-0 [001] 417.625294: function: .xics_teardown_cpu
<idle>-0 [001] 417.625294: function: .xics_set_cpu_priority
<idle>-0 [001] 417.625295: function: .__trace_hcall_entry
<idle>-0 [001] 417.625296: function: .probe_hcall_entry
<idle>-0 [001] 417.625297: function: .__trace_hcall_exit
<idle>-0 [001] 417.625298: function: .probe_hcall_exit
<idle>-0 [001] 417.625299: function: .__trace_hcall_entry
<idle>-0 [001] 417.625300: function: .probe_hcall_entry
<idle>-0 [001] 417.625301: function: .__trace_hcall_exit
<idle>-0 [001] 417.625302: function: .probe_hcall_exit
<idle>-0 [001] 417.625305: bprint: .pseries_mach_cpu_die : cpu 1 (hwid 1) ceding for offline with hint 2
<idle>-0 [001] 417.625307: bprint: .pseries_mach_cpu_die : calling extended cede, pcnt: 0
<idle>-0 [001] 417.625308: function: .__trace_hcall_entry
<idle>-0 [001] 417.625308: function: .probe_hcall_entry
<idle>-0 [001] 417.837734: function: .__trace_hcall_exit
<idle>-0 [001] 417.837736: function: .probe_hcall_exit
<idle>-0 [001] 417.837740: bprint: .pseries_mach_cpu_die : returned from cede with pcnt: ffffffff
<idle>-0 [001] 417.837742: bprint: .pseries_mach_cpu_die : forcing pcnt to 0
<idle>-0 [001] 417.837743: bprint: .pseries_mach_cpu_die : cpu 1 (hwid 1) returned from cede.
<idle>-0 [001] 417.837745: bprint: .pseries_mach_cpu_die : Decrementer value = 79844cbf Timebase value = bb3cf7ab2771c
<idle>-0 [001] 417.837747: bprint: .pseries_mach_cpu_die : cpu 1 (hwid 1) got prodded to go online
<idle>-0 [001] 417.837749: function: .__trace_hcall_entry
<idle>-0 [001] 417.837749: function: .probe_hcall_entry
<idle>-0 [001] 417.837755: function: .__trace_hcall_exit
<idle>-0 [001] 417.837755: function: .probe_hcall_exit
<idle>-0 [001] 417.837757: bprint: .pseries_mach_cpu_die : calling start_seconday, pcnt: 0
<idle>-0 [001] 417.837758: bprint: .pseries_mach_cpu_die : forcing pcnt to 0
I replaced the extended_cede_processor() call with mdelay(2). When I do
that, I don't see the preempt_count() change across the mdelay(2) call.
However, the system still eventually crashes in sub_preempt_count().
This appears to be independent of if preempt_count changes across
CEDE (since it doesn't occur across mdelay). I will try with larger
values of mdelay tomorrow to see if a longer delay will experience the
preempt_count value change.
Badness at kernel/sched.c:3726
NIP: c000000000698684 LR: c000000000698668 CTR: c00000000007a89c
REGS: c00000008e7a0170 TRAP: 0700 Not tainted (2.6.33-rt-dvhrt16-02358-g61223dd-dirty)
MSR: 8000000000021032 <ME,CE,IR,DR> CR: 28000022 XER: 00000000
TASK = c00000010e6de040[0] 'swapper' THREAD: c00000008e7a0000 CPU: 1
GPR00: 0000000000000000 c00000008e7a03f0 c000000000b55008 0000000000000001
GPR04: 0000000000000000 0000000000000000 0000000000000000 000000000000000f
GPR08: 0000000000000200 c000000000ca5420 0000000000000001 c000000000bc22f0
GPR12: 8000000000009032 c000000000ba4680 c00000008e7a0a10 0000000000000001
GPR16: fffffffffffff4d1 c00000010e6de040 0000000000000000 7fffffffffffffff
GPR20: 0000000000000000 0000000000000001 0000000000000001 c000000000f42c80
GPR24: 0000000000000001 0000000000000000 0000000000000000 0000000000000001
GPR28: c000000000f42c80 0000000000000001 c000000000ad7698 c00000008e7a03f0
NIP [c000000000698684] .sub_preempt_count+0xa8/0xdc
LR [c000000000698668] .sub_preempt_count+0x8c/0xdc
Call Trace:
Instruction dump:
41fd0038 78000620 2fa00000 40fe002c 4bd08a61 60000000 2fa30000 419e002c
e93e87e8 80090000 2f800000 409e001c <0fe00000> 48000014 78290464 80090014
Unable to handle kernel paging request for data at address 0xc000018000ba44c0
Faulting instruction address: 0xc00000000006aa1c
Oops: Kernel access of bad area, sig: 11 [#1]
PREEMPT SMP NR_CPUS=128 NUMA pSeries
last sysfs file:line
--
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team
^ permalink raw reply
* Re: [PATCH] gpiolib: Add 'struct gpio_chip' forward declaration for !GPIOLIB case
From: Grant Likely @ 2010-08-31 8:03 UTC (permalink / raw)
To: Anton Vorontsov
Cc: David Brownell, Andrew Morton, Benjamin Herrenschmidt,
linuxppc-dev, linux-kernel
In-Reply-To: <20100824092623.GA20334@oksana.dev.rtsoft.ru>
On Tue, Aug 24, 2010 at 01:26:23PM +0400, Anton Vorontsov wrote:
> With CONFIG_GPIOLIB=n, the 'struct gpio_chip' is not declared,
> so the following pops up on PowerPC:
>
> cc1: warnings being treated as errors
> In file included from arch/powerpc/platforms/52xx/mpc52xx_common.c:19:
> include/linux/of_gpio.h:74: warning: 'struct gpio_chip' declared
> inside parameter list
> include/linux/of_gpio.h:74: warning: its scope is only this definition
> or declaration, which is probably not what
> you want
> include/linux/of_gpio.h:75: warning: 'struct gpio_chip' declared
> inside parameter list
> make[2]: *** [arch/powerpc/platforms/52xx/mpc52xx_common.o] Error 1
>
> This patch fixes the issue by providing the proper forward declaration.
>
> Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
This doesn't actually solve the problem, and gpiochip should remain undefined when CONFIG_GPIOLIB=n to catch exactly these build failures. The real problem is that I merged a change into the mpc5200 code that required CONFIG_GPIOLIB be enabled without reflecting that requirement in Kconfig.
g.
> ---
>
> On Tue, Aug 24, 2010 at 04:26:08PM +1000, Benjamin Herrenschmidt wrote:
> > I get that with my current stuff:
> >
> > cc1: warnings being treated as errors
> > In file included from [..]/mpc52xx_common.c:19:
> > of_gpio.h:74: error: ‘struct gpio_chip’ declared inside parameter list
> [...]
> > make[3]: *** Waiting for unfinished jobs....
>
> That's because with GPIOCHIP=n no one declares struct gpio_chip.
>
> It should be either of_gpio.h or gpio.h. Let's make it gpio.h, as
> this feels more generic, and we already have some !GPIOLIB handling
> in there.
>
> include/linux/gpio.h | 6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> index 03f616b..85207d2 100644
> --- a/include/linux/gpio.h
> +++ b/include/linux/gpio.h
> @@ -15,6 +15,12 @@
> struct device;
>
> /*
> + * Some code might rely on the declaration. Still, it is illegal
> + * to dereference it for !GPIOLIB case.
> + */
> +struct gpio_chip;
> +
> +/*
> * Some platforms don't support the GPIO programming interface.
> *
> * In case some driver uses it anyway (it should normally have
> --
> 1.7.0.5
>
^ permalink raw reply
* Re: [PATCH] gpiolib: Add 'struct gpio_chip' forward declaration for !GPIOLIB case
From: Anton Vorontsov @ 2010-08-31 8:37 UTC (permalink / raw)
To: Grant Likely
Cc: David Brownell, Andrew Morton, Benjamin Herrenschmidt,
linuxppc-dev, linux-kernel
In-Reply-To: <20100831080344.GA20515@angua.secretlab.ca>
On Tue, Aug 31, 2010 at 02:03:44AM -0600, Grant Likely wrote:
> On Tue, Aug 24, 2010 at 01:26:23PM +0400, Anton Vorontsov wrote:
> > With CONFIG_GPIOLIB=n, the 'struct gpio_chip' is not declared,
> > so the following pops up on PowerPC:
> >
> > cc1: warnings being treated as errors
> > In file included from arch/powerpc/platforms/52xx/mpc52xx_common.c:19:
> > include/linux/of_gpio.h:74: warning: 'struct gpio_chip' declared
> > inside parameter list
> > include/linux/of_gpio.h:74: warning: its scope is only this definition
> > or declaration, which is probably not what
> > you want
> > include/linux/of_gpio.h:75: warning: 'struct gpio_chip' declared
> > inside parameter list
> > make[2]: *** [arch/powerpc/platforms/52xx/mpc52xx_common.o] Error 1
> >
> > This patch fixes the issue by providing the proper forward declaration.
> >
> > Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
>
> This doesn't actually solve the problem, and gpiochip should
> remain undefined when CONFIG_GPIOLIB=n to catch exactly these
> build failures. The real problem is that I merged a change
> into the mpc5200 code that required CONFIG_GPIOLIB be enabled
> without reflecting that requirement in Kconfig.
No, look closer. The error is in of_gpio.h, and it's perfectly
fine to include it w/o GPIOLIB=y.
> > ---
> >
> > On Tue, Aug 24, 2010 at 04:26:08PM +1000, Benjamin Herrenschmidt wrote:
> > > I get that with my current stuff:
> > >
> > > cc1: warnings being treated as errors
> > > In file included from [..]/mpc52xx_common.c:19:
> > > of_gpio.h:74: error: ‘struct gpio_chip’ declared inside parameter list
> > [...]
> > > make[3]: *** Waiting for unfinished jobs....
> >
> > That's because with GPIOCHIP=n no one declares struct gpio_chip.
> >
> > It should be either of_gpio.h or gpio.h. Let's make it gpio.h, as
> > this feels more generic, and we already have some !GPIOLIB handling
> > in there.
> >
> > include/linux/gpio.h | 6 ++++++
> > 1 files changed, 6 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> > index 03f616b..85207d2 100644
> > --- a/include/linux/gpio.h
> > +++ b/include/linux/gpio.h
> > @@ -15,6 +15,12 @@
> > struct device;
> >
> > /*
> > + * Some code might rely on the declaration. Still, it is illegal
> > + * to dereference it for !GPIOLIB case.
> > + */
> > +struct gpio_chip;
> > +
> > +/*
> > * Some platforms don't support the GPIO programming interface.
> > *
> > * In case some driver uses it anyway (it should normally have
> > --
> > 1.7.0.5
> >
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
* [PATCH] powerpc/512x: fix clk_get() return value
From: Akinobu Mita @ 2010-08-31 8:43 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Akinobu Mita
clk_get() should return an ERR_PTR value on error, not NULL.
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: linuxppc-dev@lists.ozlabs.org
---
arch/powerpc/platforms/512x/clock.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/powerpc/platforms/512x/clock.c b/arch/powerpc/platforms/512x/clock.c
index 5b243bd..3dc2a8d 100644
--- a/arch/powerpc/platforms/512x/clock.c
+++ b/arch/powerpc/platforms/512x/clock.c
@@ -57,7 +57,7 @@ static struct clk *mpc5121_clk_get(struct device *dev, const char *id)
int id_match = 0;
if (dev == NULL || id == NULL)
- return NULL;
+ return clk;
mutex_lock(&clocks_mutex);
list_for_each_entry(p, &clocks, node) {
--
1.7.1.231.gd0b16
^ permalink raw reply related
* [PATCH 2/2 v2] powerpc, pseries: Re-enable dispatch trace log userspace interface
From: Paul Mackerras @ 2010-08-31 11:59 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20100827055732.GB19033@brick.ozlabs.ibm.com>
Since the cpu accounting code uses the hypervisor dispatch trace log
now when CONFIG_VIRT_CPU_ACCOUNTING = y, the previous commit disabled
access to it via files in the /sys/kernel/debug/powerpc/dtl/ directory
in that case. This restores those files.
To do this, we now have a hook that the cpu accounting code will call
as it processes each entry from the hypervisor dispatch trace log.
The code in dtl.c now uses that to fill up its ring buffer, rather
than having the hypervisor fill the ring buffer directly.
This also fixes dtl_file_read() to handle overflow conditions a bit
better and adds a spinlock to ensure that race conditions (multiple
processes opening or reading the file concurrently) are handled
correctly.
Signed-off-by: Paul Mackerras <paulus@samba.org>
---
Removed extraneous increment of dtl->last_idx in this version.
Patch 1/1 of this series is unchanged.
arch/powerpc/include/asm/lppaca.h | 8 ++
arch/powerpc/kernel/time.c | 6 +-
arch/powerpc/platforms/pseries/dtl.c | 207 +++++++++++++++++++++++++++-------
3 files changed, 180 insertions(+), 41 deletions(-)
diff --git a/arch/powerpc/include/asm/lppaca.h b/arch/powerpc/include/asm/lppaca.h
index cfb85ec..7f5e0fe 100644
--- a/arch/powerpc/include/asm/lppaca.h
+++ b/arch/powerpc/include/asm/lppaca.h
@@ -191,6 +191,14 @@ struct dtl_entry {
#define DISPATCH_LOG_BYTES 4096 /* bytes per cpu */
#define N_DISPATCH_LOG (DISPATCH_LOG_BYTES / sizeof(struct dtl_entry))
+/*
+ * When CONFIG_VIRT_CPU_ACCOUNTING = y, the cpu accounting code controls
+ * reading from the dispatch trace log. If other code wants to consume
+ * DTL entries, it can set this pointer to a function that will get
+ * called once for each DTL entry that gets processed.
+ */
+extern void (*dtl_consumer)(struct dtl_entry *entry, u64 index);
+
#endif /* CONFIG_PPC_BOOK3S */
#endif /* __KERNEL__ */
#endif /* _ASM_POWERPC_LPPACA_H */
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index fca2064..bcb738b 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -183,6 +183,8 @@ DEFINE_PER_CPU(unsigned long, cputime_scaled_last_delta);
cputime_t cputime_one_jiffy;
+void (*dtl_consumer)(struct dtl_entry *, u64);
+
static void calc_cputime_factors(void)
{
struct div_result res;
@@ -218,7 +220,7 @@ static u64 read_spurr(u64 tb)
*/
static u64 scan_dispatch_log(u64 stop_tb)
{
- unsigned long i = local_paca->dtl_ridx;
+ u64 i = local_paca->dtl_ridx;
struct dtl_entry *dtl = local_paca->dtl_curr;
struct dtl_entry *dtl_end = local_paca->dispatch_log_end;
struct lppaca *vpa = local_paca->lppaca_ptr;
@@ -229,6 +231,8 @@ static u64 scan_dispatch_log(u64 stop_tb)
if (i == vpa->dtl_idx)
return 0;
while (i < vpa->dtl_idx) {
+ if (dtl_consumer)
+ dtl_consumer(dtl, i);
dtb = dtl->timebase;
tb_delta = dtl->enqueue_to_dispatch_time +
dtl->ready_to_enqueue_time;
diff --git a/arch/powerpc/platforms/pseries/dtl.c b/arch/powerpc/platforms/pseries/dtl.c
index 0357655..68cb2f2 100644
--- a/arch/powerpc/platforms/pseries/dtl.c
+++ b/arch/powerpc/platforms/pseries/dtl.c
@@ -23,6 +23,7 @@
#include <linux/init.h>
#include <linux/slab.h>
#include <linux/debugfs.h>
+#include <linux/spinlock.h>
#include <asm/smp.h>
#include <asm/system.h>
#include <asm/uaccess.h>
@@ -37,6 +38,7 @@ struct dtl {
int cpu;
int buf_entries;
u64 last_idx;
+ spinlock_t lock;
};
static DEFINE_PER_CPU(struct dtl, cpu_dtl);
@@ -55,25 +57,97 @@ static u8 dtl_event_mask = 0x7;
static int dtl_buf_entries = (16 * 85);
-static int dtl_enable(struct dtl *dtl)
+#ifdef CONFIG_VIRT_CPU_ACCOUNTING
+struct dtl_ring {
+ u64 write_index;
+ struct dtl_entry *write_ptr;
+ struct dtl_entry *buf;
+ struct dtl_entry *buf_end;
+ u8 saved_dtl_mask;
+};
+
+static DEFINE_PER_CPU(struct dtl_ring, dtl_rings);
+
+static atomic_t dtl_count;
+
+/*
+ * The cpu accounting code controls the DTL ring buffer, and we get
+ * given entries as they are processed.
+ */
+static void consume_dtle(struct dtl_entry *dtle, u64 index)
{
- unsigned long addr;
- int ret, hwcpu;
+ struct dtl_ring *dtlr = &__get_cpu_var(dtl_rings);
+ struct dtl_entry *wp = dtlr->write_ptr;
+ struct lppaca *vpa = local_paca->lppaca_ptr;
- /* only allow one reader */
- if (dtl->buf)
- return -EBUSY;
+ if (!wp)
+ return;
- /* we need to store the original allocation size for use during read */
- dtl->buf_entries = dtl_buf_entries;
+ *wp = *dtle;
+ barrier();
- dtl->buf = kmalloc_node(dtl->buf_entries * sizeof(struct dtl_entry),
- GFP_KERNEL, cpu_to_node(dtl->cpu));
- if (!dtl->buf) {
- printk(KERN_WARNING "%s: buffer alloc failed for cpu %d\n",
- __func__, dtl->cpu);
- return -ENOMEM;
- }
+ /* check for hypervisor ring buffer overflow, ignore this entry if so */
+ if (index + N_DISPATCH_LOG < vpa->dtl_idx)
+ return;
+
+ ++wp;
+ if (wp == dtlr->buf_end)
+ wp = dtlr->buf;
+ dtlr->write_ptr = wp;
+
+ /* incrementing write_index makes the new entry visible */
+ smp_wmb();
+ ++dtlr->write_index;
+}
+
+static int dtl_start(struct dtl *dtl)
+{
+ struct dtl_ring *dtlr = &per_cpu(dtl_rings, dtl->cpu);
+
+ dtlr->buf = dtl->buf;
+ dtlr->buf_end = dtl->buf + dtl->buf_entries;
+ dtlr->write_index = 0;
+
+ /* setting write_ptr enables logging into our buffer */
+ smp_wmb();
+ dtlr->write_ptr = dtl->buf;
+
+ /* enable event logging */
+ dtlr->saved_dtl_mask = lppaca_of(dtl->cpu).dtl_enable_mask;
+ lppaca_of(dtl->cpu).dtl_enable_mask |= dtl_event_mask;
+
+ dtl_consumer = consume_dtle;
+ atomic_inc(&dtl_count);
+ return 0;
+}
+
+static void dtl_stop(struct dtl *dtl)
+{
+ struct dtl_ring *dtlr = &per_cpu(dtl_rings, dtl->cpu);
+
+ dtlr->write_ptr = NULL;
+ smp_wmb();
+
+ dtlr->buf = NULL;
+
+ /* restore dtl_enable_mask */
+ lppaca_of(dtl->cpu).dtl_enable_mask = dtlr->saved_dtl_mask;
+
+ if (atomic_dec_and_test(&dtl_count))
+ dtl_consumer = NULL;
+}
+
+static u64 dtl_current_index(struct dtl *dtl)
+{
+ return per_cpu(dtl_rings, dtl->cpu).write_index;
+}
+
+#else /* CONFIG_VIRT_CPU_ACCOUNTING */
+
+static int dtl_start(struct dtl *dtl)
+{
+ unsigned long addr;
+ int ret, hwcpu;
/* Register our dtl buffer with the hypervisor. The HV expects the
* buffer size to be passed in the second word of the buffer */
@@ -85,12 +159,11 @@ static int dtl_enable(struct dtl *dtl)
if (ret) {
printk(KERN_WARNING "%s: DTL registration for cpu %d (hw %d) "
"failed with %d\n", __func__, dtl->cpu, hwcpu, ret);
- kfree(dtl->buf);
return -EIO;
}
/* set our initial buffer indices */
- dtl->last_idx = lppaca_of(dtl->cpu).dtl_idx = 0;
+ lppaca_of(dtl->cpu).dtl_idx = 0;
/* ensure that our updates to the lppaca fields have occurred before
* we actually enable the logging */
@@ -102,17 +175,66 @@ static int dtl_enable(struct dtl *dtl)
return 0;
}
-static void dtl_disable(struct dtl *dtl)
+static void dtl_stop(struct dtl *dtl)
{
int hwcpu = get_hard_smp_processor_id(dtl->cpu);
lppaca_of(dtl->cpu).dtl_enable_mask = 0x0;
unregister_dtl(hwcpu, __pa(dtl->buf));
+}
+
+static u64 dtl_current_index(struct dtl *dtl)
+{
+ return lppaca_of(dtl->cpu).dtl_idx;
+}
+#endif /* CONFIG_VIRT_CPU_ACCOUNTING */
+static int dtl_enable(struct dtl *dtl)
+{
+ long int n_entries;
+ long int rc;
+ struct dtl_entry *buf = NULL;
+
+ /* only allow one reader */
+ if (dtl->buf)
+ return -EBUSY;
+
+ n_entries = dtl_buf_entries;
+ buf = kmalloc_node(n_entries * sizeof(struct dtl_entry),
+ GFP_KERNEL, cpu_to_node(dtl->cpu));
+ if (!buf) {
+ printk(KERN_WARNING "%s: buffer alloc failed for cpu %d\n",
+ __func__, dtl->cpu);
+ return -ENOMEM;
+ }
+
+ spin_lock(&dtl->lock);
+ rc = -EBUSY;
+ if (!dtl->buf) {
+ /* store the original allocation size for use during read */
+ dtl->buf_entries = n_entries;
+ dtl->buf = buf;
+ dtl->last_idx = 0;
+ rc = dtl_start(dtl);
+ if (rc)
+ dtl->buf = NULL;
+ }
+ spin_unlock(&dtl->lock);
+
+ if (rc)
+ kfree(buf);
+ return rc;
+}
+
+static void dtl_disable(struct dtl *dtl)
+{
+ spin_lock(&dtl->lock);
+ dtl_stop(dtl);
kfree(dtl->buf);
dtl->buf = NULL;
dtl->buf_entries = 0;
+ spin_unlock(&dtl->lock);
}
/* file interface */
@@ -140,8 +262,9 @@ static int dtl_file_release(struct inode *inode, struct file *filp)
static ssize_t dtl_file_read(struct file *filp, char __user *buf, size_t len,
loff_t *pos)
{
- int rc, cur_idx, last_idx, n_read, n_req, read_size;
+ long int rc, n_read, n_req, read_size;
struct dtl *dtl;
+ u64 cur_idx, last_idx, i;
if ((len % sizeof(struct dtl_entry)) != 0)
return -EINVAL;
@@ -154,41 +277,48 @@ static ssize_t dtl_file_read(struct file *filp, char __user *buf, size_t len,
/* actual number of entries read */
n_read = 0;
- cur_idx = lppaca_of(dtl->cpu).dtl_idx;
+ spin_lock(&dtl->lock);
+
+ cur_idx = dtl_current_index(dtl);
last_idx = dtl->last_idx;
- if (cur_idx - last_idx > dtl->buf_entries) {
- pr_debug("%s: hv buffer overflow for cpu %d, samples lost\n",
- __func__, dtl->cpu);
- }
+ if (last_idx + dtl->buf_entries <= cur_idx)
+ last_idx = cur_idx - dtl->buf_entries + 1;
+
+ if (last_idx + n_req > cur_idx)
+ n_req = cur_idx - last_idx;
- cur_idx %= dtl->buf_entries;
- last_idx %= dtl->buf_entries;
+ if (n_req > 0)
+ dtl->last_idx = last_idx + n_req;
+
+ spin_unlock(&dtl->lock);
+
+ if (n_req <= 0)
+ return 0;
+
+ i = last_idx % dtl->buf_entries;
/* read the tail of the buffer if we've wrapped */
- if (last_idx > cur_idx) {
- read_size = min(n_req, dtl->buf_entries - last_idx);
+ if (i + n_req > dtl->buf_entries) {
+ read_size = dtl->buf_entries - i;
- rc = copy_to_user(buf, &dtl->buf[last_idx],
+ rc = copy_to_user(buf, &dtl->buf[i],
read_size * sizeof(struct dtl_entry));
if (rc)
return -EFAULT;
- last_idx = 0;
+ i = 0;
n_req -= read_size;
n_read += read_size;
buf += read_size * sizeof(struct dtl_entry);
}
/* .. and now the head */
- read_size = min(n_req, cur_idx - last_idx);
- rc = copy_to_user(buf, &dtl->buf[last_idx],
- read_size * sizeof(struct dtl_entry));
+ rc = copy_to_user(buf, &dtl->buf[i], n_req * sizeof(struct dtl_entry));
if (rc)
return -EFAULT;
- n_read += read_size;
- dtl->last_idx += n_read;
+ n_read += n_req;
return n_read * sizeof(struct dtl_entry);
}
@@ -220,11 +351,6 @@ static int dtl_init(void)
struct dentry *event_mask_file, *buf_entries_file;
int rc, i;
-#ifdef CONFIG_VIRT_CPU_ACCOUNTING
- /* disable this for now */
- return -ENODEV;
-#endif
-
if (!firmware_has_feature(FW_FEATURE_SPLPAR))
return -ENODEV;
@@ -251,6 +377,7 @@ static int dtl_init(void)
/* set up the per-cpu log structures */
for_each_possible_cpu(i) {
struct dtl *dtl = &per_cpu(cpu_dtl, i);
+ spin_lock_init(&dtl->lock);
dtl->cpu = i;
rc = dtl_setup_file(dtl);
--
1.7.1
^ permalink raw reply related
* SPI driver for MPC837xERDB
From: Ravi Gupta @ 2010-08-31 12:30 UTC (permalink / raw)
To: linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]
Hi all,
I am new to linux device driver development. I have to develop a SPI driver
for MPC8377 processor based board. Right now I don't have a that board
ready(but I have the MPC837xERDB with me), so I was thinking of developing
and testing it on the RDB. But in MPC837xERDB, I don't have an SPI slave
device, the SPI pins on the board is connected to the SD card slot. Is it
possible to use SD card slot as SPI device? Also I have written a sample SPI
driver, but its probe function is not getting called?
#include <linux/module.h>
#include <linux/spi/spi.h>
static int __devinit spi_probe(struct spi_device *spi)
{
printk(KERN_DEBUG "spi_probe...");
return 0;
}
static int __devexit spi_remove(struct spi_device *spi)
{
printk(KERN_DEBUG "spi_remove\n");
return 0;
}
static struct spi_driver sample_spi_driver = {
.driver = {
.name = "sample-spi",
.bus = &spi_bus_type,
.owner = THIS_MODULE,
},
.probe = spi_probe,
.remove = spi_remove,
};
static __init int spi_module_init(void)
{
printk(KERN_INFO "SPI Driver Version 0.1\n");
return spi_register_driver(&sample_spi_driver);
}
static __exit void spi_module_exit(void)
{
/* unregister SPI device */
spi_unregister_driver(&sample_spi_driver);
}
MODULE_LICENSE("GPL");
module_init(spi_module_init);
module_exit(spi_module_exit);
Any help would be greatly appreciated.
Thanks in advance,
Ravi
[-- Attachment #2: Type: text/html, Size: 1944 bytes --]
^ permalink raw reply
* Re: [PATCH] ucc_geth: fix ethtool set ring param bug
From: Ben Hutchings @ 2010-08-31 14:41 UTC (permalink / raw)
To: Liang Li; +Cc: netdev, avorontsov, linuxppc-dev, davem
In-Reply-To: <1283179677-21262-1-git-send-email-liang.li@windriver.com>
On Mon, 2010-08-30 at 22:47 +0800, Liang Li wrote:
> It's common sense that when we should do change to driver ring
> desc/buffer etc only after 'stop/shutdown' the device. When we
> do change while devices/driver is running, kernel oops occur:
[...]
> diff --git a/drivers/net/ucc_geth_ethtool.c b/drivers/net/ucc_geth_ethtool.c
> index 6f92e48..1b37aaa 100644
> --- a/drivers/net/ucc_geth_ethtool.c
> +++ b/drivers/net/ucc_geth_ethtool.c
> @@ -255,13 +255,18 @@ uec_set_ringparam(struct net_device *netdev,
> return -EINVAL;
> }
>
> - ug_info->bdRingLenRx[queue] = ring->rx_pending;
> - ug_info->bdRingLenTx[queue] = ring->tx_pending;
> -
> if (netif_running(netdev)) {
> - /* FIXME: restart automatically */
> - printk(KERN_INFO
> - "Please re-open the interface.\n");
> + printk(KERN_INFO "Stopping interface %s.\n", netdev->name);
> + ucc_geth_close(netdev);
> +
> + ug_info->bdRingLenRx[queue] = ring->rx_pending;
> + ug_info->bdRingLenTx[queue] = ring->tx_pending;
> +
> + printk(KERN_INFO "Reactivating interface %s.\n", netdev->name);
> + ucc_geth_open(netdev);
What if ucc_geth_open() fails?
Ben.
> + } else {
> + ug_info->bdRingLenRx[queue] = ring->rx_pending;
> + ug_info->bdRingLenTx[queue] = ring->tx_pending;
> }
>
> return ret;
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH] ucc_geth: fix ethtool set ring param bug
From: Liang Li @ 2010-08-31 15:16 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, avorontsov, linuxppc-dev, davem
In-Reply-To: <1283265682.2244.0.camel@achroite.uk.solarflarecom.com>
On Tue, Aug 31, 2010 at 03:41:22PM +0100, Ben Hutchings wrote:
> On Mon, 2010-08-30 at 22:47 +0800, Liang Li wrote:
> > It's common sense that when we should do change to driver ring
> > desc/buffer etc only after 'stop/shutdown' the device. When we
> > do change while devices/driver is running, kernel oops occur:
> [...]
> > diff --git a/drivers/net/ucc_geth_ethtool.c b/drivers/net/ucc_geth_ethtool.c
> > index 6f92e48..1b37aaa 100644
> > --- a/drivers/net/ucc_geth_ethtool.c
> > +++ b/drivers/net/ucc_geth_ethtool.c
> > @@ -255,13 +255,18 @@ uec_set_ringparam(struct net_device *netdev,
> > return -EINVAL;
> > }
> >
> > - ug_info->bdRingLenRx[queue] = ring->rx_pending;
> > - ug_info->bdRingLenTx[queue] = ring->tx_pending;
> > -
> > if (netif_running(netdev)) {
> > - /* FIXME: restart automatically */
> > - printk(KERN_INFO
> > - "Please re-open the interface.\n");
> > + printk(KERN_INFO "Stopping interface %s.\n", netdev->name);
> > + ucc_geth_close(netdev);
> > +
> > + ug_info->bdRingLenRx[queue] = ring->rx_pending;
> > + ug_info->bdRingLenTx[queue] = ring->tx_pending;
> > +
> > + printk(KERN_INFO "Reactivating interface %s.\n", netdev->name);
> > + ucc_geth_open(netdev);
>
> What if ucc_geth_open() fails?
I did some runtime tests but did not witness the ucc_geth_open fail.
Assume it may fail for some reason, then I tend to think give out
warnings for request user to open/enable it mannually? Or we may need
to keep the 'FIXME' line?
Thanks,
-Liang Li
>
> Ben.
>
> > + } else {
> > + ug_info->bdRingLenRx[queue] = ring->rx_pending;
> > + ug_info->bdRingLenTx[queue] = ring->tx_pending;
> > }
> >
> > return ret;
>
> --
> Ben Hutchings, Senior Software Engineer, Solarflare Communications
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
^ permalink raw reply
* Re: [PATCH] ucc_geth: fix ethtool set ring param bug
From: Ben Hutchings @ 2010-08-31 15:23 UTC (permalink / raw)
To: Liang Li; +Cc: netdev, avorontsov, linuxppc-dev, davem
In-Reply-To: <20100831151629.GA21442@localhost>
On Tue, 2010-08-31 at 23:16 +0800, Liang Li wrote:
> On Tue, Aug 31, 2010 at 03:41:22PM +0100, Ben Hutchings wrote:
> > On Mon, 2010-08-30 at 22:47 +0800, Liang Li wrote:
> > > It's common sense that when we should do change to driver ring
> > > desc/buffer etc only after 'stop/shutdown' the device. When we
> > > do change while devices/driver is running, kernel oops occur:
> > [...]
> > > diff --git a/drivers/net/ucc_geth_ethtool.c b/drivers/net/ucc_geth_ethtool.c
> > > index 6f92e48..1b37aaa 100644
> > > --- a/drivers/net/ucc_geth_ethtool.c
> > > +++ b/drivers/net/ucc_geth_ethtool.c
> > > @@ -255,13 +255,18 @@ uec_set_ringparam(struct net_device *netdev,
> > > return -EINVAL;
> > > }
> > >
> > > - ug_info->bdRingLenRx[queue] = ring->rx_pending;
> > > - ug_info->bdRingLenTx[queue] = ring->tx_pending;
> > > -
> > > if (netif_running(netdev)) {
> > > - /* FIXME: restart automatically */
> > > - printk(KERN_INFO
> > > - "Please re-open the interface.\n");
> > > + printk(KERN_INFO "Stopping interface %s.\n", netdev->name);
> > > + ucc_geth_close(netdev);
> > > +
> > > + ug_info->bdRingLenRx[queue] = ring->rx_pending;
> > > + ug_info->bdRingLenTx[queue] = ring->tx_pending;
> > > +
> > > + printk(KERN_INFO "Reactivating interface %s.\n", netdev->name);
> > > + ucc_geth_open(netdev);
> >
> > What if ucc_geth_open() fails?
>
> I did some runtime tests but did not witness the ucc_geth_open fail.
> Assume it may fail for some reason, then I tend to think give out
> warnings for request user to open/enable it mannually? Or we may need
> to keep the 'FIXME' line?
The easy way out is to allow changing the ring size only while the
interface is down. The hard way is to make the change in such a way
that you can roll back in case of failure.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* [PATCH 1/4] drivers/serial/ucc_uart.c: Add of_node_put to avoid memory leak
From: Julia Lawall @ 2010-08-31 15:48 UTC (permalink / raw)
To: Timur Tabi
Cc: linuxppc-dev, devicetree-discuss, kernel-janitors, linux-kernel
In-Reply-To: <1283269738-14612-1-git-send-email-julia@diku.dk>
Add a call to of_node_put in the error handling code following a call to
of_find_compatible_node or of_find_node_by_type.
This patch also substantially reorganizes the error handling code in the
function, to that it is possible first to jump to code that frees qe_port
and then to jump to code that also puts np.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r exists@
local idexpression x;
expression E,E1,E2;
statement S;
@@
*x =
(of_find_node_by_path
|of_find_node_by_name
|of_find_node_by_phandle
|of_get_parent
|of_get_next_parent
|of_get_next_child
|of_find_compatible_node
|of_match_node
|of_find_node_by_type
|of_find_node_with_property
|of_find_matching_node
|of_parse_phandle
)(...);
...
if (x == NULL) S
<... when != x = E
*if (...) {
... when != of_node_put(x)
when != if (...) { ... of_node_put(x); ... }
(
return <+...x...+>;
|
* return ...;
)
}
...>
(
E2 = x;
|
of_node_put(x);
)
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
drivers/serial/ucc_uart.c | 67 ++++++++++++++++++++++++----------------------
1 file changed, 35 insertions(+), 32 deletions(-)
diff --git a/drivers/serial/ucc_uart.c b/drivers/serial/ucc_uart.c
index 3f4848e..38a5ef0 100644
--- a/drivers/serial/ucc_uart.c
+++ b/drivers/serial/ucc_uart.c
@@ -1270,13 +1270,12 @@ static int ucc_uart_probe(struct platform_device *ofdev,
ret = of_address_to_resource(np, 0, &res);
if (ret) {
dev_err(&ofdev->dev, "missing 'reg' property in device tree\n");
- kfree(qe_port);
- return ret;
+ goto out_free;
}
if (!res.start) {
dev_err(&ofdev->dev, "invalid 'reg' property in device tree\n");
- kfree(qe_port);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_free;
}
qe_port->port.mapbase = res.start;
@@ -1286,17 +1285,17 @@ static int ucc_uart_probe(struct platform_device *ofdev,
if (!iprop) {
iprop = of_get_property(np, "device-id", NULL);
if (!iprop) {
- kfree(qe_port);
dev_err(&ofdev->dev, "UCC is unspecified in "
"device tree\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_free;
}
}
if ((*iprop < 1) || (*iprop > UCC_MAX_NUM)) {
dev_err(&ofdev->dev, "no support for UCC%u\n", *iprop);
- kfree(qe_port);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_free;
}
qe_port->ucc_num = *iprop - 1;
@@ -1310,16 +1309,16 @@ static int ucc_uart_probe(struct platform_device *ofdev,
sprop = of_get_property(np, "rx-clock-name", NULL);
if (!sprop) {
dev_err(&ofdev->dev, "missing rx-clock-name in device tree\n");
- kfree(qe_port);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_free;
}
qe_port->us_info.rx_clock = qe_clock_source(sprop);
if ((qe_port->us_info.rx_clock < QE_BRG1) ||
(qe_port->us_info.rx_clock > QE_BRG16)) {
dev_err(&ofdev->dev, "rx-clock-name must be a BRG for UART\n");
- kfree(qe_port);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_free;
}
#ifdef LOOPBACK
@@ -1329,39 +1328,39 @@ static int ucc_uart_probe(struct platform_device *ofdev,
sprop = of_get_property(np, "tx-clock-name", NULL);
if (!sprop) {
dev_err(&ofdev->dev, "missing tx-clock-name in device tree\n");
- kfree(qe_port);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_free;
}
qe_port->us_info.tx_clock = qe_clock_source(sprop);
#endif
if ((qe_port->us_info.tx_clock < QE_BRG1) ||
(qe_port->us_info.tx_clock > QE_BRG16)) {
dev_err(&ofdev->dev, "tx-clock-name must be a BRG for UART\n");
- kfree(qe_port);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_free;
}
/* Get the port number, numbered 0-3 */
iprop = of_get_property(np, "port-number", NULL);
if (!iprop) {
dev_err(&ofdev->dev, "missing port-number in device tree\n");
- kfree(qe_port);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_free;
}
qe_port->port.line = *iprop;
if (qe_port->port.line >= UCC_MAX_UART) {
dev_err(&ofdev->dev, "port-number must be 0-%u\n",
UCC_MAX_UART - 1);
- kfree(qe_port);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_free;
}
qe_port->port.irq = irq_of_parse_and_map(np, 0);
if (qe_port->port.irq == NO_IRQ) {
dev_err(&ofdev->dev, "could not map IRQ for UCC%u\n",
qe_port->ucc_num + 1);
- kfree(qe_port);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_free;
}
/*
@@ -1373,8 +1372,8 @@ static int ucc_uart_probe(struct platform_device *ofdev,
np = of_find_node_by_type(NULL, "qe");
if (!np) {
dev_err(&ofdev->dev, "could not find 'qe' node\n");
- kfree(qe_port);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_free;
}
}
@@ -1382,8 +1381,8 @@ static int ucc_uart_probe(struct platform_device *ofdev,
if (!iprop) {
dev_err(&ofdev->dev,
"missing brg-frequency in device tree\n");
- kfree(qe_port);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_np;
}
if (*iprop)
@@ -1398,16 +1397,16 @@ static int ucc_uart_probe(struct platform_device *ofdev,
if (!iprop) {
dev_err(&ofdev->dev,
"missing QE bus-frequency in device tree\n");
- kfree(qe_port);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_np;
}
if (*iprop)
qe_port->port.uartclk = *iprop / 2;
else {
dev_err(&ofdev->dev,
"invalid QE bus-frequency in device tree\n");
- kfree(qe_port);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_np;
}
}
@@ -1445,8 +1444,7 @@ static int ucc_uart_probe(struct platform_device *ofdev,
if (ret) {
dev_err(&ofdev->dev, "could not add /dev/ttyQE%u\n",
qe_port->port.line);
- kfree(qe_port);
- return ret;
+ goto out_np;
}
dev_set_drvdata(&ofdev->dev, qe_port);
@@ -1460,6 +1458,11 @@ static int ucc_uart_probe(struct platform_device *ofdev,
SERIAL_QE_MINOR + qe_port->port.line);
return 0;
+out_np:
+ of_node_put(np);
+out_free:
+ kfree(qe_port);
+ return ret;
}
static int ucc_uart_remove(struct platform_device *ofdev)
^ permalink raw reply related
* [PATCH 4/4] arch/powerpc/platforms/chrp/nvram.c: Add of_node_put to avoid memory leak
From: Julia Lawall @ 2010-08-31 15:48 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: devicetree-discuss, kernel-janitors, linux-kernel, Paul Mackerras,
linuxppc-dev
In-Reply-To: <1283269738-14612-1-git-send-email-julia@diku.dk>
Add a call to of_node_put in the error handling code following a call to
of_find_node_by_type.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@r exists@
local idexpression x;
expression E,E1,E2;
statement S;
@@
*x =
(of_find_node_by_path
|of_find_node_by_name
|of_find_node_by_phandle
|of_get_parent
|of_get_next_parent
|of_get_next_child
|of_find_compatible_node
|of_match_node
|of_find_node_by_type
|of_find_node_with_property
|of_find_matching_node
|of_parse_phandle
)(...);
...
if (x == NULL) S
<... when != x = E
*if (...) {
... when != of_node_put(x)
when != if (...) { ... of_node_put(x); ... }
(
return <+...x...+>;
|
* return ...;
)
}
...>
(
E2 = x;
|
of_node_put(x);
)
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
arch/powerpc/platforms/chrp/nvram.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/chrp/nvram.c b/arch/powerpc/platforms/chrp/nvram.c
index ba3588f..d3ceff0 100644
--- a/arch/powerpc/platforms/chrp/nvram.c
+++ b/arch/powerpc/platforms/chrp/nvram.c
@@ -74,8 +74,10 @@ void __init chrp_nvram_init(void)
return;
nbytes_p = of_get_property(nvram, "#bytes", &proplen);
- if (nbytes_p == NULL || proplen != sizeof(unsigned int))
+ if (nbytes_p == NULL || proplen != sizeof(unsigned int)) {
+ of_node_put(nvram);
return;
+ }
nvram_size = *nbytes_p;
^ permalink raw reply related
* Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak
From: walter harms @ 2010-08-31 15:49 UTC (permalink / raw)
To: Julia Lawall
Cc: devicetree-discuss, kernel-janitors, linux-kernel, linuxppc-dev
In-Reply-To: <1283075566-27441-2-git-send-email-julia@diku.dk>
Julia Lawall schrieb:
> Add a call to of_node_put in the error handling code following a call to
> of_find_node_by_path.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r exists@
> local idexpression x;
> expression E,E1;
> statement S;
> @@
>
> *x =
> (of_find_node_by_path
> |of_find_node_by_name
> |of_find_node_by_phandle
> |of_get_parent
> |of_get_next_parent
> |of_get_next_child
> |of_find_compatible_node
> |of_match_node
> )(...);
> ...
> if (x == NULL) S
> <... when != x = E
> *if (...) {
> ... when != of_node_put(x)
> when != if (...) { ... of_node_put(x); ... }
> (
> return <+...x...+>;
> |
> * return ...;
> )
> }
> ...>
> of_node_put(x);
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
>
> ---
> drivers/macintosh/via-pmu-led.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/macintosh/via-pmu-led.c b/drivers/macintosh/via-pmu-led.c
> index d242976..19c3718 100644
> --- a/drivers/macintosh/via-pmu-led.c
> +++ b/drivers/macintosh/via-pmu-led.c
> @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void)
> if (dt == NULL)
> return -ENODEV;
> model = of_get_property(dt, "model", NULL);
> - if (model == NULL)
> + if (model == NULL) {
> + of_node_put(dt);
> return -ENODEV;
> + }
> if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 &&
> strncmp(model, "iBook", strlen("iBook")) != 0 &&
> strcmp(model, "PowerMac7,2") != 0 &&
>
is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here
(what is done in the last cmp).
re,
wh
^ permalink raw reply
* Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak
From: Julia Lawall @ 2010-08-31 16:08 UTC (permalink / raw)
To: walter harms
Cc: devicetree-discuss, kernel-janitors, linux-kernel, linuxppc-dev
In-Reply-To: <4C7D247F.4060500@bfs.de>
On Tue, 31 Aug 2010, walter harms wrote:
>
>
> Julia Lawall schrieb:
> > Add a call to of_node_put in the error handling code following a call to
> > of_find_node_by_path.
> >
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> >
> > // <smpl>
> > @r exists@
> > local idexpression x;
> > expression E,E1;
> > statement S;
> > @@
> >
> > *x =
> > (of_find_node_by_path
> > |of_find_node_by_name
> > |of_find_node_by_phandle
> > |of_get_parent
> > |of_get_next_parent
> > |of_get_next_child
> > |of_find_compatible_node
> > |of_match_node
> > )(...);
> > ...
> > if (x == NULL) S
> > <... when != x = E
> > *if (...) {
> > ... when != of_node_put(x)
> > when != if (...) { ... of_node_put(x); ... }
> > (
> > return <+...x...+>;
> > |
> > * return ...;
> > )
> > }
> > ...>
> > of_node_put(x);
> > // </smpl>
> >
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> >
> > ---
> > drivers/macintosh/via-pmu-led.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/macintosh/via-pmu-led.c b/drivers/macintosh/via-pmu-led.c
> > index d242976..19c3718 100644
> > --- a/drivers/macintosh/via-pmu-led.c
> > +++ b/drivers/macintosh/via-pmu-led.c
> > @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void)
> > if (dt == NULL)
> > return -ENODEV;
> > model = of_get_property(dt, "model", NULL);
> > - if (model == NULL)
> > + if (model == NULL) {
> > + of_node_put(dt);
> > return -ENODEV;
> > + }
> > if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 &&
> > strncmp(model, "iBook", strlen("iBook")) != 0 &&
> > strcmp(model, "PowerMac7,2") != 0 &&
> >
>
> is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here
> (what is done in the last cmp).
Perhaps there are some characters after eg PowerBook that one doesn't want
to compare with?
julia
^ permalink raw reply
* Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak
From: Grant Likely @ 2010-08-31 16:13 UTC (permalink / raw)
To: Julia Lawall
Cc: devicetree-discuss, kernel-janitors, linux-kernel, linuxppc-dev,
walter harms
In-Reply-To: <Pine.LNX.4.64.1008311807470.2668@ask.diku.dk>
On Tue, Aug 31, 2010 at 06:08:19PM +0200, Julia Lawall wrote:
> On Tue, 31 Aug 2010, walter harms wrote:
> > > @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void)
> > > if (dt == NULL)
> > > return -ENODEV;
> > > model = of_get_property(dt, "model", NULL);
> > > - if (model == NULL)
> > > + if (model == NULL) {
> > > + of_node_put(dt);
> > > return -ENODEV;
> > > + }
> > > if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 &&
> > > strncmp(model, "iBook", strlen("iBook")) != 0 &&
> > > strcmp(model, "PowerMac7,2") != 0 &&
> > >
> >
> > is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here
> > (what is done in the last cmp).
>
> Perhaps there are some characters after eg PowerBook that one doesn't want
> to compare with?
Yes. The model property on powermacs always has the version number. strncmp makes sure that *all* PowerBooks and iBooks are matched.
g.
^ permalink raw reply
* Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak
From: Vasiliy Kulikov @ 2010-08-31 16:16 UTC (permalink / raw)
To: Julia Lawall
Cc: devicetree-discuss, kernel-janitors, linux-kernel, linuxppc-dev,
walter harms
In-Reply-To: <Pine.LNX.4.64.1008311807470.2668@ask.diku.dk>
On Tue, Aug 31, 2010 at 18:08 +0200, Julia Lawall wrote:
> On Tue, 31 Aug 2010, walter harms wrote:
>
> >
> >
> > Julia Lawall schrieb:
> > > Add a call to of_node_put in the error handling code following a call to
> > > of_find_node_by_path.
[...]
> > > --- a/drivers/macintosh/via-pmu-led.c
> > > +++ b/drivers/macintosh/via-pmu-led.c
> > > @@ -92,8 +92,10 @@ static int __init via_pmu_led_init(void)
> > > if (dt == NULL)
> > > return -ENODEV;
> > > model = of_get_property(dt, "model", NULL);
> > > - if (model == NULL)
> > > + if (model == NULL) {
> > > + of_node_put(dt);
> > > return -ENODEV;
> > > + }
> > > if (strncmp(model, "PowerBook", strlen("PowerBook")) != 0 &&
> > > strncmp(model, "iBook", strlen("iBook")) != 0 &&
> > > strcmp(model, "PowerMac7,2") != 0 &&
> > >
> >
> > is there any rule that says when to use strncmp ? it seems perfecly valid to use strcpy here
> > (what is done in the last cmp).
>
> Perhaps there are some characters after eg PowerBook that one doesn't want
> to compare with?
It seems to me that model has no '\0' in the end. If model is got from
the hardware then we should double check it - maybe harware is buggy.
Otherwise we'll overflow model.
But why strcmp(model, "PowerMac7,2")? IMO it should be replaced
with strncmp().
--
Vasiliy
^ permalink raw reply
* Re: [PATCH] powerpc: mtmsrd not defined
From: Kumar Gala @ 2010-08-31 16:17 UTC (permalink / raw)
To: Sean MacLennan; +Cc: linuxppc-dev
In-Reply-To: <20100822184844.37c30d5e@lappy.seanm.ca>
On Aug 22, 2010, at 5:48 PM, Sean MacLennan wrote:
> On Mon, 23 Aug 2010 08:34:54 +1000
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
>> I'd rather have a macro somewhere in ppc_asm.h (MTMSR ?) that does the
>> right thing. We might even already have one...
>
> We do.... here is a new, improved patch.
>
> Replace the BOOK3S_64 specific mtmsrd with the generic MTMSRD macro.
>
> Signed-off-by: Sean MacLennan <smaclennan@pikatech.com>
> ---
Can we add proper CONFIG_PPC_FPU into this file.
- k
^ permalink raw reply
* Re: [PATCH 1/7] drivers/macintosh/via-pmu-led.c: Add of_node_put to avoid memory leak
From: Grant Likely @ 2010-08-31 16:33 UTC (permalink / raw)
To: Vasiliy Kulikov
Cc: devicetree-discuss, kernel-janitors, linux-kernel, Julia Lawall,
linuxppc-dev, walter harms
In-Reply-To: <20100831161644.GA14505@albatros>
On Tue, Aug 31, 2010 at 10:16 AM, Vasiliy Kulikov <segooon@gmail.com> wrote=
:
> On Tue, Aug 31, 2010 at 18:08 +0200, Julia Lawall wrote:
>> On Tue, 31 Aug 2010, walter harms wrote:
>> > > =A0 if (strncmp(model, "PowerBook", strlen("PowerBook")) !=3D 0 &&
>> > > =A0 =A0 =A0 strncmp(model, "iBook", strlen("iBook")) !=3D 0 &&
>> > > =A0 =A0 =A0 strcmp(model, "PowerMac7,2") !=3D 0 &&
>> > >
>> >
>> > is there any rule that says when to use strncmp ? it seems perfecly va=
lid to use strcpy here
>> > (what is done in the last cmp).
>>
>> Perhaps there are some characters after eg PowerBook that one doesn't wa=
nt
>> to compare with?
>
> It seems to me that model has no '\0' in the end. If model is got from
> the hardware then we should double check it - maybe harware is buggy.
> Otherwise we'll overflow model.
Model does have \0 at the end. This code is using strncmp to
purposefully ignore the model suffix.
> But why strcmp(model, "PowerMac7,2")? IMO it should be replaced
> with strncmp().
We use strcmp when parsing the device tree because the the length of
the model property string is unknown and in most cases we *must* match
the exact entire string, such as with this PowerMac7,2 example. Using
strncmp would also happen to match with something like
"PowerMac7,2345" which is not the desired behaviour.
g.
^ permalink raw reply
* Re: [PATCH] powerpc/fsl_soc: Search all global-utilities nodes for rstccr
From: Matthew McClintock @ 2010-08-31 16:26 UTC (permalink / raw)
To: Timur Tabi; +Cc: kumar.gala, linuxppc-dev
In-Reply-To: <AANLkTimYz6v2tMJRYnki9Ph33HL9yGsOcpgJ78LTDCTG@mail.gmail.com>
On Aug 28, 2010, at 5:34 PM, Timur Tabi wrote:
>> <msm@freescale.com> wrote:
>=20
>> +
>> + for_each_node_by_name(np, "global-utilities") {
>> + if ((of_get_property(np, "fsl,has-rstcr", NULL))) {
>> + rstcr =3D of_iomap(np, 0) + 0xb0;
>> + if (!rstcr)
>> + printk (KERN_EMERG "Error: reset =
control "
>=20
> I'm not sure KERN_EMERG is warranted for this kind of error.
I'm not sure either - I left it as it was before.
>=20
>> + "register not =
mapped!\n");
>> + }
>=20
> So if a node has an fsl,rstcr property, but the of_iomap() fails, we
> jump to the next global-utilities node? Perhaps you need a 'break'
> after the printk()?
Or potentially a continue to be more robust? Or would two (or more) =
"has-rstcr" nodes be wrong?
>=20
>> + }
>> +
>> + if (!rstcr && ppc_md.restart =3D=3D fsl_rstcr_restart)
>=20
> Wouldn't it make more sense to assign fsl_rstcr_restart to
> ppc_md.restart only if we find a valid fsl,has-rstcr property?
Again I'm not entirely sure, I left this as it was before. Is there =
another way to reset the board if the rstcr node was not found =
correctly?
-M
>=20
> --=20
> Timur Tabi
> Linux kernel developer at Freescale
>=20
^ permalink raw reply
* [PATCH] powerpc/85xx: Fix compile issue with p1022_ds due to lmb rename to memblock
From: Kumar Gala @ 2010-08-31 16:42 UTC (permalink / raw)
To: linuxppc-dev
arch/powerpc/platforms/85xx/p1022_ds.c:22:23: error: linux/lmb.h: No such file or directory
arch/powerpc/platforms/85xx/p1022_ds.c: In function 'p1022_ds_setup_arch':
arch/powerpc/platforms/85xx/p1022_ds.c:100: error: implicit declaration of function 'memblock_end_of_DRAM'
arch/powerpc/platforms/85xx/p1022_ds.c: At top level:
arch/powerpc/platforms/85xx/p1022_ds.c:147: error: 'udbg_progress' undeclared here (not in a function)
make[2]: *** [arch/powerpc/platforms/85xx/p1022_ds.o] Error 1
Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
arch/powerpc/platforms/85xx/p1022_ds.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/85xx/p1022_ds.c b/arch/powerpc/platforms/85xx/p1022_ds.c
index e1467c9..34e0090 100644
--- a/arch/powerpc/platforms/85xx/p1022_ds.c
+++ b/arch/powerpc/platforms/85xx/p1022_ds.c
@@ -19,7 +19,7 @@
#include <linux/pci.h>
#include <linux/of_platform.h>
-#include <linux/lmb.h>
+#include <linux/memblock.h>
#include <asm/mpic.h>
#include <asm/swiotlb.h>
@@ -97,7 +97,7 @@ static void __init p1022_ds_setup_arch(void)
#endif
#ifdef CONFIG_SWIOTLB
- if (lmb_end_of_DRAM() > max) {
+ if (memblock_end_of_DRAM() > max) {
ppc_swiotlb_enable = 1;
set_pci_dma_ops(&swiotlb_dma_ops);
ppc_md.pci_dma_dev_setup = pci_dma_dev_setup_swiotlb;
--
1.6.0.6
^ permalink raw reply related
* Re: [PATCH] gpiolib: Add 'struct gpio_chip' forward declaration for !GPIOLIB case
From: Grant Likely @ 2010-08-31 16:44 UTC (permalink / raw)
To: Anton Vorontsov
Cc: David Brownell, Andrew Morton, Benjamin Herrenschmidt,
linuxppc-dev, linux-kernel
In-Reply-To: <20100831083716.GA28362@oksana.dev.rtsoft.ru>
On Tue, Aug 31, 2010 at 2:37 AM, Anton Vorontsov <cbouatmailru@gmail.com> w=
rote:
> On Tue, Aug 31, 2010 at 02:03:44AM -0600, Grant Likely wrote:
>> On Tue, Aug 24, 2010 at 01:26:23PM +0400, Anton Vorontsov wrote:
>> > With CONFIG_GPIOLIB=3Dn, the 'struct gpio_chip' is not declared,
>> > so the following pops up on PowerPC:
>> >
>> > =A0 cc1: warnings being treated as errors
>> > =A0 In file included from arch/powerpc/platforms/52xx/mpc52xx_common.c=
:19:
>> > =A0 include/linux/of_gpio.h:74: warning: 'struct gpio_chip' declared
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 inside par=
ameter list
>> > =A0 include/linux/of_gpio.h:74: warning: its scope is only this defini=
tion
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 or declara=
tion, which is probably not what
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 you want
>> > =A0 include/linux/of_gpio.h:75: warning: 'struct gpio_chip' declared
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 inside par=
ameter list
>> > =A0 make[2]: *** [arch/powerpc/platforms/52xx/mpc52xx_common.o] Error =
1
>> >
>> > This patch fixes the issue by providing the proper forward declaration=
.
>> >
>> > Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
>>
>> This doesn't actually solve the problem, and gpiochip should
>> remain undefined when CONFIG_GPIOLIB=3Dn to catch exactly these
>> build failures. =A0The real problem is that I merged a change
>> into the mpc5200 code that required CONFIG_GPIOLIB be enabled
>> without reflecting that requirement in Kconfig.
>
> No, look closer. The error is in of_gpio.h, and it's perfectly
> fine to include it w/o GPIOLIB=3Dy.
Looking even closer, we're both wrong. You're right I didn't look
carefully enough, and the error is in of_gpio.h, not the .c file.
However, it is not okay to get the definitions from of_gpio.h when
CONFIG_GPIOLIB=3Dn. If GPIOLIB, or more specifically OF_GPIO isn't set,
then the of_gpio.h definitions should either not be defined, or should
be defined as empty stubs (where appropriate).
So, instead of adding a forward declarations of the struct, the
correct thing I think to do is to #ifdef out the contents of of_gpio.h
when GPIOLIB isn't selected so that extra #includes are completely
benign. I've been doing the same thing on the other linux/of*.h
headers. I've got a patch in my tree that I'm testing right now and
I'll post later today.
g.
>
>> > ---
>> >
>> > On Tue, Aug 24, 2010 at 04:26:08PM +1000, Benjamin Herrenschmidt wrote=
:
>> > > I get that with my current stuff:
>> > >
>> > > cc1: warnings being treated as errors
>> > > In file included from [..]/mpc52xx_common.c:19:
>> > > of_gpio.h:74: error: =91struct gpio_chip=92 declared inside paramete=
r list
>> > [...]
>> > > make[3]: *** Waiting for unfinished jobs....
>> >
>> > That's because with GPIOCHIP=3Dn no one declares struct gpio_chip.
>> >
>> > It should be either of_gpio.h or gpio.h. Let's make it gpio.h, as
>> > this feels more generic, and we already have some !GPIOLIB handling
>> > in there.
>> >
>> > =A0include/linux/gpio.h | =A0 =A06 ++++++
>> > =A01 files changed, 6 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/include/linux/gpio.h b/include/linux/gpio.h
>> > index 03f616b..85207d2 100644
>> > --- a/include/linux/gpio.h
>> > +++ b/include/linux/gpio.h
>> > @@ -15,6 +15,12 @@
>> > =A0struct device;
>> >
>> > =A0/*
>> > + * Some code might rely on the declaration. Still, it is illegal
>> > + * to dereference it for !GPIOLIB case.
>> > + */
>> > +struct gpio_chip;
>> > +
>> > +/*
>> > =A0 * Some platforms don't support the GPIO programming interface.
>> > =A0 *
>> > =A0 * In case some driver uses it anyway (it should normally have
>> > --
>> > 1.7.0.5
>> >
>
> --
> Anton Vorontsov
> email: cbouatmailru@gmail.com
> irc://irc.freenode.net/bd2
>
--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH] powerpc/fsl_soc: Search all global-utilities nodes for rstccr
From: Timur Tabi @ 2010-08-31 16:56 UTC (permalink / raw)
To: Matthew McClintock; +Cc: kumar.gala, linuxppc-dev
In-Reply-To: <09C49916-9A20-41CD-A7BA-2F467BFD24DA@freescale.com>
On Tue, Aug 31, 2010 at 11:26 AM, Matthew McClintock <msm@freescale.com> wr=
ote:
>> I'm not sure KERN_EMERG is warranted for this kind of error.
>
> I'm not sure either - I left it as it was before.
My vote is to change it to KERN_ERR, but it's your call.
>> So if a node has an fsl,rstcr property, but the of_iomap() fails, we
>> jump to the next global-utilities node? =A0Perhaps you need a 'break'
>> after the printk()?
>
> Or potentially a continue to be more robust? Or would two (or more) "has-=
rstcr" nodes be wrong?
My point is that if the of_iomap() call fails, looking for another
global-utilities node is not the right course of action. of_iomap()
failing is a serious error that should result in an immediate exit.
>> Wouldn't it make more sense to assign fsl_rstcr_restart to
>> ppc_md.restart only if we find a valid fsl,has-rstcr property?
>
> Again I'm not entirely sure, I left this as it was before. Is there anoth=
er way to reset the board if the rstcr node was not found correctly?
Not on our 85xx boards. On 83xx, there's mpc83xx_restart(). I just
don't like "ppc_md.restart =3D=3D fsl_rstcr_restart", because it assumes
that the define_machine() entry is incorrect.
--=20
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH] gpiolib: Add 'struct gpio_chip' forward declaration for !GPIOLIB case
From: Anton Vorontsov @ 2010-08-31 17:07 UTC (permalink / raw)
To: Grant Likely
Cc: David Brownell, Andrew Morton, Benjamin Herrenschmidt,
linuxppc-dev, linux-kernel
In-Reply-To: <AANLkTi=EaHxDgwVfh-bhGANcBYWjHW7-aZt4K5ZZ7WRi@mail.gmail.com>
On Tue, Aug 31, 2010 at 10:44:14AM -0600, Grant Likely wrote:
> On Tue, Aug 31, 2010 at 2:37 AM, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> > On Tue, Aug 31, 2010 at 02:03:44AM -0600, Grant Likely wrote:
> >> On Tue, Aug 24, 2010 at 01:26:23PM +0400, Anton Vorontsov wrote:
> >> > With CONFIG_GPIOLIB=n, the 'struct gpio_chip' is not declared,
> >> > so the following pops up on PowerPC:
> >> >
> >> > cc1: warnings being treated as errors
> >> > In file included from arch/powerpc/platforms/52xx/mpc52xx_common.c:19:
> >> > include/linux/of_gpio.h:74: warning: 'struct gpio_chip' declared
> >> > inside parameter list
> >> > include/linux/of_gpio.h:74: warning: its scope is only this definition
> >> > or declaration, which is probably not what
> >> > you want
> >> > include/linux/of_gpio.h:75: warning: 'struct gpio_chip' declared
> >> > inside parameter list
> >> > make[2]: *** [arch/powerpc/platforms/52xx/mpc52xx_common.o] Error 1
> >> >
> >> > This patch fixes the issue by providing the proper forward declaration.
> >> >
> >> > Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
> >>
> >> This doesn't actually solve the problem, and gpiochip should
> >> remain undefined when CONFIG_GPIOLIB=n to catch exactly these
> >> build failures. The real problem is that I merged a change
> >> into the mpc5200 code that required CONFIG_GPIOLIB be enabled
> >> without reflecting that requirement in Kconfig.
> >
> > No, look closer. The error is in of_gpio.h, and it's perfectly
> > fine to include it w/o GPIOLIB=y.
>
> Looking even closer, we're both wrong. You're right I didn't look
> carefully enough, and the error is in of_gpio.h, not the .c file.
>
> However, it is not okay to get the definitions from of_gpio.h when
> CONFIG_GPIOLIB=n. If GPIOLIB, or more specifically OF_GPIO isn't set,
> then the of_gpio.h definitions should either not be defined, or should
> be defined as empty stubs (where appropriate).
Grrr. Grant, look again, even closer than you did.
They are stubs!
#else /* CONFIG_OF_GPIO */ <<<<<< !OF_GPIO (or !GPIOLIB) case.
/* Drivers may not strictly depend on the GPIO support, so let them link. */
static inline int of_get_gpio_flags(struct device_node *np, int index,
enum of_gpio_flags *flags)
{
return -ENOSYS;
}
static inline unsigned int of_gpio_count(struct device_node *np)
{
return 0;
}
static inline void of_gpiochip_add(struct gpio_chip *gc) { }
static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
#endif /* CONFIG_OF_GPIO */
The errors are triggered by the of_gpiochip_*() stubs, which
are needed by the drivers/gpio/gpiolib.c.
Do you want to add another #ifdef CONFIG_GPIOLIB around
of_gpiochip_*()? That would be ugly.
There's nothing wrong in providing the forward decls, you
can't dereference it anyway (so you would still catch the
bogus users). And at the same time it's will work great
for these stubs.
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply
* Re: [PATCH] powerpc: mtmsrd not defined
From: Sean MacLennan @ 2010-08-31 17:46 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev
In-Reply-To: <D9007E15-F764-450D-9B20-6506F0051BA7@kernel.crashing.org>
On Tue, 31 Aug 2010 11:17:17 -0500
Kumar Gala <galak@kernel.crashing.org> wrote:
> Can we add proper CONFIG_PPC_FPU into this file.
If nobody beats me to it.... I can try this evening.
Cheers,
Sean
^ permalink raw reply
* Re: [PATCH 2/3] PPC: Fix compilation of fsl_rio.c
From: Scott Wood @ 2010-08-31 18:10 UTC (permalink / raw)
To: Alexander Graf; +Cc: Linuxppc-dev, Thomas Moll, Alexandre Bounine
In-Reply-To: <1283220922-20369-3-git-send-email-agraf@suse.de>
On Tue, 31 Aug 2010 04:15:21 +0200
Alexander Graf <agraf@suse.de> wrote:
> Commit a52c8f52 introduced machine check magic for the RapidIO chip.
> Unfortunately it was so magical that it used constants that aren't even
> defined!
>
> This patch bluntly comments out the broken constant's usage. This
> probably means that said functionality thus doesn't work, but at
> least it makes it compile for me.
The MCSR_MASK is actually completely unnecessary -- it doesn't change
the result of testing bits that are within the mask.
Multiple patches have been posted for this already, including:
http://patchwork.ozlabs.org/patch/56135/
Someone just needs to apply it. :-)
-Scott
^ permalink raw reply
* Re: [PATCH 0/8] v5 De-couple sysfs memory directories from memory sections
From: Dave Hansen @ 2010-08-31 18:12 UTC (permalink / raw)
To: Nathan Fontenot
Cc: linuxppc-dev, Greg KH, linux-kernel, linux-mm, Andrew Morton,
KAMEZAWA Hiroyuki
In-Reply-To: <4C694C60.6030207@austin.ibm.com>
On Mon, 2010-08-16 at 09:34 -0500, Nathan Fontenot wrote:
> > It's not an unresolvable issue, as this is a must-fix problem. But you
> > should tell us what your proposal is to prevent breakage of existing
> > installations. A Kconfig option would be good, but a boot-time kernel
> > command line option which selects the new format would be much better.
>
> This shouldn't break existing installations, unless an architecture chooses
> to do so. With my patch only the powerpc/pseries arch is updated such that
> what is seen in userspace is different.
Even if an arch defines the override for the sysfs dir size, I still
don't think this breaks anything (it shouldn't). We move _all_ of the
directories over, all at once, to a single, uniform size. The only
apparent change to a user moving kernels would be a larger
block_size_bytes (which is certainly not changing the ABI) and a new
sysfs file for the end of the section. The new sysfs file is
_completely_ redundant at this point.
The architecture is only supposed to bump up the directory size when it
*KNOWS* that all operations will be done at the larger section size,
such as if the specific hardware has physical DIMMs which are much
larger than SECTION_SIZE.
Let's say we have a system with 20MB of memory, SECTION_SIZE of 1MB and
a sysfs dir size of 4MB.
Before the patch, we have 20 directories: one for each section. After
this patch, we have 5 directories.
The thing that I think is the next step, but that we _will_ probably
need eventually is this, take the 5 sysfs dirs in the above case:
0->3, 4->7, 8->11, 12->15, 16->19
and turn that into a single one:
0->19
*That* will require changing the ABI, but we could certainly have some
bloated and slow, but backward-compatible mode.
-- Dave
^ 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