From: Bernd Schmidt <[email protected]>
This just adds minimum support for the Blackfin relocations,
since we don't have enough space in each reloc. The idea
is to store a value with one relocation so that subsequent ones can
access it.
Actually, this patch is required for Blackfin. Currently if BINFMT_FLAT
is enabled, git-tree kernel will fail to compile. Please consider to
accept it.
Signed-off-by: Bernd Schmidt <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
Cc: David McCullough <[email protected]>
Cc: Greg Ungerer <[email protected]>
---
fs/binfmt_flat.c | 5 ++++-
include/asm-h8300/flat.h | 3 ++-
include/asm-m32r/flat.h | 3 ++-
include/asm-m68knommu/flat.h | 3 ++-
include/asm-sh/flat.h | 3 ++-
include/asm-v850/flat.h | 4 +++-
6 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
index 7b0265d..13b58f8 100644
--- a/fs/binfmt_flat.c
+++ b/fs/binfmt_flat.c
@@ -742,6 +742,7 @@ static int load_flat_file(struct linux_binprm * bprm,
* __start to address 4 so that is okay).
*/
if (rev > OLD_FLAT_VERSION) {
+ unsigned long persistent = 0;
for (i=0; i < relocs; i++) {
unsigned long addr, relval;
@@ -749,6 +750,8 @@ static int load_flat_file(struct linux_binprm * bprm,
relocated (of course, the address has to be
relocated first). */
relval = ntohl(reloc[i]);
+ if (flat_set_persistent (relval, &persistent))
+ continue;
addr = flat_get_relocate_addr(relval);
rp = (unsigned long *) calc_reloc(addr, libinfo, id, 1);
if (rp == (unsigned long *)RELOC_FAILED) {
@@ -757,7 +760,7 @@ static int load_flat_file(struct linux_binprm * bprm,
}
/* Get the pointer's value. */
- addr = flat_get_addr_from_rp(rp, relval, flags);
+ addr = flat_get_addr_from_rp(rp, relval, flags, &persistent);
if (addr != 0) {
/*
* Do the relocation. PIC relocs in the data section are
diff --git a/include/asm-h8300/flat.h b/include/asm-h8300/flat.h
index c20eee7..2a87350 100644
--- a/include/asm-h8300/flat.h
+++ b/include/asm-h8300/flat.h
@@ -9,6 +9,7 @@
#define flat_argvp_envp_on_stack() 1
#define flat_old_ram_flag(flags) 1
#define flat_reloc_valid(reloc, size) ((reloc) <= (size))
+#define flat_set_persistent(relval, p) 0
/*
* on the H8 a couple of the relocations have an instruction in the
@@ -18,7 +19,7 @@
*/
#define flat_get_relocate_addr(rel) (rel)
-#define flat_get_addr_from_rp(rp, relval, flags) \
+#define flat_get_addr_from_rp(rp, relval, flags, persistent) \
(get_unaligned(rp) & ((flags & FLAT_FLAG_GOTPIC) ? 0xffffffff: 0x00ffffff))
#define flat_put_addr_at_rp(rp, addr, rel) \
put_unaligned (((*(char *)(rp)) << 24) | ((addr) & 0x00ffffff), rp)
diff --git a/include/asm-m32r/flat.h b/include/asm-m32r/flat.h
index 1b285f6..d851cf0 100644
--- a/include/asm-m32r/flat.h
+++ b/include/asm-m32r/flat.h
@@ -15,9 +15,10 @@
#define flat_stack_align(sp) (*sp += (*sp & 3 ? (4 - (*sp & 3)): 0))
#define flat_argvp_envp_on_stack() 0
#define flat_old_ram_flag(flags) (flags)
+#define flat_set_persistent(relval, p) 0
#define flat_reloc_valid(reloc, size) \
(((reloc) - textlen_for_m32r_lo16_data) <= (size))
-#define flat_get_addr_from_rp(rp, relval, flags) \
+#define flat_get_addr_from_rp(rp, relval, flags, persistent) \
m32r_flat_get_addr_from_rp(rp, relval, (text_len) )
#define flat_put_addr_at_rp(rp, addr, relval) \
diff --git a/include/asm-m68knommu/flat.h b/include/asm-m68knommu/flat.h
index 2d836ed..814b517 100644
--- a/include/asm-m68knommu/flat.h
+++ b/include/asm-m68knommu/flat.h
@@ -9,8 +9,9 @@
#define flat_argvp_envp_on_stack() 1
#define flat_old_ram_flag(flags) (flags)
#define flat_reloc_valid(reloc, size) ((reloc) <= (size))
-#define flat_get_addr_from_rp(rp, relval, flags) get_unaligned(rp)
+#define flat_get_addr_from_rp(rp, relval, flags, p) get_unaligned(rp)
#define flat_put_addr_at_rp(rp, val, relval) put_unaligned(val,rp)
#define flat_get_relocate_addr(rel) (rel)
+#define flat_set_persistent(relval, p) 0
#endif /* __M68KNOMMU_FLAT_H__ */
diff --git a/include/asm-sh/flat.h b/include/asm-sh/flat.h
index 0d5cc04..dc4f595 100644
--- a/include/asm-sh/flat.h
+++ b/include/asm-sh/flat.h
@@ -16,8 +16,9 @@
#define flat_argvp_envp_on_stack() 0
#define flat_old_ram_flag(flags) (flags)
#define flat_reloc_valid(reloc, size) ((reloc) <= (size))
-#define flat_get_addr_from_rp(rp, relval, flags) get_unaligned(rp)
+#define flat_get_addr_from_rp(rp, relval, flags, p) get_unaligned(rp)
#define flat_put_addr_at_rp(rp, val, relval) put_unaligned(val,rp)
#define flat_get_relocate_addr(rel) (rel)
+#define flat_set_persistent(relval, p) 0
#endif /* __ASM_SH_FLAT_H */
diff --git a/include/asm-v850/flat.h b/include/asm-v850/flat.h
index 3888f59..17f0ea5 100644
--- a/include/asm-v850/flat.h
+++ b/include/asm-v850/flat.h
@@ -25,6 +25,7 @@
#define flat_stack_align(sp) /* nothing needed */
#define flat_argvp_envp_on_stack() 0
#define flat_old_ram_flag(flags) (flags)
+#define flat_set_persistent(relval, p) 0
/* We store the type of relocation in the top 4 bits of the `relval.' */
@@ -46,7 +47,8 @@ flat_get_relocate_addr (unsigned long relval)
For the v850, RP should always be half-word aligned. */
static inline unsigned long flat_get_addr_from_rp (unsigned long *rp,
unsigned long relval,
- unsigned long flags)
+ unsigned long flags,
+ unsigned long *persistent)
{
short *srp = (short *)rp;
--
1.5.2
On Tue, 2007-09-18 at 16:09 +0800, Bryan Wu wrote:
> From: Bernd Schmidt <[email protected]>
>
> This just adds minimum support for the Blackfin relocations,
> since we don't have enough space in each reloc. The idea
> is to store a value with one relocation so that subsequent ones can
> access it.
>
> Actually, this patch is required for Blackfin. Currently if BINFMT_FLAT
> is enabled, git-tree kernel will fail to compile. Please consider to
> accept it.
>
As this patch exits for a long time, it might be ignored by maintainers.
I just resent it, because it is necessary for Blackfin bimfmt_flat
configuration and compiling.
Any comments are welcome.
Thanks
- Bryan Wu
> Signed-off-by: Bernd Schmidt <[email protected]>
> Signed-off-by: Bryan Wu <[email protected]>
> Cc: David McCullough <[email protected]>
> Cc: Greg Ungerer <[email protected]>
> ---
> fs/binfmt_flat.c | 5 ++++-
> include/asm-h8300/flat.h | 3 ++-
> include/asm-m32r/flat.h | 3 ++-
> include/asm-m68knommu/flat.h | 3 ++-
> include/asm-sh/flat.h | 3 ++-
> include/asm-v850/flat.h | 4 +++-
> 6 files changed, 15 insertions(+), 6 deletions(-)
> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
> index 7b0265d..13b58f8 100644
> --- a/fs/binfmt_flat.c
> +++ b/fs/binfmt_flat.c
> @@ -742,6 +742,7 @@ static int load_flat_file(struct linux_binprm * bprm,
> * __start to address 4 so that is okay).
> */
> if (rev > OLD_FLAT_VERSION) {
> + unsigned long persistent = 0;
> for (i=0; i < relocs; i++) {
> unsigned long addr, relval;
>
> @@ -749,6 +750,8 @@ static int load_flat_file(struct linux_binprm * bprm,
> relocated (of course, the address has to be
> relocated first). */
> relval = ntohl(reloc[i]);
> + if (flat_set_persistent (relval, &persistent))
> + continue;
> addr = flat_get_relocate_addr(relval);
> rp = (unsigned long *) calc_reloc(addr, libinfo, id, 1);
> if (rp == (unsigned long *)RELOC_FAILED) {
> @@ -757,7 +760,7 @@ static int load_flat_file(struct linux_binprm * bprm,
> }
>
> /* Get the pointer's value. */
> - addr = flat_get_addr_from_rp(rp, relval, flags);
> + addr = flat_get_addr_from_rp(rp, relval, flags, &persistent);
> if (addr != 0) {
> /*
> * Do the relocation. PIC relocs in the data section are
> diff --git a/include/asm-h8300/flat.h b/include/asm-h8300/flat.h
> index c20eee7..2a87350 100644
> --- a/include/asm-h8300/flat.h
> +++ b/include/asm-h8300/flat.h
> @@ -9,6 +9,7 @@
> #define flat_argvp_envp_on_stack() 1
> #define flat_old_ram_flag(flags) 1
> #define flat_reloc_valid(reloc, size) ((reloc) <= (size))
> +#define flat_set_persistent(relval, p) 0
>
> /*
> * on the H8 a couple of the relocations have an instruction in the
> @@ -18,7 +19,7 @@
> */
>
> #define flat_get_relocate_addr(rel) (rel)
> -#define flat_get_addr_from_rp(rp, relval, flags) \
> +#define flat_get_addr_from_rp(rp, relval, flags, persistent) \
> (get_unaligned(rp) & ((flags & FLAT_FLAG_GOTPIC) ? 0xffffffff: 0x00ffffff))
> #define flat_put_addr_at_rp(rp, addr, rel) \
> put_unaligned (((*(char *)(rp)) << 24) | ((addr) & 0x00ffffff), rp)
> diff --git a/include/asm-m32r/flat.h b/include/asm-m32r/flat.h
> index 1b285f6..d851cf0 100644
> --- a/include/asm-m32r/flat.h
> +++ b/include/asm-m32r/flat.h
> @@ -15,9 +15,10 @@
> #define flat_stack_align(sp) (*sp += (*sp & 3 ? (4 - (*sp & 3)): 0))
> #define flat_argvp_envp_on_stack() 0
> #define flat_old_ram_flag(flags) (flags)
> +#define flat_set_persistent(relval, p) 0
> #define flat_reloc_valid(reloc, size) \
> (((reloc) - textlen_for_m32r_lo16_data) <= (size))
> -#define flat_get_addr_from_rp(rp, relval, flags) \
> +#define flat_get_addr_from_rp(rp, relval, flags, persistent) \
> m32r_flat_get_addr_from_rp(rp, relval, (text_len) )
>
> #define flat_put_addr_at_rp(rp, addr, relval) \
> diff --git a/include/asm-m68knommu/flat.h b/include/asm-m68knommu/flat.h
> index 2d836ed..814b517 100644
> --- a/include/asm-m68knommu/flat.h
> +++ b/include/asm-m68knommu/flat.h
> @@ -9,8 +9,9 @@
> #define flat_argvp_envp_on_stack() 1
> #define flat_old_ram_flag(flags) (flags)
> #define flat_reloc_valid(reloc, size) ((reloc) <= (size))
> -#define flat_get_addr_from_rp(rp, relval, flags) get_unaligned(rp)
> +#define flat_get_addr_from_rp(rp, relval, flags, p) get_unaligned(rp)
> #define flat_put_addr_at_rp(rp, val, relval) put_unaligned(val,rp)
> #define flat_get_relocate_addr(rel) (rel)
> +#define flat_set_persistent(relval, p) 0
>
> #endif /* __M68KNOMMU_FLAT_H__ */
> diff --git a/include/asm-sh/flat.h b/include/asm-sh/flat.h
> index 0d5cc04..dc4f595 100644
> --- a/include/asm-sh/flat.h
> +++ b/include/asm-sh/flat.h
> @@ -16,8 +16,9 @@
> #define flat_argvp_envp_on_stack() 0
> #define flat_old_ram_flag(flags) (flags)
> #define flat_reloc_valid(reloc, size) ((reloc) <= (size))
> -#define flat_get_addr_from_rp(rp, relval, flags) get_unaligned(rp)
> +#define flat_get_addr_from_rp(rp, relval, flags, p) get_unaligned(rp)
> #define flat_put_addr_at_rp(rp, val, relval) put_unaligned(val,rp)
> #define flat_get_relocate_addr(rel) (rel)
> +#define flat_set_persistent(relval, p) 0
>
> #endif /* __ASM_SH_FLAT_H */
> diff --git a/include/asm-v850/flat.h b/include/asm-v850/flat.h
> index 3888f59..17f0ea5 100644
> --- a/include/asm-v850/flat.h
> +++ b/include/asm-v850/flat.h
> @@ -25,6 +25,7 @@
> #define flat_stack_align(sp) /* nothing needed */
> #define flat_argvp_envp_on_stack() 0
> #define flat_old_ram_flag(flags) (flags)
> +#define flat_set_persistent(relval, p) 0
>
> /* We store the type of relocation in the top 4 bits of the `relval.' */
>
> @@ -46,7 +47,8 @@ flat_get_relocate_addr (unsigned long relval)
> For the v850, RP should always be half-word aligned. */
> static inline unsigned long flat_get_addr_from_rp (unsigned long *rp,
> unsigned long relval,
> - unsigned long flags)
> + unsigned long flags,
> + unsigned long *persistent)
> {
> short *srp = (short *)rp;
>
On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
> From: Bernd Schmidt <[email protected]>
>
> This just adds minimum support for the Blackfin relocations,
> since we don't have enough space in each reloc. The idea
> is to store a value with one relocation so that subsequent ones can
> access it.
>
> Signed-off-by: Bernd Schmidt <[email protected]>
> Signed-off-by: Bryan Wu <[email protected]>
> Cc: David McCullough <[email protected]>
> Cc: Greg Ungerer <[email protected]>
Adding the other appropriate maintainers. for h8, m32r, sh and v850.
> ---
> fs/binfmt_flat.c | 5 ++++-
> include/asm-h8300/flat.h | 3 ++-
> include/asm-m32r/flat.h | 3 ++-
> include/asm-m68knommu/flat.h | 3 ++-
> include/asm-sh/flat.h | 3 ++-
> include/asm-v850/flat.h | 4 +++-
> 6 files changed, 15 insertions(+), 6 deletions(-)
> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
> index 7b0265d..13b58f8 100644
> --- a/fs/binfmt_flat.c
> +++ b/fs/binfmt_flat.c
> @@ -742,6 +742,7 @@ static int load_flat_file(struct linux_binprm * bprm,
> * __start to address 4 so that is okay).
> */
> if (rev > OLD_FLAT_VERSION) {
> + unsigned long persistent = 0;
> for (i=0; i < relocs; i++) {
> unsigned long addr, relval;
>
> @@ -749,6 +750,8 @@ static int load_flat_file(struct linux_binprm * bprm,
> relocated (of course, the address has to be
> relocated first). */
> relval = ntohl(reloc[i]);
> + if (flat_set_persistent (relval, &persistent))
> + continue;
> addr = flat_get_relocate_addr(relval);
> rp = (unsigned long *) calc_reloc(addr, libinfo, id, 1);
> if (rp == (unsigned long *)RELOC_FAILED) {
> @@ -757,7 +760,7 @@ static int load_flat_file(struct linux_binprm * bprm,
> }
>
> /* Get the pointer's value. */
> - addr = flat_get_addr_from_rp(rp, relval, flags);
> + addr = flat_get_addr_from_rp(rp, relval, flags, &persistent);
> if (addr != 0) {
> /*
> * Do the relocation. PIC relocs in the data section are
> diff --git a/include/asm-h8300/flat.h b/include/asm-h8300/flat.h
> index c20eee7..2a87350 100644
> --- a/include/asm-h8300/flat.h
> +++ b/include/asm-h8300/flat.h
> @@ -9,6 +9,7 @@
> #define flat_argvp_envp_on_stack() 1
> #define flat_old_ram_flag(flags) 1
> #define flat_reloc_valid(reloc, size) ((reloc) <= (size))
> +#define flat_set_persistent(relval, p) 0
>
> /*
> * on the H8 a couple of the relocations have an instruction in the
> @@ -18,7 +19,7 @@
> */
>
> #define flat_get_relocate_addr(rel) (rel)
> -#define flat_get_addr_from_rp(rp, relval, flags) \
> +#define flat_get_addr_from_rp(rp, relval, flags, persistent) \
> (get_unaligned(rp) & ((flags & FLAT_FLAG_GOTPIC) ? 0xffffffff: 0x00ffffff))
> #define flat_put_addr_at_rp(rp, addr, rel) \
> put_unaligned (((*(char *)(rp)) << 24) | ((addr) & 0x00ffffff), rp)
> diff --git a/include/asm-m32r/flat.h b/include/asm-m32r/flat.h
> index 1b285f6..d851cf0 100644
> --- a/include/asm-m32r/flat.h
> +++ b/include/asm-m32r/flat.h
> @@ -15,9 +15,10 @@
> #define flat_stack_align(sp) (*sp += (*sp & 3 ? (4 - (*sp & 3)): 0))
> #define flat_argvp_envp_on_stack() 0
> #define flat_old_ram_flag(flags) (flags)
> +#define flat_set_persistent(relval, p) 0
> #define flat_reloc_valid(reloc, size) \
> (((reloc) - textlen_for_m32r_lo16_data) <= (size))
> -#define flat_get_addr_from_rp(rp, relval, flags) \
> +#define flat_get_addr_from_rp(rp, relval, flags, persistent) \
> m32r_flat_get_addr_from_rp(rp, relval, (text_len) )
>
> #define flat_put_addr_at_rp(rp, addr, relval) \
> diff --git a/include/asm-m68knommu/flat.h b/include/asm-m68knommu/flat.h
> index 2d836ed..814b517 100644
> --- a/include/asm-m68knommu/flat.h
> +++ b/include/asm-m68knommu/flat.h
> @@ -9,8 +9,9 @@
> #define flat_argvp_envp_on_stack() 1
> #define flat_old_ram_flag(flags) (flags)
> #define flat_reloc_valid(reloc, size) ((reloc) <= (size))
> -#define flat_get_addr_from_rp(rp, relval, flags) get_unaligned(rp)
> +#define flat_get_addr_from_rp(rp, relval, flags, p) get_unaligned(rp)
> #define flat_put_addr_at_rp(rp, val, relval) put_unaligned(val,rp)
> #define flat_get_relocate_addr(rel) (rel)
> +#define flat_set_persistent(relval, p) 0
>
> #endif /* __M68KNOMMU_FLAT_H__ */
> diff --git a/include/asm-sh/flat.h b/include/asm-sh/flat.h
> index 0d5cc04..dc4f595 100644
> --- a/include/asm-sh/flat.h
> +++ b/include/asm-sh/flat.h
> @@ -16,8 +16,9 @@
> #define flat_argvp_envp_on_stack() 0
> #define flat_old_ram_flag(flags) (flags)
> #define flat_reloc_valid(reloc, size) ((reloc) <= (size))
> -#define flat_get_addr_from_rp(rp, relval, flags) get_unaligned(rp)
> +#define flat_get_addr_from_rp(rp, relval, flags, p) get_unaligned(rp)
> #define flat_put_addr_at_rp(rp, val, relval) put_unaligned(val,rp)
> #define flat_get_relocate_addr(rel) (rel)
> +#define flat_set_persistent(relval, p) 0
>
> #endif /* __ASM_SH_FLAT_H */
> diff --git a/include/asm-v850/flat.h b/include/asm-v850/flat.h
> index 3888f59..17f0ea5 100644
> --- a/include/asm-v850/flat.h
> +++ b/include/asm-v850/flat.h
> @@ -25,6 +25,7 @@
> #define flat_stack_align(sp) /* nothing needed */
> #define flat_argvp_envp_on_stack() 0
> #define flat_old_ram_flag(flags) (flags)
> +#define flat_set_persistent(relval, p) 0
>
> /* We store the type of relocation in the top 4 bits of the `relval.'
> */
>
> @@ -46,7 +47,8 @@ flat_get_relocate_addr (unsigned long relval)
> For the v850, RP should always be half-word aligned. */
> static inline unsigned long flat_get_addr_from_rp (unsigned long *rp,
> unsigned long relval,
> - unsigned long flags)
> + unsigned long flags,
> + unsigned long *persistent)
> {
> short *srp = (short *)rp;
Jivin Robin Getz lays it down ...
> On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
> > From: Bernd Schmidt <[email protected]>
> >
> > This just adds minimum support for the Blackfin relocations,
> > since we don't have enough space in each reloc. The idea
> > is to store a value with one relocation so that subsequent ones can
> > access it.
> >
> > Signed-off-by: Bernd Schmidt <[email protected]>
> > Signed-off-by: Bryan Wu <[email protected]>
> > Cc: David McCullough <[email protected]>
> > Cc: Greg Ungerer <[email protected]>
>
> Adding the other appropriate maintainers. for h8, m32r, sh and v850.
Looks fine to me, obviously impacts the existing arches very little.
Can't see why it shouldn't get included,
Cheers,
Davidm
> > ---
> > fs/binfmt_flat.c | 5 ++++-
> > include/asm-h8300/flat.h | 3 ++-
> > include/asm-m32r/flat.h | 3 ++-
> > include/asm-m68knommu/flat.h | 3 ++-
> > include/asm-sh/flat.h | 3 ++-
> > include/asm-v850/flat.h | 4 +++-
> > 6 files changed, 15 insertions(+), 6 deletions(-)
> > diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
> > index 7b0265d..13b58f8 100644
> > --- a/fs/binfmt_flat.c
> > +++ b/fs/binfmt_flat.c
> > @@ -742,6 +742,7 @@ static int load_flat_file(struct linux_binprm * bprm,
> > * __start to address 4 so that is okay).
> > */
> > if (rev > OLD_FLAT_VERSION) {
> > + unsigned long persistent = 0;
> > for (i=0; i < relocs; i++) {
> > unsigned long addr, relval;
> >
> > @@ -749,6 +750,8 @@ static int load_flat_file(struct linux_binprm * bprm,
> > relocated (of course, the address has to be
> > relocated first). */
> > relval = ntohl(reloc[i]);
> > + if (flat_set_persistent (relval, &persistent))
> > + continue;
> > addr = flat_get_relocate_addr(relval);
> > rp = (unsigned long *) calc_reloc(addr, libinfo, id, 1);
> > if (rp == (unsigned long *)RELOC_FAILED) {
> > @@ -757,7 +760,7 @@ static int load_flat_file(struct linux_binprm * bprm,
> > }
> >
> > /* Get the pointer's value. */
> > - addr = flat_get_addr_from_rp(rp, relval, flags);
> > + addr = flat_get_addr_from_rp(rp, relval, flags, &persistent);
> > if (addr != 0) {
> > /*
> > * Do the relocation. PIC relocs in the data section are
> > diff --git a/include/asm-h8300/flat.h b/include/asm-h8300/flat.h
> > index c20eee7..2a87350 100644
> > --- a/include/asm-h8300/flat.h
> > +++ b/include/asm-h8300/flat.h
> > @@ -9,6 +9,7 @@
> > #define flat_argvp_envp_on_stack() 1
> > #define flat_old_ram_flag(flags) 1
> > #define flat_reloc_valid(reloc, size) ((reloc) <= (size))
> > +#define flat_set_persistent(relval, p) 0
> >
> > /*
> > * on the H8 a couple of the relocations have an instruction in the
> > @@ -18,7 +19,7 @@
> > */
> >
> > #define flat_get_relocate_addr(rel) (rel)
> > -#define flat_get_addr_from_rp(rp, relval, flags) \
> > +#define flat_get_addr_from_rp(rp, relval, flags, persistent) \
> > (get_unaligned(rp) & ((flags & FLAT_FLAG_GOTPIC) ? 0xffffffff: 0x00ffffff))
> > #define flat_put_addr_at_rp(rp, addr, rel) \
> > put_unaligned (((*(char *)(rp)) << 24) | ((addr) & 0x00ffffff), rp)
> > diff --git a/include/asm-m32r/flat.h b/include/asm-m32r/flat.h
> > index 1b285f6..d851cf0 100644
> > --- a/include/asm-m32r/flat.h
> > +++ b/include/asm-m32r/flat.h
> > @@ -15,9 +15,10 @@
> > #define flat_stack_align(sp) (*sp += (*sp & 3 ? (4 - (*sp & 3)): 0))
> > #define flat_argvp_envp_on_stack() 0
> > #define flat_old_ram_flag(flags) (flags)
> > +#define flat_set_persistent(relval, p) 0
> > #define flat_reloc_valid(reloc, size) \
> > (((reloc) - textlen_for_m32r_lo16_data) <= (size))
> > -#define flat_get_addr_from_rp(rp, relval, flags) \
> > +#define flat_get_addr_from_rp(rp, relval, flags, persistent) \
> > m32r_flat_get_addr_from_rp(rp, relval, (text_len) )
> >
> > #define flat_put_addr_at_rp(rp, addr, relval) \
> > diff --git a/include/asm-m68knommu/flat.h b/include/asm-m68knommu/flat.h
> > index 2d836ed..814b517 100644
> > --- a/include/asm-m68knommu/flat.h
> > +++ b/include/asm-m68knommu/flat.h
> > @@ -9,8 +9,9 @@
> > #define flat_argvp_envp_on_stack() 1
> > #define flat_old_ram_flag(flags) (flags)
> > #define flat_reloc_valid(reloc, size) ((reloc) <= (size))
> > -#define flat_get_addr_from_rp(rp, relval, flags) get_unaligned(rp)
> > +#define flat_get_addr_from_rp(rp, relval, flags, p) get_unaligned(rp)
> > #define flat_put_addr_at_rp(rp, val, relval) put_unaligned(val,rp)
> > #define flat_get_relocate_addr(rel) (rel)
> > +#define flat_set_persistent(relval, p) 0
> >
> > #endif /* __M68KNOMMU_FLAT_H__ */
> > diff --git a/include/asm-sh/flat.h b/include/asm-sh/flat.h
> > index 0d5cc04..dc4f595 100644
> > --- a/include/asm-sh/flat.h
> > +++ b/include/asm-sh/flat.h
> > @@ -16,8 +16,9 @@
> > #define flat_argvp_envp_on_stack() 0
> > #define flat_old_ram_flag(flags) (flags)
> > #define flat_reloc_valid(reloc, size) ((reloc) <= (size))
> > -#define flat_get_addr_from_rp(rp, relval, flags) get_unaligned(rp)
> > +#define flat_get_addr_from_rp(rp, relval, flags, p) get_unaligned(rp)
> > #define flat_put_addr_at_rp(rp, val, relval) put_unaligned(val,rp)
> > #define flat_get_relocate_addr(rel) (rel)
> > +#define flat_set_persistent(relval, p) 0
> >
> > #endif /* __ASM_SH_FLAT_H */
> > diff --git a/include/asm-v850/flat.h b/include/asm-v850/flat.h
> > index 3888f59..17f0ea5 100644
> > --- a/include/asm-v850/flat.h
> > +++ b/include/asm-v850/flat.h
> > @@ -25,6 +25,7 @@
> > #define flat_stack_align(sp) /* nothing needed */
> > #define flat_argvp_envp_on_stack() 0
> > #define flat_old_ram_flag(flags) (flags)
> > +#define flat_set_persistent(relval, p) 0
> >
> > /* We store the type of relocation in the top 4 bits of the `relval.'
> > */
> >
> > @@ -46,7 +47,8 @@ flat_get_relocate_addr (unsigned long relval)
> > For the v850, RP should always be half-word aligned. */
> > static inline unsigned long flat_get_addr_from_rp (unsigned long *rp,
> > unsigned long relval,
> > - unsigned long flags)
> > + unsigned long flags,
> > + unsigned long *persistent)
> > {
> > short *srp = (short *)rp;
--
David McCullough, [email protected], Ph:+61 734352815
Secure Computing - SnapGear http://www.uCdot.org http://www.cyberguard.com
On Wed 19 Sep 2007 21:55, David McCullough pondered:
> Jivin Robin Getz lays it down ...
> > On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
> > > From: Bernd Schmidt <[email protected]>
> > >
> > > This just adds minimum support for the Blackfin relocations,
> > > since we don't have enough space in each reloc. The idea
> > > is to store a value with one relocation so that subsequent ones can
> > > access it.
> > >
> > > Signed-off-by: Bernd Schmidt <[email protected]>
> > > Signed-off-by: Bryan Wu <[email protected]>
> > > Cc: David McCullough <[email protected]>
> > > Cc: Greg Ungerer <[email protected]>
> >
> > Adding the other appropriate maintainers. for h8, m32r, sh and v850.
>
> Looks fine to me, obviously impacts the existing arches very little.
> Can't see why it shouldn't get included,
Is that Australian for Acked-by:? :)
-Robin
On Thu, Sep 20, 2007 at 11:55:25AM +1000, David McCullough wrote:
> Jivin Robin Getz lays it down ...
> > On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
> > > This just adds minimum support for the Blackfin relocations,
> > > since we don't have enough space in each reloc. The idea
> > > is to store a value with one relocation so that subsequent ones can
> > > access it.
> >
> > Adding the other appropriate maintainers. for h8, m32r, sh and v850.
>
> Looks fine to me, obviously impacts the existing arches very little.
> Can't see why it shouldn't get included,
>
I find it a bit disconcerting that blackfin already depends on this
in-tree without there being any earlier discussion on making these
changes.
> > > */
> > > if (rev > OLD_FLAT_VERSION) {
> > > + unsigned long persistent = 0;
> > > for (i=0; i < relocs; i++) {
> > > unsigned long addr, relval;
> > >
> > > @@ -749,6 +750,8 @@ static int load_flat_file(struct linux_binprm * bprm,
> > > relocated (of course, the address has to be
> > > relocated first). */
> > > relval = ntohl(reloc[i]);
> > > + if (flat_set_persistent (relval, &persistent))
> > > + continue;
> > > addr = flat_get_relocate_addr(relval);
> > > rp = (unsigned long *) calc_reloc(addr, libinfo, id, 1);
> > > if (rp == (unsigned long *)RELOC_FAILED) {
I don't much care for this API. It's shuffling around a temporary
variable for the architecture code that's set for certain relocations
that are otherwise unhandled.
Since all the architecture is interested in is the relval that has
associated "persistent" data encoded in it, why don't we just have a stub
to give the architecture a chance to validate the relval before the
flat_get_relocate_addr() and move this stuff there instead? ie, blackfin
takes this out-of-line and manages its persistent value there.
> > > @@ -757,7 +760,7 @@ static int load_flat_file(struct linux_binprm * bprm,
> > > }
> > >
> > > /* Get the pointer's value. */
> > > - addr = flat_get_addr_from_rp(rp, relval, flags);
> > > + addr = flat_get_addr_from_rp(rp, relval, flags, &persistent);
There's no reason for this to be a pointer. The current in-tree blackfin
code doesn't change this value, and if you have patches that are doing
that, this is even more reason to bury this kind of silliness in your
architecture code.
load_flat_file() is ugly enough without dumping more architecture
callback abuses in it.
On 9/19/07, Paul Mundt <[email protected]> wrote:
> On Thu, Sep 20, 2007 at 11:55:25AM +1000, David McCullough wrote:
> > Jivin Robin Getz lays it down ...
> > > On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
> > > > This just adds minimum support for the Blackfin relocations,
> > > > since we don't have enough space in each reloc. The idea
> > > > is to store a value with one relocation so that subsequent ones can
> > > > access it.
> > >
> > > Adding the other appropriate maintainers. for h8, m32r, sh and v850.
> >
> > Looks fine to me, obviously impacts the existing arches very little.
> > Can't see why it shouldn't get included,
>
> I find it a bit disconcerting that blackfin already depends on this
> in-tree without there being any earlier discussion on making these
> changes.
not really ... this patch was posted before but was lost in the
shuffle ... and i'm not quite sure what you mean by "depends on" ...
if you want to use the FLAT file format on a Blackfin processor, then
this patch is needed, but that isnt the only file format that works on
the Blackfin port as we also have FDPIC ELF
i havent used FLAT files on a Blackfin board in quite a long time actually ...
-mike
On Wed, Sep 19, 2007 at 11:42:53PM -0400, Mike Frysinger wrote:
> On 9/19/07, Paul Mundt <[email protected]> wrote:
> > On Thu, Sep 20, 2007 at 11:55:25AM +1000, David McCullough wrote:
> > > Jivin Robin Getz lays it down ...
> > > > On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
> > > > > This just adds minimum support for the Blackfin relocations,
> > > > > since we don't have enough space in each reloc. The idea
> > > > > is to store a value with one relocation so that subsequent ones can
> > > > > access it.
> > > >
> > > > Adding the other appropriate maintainers. for h8, m32r, sh and v850.
> > >
> > > Looks fine to me, obviously impacts the existing arches very little.
> > > Can't see why it shouldn't get included,
> >
> > I find it a bit disconcerting that blackfin already depends on this
> > in-tree without there being any earlier discussion on making these
> > changes.
>
> not really ... this patch was posted before but was lost in the
> shuffle ... and i'm not quite sure what you mean by "depends on" ...
> if you want to use the FLAT file format on a Blackfin processor, then
> this patch is needed, but that isnt the only file format that works on
> the Blackfin port as we also have FDPIC ELF
>
What I mean by "depends on" is that for what you are attempting to patch,
your architecture has an in-tree dependency on something that hasn't been
discussed. It's more than a bit backwards to push the follow-up bits before
even getting an Ack on the changes you want to make in the common code.
The fact you have other options is great, so at least you haven't shafted
all of your git kernel users, but that's not the point. You should not be
pushing things in to the kernel that have dependencies on API changes
that may or may not be merged, especially when you're breaking the entire
kernel for your platform (and the ability to bisect for those users) in
the process.
On Wed 19 Sep 2007 23:54, Paul Mundt pondered:
> On Wed, Sep 19, 2007 at 11:42:53PM -0400, Mike Frysinger wrote:
> > On 9/19/07, Paul Mundt <[email protected]> wrote:
> > > On Thu, Sep 20, 2007 at 11:55:25AM +1000, David McCullough wrote:
> > > > Jivin Robin Getz lays it down ...
> > > > > On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
> > > > > > This just adds minimum support for the Blackfin relocations,
> > > > > > since we don't have enough space in each reloc. The idea
> > > > > > is to store a value with one relocation so that subsequent
> > > > > > ones can access it.
> > > > >
> > > > > Adding the other appropriate maintainers. for h8, m32r, sh and
> > > > > v850.
> > > >
> > > > Looks fine to me, obviously impacts the existing arches very
> > > > little. Can't see why it shouldn't get included,
> > >
> > > I find it a bit disconcerting that blackfin already depends on this
> > > in-tree without there being any earlier discussion on making these
> > > changes.
> >
> > not really ... this patch was posted before but was lost in the
> > shuffle ... and i'm not quite sure what you mean by "depends on" ...
> > if you want to use the FLAT file format on a Blackfin processor, then
> > this patch is needed, but that isnt the only file format that works on
> > the Blackfin port as we also have FDPIC ELF
> >
> What I mean by "depends on" is that for what you are attempting to
> patch, your architecture has an in-tree dependency on something that hasn't
> been discussed.
"not been discussed" because it was sent to lkml -
http://lkml.org/lkml/2006/9/22/60
- and it got missed/left on the floor during the arch/blackfin inclusion
(which was huge), not because of any deliberate malicious intent on our part
to mislead or try to get this in now by doing an end around as you are
implying.
Our mistake for not poking everyone/resending things sooner/before
arch/blackfin was included. Bryan will try to make sure that doesn't happen
again (right Bryan?) - like he is now, by poking/resending things, and making
sure that the appropriate maintainers (like you, if we are changing things
you maintain) are included. (which is what I think the problem was).
If we can focus on the technical merits of things, rather than how we got
here - which I agree sucks - was our mistake - we are sorry - we will try to
make sure that it doesn't happen again - I think it would be time/effort
better spent.
-Robin
BTW - does anyone know Miles Bader's current email address? There isn't one
listed in MAINTAINERS - and he is still listed for the NEC V850?
On Thu, 2007-09-20 at 02:08 -0400, Robin Getz wrote:
> On Wed 19 Sep 2007 23:54, Paul Mundt pondered:
> > On Wed, Sep 19, 2007 at 11:42:53PM -0400, Mike Frysinger wrote:
> > > On 9/19/07, Paul Mundt <[email protected]> wrote:
> > > > On Thu, Sep 20, 2007 at 11:55:25AM +1000, David McCullough wrote:
> > > > > Jivin Robin Getz lays it down ...
> > > > > > On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
> > > > > > > This just adds minimum support for the Blackfin relocations,
> > > > > > > since we don't have enough space in each reloc. The idea
> > > > > > > is to store a value with one relocation so that subsequent
> > > > > > > ones can access it.
> > > > > >
> > > > > > Adding the other appropriate maintainers. for h8, m32r, sh and
> > > > > > v850.
> > > > >
> > > > > Looks fine to me, obviously impacts the existing arches very
> > > > > little. Can't see why it shouldn't get included,
> > > >
> > > > I find it a bit disconcerting that blackfin already depends on this
> > > > in-tree without there being any earlier discussion on making these
> > > > changes.
> > >
> > > not really ... this patch was posted before but was lost in the
> > > shuffle ... and i'm not quite sure what you mean by "depends on" ...
> > > if you want to use the FLAT file format on a Blackfin processor, then
> > > this patch is needed, but that isnt the only file format that works on
> > > the Blackfin port as we also have FDPIC ELF
> > >
> > What I mean by "depends on" is that for what you are attempting to
> > patch, your architecture has an in-tree dependency on something that hasn't
> > been discussed.
>
> "not been discussed" because it was sent to lkml -
> http://lkml.org/lkml/2006/9/22/60
> - and it got missed/left on the floor during the arch/blackfin inclusion
> (which was huge), not because of any deliberate malicious intent on our part
> to mislead or try to get this in now by doing an end around as you are
> implying.
>
Yes, as Robin pointed out, this patch was sent to LKML at least 3 times
including Bernd's email. This is the 4th round.
http://lkml.org/lkml/2007/5/29/24
http://lkml.org/lkml/2007/5/28/63
I don't wanna to resend it again and again to annoy the receiver and
LKML. But IMO, technically this patch looks fine to binfmt_flat and
other architectures. And the in-tree Blackfin port will fail to be
compiled with this patch if the BINFMT_FLAT is enabled.
> Our mistake for not poking everyone/resending things sooner/before
> arch/blackfin was included. Bryan will try to make sure that doesn't happen
> again (right Bryan?) - like he is now, by poking/resending things, and making
> sure that the appropriate maintainers (like you, if we are changing things
> you maintain) are included. (which is what I think the problem was).
>
> If we can focus on the technical merits of things, rather than how we got
> here - which I agree sucks - was our mistake - we are sorry - we will try to
> make sure that it doesn't happen again - I think it would be time/effort
> better spent.
>
Yes, I will try my best to avoid this happen again. But on the other
hand, several reasons made this mess happen:
a) not very easy found the maintainer information for several domain,
for example this patch should invite binfmt_flat maintainers, arch
maintainers (Thanks Robin to invite them), blackfin port maintainers and
toolchain maintainers.
b) even the maintainers got this patch email, they don't have time to
review or reply. So the patch was ignored by LKML and the sender, sorry
-:))).
c) There is a rule that do __NOT__ send the same patch again and again,
I don't wanna to break this rule. But if there is no feedback about the
patch, we have no choice but resent it or just ignore it.
I know it is general problem in the LKML patch review.
Thanks
-Bryan
On Thu, 2007-09-20 at 14:34 +0800, Bryan Wu wrote:
> On Thu, 2007-09-20 at 02:08 -0400, Robin Getz wrote:
> > On Wed 19 Sep 2007 23:54, Paul Mundt pondered:
> > > On Wed, Sep 19, 2007 at 11:42:53PM -0400, Mike Frysinger wrote:
> > > > On 9/19/07, Paul Mundt <[email protected]> wrote:
> > > > > On Thu, Sep 20, 2007 at 11:55:25AM +1000, David McCullough wrote:
> > > > > > Jivin Robin Getz lays it down ...
> > > > > > > On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
> > > > > > > > This just adds minimum support for the Blackfin relocations,
> > > > > > > > since we don't have enough space in each reloc. The idea
> > > > > > > > is to store a value with one relocation so that subsequent
> > > > > > > > ones can access it.
> > > > > > >
> > > > > > > Adding the other appropriate maintainers. for h8, m32r, sh and
> > > > > > > v850.
> > > > > >
> > > > > > Looks fine to me, obviously impacts the existing arches very
> > > > > > little. Can't see why it shouldn't get included,
> > > > >
> > > > > I find it a bit disconcerting that blackfin already depends on this
> > > > > in-tree without there being any earlier discussion on making these
> > > > > changes.
> > > >
> > > > not really ... this patch was posted before but was lost in the
> > > > shuffle ... and i'm not quite sure what you mean by "depends on" ...
> > > > if you want to use the FLAT file format on a Blackfin processor, then
> > > > this patch is needed, but that isnt the only file format that works on
> > > > the Blackfin port as we also have FDPIC ELF
> > > >
> > > What I mean by "depends on" is that for what you are attempting to
> > > patch, your architecture has an in-tree dependency on something that hasn't
> > > been discussed.
> >
> > "not been discussed" because it was sent to lkml -
> > http://lkml.org/lkml/2006/9/22/60
> > - and it got missed/left on the floor during the arch/blackfin inclusion
> > (which was huge), not because of any deliberate malicious intent on our part
> > to mislead or try to get this in now by doing an end around as you are
> > implying.
> >
>
> Yes, as Robin pointed out, this patch was sent to LKML at least 3 times
> including Bernd's email. This is the 4th round.
> http://lkml.org/lkml/2007/5/29/24
> http://lkml.org/lkml/2007/5/28/63
>
> I don't wanna to resend it again and again to annoy the receiver and
> LKML. But IMO, technically this patch looks fine to binfmt_flat and
> other architectures. And the in-tree Blackfin port will fail to be
> compiled with this patch if the BINFMT_FLAT is enabled.
>
Oops, typo. Correct one: it will fail to be compiled without this
patch.
> > Our mistake for not poking everyone/resending things sooner/before
> > arch/blackfin was included. Bryan will try to make sure that doesn't happen
> > again (right Bryan?) - like he is now, by poking/resending things, and making
> > sure that the appropriate maintainers (like you, if we are changing things
> > you maintain) are included. (which is what I think the problem was).
> >
> > If we can focus on the technical merits of things, rather than how we got
> > here - which I agree sucks - was our mistake - we are sorry - we will try to
> > make sure that it doesn't happen again - I think it would be time/effort
> > better spent.
> >
>
> Yes, I will try my best to avoid this happen again. But on the other
> hand, several reasons made this mess happen:
> a) not very easy found the maintainer information for several domain,
> for example this patch should invite binfmt_flat maintainers, arch
> maintainers (Thanks Robin to invite them), blackfin port maintainers and
> toolchain maintainers.
> b) even the maintainers got this patch email, they don't have time to
> review or reply. So the patch was ignored by LKML and the sender, sorry
> -:))).
> c) There is a rule that do __NOT__ send the same patch again and again,
> I don't wanna to break this rule. But if there is no feedback about the
> patch, we have no choice but resent it or just ignore it.
>
> I know it is general problem in the LKML patch review.
>
> Thanks
> -Bryan
Robin Getz <[email protected]> writes:
> BTW - does anyone know Miles Bader's current email address? There isn't one
> listed in MAINTAINERS - and he is still listed for the NEC V850?
[email protected]
-Miles
--
Everywhere is walking distance if you have the time. -- Steven Wright
From: David McCullough <[email protected]>
Date: Thu, 20 Sep 2007 11:55:25 +1000
>
> Jivin Robin Getz lays it down ...
> > On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
> > > From: Bernd Schmidt <[email protected]>
> > >
> > > This just adds minimum support for the Blackfin relocations,
> > > since we don't have enough space in each reloc. The idea
> > > is to store a value with one relocation so that subsequent ones can
> > > access it.
> > >
> > > Signed-off-by: Bernd Schmidt <[email protected]>
> > > Signed-off-by: Bryan Wu <[email protected]>
> > > Cc: David McCullough <[email protected]>
> > > Cc: Greg Ungerer <[email protected]>
> >
> > Adding the other appropriate maintainers. for h8, m32r, sh and v850.
>
> Looks fine to me, obviously impacts the existing arches very little.
> Can't see why it shouldn't get included,
>
> Cheers,
> Davidm
Looks fine to me, too.
Acked-by: Hirokazu Takata <[email protected]>
--
Hirokazu Takata <[email protected]>
Linux/M32R Project: http://www.linux-m32r.org/
Paul Mundt wrote:
> I find it a bit disconcerting that blackfin already depends on this
> in-tree without there being any earlier discussion on making these
> changes.
Parts of the initial submission were picked up (the include/asm
directory), other's weren't. Little we can do about that.
>>>> */
>>>> if (rev > OLD_FLAT_VERSION) {
>>>> + unsigned long persistent = 0;
>>>> for (i=0; i < relocs; i++) {
>>>> unsigned long addr, relval;
>>>>
>>>> @@ -749,6 +750,8 @@ static int load_flat_file(struct linux_binprm * bprm,
>>>> relocated (of course, the address has to be
>>>> relocated first). */
>>>> relval = ntohl(reloc[i]);
>>>> + if (flat_set_persistent (relval, &persistent))
>>>> + continue;
>>>> addr = flat_get_relocate_addr(relval);
>>>> rp = (unsigned long *) calc_reloc(addr, libinfo, id, 1);
>>>> if (rp == (unsigned long *)RELOC_FAILED) {
>
> I don't much care for this API. It's shuffling around a temporary
> variable for the architecture code that's set for certain relocations
> that are otherwise unhandled.
>
> Since all the architecture is interested in is the relval that has
> associated "persistent" data encoded in it, why don't we just have a stub
> to give the architecture a chance to validate the relval before the
> flat_get_relocate_addr() and move this stuff there instead? ie, blackfin
> takes this out-of-line and manages its persistent value there.
What is flat_set_persistent other than a stub to validate the relval?
I'm not at all sure what you're proposing or how it would be different.
> load_flat_file() is ugly enough without dumping more architecture
> callback abuses in it.
The other maintainers who have spoken up didn't seem to think this was
ugly, or an abuse. I'm surprised to hear language like that when
discussing a patch that adds an if statement, a local variable and one
parameter in a function call.
Bernd
--
This footer brought to you by insane German lawmakers.
Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 40368
Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif
On Thu, Sep 20, 2007 at 02:04:26PM +0200, Bernd Schmidt wrote:
> Paul Mundt wrote:
> >I find it a bit disconcerting that blackfin already depends on this
> >in-tree without there being any earlier discussion on making these
> >changes.
>
> Parts of the initial submission were picked up (the include/asm
> directory), other's weren't. Little we can do about that.
>
What you can do about that is to order the patches, so one doesn't get
applied ahead of the other. People have successfully managed to submit
patches with dependencies in the past, you can look through the archives
for examples.
> >Since all the architecture is interested in is the relval that has
> >associated "persistent" data encoded in it, why don't we just have a stub
> >to give the architecture a chance to validate the relval before the
> >flat_get_relocate_addr() and move this stuff there instead? ie, blackfin
> >takes this out-of-line and manages its persistent value there.
>
> What is flat_set_persistent other than a stub to validate the relval?
> I'm not at all sure what you're proposing or how it would be different.
>
My apologies for not making it clearer. I did not get a chance to hack
together a patch today. Hopefully the below will clear up whatever
confusion there was.
> >load_flat_file() is ugly enough without dumping more architecture
> >callback abuses in it.
>
> The other maintainers who have spoken up didn't seem to think this was
> ugly, or an abuse. I'm surprised to hear language like that when
> discussing a patch that adds an if statement, a local variable and one
> parameter in a function call.
>
Yes, well, the scope of the changes is really rather irrelevant, it's the
technical basis behind it that should be the concern. My suggestion was
to lose the local variable, get rid of this "persistent" API, and plug in
something like flat_validate_relval() or something equally silly where
each architecture has the option to grab the relval and set up whatever
sort of state it wants to out-of-line.
This is making API changes where it's convenient for your platform to use
this value, and there's no reason to change the API here at all. The
if/continue block are not an issue, it is the whole concept of persistent
data in this case that has no need to be applied uniformally across all
other architectures.
Why should all architectures have to change their APIs (not just adding
new things mind you, also changing existing definitions) to accomodate
something that can trivially be kept in the blackfin code?
I wager the other maintainers either a) don't care because it doesn't
effect them, or b) have resigned binfmt_flat to its fate. Neither option
is particularly appealing in terms of getting that code tidied. That sort
of indifference is largely why binfmt_flat is as much of a mess as it is
today. Your patch is certainly not the cause of this, the architecture
callbacks there are just a mess to begin with, I would prefer that we try
to keep new additions flexible without blindly hardcoding more usage
assumptions all over the place and having to paper over them again later.
I can hack up some patches tomorrow if you're still unsure of what I'm
proposing.
Paul Mundt wrote:
> This is making API changes where it's convenient for your platform to use
> this value, and there's no reason to change the API here at all.
Your proposed addition of flat_validate_relval is an API change, and
very similar in nature to what I've done.
A local variable here is the most natural way to store this information.
What do you suggest we use, a global? A member in the task struct?
> Why should all architectures have to change their APIs (not just adding
> new things mind you, also changing existing definitions) to accomodate
> something that can trivially be kept in the blackfin code?
I don't see how there's a burden given that we've provided patches, and
most maintainers have already said their fine with it. It seems to me
that it's a natural and common thing for many architectures to be
updated when new things are added to core code.
Bernd
--
This footer brought to you by insane German lawmakers.
Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 40368
Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif
Jivin Paul Mundt lays it down ...
...
> > The other maintainers who have spoken up didn't seem to think this was
> > ugly, or an abuse. I'm surprised to hear language like that when
> > discussing a patch that adds an if statement, a local variable and one
> > parameter in a function call.
> >
> Yes, well, the scope of the changes is really rather irrelevant, it's the
> technical basis behind it that should be the concern. My suggestion was
> to lose the local variable, get rid of this "persistent" API, and plug in
> something like flat_validate_relval() or something equally silly where
> each architecture has the option to grab the relval and set up whatever
> sort of state it wants to out-of-line.
>
> This is making API changes where it's convenient for your platform to use
> this value, and there's no reason to change the API here at all. The
> if/continue block are not an issue, it is the whole concept of persistent
> data in this case that has no need to be applied uniformally across all
> other architectures.
>
> Why should all architectures have to change their APIs (not just adding
> new things mind you, also changing existing definitions) to accomodate
> something that can trivially be kept in the blackfin code?
Fair comment. I hadn't considered that it could be even cleaner.
If it's easily doable then I agree with your concerns.
> I wager the other maintainers either a) don't care because it doesn't
> effect them, or b) have resigned binfmt_flat to its fate. Neither option
> is particularly appealing in terms of getting that code tidied. That sort
> of indifference is largely why binfmt_flat is as much of a mess as it is
> today. Your patch is certainly not the cause of this, the architecture
> callbacks there are just a mess to begin with, I would prefer that we try
> to keep new additions flexible without blindly hardcoding more usage
> assumptions all over the place and having to paper over them again later.
I would say that (a) is definately not the case. I am sure the BF guys
will say they have been banging us on the head with changes for a long
time and getting no where as we considered the changes to severe or out
of line.
This particular patch was trivial in comparison to others I've seen,
it fixed all the existing arches (not something that is always done) and
seemed a reasonable start to finally get the BF guys up and running.
Still, happy to make it better of course ;-)
As for (b), I think it will be around for a while. It's not as flexible
as fdpic, but it's still a tiny, simple format and not everyone has an
fdpic implementation yet :-)
Cheers,
Davidm
--
David McCullough, [email protected], Ph:+61 734352815
Secure Computing - SnapGear http://www.uCdot.org http://www.cyberguard.com
On Thu 20 Sep 2007 11:03, David McCullough pondered:
> I would say that (a) is definately not the case. I am sure the BF guys
> will say they have been banging us on the head with changes for a long
> time and getting no where as we considered the changes to severe or out
> of line.
I don't think we have been "banging heads" with you (unless that is your
feeling?) - how about "working together, but diverting to satisfy different
needs" :)
I think that we have had more issues in the uClinux-dist (userspace and build
environment), but for kernel code, we have moved from some non-standard
(stupid) things we were doing early on to what we have today - which is as
common/standard with other archs as we can be.
Although this is slightly off topic - on the uClinux distribution side - most
of our changes are based on requirements/desires from being able to support
fdpic elf and flat formats, and to attempt to make things easier for end
users/us to use/maintain. Where we do make changes - we always send the patch
upstream and have the conversation with you (not everyone else does this),
and some/most times rework things so they are more acceptable to you. We
don't always come to an agreement - but we always have the discussion, and
are willing to move if we can make things better that still meets both our
needs/desires.
> This particular patch was trivial in comparison to others I've seen,
That is what we thought.
> it fixed all the existing arches (not something that is always done) and
> seemed a reasonable start to finally get the BF guys up and running.
> Still, happy to make it better of course ;-)
As always - we are more than happy to explore/review alternative patches if
people want to write/sumbit them.
-Robin
Jivin Robin Getz lays it down ...
> On Thu 20 Sep 2007 11:03, David McCullough pondered:
> > I would say that (a) is definately not the case. I am sure the BF guys
> > will say they have been banging us on the head with changes for a long
> > time and getting no where as we considered the changes to severe or out
> > of line.
>
> I don't think we have been "banging heads" with you (unless that is your
> feeling?) - how about "working together, but diverting to satisfy different
> needs" :)
No head banging feelings here, but I would understand if you guys felt
that way occasionally ;-) I obviously forgot the happy face on that
statement. It was meant as a good thing.
> I think that we have had more issues in the uClinux-dist (userspace and build
> environment), but for kernel code, we have moved from some non-standard
> (stupid) things we were doing early on to what we have today - which is as
> common/standard with other archs as we can be.
>
> Although this is slightly off topic - on the uClinux distribution side - most
> of our changes are based on requirements/desires from being able to support
> fdpic elf and flat formats, and to attempt to make things easier for end
> users/us to use/maintain. Where we do make changes - we always send the patch
> upstream and have the conversation with you (not everyone else does this),
> and some/most times rework things so they are more acceptable to you. We
> don't always come to an agreement - but we always have the discussion, and
> are willing to move if we can make things better that still meets both our
> needs/desires.
>
> > This particular patch was trivial in comparison to others I've seen,
>
> That is what we thought.
>
> > it fixed all the existing arches (not something that is always done) and
> > seemed a reasonable start to finally get the BF guys up and running.
> > Still, happy to make it better of course ;-)
>
> As always - we are more than happy to explore/review alternative patches if
> people want to write/sumbit them.
Cheers,
Davidm
--
David McCullough, [email protected], Ph:+61 734352815
Secure Computing - SnapGear http://www.uCdot.org http://www.cyberguard.com
Andrew, could you please add this patch to -mm.
As we discussed here, it should be OK for us.
Thanks,
- Bryan Wu
On Fri, 2007-09-21 at 11:32 +0800, David McCullough wrote:
>
> Jivin Robin Getz lays it down ...
> > On Thu 20 Sep 2007 11:03, David McCullough pondered:
> > > I would say that (a) is definately not the case. I am sure the BF
> guys
> > > will say they have been banging us on the head with changes for a
> long
> > > time and getting no where as we considered the changes to severe
> or out
> > > of line.
> >
> > I don't think we have been "banging heads" with you (unless that is
> your
> > feeling?) - how about "working together, but diverting to satisfy
> different
> > needs" :)
>
> No head banging feelings here, but I would understand if you guys
> felt
> that way occasionally ;-) I obviously forgot the happy face on that
> statement. It was meant as a good thing.
>
> > I think that we have had more issues in the uClinux-dist (userspace
> and build
> > environment), but for kernel code, we have moved from some
> non-standard
> > (stupid) things we were doing early on to what we have today - which
> is as
> > common/standard with other archs as we can be.
> >
> > Although this is slightly off topic - on the uClinux distribution
> side - most
> > of our changes are based on requirements/desires from being able to
> support
> > fdpic elf and flat formats, and to attempt to make things easier for
> end
> > users/us to use/maintain. Where we do make changes - we always send
> the patch
> > upstream and have the conversation with you (not everyone else does
> this),
> > and some/most times rework things so they are more acceptable to
> you. We
> > don't always come to an agreement - but we always have the
> discussion, and
> > are willing to move if we can make things better that still meets
> both our
> > needs/desires.
> >
> > > This particular patch was trivial in comparison to others I've
> seen,
> >
> > That is what we thought.
> >
> > > it fixed all the existing arches (not something that is always
> done) and
> > > seemed a reasonable start to finally get the BF guys up and
> running.
> > > Still, happy to make it better of course ;-)
> >
> > As always - we are more than happy to explore/review alternative
> patches if
> > people want to write/sumbit them.
>
> Cheers,
> Davidm
>
> --
> David McCullough, [email protected], Ph:+61
> 734352815
> Secure Computing - SnapGear http://www.uCdot.org
> http://www.cyberguard.com
>
On Tue, 18 Sep 2007 16:09:25 +0800
Bryan Wu <[email protected]> wrote:
> From: Bernd Schmidt <[email protected]>
>
> This just adds minimum support for the Blackfin relocations,
> since we don't have enough space in each reloc. The idea
> is to store a value with one relocation so that subsequent ones can
> access it.
>
> Actually, this patch is required for Blackfin. Currently if BINFMT_FLAT
> is enabled, git-tree kernel will fail to compile. Please consider to
> accept it.
>
A few things
> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
> index 7b0265d..13b58f8 100644
> --- a/fs/binfmt_flat.c
> +++ b/fs/binfmt_flat.c
> @@ -742,6 +742,7 @@ static int load_flat_file(struct linux_binprm * bprm,
> * __start to address 4 so that is okay).
> */
> if (rev > OLD_FLAT_VERSION) {
> + unsigned long persistent = 0;
`persistent' here only has meaning inside the next nesting level, so should
be moved down into that scope for readability reasons.
Also, the initialisation to zero is, afaict, unneeded and wastes
instructions. I suspect that's just there to suppress a gcc warning, which
is better done with uninitialized_var().
> for (i=0; i < relocs; i++) {
> unsigned long addr, relval;
>
> @@ -749,6 +750,8 @@ static int load_flat_file(struct linux_binprm * bprm,
> relocated (of course, the address has to be
> relocated first). */
> relval = ntohl(reloc[i]);
> + if (flat_set_persistent (relval, &persistent))
> + continue;
If this correct? flat_set_persistent() returns zero if it didn't write
anything to `persistent'. It seems strange that in the case where
flat_set_persistent() _does_ write something to `persistent', we just throw
it away by doing `continue'.
Either that, or I've misread the code and you really did mean to put
`persistent' in the outer scope, and its value is supposed to propagate
over into the next iteration of the loop. If so, that's all a bit too
tricky for it to be implemented with zero code comments, dontcha think?
Also, given that you are proposing that flat_set_persistent() becomes part
of the kernel's core<->arch interface (for all architectures which want to
implement binfmt_flat()), it is no longer appropriate that the reference
implementation of flat_set_persistent() (ie: blackfins's implementation) be
completely undocumented. IMO.
> --- a/include/asm-h8300/flat.h
> +++ b/include/asm-h8300/flat.h
> @@ -9,6 +9,7 @@
> #define flat_argvp_envp_on_stack() 1
> #define flat_old_ram_flag(flags) 1
> #define flat_reloc_valid(reloc, size) ((reloc) <= (size))
> +#define flat_set_persistent(relval, p) 0
ug. those macros are crap. who did that. They will generate compiler
warnings and runtime failures in a whole host of well-known scenarios.
My kingdom to the person who invents a keyboard which delivers 12 kV when
it detects the sequence # d e f i n e. There is no reason why these
"functions" need to be implemented as crappy #defines and converting them
to C will fix bug, warnings and will clean stuff up.
Sigh.
Andrew Morton wrote:
>> if (rev > OLD_FLAT_VERSION) {
>> + unsigned long persistent = 0;
>
> `persistent' here only has meaning inside the next nesting level, so should
> be moved down into that scope for readability reasons.
See below.
>> + if (flat_set_persistent (relval, &persistent))
>> + continue;
>
> If this correct? flat_set_persistent() returns zero if it didn't write
> anything to `persistent'. It seems strange that in the case where
> flat_set_persistent() _does_ write something to `persistent', we just throw
> it away by doing `continue'.
>
> Either that, or I've misread the code and you really did mean to put
> `persistent' in the outer scope, and its value is supposed to propagate
> over into the next iteration of the loop. If so, that's all a bit too
> tricky for it to be implemented with zero code comments, dontcha think?
The latter. We need to be able to use more data than we can fit into a
single reloc, so we store a value with one reloc and reuse it with the
next. There'd be no point in having this function otherwise since you
could perform whatever needs to be done in flat_get_relocate_addr.
This seemed fairly obvious at the time... when you're familiar with the
flat format, the loop isn't all that hard to understand. I'll add
comments in the next version.
Bernd
--
This footer brought to you by insane German lawmakers.
Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 40368
Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif