Re: [PATCH] module: ppc64 module CRC relocation fix causes perf issues

2013-07-25 Thread Neil Horman
On Thu, Jul 25, 2013 at 09:14:25AM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2013-07-25 at 08:34 +1000, Anton Blanchard wrote:
> > > Apart from the annoying colors, is there anything specific I should
> > > be looking for?  Some sort of error message, or output that actually
> > > makes sense?
> > 
> > Thanks for testing! Ben, I think the patch is good to go.
> 
> Sent it yesterday to Linus, it's upstream already :-)
> 
> Cheers,
> Ben.
> 
Sorry I'm a bit late to the thread, I've ben swamped.  Has someone tested this
with kexec/kdump?  Thats why the origional patch was created, because when kexec
loads the kernel at a different physical address, the relocations messed with
the module crc's, and modules couldn't load during the kexec boot.  Assuming
that kernaddr_start gets set appropriately during boot, using PHYSICAL_START
should be fine, but I wanted to check, and don't currently have access to a
powerpc system to do so.
Neil

> 
> 
> 
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


Re: [PATCH] module: ppc64 module CRC relocation fix causes perf issues

2013-07-26 Thread Neil Horman
On Fri, Jul 26, 2013 at 11:19:13AM +1000, Anton Blanchard wrote:
> 
> Hi Neil,
> 
> > Sorry I'm a bit late to the thread, I've ben swamped.  Has someone
> > tested this with kexec/kdump?  Thats why the origional patch was
> > created, because when kexec loads the kernel at a different physical
> > address, the relocations messed with the module crc's, and modules
> > couldn't load during the kexec boot.  Assuming that kernaddr_start
> > gets set appropriately during boot, using PHYSICAL_START should be
> > fine, but I wanted to check, and don't currently have access to a
> > powerpc system to do so. Neil
> 
> I tested a relocatable kernel forced to run at a non zero physical
> address (ie basically kdump). I verified CRCs were bad with your
> original patch backed out, and were good with this patch applied.
> 
> Anton
> 

Perfect, sounds like a sufficient test to me.

Acked-by: Neil Horman 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


Re: cscope: issue with symlinks in tools/testing/selftests/powerpc/copyloops/

2014-04-07 Thread Neil Horman
On Thu, Apr 03, 2014 at 03:16:15PM +0200, Yann Droneaud wrote:
> Hi,
> 
> I'm using cscope to browse kernel sources, but I'm facing warnings from
> the tool since following commit:
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=22d651dcef536c75f75537290bf3da5038e68b6b
> 
> commit 22d651dcef536c75f75537290bf3da5038e68b6b
> Author: Michael Ellerman 
> Date:   Tue Jan 21 15:22:17 2014 +1100
> 
> selftests/powerpc: Import Anton's memcpy / copy_tofrom_user tests
> 
> Turn Anton's memcpy / copy_tofrom_user test into something that can
> live in tools/testing/selftests.
> 
> It requires one turd in arch/powerpc/lib/memcpy_64.S, but it's 
> pretty harmless IMHO.
> 
> We are sailing very close to the wind with the feature macros. We 
> define them to nothing, which currently means we get a few extra 
> nops and include the unaligned calls.
> 
> Signed-off-by: Anton Blanchard 
> Signed-off-by: Michael Ellerman 
> Signed-off-by: Benjamin Herrenschmidt 
> 
> 
> cscope reports error when generating the cross-reference database:
> 
> $ make ALLSOURCE_ARCHS=all O=./obj-cscope/ cscope
>   GEN cscope
> cscope: cannot find
> file 
> /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/copyuser_power7.S
> cscope: cannot find
> file 
> /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/memcpy_64.S
> cscope: cannot find
> file 
> /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/memcpy_power7.S
> cscope: cannot find
> file 
> /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/copyuser_64.S
> 
> And when calling cscope from ./obj-cscope/ directory, it reports errors
> too.
> 
> Hopefully it doesn't stop it from working, so I'm still able to use
> cscope to browse kernel sources.
> 
No, it won't stop it from working, it just won't search those files.  I don't
recall exactly the reason, but IIRC there was a big discussion long ago about
symlinks and our ability to support them (around version 1.94 I think).  We
decided to not handle symlinks, as they would either point outside our search
tree, which we didn't want to include, or would point to another file in the
search tree, which made loading them pointless (as we would cover the search in
the pointed file).

Neil

> It's a rather uncommon side effect of having (for the first time ?)
> sources files as symlinks: looking for symlinks in the kernel sources
> returns only:
> 
> $ find . -type l
> ./arch/mips/boot/dts/include/dt-bindings
> ./arch/microblaze/boot/dts/system.dts
> ./arch/powerpc/boot/dts/include/dt-bindings
> ./arch/metag/boot/dts/include/dt-bindings
> ./arch/arm/boot/dts/include/dt-bindings
> ./tools/testing/selftests/powerpc/copyloops/copyuser_power7.S
> ./tools/testing/selftests/powerpc/copyloops/memcpy_64.S
> ./tools/testing/selftests/powerpc/copyloops/memcpy_power7.S
> ./tools/testing/selftests/powerpc/copyloops/copyuser_64.S
> ./obj-cscope/source
> ./Documentation/DocBook/vidioc-g-sliced-vbi-cap.xml
> ./Documentation/DocBook/vidioc-decoder-cmd.xml
> ...
> ./Documentation/DocBook/media-func-ioctl.xml
> ./Documentation/DocBook/vidioc-enumoutput.xml
> 
> 
> So one can wonder if having symlinked sources files is an expected
> supported feature for kbuild and all the various kernel
> tools/infrastructure ?
> 
> Regarding cscope specifically, it does not support symlink, and it's the
> expected behavior according to the bug reports I was able to find:
> 
> #214 cscope ignores symlinks to files 
> http://sourceforge.net/p/cscope/bugs/214/
> 
> #229 -I options doesn't handle symbolic link
> http://sourceforge.net/p/cscope/bugs/229/
> 
> #247 cscope: cannot find file 
> http://sourceforge.net/p/cscope/bugs/247/
> 
> #252 cscope: cannot find file *** 
> http://sourceforge.net/p/cscope/bugs/252/
> 
> #261 Regression - version 15.7a does not follow symbolic links 
> http://sourceforge.net/p/cscope/bugs/261/
> 
> 
> Regards.
> 
> -- 
> Yann Droneaud
> OPTEYA
> 
> 
> 
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: cscope: issue with symlinks in tools/testing/selftests/powerpc/copyloops/

