Skip to contentSkip to conversation

drivers/char/specialix.c: stop inlining largish static functions

PATCH In review Denys Vlasenko 4 participants 6 messages Apr 8, 2008

Subsystems (2)
TTY layer and serial drivers Char and misc drivers
lore
Share & tools
lore

Denys Vlasenko

author
#0 lore raw

PATCH 1/1 drivers/char/specialix.c: stop inlining largish static functions

Hi Alan,

drivers/char/specialix.c has unusually large number of static inline functions - 22.

I looked through them. The file is positively inline-happy. Inlines with udelay() calls. Inlines with complex loops. Nested inlines. Rarely called inlines (e.g. with request_region inside).

This patch removes "inline" from 15 static functions (regardless of number of callsites - gcc nowadays auto-inlines statics with one callsite).

Size difference for 32bit x86:
text data bss dec hex filename

  21669     204    8780   30653    77bd linux-2.6-ALLYES/drivers/char/specialix.o
  18470     204    8780   27454    6b3e linux-2.6.inline-ALLYES/drivers/char/specialix.o
Signed-off-by Denys Vlasenko <[email protected]>

--
vda

drivers/char/specialix.c +25 -19
diff -urp -U 10 linux-2.6/drivers/char/specialix.c linux-2.6.inline/drivers/char/specialix.c
@@ -190,40 +190,46 @@ static struct specialix_board sx_board[S
 static struct specialix_port sx_port[SX_NBOARD * SX_NPORT];
 
 
 #ifdef SPECIALIX_TIMER
 static struct timer_list missed_irq_timer;
 static irqreturn_t sx_interrupt(int irq, void * dev_id);
 #endif
 
 
 
-static inline int sx_paranoia_check(struct specialix_port const * port,
+#ifdef SPECIALIX_PARANOIA_CHECK
+static int sx_paranoia_check(const struct specialix_port *port,
 				    char *name, const char *routine)
 {
-#ifdef SPECIALIX_PARANOIA_CHECK
-	static const char *badmagic =
+	static const char badmagic[] =
 		KERN_ERR "sx: Warning: bad specialix port magic number for device %s in %s\n";
-	static const char *badinfo =
+	static const char badinfo[] =
 		KERN_ERR "sx: Warning: null specialix port for device %s in %s\n";
 
 	if (!port) {
 		printk(badinfo, name, routine);
 		return 1;
 	}
 	if (port->magic != SPECIALIX_MAGIC) {
 		printk(badmagic, name, routine);
 		return 1;
 	}
-#endif
 	return 0;
 }
+#else
+static inline int sx_paranoia_check(const struct specialix_port *port,
+				    char *name, const char *routine)
+{
+	return 0;
+}
+#endif
 
 
 /*
  *
  *  Service functions for specialix IO8+ driver.
  *
  */
 
 /* Get board number from pointer */
 static inline int board_No (struct specialix_board * bp)

@@ -278,40 +284,40 @@ static inline unsigned char sx_in_off(st
 static inline void sx_out_off(struct specialix_board  * bp, unsigned short reg,
 			  unsigned char val)
 {
 	bp->reg = reg;
 	outb (reg, bp->base + SX_ADDR_REG);
 	outb (val, bp->base + SX_DATA_REG);
 }
 
 
 /* Wait for Channel Command Register ready */
-static inline void sx_wait_CCR(struct specialix_board  * bp)
+static void sx_wait_CCR(struct specialix_board  * bp)
 {
 	unsigned long delay, flags;
 	unsigned char ccr;
 
 	for (delay = SX_CCR_TIMEOUT; delay; delay--) {
 		spin_lock_irqsave(&bp->lock, flags);
 		ccr = sx_in(bp, CD186x_CCR);
 		spin_unlock_irqrestore(&bp->lock, flags);
 		if (!ccr)
 			return;
 		udelay (1);
 	}
 
 	printk(KERN_ERR "sx%d: Timeout waiting for CCR.\n", board_No(bp));
 }
 
 
 /* Wait for Channel Command Register ready */
-static inline void sx_wait_CCR_off(struct specialix_board  * bp)
+static void sx_wait_CCR_off(struct specialix_board  * bp)
 {
 	unsigned long delay;
 	unsigned char crr;
 	unsigned long flags;
 
 	for (delay = SX_CCR_TIMEOUT; delay; delay--) {
 		spin_lock_irqsave(&bp->lock, flags);
 		crr = sx_in_off(bp, CD186x_CCR);
 		spin_unlock_irqrestore(&bp->lock, flags);
 		if (!crr)

@@ -320,29 +326,29 @@ static inline void sx_wait_CCR_off(struc
 	}
 
 	printk(KERN_ERR "sx%d: Timeout waiting for CCR.\n", board_No(bp));
 }
 
 
 /*
  *  specialix IO8+ IO range functions.
  */
 
-static inline int sx_request_io_range(struct specialix_board * bp)
+static int sx_request_io_range(struct specialix_board * bp)
 {
 	return request_region(bp->base,
 		bp->flags & SX_BOARD_IS_PCI ? SX_PCI_IO_SPACE : SX_IO_SPACE,
 		"specialix IO8+") == NULL;
 }
 
 
-static inline void sx_release_io_range(struct specialix_board * bp)
+static void sx_release_io_range(struct specialix_board * bp)
 {
 	release_region(bp->base,
 	               bp->flags&SX_BOARD_IS_PCI?SX_PCI_IO_SPACE:SX_IO_SPACE);
 }
 
 
 /* Set the IRQ using the RTS lines that run to the PAL on the board.... */
 static int sx_set_irq ( struct specialix_board *bp)
 {
 	int virq;

@@ -592,21 +598,21 @@ static int sx_probe(struct specialix_boa
 
 	func_exit();
 	return 0;
 }
 
 /*
  *
  *  Interrupt processing routines.
  * */
 
-static inline struct specialix_port * sx_get_port(struct specialix_board * bp,
+static struct specialix_port * sx_get_port(struct specialix_board * bp,
 					       unsigned char const * what)
 {
 	unsigned char channel;
 	struct specialix_port * port = NULL;
 
 	channel = sx_in(bp, CD186x_GICR) >> GICR_CHAN_OFF;
 	dprintk (SX_DEBUG_CHAN, "channel: %d\n", channel);
 	if (channel < CD186x_NCH) {
 		port = &sx_port[board_No(bp) * SX_NPORT + channel];
 		dprintk (SX_DEBUG_CHAN, "port: %d %p flags: 0x%x\n",board_No(bp) * SX_NPORT + channel,  port, port->flags & ASYNC_INITIALIZED);

@@ -616,21 +622,21 @@ static inline struct specialix_port * sx
 			func_exit();
 			return port;
 		}
 	}
 	printk(KERN_INFO "sx%d: %s interrupt from invalid port %d\n",
 	       board_No(bp), what, channel);
 	return NULL;
 }
 
 
-static inline void sx_receive_exc(struct specialix_board * bp)
+static void sx_receive_exc(struct specialix_board * bp)
 {
 	struct specialix_port *port;
 	struct tty_struct *tty;
 	unsigned char status;
 	unsigned char ch, flag;
 
 	func_enter();
 
 	port = sx_get_port(bp, "Receive");
 	if (!port) {

@@ -688,21 +694,21 @@ static inline void sx_receive_exc(struct
 
 	else
 		flag = TTY_NORMAL;
 
 	if(tty_insert_flip_char(tty, ch, flag))
 		tty_flip_buffer_push(tty);
 	func_exit();
 }
 
 
-static inline void sx_receive(struct specialix_board * bp)
+static void sx_receive(struct specialix_board * bp)
 {
 	struct specialix_port *port;
 	struct tty_struct *tty;
 	unsigned char count;
 
 	func_enter();
 
 	if (!(port = sx_get_port(bp, "Receive"))) {
 		dprintk (SX_DEBUG_RX, "Hmm, couldn't find port.\n");
 		func_exit();

@@ -716,21 +722,21 @@ static inline void sx_receive(struct spe
 
 	tty_buffer_request_room(tty, count);
 
 	while (count--)
 		tty_insert_flip_char(tty, sx_in(bp, CD186x_RDR), TTY_NORMAL);
 	tty_flip_buffer_push(tty);
 	func_exit();
 }
 
 
-static inline void sx_transmit(struct specialix_board * bp)
+static void sx_transmit(struct specialix_board * bp)
 {
 	struct specialix_port *port;
 	struct tty_struct *tty;
 	unsigned char count;
 
 	func_enter();
 	if (!(port = sx_get_port(bp, "Transmit"))) {
 		func_exit();
 		return;
 	}

@@ -794,21 +800,21 @@ static inline void sx_transmit(struct sp
 		port->IER &= ~IER_TXRDY;
 		sx_out(bp, CD186x_IER, port->IER);
 	}
 	if (port->xmit_cnt <= port->wakeup_chars)
  		tty_wakeup(tty);
 
 	func_exit();
 }
 
 
-static inline void sx_check_modem(struct specialix_board * bp)
+static void sx_check_modem(struct specialix_board * bp)
 {
 	struct specialix_port *port;
 	struct tty_struct *tty;
 	unsigned char mcr;
 	int msvr_cd;
 
 	dprintk (SX_DEBUG_SIGNALS, "Modem intr. ");
 	if (!(port = sx_get_port(bp, "Modem")))
 		return;
 

@@ -964,21 +970,21 @@ static void turn_ints_on (struct special
 	}
 	spin_lock_irqsave(&bp->lock, flags);
 	(void) sx_in (bp, 0); /* Turn ON interrupts. */
 	spin_unlock_irqrestore(&bp->lock, flags);
 
 	func_exit();
 }
 
 
 /* Called with disabled interrupts */
-static inline int sx_setup_board(struct specialix_board * bp)
+static int sx_setup_board(struct specialix_board * bp)
 {
 	int error;
 
 	if (bp->flags & SX_BOARD_ACTIVE)
 		return 0;
 
 	if (bp->flags & SX_BOARD_IS_PCI)
 		error = request_irq(bp->irq, sx_interrupt, IRQF_DISABLED | IRQF_SHARED, "specialix IO8+", bp);
 	else
 		error = request_irq(bp->irq, sx_interrupt, IRQF_DISABLED, "specialix IO8+", bp);

@@ -987,21 +993,21 @@ static inline int sx_setup_board(struct 
 		return error;
 
 	turn_ints_on (bp);
 	bp->flags |= SX_BOARD_ACTIVE;
 
 	return 0;
 }
 
 
 /* Called with disabled interrupts */
-static inline void sx_shutdown_board(struct specialix_board *bp)
+static void sx_shutdown_board(struct specialix_board *bp)
 {
 	func_enter();
 
 	if (!(bp->flags & SX_BOARD_ACTIVE)) {
 		func_exit();
 		return;
 	}
 
 	bp->flags &= ~SX_BOARD_ACTIVE;
 

@@ -1882,21 +1888,21 @@ static int sx_tiocmset(struct tty_struct
 	spin_lock_irqsave(&bp->lock, flags);
 	sx_out(bp, CD186x_CAR, port_No(port));
 	sx_out(bp, CD186x_MSVR, port->MSVR);
 	spin_unlock_irqrestore(&bp->lock, flags);
 	spin_unlock_irqrestore(&port->lock, flags);
 	func_exit();
 	return 0;
 }
 
 
-static inline void sx_send_break(struct specialix_port * port, unsigned long length)
+static void sx_send_break(struct specialix_port * port, unsigned long length)
 {
 	struct specialix_board *bp = port_Board(port);
 	unsigned long flags;
 
 	func_enter();
 
 	spin_lock_irqsave (&port->lock, flags);
 	port->break_length = SPECIALIX_TPS / HZ * length;
 	port->COR2 |= COR2_ETC;
 	port->IER  |= IER_TXRDY;

@@ -1909,21 +1915,21 @@ static inline void sx_send_break(struct 
 	sx_wait_CCR(bp);
 	spin_lock_irqsave(&bp->lock, flags);
 	sx_out(bp, CD186x_CCR, CCR_CORCHG2);
 	spin_unlock_irqrestore(&bp->lock, flags);
 	sx_wait_CCR(bp);
 
 	func_exit();
 }
 
 
-static inline int sx_set_serial_info(struct specialix_port * port,
+static int sx_set_serial_info(struct specialix_port * port,
                                      struct serial_struct __user * newinfo)
 {
 	struct serial_struct tmp;
 	struct specialix_board *bp = port_Board(port);
 	int change_speed;
 
 	func_enter();
 	/*
 	if (!access_ok(VERIFY_READ, (void *) newinfo, sizeof(tmp))) {
 		func_exit();

@@ -1971,21 +1977,21 @@ static inline int sx_set_serial_info(str
 		port->custom_divisor = tmp.custom_divisor;
 	}
 	if (change_speed) {
 		sx_change_speed(bp, port);
 	}
 	func_exit();
 	return 0;
 }
 
 
-static inline int sx_get_serial_info(struct specialix_port * port,
+static int sx_get_serial_info(struct specialix_port * port,
 				     struct serial_struct __user *retinfo)
 {
 	struct serial_struct tmp;
 	struct specialix_board *bp = port_Board(port);
 
 	func_enter();
 
 	/*
 	if (!access_ok(VERIFY_WRITE, (void *) retinfo, sizeof(tmp)))
 		return -EFAULT;
#1 lore raw
Quoted 14 lines · Denys Vlasenko

Hi Alan,

drivers/char/specialix.c has unusually large number of static inline functions - 22.

I looked through them. The file is positively inline-happy. Inlines with udelay() calls. Inlines with complex loops. Nested inlines. Rarely called inlines (e.g. with request_region inside).

This patch removes "inline" from 15 static functions (regardless of number of callsites - gcc nowadays auto-inlines statics with one callsite).

Quoted 7 lines

Size difference for 32bit x86:
text data bss dec hex filename

  21669     204    8780   30653    77bd linux-2.6-ALLYES/drivers/char/specialix.o
  18470     204    8780   27454    6b3e linux-2.6.inline-ALLYES/drivers/char/specialix.o
Signed-off-by Denys Vlasenko <[email protected]>

Ack!

Signed-off-by Rogier Wolff <[email protected]>

Roger.

Signature · 6 lines
** [email protected] ** http://www.BitWizard.nl/ ** +31-15-2600998 **
**    Delftechpark 26 2628 XH  Delft, The Netherlands. KVK: 27239233    **
*-- BitWizard writes Linux device drivers for any device you may have! --*
Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement. 
Does it sit on the couch all day? Is it unemployed? Please be specific! 
Define 'it' and what it isn't doing. --------- Adapted from lxrbot FAQ
#3 lore raw
In reply to Denys Vlasenko
Quoted 14 lines · Denys Vlasenko

Hi Alan,

drivers/char/specialix.c has unusually large number of static inline functions - 22.

I looked through them. The file is positively inline-happy. Inlines with udelay() calls. Inlines with complex loops. Nested inlines. Rarely called inlines (e.g. with request_region inside).

This patch removes "inline" from 15 static functions (regardless of number of callsites - gcc nowadays auto-inlines statics with one callsite).

Quoted 7 lines

Size difference for 32bit x86:
text data bss dec hex filename

  21669     204    8780   30653    77bd linux-2.6-ALLYES/drivers/char/specialix.o
  18470     204    8780   27454    6b3e linux-2.6.inline-ALLYES/drivers/char/specialix.o
Signed-off-by Denys Vlasenko <[email protected]>

- static const char *badmagic =
+ static const char badmagic[] =
KERN_ERR "sx: Warning: bad specialix port magic number for device %s in %s\n";
- static const char *badinfo =
+ static const char badinfo[] =
KERN_ERR "sx: Warning: null specialix port for device %s in %s\n";

BTW what's this good for? I mean, why we need this as a variable not directly as a parameter?

if (!port) {
printk(badinfo, name, routine);
return 1;
}
if (port->magic != SPECIALIX_MAGIC) {
printk(badmagic, name, routine);
return 1;
}

#4 lore raw
Quoted 5 lines · Jiri Slaby
drivers/char/specialix.c +1 -1
-	static const char *badmagic =
+	static const char badmagic[] =
 		KERN_ERR "sx: Warning: bad specialix port magic number for 
 		device %s in %s\n";

[...]

drivers/char/specialix.c static struct specialix_board sx_board[S · line 216

Quoted 4 lines
drivers/char/specialix.c +0 -0
 	if (port->magic != SPECIALIX_MAGIC) {
 		printk(badmagic, name, routine);
 		return 1;
 	}

In a galaxy, far, far away, someone thought that this would save memory. Especially if you print a message multiple times, the compiler usually doesn't merge the string constants of two copies of:

	printk (KERN_ERR "Bad magic number...");

Maybe it does now. (I don't think I came up with this. I probably copied it over from somewhere....)

Roger.

Signature · 6 lines
** [email protected] ** http://www.BitWizard.nl/ ** +31-15-2600998 **
**    Delftechpark 26 2628 XH  Delft, The Netherlands. KVK: 27239233    **
*-- BitWizard writes Linux device drivers for any device you may have! --*
Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement. 
Does it sit on the couch all day? Is it unemployed? Please be specific! 
Define 'it' and what it isn't doing. --------- Adapted from lxrbot FAQ
author
#5 lore raw
In reply to Jiri Slaby
Quoted 31 lines · Jiri Slaby
Quoted 19 lines

drivers/char/specialix.c has unusually large number of static inline functions - 22.

I looked through them. The file is positively inline-happy. Inlines with udelay() calls. Inlines with complex loops. Nested inlines. Rarely called inlines (e.g. with request_region inside).

This patch removes "inline" from 15 static functions (regardless of number of callsites - gcc nowadays auto-inlines statics with one callsite).

Size difference for 32bit x86:
text data bss dec hex filename

  21669     204    8780   30653    77bd linux-2.6-ALLYES/drivers/char/specialix.o
  18470     204    8780   27454    6b3e linux-2.6.inline-ALLYES/drivers/char/specialix.o
Signed-off-by Denys Vlasenko <[email protected]>
drivers/char/specialix.c +2 -2
-	static const char *badmagic =
+	static const char badmagic[] =
  		KERN_ERR "sx: Warning: bad specialix port magic number for device %s in %s\n";
-	static const char *badinfo =
+	static const char badinfo[] =
  		KERN_ERR "sx: Warning: null specialix port for device %s in %s\n";

BTW what's this good for? I mean, why we need this as a variable not directly as a parameter?

Quoted 8 lines
  	if (!port) {
  		printk(badinfo, name, routine);
  		return 1;
  	}
  	if (port->magic != SPECIALIX_MAGIC) {
  		printk(badmagic, name, routine);
  		return 1;
  	}

I am sure these strings can be used directly in printk, there should be no size difference (sans gcc adding padding to string arrays "just because").

I chose to make minimal change which only eliminates the waste of having a pinter to these strings (char *msg = "xxx" versus char msg[] = "xxx"), leaving more extensive editing to someone who wants to attack specialix.c on the wider front. --
vda