* Re: Question on mpc52xx_common.c
From: Scott Wood @ 2008-04-08 20:07 UTC (permalink / raw)
To: Robert Schwebel; +Cc: linuxppc-dev
In-Reply-To: <20080408194517.GX13814@pengutronix.de>
Robert Schwebel wrote:
> Well observed; isn't this the prove of the assumption that the whole
> device tree idea is not working? It is *always* inconsistent and it is
> *maintenance hell* because out-of-tree ports do *always* breakt because
> of string inconsistencies. We have just ported a 8260 board from 2.6.22
> to 2.6.25 and it is almost 100% oftree porting.
There's going to be more churn in the initial stages than down the road.
82xx had barely been added to arch/powerpc in 2.6.22, and there was
little review of the initial device tree bindings.
> The ARM method of using just a device number is so much easier ...
Yeah, it's so much fun to have to allocate a globally unique number for
every minor tweak of a board, and to have to maintain a mapping from
said numbers to information that is semantically equivalent to a device
tree but in less maintainable form in the kernel source.
-Scott
^ permalink raw reply
* Re: Question on mpc52xx_common.c
From: Timur Tabi @ 2008-04-08 20:12 UTC (permalink / raw)
To: Robert Schwebel; +Cc: linuxppc-dev
In-Reply-To: <20080408194517.GX13814@pengutronix.de>
Robert Schwebel wrote:
> The ARM method of using just a device number is so much easier ...
And I was going to suggest that the ARM guys should use device trees, too.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [Cbe-oss-dev] [PATCH] Cell OProfile: SPU mutex lock fix
From: Carl Love @ 2008-04-08 20:21 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linuxppc-dev, cel, cbe-oss-dev, linux-kernel
In-Reply-To: <200804040838.40179.arnd@arndb.de>
On Fri, 2008-04-04 at 08:38 +0200, Arnd Bergmann wrote:
> On Wednesday 02 April 2008, Carl Love wrote:
> > On Wed, 2008-04-02 at 07:21 +0200, Arnd Bergmann wrote:
> > > On Tuesday 25 March 2008, Carl Love wrote:
> > > > This patch fixes a bug in the code that records the SPU data and
> > > > context switches. The buffer_mutex lock must be held when the
> > > > kernel is adding data to the buffer between the kernel and the
> > > > OProfile daemon. The lock is not being held in the current code
> > > > base. This patch fixes the bug using work queues. The data to
> > > > be passed to the daemon is caputured by the interrupt handler.
> > > > The workqueue function is invoked to grab the buffer_mutex lock
> > > > and add the data to the buffer.
> > >
> > > So what was the exact bug you're fixing with this? There was no
> > > buffer_mutex before, so why do you need it now? Can't this be a
> > > spinlock so you can get it from interrupt context instead of
> > > using a workqueue?
> >
> > The generic OProfile code defines a mutex lock, called buffer_mutex, to
> > protect the kernel/daemon data buffer from being writen by the kernal
> > and simultaneously read by the Daemon. When adding a PPU sample the
> > oprofile routine oprofile_add_ext_sample(pc, regs, i, is_kernel) is
> > called from the interrupt context to request the sample be stored. The
> > generic oprofile code takes care of passing the data to a non interrupt
> > context where the mutex lock is held and the necessary sequence of data
> > is written into the kernel/daemon data buffer. However, OProfile does
> > not have any built in functions for handling the SPU. Hence, we have to
> > implement the code to capture the data in the interrupt context, pass it
> > to a non interrupt context and put it into the buffer. This was not
> > done correctly in the original implementation. Specifically, the mutex
> > lock was not being held.
>
> Ok, I see.
>
> However, I'm pretty sure that the switch notification does not get
> called from an atomic context, so you don't need a workqueue for
> bringing that into a process context. Doing the context switch
> notification directly from the scheduler sounds much better regarding
> the impact on the measurement.
Our first thought to fix the bug was to just grab the mutex lock when
adding the switch notification data to the queue. The kernel gives us
an oops message saying something along the line of "could not call mutex
lock in interrupt context". Hence we had to go to work queues so we
could access the lock outside of the SPU switch notification context.
Secondly, it is my understanding that if the schedule_work() call tries
to queue the same work function multiple times the subsequent requests
are dropped. Thus we were not able to pass the context switch data as
part of the schedule work requests. This forced us to have an array to
store the data for each SPU.
>
> > > Never put extern statements in the implementation, they describe the
> > > interface between two parts of the code and should be inside of a
> > > common header.
> > >
> > > Why do you want to have your own workqueue instead of using the
> > > global one?
> >
> > It is important that the data get context switch data get recorded as
> > quickly as possible to avoid dropping data unnecessarily. The PC
> > counter data for each SPU is ignored until the context switch record is
> > put into the kernel/daemon buffer. The API documentation says that
> > using a private workqueue has better performance then using the global
> > workqueue. There is a comment in the code about this, perhaps it is not
> > clear enough.
>
> This sounds like an unrelated bug in the implementation. The PC
> data should *not* be ignored in any case. As long as the records
> get stored in the right order, everything should be fine here.
Until the OProfile sees the context switch record, it does not know what
to do with the PC samples and just drops them. The thought was using a
private work queue might help get the context switch records processed a
little earlier. It probably doesn't make that much difference. I can
just use the generic work queue.
>
>
> > > This looks like you want to use a delayed_work rather than building your
> > > own out of hrtimer and work. Is there any point why you want to use
> > > an hrtimer?
> >
> > The current implementation uses the hrtimer to schedule when to read the
> > trace buffer the next time. This patch does not change how the
> > scheduling of the buffer reads is done. Yes, you could change the
> > implementation to use workqueues instead. If you feel that it is better
> > to use the workqueue then we could make that change. Not sure that
> > making that change in this bug fix patch is appropriate. I would need
> > to create a second patch for that change.
>
> I would guess that the change from hrtimer to delayed_workqueue is
> smaller than the current patch changing from hrtimer to hrtimer plus
> workqueue, so I would prefer to have only one changeset.
>
> Since the timer only causes statistical data collection anyway, delaying
> it a bit should not have any negative effect on the accuracy of the
> measurement, unlike delaying the context switch notification.
The goal is to be able to support very high sampling rates (small
periods). The schedule_delayed_work() is based on jiffies which I
believe is 1/250 for this machine. This only gives millisecond
resolution. The goal is for the users to be able to specify a time
period of 60,000 cycles or less then 20 micro second sampling periods
when the real high resolution timers are available. We can't achieve
the desired sampling rates with the schedule_dealyed_work() function.
>
> > > > -static DEFINE_SPINLOCK(buffer_lock);
> > > > +extern struct mutex buffer_mutex;
> > > > +extern struct workqueue_struct *oprofile_spu_wq;
> > > > +extern int calls_to_record_switch;
> > > > +
> > >
> > > Again, public interfaces need to go to a header file, and should
> > > have a name that identifies the interface. "buffer_mutex" is
> > > certainly not a suitable name for a kernel-wide global variable!
> >
> > As stated earlier, the generic OProfile code defines the variable
> > "buffer_mutex". Changing the name in the generic OProfile code is
> > beyond the scope of this patch.
>
> Ok, didn't see that the name was already part of the main oprofile
> driver. However, this makes it even worse: you are accessing data
> structures that are clearly not meant to be shared with architecture
> code. The fact that it was not declared in a global header file should
> have told you that.
>
> I think you should instead add a function to drivers/oprofile/buffer_sync.c
> that takes care of moving the data to the common buffer under the right
> mutex_lock.
Oprofile provides nice clean interfaces for recording kernel/user
switches and CPU data recording. This is all that was needed by any
architecture until CELL came along. With CELL, we now have need to add
processor data plus SPU data to the queue. The buffer_mutex variable
and the add_event_entry() were not visible outside of the OProfile
driver code. The original SPU support added add_event_entry() to the
include/linux/oprofile.h file. We can add the buffer_mutex as well
since there is now a need to access both of these.
I have been looking to see how I could create a generic oprofile routine
which could take the data. The routine would still have to work from an
interrupt context, so it will need to store the data and call a work
queue function. The function would need to know how much data will be
needed, thus you would probably need to statically allocate data or use
a list and malloc the data as needed. I don't really want to have to
malloc data from an interrupt context. List management adds additional
overhead. It would be possible to have an init function that you could
call at startup time telling it how much memory you need, in this case
we could allocate a buffer the size of spu_info (defined below) at
startup time. The call could pass an array to the OProfile routine that
would put the data into the buffer and call the work function. We still
have to allocate the storage, it does clean up the arch specific code.
Not sure if this really buys us much. There is more copying of data
i.e. more overhead. Not convinced the OProfile maintainers would accept
anything I have thought of so far.
Any suggestions?
>
> > >
> > > > static DEFINE_SPINLOCK(cache_lock);
> > > > static int num_spu_nodes;
> > > > +
> > > > int spu_prof_num_nodes;
> > > > int last_guard_val[MAX_NUMNODES * 8];
> > > > +int cnt_swtch_processed_flag[MAX_NUMNODES * 8];
> > > > +
> > > > +struct spus_profiling_code_data_s {
> > > > + int num_spu_nodes;
> > > > + struct work_struct spu_prof_code_wq;
> > > > +} spus_profiling_code_data;
> > > > +
> > > > +struct spu_context_switch_data_s {
> > > > + struct spu *spu;
> > > > + unsigned long spu_cookie;
> > > > + unsigned long app_dcookie;
> > > > + unsigned int offset;
> > > > + unsigned long objectId;
> > > > + int valid_entry;
> > > > +} spu_context_switch_data;
> > >
> > > I don't understand what these variables are really doing, but
> > > having e.g. just one spu_context_switch_data for all the SPUs
> > > doesn't seem to make much sense. What happens when two SPUs do
> > > a context switch at the same time?
> >
> > This is the data same data that was being put into the event buffer
> > directly from the interrupt context. We need to store the data that is
> > only available in the interrupt context so the same data can be put into
> > the buffer by the work queue function in the non interrupt context.
> > This is the declaration of the data needed per SPU. Below in the
> > spu_cntx_sw_data structure, we declare an array of entries so we can
> > store the switch data on a per SPU basis as you alluded to.
>
> The spu_context_switch_data and spus_profiling_code_data variables
> are also unused, or just write-only. They look like they were left
> over after a conversion from a typedef.
The spu_context_switch_data structure is the type of the array used in
struct spus_cntxt_sw_data_s which is the data used by the work queue.
Yes, spus_profiling_code_data seems to be unused.
>
> > The calls_to_record_switch variable is not used, my mistake for not
> > getting it out of the patch. The record_spu_stat_flag is used. It is
> > set in the spu_sync_start when SPU profiling is started. The first time
> > the work function is called to record SPU context switches it sees the
> > flag is set and writes the initial record to the daemon/kernel buffer
> > stating that this is an SPU profile run not a PPU profile run. The
> > daemon needs to know this as it effects how the postprocessing is done.
> > The initial record is only written once.
> >
> > The spus_context_sw_data structure has the array per SPU for all of the
> > interrupt context data that was recorded and needs to be written to the
> > kernel/daemon buffer.
>
> An ideal driver should not have *any* global variables at all, but store
> all data in the (reference counted) objects it is dealing with, or
> just on the stack while it's processing the data.
>
> Storing the context switch information in a global breaks down as soon
> as there are multiple context switches taking place for a single
> SPU without the workqueue running in between, which is a very likely
> scenario if you have high-priority tasks on the SPU.
Yes, it was recognized that we could overwrite data. The expectation is
that the workqueue function will run fairly quickly. If the SPU context
was only loaded for a very short period of time at most a few samples
would be captured for that frame so the error would be down in the
noise. OProfile is a statistical tool. If we don't get enough samples
for the spu context, then the data is not statistically valid. Losing
the context switch is not an issue. The context must be loaded long
enough that we collect a reasonable number of samples for the output to
be meaningful.
>
> > > > /* Container for caching information about an active SPU task. */
> > > > struct cached_info {
> > > > @@ -44,6 +73,8 @@ struct cached_info {
> > > > struct kref cache_ref;
> > > > };
> > > >
> > > > +struct workqueue_struct *oprofile_spu_wq;
> > > > +
> > > > static struct cached_info *spu_info[MAX_NUMNODES * 8];
> > >
> > > While you're cleaning this up, I guess the cached_info should
> > > be moved into a pointer from struct spu as well, instead of
> > > having this global variable here.
> >
> > This would be a functional change and it belongs in a functional change
> > patch not in a bug fix patch.
>
> The patch is already far bigger than a simple bug fix, but you're right
> in that this part should be separate. Upon reading through the code
> again, I noticed that the cached_info is thrown away on each context
> switch and rebuilt, which I guess makes it impossible to really profile
> the context switch code. In the initial design phase for spu oprofile, we
> decided that the information should be cached in the spu_context, which
> would not only be much cleaner but also avoid the performance problem.
>
> Do you have any idea why this idea was dropped? Is it still work in
> progress to get that functionality right?
It was not dropped. It was implemented. The issue that I have is the
dcookie spu_cookie, app_dcookie, offset, objectId is not included in the
local cahced spu context data. Previously, there was no need to save it
since it was being immediatly written to oprofile buffer (without
holding the mutex lock). Now we need to store the data until the
workqueue function runs. We can store it in the array as I have done or
you could put it in the spu context. Functionally, it doesn't change
anything. The data in the SPU context would get overwritten, just as it
does in the array, if there was an SPU context switch before the
workqueue function runs so that doesn't solve that issue.
>
> > > I would guess that you need one work struct per SPU instead of a global
> > > one, if you want to pass the SPU pointer as an argument.
> >
> > We only need one work struct because we have an array that contains the
> > data for each SPU that has done a context switch.
>
> right, but as I explained, the global array is the real problem that should
> be fixed.
>
> Arnd <><
^ permalink raw reply
* Re: Question on mpc52xx_common.c
From: Grant Likely @ 2008-04-08 21:26 UTC (permalink / raw)
To: Robert Schwebel; +Cc: linuxppc-dev
In-Reply-To: <20080408194517.GX13814@pengutronix.de>
On Tue, Apr 8, 2008 at 1:45 PM, Robert Schwebel
<r.schwebel@pengutronix.de> wrote:
> On Tue, Apr 08, 2008 at 08:52:55AM -0600, Grant Likely wrote:
> > It may be ideal, but I don't think it is realistic. I'm now of the
> > firm opinion that device trees and firmware are *never* perfect.
> > Especially when the definition of perfect is a moving target.
>
> Well observed; isn't this the prove of the assumption that the whole
> device tree idea is not working? It is *always* inconsistent and it is
> *maintenance hell* because out-of-tree ports do *always* breakt because
> of string inconsistencies. We have just ported a 8260 board from 2.6.22
> to 2.6.25 and it is almost 100% oftree porting. And you do not even have
> a single point of a parser, because all this string parsing is
> completely scattered all over the tree.
I disagree and that is not my point. My point is that perfection is
neither obtainable or necessary. Many of the recently established
embedded guidelines are not "perfect" because they are counter to a
few of the OF recommended practices. However, they are consistent
within themselves, they work, and once established they are not going
to change. imperfect != bad. I brought up a new 5200 board this week
which required zero kernel changes. All I needed was a new dt to
describe the board.
Now, if out-of-tree ports continue to break then we've got a problem
that needs to be fixed. Once a binding is established (which usually
takes a few kernel releases) it should be very stable and even if the
definition of ideal is changed, backwards compatibility must be
maintained.
> The ARM method of using just a device number is so much easier ...
Only if the assumption is made that very little data needs to be
shared between the kernel and the firmware. The moment you try to do
something more complex you either have the nightmare of bd_info or you
use a structured data format (like the device tree)
On another node, there are platforms where a device number is
unworkable. For example, for Linux on an FPGA like the Xilinx Virtex,
there would need to be a new platform number every time the FPGA
bitstream was updated because it is effectively an entirely different
platform.
Finally, using a device number means you need to encode into the
kernel the exact layout of every platform it supports. That adds up
to a lot of code in a real hurry; even if most of it is just
boilerplate instantiations.
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: Booting a Xilinx board
From: Grant Likely @ 2008-04-08 21:32 UTC (permalink / raw)
To: Guillaume Dargaud; +Cc: linuxppc-dev
In-Reply-To: <10fd01c89974$57a0c830$f52f9e86@LPSC0173W>
On Tue, Apr 8, 2008 at 6:30 AM, Guillaume Dargaud <dargaud@lpsc.in2p3.fr> wrote:
> I'm making progress, thanks in no small part to this list, but I still need
> coaching, if you don't mind. The current boot is currently going as such:
>
>
> loaded at: 00400000 005A819C
> board data at: 005A6120 005A619C
<snip>
> [ 4.897987] RPC: Registered udp transport module.
> [ 4.953147] RPC: Registered tcp transport module.
> [ 5.011138] RAMDISK: Compressed image found at block 0
> [ 5.905538] VFS: Mounted root (ext2 filesystem).
> [ 5.945703] Freeing unused kernel memormdev: /etc/mdev.conf: No such
> file
> or directory
> mdev: /etc/mdev.conf: No such file or directory
<snip>
> mdev: /etc/mdev.conf: No such file or directory
> mdev: /etc/mdev.conf: No such file or directory
> mount: mounting configfs on /config failed: No such device
> Initializing random number generator... done.
> Starting network...
> ip: RTNETLINK answers: File exists
>
>
> And then it stops, although I still have echo when I type.
> Now there's no /etc/mdev.conf file in my root fylesystem and I understand I
> need to create the devices.
> I've been reading docs/mdev.txt and
> http://www.lfs-matrix.org/clfs/view/clfs-embedded/mips/bootscripts/mdev.html
> and I'm a bit confused.
Congratulations, your kernel is in good shape. Now you're dealing
with userspace issues. You can try bypassing most of the init process
to see if you can get a prompt. Add init=/bin/sh to the kernel
command line.
>
> - do the devices need to be created in the FS before building the ramdisk
> and subsequent ACE file ?
Yes, you can statically create the device files. MAKEDEV will do this
for you. You can even just copy them from a working Linux system (any
arch).
> - are they created dynamically by /bin/mdev on boot based on a manually
> written /etc/mdev.conf table as the error messages seem to imply ?
That depends on your rootfs. Where did you get your rootfs image?
> - can they be generated automagically by a MAKEDEV script ?
>
> In the second case, how do I know which devices and what names I'm supposed
> to define ?
Easiest way: copy from the /dev tree in a running Linux machine.
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* [PATCH] ata: Fix pata SIL680 build on arch/ppc
From: Benjamin Herrenschmidt @ 2008-04-08 21:51 UTC (permalink / raw)
To: jgarzik; +Cc: linuxppc-dev, Linux IDE
Commit 0f436eff54f90419ac1b8accfb3e6e17c4b49a4e breaks build on
arch/ppc as it doesn't implement the machine_is() macro.
This fixes it by using CONFIG_PPC_MERGE instead which represents
arch/powerpc only, while CONFIG_PPC is set for both.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
Index: linux-work/drivers/ata/pata_sil680.c
===================================================================
drivers/ata/pata_sil680.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- linux-work.orig/drivers/ata/pata_sil680.c 2008-04-09 07:47:23.000000000 +1000
+++ linux-work/drivers/ata/pata_sil680.c 2008-04-09 07:47:29.000000000 +1000
@@ -270,7 +270,7 @@ static u8 sil680_init_chip(struct pci_de
tmpbyte & 1, tmpbyte & 0x30);
*try_mmio = 0;
-#ifdef CONFIG_PPC
+#ifdef CONFIG_PPC_MERGE
if (machine_is(cell))
*try_mmio = (tmpbyte & 1) || pci_resource_start(pdev, 5);
#endif
^ permalink raw reply
* Re: Question on mpc52xx_common.c
From: Segher Boessenkool @ 2008-04-08 21:51 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
In-Reply-To: <fa686aa40804081426u2c2a530ej8ecb6ce9f8fb49a0@mail.gmail.com>
> I disagree and that is not my point. My point is that perfection is
> neither obtainable or necessary.
It's a nice goal though.
> Many of the recently established
> embedded guidelines are not "perfect" because they are counter to a
> few of the OF recommended practices. However, they are consistent
> within themselves, they work, and once established they are not going
> to change.
Yeah. Which is good, even if the bindings themselves aren't
so good.
> Now, if out-of-tree ports continue to break then we've got a problem
> that needs to be fixed. Once a binding is established (which usually
> takes a few kernel releases)
This is a big problem that is totally avoidable.
*Before* accepting any patches that use a new binding (including
patches to .dts files), that binding itself needs to be discussed.
This will be a lot less work than trying to see what's going on in
some platform/device code and/or some example device tree, and
spotting mistakes there. It might be a little more work upfront,
and it might seem like it would increase lead time, but as usual,
it's worth it.
Let's flat out refuse any patch series that uses a non-documented
binding.
> it should be very stable and even if the
> definition of ideal is changed, backwards compatibility must be
> maintained.
Which is a good argument why getting it right the first time is
so important (as with all interfaces, nothing specific about
device trees here).
>> The ARM method of using just a device number is so much easier ...
>
> Only if the assumption is made that very little data needs to be
> shared between the kernel and the firmware.
...which means the kernel has to know *everything* about the hardware
setup and/or what settings the firmware established; i.e., the kernel
has to 100% replace the firmware. This can be nice for some use cases
(it's the route LinuxBIOS took originally), but it simply doesn't
scale, not in any dimension.
Segher
^ permalink raw reply
* ppc440 caches - change proposal [RFC]
From: John Bonesio @ 2008-04-08 22:53 UTC (permalink / raw)
To: Linuxppc-dev
Hi all,
We've discovered something interesting when not using a bootloader such as uboot to start Linux.
The Linux kernel seems to be depending on the bootloader to set up the ppc440 cache control registers, ivlim and dvlim, to set up the Normal and Transient areas in the caches. When a separate bootloader is not used, these cache control registers have random values.
I can think of a couple reasons why somone might not want to use a separate bootloader. Their device could be so small that there's not enough flash to store booth the bootloader, the kernel image, and other things they need in flash. They could want to improve bootup time for their device.
I was thinking it might be good to have the kernel initialize these cache control registers in it's own start up code. Or perhaps this could be done in the kernel's simple bootloader. We could probably put this change in a Xilinx specific startup file, but this change doesn't seem specific to Xilinx FPGA boards.
Thoughts?
- John
^ permalink raw reply
* Re: ppc440 caches - change proposal [RFC]
From: Benjamin Herrenschmidt @ 2008-04-08 22:56 UTC (permalink / raw)
To: John Bonesio; +Cc: Linuxppc-dev
In-Reply-To: <20080408225329.5EC171A8007D@mail11-dub.bigfish.com>
On Tue, 2008-04-08 at 15:53 -0700, John Bonesio wrote:
> I was thinking it might be good to have the kernel initialize these
> cache control registers in it's own start up code. Or perhaps this
> could be done in the kernel's simple bootloader. We could probably put
> this change in a Xilinx specific startup file, but this change doesn't
> seem specific to Xilinx FPGA boards.
The kernel's wrapper would be a good place to put that I suspect. That's
the kind of thing that should be provided as a "library" function to be
optionally called by platform code. Either in the wrapper or the main
kernel platform code.
Cheers,
Ben.
^ permalink raw reply
* Re: ppc440 caches - change proposal [RFC]
From: Grant Likely @ 2008-04-08 23:15 UTC (permalink / raw)
To: benh; +Cc: John Bonesio, Linuxppc-dev
In-Reply-To: <1207695415.14711.11.camel@pasglop>
On Tue, Apr 8, 2008 at 4:56 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> On Tue, 2008-04-08 at 15:53 -0700, John Bonesio wrote:
> > I was thinking it might be good to have the kernel initialize these
> > cache control registers in it's own start up code. Or perhaps this
> > could be done in the kernel's simple bootloader. We could probably put
> > this change in a Xilinx specific startup file, but this change doesn't
> > seem specific to Xilinx FPGA boards.
>
> The kernel's wrapper would be a good place to put that I suspect. That's
> the kind of thing that should be provided as a "library" function to be
> optionally called by platform code. Either in the wrapper or the main
> kernel platform code.
Code is already queued up for 2.6.26 to do exactly this on ppc405
virtex platforms. We can do the same thing for 440. Look at
virtex405-head.S in the following patch:
http://patchwork.ozlabs.org/linuxppc/patch?person=486&id=17410
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [BUG] 2.6.25-rc2-git4 - Regression Kernel oops while running kernbench and tbench on powerpc
From: Paul Mackerras @ 2008-04-08 23:26 UTC (permalink / raw)
To: Kamalesh Babulal
Cc: kernel list, linuxppc-dev, linux-next, nacc, Andrew Morton,
Balbir Singh
In-Reply-To: <47FB5C5A.5020104@linux.vnet.ibm.com>
Kamalesh Babulal writes:
> The kernel oops after applying the patch. Some time it takes more than
> one run to reproduce it, it was reproducible in the second run this
> time.
>
> Unrecoverable exception 4100 at c000000000008c8c
> Oops: Unrecoverable exception, sig: 6 [#1]
> SMP NR_CPUS=128 NUMA pSeries
> Modules linked in:
> NIP: c000000000008c8c LR: 000000000ff0135c CTR: 000000000ff012f0
> REGS: c000000772343bb0 TRAP: 4100 Not tainted (2.6.25-rc8-autotest)
> MSR: 8000000000001030 <ME,IR,DR> CR: 44044228 XER: 00000000
> TASK = c00000077cfa0900[13437] 'cc1' THREAD: c000000772340000 CPU: 2
> GPR00: 0000000000004000 c000000772343e30 00000000000000bb 000000000000d032
> GPR04: 00000000000000bb 0000000000000400 000000000000000a 0000000000000002
> GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> GPR12: 0000000000000000 c000000000734000 0000000000000064 00000000ffe6df08
> GPR16: 00000000105b0000 00000000105b0000 0000000010440000 00000000105b0000
> GPR20: 00000000ffe6e008 00000000105b0000 00000000105b0000 000000000000000a
> GPR24: 000000000ffec408 0000000000000001 00000000ffe6ddca 0000000000000400
> GPR28: 000000000ffec408 00000000f7ff8000 000000000ffebff4 0000000000000400
> NIP [c000000000008c8c] restore+0x8c/0xc0
> LR [000000000ff0135c] 0xff0135c
> Call Trace:
> [c000000772343e30] [c000000000008cd4] do_work+0x14/0x2c (unreliable)
> Instruction dump:
> 7c840078 7c810164 70604000 41820028 60000000 7c4c42e6 e88d01f0 f84d01f0
> 7c841050 e84d01e8 7c422214 f84d01e8 <e9a100d8> 7c7b03a6 e84101a0 7c4ff120
That looks like the bug that was supposed to be fixed by commit
44387e9ff25267c78a99229aca55ed750e9174c7, which is in 2.6.25-rc7 and
later.
What was the SHA1 ID of the head commit for the kernel source that
gave you this oops? Did you have any other patches besides the one I
sent you applied?
Paul.
^ permalink raw reply
* Re: Question on mpc52xx_common.c
From: David Gibson @ 2008-04-08 23:51 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev
In-Reply-To: <47FBD09E.80504@freescale.com>
On Tue, Apr 08, 2008 at 03:07:58PM -0500, Scott Wood wrote:
> Robert Schwebel wrote:
>> Well observed; isn't this the prove of the assumption that the whole
>> device tree idea is not working? It is *always* inconsistent and it is
>> *maintenance hell* because out-of-tree ports do *always* breakt because
>> of string inconsistencies. We have just ported a 8260 board from 2.6.22
>> to 2.6.25 and it is almost 100% oftree porting.
>
> There's going to be more churn in the initial stages than down the road.
> 82xx had barely been added to arch/powerpc in 2.6.22, and there was little
> review of the initial device tree bindings.
>
>> The ARM method of using just a device number is so much easier ...
>
> Yeah, it's so much fun to have to allocate a globally unique number for
> every minor tweak of a board, and to have to maintain a mapping from said
> numbers to information that is semantically equivalent to a device tree but
> in less maintainable form in the kernel source.
And we can already do device numbers if we really want. A bootwrapper
that identifies the board and supplies a device tree essentially does
that, and that way the device tree is maintained in sync with the
kernel.
This is why I've always had mixed feelings about merging device trees
into u-boot, rather than having them supplied by the wrapper.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply
* Re: ppc440 caches - change proposal [RFC]
From: Josh Boyer @ 2008-04-09 0:47 UTC (permalink / raw)
To: Grant Likely; +Cc: John Bonesio, Linuxppc-dev
In-Reply-To: <fa686aa40804081615n284aa750n12ceb0bf1d1bcce1@mail.gmail.com>
On Tue, 8 Apr 2008 17:15:44 -0600
"Grant Likely" <grant.likely@secretlab.ca> wrote:
> On Tue, Apr 8, 2008 at 4:56 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> >
> > On Tue, 2008-04-08 at 15:53 -0700, John Bonesio wrote:
> > > I was thinking it might be good to have the kernel initialize these
> > > cache control registers in it's own start up code. Or perhaps this
> > > could be done in the kernel's simple bootloader. We could probably put
> > > this change in a Xilinx specific startup file, but this change doesn't
> > > seem specific to Xilinx FPGA boards.
> >
> > The kernel's wrapper would be a good place to put that I suspect. That's
> > the kind of thing that should be provided as a "library" function to be
> > optionally called by platform code. Either in the wrapper or the main
> > kernel platform code.
>
> Code is already queued up for 2.6.26 to do exactly this on ppc405
> virtex platforms. We can do the same thing for 440. Look at
> virtex405-head.S in the following patch:
>
> http://patchwork.ozlabs.org/linuxppc/patch?person=486&id=17410
That patch is already in Paul's tree btw.
As for 440, yes it might be good to init the cache registers in the
wrapper.
josh
^ permalink raw reply
* Re: [Cbe-oss-dev] [PATCH] Cell OProfile: SPU mutex lock fix
From: Arnd Bergmann @ 2008-04-09 2:08 UTC (permalink / raw)
To: cbe-oss-dev; +Cc: linuxppc-dev, cel, linux-kernel, Carl Love
In-Reply-To: <1207686116.4734.9.camel@carll-linux-desktop>
On Tuesday 08 April 2008, Carl Love wrote:
> On Fri, 2008-04-04 at 08:38 +0200, Arnd Bergmann wrote:
> Our first thought to fix the bug was to just grab the mutex lock when
> adding the switch notification data to the queue. The kernel gives us
> an oops message saying something along the line of "could not call mutex
> lock in interrupt context". Hence we had to go to work queues so we
> could access the lock outside of the SPU switch notification context.
By the time the notifier is called, the kernel is certainly not
in an atomic context. Maybe you were nesting the mutex lock inside
of your own spinlock?
> Secondly, it is my understanding that if the schedule_work() call tries
> to queue the same work function multiple times the subsequent requests
> are dropped. Thus we were not able to pass the context switch data as
> part of the schedule work requests. This forced us to have an array to
> store the data for each SPU.
The way that work queues are designed, you embed the struct work in the
data you pass down, so that you are guaranteed to execute the work struct
exactly once per data element and you don't need any global pointers.
> > I would guess that the change from hrtimer to delayed_workqueue is
> > smaller than the current patch changing from hrtimer to hrtimer plus
> > workqueue, so I would prefer to have only one changeset.
> >
> > Since the timer only causes statistical data collection anyway, delaying
> > it a bit should not have any negative effect on the accuracy of the
> > measurement, unlike delaying the context switch notification.
>
> The goal is to be able to support very high sampling rates (small
> periods). The schedule_delayed_work() is based on jiffies which I
> believe is 1/250 for this machine. This only gives millisecond
> resolution. The goal is for the users to be able to specify a time
> period of 60,000 cycles or less then 20 micro second sampling periods
> when the real high resolution timers are available. We can't achieve
> the desired sampling rates with the schedule_dealyed_work() function.
You actually can't get anywhere close to that resolution if you do your
data collection in a work queue, because under high load (which is what
the only time when measuring really gets interesting) the work queue
is likely to be delayed by a few jiffies!
If you rely on high resolution timer precision, you need to look
at the performance counters from inside the timer function, and deal
with the problem of the work queue not being called for a number of
timer events.
>
> Oprofile provides nice clean interfaces for recording kernel/user
> switches and CPU data recording. This is all that was needed by any
> architecture until CELL came along. With CELL, we now have need to add
> processor data plus SPU data to the queue. The buffer_mutex variable
> and the add_event_entry() were not visible outside of the OProfile
> driver code. The original SPU support added add_event_entry() to the
> include/linux/oprofile.h file. We can add the buffer_mutex as well
> since there is now a need to access both of these.
>
> I have been looking to see how I could create a generic oprofile routine
> which could take the data. The routine would still have to work from an
> interrupt context, so it will need to store the data and call a work
> queue function. The function would need to know how much data will be
> needed, thus you would probably need to statically allocate data or use
> a list and malloc the data as needed. I don't really want to have to
> malloc data from an interrupt context. List management adds additional
> overhead. It would be possible to have an init function that you could
> call at startup time telling it how much memory you need, in this case
> we could allocate a buffer the size of spu_info (defined below) at
> startup time. The call could pass an array to the OProfile routine that
> would put the data into the buffer and call the work function. We still
> have to allocate the storage, it does clean up the arch specific code.
> Not sure if this really buys us much. There is more copying of data
> i.e. more overhead. Not convinced the OProfile maintainers would accept
> anything I have thought of so far.
>
> Any suggestions?
My first attempt to do this would be to add this to the oprofile/cpu_buffer.c
infrastructure. Basically extend the add_sample() function to have
helpers you can call from the spu code to put entries into the per-cpu
buffer of the CPU that happens to execute the code at the time.
add_sample() can already be called from an atomic context since it uses
its own buffer, and it uses a clever ring buffer to get around the
need for locking against the event_buffer functions.
Only event_buffer needs the mutex, so it's best to leave that out
of the architecture code running at interrupt time altogether.
> > An ideal driver should not have *any* global variables at all, but store
> > all data in the (reference counted) objects it is dealing with, or
> > just on the stack while it's processing the data.
> >
> > Storing the context switch information in a global breaks down as soon
> > as there are multiple context switches taking place for a single
> > SPU without the workqueue running in between, which is a very likely
> > scenario if you have high-priority tasks on the SPU.
>
> Yes, it was recognized that we could overwrite data. The expectation is
> that the workqueue function will run fairly quickly. If the SPU context
> was only loaded for a very short period of time at most a few samples
> would be captured for that frame so the error would be down in the
> noise. OProfile is a statistical tool. If we don't get enough samples
> for the spu context, then the data is not statistically valid. Losing
> the context switch is not an issue. The context must be loaded long
> enough that we collect a reasonable number of samples for the output to
> be meaningful.
Hmm, I wonder if you have a problem here when the context switch time
is not independent of the program you are analysing. Usually, context
switches happen when a program does a callback or syscall to the ppu
and the spu becomes idle. If you often lose events just after a context
switch, that means that the code that gets called after the context
is restored shows up in the profile as being called less often than
it actually is, right?
> > The patch is already far bigger than a simple bug fix, but you're right
> > in that this part should be separate. Upon reading through the code
> > again, I noticed that the cached_info is thrown away on each context
> > switch and rebuilt, which I guess makes it impossible to really profile
> > the context switch code. In the initial design phase for spu oprofile, we
> > decided that the information should be cached in the spu_context, which
> > would not only be much cleaner but also avoid the performance problem.
> >
> > Do you have any idea why this idea was dropped? Is it still work in
> > progress to get that functionality right?
>
> It was not dropped. It was implemented. The issue that I have is the
> dcookie spu_cookie, app_dcookie, offset, objectId is not included in the
> local cahced spu context data. Previously, there was no need to save it
> since it was being immediatly written to oprofile buffer (without
> holding the mutex lock). Now we need to store the data until the
> workqueue function runs. We can store it in the array as I have done or
> you could put it in the spu context. Functionally, it doesn't change
> anything. The data in the SPU context would get overwritten, just as it
> does in the array, if there was an SPU context switch before the
> workqueue function runs so that doesn't solve that issue.
That's not what I meant. My point was that the information about the
location of overlays in the context remains constant over the contexts
life time. If you store it in the context, all the data will still
be there by the time you restore the context when it was switched
out and comes back.
Arnd <><
^ permalink raw reply
* [PATCH] [POWERPC] Make pci_bus_to_host()'s struct pci_bus * argument const
From: Trent Piepho @ 2008-04-09 2:19 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Trent Piepho
Why? Because:
A) It's not modified and so it can be made const. const is good.
B) If one has a function that was given a const pci_bus pointer and you
want to get a pointer to its pci_controller, you'll get a warning from gcc
when you use pci_bus_to_host(). This is the right way to stop that
warning.
Signed-off-by: Trent Piepho <tpiepho@freescale.com>
---
include/asm-powerpc/pci-bridge.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/asm-powerpc/pci-bridge.h b/include/asm-powerpc/pci-bridge.h
index e5802c6..b95d033 100644
--- a/include/asm-powerpc/pci-bridge.h
+++ b/include/asm-powerpc/pci-bridge.h
@@ -117,7 +117,7 @@ struct pci_controller {
#ifndef CONFIG_PPC64
-static inline struct pci_controller *pci_bus_to_host(struct pci_bus *bus)
+static inline struct pci_controller *pci_bus_to_host(const struct pci_bus *bus)
{
return bus->sysdata;
}
@@ -235,7 +235,7 @@ extern void pcibios_fixup_new_pci_devices(struct pci_bus *bus);
extern int pcibios_remove_root_bus(struct pci_controller *phb);
-static inline struct pci_controller *pci_bus_to_host(struct pci_bus *bus)
+static inline struct pci_controller *pci_bus_to_host(const struct pci_bus *bus)
{
struct device_node *busdn = bus->sysdata;
--
1.5.4.1
^ permalink raw reply related
* Re: 4xx defconfig reorg
From: Josh Boyer @ 2008-04-09 2:53 UTC (permalink / raw)
To: Kumar Gala; +Cc: linuxppc-dev, paulus
In-Reply-To: <73BD1263-F09B-4E08-AA77-BEBD67D91C58@kernel.crashing.org>
On Mon, 2008-04-07 at 16:36 -0500, Kumar Gala wrote:
> On Apr 6, 2008, at 8:06 AM, Josh Boyer wrote:
> > Hi All,
> >
> > Unless someone screams loudly and has reasons why this shouldn't go
> > in,
> > the following commit should hit the 4xx next branch in the next day or
> > so.
> >
> > josh
>
> If this is acceptable to people, I'll make similar defconfig movements
> for the Freescale parts.
I've not heard any objections. I'll be pushing my commit to my next
branch tomorrow morning.
josh
^ permalink raw reply
* Re: [PATCH] [POWERPC] Make pci_bus_to_host()'s struct pci_bus * argument const
From: Benjamin Herrenschmidt @ 2008-04-09 3:44 UTC (permalink / raw)
To: Trent Piepho; +Cc: linuxppc-dev
In-Reply-To: <1207707572-24364-1-git-send-email-tpiepho@freescale.com>
On Tue, 2008-04-08 at 19:19 -0700, Trent Piepho wrote:
> Why? Because:
> A) It's not modified and so it can be made const. const is good.
> B) If one has a function that was given a const pci_bus pointer and you
> want to get a pointer to its pci_controller, you'll get a warning from gcc
> when you use pci_bus_to_host(). This is the right way to stop that
> warning.
>
> Signed-off-by: Trent Piepho <tpiepho@freescale.com>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> include/asm-powerpc/pci-bridge.h | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/asm-powerpc/pci-bridge.h b/include/asm-powerpc/pci-bridge.h
> index e5802c6..b95d033 100644
> --- a/include/asm-powerpc/pci-bridge.h
> +++ b/include/asm-powerpc/pci-bridge.h
> @@ -117,7 +117,7 @@ struct pci_controller {
>
> #ifndef CONFIG_PPC64
>
> -static inline struct pci_controller *pci_bus_to_host(struct pci_bus *bus)
> +static inline struct pci_controller *pci_bus_to_host(const struct pci_bus *bus)
> {
> return bus->sysdata;
> }
> @@ -235,7 +235,7 @@ extern void pcibios_fixup_new_pci_devices(struct pci_bus *bus);
>
> extern int pcibios_remove_root_bus(struct pci_controller *phb);
>
> -static inline struct pci_controller *pci_bus_to_host(struct pci_bus *bus)
> +static inline struct pci_controller *pci_bus_to_host(const struct pci_bus *bus)
> {
> struct device_node *busdn = bus->sysdata;
>
^ permalink raw reply
* Re: [BUG] 2.6.25-rc2-git4 - Regression Kernel oops while running kernbench and tbench on powerpc
From: Kamalesh Babulal @ 2008-04-09 5:20 UTC (permalink / raw)
To: Paul Mackerras
Cc: kernel list, linuxppc-dev, linux-next, nacc, Andrew Morton,
Balbir Singh
In-Reply-To: <18427.65299.403151.344959@cargo.ozlabs.ibm.com>
Paul Mackerras wrote:
> Kamalesh Babulal writes:
>
>> The kernel oops after applying the patch. Some time it takes more than
>> one run to reproduce it, it was reproducible in the second run this
>> time.
>>
>> Unrecoverable exception 4100 at c000000000008c8c
>> Oops: Unrecoverable exception, sig: 6 [#1]
>> SMP NR_CPUS=128 NUMA pSeries
>> Modules linked in:
>> NIP: c000000000008c8c LR: 000000000ff0135c CTR: 000000000ff012f0
>> REGS: c000000772343bb0 TRAP: 4100 Not tainted (2.6.25-rc8-autotest)
>> MSR: 8000000000001030 <ME,IR,DR> CR: 44044228 XER: 00000000
>> TASK = c00000077cfa0900[13437] 'cc1' THREAD: c000000772340000 CPU: 2
>> GPR00: 0000000000004000 c000000772343e30 00000000000000bb 000000000000d032
>> GPR04: 00000000000000bb 0000000000000400 000000000000000a 0000000000000002
>> GPR08: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> GPR12: 0000000000000000 c000000000734000 0000000000000064 00000000ffe6df08
>> GPR16: 00000000105b0000 00000000105b0000 0000000010440000 00000000105b0000
>> GPR20: 00000000ffe6e008 00000000105b0000 00000000105b0000 000000000000000a
>> GPR24: 000000000ffec408 0000000000000001 00000000ffe6ddca 0000000000000400
>> GPR28: 000000000ffec408 00000000f7ff8000 000000000ffebff4 0000000000000400
>> NIP [c000000000008c8c] restore+0x8c/0xc0
>> LR [000000000ff0135c] 0xff0135c
>> Call Trace:
>> [c000000772343e30] [c000000000008cd4] do_work+0x14/0x2c (unreliable)
>> Instruction dump:
>> 7c840078 7c810164 70604000 41820028 60000000 7c4c42e6 e88d01f0 f84d01f0
>> 7c841050 e84d01e8 7c422214 f84d01e8 <e9a100d8> 7c7b03a6 e84101a0 7c4ff120
>
> That looks like the bug that was supposed to be fixed by commit
> 44387e9ff25267c78a99229aca55ed750e9174c7, which is in 2.6.25-rc7 and
> later.
>
> What was the SHA1 ID of the head commit for the kernel source that
> gave you this oops? Did you have any other patches besides the one I
> sent you applied?
>
> Paul.
The SHA1 ID of the kernel is 0e81a8ae37687845f7cdfa2adce14ea6a5f1dd34 (2.6.25-rc8)
and the source seems to have the patch 44387e9ff25267c78a99229aca55ed750e9174c7.
The kernel was patched only the patch you gave me (http://lkml.org/lkml/2008/4/8/42).
--
Thanks & Regards,
Kamalesh Babulal,
Linux Technology Center,
IBM, ISTL.
^ permalink raw reply
* Re: Question on mpc52xx_common.c
From: Wolfgang Denk @ 2008-04-09 6:18 UTC (permalink / raw)
To: David Gibson; +Cc: Scott Wood, linuxppc-dev
In-Reply-To: <20080408235150.GA8092@localhost.localdomain>
In message <20080408235150.GA8092@localhost.localdomain> you wrote:
>
> This is why I've always had mixed feelings about merging device trees
> into u-boot, rather than having them supplied by the wrapper.
On the other hand, we can now use the device tree to dynamically
configure U-Boot, thus allowing to run the same binary image of
U-Boot on several different hardware configurations.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Parkinson's Law: Work expands to fill the time alloted it.
^ permalink raw reply
* Re: 4xx defconfig reorg
From: Paul Mackerras @ 2008-04-09 6:22 UTC (permalink / raw)
To: jwboyer; +Cc: linuxppc-dev
In-Reply-To: <1207709608.3996.0.camel@vader.jdub.homelinux.org>
Josh Boyer writes:
> I've not heard any objections. I'll be pushing my commit to my next
> branch tomorrow morning.
I don't have any objections. I wonder if Sam Ravnborg responded to
Segher's patch to extend make help?
Paul.
^ permalink raw reply
* [PATCH 1/6] [POWERPC] Initialize paca->current earlier
From: Benjamin Herrenschmidt @ 2008-04-09 7:21 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev
Currently, we initialize the "current" pointer in the PACA (which
is used by the "current" macro in the kernel) before calling
setup_system(). That means that early_setup() is called with
current still "NULL" which is -not- a good idea. It happens to
work so far but breaks with lockdep when early code calls printk.
This changes it so that all PACAs are statically initialized with
__current pointing to the init task. For non-0 CPUs, this is fixed
up before use.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/kernel/head_64.S | 4 ----
arch/powerpc/kernel/paca.c | 3 ++-
2 files changed, 2 insertions(+), 5 deletions(-)
--- linux-work.orig/arch/powerpc/kernel/head_64.S 2008-04-02 18:20:53.000000000 +1100
+++ linux-work/arch/powerpc/kernel/head_64.S 2008-04-02 18:24:45.000000000 +1100
@@ -1505,10 +1505,6 @@ _INIT_GLOBAL(start_here_common)
li r0,0
stdu r0,-STACK_FRAME_OVERHEAD(r1)
- /* ptr to current */
- LOAD_REG_IMMEDIATE(r4, init_task)
- std r4,PACACURRENT(r13)
-
/* Load the TOC */
ld r2,PACATOC(r13)
std r1,PACAKSAVE(r13)
Index: linux-work/arch/powerpc/kernel/paca.c
===================================================================
--- linux-work.orig/arch/powerpc/kernel/paca.c 2008-04-02 18:28:07.000000000 +1100
+++ linux-work/arch/powerpc/kernel/paca.c 2008-04-02 18:28:22.000000000 +1100
@@ -72,7 +72,8 @@ struct slb_shadow slb_shadow[] __cacheli
.paca_index = (number), /* Paca Index */ \
.kernel_toc = (unsigned long)(&__toc_start) + 0x8000UL, \
.hw_cpu_id = 0xffff, \
- .slb_shadow_ptr = &slb_shadow[number],
+ .slb_shadow_ptr = &slb_shadow[number], \
+ .__current = &init_task,
#ifdef CONFIG_PPC_ISERIES
#define PACA_INIT_ISERIES(number) \
^ permalink raw reply
* [PATCH 2/6] [POWERPC] lockdep stacktrace support
From: Benjamin Herrenschmidt @ 2008-04-09 7:21 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev
From: Christoph Hellwig <hch@lst.de>
I recently tried to work on lockdep for powerpc. I have preliminary
version of the stacktrace code, but had to give up on trace irqflags
support because I'm not that knowledgeable on lowlevel ppc details.
Maybe someone more faimilar with the code wants to give it another try?
My stacktrace code is below:
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/Kconfig | 4 +++
arch/powerpc/kernel/Makefile | 1
arch/powerpc/kernel/stacktrace.c | 52 +++++++++++++++++++++++++++++++++++++++
3 files changed, 57 insertions(+)
--- linux-work.orig/arch/powerpc/Kconfig 2008-04-02 15:46:07.000000000 +1100
+++ linux-work/arch/powerpc/Kconfig 2008-04-02 16:47:46.000000000 +1100
@@ -49,6 +49,10 @@ config IRQ_PER_CPU
bool
default y
+config STACKTRACE_SUPPORT
+ bool
+ default y
+
config RWSEM_GENERIC_SPINLOCK
bool
Index: linux-work/arch/powerpc/kernel/Makefile
===================================================================
--- linux-work.orig/arch/powerpc/kernel/Makefile 2008-04-02 15:46:07.000000000 +1100
+++ linux-work/arch/powerpc/kernel/Makefile 2008-04-02 16:46:07.000000000 +1100
@@ -67,6 +67,7 @@ obj-$(CONFIG_BOOTX_TEXT) += btext.o
obj-$(CONFIG_SMP) += smp.o
obj-$(CONFIG_KPROBES) += kprobes.o
obj-$(CONFIG_PPC_UDBG_16550) += legacy_serial.o udbg_16550.o
+obj-$(CONFIG_STACKTRACE) += stacktrace.o
pci64-$(CONFIG_PPC64) += pci_dn.o isa-bridge.o
obj-$(CONFIG_PCI) += pci_$(CONFIG_WORD_SIZE).o $(pci64-y) \
Index: linux-work/arch/powerpc/kernel/stacktrace.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-work/arch/powerpc/kernel/stacktrace.c 2008-04-02 16:46:07.000000000 +1100
@@ -0,0 +1,52 @@
+
+#include <linux/sched.h>
+#include <linux/stacktrace.h>
+
+
+#ifdef CONFIG_PPC64
+#define MIN_STACK_FRAME 112 /* same as STACK_FRAME_OVERHEAD, in fact */
+#define FRAME_LR_SAVE 2
+#define INT_FRAME_SIZE (sizeof(struct pt_regs) + STACK_FRAME_OVERHEAD + 288)
+#define REGS_MARKER 0x7265677368657265ul
+#define FRAME_MARKER 12
+#else
+#define MIN_STACK_FRAME 16
+#define FRAME_LR_SAVE 1
+#define INT_FRAME_SIZE (sizeof(struct pt_regs) + STACK_FRAME_OVERHEAD)
+#define REGS_MARKER 0x72656773ul
+#define FRAME_MARKER 2
+#endif
+
+
+/*
+ * Save stack-backtrace addresses into a stack_trace buffer.
+ * If all_contexts is set, all contexts (hardirq, softirq and process)
+ * are saved. If not set then only the current context is saved.
+ */
+void save_stack_trace(struct stack_trace *trace)
+{
+ unsigned long sp;
+
+ asm("mr %0,1" : "=r" (sp));
+
+ for (;;) {
+ unsigned long *stack = (unsigned long *) sp;
+ unsigned long newsp, ip;
+
+ if (!validate_sp(sp, current, MIN_STACK_FRAME))
+ return;
+
+ newsp = stack[0];
+ ip = stack[FRAME_LR_SAVE];
+
+ if (!trace->skip)
+ trace->entries[trace->nr_entries++] = ip;
+ else
+ trace->skip--;
+
+ if (trace->nr_entries >= trace->max_entries)
+ return;
+
+ sp = newsp;
+ }
+}
^ permalink raw reply
* [PATCH 3/6] [POWERPC] Fixup softirq preempt count
From: Benjamin Herrenschmidt @ 2008-04-09 7:21 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev
This fixes the handling of the preempt count when switching
interrupt stacks so that HW interrupt properly get the softirq
mask copied over from the previous stack.
It also initializes the softirq stack preempt_count to 0 instead
of SOFTIRQ_OFFSET, like x86, as __do_softirq() does the increment,
and we hit some lockdep checks if we have it twice.
That means we do run for a little while off the softirq stack
with the preempt-count set to 0, which could be deadly if we
try to take a softirq at that point, however we do so with
interrupts disabled, so I think we are ok.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/kernel/irq.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
--- linux-work.orig/arch/powerpc/kernel/irq.c 2008-04-02 16:48:58.000000000 +1100
+++ linux-work/arch/powerpc/kernel/irq.c 2008-04-02 16:49:36.000000000 +1100
@@ -310,8 +310,21 @@ void do_IRQ(struct pt_regs *regs)
handler = &__do_IRQ;
irqtp->task = curtp->task;
irqtp->flags = 0;
+
+ /* Copy the softirq bits in preempt_count so that the
+ * softirq checks work in the hardirq context.
+ */
+ irqtp->preempt_count =
+ (irqtp->preempt_count & ~SOFTIRQ_MASK) |
+ (curtp->preempt_count & SOFTIRQ_MASK);
+
call_handle_irq(irq, desc, irqtp, handler);
irqtp->task = NULL;
+
+
+ /* Set any flag that may have been set on the
+ * alternate stack
+ */
if (irqtp->flags)
set_bits(irqtp->flags, &curtp->flags);
} else
@@ -357,7 +370,7 @@ void irq_ctx_init(void)
memset((void *)softirq_ctx[i], 0, THREAD_SIZE);
tp = softirq_ctx[i];
tp->cpu = i;
- tp->preempt_count = SOFTIRQ_OFFSET;
+ tp->preempt_count = 0;
memset((void *)hardirq_ctx[i], 0, THREAD_SIZE);
tp = hardirq_ctx[i];
^ permalink raw reply
* [PATCH 4/6] [POWERPC] irqtrace support to 64-bit powerpc
From: Benjamin Herrenschmidt @ 2008-04-09 7:21 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev
This adds the low level irq tracing hooks to the powerpc architecture
needed to enable full lockdep functionality
Partly based on Johannes Berg's initial version, removing the asm
trampoline that isn't needed (thus improving perfs) and all sorts
of bits and pieces, reworking most of the assembly, etc...
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/Kconfig | 9 +++++++
arch/powerpc/kernel/entry_64.S | 27 +++++++++++++++++++++-
arch/powerpc/kernel/head_64.S | 47 +++++++++++++++++++++++++++-------------
arch/powerpc/kernel/irq.c | 3 +-
arch/powerpc/kernel/ppc_ksyms.c | 4 ---
arch/powerpc/kernel/setup_64.c | 4 +++
include/asm-powerpc/exception.h | 6 ++---
include/asm-powerpc/hw_irq.h | 13 +++++------
include/asm-powerpc/irqflags.h | 37 +++++++++++++++++++++----------
include/asm-powerpc/rwsem.h | 35 ++++++++++++++++++++++-------
include/asm-powerpc/spinlock.h | 1
11 files changed, 134 insertions(+), 52 deletions(-)
--- linux-work.orig/arch/powerpc/Kconfig 2008-04-02 18:30:18.000000000 +1100
+++ linux-work/arch/powerpc/Kconfig 2008-04-02 18:30:21.000000000 +1100
@@ -53,6 +53,15 @@ config STACKTRACE_SUPPORT
bool
default y
+config TRACE_IRQFLAGS_SUPPORT
+ bool
+ depends on PPC64
+ default y
+
+config LOCKDEP_SUPPORT
+ bool
+ default y
+
config RWSEM_GENERIC_SPINLOCK
bool
Index: linux-work/arch/powerpc/kernel/irq.c
===================================================================
--- linux-work.orig/arch/powerpc/kernel/irq.c 2008-04-02 18:30:20.000000000 +1100
+++ linux-work/arch/powerpc/kernel/irq.c 2008-04-02 18:30:21.000000000 +1100
@@ -114,7 +114,7 @@ static inline void set_soft_enabled(unsi
: : "r" (enable), "i" (offsetof(struct paca_struct, soft_enabled)));
}
-void local_irq_restore(unsigned long en)
+void raw_local_irq_restore(unsigned long en)
{
/*
* get_paca()->soft_enabled = en;
@@ -174,6 +174,7 @@ void local_irq_restore(unsigned long en)
__hard_irq_enable();
}
+EXPORT_SYMBOL(raw_local_irq_restore);
#endif /* CONFIG_PPC64 */
int show_interrupts(struct seq_file *p, void *v)
Index: linux-work/arch/powerpc/kernel/ppc_ksyms.c
===================================================================
--- linux-work.orig/arch/powerpc/kernel/ppc_ksyms.c 2008-04-02 18:11:56.000000000 +1100
+++ linux-work/arch/powerpc/kernel/ppc_ksyms.c 2008-04-02 18:30:21.000000000 +1100
@@ -45,10 +45,6 @@
#include <asm/signal.h>
#include <asm/dcr.h>
-#ifdef CONFIG_PPC64
-EXPORT_SYMBOL(local_irq_restore);
-#endif
-
#ifdef CONFIG_PPC32
extern void transfer_to_handler(void);
extern void do_IRQ(struct pt_regs *regs);
Index: linux-work/include/asm-powerpc/hw_irq.h
===================================================================
--- linux-work.orig/include/asm-powerpc/hw_irq.h 2008-04-02 18:11:56.000000000 +1100
+++ linux-work/include/asm-powerpc/hw_irq.h 2008-04-02 18:30:21.000000000 +1100
@@ -27,7 +27,7 @@ static inline unsigned long local_get_fl
return flags;
}
-static inline unsigned long local_irq_disable(void)
+static inline unsigned long raw_local_irq_disable(void)
{
unsigned long flags, zero;
@@ -39,14 +39,15 @@ static inline unsigned long local_irq_di
return flags;
}
-extern void local_irq_restore(unsigned long);
+extern void raw_local_irq_restore(unsigned long);
extern void iseries_handle_interrupts(void);
-#define local_irq_enable() local_irq_restore(1)
-#define local_save_flags(flags) ((flags) = local_get_flags())
-#define local_irq_save(flags) ((flags) = local_irq_disable())
+#define raw_local_irq_enable() raw_local_irq_restore(1)
+#define raw_local_save_flags(flags) ((flags) = local_get_flags())
+#define raw_local_irq_save(flags) ((flags) = raw_local_irq_disable())
-#define irqs_disabled() (local_get_flags() == 0)
+#define raw_irqs_disabled() (local_get_flags() == 0)
+#define raw_irqs_disabled_flags(flags) ((flags) == 0)
#define __hard_irq_enable() __mtmsrd(mfmsr() | MSR_EE, 1)
#define __hard_irq_disable() __mtmsrd(mfmsr() & ~MSR_EE, 1)
Index: linux-work/include/asm-powerpc/irqflags.h
===================================================================
--- linux-work.orig/include/asm-powerpc/irqflags.h 2008-04-02 18:11:56.000000000 +1100
+++ linux-work/include/asm-powerpc/irqflags.h 2008-04-02 18:30:21.000000000 +1100
@@ -2,30 +2,43 @@
* include/asm-powerpc/irqflags.h
*
* IRQ flags handling
- *
- * This file gets included from lowlevel asm headers too, to provide
- * wrapped versions of the local_irq_*() APIs, based on the
- * raw_local_irq_*() macros from the lowlevel headers.
*/
#ifndef _ASM_IRQFLAGS_H
#define _ASM_IRQFLAGS_H
+#ifndef __ASSEMBLY__
/*
* Get definitions for raw_local_save_flags(x), etc.
*/
#include <asm-powerpc/hw_irq.h>
+#else
+#ifdef CONFIG_TRACE_IRQFLAGS
/*
- * Do the CPU's IRQ-state tracing from assembly code. We call a
- * C function, so save all the C-clobbered registers:
+ * Most of the CPU's IRQ-state tracing is done from assembly code; we
+ * have to call a C function so call a wrapper that saves all the
+ * C-clobbered registers.
*/
-#ifdef CONFIG_TRACE_IRQFLAGS
-
-#error No support on PowerPC yet for CONFIG_TRACE_IRQFLAGS
-
+#define TRACE_ENABLE_INTS bl .trace_hardirqs_on
+#define TRACE_DISABLE_INTS bl .trace_hardirqs_off
+#define TRACE_AND_RESTORE_IRQ_PARTIAL(en,skip) \
+ cmpdi en, 0; \
+ bne 95f; \
+ stb en,PACASOFTIRQEN(r13); \
+ bl .trace_hardirqs_off; \
+ b skip; \
+95: bl .trace_hardirqs_on; \
+ li en,1;
+#define TRACE_AND_RESTORE_IRQ(en) \
+ TRACE_AND_RESTORE_IRQ_PARTIAL(en,96f); \
+96: stb en,PACASOFTIRQEN(r13)
#else
-# define TRACE_IRQS_ON
-# define TRACE_IRQS_OFF
+#define TRACE_ENABLE_INTS
+#define TRACE_DISABLE_INTS
+#define TRACE_AND_RESTORE_IRQ_PARTIAL(en,skip)
+#define TRACE_AND_RESTORE_IRQ(en) \
+ stb en,PACASOFTIRQEN(r13)
+#endif
#endif
#endif
Index: linux-work/include/asm-powerpc/rwsem.h
===================================================================
--- linux-work.orig/include/asm-powerpc/rwsem.h 2008-04-02 18:11:56.000000000 +1100
+++ linux-work/include/asm-powerpc/rwsem.h 2008-04-02 18:30:21.000000000 +1100
@@ -32,11 +32,20 @@ struct rw_semaphore {
#define RWSEM_ACTIVE_WRITE_BIAS (RWSEM_WAITING_BIAS + RWSEM_ACTIVE_BIAS)
spinlock_t wait_lock;
struct list_head wait_list;
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+ struct lockdep_map dep_map;
+#endif
};
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+# define __RWSEM_DEP_MAP_INIT(lockname) , .dep_map = { .name = #lockname }
+#else
+# define __RWSEM_DEP_MAP_INIT(lockname)
+#endif
+
#define __RWSEM_INITIALIZER(name) \
- { RWSEM_UNLOCKED_VALUE, SPIN_LOCK_UNLOCKED, \
- LIST_HEAD_INIT((name).wait_list) }
+ { RWSEM_UNLOCKED_VALUE, __SPIN_LOCK_UNLOCKED((name).wait_lock), \
+ LIST_HEAD_INIT((name).wait_list) __RWSEM_DEP_MAP_INIT(name) }
#define DECLARE_RWSEM(name) \
struct rw_semaphore name = __RWSEM_INITIALIZER(name)
@@ -46,12 +55,15 @@ extern struct rw_semaphore *rwsem_down_w
extern struct rw_semaphore *rwsem_wake(struct rw_semaphore *sem);
extern struct rw_semaphore *rwsem_downgrade_wake(struct rw_semaphore *sem);
-static inline void init_rwsem(struct rw_semaphore *sem)
-{
- sem->count = RWSEM_UNLOCKED_VALUE;
- spin_lock_init(&sem->wait_lock);
- INIT_LIST_HEAD(&sem->wait_list);
-}
+extern void __init_rwsem(struct rw_semaphore *sem, const char *name,
+ struct lock_class_key *key);
+
+#define init_rwsem(sem) \
+ do { \
+ static struct lock_class_key __key; \
+ \
+ __init_rwsem((sem), #sem, &__key); \
+ } while (0)
/*
* lock for reading
@@ -78,7 +90,7 @@ static inline int __down_read_trylock(st
/*
* lock for writing
*/
-static inline void __down_write(struct rw_semaphore *sem)
+static inline void __down_write_nested(struct rw_semaphore *sem, int subclass)
{
int tmp;
@@ -88,6 +100,11 @@ static inline void __down_write(struct r
rwsem_down_write_failed(sem);
}
+static inline void __down_write(struct rw_semaphore *sem)
+{
+ __down_write_nested(sem, 0);
+}
+
static inline int __down_write_trylock(struct rw_semaphore *sem)
{
int tmp;
Index: linux-work/include/asm-powerpc/spinlock.h
===================================================================
--- linux-work.orig/include/asm-powerpc/spinlock.h 2008-04-02 18:11:56.000000000 +1100
+++ linux-work/include/asm-powerpc/spinlock.h 2008-04-02 18:30:21.000000000 +1100
@@ -19,6 +19,7 @@
*
* (the type definitions are in asm/spinlock_types.h)
*/
+#include <linux/irqflags.h>
#ifdef CONFIG_PPC64
#include <asm/paca.h>
#include <asm/hvcall.h>
Index: linux-work/include/asm-powerpc/exception.h
===================================================================
--- linux-work.orig/include/asm-powerpc/exception.h 2008-04-02 18:11:56.000000000 +1100
+++ linux-work/include/asm-powerpc/exception.h 2008-04-02 18:30:21.000000000 +1100
@@ -228,18 +228,18 @@ label##_pSeries: \
BEGIN_FW_FTR_SECTION; \
stb r11,PACAHARDIRQEN(r13); \
END_FW_FTR_SECTION_IFCLR(FW_FEATURE_ISERIES); \
+ TRACE_DISABLE_INTS; \
BEGIN_FW_FTR_SECTION; \
mfmsr r10; \
ori r10,r10,MSR_EE; \
mtmsrd r10,1; \
END_FW_FTR_SECTION_IFSET(FW_FEATURE_ISERIES)
-
#else
#define DISABLE_INTS \
li r11,0; \
stb r11,PACASOFTIRQEN(r13); \
- stb r11,PACAHARDIRQEN(r13)
-
+ stb r11,PACAHARDIRQEN(r13); \
+ TRACE_DISABLE_INTS
#endif /* CONFIG_PPC_ISERIES */
#define ENABLE_INTS \
Index: linux-work/arch/powerpc/kernel/head_64.S
===================================================================
--- linux-work.orig/arch/powerpc/kernel/head_64.S 2008-04-02 18:24:45.000000000 +1100
+++ linux-work/arch/powerpc/kernel/head_64.S 2008-04-02 18:30:21.000000000 +1100
@@ -36,8 +36,7 @@
#include <asm/firmware.h>
#include <asm/page_64.h>
#include <asm/exception.h>
-
-#define DO_SOFT_DISABLE
+#include <asm/irqflags.h>
/*
* We layout physical memory as follows:
@@ -450,8 +449,8 @@ bad_stack:
*/
fast_exc_return_irq: /* restores irq state too */
ld r3,SOFTE(r1)
+ TRACE_AND_RESTORE_IRQ(r3);
ld r12,_MSR(r1)
- stb r3,PACASOFTIRQEN(r13) /* restore paca->soft_enabled */
rldicl r4,r12,49,63 /* get MSR_EE to LSB */
stb r4,PACAHARDIRQEN(r13) /* restore paca->hard_enabled */
b 1f
@@ -808,7 +807,7 @@ _STATIC(load_up_altivec)
* Hash table stuff
*/
.align 7
-_GLOBAL(do_hash_page)
+_STATIC(do_hash_page)
std r3,_DAR(r1)
std r4,_DSISR(r1)
@@ -820,6 +819,27 @@ BEGIN_FTR_SECTION
END_FTR_SECTION_IFCLR(CPU_FTR_SLB)
/*
+ * On iSeries, we soft-disable interrupts here, then
+ * hard-enable interrupts so that the hash_page code can spin on
+ * the hash_table_lock without problems on a shared processor.
+ */
+ DISABLE_INTS
+
+ /*
+ * Currently, trace_hardirqs_off() will be called by DISABLE_INTS
+ * and will clobber volatile registers when irq tracing is enabled
+ * so we need to reload them. It may be possible to be smarter here
+ * and move the irq tracing elsewhere but let's keep it simple for
+ * now
+ */
+#ifdef CONFIG_TRACE_IRQFLAGS
+ ld r3,_DAR(r1)
+ ld r4,_DSISR(r1)
+ ld r5,_TRAP(r1)
+ ld r12,_MSR(r1)
+ clrrdi r5,r5,4
+#endif /* CONFIG_TRACE_IRQFLAGS */
+ /*
* We need to set the _PAGE_USER bit if MSR_PR is set or if we are
* accessing a userspace segment (even from the kernel). We assume
* kernel addresses always have the high bit set.
@@ -832,13 +852,6 @@ END_FTR_SECTION_IFCLR(CPU_FTR_SLB)
rlwimi r4,r5,22+2,31-2,31-2 /* Set _PAGE_EXEC if trap is 0x400 */
/*
- * On iSeries, we soft-disable interrupts here, then
- * hard-enable interrupts so that the hash_page code can spin on
- * the hash_table_lock without problems on a shared processor.
- */
- DISABLE_INTS
-
- /*
* r3 contains the faulting address
* r4 contains the required access permissions
* r5 contains the trap number
@@ -848,7 +861,6 @@ END_FTR_SECTION_IFCLR(CPU_FTR_SLB)
bl .hash_page /* build HPTE if possible */
cmpdi r3,0 /* see if hash_page succeeded */
-#ifdef DO_SOFT_DISABLE
BEGIN_FW_FTR_SECTION
/*
* If we had interrupts soft-enabled at the point where the
@@ -860,7 +872,7 @@ BEGIN_FW_FTR_SECTION
*/
beq 13f
END_FW_FTR_SECTION_IFSET(FW_FEATURE_ISERIES)
-#endif
+
BEGIN_FW_FTR_SECTION
/*
* Here we have interrupts hard-disabled, so it is sufficient
@@ -874,11 +886,12 @@ END_FW_FTR_SECTION_IFCLR(FW_FEATURE_ISER
/*
* hash_page couldn't handle it, set soft interrupt enable back
- * to what it was before the trap. Note that .local_irq_restore
+ * to what it was before the trap. Note that .raw_local_irq_restore
* handles any interrupts pending at this point.
*/
ld r3,SOFTE(r1)
- bl .local_irq_restore
+ TRACE_AND_RESTORE_IRQ_PARTIAL(r3, 11f)
+ bl .raw_local_irq_restore
b 11f
/* Here we have a page fault that hash_page can't handle. */
@@ -1477,6 +1490,10 @@ _INIT_STATIC(start_here_multiplatform)
addi r2,r2,0x4000
add r2,r2,r26
+ /* Set initial ptr to current */
+ LOAD_REG_IMMEDIATE(r4, init_task)
+ std r4,PACACURRENT(r13)
+
/* Do very early kernel initializations, including initial hash table,
* stab and slb setup before we turn on relocation. */
Index: linux-work/arch/powerpc/kernel/setup_64.c
===================================================================
--- linux-work.orig/arch/powerpc/kernel/setup_64.c 2008-04-02 18:28:32.000000000 +1100
+++ linux-work/arch/powerpc/kernel/setup_64.c 2008-04-02 18:30:21.000000000 +1100
@@ -33,6 +33,7 @@
#include <linux/serial_8250.h>
#include <linux/bootmem.h>
#include <linux/pci.h>
+#include <linux/lockdep.h>
#include <asm/io.h>
#include <asm/kdump.h>
#include <asm/prom.h>
@@ -178,6 +179,9 @@ void __init early_setup(unsigned long dt
/* Enable early debugging if any specified (see udbg.h) */
udbg_early_init();
+ /* Initialize lockdep early or else spinlocks will blow */
+ lockdep_init();
+
DBG(" -> early_setup(), dt_ptr: 0x%lx\n", dt_ptr);
/*
Index: linux-work/arch/powerpc/kernel/entry_64.S
===================================================================
--- linux-work.orig/arch/powerpc/kernel/entry_64.S 2008-04-02 18:11:56.000000000 +1100
+++ linux-work/arch/powerpc/kernel/entry_64.S 2008-04-02 18:30:21.000000000 +1100
@@ -29,6 +29,7 @@
#include <asm/cputable.h>
#include <asm/firmware.h>
#include <asm/bug.h>
+#include <asm/irqflags.h>
/*
* System calls.
@@ -88,6 +89,14 @@ system_call_common:
addi r9,r1,STACK_FRAME_OVERHEAD
ld r11,exception_marker@toc(r2)
std r11,-16(r9) /* "regshere" marker */
+#ifdef CONFIG_TRACE_IRQFLAGS
+ bl .trace_hardirqs_on
+ REST_GPR(0,r1)
+ REST_4GPRS(3,r1)
+ REST_2GPRS(7,r1)
+ addi r9,r1,STACK_FRAME_OVERHEAD
+ ld r12,_MSR(r1)
+#endif /* CONFIG_TRACE_IRQFLAGS */
li r10,1
stb r10,PACASOFTIRQEN(r13)
stb r10,PACAHARDIRQEN(r13)
@@ -102,7 +111,7 @@ BEGIN_FW_FTR_SECTION
b hardware_interrupt_entry
2:
END_FW_FTR_SECTION_IFSET(FW_FEATURE_ISERIES)
-#endif
+#endif /* CONFIG_PPC_ISERIES */
mfmsr r11
ori r11,r11,MSR_EE
mtmsrd r11,1
@@ -504,6 +513,10 @@ BEGIN_FW_FTR_SECTION
li r3,0
stb r3,PACASOFTIRQEN(r13) /* ensure we are soft-disabled */
+#ifdef CONFIG_TRACE_IRQFLAGS
+ bl .trace_hardirqs_off
+ mfmsr r10
+#endif
ori r10,r10,MSR_EE
mtmsrd r10 /* hard-enable again */
addi r3,r1,STACK_FRAME_OVERHEAD
@@ -512,7 +525,7 @@ BEGIN_FW_FTR_SECTION
4:
END_FW_FTR_SECTION_IFSET(FW_FEATURE_ISERIES)
#endif
- stb r5,PACASOFTIRQEN(r13)
+ TRACE_AND_RESTORE_IRQ(r5);
/* extract EE bit and use it to restore paca->hard_enabled */
ld r3,_MSR(r1)
@@ -580,6 +593,16 @@ do_work:
bne restore
/* here we are preempting the current task */
1:
+#ifdef CONFIG_TRACE_IRQFLAGS
+ bl .trace_hardirqs_on
+ /* Note: we just clobbered r10 which used to contain the previous
+ * MSR before the hard-disabling done by the caller of do_work.
+ * We don't have that value anymore, but it doesn't matter as
+ * we will hard-enable unconditionally, we can just reload the
+ * current MSR into r10
+ */
+ mfmsr r10
+#endif /* CONFIG_TRACE_IRQFLAGS */
li r0,1
stb r0,PACASOFTIRQEN(r13)
stb r0,PACAHARDIRQEN(r13)
^ permalink raw reply
* [PATCH 5/6] [POWERPC] properly declare onstack completion in iseries veth
From: Benjamin Herrenschmidt @ 2008-04-09 7:21 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev
The iSeries veth driver uses an on-stack struct completion that
it initializes using the COMPLETION_INITIALIZER instead of
COMPLETION_INITIALIZER_ONSTACK macro, causing problems with
lockdep.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
drivers/net/iseries_veth.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- linux-work.orig/drivers/net/iseries_veth.c 2008-04-09 13:55:24.000000000 +1000
+++ linux-work/drivers/net/iseries_veth.c 2008-04-09 13:55:48.000000000 +1000
@@ -308,7 +308,8 @@ static void veth_complete_allocation(voi
static int veth_allocate_events(HvLpIndex rlp, int number)
{
- struct veth_allocation vc = { COMPLETION_INITIALIZER(vc.c), 0 };
+ struct veth_allocation vc =
+ { COMPLETION_INITIALIZER_ONSTACK(vc.c), 0 };
mf_allocate_lp_events(rlp, HvLpEvent_Type_VirtualLan,
sizeof(struct veth_lpevent), number,
^ 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