2014-04-07 Thread Neil Horman
On Mon, Apr 07, 2014 at 02:42:59PM +0200, Gerhard Sittig wrote:
> On Mon, 2014-04-07 at 06:42 -0400, Neil Horman wrote:
> > 
> > On Thu, Apr 03, 2014 at 03:16:15PM +0200, Yann Droneaud wrote:
> > > 
> > > [ ... ]
> > > 
> > > cscope reports error when generating the cross-reference database:
> > > 
> > > $ make ALLSOURCE_ARCHS=all O=./obj-cscope/ cscope
> > >   GEN cscope
> > > cscope: cannot find
> > > file 
> > > /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/copyuser_power7.S
> > > cscope: cannot find
> > > file 
> > > /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/memcpy_64.S
> > > cscope: cannot find
> > > file 
> > > /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/memcpy_power7.S
> > > cscope: cannot find
> > > file 
> > > /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/copyuser_64.S
> > > 
> > > And when calling cscope from ./obj-cscope/ directory, it reports errors
> > > too.
> > > 
> > > Hopefully it doesn't stop it from working, so I'm still able to use
> > > cscope to browse kernel sources.
> > > 
> > No, it won't stop it from working, it just won't search those files.  I 
> > don't
> > recall exactly the reason, but IIRC there was a big discussion long ago 
> > about
> > symlinks and our ability to support them (around version 1.94 I think).  We
> > decided to not handle symlinks, as they would either point outside our 
> > search
> > tree, which we didn't want to include, or would point to another file in the
> > search tree, which made loading them pointless (as we would cover the 
> > search in
> > the pointed file).
> 
> So there are valid reasons to not process those filesystem
> entries.  Would it be useful to not emit the warnings then?  Or
> to silent those warnings when the user knows it's perfectly legal
> to skip those filesytem entries?  Like what you can do with the
> ctags(1) command and its --links option.
> 
I would see no problem with an option to do that.  I'd like to make it opt-in,
so that people who want to know about symlink issues will still see them, but
I'd be supportive of an option to quiet them
Neil

> 
> virtually yours
> Gerhard Sittig
> -- 
> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: off...@denx.de
> 
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: cscope: issue with symlinks in tools/testing/selftests/powerpc/copyloops/

2014-04-08 Thread Neil Horman
On Tue, Apr 08, 2014 at 09:56:10AM +0200, Gerhard Sittig wrote:
> [ removed cscope-devel from Cc:, non-subscriber mails get blocked anyway ]
> 
> On Mon, 2014-04-07 at 14:42 +0200, Gerhard Sittig wrote:
> > 
> > On Mon, 2014-04-07 at 06:42 -0400, Neil Horman wrote:
> > > 
> > > On Thu, Apr 03, 2014 at 03:16:15PM +0200, Yann Droneaud wrote:
> > > > 
> > > > [ ... ]
> > > > 
> > > > cscope reports error when generating the cross-reference database:
> > > > 
> > > > $ make ALLSOURCE_ARCHS=all O=./obj-cscope/ cscope
> > > >   GEN cscope
> > > > cscope: cannot find
> > > > file 
> > > > /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/copyuser_power7.S
> > > > cscope: cannot find
> > > > file 
> > > > /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/memcpy_64.S
> > > > cscope: cannot find
> > > > file 
> > > > /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/memcpy_power7.S
> > > > cscope: cannot find
> > > > file 
> > > > /home/ydroneaud/src/linux/tools/testing/selftests/powerpc/copyloops/copyuser_64.S
> > > > 
> > > > And when calling cscope from ./obj-cscope/ directory, it reports errors
> > > > too.
> > > > 
> > > > Hopefully it doesn't stop it from working, so I'm still able to use
> > > > cscope to browse kernel sources.
> > > > 
> > > No, it won't stop it from working, it just won't search those files.  I 
> > > don't
> > > recall exactly the reason, but IIRC there was a big discussion long ago 
> > > about
> > > symlinks and our ability to support them (around version 1.94 I think).  
> > > We
> > > decided to not handle symlinks, as they would either point outside our 
> > > search
> > > tree, which we didn't want to include, or would point to another file in 
> > > the
> > > search tree, which made loading them pointless (as we would cover the 
> > > search in
> > > the pointed file).
> > 
> > So there are valid reasons to not process those filesystem
> > entries.  Would it be useful to not emit the warnings then?  Or
> > to silent those warnings when the user knows it's perfectly legal
> > to skip those filesytem entries?  Like what you can do with the
> > ctags(1) command and its --links option.
> 
> For the record:  I got a response "off list" (actually to the
> cscope list only which is closed for non-subscribers, while the
> Linux related recipients were removed despite the fact that the
> issue appears to be in Linux), see
> http://article.gmane.org/gmane.comp.programming.tools.cscope.devel/105
> 
I don't agree with Hans here. He's right in that the Linux makefiles could
exclude the symlinks from the index file that it builds, but if cscope were left
to its own devices it would also exclude the assembly files and other code that
it doesn't officially parse, but does more or less ok with.  We can argue all
day weather its ok for Linux to do that, but the facts of the matter is that
people use cscope  to search the linux source tree asm files and all, and while
we could propose that the cscope makefile target filter out symlinks, it seems
IMHO to be more difficult than its worth.  Adding code to silence db build
warnings would be good.

> This reponse suggests that the issue is not in cscope(1) itself,
> but instead is in how the cscope(1) command got invoked.  Which
> translates into "the Linux infrastructure does something wrong".
> 
> A quick search identifies ./scripts/tags.sh:all_target_sources()
> as the spot where symlinks should get filtered out.  Where both
> paths of all_target_sources() end up calling all_sources().
> 
> 
> virtually yours
> Gerhard Sittig
> -- 
> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: off...@denx.de
> 
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] irqbalance, powerpc: add IRQs without settable SMP affinity to banned list

