binfmt_flat: minimum support for the Blackfin relocations
Subsystems (2)
PATCH 1/1 binfmt_flat: minimum support for the Blackfin relocations
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.
--- 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(-)
fs/binfmt_flat.c +4 -1
@@ -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
include/asm-h8300/flat.h +2 -1
@@ -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)
include/asm-m32r/flat.h +2 -1
@@ -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) \
include/asm-m68knommu/flat.h +2 -1
@@ -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__ */
include/asm-sh/flat.h +2 -1
@@ -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 */
include/asm-v850/flat.h +3 -1
@@ -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;
Signature · 1 line
1.5.2
Quoted 11 lines · Bryan Wu
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_FLATis 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
Quoted 131 lines
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(-)fs/binfmt_flat.c +4
-1@@ -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 include/asm-h8300/flat.h +2
-1@@ -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)include/asm-m32r/flat.h +2
-1@@ -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) \include/asm-m68knommu/flat.h +2
-1@@ -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__ */ include/asm-sh/flat.h +2
-1@@ -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 */ include/asm-v850/flat.h +3
-1@@ -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;
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
In reply to Bryan WuOn Tue 18 Sep 2007 04:09, Bryan Wu pondered:
Quoted 11 lines
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.
Quoted 127 lines
--- 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(-)fs/binfmt_flat.c +4
-1@@ -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 include/asm-h8300/flat.h +2
-1@@ -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)include/asm-m32r/flat.h +2
-1@@ -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) \include/asm-m68knommu/flat.h +2
-1@@ -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__ */ include/asm-sh/flat.h +2
-1@@ -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 */ include/asm-v850/flat.h +3
-1@@ -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;
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
Jivin Robin Getz lays it down ...
Quoted 8 lines
On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
Quoted 7 lines
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.
Quoted 6 lines
Quoted 4 lines
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
Quoted 127 lines
Quoted 127 lines
--- 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(-)fs/binfmt_flat.c +4
-1@@ -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 include/asm-h8300/flat.h +2
-1@@ -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)include/asm-m32r/flat.h +2
-1@@ -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) \include/asm-m68knommu/flat.h +2
-1@@ -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__ */ include/asm-sh/flat.h +2
-1@@ -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 */ include/asm-v850/flat.h +3
-1@@ -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;
Signature · 2 lines
David McCullough, [email protected], Ph:+61 734352815 Secure Computing - SnapGear http://www.uCdot.org http://www.cyberguard.com
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
On Wed 19 Sep 2007 21:55, David McCullough pondered:
Quoted 14 lines
Jivin Robin Getz lays it down ...
Quoted 13 lines
On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
Quoted 11 lines
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]>
Quoted 4 lines
Quoted 1 line
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
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
In reply to David McCulloughQuoted 7 lines · David McCullough
Jivin Robin Getz lays it down ...
Quoted 6 lines
On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
Quoted 4 lines
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.
Quoted 5 lines
Quoted 1 line
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.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
fs/binfmt_flat.c +1
-0*/ if (rev >OLD_FLAT_VERSION ) { +unsigned long persistent =0 ;for (i=0 ; i < relocs; i++) {unsigned long addr, relval;
Quoted 9 lines
Quoted 9 lines
fs/binfmt_flat.c static int load_flat_file(struct linux_binprm * bprm, · line 757
Quoted 9 lines
fs/binfmt_flat.c +2
-0@@ -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.
Quoted 6 lines
Quoted 6 lines
fs/binfmt_flat.c static int load_flat_file(struct linux_binprm * bprm, · line 763
Quoted 6 lines
fs/binfmt_flat.c +1
-1@@ -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.
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
Quoted 8 lines · Paul Mundt
Quoted 7 lines · David McCullough
Jivin Robin Getz lays it down ...
Quoted 6 lines
On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
Quoted 4 lines
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.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
Quoted 18 lines · Mike Frysinger
Quoted 8 lines · Paul Mundt
Quoted 7 lines · David McCullough
Jivin Robin Getz lays it down ...
Quoted 6 lines
On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
Quoted 4 lines
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.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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.
Quoted 6 lines
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.
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
On Wed 19 Sep 2007 23:54, Paul Mundt pondered:
Quoted 26 lines · Mike Frysinger
Quoted 19 lines · Paul Mundt
Quoted 11 lines · David McCullough
Quoted 10 lines
Jivin Robin Getz lays it down ...
Quoted 8 lines
On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
Quoted 4 lines
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 6 lines
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
Quoted 3 lines
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?
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
Quoted 31 lines · Robin Getz
On Wed 19 Sep 2007 23:54, Paul Mundt pondered:
Quoted 26 lines · Mike Frysinger
Quoted 19 lines · Paul Mundt
Quoted 11 lines · David McCullough
Quoted 10 lines
Jivin Robin Getz lays it down ...
Quoted 8 lines
On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
Quoted 4 lines
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 6 lines
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
Quoted 3 lines
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.
Quoted 7 lines
"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 thearch/blackfininclusion (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.
Quoted 11 lines
Our mistake for not poking everyone/resending things sooner/before
arch/blackfinwas 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
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
Quoted 45 lines · Bryan Wu
Quoted 31 lines · Robin Getz
On Wed 19 Sep 2007 23:54, Paul Mundt pondered:
Quoted 26 lines · Mike Frysinger
Quoted 19 lines · Paul Mundt
Quoted 11 lines · David McCullough
Quoted 10 lines
Jivin Robin Getz lays it down ...
Quoted 8 lines
On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
Quoted 4 lines
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 6 lines
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
Quoted 3 lines
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.
Quoted 7 lines
"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 thearch/blackfininclusion (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
Quoted 5 lines
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_flatand other architectures. And the in-tree Blackfin port will fail to be compiled with this patch if theBINFMT_FLATis enabled.
Oops, typo. Correct one: it will fail to be compiled without this patch.
Quoted 29 lines
Quoted 11 lines
Our mistake for not poking everyone/resending things sooner/before
arch/blackfinwas 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 invitebinfmt_flatmaintainers, 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
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
In reply to Robin GetzQuoted 2 lines · Robin Getz
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?
-Miles
Signature · 1 line
Everywhere is walking distance if you have the time. -- Steven Wright
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
In reply to Paul MundtQuoted 3 lines · Paul Mundt
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.
Quoted 20 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
fs/binfmt_flat.c +1
-0*/ if (rev >OLD_FLAT_VERSION ) { +unsigned long persistent =0 ;for (i=0 ; i < relocs; i++) {unsigned long addr, relval;Quoted 9 lines
Quoted 9 lines
fs/binfmt_flat.c static int load_flat_file(struct linux_binprm * bprm, · line 757
Quoted 9 lines
fs/binfmt_flat.c +2
-0@@ -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.
Quoted 5 lines
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.
Quoted 2 lines
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
Signature · 4 lines
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
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
Quoted 8 lines · Bernd Schmidt
Quoted 3 lines · Paul Mundt
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/asmdirectory), 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.
Quoted 9 lines
Quoted 5 lines
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_persistentother 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.
Quoted 8 lines
Quoted 2 lines
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.
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
Quoted 2 lines · Paul Mundt
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?
Quoted 3 lines
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
Signature · 4 lines
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
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
In reply to Paul MundtJivin Paul Mundt lays it down ...
...
Quoted 18 lines
Quoted 5 lines
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.
Quoted 3 lines
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.
Quoted 8 lines
I wager the other maintainers either a) don't care because it doesn't effect them, or b) have resigned
binfmt_flatto its fate. Neither option is particularly appealing in terms of getting that code tidied. That sort of indifference is largely whybinfmt_flatis 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
Signature · 2 lines
David McCullough, [email protected], Ph:+61 734352815 Secure Computing - SnapGear http://www.uCdot.org http://www.cyberguard.com
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
On Thu 20 Sep 2007 11:03, David McCullough pondered:
Quoted 4 lines
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.
Quoted 1 line
This particular patch was trivial in comparison to others I've seen,
That is what we thought.
Quoted 3 lines
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
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
Jivin Robin Getz lays it down ...
Quoted 9 lines
On Thu 20 Sep 2007 11:03, David McCullough pondered:
Quoted 4 lines
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.
Quoted 17 lines
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.
Quoted 1 line
This particular patch was trivial in comparison to others I've seen,
Quoted 8 lines
That is what we thought.
Quoted 3 lines
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
Signature · 2 lines
David McCullough, [email protected], Ph:+61 734352815 Secure Computing - SnapGear http://www.uCdot.org http://www.cyberguard.com
Re: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
Andrew, could you please add this patch to -mm.
As we discussed here, it should be OK for us.
Thanks,
- Bryan Wu
Quoted 72 lines · David McCullough
Jivin Robin Getz lays it down ...
Quoted 2 lines
On Thu 20 Sep 2007 11:03, David McCullough pondered:
Quoted 1 line
I would say that (a) is definately not the case. I am sure the BF
guys
Quoted 1 line
Quoted 1 line
will say they have been banging us on the head with changes for a
long
Quoted 1 line
Quoted 1 line
time and getting no where as we considered the changes to severe
or out
Quoted 3 lines
Quoted 1 line
of line.
I don't think we have been "banging heads" with you (unless that is
your
Quoted 1 line
feeling?) - how about "working together, but diverting to satisfy
different
Quoted 1 line
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.Quoted 1 line
I think that we have had more issues in the uClinux-dist (userspace
and build
Quoted 1 line
environment), but for kernel code, we have moved from some
non-standard
Quoted 1 line
(stupid) things we were doing early on to what we have today - which
is as
Quoted 3 lines
common/standard with other archs as we can be.
Although this is slightly off topic - on the uClinux distribution
side - most
Quoted 1 line
of our changes are based on requirements/desires from being able to
support
Quoted 1 line
fdpic elf and flat formats, and to attempt to make things easier for
end
Quoted 1 line
users/us to use/maintain. Where we do make changes - we always send
the patch
Quoted 1 line
upstream and have the conversation with you (not everyone else does
this),
Quoted 1 line
and some/most times rework things so they are more acceptable to
you. We
Quoted 1 line
don't always come to an agreement - but we always have the
discussion, and
Quoted 1 line
are willing to move if we can make things better that still meets
both our
Quoted 3 lines
needs/desires.
Quoted 1 line
This particular patch was trivial in comparison to others I've
seen,
Quoted 4 lines
That is what we thought.
Quoted 1 line
it fixed all the existing arches (not something that is always
done) and
Quoted 1 line
Quoted 1 line
seemed a reasonable start to finally get the BF guys up and
running.
Quoted 3 lines
Quoted 1 line
Still, happy to make it better of course ;-)
As always - we are more than happy to explore/review alternative
patches if
Quoted 1 line
people want to write/sumbit them.
Cheers,
DavidmSignature · 4 lines
David McCullough, [email protected], Ph:+61 734352815 Secure Computing - SnapGear http://www.uCdot.org http://www.cyberguard.com
Hirokazu Takata
Acked-byRe: [PATCH] binfmt_flat: minimum support for theBlackfin relocations
In reply to David McCulloughFrom: David McCullough <[email protected]>
Date: Thu, 20 Sep 2007 11:55:25 +1000
Quoted 15 lines
Jivin Robin Getz lays it down ...
Quoted 13 lines
On Tue 18 Sep 2007 04:09, Bryan Wu pondered:
Quoted 11 lines
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]>
Quoted 7 lines
Quoted 1 line
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.
--
Hirokazu Takata <[email protected]>
Linux/M32R Project: http://www.linux-m32r.org/
Quoted 11 lines · 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_FLATis enabled, git-tree kernel will fail to compile. Please consider to accept it.
A few things
fs/binfmt_flat.c static int load_flat_file(struct linux_binprm * bprm, · line 745
Quoted 9 lines
fs/binfmt_flat.c +1
-0@@ -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().
fs/binfmt_flat.c static int load_flat_file(struct linux_binprm * bprm, · line 754
Quoted 9 lines
fs/binfmt_flat.c +2
-0for (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.
include/asm-h8300/flat.h line 12
Quoted 7 lines
include/asm-h8300/flat.h +1
-0@@ -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.
Quoted 5 lines · Andrew Morton
fs/binfmt_flat.c static int load_flat_file(struct linux_binprm * bprm, · line 745
Quoted 2 lines
fs/binfmt_flat.c +1
-0if (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.
Quoted 8 lines
fs/binfmt_flat.c static int load_flat_file(struct linux_binprm * bprm, · line 754
Quoted 2 lines
fs/binfmt_flat.c +2
-0+if (flat_set_persistent (relval, &persistent)) +continue ;If this correct?
flat_set_persistent()returns zero if it didn't write anything topersistent'. It seems strange that in the case where flat_set_persistent() _does_ write something topersistent', we just throw it away by doing `continue'.
Quoted 4 lines
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
Signature · 4 lines
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