2.4.8-pre1 build error in drivers/parport/parport_pc.c

2.4.8-pre1 build error in drivers/parport/parport_pc.c

[Steven Cole]

I got the following errors building 2.4.8-pre1.

Snippet from .config:

CONFIG_PARPORT=y
CONFIG_PARPORT_PC=y
CONFIG_PARPORT_PC_CML1=y

Steven

parport_pc.c:257: redefinition of `parport_pc_write_data'
/usr/src/linux-2.4.8-pre1/include/linux/parport_pc.h:45: `parport_pc_write_data' previously defined here
parport_pc.c:262: redefinition of `parport_pc_read_data'
/usr/src/linux-2.4.8-pre1/include/linux/parport_pc.h:53: `parport_pc_read_data' previously defined here
parport_pc.c:267: redefinition of `parport_pc_write_control'
/usr/src/linux-2.4.8-pre1/include/linux/parport_pc.h:139: `parport_pc_write_control' previously defined here
parport_pc.c:284: redefinition of `parport_pc_read_control'
/usr/src/linux-2.4.8-pre1/include/linux/parport_pc.h:156: `parport_pc_read_control' previously defined here
parport_pc.c:295: redefinition of `parport_pc_frob_control'
/usr/src/linux-2.4.8-pre1/include/linux/parport_pc.h:168: `parport_pc_frob_control' previously defined here
parport_pc.c:320: redefinition of `parport_pc_read_status'
/usr/src/linux-2.4.8-pre1/include/linux/parport_pc.h:193: `parport_pc_read_status' previously defined here
parport_pc.c:325: redefinition of `parport_pc_disable_irq'
/usr/src/linux-2.4.8-pre1/include/linux/parport_pc.h:199: `parport_pc_disable_irq' previously defined here
parport_pc.c:330: redefinition of `parport_pc_enable_irq'
/usr/src/linux-2.4.8-pre1/include/linux/parport_pc.h:204: `parport_pc_enable_irq' previously defined here
parport_pc.c:335: redefinition of `parport_pc_data_forward'
/usr/src/linux-2.4.8-pre1/include/linux/parport_pc.h:133: `parport_pc_data_forward' previously defined here
parport_pc.c:340: redefinition of `parport_pc_data_reverse'
/usr/src/linux-2.4.8-pre1/include/linux/parport_pc.h:128: `parport_pc_data_reverse' previously defined here
make[3]: *** [parport_pc.o] Error 1
make[3]: Leaving directory `/usr/src/linux-2.4.8-pre1/drivers/parport'

[PATCH] Re: 2.4.8-pre1 build error in drivers/parport/parport_pc.c

[Robert Schiele]

[-- Attachment #1: Type: text/plain, Size: 4001 bytes --]

On Thu, Jul 26, 2001 at 07:53:11PM -0600, Steven Cole wrote:
> I got the following errors building 2.4.8-pre1.
> 
> parport_pc.c:257: redefinition of `parport_pc_write_data'
> /usr/src/linux-2.4.8-pre1/include/linux/parport_pc.h:45: `parport_pc_write_data' previously defined here
> parport_pc.c:262: redefinition of `parport_pc_read_data'
> /usr/src/linux-2.4.8-pre1/include/linux/parport_pc.h:53: `parport_pc_read_data' previously defined here
> ...
> make[3]: *** [parport_pc.o] Error 1
> make[3]: Leaving directory `/usr/src/linux-2.4.8-pre1/drivers/parport'

Hmm, these functions are multiply defined, namely in the c source and
in it's header file. I see no reason why someone should do this. The
problem was hidden in older kernel releases by the fact that these
functions were declared "extern __inline__" which is absolutely
nonsense in my opinion. So the solution should be to just remove these
inline functions from the c source file, which can be done with the
following simple and stupid patch.

This should be the correct solution, or did I miss the vital point?

Robert

diff -u --recursive --new-file v2.4.7/linux/drivers/parport/parport_pc.c linux/drivers/parport/parport_pc.c
--- v2.4.7/linux/drivers/parport/parport_pc.c	Wed Jul 11 01:07:46 2001
+++ linux/drivers/parport/parport_pc.c	Fri Jul 27 09:24:50 2001
@@ -253,94 +253,6 @@
 	parport_generic_irq(irq, (struct parport *) dev_id, regs);
 }
 
