[PATCH] link time error in drivers/mtd (240t13p2)

[PATCH] link time error in drivers/mtd (240t13p2)

[Rasmus Andersen]

Hi.

Various files in drivers/mtd references cfi_probe (by way of do_cfi_probe).
This function is static and thus not shared. The following patch removes
the static declaration but if it is What Was Intended I do not know. It
makes the kernel link, however.


--- linux-240-t13-pre2-clean/drivers/mtd/cfi_probe.c	Wed Nov 22 22:41:39 2000
+++ linux/drivers/mtd/cfi_probe.c	Sat Dec 16 22:58:57 2000
@@ -17,7 +17,7 @@
 #include <linux/mtd/cfi.h>
 
 
-static struct mtd_info *cfi_probe(struct map_info *);
+struct mtd_info *cfi_probe(struct map_info *);
 
 static void print_cfi_ident(struct cfi_ident *);
 static void check_cmd_set(struct map_info *, int, unsigned long);
@@ -32,7 +32,7 @@
  * this module is non-zero, i.e. between inter_module_get and
  * inter_module_put.  Keith Owens <kaos@ocs.com.au> 29 Oct 2000.
  */
-static struct mtd_info *cfi_probe(struct map_info *map)
+struct mtd_info *cfi_probe(struct map_info *map)
 {
 	struct mtd_info *mtd = NULL;
 	struct cfi_private *cfi;

-- 
        Rasmus(rasmus@jaquet.dk)

The Holocaust was an obscene period in our nation's history. I mean
in this century's history. But we all lived in this century. I didn't
live in this century.
                -- Senator Dan Quayle, 9/15/88
                   (reported in Esquire, 8/92, The New Yorker, 10/10/88, p.102)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH] link time error in drivers/mtd (240t13p2)

[Keith Owens]

On Sat, 16 Dec 2000 23:07:01 +0100, 
Rasmus Andersen <rasmus@jaquet.dk> wrote:
>Various files in drivers/mtd references cfi_probe (by way of do_cfi_probe).
>This function is static and thus not shared. The following patch removes
>the static declaration but if it is What Was Intended I do not know. It
>makes the kernel link, however.

Somebody changed include/linux/mtd/map.h between 2.4.0-test11 and
test12.  That change is wrong, it adds conditional complexity where it
is not required - inter_module_xxx works even without CONFIG_MODULES.
cfi_probe should still be static.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH] link time error in drivers/mtd (240t13p2)

[David Woodhouse]

On Sun, 17 Dec 2000, Keith Owens wrote:

> Somebody changed include/linux/mtd/map.h between 2.4.0-test11 and
> test12.  That change is wrong, it adds conditional complexity where it
> is not required - inter_module_xxx works even without CONFIG_MODULES.
> cfi_probe should still be static.

No. Think about the link order. inter_module_xxx doesn't work reliably.
get_module_symbol() did.

-- 
dwmw2


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH] link time error in drivers/mtd (240t13p2)

[Keith Owens]

On Sun, 17 Dec 2000 10:01:07 +0000 (GMT), 
David Woodhouse <dwmw2@infradead.org> wrote:
>On Sun, 17 Dec 2000, Keith Owens wrote:
>
>> Somebody changed include/linux/mtd/map.h between 2.4.0-test11 and
>> test12.  That change is wrong, it adds conditional complexity where it
>> is not required - inter_module_xxx works even without CONFIG_MODULES.
>> cfi_probe should still be static.
>
>No. Think about the link order. inter_module_xxx doesn't work reliably.
>get_module_symbol() did.

Messing about with conditional compilation because the link order is
incorrect is the wrong fix.  The mtd/Makefile must link the objects in
the correct order.

cfi_probe.o needs to come after cfi_cmdset_000?.o.
doc_probe.o needs to come after doc200?.o.
nora.o, octagon-5066.o, physmap.o, rpxlite.o, vmax301.o, pnc2000.o need
to come after cfi_probe.o.
octagon-5066.o, vmax301.o need to come after jedec.o.
octagon-5066.o, vmax301.o need to come after map_ram.o.
octagon-5066.o, vmax301.o need to come after map_rom.o.

2.4.0-test13-pre2 almost does that, the only obvious problem is that
cfi_probe appears before cfi_cmdset.  Move cfi_probe to link after
cfi_cmdset, do you still get link order problems with the 2.4.0-test11
version of include/linux/mtd.h?

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH] link time error in drivers/mtd (240t13p2)

[David Woodhouse]

On Sun, 17 Dec 2000, Keith Owens wrote:

> Messing about with conditional compilation because the link order is
> incorrect is the wrong fix.  The mtd/Makefile must link the objects in
> the correct order.

The conditional compilation is far more obvious to people than subtle
issues with link order. So I prefer to avoid the latter at all costs.

I have to have some conditional compilation in my tree to allow it to
compile under 2.0 uClinux. Admittedly that doesn't have to get into 2.4,
but I obviously prefer the code in 2.4 to be as close to my working copy
as possible.

I'll poke at it and try to come up with a cleaner solution. It may be that
I can shift all the conditional stuff off into the compatmac.h and leave
the 'real' code path in a cleaner state than the current one.

> 2.4.0-test13-pre2 almost does that, the only obvious problem is that
> cfi_probe appears before cfi_cmdset.  Move cfi_probe to link after
> cfi_cmdset, do you still get link order problems with the 2.4.0-test11
> version of include/linux/mtd.h?

I haven't had problems. But the possibility exists.

-- 
dwmw2


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH] link time error in drivers/mtd (240t13p2)

[Keith Owens]