2010-09-24 Thread Neil Horman
On Fri, Sep 24, 2010 at 04:56:34PM +1000, Michael Neuling wrote:
> 
> > > > size_t size =3D 0;
> > > > FILE *file;
> > > > sprintf(buf, "/proc/irq/%i/smp_affinity", 
> > > > number);
> > > > -   file =3D fopen(buf, "r");
> > > > +   file =3D fopen(buf, "r+");
> > > > if (!file)
> > > > continue;
> > > > if (getline(&line, &size, file)=3D=3D0) {
> > > > @@ -89,7 +89,14 @@
> > > > continue;
> > > > }
> > > > cpumask_parse_user(line, strlen(line), 
> > > > irq->mask);
> > > > -   fclose(file);
> > > > +   /*
> > > > +* Check that we can write the affinity, if
> > > > +* not take it out of the list.
> > > > +*/
> > > > +   if (fputs(line, file) =3D=3D EOF)
> > > > +   can_set =3D 0;
> > 
> > > This is maybe a nit, but writing to the affinity file can fail for a few
> > > different reasons, some of them permanent, some transient.  For instance,=
> >  if
> > > we're in a memory constrained condition temporarily irq_affinity_proc_wri=
> > te
> > > might return -ENOMEM. =20
> > 
> > Yeah true, usually followed shortly by your kernel going so far into
> > swap you never get it back, or OOMing, but I guess it's possible.
> > 
> > > Might it be better to modify this code so that, instead
> > > of using fputs to merge the various errors into an EOF, we use some other=
> >  write
> > > method that lets us better determine the error and selectively ban the in=
> > terrupt
> > > only for those errors which we consider permanent?
> > 
> > Yep. It seems fputs() gives you know way to get the actual error from
> > write(), so it looks we'll need to switch to open/write, but that's
> > probably not so terrible.
> 
> fclose inherits the error from fputs and it sets errno correctly.  Below
> uses this to catch only EIO errors and mark them for the banned list.
> 
> Mikey
> 
> irqbalance, powerpc: add IRQs without settable SMP affinity to banned list
> 
> On pseries powerpc, IPIs are registered with an IRQ number so
> /proc/interrupts looks like this on a 2 core/2 thread machine:
> 
>CPU0   CPU1   CPU2   CPU3
>  16:316428232905141138794 983121   XICS Level 
>IPI
>  18:2605674  0 304994  0   XICS Level 
>lan0
>  30: 400057  0 169209  0   XICS Level 
>ibmvscsi
> LOC: 133734  77250 106425  91951   Local timer interrupts
> SPU:  0  0  0  0   Spurious interrupts
> CNT:  0  0  0  0   Performance monitoring 
> interrupts
> MCE:  0  0  0  0   Machine check exceptions
> 
> Unfortunately this means irqbalance attempts to set the affinity of IPIs
> which is not possible.  So in the above case, when irqbalance is in
> performance mode due to heavy IPI, lan0 and ibmvscsi activity, it
> sometimes attempts to put the IPIs on one core (CPU0&1) and lan0 and
> ibmvscsi on the other core (CPU2&3).  This is suboptimal as we want lan0
> and ibmvscsi to be on separate cores and IPIs to be ignored.
> 
> When irqblance attempts writes to the IPI smp_affinity (ie.
> /proc/irq/16/smp_affinity in the above example) it fails with an EIO but
> irqbalance currently ignores this.
> 
> This patch catches these write fails and in this case adds that IRQ
> number to the banned IRQ list.  This will catch the above IPI case and
> any other IRQ where the SMP affinity can't be set.
> 
> Tested on POWER6, POWER7 and x86.
> 
> Signed-off-by: Michael Neuling 
>  
> Index: irqbalance/irqlist.c
> ===
> --- irqbalance.orig/irqlist.c
> +++ irqbalance/irqlist.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "types.h"
>  #include "irqbalance.h"
> @@ -67,7 +68,7 @@
>   DIR *dir;
>   struct dirent *entry;
>   char *c, *c2;
> - int nr , count = 0;
> + int nr , count = 0, can_set = 1;
>   char buf[PATH_MAX];
>   sprintf(buf, "/proc/irq/%i", number);
>   dir = opendir(buf);
> @@ -80,7 +81,7 @@
>   size_t size = 0;
>   FILE *file;
>   sprintf(buf, "/proc/irq/%i/smp_affinity", number);
> - file = fopen(buf, "r");
> + file = fopen(buf, "r+");
>   if (!file)
>   continue;
>   if (getline(&line, &size, file)==0) {
> @@ -89,7 +90,13 @@
>   continue;
>   }
>

[PATCH] ppc64: re-enable kexec to allow module loads with CONFIG_MODVERSIONS and CONFIG_RELOCATABLE turned on

2009-11-19 Thread Neil Horman
Hey there-
Before anyone flames me for what a oddball solution this is, let me just
say I'm trying to get the ball rolling here.  I think there may be better
solutions that can be impemented in reloc_64.S, but I've yet to make any of the
ones I've tried work successfully.  I'm open to suggestions, but this solution
is the only one so far that I've been able to get to work. thanks :)


Adjust crcs in __kcrctab_* sections if relocs are used with CONFIG_RELOCATABLE

When CONFIG_MODVERSIONS and CONFIG_RELOCATABLE are enabled on powerpc platforms,
kdump has been failing in a rather odd way.  specifically modules will not
install.  This is because when validating the crcs of symbols that the module
needs, the crc of the module never matches the crc that is stored in the kernel.

The underlying cause of this is how CONFIG_MODVERSIONS is implemented, and how
CONFIG_RELOCATABLE are implemented.  with CONFIG_MODVERSIONS enabled, for every
exported symbol in the kernel we emit 2 symbols, __crc_#sym which is declared
extern and __kcrctab_##sym, which is placed in the __kcrctab section of the
binary.  The latter has its value set equal to the address of the former
(recalling it is declared extern).  After the object file is built, genksyms is
run on the processed source, and crcs are computed for each exported symbol.
genksyms then emits a linker script which defines each of the needed __crc_##sym
symbols, and sets their addresses euqal to their respective crcs.  This script
is then used in a secondary link to the previously build object file, so that
the crcs of the missing symbol can be validated on module insert.

The problem here is that because __kcrctab_sym is set equal to &__crc_##sym, a
relocation entry is emitted by the compiler for the __kcrctab__##sym.  Normally
this is not a problem, since relocation on other arches is done without the aid
of .rel.dyn sections.  PPC however uses these relocations when
CONFIG_RELOCATABLE is enabled.  nominally, since addressing starts at 0 for ppc,
its irrelevant, but if we start at a non-zero address (like we do when booting
via kexec from reserved crashkernel memory), the ppc boot code iterates over the
relocation entries, and winds up adding that relocation offset to all symbols,
including the symbols that are actually the aforementioned crc values in the
__kcrctab_* sections.  This effectively corrupts the crcs and prevents any
module loads from happening during a kdump.

My solution is to 'undo' these relocations prior to boot up.  If
ARCH_USES_RELOC_ENTRIES is defined, we add a symbol at address zero to the
linker script for that arch (I call it reloc_start, so that &reloc_start = 0).
This symbol will then indicate the relocation offset for any given boot.  We
also add an initcall to the module code that, during boot, scans the __kcrctab_*
sections and subtracts &reloc_start from every entry in those sections,
restoring the appropriate crc value.

I've verified that this allows kexec to work properly on ppc64 systems myself.

Signed-off-by: Neil Horman 


 arch/powerpc/include/asm/local.h  |6 ++
 arch/powerpc/kernel/vmlinux.lds.S |4 
 kernel/module.c   |   30 ++
 3 files changed, 40 insertions(+)

diff --git a/arch/powerpc/include/asm/local.h b/arch/powerpc/include/asm/local.h
index 84b457a..9cc49e5 100644
--- a/arch/powerpc/include/asm/local.h
+++ b/arch/powerpc/include/asm/local.h
@@ -4,6 +4,12 @@
 #include 
 #include 
 
