Skip to contentSkip to conversation

deinline some functions in aic7xxx drivers, save 80k of text

PATCH In review Denis Vlasenko 6 participants 13 messages Apr 10, 2006 2 messages not in the archive

Subsystems (2)
SCSI AIC7XXX / AIC79XX SCSI driver
lore
Share & tools
lore

Denis Vlasenko

author
#0 lore raw

PATCH 1/1 deinline some functions in aic7xxx drivers, save 80k of text

Hi Justin, hi scsi crowd,

I moved a bunch of inlined functions from aic79xx_osm.h and aic79xx_inline.h to aic79xx_osm_o.c and aic79xx_inline.c and #included them from aic79xx_core.c. This is a bit ugly, but I was not sure where would maintainer like them to be moved to.

Same for aic7xxx. End result is:

# size */aic7*xx.o
   text    data     bss     dec     hex filename
 144078   11617     600  156295   26287 aic7xxx.old/aic79xx.o
 108610   16289     564  125463   1ea17 aic7xxx.old/aic7xxx.o
  85255   11617     600   97472   17cc0 aic7xxx/aic79xx.o
  83395   16289     564  100248   18798 aic7xxx/aic7xxx.o

I also spotted two bugs in the process, patches for those will follow.

Signed-off-by Denis Vlasenko <[email protected]>

--
vda

# size */aic7*xx.o
   text    data     bss     dec     hex filename
 144078   11617     600  156295   26287 aic7xxx.old/aic79xx.o
 108610   16289     564  125463   1ea17 aic7xxx.old/aic7xxx.o
  85255   11617     600   97472   17cc0 aic7xxx/aic79xx.o
  83395   16289     564  100248   18798 aic7xxx/aic7xxx.o
drivers/scsi/aic7xxx/aic79xx_core.c +6 -3
diff -urpN linux-2.6.16.org/drivers/scsi/aic7xxx/aic79xx_core.c linux-2.6.16.aic7/drivers/scsi/aic7xxx/aic79xx_core.c
@@ -50,6 +50,9 @@
 #include <dev/aic7xxx/aicasm/aicasm_insformat.h>
 #endif
 
+/* Were inlined, but moved out-of-line because they are HUGE: */
+#include "aic79xx_osm_o.c"
+#include "aic79xx_inline.c"
 
 /***************************** Lookup Tables **********************************/
 char *ahd_chip_names[] =