-void parport_pc_write_data(struct parport *p, unsigned char d)
-{
-	outb (d, DATA (p));
-}
-
-unsigned char parport_pc_read_data(struct parport *p)
-{
-	return inb (DATA (p));
-}
-
-void parport_pc_write_control(struct parport *p, unsigned char d)
-{
-	const unsigned char wm = (PARPORT_CONTROL_STROBE |
-				  PARPORT_CONTROL_AUTOFD |
-				  PARPORT_CONTROL_INIT |
-				  PARPORT_CONTROL_SELECT);
-
-	/* Take this out when drivers have adapted to the newer interface. */
-	if (d & 0x20) {
-		printk (KERN_DEBUG "%s (%s): use data_reverse for this!\n",
-			p->name, p->cad->name);
-		parport_pc_data_reverse (p);
-	}
-
-	__parport_pc_frob_control (p, wm, d & wm);
-}
-
-unsigned char parport_pc_read_control(struct parport *p)
-{
-	const unsigned char wm = (PARPORT_CONTROL_STROBE |
-				  PARPORT_CONTROL_AUTOFD |
-				  PARPORT_CONTROL_INIT |
-				  PARPORT_CONTROL_SELECT);
-	const struct parport_pc_private *priv = p->physport->private_data;
-	return priv->ctr & wm; /* Use soft copy */
-}
-
-unsigned char parport_pc_frob_control (struct parport *p, unsigned char mask,
-				       unsigned char val)
-{
-	const unsigned char wm = (PARPORT_CONTROL_STROBE |
-				  PARPORT_CONTROL_AUTOFD |
-				  PARPORT_CONTROL_INIT |
-				  PARPORT_CONTROL_SELECT);
-
-	/* Take this out when drivers have adapted to the newer interface. */
-	if (mask & 0x20) {
-		printk (KERN_DEBUG "%s (%s): use data_%s for this!\n",
-			p->name, p->cad->name,
-			(val & 0x20) ? "reverse" : "forward");
-		if (val & 0x20)
-			parport_pc_data_reverse (p);
-		else
-			parport_pc_data_forward (p);
-	}
-
-	/* Restrict mask and val to control lines. */
-	mask &= wm;
-	val &= wm;
-
-	return __parport_pc_frob_control (p, mask, val);
-}
-
-unsigned char parport_pc_read_status(struct parport *p)
-{
-	return inb (STATUS (p));
-}
-
-void parport_pc_disable_irq(struct parport *p)
-{
-	__parport_pc_frob_control (p, 0x10, 0);
-}
-
-void parport_pc_enable_irq(struct parport *p)
-{
-	__parport_pc_frob_control (p, 0x10, 0x10);
-}
-
-void parport_pc_data_forward (struct parport *p)
-{
-	__parport_pc_frob_control (p, 0x20, 0);
-}
-
-void parport_pc_data_reverse (struct parport *p)
-{
-	__parport_pc_frob_control (p, 0x20, 0x20);
-}
-
 void parport_pc_init_state(struct pardevice *dev, struct parport_state *s)
 {
 	s->u.pc.ctr = 0xc | (dev->irq_func ? 0x10 : 0x0);

-- 
Robert Schiele			mailto:rschiele@uni-mannheim.de
Tel./Fax: +49-621-10059		http://webrum.uni-mannheim.de/math/rschiele/

[-- Attachment #2: Type: application/pgp-signature, Size: 524 bytes --]

Re: [PATCH] Re: 2.4.8-pre1 build error in drivers/parport/parport_pc.c

[Steven Cole]

On Friday 27 July 2001 02:12, Robert Schiele wrote:
> On Thu, Jul 26, 2001 at 07:53:11PM -0600, Steven Cole wrote:
> > I got the following errors building 2.4.8-pre1.
> >
> > parport_pc.c:257: redefinition of `parport_pc_write_data'
> > /usr/src/linux-2.4.8-pre1/include/linux/parport_pc.h:45:
> > `parport_pc_write_data' previously defined here parport_pc.c:262:
> > redefinition of `parport_pc_read_data'
> > /usr/src/linux-2.4.8-pre1/include/linux/parport_pc.h:53:
> > `parport_pc_read_data' previously defined here ...
> > make[3]: *** [parport_pc.o] Error 1
> > make[3]: Leaving directory `/usr/src/linux-2.4.8-pre1/drivers/parport'
>
> Hmm, these functions are multiply defined, namely in the c source and
> in it's header file. I see no reason why someone should do this. The
> problem was hidden in older kernel releases by the fact that these
> functions were declared "extern __inline__" which is absolutely
> nonsense in my opinion. So the solution should be to just remove these
> inline functions from the c source file, which can be done with the
> following simple and stupid patch.
>
> This should be the correct solution, or did I miss the vital point?
>
> Robert

Your patch works for me.  Thank you.  I can now print from my parallel port
printer using 2.4.8-pre1.  FWIW, here is my gcc version:

[steven@localhost steven]$ gcc -v
Reading specs from /usr/lib/gcc-lib/i586-mandrake-linux/2.96/specs
gcc version 2.96 20000731 (Linux-Mandrake 8.0 2.96-0.47mdk)

Steven

Re: [PATCH] Re: 2.4.8-pre1 build error in drivers/parport/parport_pc.c

[Philip Blundell]

[-- Attachment #1: Type: text/plain, Size: 741 bytes --]

>Hmm, these functions are multiply defined, namely in the c source and
>in it's header file. I see no reason why someone should do this. The
>problem was hidden in older kernel releases by the fact that these
>functions were declared "extern __inline__" which is absolutely
>nonsense in my opinion. So the solution should be to just remove these
>inline functions from the c source file, which can be done with the
>following simple and stupid patch.
>
>This should be the correct solution, or did I miss the vital point?

I think you did miss the vital point: this will probably break with 
CONFIG_PARPORT_OTHER.

Declaring them "extern inline" in parport_pc.h is exactly the right thing to 
do.  What do you think is wrong with that?

p.


[-- Attachment #2: Type: application/pgp-signature, Size: 237 bytes --]

Re: [PATCH] Re: 2.4.8-pre1 build error in drivers/parport/parport_pc.c

[Robert Schiele]

[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]

On Sat, Jul 28, 2001 at 05:39:22PM +0100, Philip Blundell wrote:
> I think you did miss the vital point: this will probably break with 
> CONFIG_PARPORT_OTHER.

No this cannot happen. These functions are only used from source files
that also include parport_pc.h. If this were not the case, it would
have been a bug anyway.

> 
> Declaring them "extern inline" in parport_pc.h is exactly the right thing to 
> do.  What do you think is wrong with that?

The "extern" was only an escape for the case that the compiler cannot
inline the function. Due to the fact, that current gcc has "static
inline" it is better to use this, because with "static inline" we do
not need to keep a global symbol just for the case the compiler is not
capable to inline the function in some place.

Let's turn the tables: What do you think is wrong with "static
inline"? In my opinion it's a much cleaner solution than "extern
inline".

Robert

-- 
Robert Schiele			mailto:rschiele@uni-mannheim.de
Tel./Fax: +49-621-10059		http://webrum.uni-mannheim.de/math/rschiele/

[-- Attachment #2: Type: application/pgp-signature, Size: 524 bytes --]

Re: [PATCH] Re: 2.4.8-pre1 build error in drivers/parport/parport_pc.c

[Philip Blundell]

[-- Attachment #1: Type: text/plain, Size: 759 bytes --]

>The "extern" was only an escape for the case that the compiler cannot
>inline the function. Due to the fact, that current gcc has "static
>inline" it is better to use this, because with "static inline" we do
>not need to keep a global symbol just for the case the compiler is not
>capable to inline the function in some place.

The versions in the .c file are there so that the "ops" structure can point to 
them.  The ones in the .h file are purely an optimisation to allow you to 
short-circuit the ops struct if you know only one driver is involved.

Changing this stuff to "static inline" still offends my sense of aesthetics 
somewhat, but I guess it's okay if you have checked that it still does the 
right thing in the CONFIG_PARPORT_OTHER case.

p.


[-- Attachment #2: Type: application/pgp-signature, Size: 237 bytes --]

Re: [PATCH] Re: 2.4.8-pre1 build error in drivers/parport/parport_pc.c

[Robert Schiele]

[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]

On Sat, Jul 28, 2001 at 10:06:43PM +0100, Philip Blundell wrote:
> >The "extern" was only an escape for the case that the compiler cannot
> >inline the function. Due to the fact, that current gcc has "static
> >inline" it is better to use this, because with "static inline" we do
> >not need to keep a global symbol just for the case the compiler is not
> >capable to inline the function in some place.
> 
> The versions in the .c file are there so that the "ops" structure can point to 
> them.  The ones in the .h file are purely an optimisation to allow you to 
> short-circuit the ops struct if you know only one driver is involved.

Because of that the compiler has to create a "real" function from the
code anyway. The way this is handled by gcc is stated in the gcc info
pages:

   "When a function is both inline and `static', if all calls to the
    function are integrated into the caller, and the function's
    address is never used, then the function's own assembler code is
    never referenced.  In this case, GNU CC does not actually output
    assembler code for the function, unless you specify the option
    `-fkeep-inline-functions'.  Some calls cannot be integrated for
    various reasons (in particular, calls that precede the function's
    definition cannot be integrated, and neither can recursive calls
    within the definition).  If there is a nonintegrated call, then
    the function is compiled to assembler code as usual.  The function
    must also be compiled as usual if the program refers to its
    address, because that can't be inlined."

> 
> Changing this stuff to "static inline" still offends my sense of aesthetics 
> somewhat, but I guess it's okay if you have checked that it still does the 
> right thing in the CONFIG_PARPORT_OTHER case.

I didn't test this before your mail. But to be absolutely sure, I
tested it now. --- And it works perfectly.

Robert

-- 
Robert Schiele			mailto:rschiele@uni-mannheim.de
Tel./Fax: +49-621-10059		http://webrum.uni-mannheim.de/math/rschiele/

[-- Attachment #2: Type: application/pgp-signature, Size: 524 bytes --]