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

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'

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
@@ -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:[email protected]
Tel./Fax: +49-621-10059		http://webrum.uni-mannheim.de/math/rschiele/

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.

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

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.

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:[email protected]
Tel./Fax: +49-621-10059		http://webrum.uni-mannheim.de/math/rschiele/

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.

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

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:[email protected]
Tel./Fax: +49-621-10059		http://webrum.uni-mannheim.de/math/rschiele/