@@ -241,7 +244,7 @@ static int		ahd_handle_target_cmd(struct
 /******************************** Private Inlines *****************************/
 static __inline void	ahd_assert_atn(struct ahd_softc *ahd);
 static __inline int	ahd_currently_packetized(struct ahd_softc *ahd);
-static __inline int	ahd_set_active_fifo(struct ahd_softc *ahd);
+static int		ahd_set_active_fifo(struct ahd_softc *ahd);
 
 static __inline void
 ahd_assert_atn(struct ahd_softc *ahd)

@@ -278,7 +281,7 @@ ahd_currently_packetized(struct ahd_soft
 	return (packetized);
 }
 
-static __inline int
+static int
 ahd_set_active_fifo(struct ahd_softc *ahd)
 {
 	u_int active_fifo;

@@ -7131,7 +7134,7 @@ ahd_resume(struct ahd_softc *ahd)
  * scbid that should be restored once manipualtion
  * of the TCL entry is complete.
  */
-static __inline u_int
+static u_int
 ahd_index_busy_tcl(struct ahd_softc *ahd, u_int *saved_scbid, u_int tcl)
 {
 	/*
drivers/scsi/aic7xxx/aic79xx_inline.c +460 -0

460 lines not on this page · raw message

drivers/scsi/aic7xxx/aic79xx_inline.h +20 -474

570 lines not on this page · raw message

drivers/scsi/aic7xxx/aic79xx_osm.c +1 -1
diff -urpN linux-2.6.16.org/drivers/scsi/aic7xxx/aic79xx_osm.c linux-2.6.16.aic7/drivers/scsi/aic7xxx/aic79xx_osm.c
@@ -468,7 +468,7 @@ ahd_linux_queue(struct scsi_cmnd * cmd, 
 	return rtn;
 }
 
-static inline struct scsi_target **
+static struct scsi_target **
 ahd_linux_target_in_softc(struct scsi_target *starget)
 {
 	struct	ahd_softc *ahd =
drivers/scsi/aic7xxx/aic79xx_osm.h +11 -115

164 lines not on this page · raw message

drivers/scsi/aic7xxx/aic79xx_osm_o.c +114 -0

114 lines not on this page · raw message

drivers/scsi/aic7xxx/aic7xxx_core.c +4 -0
diff -urpN linux-2.6.16.org/drivers/scsi/aic7xxx/aic7xxx_core.c linux-2.6.16.aic7/drivers/scsi/aic7xxx/aic7xxx_core.c
@@ -50,6 +50,10 @@
 #include <dev/aic7xxx/aicasm/aicasm_insformat.h>
 #endif
 
+/* Were inlined, but moved out-of-line because they are HUGE: */
+#include "aic7xxx_osm_o.c"
+#include "aic7xxx_inline.c"
+
 /***************************** Lookup Tables **********************************/
 char *ahc_chip_names[] =
 {
drivers/scsi/aic7xxx/aic7xxx_inline.c +311 -0

311 lines not on this page · raw message

drivers/scsi/aic7xxx/aic7xxx_inline.h +15 -322

362 lines not on this page · raw message

drivers/scsi/aic7xxx/aic7xxx_osm.c +3 -3
diff -urpN linux-2.6.16.org/drivers/scsi/aic7xxx/aic7xxx_osm.c linux-2.6.16.aic7/drivers/scsi/aic7xxx/aic7xxx_osm.c
@@ -393,7 +393,7 @@ static int ahc_linux_unit;
 /********************************* Inlines ************************************/
 static __inline void ahc_linux_unmap_scb(struct ahc_softc*, struct scb*);
 
-static __inline int ahc_linux_map_seg(struct ahc_softc *ahc, struct scb *scb,
+static int ahc_linux_map_seg(struct ahc_softc *ahc, struct scb *scb,
 		 		      struct ahc_dma_seg *sg,
 				      dma_addr_t addr, bus_size_t len);
 

@@ -418,7 +418,7 @@ ahc_linux_unmap_scb(struct ahc_softc *ah
 	}
 }
 
-static __inline int
+static int
 ahc_linux_map_seg(struct ahc_softc *ahc, struct scb *scb,
 		  struct ahc_dma_seg *sg, dma_addr_t addr, bus_size_t len)
 {

@@ -492,7 +492,7 @@ ahc_linux_queue(struct scsi_cmnd * cmd, 
 	return rtn;
 }
 
-static inline struct scsi_target **
+static struct scsi_target **
 ahc_linux_target_in_softc(struct scsi_target *starget)
 {
 	struct	ahc_softc *ahc =
drivers/scsi/aic7xxx/aic7xxx_osm.h +10 -76

112 lines not on this page · raw message

drivers/scsi/aic7xxx/aic7xxx_osm_o.c +75 -0

75 lines not on this page · raw message

author
#1 lore raw
Quoted 2 lines · Denis Vlasenko

I also spotted two bugs in the process, patches
for those will follow.

Fix ahc_pci_write_config's (wrong order of arguments).

Signed-off-by Denis Vlasenko <[email protected]>

--
vda

Fix a bug uncovered by previous change

drivers/scsi/aic7xxx/aic7xxx_pci.c +6 -6
diff -urpN linux-2.6.16.aic7/drivers/scsi/aic7xxx/aic7xxx_pci.c linux-2.6.16.aic7_2/drivers/scsi/aic7xxx/aic7xxx_pci.c
@@ -2036,12 +2036,12 @@ ahc_pci_resume(struct ahc_softc *ahc)
 	 * that the OS doesn't know about and rely on our chip
 	 * reset handler to handle the rest.
 	 */
-	ahc_pci_write_config(ahc->dev_softc, DEVCONFIG, /*bytes*/4,
-			     ahc->bus_softc.pci_softc.devconfig);
-	ahc_pci_write_config(ahc->dev_softc, PCIR_COMMAND, /*bytes*/1,
-			     ahc->bus_softc.pci_softc.command);
-	ahc_pci_write_config(ahc->dev_softc, CSIZE_LATTIME, /*bytes*/1,
-			     ahc->bus_softc.pci_softc.csize_lattime);
+	ahc_pci_write_config(ahc->dev_softc, DEVCONFIG,
+			     ahc->bus_softc.pci_softc.devconfig, /*bytes*/4);
+	ahc_pci_write_config(ahc->dev_softc, PCIR_COMMAND,
+			     ahc->bus_softc.pci_softc.command, /*bytes*/1);
+	ahc_pci_write_config(ahc->dev_softc, CSIZE_LATTIME,
+			     ahc->bus_softc.pci_softc.csize_lattime, /*bytes*/1);
 	if ((ahc->flags & AHC_HAS_TERM_LOGIC) != 0) {
 		struct	seeprom_descriptor sd;
 		u_int	sxfrctl1;
author
#2 lore raw
In reply to Denis Vlasenko
Quoted 2 lines · Denis Vlasenko

I also spotted two bugs in the process, patches
for those will follow.

ahd_delay(usec) is buggy. Just think how would it work with usec == 1024*100 + 1...

Signed-off-by Denis Vlasenko <[email protected]>

--
vda

Fix yet another bug

drivers/scsi/aic7xxx/aic79xx_osm_o.c +4 -2
diff -urpN linux-2.6.16.aic7_2/drivers/scsi/aic7xxx/aic79xx_osm_o.c linux-2.6.16.aic7_3/drivers/scsi/aic7xxx/aic79xx_osm_o.c
@@ -28,9 +28,11 @@ ahd_delay(long usec)
 	 * multi-millisecond waits.  Wait at most
 	 * 1024us per call.
 	 */
+	udelay(usec & 1023);
+	usec >>= 10;
 	while (usec > 0) {
-		udelay(usec % 1024);
-		usec -= 1024;
+		udelay(1024);
+		usec--;
 	}
 }
drivers/scsi/aic7xxx/aic7xxx_osm_o.c +4 -2
diff -urpN linux-2.6.16.aic7_2/drivers/scsi/aic7xxx/aic7xxx_osm_o.c linux-2.6.16.aic7_3/drivers/scsi/aic7xxx/aic7xxx_osm_o.c
@@ -14,9 +14,11 @@ ahc_delay(long usec)
 	 * multi-millisecond waits.  Wait at most
 	 * 1024us per call.
 	 */
+	udelay(usec & 1023);
+	usec >>= 10;
 	while (usec > 0) {
-		udelay(usec % 1024);
-		usec -= 1024;
+		udelay(1024);
+		usec--;
 	}
 }

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

#5 lore raw

[Full quote and readded CC adresses. My fault, pressed wrong button]

Quoted 12 lines · Denis Vlasenko
Quoted 10 lines · Rolf Eike Beer

Am Montag, 10. April 2006 07:49 schrieben Sie:

Quoted 6 lines · Denis Vlasenko
Quoted 2 lines

I also spotted two bugs in the process, patches
for those will follow.

ahd_delay(usec) is buggy. Just think how would it work with usec == 1024*100 + 1...

This is unneeded. The biggest argument this function is ever called with is 1000.

Quoted 7 lines

I know.

Quoted 2 lines

I would suggest to delete this function completely. If one ever has to wait for a longer period mdelay() is the right function to call.

I am leaving it up to maintainer to decide. After all, the driver is for multiple OSes, other OS may lack mdelay().

The comment says about multiple milliseconds sleeps which just don't happen.

Eike

author
#6 lore raw
Quoted 17 lines · Rolf Eike Beer

[Full quote and readded CC adresses. My fault, pressed wrong button]

Quoted 14 lines · Denis Vlasenko
Quoted 10 lines · Rolf Eike Beer

Am Montag, 10. April 2006 07:49 schrieben Sie:

Quoted 6 lines · Denis Vlasenko
Quoted 2 lines

I also spotted two bugs in the process, patches
for those will follow.

ahd_delay(usec) is buggy. Just think how would it work with usec == 1024*100 + 1...

This is unneeded. The biggest argument this function is ever called with is 1000.

I know.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

I would suggest to delete this function completely. If one ever has to wait for a longer period mdelay() is the right function to call.

I am leaving it up to maintainer to decide. After all, the driver is for multiple OSes, other OS may lack mdelay().

The comment says about multiple milliseconds sleeps which just don't happen.

# grep -r ah._delay.[A-Z] .
./aic7xxx_core.c:       ahc_delay(AHC_BUSRESET_DELAY);
./aic79xx_core.c:       ahd_delay(AHD_BUSRESET_DELAY);
./aic79xx_core.c:       ahd_delay(AHD_BUSRESET_DELAY);

I am not sure that this constant won't be redefined to something large. If maintainer knows better, he can take care of it.
I just fixed an obvious latent bug.
--
vda

#7 lore raw
In reply to Rolf Eike Beer
Quoted 5 lines · Rolf Eike Beer
Quoted 2 lines · Denis Vlasenko

I am leaving it up to maintainer to decide. After all, the driver is for multiple OSes, other OS may lack mdelay().

The comment says about multiple milliseconds sleeps which just don't happen.

Given what ah{c,d}_delay are (OS dependent wrappers) and how they are used (definitely not for multi-msec delays), they should just be changed into a #define ah{c,d}_delay(us) udelay(us) or into void inline ah{c,d}_delay(long us) {udelay(us);}.

Signature · 3 lines
Stefan Richter
-=====-=-==- -=-- -=-=-
http://arcgraph.de/sr/
#8 lore raw
Quoted 10 lines · Stefan Richter
Quoted 4 lines · Rolf Eike Beer
Quoted 2 lines · Denis Vlasenko

I am leaving it up to maintainer to decide. After all, the driver is for multiple OSes, other OS may lack mdelay().

The comment says about multiple milliseconds sleeps which just don't happen.

Given what ah{c,d}_delay are (OS dependent wrappers) and how they are used (definitely not for multi-msec delays), they should just be changed into a #define ah{c,d}_delay(us) udelay(us) or into void inline ah{c,d}_delay(long us) {udelay(us);}.

I'd rather do a #define. Inlining simple functions is quite unneccessary here.

Re multiplatform development: aic7{9,x}xx have ceased to be multiplatfrom since the integration of scsi_transport_spi. So I wouldn't worry too much about it.

Cheers,

Hannes

Signature · 8 lines
Dr. Hannes Reinecke			[email protected]
SuSE Linux Products GmbH		S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#9 lore raw
Quoted 3 lines · Hannes Reinecke

Re multiplatform development: aic7{9,x}xx have ceased to be multiplatfrom since the integration of scsi_transport_spi. So I wouldn't worry too much about it.

Then it may be possible to save even more than 80k of text.

Signature · 3 lines
Stefan Richter
-=====-=-==- -=-- -=-=-
http://arcgraph.de/sr/
#10 lore raw
In reply to Rolf Eike Beer
Quoted 22 lines · Rolf Eike Beer

[Full quote and readded CC adresses. My fault, pressed wrong button]

Quoted 19 lines · Denis Vlasenko
Quoted 10 lines · Rolf Eike Beer

Am Montag, 10. April 2006 07:49 schrieben Sie:

Quoted 6 lines · Denis Vlasenko
Quoted 2 lines

I also spotted two bugs in the process, patches
for those will follow.

ahd_delay(usec) is buggy. Just think how would it work with usec == 1024*100 + 1...

This is unneeded. The biggest argument this function is ever called with is 1000.

I know.

Quoted 2 lines

I would suggest to delete this function completely. If one ever has to wait for a longer period mdelay() is the right function to call.

I am leaving it up to maintainer to decide. After all, the driver is for multiple OSes, other OS may lack mdelay().

actually this driver isn't shared anymore with other oses so it's ok to remove (buggy) abstractions

#11 lore raw
In reply to Denis Vlasenko

Denis Vlasenko wrote:
...

Quoted 1 line

+++ linux-2.6.16.aic7/drivers/scsi/aic7xxx/aic79xx_core.c Sun Apr 9 21:49:25 2006

...

Quoted 2 lines
drivers/scsi/aic7xxx/aic79xx_core.c +2 -0
+#include "aic79xx_osm_o.c"
+#include "aic79xx_inline.c"

...

Quoted 1 line

+++ linux-2.6.16.aic7/drivers/scsi/aic7xxx/aic7xxx_core.c Sun Apr 9 21:49:25 2006

...

drivers/scsi/aic7xxx/aic7xxx_core.c line 55

Quoted 2 lines
drivers/scsi/aic7xxx/aic7xxx_core.c +2 -0
+#include "aic7xxx_osm_o.c"
+#include "aic7xxx_inline.c"

...

Instead of including c files with function definitions, you should add function prototypes to header files (it seems you already did so) and include only the header files. Include these header files in the c files which call the functions as well as in the c files which define the functions.

It is obviously necessary to modify the Makefile to have aic7?xx_osm_o.o and aic7?xx_inline.o linked to an appropriate .ko file.

Furthermore, aic7?xx_inline.c are not very fitting file names since they do not contain inline functions. aic7?xx_osm_o.c are somewhat strange names either. Can't you move the functions into existing c files? E.g. into those which contain most of the calls to the now de-inlined functions. From the point of view of cross-OS driver maintenance (but not necessarily from the point of view of Linux driver maintenance), it may be useful to distinguish between functions used across OSs and those used only in Linux when deciding where to move the functions.

Signature · 3 lines
Stefan Richter
-=====-=-==- -=-- -=-=-
http://arcgraph.de/sr/
author
#12 lore raw
Quoted 13 lines · Stefan Richter

Denis Vlasenko wrote:
...

Quoted 1 line

+++ linux-2.6.16.aic7/drivers/scsi/aic7xxx/aic79xx_core.c Sun Apr 9 21:49:25 2006

...

Quoted 2 lines
drivers/scsi/aic7xxx/aic79xx_core.c +2 -0
+#include "aic79xx_osm_o.c"
+#include "aic79xx_inline.c"

...

Quoted 1 line

+++ linux-2.6.16.aic7/drivers/scsi/aic7xxx/aic7xxx_core.c Sun Apr 9 21:49:25 2006

...

Quoted 2 lines
drivers/scsi/aic7xxx/aic7xxx_core.c +2 -0
+#include "aic7xxx_osm_o.c"
+#include "aic7xxx_inline.c"

...

Quoted 5 lines

Instead of including c files with function definitions, you should add function prototypes to header files (it seems you already did so) and include only the header files. Include these header files in the c files which call the functions as well as in the c files which define the functions.

I will do this if maintainer will inform me that he wants it done. Or maybe he wants it done in some other way, who knows? I am not lazy, I am just not good at reading other peoples' minds.

Quoted 2 lines

It is obviously necessary to modify the Makefile to have aic7?xx_osm_o.o and aic7?xx_inline.o linked to an appropriate .ko file.

I did compile test my changes.

Quoted 3 lines

Furthermore, aic7?xx_inline.c are not very fitting file names since they do not contain inline functions. aic7?xx_osm_o.c are somewhat strange names either. Can't you move the functions into existing c files? E.g.

I can, but I won't without maintainer's consent.

Quoted 5 lines

into those which contain most of the calls to the now de-inlined functions. From the point of view of cross-OS driver maintenance (but not necessarily from the point of view of Linux driver maintenance), it may be useful to distinguish between functions used across OSs and those used only in Linux when deciding where to move the functions.

--
vda

#13 lore raw
Quoted 5 lines · Denis Vlasenko
Quoted 2 lines · Stefan Richter

It is obviously necessary to modify the Makefile to have aic7?xx_osm_o.o and aic7?xx_inline.o linked to an appropriate .ko file.

I did compile test my changes.

Sure, if you #include .c files (which is awkward), there are no additional .o files which would need to be added to the Makefile.

Signature · 3 lines
Stefan Richter
-=====-=-==- -=-- -=-=-
http://arcgraph.de/sr/