From: Bernd Schmidt <bernd.schmidt@analog.com>
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 <bernd.schmidt@analog.com>

Signed-off-by: Bryan Wu <bryan.wu@analog.com>

Cc: David McCullough <davidm@snapgear.com>

Cc: Greg Ungerer <gerg@snapgear.com>

---

 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 relo...
[ message continues ]

On Tue, 18 Sep 2007 16:09:25 +0800
`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
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
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.
-

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

-

-

Looks fine to me,  obviously impacts the existing arches very little.

Can't see why it shouldn't get included,
Cheers,
--

David McCullough,  david_mccullough@securecomputing.com,   Ph:+61 734352815

Secure Computing - SnapGear  http://www.uCdot.org http://www.cyberguard.com

-

From: David McCullough <David_Mccullough@securecomputing.com>
Looks fine to me, too.
Acked-by: Hirokazu Takata <takata@linux-m32r.org>
--

Hirokazu Takata <takata@linux-m32r.org>

Linux/M32R Project:  http://www.linux-m32r.org/
-

I find it a bit disconcerting that blackfin already depends on this

in-tree without there being any earlier discussion on making these
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

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.

-

Parts of the initial submission were picked up (the include/asm 
What is flat_set_persistent other than a stub to validate the relval? 
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

-

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

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

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.

-

Jivin Paul Mundt lays it down ...
Fair comment.  I hadn't considered that it could be even cleaner.
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,  david_mccullough@securecomputing.com,   Ph:+61 734352815

Secure Computing - SnapGear  http://www.uCdot.org http://www.cyberguard.com

-

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 
As always - we are more than happy to explore/review alternative patches if

people want to write/sumbit them.
-Robin

-

No head banging feelings here,  but I would understand if you guys felt

that way occasionally ;-)  I obviously forgot the happy face on that
Cheers,

Davidm
--

David McCullough,  david_mccullough@securecomputing.com,   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

-

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. 
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

-

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

-

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.

-

"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?

-

miles.bader@necel.com
-Miles
--

Everywhere is walking distance if you have the time.  -- Steven Wright

-

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
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

-

Oops, typo. Correct one:  it will fail to be compiled without this

-

Is that Australian for Acked-by:? :)
-Robin

-

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

-