On Sun, 17 Dec 2000 10:44:09 +0000 (GMT), 
David Woodhouse <dwmw2@infradead.org> wrote:
>The conditional compilation is far more obvious to people than subtle
>issues with link order. So I prefer to avoid the latter at all costs.

The rest of the kernel already depends totally on these "subtle" issues
with link order.  Why should mtd be different?

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH] link time error in drivers/mtd (240t13p2)

[David Woodhouse]

On Sun, 17 Dec 2000, Keith Owens wrote:

> The rest of the kernel already depends totally on these "subtle" issues
> with link order.  Why should mtd be different?

Because I maintain the MTD code and I want it to be. I think the link
order dependencies are ugly, unnecessary and far more likely to be
problematic then the alternatives. I'll code an alternative which is
cleaner than the current code, and Linus can either accept it or not, as
he sees fit.

-- 
dwmw2


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH] link time error in drivers/mtd (240t13p2)

[Keith Owens]

On Sun, 17 Dec 2000 11:39:50 +0000 (GMT), 
David Woodhouse <dwmw2@infradead.org> wrote:
>On Sun, 17 Dec 2000, Keith Owens wrote:
>
>> The rest of the kernel already depends totally on these "subtle" issues
>> with link order.  Why should mtd be different?
>
>Because I maintain the MTD code and I want it to be. I think the link
>order dependencies are ugly, unnecessary and far more likely to be
>problematic then the alternatives. I'll code an alternative which is
>cleaner than the current code, and Linus can either accept it or not, as
>he sees fit.

Your choice.  Just bear in mind that if CONFIG_MODULES=y but mtd
objects are built into the kernel then mtd _must_ have a correct link
order.  Consider a config with CONFIG_MODULES=y but every mtd option is
set to 'y', link order is critical.  The moment you have two or more
mtd modules built in then you are stuck with link order issues, no
matter what you do.  Of course you could invent and maintain your own
unique method for controlling mtd initialisation order ...

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH] link time error in drivers/mtd (240t13p2)

[David Woodhouse]

On Sun, 17 Dec 2000, Keith Owens wrote:

> Your choice.  Just bear in mind that if CONFIG_MODULES=y but mtd
> objects are built into the kernel then mtd _must_ have a correct link
> order.  Consider a config with CONFIG_MODULES=y but every mtd option is
> set to 'y', link order is critical.

Yep, I'd just noticed that one. The patch was actually put in by someone
to fix 2.0 compilation - and I noticed that it made the link order
problem go away for certain configs.

> Of course you could invent and maintain your own unique method for
> controlling mtd initialisation order ...

I'll try to find a clean way to make the MTD code work in all
configurations without having to do that. If it really comes to doing the
above, I'll probably just give up and let it stay 'broken' (IMO) along
with the rest of the kernel code which as you say already has link order
dependencies.

-- 
dwmw2


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH] link time error in drivers/mtd (240t13p2)

[Alan Cox]

> David Woodhouse <dwmw2@infradead.org> wrote:
> >The conditional compilation is far more obvious to people than subtle
> >issues with link order. So I prefer to avoid the latter at all costs.
> 
> The rest of the kernel already depends totally on these "subtle" issues
> with link order.  Why should mtd be different?

Why should mtd be broken just because the rest of the Makefiles are

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH] link time error in drivers/mtd (240t13p2)

[Horst von Brand]

Keith Owens <kaos@ocs.com.au> said:

[...]

> Messing about with conditional compilation because the link order is
> incorrect is the wrong fix.  The mtd/Makefile must link the objects in
> the correct order.
> 
> cfi_probe.o needs to come after cfi_cmdset_000?.o.
> doc_probe.o needs to come after doc200?.o.
> nora.o, octagon-5066.o, physmap.o, rpxlite.o, vmax301.o, pnc2000.o need
> to come after cfi_probe.o.
> octagon-5066.o, vmax301.o need to come after jedec.o.
> octagon-5066.o, vmax301.o need to come after map_ram.o.
> octagon-5066.o, vmax301.o need to come after map_rom.o.

Would tsort(1) perhaps help?
-- 
Horst von Brand                             vonbrand@sleipnir.valparaiso.cl
Casilla 9G, Vin~a del Mar, Chile                               +56 32 672616
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH] link time error in drivers/mtd (240t13p2)

[Peter Samuelson]

[Horst von Brand]
> Would tsort(1) perhaps help?

I'm betting Linus would never go for using tsort to resolve such issues
-- unless tsort output is guaranteed to be stable (the docs for GNU
textutils don't say).  This would be for the same reason that he
rejected the partial ordering in the LINK_FIRST patch -- because it was
only partial ordering and he thinks total ordering is necessary.  For
me, BTW, that's still an article of faith -- I still do not see why
total ordering *is* necessary, but <shrug> thus saith the penguin.

Peter
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH] link time error in drivers/mtd (240t13p2)

[David Woodhouse]

On Sat, 16 Dec 2000, Rasmus Andersen wrote:

> Various files in drivers/mtd references cfi_probe (by way of do_cfi_probe).
> This function is static and thus not shared. The following patch removes
> the static declaration but if it is What Was Intended I do not know. It
> makes the kernel link, however.

It is intended, thanks. Not only does it make the inter_module_xxx case
work reliably, but it also allows you to compile the code at all under
2.0 uCLinux. The reason it was omitted from test12 is because there are a
handful of other changes to the CFI code which I haven't yet tested as
thoroughly as I want to. If you're using CFI flash, please could you test
the latest version from my CVS tree and let me know the results?


-- 
dwmw2


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/