+#ifdef CONFIG_MODVERSIONS
+#define ARCH_USES_RELOC_ENTRIES
+
+extern unsigned long reloc_start;
+#endif
+
 typedef struct
 {
atomic_long_t a;
diff --git a/arch/powerpc/kernel/vmlinux.lds.S 
b/arch/powerpc/kernel/vmlinux.lds.S
index 27735a7..2b9fb2e 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -38,6 +38,10 @@ jiffies = jiffies_64 + 4;
 #endif
 SECTIONS
 {
+   . = 0;
+   reloc_start = .;
+   . = 0;
+
. = KERNELBASE;
 
 /*
diff --git a/kernel/module.c b/kernel/module.c
index 8b7d880..87a4928 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -181,8 +181,11 @@ extern const struct kernel_symbol 
__stop___ksymtab_gpl_future[];
 extern const struct kernel_symbol __start___ksymtab_gpl_future[];
 extern const struct kernel_symbol __stop___ksymtab_gpl_future[];
 extern const unsigned long __start___kcrctab[];
+extern const unsigned long __stop___kcrctab[];
 extern const unsigned long __start___kcrctab_gpl[];
+extern const unsigned long __stop___kcrctab_gpl[];
 extern const unsigned long __start___kcrctab_gpl_future[];
+extern const unsigned long __stop___kcrctab_gpl_future[];
 #ifdef CONFIG_UNUSED_SYMBOLS
 extern const struct kernel_symbol __start___ksymtab_unused[];
 extern const struct kernel_symbol __stop___ksymtab_unused[];
@@ -3144,3 +3147,30 @@ int module_get_iter_tracepoints(struct tracepoint_iter 
*iter)
return found;
 }
 #endif
+
+#ifdef ARCH_USES_

Re: [PATCH] ppc64: re-enable kexec to allow module loads with CONFIG_MODVERSIONS and CONFIG_RELOCATABLE turned on

2009-11-24 Thread Neil Horman
On Thu, Nov 19, 2009 at 02:52:16PM -0500, Neil Horman wrote:
> Hey there-
>   Before anyone flames me for what a oddball solution this is, let me just
> say I'm trying to get the ball rolling here.  I think there may be better
> solutions that can be impemented in reloc_64.S, but I've yet to make any of 
> the
> ones I've tried work successfully.  I'm open to suggestions, but this solution
> is the only one so far that I've been able to get to work. thanks :)
> 
> 
> Adjust crcs in __kcrctab_* sections if relocs are used with CONFIG_RELOCATABLE
> 
> When CONFIG_MODVERSIONS and CONFIG_RELOCATABLE are enabled on powerpc 
> platforms,
> kdump has been failing in a rather odd way.  specifically modules will not
> install.  This is because when validating the crcs of symbols that the module
> needs, the crc of the module never matches the crc that is stored in the 
> kernel.
> 
> The underlying cause of this is how CONFIG_MODVERSIONS is implemented, and how
> CONFIG_RELOCATABLE are implemented.  with CONFIG_MODVERSIONS enabled, for 
> every
> exported symbol in the kernel we emit 2 symbols, __crc_#sym which is declared
> extern and __kcrctab_##sym, which is placed in the __kcrctab section of the
> binary.  The latter has its value set equal to the address of the former
> (recalling it is declared extern).  After the object file is built, genksyms 
> is
> run on the processed source, and crcs are computed for each exported symbol.
> genksyms then emits a linker script which defines each of the needed 
> __crc_##sym
> symbols, and sets their addresses euqal to their respective crcs.  This script
> is then used in a secondary link to the previously build object file, so that
> the crcs of the missing symbol can be validated on module insert.
> 
> The problem here is that because __kcrctab_sym is set equal to &__crc_##sym, a
> relocation entry is emitted by the compiler for the __kcrctab__##sym.  
> Normally
> this is not a problem, since relocation on other arches is done without the 
> aid
> of .rel.dyn sections.  PPC however uses these relocations when
> CONFIG_RELOCATABLE is enabled.  nominally, since addressing starts at 0 for 
> ppc,
> its irrelevant, but if we start at a non-zero address (like we do when booting
> via kexec from reserved crashkernel memory), the ppc boot code iterates over 
> the
> relocation entries, and winds up adding that relocation offset to all symbols,
> including the symbols that are actually the aforementioned crc values in the
> __kcrctab_* sections.  This effectively corrupts the crcs and prevents any
> module loads from happening during a kdump.
> 
> My solution is to 'undo' these relocations prior to boot up.  If
> ARCH_USES_RELOC_ENTRIES is defined, we add a symbol at address zero to the
> linker script for that arch (I call it reloc_start, so that &reloc_start = 0).
> This symbol will then indicate the relocation offset for any given boot.  We
> also add an initcall to the module code that, during boot, scans the 
> __kcrctab_*
> sections and subtracts &reloc_start from every entry in those sections,
> restoring the appropriate crc value.
> 
> I've verified that this allows kexec to work properly on ppc64 systems myself.
> 
> Signed-off-by: Neil Horman 
> 
Ping, any thoughts here?  As I've been mulling this over, I'm beginning to like
this solution better than a completely arch-specific section, as this approach
makes common the bits that any arch is going to need if they implement
CONFIG_RELOCATABLE with a .rel[a].* section set.  The alternative is of course
to simply skip the appropriate relocations, but thats something that every arch
will need to discover on their own.  This makes it a bit more clear whats
happening I think.

Regards
Neil
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


Re: [PATCH] ppc64: re-enable kexec to allow module loads with CONFIG_MODVERSIONS and CONFIG_RELOCATABLE turned on

2009-11-24 Thread Neil Horman
Neil Horman writes:

>   Before anyone flames me for what a oddball solution this is, let me just
> say I'm trying to get the ball rolling here.  I think there may be better
> solutions that can be impemented in reloc_64.S, but I've yet to make any of
the
> ones I've tried work successfully.  I'm open to suggestions, but this solution
> is the only one so far that I've been able to get to work. thanks :)

I had thought that the CRCs would be the only things with reloc
addends less than 4G, but it turns out that the toolchain folds
expressions like __pa(&sym) into just a load from a doubleword (in the
TOC) containing the physical address of sym.  That needs to be
relocated and its reloc addend will be less than 4GB.  Hence the
crashes early in boot that you were seeing with my proposed patch to
reloc_64.S.

Given that, your solution seems as reasonable as any.  Should this go
via the modules maintainer, Rusty Russell, perhaps?

In any case,

Acked-by: Paul Mackerras 


I'm not 100% sure, it does kind of split the middle in regards to what tree is
should come through.

Rusy, given the previous entries in this thread, do you have any thoughts on the
patch, or which tree it should go through?  I kind of think that the ppc tree is
just fine here since its currently the only user of this code, but I'll defer to
other opinions on the subject.

Thanks & Regards
Neil

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


Re: [PATCH] ppc64: re-enable kexec to allow module loads with CONFIG_MODVERSIONS and CONFIG_RELOCATABLE turned on

2009-11-30 Thread Neil Horman
On Thu, Nov 19, 2009 at 02:52:16PM -0500, Neil Horman wrote:
> Hey there-
>   Before anyone flames me for what a oddball solution this is, let me just
> say I'm trying to get the ball rolling here.  I think there may be better
> solutions that can be impemented in reloc_64.S, but I've yet to make any of 
> the
> ones I've tried work successfully.  I'm open to suggestions, but this solution
> is the only one so far that I've been able to get to work. thanks :)
> 
> 
> Adjust crcs in __kcrctab_* sections if relocs are used with CONFIG_RELOCATABLE
> 
> When CONFIG_MODVERSIONS and CONFIG_RELOCATABLE are enabled on powerpc 
> platforms,
> kdump has been failing in a rather odd way.  specifically modules will not
> install.  This is because when validating the crcs of symbols that the module
> needs, the crc of the module never matches the crc that is stored in the 
> kernel.
> 
> The underlying cause of this is how CONFIG_MODVERSIONS is implemented, and how
> CONFIG_RELOCATABLE are implemented.  with CONFIG_MODVERSIONS enabled, for 
> every
> exported symbol in the kernel we emit 2 symbols, __crc_#sym which is declared
> extern and __kcrctab_##sym, which is placed in the __kcrctab section of the
> binary.  The latter has its value set equal to the address of the former
> (recalling it is declared extern).  After the object file is built, genksyms 
> is
> run on the processed source, and crcs are computed for each exported symbol.
> genksyms then emits a linker script which defines each of the needed 
> __crc_##sym
> symbols, and sets their addresses euqal to their respective crcs.  This script
> is then used in a secondary link to the previously build object file, so that
> the crcs of the missing symbol can be validated on module insert.
> 
> The problem here is that because __kcrctab_sym is set equal to &__crc_##sym, a
> relocation entry is emitted by the compiler for the __kcrctab__##sym.  
> Normally
> this is not a problem, since relocation on other arches is done without the 
> aid
> of .rel.dyn sections.  PPC however uses these relocations when
> CONFIG_RELOCATABLE is enabled.  nominally, since addressing starts at 0 for 
> ppc,
> its irrelevant, but if we start at a non-zero address (like we do when booting
> via kexec from reserved crashkernel memory), the ppc boot code iterates over 
> the
> relocation entries, and winds up adding that relocation offset to all symbols,
> including the symbols that are actually the aforementioned crc values in the
> __kcrctab_* sections.  This effectively corrupts the crcs and prevents any
> module loads from happening during a kdump.
> 
> My solution is to 'undo' these relocations prior to boot up.  If
> ARCH_USES_RELOC_ENTRIES is defined, we add a symbol at address zero to the
> linker script for that arch (I call it reloc_start, so that &reloc_start = 0).
> This symbol will then indicate the relocation offset for any given boot.  We
> also add an initcall to the module code that, during boot, scans the 
> __kcrctab_*
> sections and subtracts &reloc_start from every entry in those sections,
> restoring the appropriate crc value.
> 
> I've verified that this allows kexec to work properly on ppc64 systems myself.
> 
> Signed-off-by: Neil Horman 
> 

Paul, Ben, given that Rusty hasn't come back with any opinion on this patch, do 
you
feel comfortable merging it via the ppc tree?  Currently the earlyinit routine
is only compiled in and used for your arch, so I think its fairly benign.

Regards
Neil

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


Re: [PATCH] ppc64: re-enable kexec to allow module loads with CONFIG_MODVERSIONS and CONFIG_RELOCATABLE turned on

2009-12-01 Thread Neil Horman
>> Paul, Ben, given that Rusty hasn't come back with any opinion on this patch,
>do you
>> feel comfortable merging it via the ppc tree?  Currently the earlyinit 
>> routine
>> is only compiled in and used for your arch, so I think its fairly benign.
>
>Sorry, I *did* track down the archives for linuxppc-dev, then find your post,
>then read your patch.  But I didn't actually reply.
>
>Other than minor issues, there's one significant one: you shouldn't be trying
>to change rodata.  It might work on PPC today, but it's poor form at least.
>
>How's this?  Untested on ppc.
>
I'll try grab a ppc64 system and test this soon.

>Other changes:
>1) I also changed reloc_start to an array; this is a good idea for any
>   linker-defined symbols so the compiler can't make assumptions about size.
Ok, I'll certainly trust your linker skill over mine :)

>2) local.h?  How about module.h?
Seems good to me
>3) I don't think the extra ". = 0" is necessary.
Its not, I was just trying to be clear about where reloc_start was to be placed.

>4) ARCH_USES_RELOC_ENTRIES isn't clear enough for me; I prefer
>   ARCH_RELOCATE_KCRCTAB.
Sounds good to me.


I'll test this as soon as I'm able
Thanks!
Neil

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


Re: [PATCH] ppc64: re-enable kexec to allow module loads with CONFIG_MODVERSIONS and CONFIG_RELOCATABLE turned on

2009-12-03 Thread Neil Horman
>>> Paul, Ben, given that Rusty hasn't come back with any opinion on this patch,
>>do you
>>> feel comfortable merging it via the ppc tree?  Currently the earlyinit
>routine
>>> is only compiled in and used for your arch, so I think its fairly benign.
>>
>>Sorry, I *did* track down the archives for linuxppc-dev, then find your post,
>>then read your patch.  But I didn't actually reply.
>>
>>Other than minor issues, there's one significant one: you shouldn't be trying
>>to change rodata.  It might work on PPC today, but it's poor form at least.
>>
>>How's this?  Untested on ppc.
>>
>I'll try grab a ppc64 system and test this soon.
>
>>Other changes:
>>1) I also changed reloc_start to an array; this is a good idea for any
>>   linker-defined symbols so the compiler can't make assumptions about size.
>Ok, I'll certainly trust your linker skill over mine :)
>
>>2) local.h?  How about module.h?
>Seems good to me
>>3) I don't think the extra ". = 0" is necessary.
>Its not, I was just trying to be clear about where reloc_start was to be 
>placed.
>
>>4) ARCH_USES_RELOC_ENTRIES isn't clear enough for me; I prefer
>>   ARCH_RELOCATE_KCRCTAB.
>Sounds good to me.
>
>
>I'll test this as soon as I'm able
>Thanks!
>Neil
>

Just finished testing your patch Rusty, it all works quite well, Thanks!
Will this be going in via your tree, or the ppc tree?

Acked-by: Neil Horman 

Neil
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


[PATCH] powerpc: Add vmcoreinfo symbols to allow makdumpfile to filter core files properly

2010-07-13 Thread Neil Horman
Hey all-
About 2 years ago now, I sent this patch upstream to allow makedumpfile
to properly filter cores on ppc64:
http://www.mail-archive.com/ke...@lists.infradead.org/msg02426.html
It got acks from the kexec folks so I pulled it into RHEL, but I never checked
back here to make sure it ever made it in, which apparently it didn't.  It still
needs to be included, so I'm reposting it here, making sure to copy all the ppc
folks this time.  I've retested it on the latest linus kernel and it works fine,
allowing makedumpfile to find all the symbols it needs to properly strip a
vmcore on ppc64.

Neil

Signed-off-by: Neil Horman 


 machine_kexec.c |   12 
 1 file changed, 12 insertions(+)


diff --git a/arch/powerpc/kernel/machine_kexec.c 
b/arch/powerpc/kernel/machine_kexec.c
index bb3d893..0df7031 100644
--- a/arch/powerpc/kernel/machine_kexec.c
+++ b/arch/powerpc/kernel/machine_kexec.c
@@ -45,6 +45,18 @@ void machine_kexec_cleanup(struct kimage *image)
ppc_md.machine_kexec_cleanup(image);
 }
 
+void arch_crash_save_vmcoreinfo(void)
+{
+
+#ifdef CONFIG_NEED_MULTIPLE_NODES
+   VMCOREINFO_SYMBOL(node_data);
+   VMCOREINFO_LENGTH(node_data, MAX_NUMNODES);
+#endif
+#ifndef CONFIG_NEED_MULTIPLE_NODES
+   VMCOREINFO_SYMBOL(contig_page_data);
+#endif
+}
+
 /*
  * Do not allocate memory (or fail in any way) in machine_kexec().
  * We are past the point of no return, committed to rebooting now.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


Re: [PATCH] powerpc: Add vmcoreinfo symbols to allow makdumpfile to filter core files properly

2010-07-26 Thread Neil Horman
On Tue, Jul 13, 2010 at 09:46:09AM -0400, Neil Horman wrote:
> Hey all-
>   About 2 years ago now, I sent this patch upstream to allow makedumpfile
> to properly filter cores on ppc64:
> http://www.mail-archive.com/ke...@lists.infradead.org/msg02426.html
> It got acks from the kexec folks so I pulled it into RHEL, but I never checked
> back here to make sure it ever made it in, which apparently it didn't.  It 
> still
> needs to be included, so I'm reposting it here, making sure to copy all the 
> ppc
> folks this time.  I've retested it on the latest linus kernel and it works 
> fine,
> allowing makedumpfile to find all the symbols it needs to properly strip a
> vmcore on ppc64.
> 
> Neil
> 
> Signed-off-by: Neil Horman 
> 
Ping, anyone want to chime in on this, its needed for dump filtering to work
properly on ppc64
Neil

> 
>  machine_kexec.c |   12 
>  1 file changed, 12 insertions(+)
> 
> 
> diff --git a/arch/powerpc/kernel/machine_kexec.c 
> b/arch/powerpc/kernel/machine_kexec.c
> index bb3d893..0df7031 100644
> --- a/arch/powerpc/kernel/machine_kexec.c
> +++ b/arch/powerpc/kernel/machine_kexec.c
> @@ -45,6 +45,18 @@ void machine_kexec_cleanup(struct kimage *image)
>   ppc_md.machine_kexec_cleanup(image);
>  }
>  
> +void arch_crash_save_vmcoreinfo(void)
> +{
> +
> +#ifdef CONFIG_NEED_MULTIPLE_NODES
> + VMCOREINFO_SYMBOL(node_data);
> + VMCOREINFO_LENGTH(node_data, MAX_NUMNODES);
> +#endif
> +#ifndef CONFIG_NEED_MULTIPLE_NODES
> + VMCOREINFO_SYMBOL(contig_page_data);
> +#endif
> +}
> +
>  /*
>   * Do not allocate memory (or fail in any way) in machine_kexec().
>   * We are past the point of no return, committed to rebooting now.
> 
> ___
> kexec mailing list
> ke...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


Re: [PATCH] powerpc: Add vmcoreinfo symbols to allow makdumpfile to filter core files properly

2010-08-04 Thread Neil Horman
On Tue, Jul 13, 2010 at 09:46:09AM -0400, Neil Horman wrote:
> Hey all-
>   About 2 years ago now, I sent this patch upstream to allow makedumpfile
> to properly filter cores on ppc64:
> http://www.mail-archive.com/ke...@lists.infradead.org/msg02426.html
> It got acks from the kexec folks so I pulled it into RHEL, but I never checked
> back here to make sure it ever made it in, which apparently it didn't.  It 
> still
> needs to be included, so I'm reposting it here, making sure to copy all the 
> ppc
> folks this time.  I've retested it on the latest linus kernel and it works 
> fine,
> allowing makedumpfile to find all the symbols it needs to properly strip a
> vmcore on ppc64.
> 
> Neil
> 
> Signed-off-by: Neil Horman 
> 
> 
>  machine_kexec.c |   12 
>  1 file changed, 12 insertions(+)
> 
> 
> diff --git a/arch/powerpc/kernel/machine_kexec.c 
> b/arch/powerpc/kernel/machine_kexec.c
> index bb3d893..0df7031 100644
> --- a/arch/powerpc/kernel/machine_kexec.c
> +++ b/arch/powerpc/kernel/machine_kexec.c
> @@ -45,6 +45,18 @@ void machine_kexec_cleanup(struct kimage *image)
>   ppc_md.machine_kexec_cleanup(image);
>  }
>  
> +void arch_crash_save_vmcoreinfo(void)
> +{
> +
> +#ifdef CONFIG_NEED_MULTIPLE_NODES
> + VMCOREINFO_SYMBOL(node_data);
> + VMCOREINFO_LENGTH(node_data, MAX_NUMNODES);
> +#endif
> +#ifndef CONFIG_NEED_MULTIPLE_NODES
> + VMCOREINFO_SYMBOL(contig_page_data);
> +#endif
> +}
> +
>  /*
>   * Do not allocate memory (or fail in any way) in machine_kexec().
>   * We are past the point of no return, committed to rebooting now.
> 
> ___
> kexec mailing list
> ke...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 

Ping yet again. Ben, This needs review/acceptance from you or Paul
Neil
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


Re: [PATCH] powerpc: Add vmcoreinfo symbols to allow makdumpfile to filter core files properly

2010-08-05 Thread Neil Horman
On Thu, Aug 05, 2010 at 12:04:26PM +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2010-08-04 at 10:49 -0400, Neil Horman wrote:
> > Ping yet again. Ben, This needs review/acceptance from you or Paul
> > Neil 
> 
> Isn't it already in powerpc-next about to be pulled by Linus ?
> 
Yes, there it is.  Apologies.  For whatever reason, I was looking on the main
branch of your tree.  It didn't occur to me to check your next branch.  Sorry.

> In general, I recommend you check the status of your patches on
> patchwork. I'm nagging Jeremy to add a feature so it emails the
> submitter when the patch status changes :-)
> 
Noted, I'll remember that.  Email from patchwork would be a nice feature. +1
from me. 

Thanks & Regards
Neil

> Cheers,
> Ben.
> 
> 
> ___
> kexec mailing list
> ke...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


Re: [PATCH] irqbalance, powerpc: add IRQs without settable SMP affinity to banned list

2010-09-23 Thread Neil Horman
On Thu, Sep 23, 2010 at 08:57:20PM +1000, Michael Neuling wrote:
> 
> > > + if (fwrite(line, strlen(line) - 1, 1, file) == 0)
> > 
> > if (fputs(line, file) == EOF)
> 
> Good point thanks... new patch below
> 
> Mikey
> 
> irqbalance, powerpc: add IRQs without settable SMP affinity to banned list
> 
> On pseries powerpc, IPIs are registered with an IRQ number so
> /proc/interrupts looks like this on a 2 core/2 thread machine:
> 
>CPU0   CPU1   CPU2   CPU3
>  16:316428232905141138794 983121   XICS Level 
>IPI
>  18:2605674  0 304994  0   XICS Level 
>lan0
>  30: 400057  0 169209  0   XICS Level 
>ibmvscsi
> LOC: 133734  77250 106425  91951   Local timer interrupts
> SPU:  0  0  0  0   Spurious interrupts
> CNT:  0  0  0  0   Performance monitoring 
> interrupts
> MCE:  0  0  0  0   Machine check exceptions
> 
> Unfortunately this means irqbalance attempts to set the affinity of IPIs
> which is not possible.  So in the above case, when irqbalance is in
> performance mode due to heavy IPI, lan0 and ibmvscsi activity, it
> sometimes attempts to put the IPIs on one core (CPU0&1) and lan0 and
> ibmvscsi on the other core (CPU2&3).  This is suboptimal as we want lan0
> and ibmvscsi to be on separate cores and IPIs to be ignored.
> 
> When irqblance attempts writes to the IPI smp_affinity (ie.
> /proc/irq/16/smp_affinity in the above example) it fails but irqbalance
> ignores currently ignores this.
> 
> This patch catches these write fails and in this case adds that IRQ
> number to the banned IRQ list.  This will catch the above IPI case and
> any other IRQ where the SMP affinity can't be set.
> 
> Tested on POWER6, POWER7 and x86.
> 
> Signed-off-by: Michael Neuling 
> 
> Index: irqbalance/irqlist.c
> ===
> --- irqbalance.orig/irqlist.c
> +++ irqbalance/irqlist.c
> @@ -67,7 +67,7 @@
>   DIR *dir;
>   struct dirent *entry;
>   char *c, *c2;
> - int nr , count = 0;
> + int nr , count = 0, can_set = 1;
>   char buf[PATH_MAX];
>   sprintf(buf, "/proc/irq/%i", number);
>   dir = opendir(buf);
> @@ -80,7 +80,7 @@
>   size_t size = 0;
>   FILE *file;
>   sprintf(buf, "/proc/irq/%i/smp_affinity", number);
> - file = fopen(buf, "r");
> + file = fopen(buf, "r+");
>   if (!file)
>   continue;
>   if (getline(&line, &size, file)==0) {
> @@ -89,7 +89,14 @@
>   continue;
>   }
>   cpumask_parse_user(line, strlen(line), irq->mask);
> - fclose(file);
> + /*
> +  * Check that we can write the affinity, if
> +  * not take it out of the list.
> +  */
> + if (fputs(line, file) == EOF)
> + can_set = 0;
This is maybe a nit, but writing to the affinity file can fail for a few
different reasons, some of them permanent, some transient.  For instance, if
we're in a memory constrained condition temporarily irq_affinity_proc_write
might return -ENOMEM.  Might it be better to modify this code so that, instead
of using fputs to merge the various errors into an EOF, we use some other write
method that lets us better determine the error and selectively ban the interrupt
only for those errors which we consider permanent?

Otherwise this looks fine to me.

Thanks
Neil

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


Re: [PATCH] Do not inline putprops function

2009-06-17 Thread Neil Horman
On Wed, Jun 17, 2009 at 10:26:35PM +1000, Michael Ellerman wrote:
> On Wed, 2009-06-17 at 17:29 +0530, M. Mohan Kumar wrote:
> > Do not inline putprops function
> > 
> > With the recent kexec-tools git tree, both kexec and kdump kernels hang (i.e
> > kexec -l and kexec -p respectively). This happened after the patch "ppc64:
> > cleanups" commit b43a84a31a4be6ed025c1bdef3bb1c3c12e01b16. I tried
> > reverting each hunk and then found out that retaining following lines in
> > fs2dt.c makes kexec/kdump work.
> > 
> > -static unsigned *dt_len; /* changed len of modified cmdline
> > -   in flat device-tree */
> > 
> > []
> > 
> > -   dt_len = dt;
> > 
> > I don't have any clue why removing a unused variable would cause the kexec
> > kernel to hang. After further investigation, I observed that if the putprops
> > function is not inlined, kexec/kdump kernel would work even after removing
> > the above lines.
> > 
> > This patch directs gcc to not inline the putprops function. Now we could
> > invoke kexec and kdump kernels.
> 
> What compiler version are you using? Does the behaviour change if you
> use a newer/older compiler? It sounds to me like there's some deeper bug
> and your patch is just papering over it.
> 
Agreed, this doesn't make any sense. Try changing the compiler version to see if
the problem goes away or stops.  It might also be worthwhile to dump the
contents of the device tree at the start and end of the kexec process.  If the
changing of how a function is inlined is causing a hang, its likely changing how
the putprops function is writing information to the device tree.  Understanding
what that change is will likely provide clues to how the code has changed.

Neil

> cheers



> ___
> kexec mailing list
> ke...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


Re: [PATCH] Do not inline putprops function

2009-06-17 Thread Neil Horman
On Wed, Jun 17, 2009 at 07:04:35PM +0530, M. Mohan Kumar wrote:
> On Wed, Jun 17, 2009 at 09:04:13AM -0400, Neil Horman wrote:
> > On Wed, Jun 17, 2009 at 10:26:35PM +1000, Michael Ellerman wrote:
> > > 
> > > What compiler version are you using? Does the behaviour change if you
> > > use a newer/older compiler? It sounds to me like there's some deeper bug
> > > and your patch is just papering over it.
> > > 
> 
> I tried with gcc 4.3.2. Let me try with a recent version and update.
> 
> > Agreed, this doesn't make any sense. Try changing the compiler version to 
> > see if
> > the problem goes away or stops.  It might also be worthwhile to dump the
> > contents of the device tree at the start and end of the kexec process.  If 
> > the
> > changing of how a function is inlined is causing a hang, its likely 
> > changing how
> > the putprops function is writing information to the device tree.  
> > Understanding
> > what that change is will likely provide clues to how the code has changed.
> 
> Neil, there was no code change in fs2dt.c
> 
Sure there was, by modifying the code to tell it to not inline the putprops
function, you changed how the complier optimizes that code block.  There may not
be any source level code change, but the assembly is certainly different, and it
must be so in a way thats causing a hang.  The putpops function changes the
contents of the ppc64 device-tree, so if this is a compiler bug, and its causing
a hang, I imagine we'll see some manifestation in a change in the device tree
contents.

Neil

> Regards,
> M. Mohan Kumar
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


Re: [PATCH] Do not inline putprops function

2009-06-17 Thread Neil Horman
On Wed, Jun 17, 2009 at 07:56:52PM +0530, M. Mohan Kumar wrote:
> On Wed, Jun 17, 2009 at 10:05:14AM -0400, Neil Horman wrote:
> > On Wed, Jun 17, 2009 at 07:04:35PM +0530, M. Mohan Kumar wrote:
> > > On Wed, Jun 17, 2009 at 09:04:13AM -0400, Neil Horman wrote:
> > > > On Wed, Jun 17, 2009 at 10:26:35PM +1000, Michael Ellerman wrote:
> > > > > 
> > > > > What compiler version are you using? Does the behaviour change if you
> > > > > use a newer/older compiler? It sounds to me like there's some deeper 
> > > > > bug
> > > > > and your patch is just papering over it.
> > > > > 
> > > 
> > > I tried with gcc 4.3.2. Let me try with a recent version and update.
> > > 
> > > > Agreed, this doesn't make any sense. Try changing the compiler version 
> > > > to see if
> > > > the problem goes away or stops.  It might also be worthwhile to dump the
> > > > contents of the device tree at the start and end of the kexec process.  
> > > > If the
> > > > changing of how a function is inlined is causing a hang, its likely 
> > > > changing how
> > > > the putprops function is writing information to the device tree.  
> > > > Understanding
> > > > what that change is will likely provide clues to how the code has 
> > > > changed.
> > > 
> > > Neil, there was no code change in fs2dt.c
> > > 
> > Sure there was, by modifying the code to tell it to not inline the putprops
> > function, you changed how the complier optimizes that code block.  There 
> > may not
> > be any source level code change, but the assembly is certainly different, 
> > and it
> > must be so in a way thats causing a hang.  The putpops function changes the
> > contents of the ppc64 device-tree, so if this is a compiler bug, and its 
> > causing
> > a hang, I imagine we'll see some manifestation in a change in the device 
> > tree
> > contents.
> 
> As I mentioned in the patch description, when I retained the variable dt_len
> and dt_len = dt assignment, kexec/kdump kernel would work. But even if I
> comment the "dt_len = dt" assignment kernel would hang. If you need I can
Yes, and as Michael has noted, that is likey an indication of a compiler bug.
You shouldn't get differing behavior in a kdump kernel by removing an unused
variable in kexec.  Which is why I'm unconvinced this patch is really fixing
anything, but rather just avoiding the real problem.

> send objdump of fs2dt.o with and without this assignment.
> 
That would be a fine thing to do, and I'd be happy to compare them.  My though
regarding the comparison of the device tree on a good and bad run was meant to
expidite what change in the assembly we'd be looking for.  If its the kdump
kernel boot thats hanging, Its likely hanging on something in the device tree,
as thats whats being manipulated by this code.  So I figure that understanding
whats changed there will point us toward what change in the assembly might be
responsible for the hang.  The assmebly's going to be signficantly different
(lots of optimization might be lost from no longer inlining a function), so
anything that helps us narrow down whats changed will be good
Neil

> Regards,
> M. Mohan Kumar.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


Re: [PATCH] clean up of netif_rx_reschedule() changes in drivers

2008-12-30 Thread Neil Horman
On Mon, Dec 29, 2008 at 06:19:08PM -0800, David Miller wrote:
> From: Neil Horman 
> Date: Mon, 29 Dec 2008 10:37:57 -0500
> 
> > On Mon, Dec 29, 2008 at 09:41:44PM +1100, Stephen Rothwell wrote:
> > > Hi Kamalesh,
> > > 
> > > On Mon, 29 Dec 2008 15:55:29 +0530 Kamalesh Babulal 
> > >  wrote:
> > > > greping through the sources for the changes missed out, we have
> > > > 
> > >   .
> > >   .
> > > > 
> > > > Signed-off-by: Kamalesh Babulal 
>  ...
> > Thanks guys, not sure how my patch missed that.  I did an allmodconfig build
> > that passed successfully.  Thanks!
> 
> It's very simple, you're not building on powerpc or ARM :-)
> 
Doh!  Stupid of me.  apologies.

> > Acked-by: Neil Horman 
> 
> Applied, thanks everyone.
> 
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev


Re: [PATCH] i2c: don't print error when adding adapter fails

2016-08-09 Thread Neil Horman
00644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -932,10 +932,8 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>   i2c_dev->adapter.dev.of_node = pdev->dev.of_node;
>  
>   ret = i2c_add_numbered_adapter(&i2c_dev->adapter);
> - if (ret) {
> - dev_err(&pdev->dev, "Failed to add I2C adapter\n");
> + if (ret)
>   goto disable_div_clk;
> - }
>  
>   return 0;
>  
> diff --git a/drivers/i2c/busses/i2c-uniphier-f.c 
> b/drivers/i2c/busses/i2c-uniphier-f.c
> index aeead0d27d1007..64318e69089439 100644
> --- a/drivers/i2c/busses/i2c-uniphier-f.c
> +++ b/drivers/i2c/busses/i2c-uniphier-f.c
> @@ -550,15 +550,10 @@ static int uniphier_fi2c_probe(struct platform_device 
> *pdev)
>   }
>  
>   ret = i2c_add_adapter(&priv->adap);
> - if (ret) {
> - dev_err(dev, "failed to add I2C adapter\n");
> - goto err;
> - }
> -
> -err:
>   if (ret)
>   clk_disable_unprepare(priv->clk);
>  
> + err:
>   return ret;
>  }
>  
> diff --git a/drivers/i2c/busses/i2c-uniphier.c 
> b/drivers/i2c/busses/i2c-uniphier.c
> index 475a5eb514e215..94f64cccfdef08 100644
> --- a/drivers/i2c/busses/i2c-uniphier.c
> +++ b/drivers/i2c/busses/i2c-uniphier.c
> @@ -407,15 +407,10 @@ static int uniphier_i2c_probe(struct platform_device 
> *pdev)
>   }
>  
>   ret = i2c_add_adapter(&priv->adap);
> - if (ret) {
> - dev_err(dev, "failed to add I2C adapter\n");
> - goto err;
> - }
> -
> -err:
>   if (ret)
>   clk_disable_unprepare(priv->clk);
>  
> + err:
>   return ret;
>  }
>  
> diff --git a/drivers/i2c/busses/i2c-wmt.c b/drivers/i2c/busses/i2c-wmt.c
> index e1e3a85596c562..fbd0fd59f31239 100644
> --- a/drivers/i2c/busses/i2c-wmt.c
> +++ b/drivers/i2c/busses/i2c-wmt.c
> @@ -432,10 +432,8 @@ static int wmt_i2c_probe(struct platform_device *pdev)
>   }
>  
>   err = i2c_add_adapter(adap);
> - if (err) {
> - dev_err(&pdev->dev, "failed to add adapter\n");
> + if (err)
>   return err;
> - }
>  
>   platform_set_drvdata(pdev, i2c_dev);
>  
> diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c 
> b/drivers/i2c/busses/i2c-xgene-slimpro.c
> index 4233f5695352fd..263685c7a51287 100644
> --- a/drivers/i2c/busses/i2c-xgene-slimpro.c
> +++ b/drivers/i2c/busses/i2c-xgene-slimpro.c
> @@ -418,7 +418,6 @@ static int xgene_slimpro_i2c_probe(struct platform_device 
> *pdev)
>   i2c_set_adapdata(adapter, ctx);
>   rc = i2c_add_adapter(adapter);
>   if (rc) {
> - dev_err(&pdev->dev, "Adapter registeration failed\n");
>   mbox_free_channel(ctx->mbox_chan);
>   return rc;
>   }
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c
> index 74f54f2f471fa7..66bce3b311a199 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -804,7 +804,6 @@ static int xiic_i2c_probe(struct platform_device *pdev)
>   /* add i2c adapter to i2c tree */
>   ret = i2c_add_adapter(&i2c->adap);
>   if (ret) {
> - dev_err(&pdev->dev, "Failed to add adapter\n");
>   xiic_deinit(i2c);
>   goto err_clk_dis;
>   }
> diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
> index 55a7bef1b2e1df..2a972ed7aa0df1 100644
> --- a/drivers/i2c/busses/i2c-xlp9xx.c
> +++ b/drivers/i2c/busses/i2c-xlp9xx.c
> @@ -400,10 +400,8 @@ static int xlp9xx_i2c_probe(struct platform_device *pdev)
>   i2c_set_adapdata(&priv->adapter, priv);
>  
>   err = i2c_add_adapter(&priv->adapter);
> - if (err) {
> - dev_err(&pdev->dev, "failed to add I2C adapter!\n");
> + if (err)
>   return err;
> - }
>  
>   platform_set_drvdata(pdev, priv);
>   dev_dbg(&pdev->dev, "I2C bus:%d added\n", priv->adapter.nr);
> diff --git a/drivers/i2c/busses/i2c-xlr.c b/drivers/i2c/busses/i2c-xlr.c
> index 613c3a4f2c5142..0968f59b6df586 100644
> --- a/drivers/i2c/busses/i2c-xlr.c
> +++ b/drivers/i2c/busses/i2c-xlr.c
> @@ -432,10 +432,8 @@ static int xlr_i2c_probe(struct platform_device *pdev)
>  
>   i2c_set_adapdata(&priv->adap, priv);
>   ret = i2c_add_numbered_adapter(&priv->adap);
> - if (ret < 0) {
> - dev_err(&priv->adap.dev, "Failed to add i2c bus.\n");
> + if (ret < 0)
>   return ret;
> - }
>  
>   platform_set_drvdata(pdev, priv);
>   dev_info(&priv->adap.dev, "Added I2C Bus.\n");
> -- 
> 2.8.1
> 
> 
Acked-by: Neil Horman