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

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

[Denys Vlasenko]

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

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 <vda.linux@googlemail.com>
--
vda

[-- Attachment #2: deinline_specialix.diff --]
[-- Type: text/x-diff, Size: 9536 bytes --]

diff -urp -U 10 linux-2.6/drivers/char/specialix.c linux-2.6.inline/drivers/char/specialix.c
--- linux-2.6/drivers/char/specialix.c	2008-03-30 03:27:43.000000000 +0200
+++ linux-2.6.inline/drivers/char/specialix.c	2008-04-08 12:32:23.000000000 +0200
@@ -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;

Re: [PATCH] drivers/char/specialix.c: stop inlining largish static functions

[Rogier Wolff]

On Tue, Apr 08, 2008 at 12:44:53PM +0200, Denys Vlasenko wrote:
> 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 <vda.linux@googlemail.com>

Ack!

Signed-off-by: Rogier Wolff <R.E.Wolff@BitWizard.nl> 

	Roger. 

-- 
** R.E.Wolff@BitWizard.nl ** 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

Re: [PATCH] drivers/char/specialix.c: stop inlining largish static functions

[Alan Cox]

> > Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
> 
> Ack!
> 
> Signed-off-by: Rogier Wolff <R.E.Wolff@BitWizard.nl> 

Acked-by: Alan Cox <alan@redhat.com>

Works for me. I have other patches that touch the same file but nothing
overlapping or that will be hard to resolve.

Re: [PATCH] drivers/char/specialix.c: stop inlining largish static functions

[Jiri Slaby]

On 04/08/2008 12:44 PM, Denys Vlasenko wrote:
> 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 <vda.linux@googlemail.com>

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

Re: [PATCH] drivers/char/specialix.c: stop inlining largish static functions

[Rogier Wolff]

On Tue, Apr 08, 2008 at 11:38:11PM +0200, Jiri Slaby wrote:
> 
> -	static const char *badmagic =
> +	static const char badmagic[] =
>  		KERN_ERR "sx: Warning: bad specialix port magic number for 
>  		device %s in %s\n";
[...]
>  	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. 

-- 
** R.E.Wolff@BitWizard.nl ** 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

Re: [PATCH] drivers/char/specialix.c: stop inlining largish static functions

[Denys Vlasenko]

On Tuesday 08 April 2008 23:38, Jiri Slaby wrote:
> > 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 <vda.linux@googlemail.com>
> 
> -	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;
>   	}

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