[git patches 1/2] warnings: attack valid cases spotted by warnings

[git patches 1/2] warnings: attack valid cases spotted by warnings

[Jeff Garzik]

Those "may be used uninitialized" warnings are annoying.  So annoying
that kernel developers tune them out, and that occasionally hides
real bugs, as my past patches (and those below) indicate.

I included the full-length changelog below the diffstat, because
that is where the best explanation for each change is found.  In most
cases that's where all the length is -- a paragraph or two describing
what is usually a one-line code change, usually an initialization or
simple cleanup.

This is the first of two git pull sets, the -parent-.  If you pull
'warnings' branch, you get what you see below and nothing else.
WYSIWYG.  You'll see in the second email why I distinguish the two.

Please pull from 'warnings' branch of
master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git warnings

Jeff Garzik (11):
      kernel/auditfilter: kill bogus uninit'd-var compiler warning
      [netdrvr] natsemi: Fix device removal bug
      [netdrvr] eepro100, ne2k-pci: abort resume if pci_enable_device() fails
      drivers/usb/misc/auerswald: fix status check, remove redundant check
      drivers/net/wan/pc300_drv: fix bug caught by gcc warning
      drivers/telephony/ixj: cleanup and fix gcc warning
      drivers/mtd/ubi/eba: minor cleanup: tighten scope of a local var
      drivers/net/wan/sbni: kill uninit'd var warning
      drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning
      [libata] sata_mv: use pci_try_set_mwi()
      drivers/atm/ambassador: kill uninit'd var warning, and fix bug

 drivers/ata/sata_mv.c                  |    2 +-
 drivers/atm/ambassador.c               |    4 +++-
 drivers/infiniband/hw/mthca/mthca_qp.c |    4 ++--
 drivers/mtd/ubi/eba.c                  |    4 ++--
 drivers/net/eepro100.c                 |    7 ++++++-
 drivers/net/natsemi.c                  |    2 +-
 drivers/net/ne2k-pci.c                 |    7 ++++++-
 drivers/net/wan/pc300_drv.c            |    2 ++
 drivers/net/wan/sbni.c                 |    7 +++----
 drivers/telephony/ixj.c                |    7 ++++++-
 drivers/usb/misc/auerswald.c           |    2 +-
 kernel/auditfilter.c                   |   12 ++++--------
 12 files changed, 37 insertions(+), 23 deletions(-)

commit b1734d2388cc45ecdec58615e35955d0d402f938
Author: Jeff Garzik <jeff@garzik.org>
Date:   Tue Jul 17 02:32:21 2007 -0400

    drivers/atm/ambassador: kill uninit'd var warning, and fix bug
    
    An uninitialized variable warning illuminated an area where indeed the
    variable was being used without initialization.  Unfortunately, after
    verifying all such paths were fixed, the warning still appears.  So we
    follow the initialization practice of other variables in this function.
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

commit ea8b4db97aa41a66c05daa4055a1974692ccd52d
Author: Jeff Garzik <jeff@garzik.org>
Date:   Tue Jul 17 02:21:50 2007 -0400

    [libata] sata_mv: use pci_try_set_mwi()
    
    Because sometimes in life, it's ok to fail.
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

commit 9db48926208562df3c778682e064990170ab8971
Author: Jeff Garzik <jeff@garzik.org>
Date:   Tue Jul 17 02:03:49 2007 -0400

    drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning
    
    drivers/infiniband/hw/mthca/mthca_qp.c: In function
      ‘mthca_tavor_post_send’:
    drivers/infiniband/hw/mthca/mthca_qp.c:1594: warning: ‘f0’ may be used
      uninitialized in this function
    drivers/infiniband/hw/mthca/mthca_qp.c: In function
      ‘mthca_arbel_post_send’:
    drivers/infiniband/hw/mthca/mthca_qp.c:1949: warning: ‘f0’ may be used
      uninitialized in this function
    
    Initializing 'f0' is not strictly necessary in either case, AFAICS.
    
    I was considering use of uninitialized_var(), but looking at the
    complex flow of control in each function, I feel it is wiser and
    safer to simply zero the var and be certain of ourselves.
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

commit e5fb4f42268654ca41ab50b1406fb7da97559db5
Author: Jeff Garzik <jeff@garzik.org>
Date:   Tue Jul 17 01:56:32 2007 -0400

    drivers/net/wan/sbni: kill uninit'd var warning
    
    It's actually convenient in the code to initialize this and a sister
    variable to zero.
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

commit 2ab934b8afa89b9b3e71b7fb66470a19772f5012
Author: Jeff Garzik <jeff@garzik.org>
Date:   Tue Jul 17 01:49:56 2007 -0400

    drivers/mtd/ubi/eba: minor cleanup: tighten scope of a local var
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

commit 0d480db85dea59e1393c3968fbdac0117431e797
Author: Jeff Garzik <jeff@garzik.org>
Date:   Tue Jul 17 01:35:08 2007 -0400

    drivers/telephony/ixj: cleanup and fix gcc warning
    
    1) Fix gcc uninit'd var warnings by adding 'default' switch stmt labels
    in two cases.  It was lightning-strikes unlikely that a problem would
    ever arise, but not impossible.
    
    2) Tighten the scope of 'blankword' in two cases.
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

commit 79c63e1976df035dee587c016d79cbccb130494a
Author: Jeff Garzik <jeff@garzik.org>
Date:   Tue Jul 17 01:32:29 2007 -0400

    drivers/net/wan/pc300_drv: fix bug caught by gcc warning
    
    The warning
    
    drivers/net/wan/pc300_drv.c: In function ‘cpc_open’:
    drivers/net/wan/pc300_drv.c:2942: warning: ‘br’ may be used
    uninitialized in this function
    
    was valid.  Ensure 'br' is initialized in all cases.
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

commit ae97fec3701a559929c3529e35417fab133a4d39
Author: Jeff Garzik <jeff@garzik.org>
Date:   Tue Jul 17 01:08:29 2007 -0400

    drivers/usb/misc/auerswald: fix status check, remove redundant check
    
    1) We should only set 'actual_length' output variable if usb length is
    known to be good.
    
    2) No need to check actual_length for NULL.  The only caller always
    passes non-NULL value.
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

commit cad1b9da74f14c5f15b63ffc93c53debe09b3781
Author: Jeff Garzik <jeff@garzik.org>
Date:   Tue Jul 17 00:15:54 2007 -0400

    [netdrvr] eepro100, ne2k-pci: abort resume if pci_enable_device() fails
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

commit f6c4286590e7cb13dd16cb2a6e4dc4a27ce6df1d
Author: Jeff Garzik <jeff@garzik.org>
Date:   Tue Jul 17 00:01:09 2007 -0400

    [netdrvr] natsemi: Fix device removal bug
    
    This episode illustrates how an overused warning can train people to
    ignore that warning, which winds up hiding bugs.
    
    The warning
    
    drivers/net/natsemi.c: In function ‘natsemi_remove1’:
    drivers/net/natsemi.c:3222: warning: ignoring return value of
    ‘device_create_file’, declared with attribute warn_unused_result
    
    is oft-ignored, even though at close inspection one notices this occurs
    in the /remove/ function, not normally where creation occurs.  A quick
    s/create/remove/ and we are fixed, with the warning gone.
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

commit 6f686d3d14621b90f3793b705bdf9fa624fd29ca
Author: Jeff Garzik <jeff@garzik.org>
Date:   Mon Jul 16 21:25:01 2007 -0400

    kernel/auditfilter: kill bogus uninit'd-var compiler warning
    
    Kill this warning...
    
    kernel/auditfilter.c: In function ‘audit_receive_filter’:
    kernel/auditfilter.c:1213: warning: ‘ndw’ may be used uninitialized in this function
    kernel/auditfilter.c:1213: warning: ‘ndp’ may be used uninitialized in this function
    
    ...with a simplification of the code.  audit_put_nd() can accept NULL
    arguments, just like kfree().  It is cleaner to init two existing vars
    to NULL, remove the redundant test variable 'putnd_needed' branches, and call
    audit_put_nd() directly.
    
    As a desired side effect, the warning goes away.
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 5d57643..fb8a749 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -2666,7 +2666,7 @@ static int mv_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	mv_print_info(host);
 
 	pci_set_master(pdev);
-	pci_set_mwi(pdev);
+	pci_try_set_mwi(pdev);
 	return ata_host_activate(host, pdev->irq, mv_interrupt, IRQF_SHARED,
 				 IS_GEN_I(hpriv) ? &mv5_sht : &mv6_sht);
 }
diff --git a/drivers/atm/ambassador.c b/drivers/atm/ambassador.c
index 59651ab..b34b382 100644
--- a/drivers/atm/ambassador.c
+++ b/drivers/atm/ambassador.c
@@ -1040,7 +1040,7 @@ static int amb_open (struct atm_vcc * atm_vcc)
   struct atm_qos * qos;
   struct atm_trafprm * txtp;
   struct atm_trafprm * rxtp;
-  u16 tx_rate_bits;
+  u16 tx_rate_bits = -1; // hush gcc
   u16 tx_vc_bits = -1; // hush gcc
   u16 tx_frame_bits = -1; // hush gcc
   
@@ -1096,6 +1096,8 @@ static int amb_open (struct atm_vcc * atm_vcc)
 	    r = round_up;
 	  }
 	  error = make_rate (pcr, r, &tx_rate_bits, NULL);
+	  if (error)
+	    return error;
 	  tx_vc_bits = TX_UBR_CAPPED;
 	  tx_frame_bits = TX_FRAME_CAPPED;
 	}
diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c b/drivers/infiniband/hw/mthca/mthca_qp.c
index eef415b..11f1d99 100644
--- a/drivers/infiniband/hw/mthca/mthca_qp.c
+++ b/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -1591,7 +1591,7 @@ int mthca_tavor_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 	int i;
 	int size;
 	int size0 = 0;
-	u32 f0;
+	u32 f0 = 0;
 	int ind;
 	u8 op0 = 0;
 
@@ -1946,7 +1946,7 @@ int mthca_arbel_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 	int i;
 	int size;
 	int size0 = 0;
-	u32 f0;
+	u32 f0 = 0;
 	int ind;
 	u8 op0 = 0;
 
diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index 7400294..4dc10c8 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -368,7 +368,7 @@ int ubi_eba_read_leb(struct ubi_device *ubi, int vol_id, int lnum, void *buf,
 	int err, pnum, scrub = 0, idx = vol_id2idx(ubi, vol_id);
 	struct ubi_vid_hdr *vid_hdr;
 	struct ubi_volume *vol = ubi->volumes[idx];
-	uint32_t crc, crc1;
+	uint32_t crc;
 
 	err = leb_read_lock(ubi, vol_id, lnum);
 	if (err)
@@ -451,7 +451,7 @@ retry:
 	}
 
 	if (check) {
-		crc1 = crc32(UBI_CRC32_INIT, buf, len);
+		uint32_t crc1 = crc32(UBI_CRC32_INIT, buf, len);
 		if (crc1 != crc) {
 			ubi_warn("CRC error: calculated %#08x, must be %#08x",
 				 crc1, crc);
diff --git a/drivers/net/eepro100.c b/drivers/net/eepro100.c
index 9afa47e..3c54014 100644
--- a/drivers/net/eepro100.c
+++ b/drivers/net/eepro100.c
@@ -2292,10 +2292,15 @@ static int eepro100_resume(struct pci_dev *pdev)
 	struct net_device *dev = pci_get_drvdata (pdev);
 	struct speedo_private *sp = netdev_priv(dev);
 	void __iomem *ioaddr = sp->regs;
+	int rc;
 
 	pci_set_power_state(pdev, PCI_D0);
 	pci_restore_state(pdev);
-	pci_enable_device(pdev);
+
+	rc = pci_enable_device(pdev);
+	if (rc)
+		return rc;
+
 	pci_set_master(pdev);
 
 	if (!netif_running(dev))
diff --git a/drivers/net/natsemi.c b/drivers/net/natsemi.c
index 3450051..6bb48ba 100644
--- a/drivers/net/natsemi.c
+++ b/drivers/net/natsemi.c
@@ -671,7 +671,7 @@ static ssize_t natsemi_show_##_name(struct device *dev, \
 #define NATSEMI_CREATE_FILE(_dev, _name) \
          device_create_file(&_dev->dev, &dev_attr_##_name)
 #define NATSEMI_REMOVE_FILE(_dev, _name) \
-         device_create_file(&_dev->dev, &dev_attr_##_name)
+         device_remove_file(&_dev->dev, &dev_attr_##_name)
 
 NATSEMI_ATTR(dspcfg_workaround);
 
diff --git a/drivers/net/ne2k-pci.c b/drivers/net/ne2k-pci.c
index 995c0a5..cfdeaf7 100644
--- a/drivers/net/ne2k-pci.c
+++ b/drivers/net/ne2k-pci.c
@@ -669,10 +669,15 @@ static int ne2k_pci_suspend (struct pci_dev *pdev, pm_message_t state)
 static int ne2k_pci_resume (struct pci_dev *pdev)
 {
 	struct net_device *dev = pci_get_drvdata (pdev);
+	int rc;
 
 	pci_set_power_state(pdev, 0);
 	pci_restore_state(pdev);
-	pci_enable_device(pdev);
+
+	rc = pci_enable_device(pdev);
+	if (rc)
+		return rc;
+
 	NS8390_init(dev, 1);
 	netif_device_attach(dev);
 
diff --git a/drivers/net/wan/pc300_drv.c b/drivers/net/wan/pc300_drv.c
index ec1c556..5d8c78e 100644
--- a/drivers/net/wan/pc300_drv.c
+++ b/drivers/net/wan/pc300_drv.c
@@ -2833,6 +2833,8 @@ static int clock_rate_calc(uclong rate, uclong clock, int *br_io)
 	int br, tc;
 	int br_pwr, error;
 
+	*br_io = 0;
+
 	if (rate == 0)
 		return (0);
 
diff --git a/drivers/net/wan/sbni.c b/drivers/net/wan/sbni.c
index 35eded7..1cc18e7 100644
--- a/drivers/net/wan/sbni.c
+++ b/drivers/net/wan/sbni.c
@@ -595,8 +595,8 @@ recv_frame( struct net_device  *dev )
 
 	u32  crc = CRC32_INITIAL;
 
-	unsigned  framelen, frameno, ack;
-	unsigned  is_first, frame_ok;
+	unsigned  framelen = 0, frameno, ack;
+	unsigned  is_first, frame_ok = 0;
 
 	if( check_fhdr( ioaddr, &framelen, &frameno, &ack, &is_first, &crc ) ) {
 		frame_ok = framelen > 4
@@ -604,8 +604,7 @@ recv_frame( struct net_device  *dev )
 			:  skip_tail( ioaddr, framelen, crc );
 		if( frame_ok )
 			interpret_ack( dev, ack );
-	} else
-		frame_ok = 0;
+	}
 
 	outb( inb( ioaddr + CSR0 ) ^ CT_ZER, ioaddr + CSR0 );
 	if( frame_ok ) {
diff --git a/drivers/telephony/ixj.c b/drivers/telephony/ixj.c
index c7b0a35..49cd979 100644
--- a/drivers/telephony/ixj.c
+++ b/drivers/telephony/ixj.c
@@ -3453,7 +3453,6 @@ static void ixj_write_frame(IXJ *j)
 {
 	int cnt, frame_count, dly;
 	IXJ_WORD dat;
-	BYTES blankword;
 
 	frame_count = 0;
 	if(j->flags.cidplay) {
@@ -3501,6 +3500,8 @@ static void ixj_write_frame(IXJ *j)
 		}
 		if (frame_count >= 1) {
 			if (j->ver.low == 0x12 && j->play_mode && j->flags.play_first_frame) {
+				BYTES blankword;
+
 				switch (j->play_mode) {
 				case PLAYBACK_MODE_ULAW:
 				case PLAYBACK_MODE_ALAW:
@@ -3508,6 +3509,7 @@ static void ixj_write_frame(IXJ *j)
 					break;
 				case PLAYBACK_MODE_8LINEAR:
 				case PLAYBACK_MODE_16LINEAR:
+				default:
 					blankword.low = blankword.high = 0x00;
 					break;
 				case PLAYBACK_MODE_8LINEAR_WSS:
@@ -3531,6 +3533,8 @@ static void ixj_write_frame(IXJ *j)
 				j->flags.play_first_frame = 0;
 			} else	if (j->play_codec == G723_63 && j->flags.play_first_frame) {
 				for (cnt = 0; cnt < 24; cnt++) {
+					BYTES blankword;
+
 					if(cnt == 12) {
 						blankword.low = 0x02;
 						blankword.high = 0x00;
@@ -4868,6 +4872,7 @@ static char daa_CR_read(IXJ *j, int cr)
 		bytes.high = 0xB0 + cr;
 		break;
 	case SOP_PU_PULSEDIALING:
+	default:
 		bytes.high = 0xF0 + cr;
 		break;
 	}
diff --git a/drivers/usb/misc/auerswald.c b/drivers/usb/misc/auerswald.c
index 1fd5fc2..3e22b2f 100644
--- a/drivers/usb/misc/auerswald.c
+++ b/drivers/usb/misc/auerswald.c
@@ -630,7 +630,7 @@ static int auerchain_start_wait_urb (pauerchain_t acp, struct urb *urb, int time
 	} else
 		status = urb->status;
 
-	if (actual_length)
+	if (status >= 0)
 		*actual_length = urb->actual_length;
 
   	return status;
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index ce61f42..1bf093d 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1210,8 +1210,8 @@ static inline int audit_add_rule(struct audit_entry *entry,
 	struct audit_entry *e;
 	struct audit_field *inode_f = entry->rule.inode_f;
 	struct audit_watch *watch = entry->rule.watch;
-	struct nameidata *ndp, *ndw;
-	int h, err, putnd_needed = 0;
+	struct nameidata *ndp = NULL, *ndw = NULL;
+	int h, err;
 #ifdef CONFIG_AUDITSYSCALL
 	int dont_count = 0;
 
@@ -1239,7 +1239,6 @@ static inline int audit_add_rule(struct audit_entry *entry,
 		err = audit_get_nd(watch->path, &ndp, &ndw);
 		if (err)
 			goto error;
-		putnd_needed = 1;
 	}
 
 	mutex_lock(&audit_filter_mutex);
@@ -1269,14 +1268,11 @@ static inline int audit_add_rule(struct audit_entry *entry,
 #endif
 	mutex_unlock(&audit_filter_mutex);
 
-	if (putnd_needed)
-		audit_put_nd(ndp, ndw);
-
+	audit_put_nd(ndp, ndw);		/* NULL args OK */
  	return 0;
 
 error:
-	if (putnd_needed)
-		audit_put_nd(ndp, ndw);
+	audit_put_nd(ndp, ndw);		/* NULL args OK */
 	if (watch)
 		audit_put_watch(watch); /* tmp watch, matches initial get */
 	return err;

[git patches 2/2] warnings: use uninitialized_var()

[Jeff Garzik]

For many months, I have maintained a hand-verified list of bogus "may be
used uninitialized" warning fixes, in misc-2.6.git#gccbug.  Andrew urged
me to head these upstream.

I have gone through and re-analyzed each warning, and verified that
these variables are indeed initialized properly, and gcc is making
needless noise.

These uninitialized_var() markers are in a separate branch from the
other warning fixes/silences to enable people to object to this
separately.

Potential upsides of pulling 'uninit-var':

* reduces noise proven to hide bugs
* pushes upstream the work I have done, hand-verifying that each
  warning so marked is indeed bogus.
* uninitialized_var a grep-friendly symbol
* uninitialized_var could be disabled via Kconfig, if someone
  really wanted to see the warnings again

Potential downsides to uninitialized_var():

* Future code changes related to a marked variable could potentially
  result in hiding a valid uninit'd-var warning (i.e. hiding a bug).
* uninitialized_var stands out in the code.  Ugly?  Or a good thing?

This is the second of two git pulls, the -child-.  If you pull branch
'uninit-var', you will receive BOTH 'uninit-var' and 'warnings'
branches.

Please pull from 'uninit-var' branch of
master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git uninit-var

commit 8e1c091cccd551557d24ce845715e8ceb6c49d36
Author: Jeff Garzik <jeff@garzik.org>
Date:   Tue Jul 17 05:40:59 2007 -0400

    arch/i386/* fs/* ipc/*: mark variables with uninitialized_var()
    
    Mark variables with uninitialized_var() if such a warning appears,
    and analysis proves that the var is initialized properly on all paths
    it is used.
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

commit a6343afb6e16b65b9f0b264f94f8207212e7e3ae
Author: Jeff Garzik <jeff@garzik.org>
Date:   Tue Jul 17 05:39:58 2007 -0400

    drivers/*: mark variables with uninitialized_var()
    
    Mark variables in drivers/* with uninitialized_var() if such a warning
    appears, and analysis proves that the var is initialized properly on all
    paths it is used.
    
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

 arch/i386/kernel/efi.c                |    2 +-
 drivers/atm/zatm.c                    |    4 +++-
 drivers/char/cyclades.c               |    4 ++--
 drivers/mtd/ubi/eba.c                 |    2 +-
 drivers/net/r8169.c                   |    2 +-
 drivers/net/tokenring/smctr.c         |    6 ++++--
 drivers/usb/misc/auerswald.c          |    2 +-
 drivers/video/matrox/matroxfb_maven.c |    9 +++++++--
 drivers/video/riva/riva_hw.c          |    7 ++++++-
 fs/ocfs2/file.c                       |    3 ++-
 fs/udf/super.c                        |    2 +-
 ipc/msg.c                             |    4 ++--
 ipc/sem.c                             |    2 +-
 13 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/arch/i386/kernel/efi.c b/arch/i386/kernel/efi.c
index a180802..2452c6f 100644
--- a/arch/i386/kernel/efi.c
+++ b/arch/i386/kernel/efi.c
@@ -278,7 +278,7 @@ void efi_memmap_walk(efi_freemem_callback_t callback, void *arg)
 	struct range {
 		unsigned long start;
 		unsigned long end;
-	} prev, curr;
+	} uninitialized_var(prev), curr;
 	efi_memory_desc_t *md;
 	unsigned long start, end;
 	void *p;
diff --git a/drivers/atm/zatm.c b/drivers/atm/zatm.c
index 020a87a..58583c6 100644
--- a/drivers/atm/zatm.c
+++ b/drivers/atm/zatm.c
@@ -915,7 +915,7 @@ static int open_tx_first(struct atm_vcc *vcc)
 	unsigned long flags;
 	u32 *loop;
 	unsigned short chan;
-	int pcr,unlimited;
+	int unlimited;
 
 	DPRINTK("open_tx_first\n");
 	zatm_dev = ZATM_DEV(vcc->dev);
@@ -936,6 +936,8 @@ static int open_tx_first(struct atm_vcc *vcc)
 	    vcc->qos.txtp.max_pcr >= ATM_OC3_PCR);
 	if (unlimited && zatm_dev->ubr != -1) zatm_vcc->shaper = zatm_dev->ubr;
 	else {
+		int uninitialized_var(pcr);
+
 		if (unlimited) vcc->qos.txtp.max_sdu = ATM_MAX_AAL5_PDU;
 		if ((zatm_vcc->shaper = alloc_shaper(vcc->dev,&pcr,
 		    vcc->qos.txtp.min_pcr,vcc->qos.txtp.max_pcr,unlimited))
diff --git a/drivers/char/cyclades.c b/drivers/char/cyclades.c
index 7b08394..9e0adfe 100644
--- a/drivers/char/cyclades.c
+++ b/drivers/char/cyclades.c
@@ -4466,10 +4466,10 @@ static void cy_hangup(struct tty_struct *tty)
 static int __devinit cy_init_card(struct cyclades_card *cinfo)
 {
 	struct cyclades_port *info;
-	u32 mailbox;
+	u32 uninitialized_var(mailbox);
 	unsigned int nports;
 	unsigned short chip_number;
-	int index, port;
+	int uninitialized_var(index), port;
 
 	spin_lock_init(&cinfo->card_lock);
 
diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index 4dc10c8..7c6b223 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -368,7 +368,7 @@ int ubi_eba_read_leb(struct ubi_device *ubi, int vol_id, int lnum, void *buf,
 	int err, pnum, scrub = 0, idx = vol_id2idx(ubi, vol_id);
 	struct ubi_vid_hdr *vid_hdr;
 	struct ubi_volume *vol = ubi->volumes[idx];
-	uint32_t crc;
+	uint32_t uninitialized_var(crc);
 
 	err = leb_read_lock(ubi, vol_id, lnum);
 	if (err)
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 982a901..bb6896a 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -2338,7 +2338,7 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
 {
 	struct skb_shared_info *info = skb_shinfo(skb);
 	unsigned int cur_frag, entry;
-	struct TxDesc *txd;
+	struct TxDesc * uninitialized_var(txd);
 
 	entry = tp->cur_tx;
 	for (cur_frag = 0; cur_frag < info->nr_frags; cur_frag++) {
diff --git a/drivers/net/tokenring/smctr.c b/drivers/net/tokenring/smctr.c
index 58d7e5d..f83bb5c 100644
--- a/drivers/net/tokenring/smctr.c
+++ b/drivers/net/tokenring/smctr.c
@@ -3692,7 +3692,6 @@ static int smctr_process_rx_packet(MAC_HEADER *rmf, __u16 size,
         __u16 rcode, correlator;
         int err = 0;
         __u8 xframe = 1;
-        __u16 tx_fstatus;
 
         rmf->vl = SWAP_BYTES(rmf->vl);
         if(rx_status & FCB_RX_STATUS_DA_MATCHED)
@@ -3783,7 +3782,9 @@ static int smctr_process_rx_packet(MAC_HEADER *rmf, __u16 size,
                                 }
                                 break;
 
-                        case TX_FORWARD:
+                        case TX_FORWARD: {
+        			__u16 uninitialized_var(tx_fstatus);
+
                                 if((rcode = smctr_rcv_tx_forward(dev, rmf))
                                         != POSITIVE_ACK)
                                 {
@@ -3811,6 +3812,7 @@ static int smctr_process_rx_packet(MAC_HEADER *rmf, __u16 size,
                                         }
                                 }
                                 break;
+			}
 
                         /* Received MAC Frames Processed by CRS/REM/RPS. */
                         case RSP:
diff --git a/drivers/usb/misc/auerswald.c b/drivers/usb/misc/auerswald.c
index 3e22b2f..42d4e64 100644
--- a/drivers/usb/misc/auerswald.c
+++ b/drivers/usb/misc/auerswald.c
@@ -664,7 +664,7 @@ static int auerchain_control_msg (pauerchain_t acp, struct usb_device *dev, unsi
 	int ret;
 	struct usb_ctrlrequest *dr;
 	struct urb *urb;
-        int length;
+        int uninitialized_var(length);
 
         dbg ("auerchain_control_msg");
         dr = kmalloc (sizeof (struct usb_ctrlrequest), GFP_KERNEL);
diff --git a/drivers/video/matrox/matroxfb_maven.c b/drivers/video/matrox/matroxfb_maven.c
index 5d29a26..de0d755 100644
--- a/drivers/video/matrox/matroxfb_maven.c
+++ b/drivers/video/matrox/matroxfb_maven.c
@@ -273,8 +273,11 @@ static int matroxfb_PLL_mavenclock(const struct matrox_pll_features2* pll,
 			}
 		}
 	}
+
+	/* if h2/post/in/feed have not been assigned, return zero (error) */
 	if (besth2 < 2)
 		return 0;
+
 	dprintk(KERN_ERR "clk: %02X %02X %02X %d %d\n", *in, *feed, *post, fxtal, fwant);
 	return fxtal * (*feed) / (*in) * ctl->den;
 }
@@ -284,7 +287,7 @@ static unsigned int matroxfb_mavenclock(const struct matrox_pll_ctl* ctl,
 		unsigned int* in, unsigned int* feed, unsigned int* post,
 		unsigned int* htotal2) {
 	unsigned int fvco;
-	unsigned int p;
+	unsigned int uninitialized_var(p);
 
 	fvco = matroxfb_PLL_mavenclock(&maven1000_pll, ctl, htotal, vtotal, in, feed, &p, htotal2);
 	if (!fvco)
@@ -715,7 +718,9 @@ static int maven_find_exact_clocks(unsigned int ht, unsigned int vt,
 	m->regs[0x82] = 0x81;
 
 	for (x = 0; x < 8; x++) {
-		unsigned int a, b, c, h2;
+		unsigned int c;
+		unsigned int uninitialized_var(a), uninitialized_var(b),
+			     uninitialized_var(h2);
 		unsigned int h = ht + 2 + x;
 
 		if (!matroxfb_mavenclock((m->mode == MATROXFB_OUTPUT_MODE_PAL) ? &maven_PAL : &maven_NTSC, h, vt, &a, &b, &c, &h2)) {
diff --git a/drivers/video/riva/riva_hw.c b/drivers/video/riva/riva_hw.c
index 70bfd78..1330770 100644
--- a/drivers/video/riva/riva_hw.c
+++ b/drivers/video/riva/riva_hw.c
@@ -1223,6 +1223,8 @@ static int CalcVClock
         }
     }
     }
+
+    /* non-zero: M/N/P/clock values assigned.  zero: error (not set) */
     return (DeltaOld != 0xFFFFFFFF);
 }
 /*
@@ -1240,7 +1242,10 @@ int CalcStateExt
     int            dotClock
 )
 {
-    int pixelDepth, VClk, m, n, p;
+    int pixelDepth;
+    int uninitialized_var(VClk),uninitialized_var(m),
+        uninitialized_var(n),	uninitialized_var(p);
+
     /*
      * Save mode parameters.
      */
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index f04c7aa..004c2ab 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -1867,7 +1867,8 @@ static ssize_t ocfs2_file_buffered_write(struct file *file, loff_t *ppos,
 	loff_t pos;
 	const struct iovec *cur_iov = iov;
 	struct page *user_page, *page;
-	char *buf, *dst;
+	char * uninitialized_var(buf);
+	char *dst;
 	void *fsdata;
 
 	/*
diff --git a/fs/udf/super.c b/fs/udf/super.c
index 6658afb..d6a504f 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -1356,7 +1356,7 @@ udf_load_partition(struct super_block *sb, kernel_lb_addr *fileset)
 			case UDF_VIRTUAL_MAP15:
 			case UDF_VIRTUAL_MAP20:
 			{
-				kernel_lb_addr ino;
+				kernel_lb_addr uninitialized_var(ino);
 
 				if (!UDF_SB_LASTBLOCK(sb))
 				{
diff --git a/ipc/msg.c b/ipc/msg.c
index cbd27e5..a03fcb5 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -385,7 +385,7 @@ copy_msqid_from_user(struct msq_setbuf *out, void __user *buf, int version)
 asmlinkage long sys_msgctl(int msqid, int cmd, struct msqid_ds __user *buf)
 {
 	struct kern_ipc_perm *ipcp;
-	struct msq_setbuf setbuf;
+	struct msq_setbuf uninitialized_var(setbuf);
 	struct msg_queue *msq;
 	int err, version;
 	struct ipc_namespace *ns;
@@ -509,7 +509,7 @@ asmlinkage long sys_msgctl(int msqid, int cmd, struct msqid_ds __user *buf)
 	err = audit_ipc_obj(ipcp);
 	if (err)
 		goto out_unlock_up;
-	if (cmd==IPC_SET) {
+	if (cmd == IPC_SET) {
 		err = audit_ipc_set_perm(setbuf.qbytes, setbuf.uid, setbuf.gid,
 					 setbuf.mode);
 		if (err)
diff --git a/ipc/sem.c b/ipc/sem.c
index 89bfdff..b676fef 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -856,7 +856,7 @@ static int semctl_down(struct ipc_namespace *ns, int semid, int semnum,
 {
 	struct sem_array *sma;
 	int err;
-	struct sem_setbuf setbuf;
+	struct sem_setbuf uninitialized_var(setbuf);
 	struct kern_ipc_perm *ipcp;
 
 	if(cmd == IPC_SET) {

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Roland Dreier]

 >     drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning
 >     
 >     drivers/infiniband/hw/mthca/mthca_qp.c: In function
 >       ‘mthca_tavor_post_send’:
 >     drivers/infiniband/hw/mthca/mthca_qp.c:1594: warning: ‘f0’ may be used
 >       uninitialized in this function
 >     drivers/infiniband/hw/mthca/mthca_qp.c: In function
 >       ‘mthca_arbel_post_send’:
 >     drivers/infiniband/hw/mthca/mthca_qp.c:1949: warning: ‘f0’ may be used
 >       uninitialized in this function
 >     
 >     Initializing 'f0' is not strictly necessary in either case, AFAICS.
 >     
 >     I was considering use of uninitialized_var(), but looking at the
 >     complex flow of control in each function, I feel it is wiser and
 >     safer to simply zero the var and be certain of ourselves.
 >     
 >     Signed-off-by: Jeff Garzik <jeff@garzik.org>

I don't really like this.  These functions are in the hottest, most
latency-sensitive code path of this driver, which is used by people
who care about nanoseconds.  I'm quite confident that the code is
correct as written, and it really feels wrong to me to add bloat to
the fastpath just to cover up a shortcoming of gcc.

And I don't like using uninitialized_var() here for a similar
reason... the functions are complex and I would prefer to avoid hiding
future bugs that may creep in.  In fact adding the initialization to 0
has a similar effect, since it shuts up the compiler even if the logic
in the function gets screwed up.

On the other hand I definitely sympathize with the desire to have a
warning-free build so that real warnings are more visible.  I guess I
could live with the uninitialized_var() patch, although I would feel a
little sad.

 - R.

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Jeff Garzik]

Roland Dreier wrote:
>  >     drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning
>  >     
>  >     drivers/infiniband/hw/mthca/mthca_qp.c: In function
>  >       ‘mthca_tavor_post_send’:
>  >     drivers/infiniband/hw/mthca/mthca_qp.c:1594: warning: ‘f0’ may be used
>  >       uninitialized in this function
>  >     drivers/infiniband/hw/mthca/mthca_qp.c: In function
>  >       ‘mthca_arbel_post_send’:
>  >     drivers/infiniband/hw/mthca/mthca_qp.c:1949: warning: ‘f0’ may be used
>  >       uninitialized in this function
>  >     
>  >     Initializing 'f0' is not strictly necessary in either case, AFAICS.
>  >     
>  >     I was considering use of uninitialized_var(), but looking at the
>  >     complex flow of control in each function, I feel it is wiser and
>  >     safer to simply zero the var and be certain of ourselves.
>  >     
>  >     Signed-off-by: Jeff Garzik <jeff@garzik.org>
> 
> I don't really like this.  These functions are in the hottest, most
> latency-sensitive code path of this driver, which is used by people
> who care about nanoseconds.  I'm quite confident that the code is
> correct as written, and it really feels wrong to me to add bloat to
> the fastpath just to cover up a shortcoming of gcc.

I don't buy that performance argument, in this case.  You are already 
dirtying the same cacheline with other variable initializations.

Like I noted in the changeset description (hey, this is precisely why I 
included it, so that we could have this discussion), IMO the flow of 
control makes it not only impossible for the compiler to understand the 
full value range of 'f0', but also difficult for humans as well.

I could perhaps understand initializing the variable to some poison 
value rather than zero, but IMO the code is stronger with f0 set to a 
sane value.

It's poorly readable, poorly commented code as-is.

	Jeff

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Jeff Garzik]

Jeff Garzik wrote:
> I don't buy that performance argument, in this case.  You are already 
> dirtying the same cacheline with other variable initializations.

Or simply sitting in a CPU register for large stretches of function 
runtime...

	Jeff

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Andrew Morton]

On Tue, 17 Jul 2007 14:53:02 -0700
Roland Dreier <rdreier@cisco.com> wrote:

> >     drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning
> >     
> >     drivers/infiniband/hw/mthca/mthca_qp.c: In function
> >       ______mthca_tavor_post_send______:
> >     drivers/infiniband/hw/mthca/mthca_qp.c:1594: warning: ______f0______ may be used
> >       uninitialized in this function

(rofl, look at that mess: it was utterly impractical, unrealistic and
stupid for gcc to go and UTFify the compiler output.  Sigh.  LANG=C, guys).

> And I don't like using uninitialized_var() here for a similar
> reason... the functions are complex and I would prefer to avoid hiding
> future bugs that may creep in. 

umm, the code *already* produces a warning.  So if you later add a
real used-uninitialised bug, you won't know about it, because everyone
was trained to ignore the warning anyway.

Best would be to find some way to restructure the code to make the warning
go away.

Meanwhile I'd say switch this to uninitialized_var() if it's that much of a
worry.

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Linus Torvalds]

On Tue, 17 Jul 2007, Andrew Morton wrote:
> 
> (rofl, look at that mess: it was utterly impractical, unrealistic and
> stupid for gcc to go and UTFify the compiler output.  Sigh.  LANG=C, guys).

Yeah, I absolutely *detest* how gcc does idiotic quoting just because you 
happen to be in a UTF-8 locale. There's no reason for it what-so-ever, and 
I don't understand the mindset of whoever did that crap.

They should use a nice ASCII tick-mark ('), not some fancy quotation 
marks.

		Linus

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Roland Dreier]

 > I don't buy that performance argument, in this case.  You are already
 > dirtying the same cacheline with other variable initializations.
 > 
 > Like I noted in the changeset description (hey, this is precisely why
 > I included it, so that we could have this discussion), IMO the flow of
 > control makes it not only impossible for the compiler to understand
 > the full value range of 'f0', but also difficult for humans as well.
 > 
 > I could perhaps understand initializing the variable to some poison
 > value rather than zero, but IMO the code is stronger with f0 set to a
 > sane value.

The more I think about it, the less sense initializing f0 to 0 makes.
The whole problem with an uninitialized variable is that a random
value from the stack might be used.  So setting a variable to
something meaningless (guaranteeing that a garbage value is used in
case of a bug) just to shut up a warning makes no sense -- it's no
safer than leaving the code as is.  uninitialized_var() gets rid of
the warning, saves a little text and instruction cache, and documents
things better.

(BTW, I agree the code is a little confusing as written.  I think
things could be cleaned up and made more efficient by getting rid of
the initialization of size0 too -- I'll look at doing that)

Anyway, I queued this up for my next merge with Linus:

commit 6d7d080e9f7cd535a8821efd3835c5cfa5223ab6
Author: Roland Dreier <rolandd@cisco.com>
Date:   Tue Jul 17 19:30:51 2007 -0700

    IB/mthca: Use uninitialized_var() for f0
    
    Commit 9db48926 ("drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd
    var warning") added "= 0" to the declarations of f0 to shut up gcc
    warnings.  However, there's no point in making the code bigger by
    initializing f0 to a random value just to get rid of a warning;
    setting f0 to 0 is no safer than just using uninitialized_var(), which
    documents the situation better and gives smaller code too.  For example,
    on x86_64:
    
    add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-16 (-16)
    function                                     old     new   delta
    mthca_tavor_post_send                       1352    1344      -8
    mthca_arbel_post_send                       1489    1481      -8
    
    Signed-off-by: Roland Dreier <rolandd@cisco.com>

diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c b/drivers/infiniband/hw/mthca/mthca_qp.c
index 11f1d99..0e9ef24 100644
--- a/drivers/infiniband/hw/mthca/mthca_qp.c
+++ b/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -1591,7 +1591,13 @@ int mthca_tavor_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 	int i;
 	int size;
 	int size0 = 0;
-	u32 f0 = 0;
+	/*
+	 * f0 is only used if nreq != 0, and f0 will be initialized
+	 * the first time through the main loop, since size0 == 0 the
+	 * first time through.  So nreq cannot become non-zero without
+	 * initializing f0, and f0 is in fact never used uninitialized.
+	 */
+	u32 uninitialized_var(f0);
 	int ind;
 	u8 op0 = 0;
 
@@ -1946,7 +1952,13 @@ int mthca_arbel_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 	int i;
 	int size;
 	int size0 = 0;
-	u32 f0 = 0;
+	/*
+	 * f0 is only used if nreq != 0, and f0 will be initialized
+	 * the first time through the main loop, since size0 == 0 the
+	 * first time through.  So nreq cannot become non-zero without
+	 * initializing f0, and f0 is in fact never used uninitialized.
+	 */
+	u32 uninitialized_var(f0);
 	int ind;
 	u8 op0 = 0;
 

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Roland Dreier]

I think this patch (on top of the previous one) actually makes the
code clearer, and also makes it smaller:

add/remove: 0/0 grow/shrink: 0/3 up/down: 0/-41 (-41)
function                                     old     new   delta
mthca_tavor_post_send                       1344    1335      -9
mthca_tavor_post_receive                     792     776     -16
mthca_arbel_post_send                       1481    1465     -16

So I'll test this a bit and probably merge it.
---
diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c b/drivers/infiniband/hw/mthca/mthca_qp.c
index 0e9ef24..186bade 100644
--- a/drivers/infiniband/hw/mthca/mthca_qp.c
+++ b/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -1590,13 +1590,14 @@ int mthca_tavor_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 	int nreq;
 	int i;
 	int size;
-	int size0 = 0;
 	/*
-	 * f0 is only used if nreq != 0, and f0 will be initialized
-	 * the first time through the main loop, since size0 == 0 the
-	 * first time through.  So nreq cannot become non-zero without
-	 * initializing f0, and f0 is in fact never used uninitialized.
+	 * f0 and size0 are only used if nreq != 0, and they will
+	 * always be initialized the first time through the main loop
+	 * before nreq is incremented.  So nreq cannot become non-zero
+	 * without initializing f0 and size0, and they are in fact
+	 * never used uninitialized.
 	 */
+	int uninitialized_var(size0);
 	u32 uninitialized_var(f0);
 	int ind;
 	u8 op0 = 0;
@@ -1774,11 +1775,11 @@ int mthca_tavor_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 				    mthca_opcode[wr->opcode]);
 		wmb();
 		((struct mthca_next_seg *) prev_wqe)->ee_nds =
-			cpu_to_be32((size0 ? 0 : MTHCA_NEXT_DBD) | size |
+			cpu_to_be32((nreq ? 0 : MTHCA_NEXT_DBD) | size |
 				    ((wr->send_flags & IB_SEND_FENCE) ?
 				    MTHCA_NEXT_FENCE : 0));
 
-		if (!size0) {
+		if (!nreq) {
 			size0 = size;
 			op0   = mthca_opcode[wr->opcode];
 			f0    = wr->send_flags & IB_SEND_FENCE ?
@@ -1828,7 +1829,14 @@ int mthca_tavor_post_receive(struct ib_qp *ibqp, struct ib_recv_wr *wr,
 	int nreq;
 	int i;
 	int size;
-	int size0 = 0;
+	/*
+	 * size0 is only used if nreq != 0, and it will always be
+	 * initialized the first time through the main loop before
+	 * nreq is incremented.  So nreq cannot become non-zero
+	 * without initializing size0, and it is in fact never used
+	 * uninitialized.
+	 */
+	int uninitialized_var(size0);
 	int ind;
 	void *wqe;
 	void *prev_wqe;
@@ -1887,7 +1895,7 @@ int mthca_tavor_post_receive(struct ib_qp *ibqp, struct ib_recv_wr *wr,
 		((struct mthca_next_seg *) prev_wqe)->ee_nds =
 			cpu_to_be32(MTHCA_NEXT_DBD | size);
 
-		if (!size0)
+		if (!nreq)
 			size0 = size;
 
 		++ind;
@@ -1909,7 +1917,6 @@ int mthca_tavor_post_receive(struct ib_qp *ibqp, struct ib_recv_wr *wr,
 
 			qp->rq.next_ind = ind;
 			qp->rq.head += MTHCA_TAVOR_MAX_WQES_PER_RECV_DB;
-			size0 = 0;
 		}
 	}
 
@@ -1951,13 +1958,14 @@ int mthca_arbel_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 	int nreq;
 	int i;
 	int size;
-	int size0 = 0;
 	/*
-	 * f0 is only used if nreq != 0, and f0 will be initialized
-	 * the first time through the main loop, since size0 == 0 the
-	 * first time through.  So nreq cannot become non-zero without
-	 * initializing f0, and f0 is in fact never used uninitialized.
+	 * f0 and size0 are only used if nreq != 0, and they will
+	 * always be initialized the first time through the main loop
+	 * before nreq is incremented.  So nreq cannot become non-zero
+	 * without initializing f0 and size0, and they are in fact
+	 * never used uninitialized.
 	 */
+	int uninitialized_var(size0);
 	u32 uninitialized_var(f0);
 	int ind;
 	u8 op0 = 0;
@@ -1978,7 +1986,6 @@ int mthca_arbel_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 			doorbell[1] = cpu_to_be32((qp->qpn << 8) | size0);
 
 			qp->sq.head += MTHCA_ARBEL_MAX_WQES_PER_SEND_DB;
-			size0 = 0;
 
 			/*
 			 * Make sure that descriptors are written before
@@ -2163,7 +2170,7 @@ int mthca_arbel_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 				    ((wr->send_flags & IB_SEND_FENCE) ?
 				     MTHCA_NEXT_FENCE : 0));
 
-		if (!size0) {
+		if (!nreq) {
 			size0 = size;
 			op0   = mthca_opcode[wr->opcode];
 			f0    = wr->send_flags & IB_SEND_FENCE ?

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Greg KH]

On Tue, Jul 17, 2007 at 05:42:39PM -0400, Jeff Garzik wrote:
> commit ae97fec3701a559929c3529e35417fab133a4d39
> Author: Jeff Garzik <jeff@garzik.org>
> Date:   Tue Jul 17 01:08:29 2007 -0400
> 
>     drivers/usb/misc/auerswald: fix status check, remove redundant check
>     
>     1) We should only set 'actual_length' output variable if usb length is
>     known to be good.
>     
>     2) No need to check actual_length for NULL.  The only caller always
>     passes non-NULL value.
>     
>     Signed-off-by: Jeff Garzik <jeff@garzik.org>

I have no objection to this patch at all, however it does not remove the
compiler warning :(

thanks,

greg k-h

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Linus Torvalds]

On Tue, 17 Jul 2007, Roland Dreier wrote:
>
> So setting a variable to something meaningless (guaranteeing that a 
> garbage value is used in case of a bug) just to shut up a warning makes 
> no sense -- it's no safer than leaving the code as is.  

Wrong.

It's safer for two reasons:
 - now everybody will see the *same* behaviour
 - the "meaningless value" is guaranteed to not be a security leak

but the whole "shut up bogus warnings" is the best reason.

So it *is* safer than leaving the code as-is.

Of course, usually the best approach is to rewrite the code to be simpler, 
so that even gcc sees that something is obviously initialized. Sadly, 
people seldom do the right thing, and sometimes gcc just blows incredibly 
hard.

		Linus

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Roland Dreier]

 > > So setting a variable to something meaningless (guaranteeing that a 
 > > garbage value is used in case of a bug) just to shut up a warning makes 
 > > no sense -- it's no safer than leaving the code as is.  
 > 
 > Wrong.
 > 
 > It's safer for two reasons:
 >  - now everybody will see the *same* behaviour
 >  - the "meaningless value" is guaranteed to not be a security leak
 > 
 > but the whole "shut up bogus warnings" is the best reason.
 > 
 > So it *is* safer than leaving the code as-is.

OK, fair enough.  What I said wasn't quite right, but in my case I
think neither of your reasons really applies, since the uninitialized
variable would be written into some hardware control block, so the
effect would probably still be random even if the value is the same
and the information leak doesn't really matter.

Anyway, I think that in this case it's not too hard to show that the
variable really can't be used uninitialized, so I prefer the smaller
generated code from uninitialized_var() (plus a comment explaining why
that's safe).

 > Of course, usually the best approach is to rewrite the code to be simpler, 
 > so that even gcc sees that something is obviously initialized. Sadly, 
 > people seldom do the right thing, and sometimes gcc just blows incredibly 
 > hard.

In this case the code is basically

	u32 x;

	for (n = 0; cond; ++n) {
		...
		if (!n)
			x = something;
		...
	}

	if (n) {
		...
		use(x);
		...
	}

and gcc still warns...

 - R.

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Jeff Garzik]

Roland Dreier wrote:
> In this case the code is basically
> 
> 	u32 x;
> 
> 	for (n = 0; cond; ++n) {
> 		...
> 		if (!n)
> 			x = something;
> 		...
> 	}
> 
> 	if (n) {
> 		...
> 		use(x);
> 		...
> 	}
> 
> and gcc still warns...


Interestingly, the above accurately describes a common code pattern 
matching code which caused gcc to emit the uninit'd-var warnings.

For the record I think initializating 'f0' to zero is safer for the 
reasons Linus gave, and in addition, f0 is or'd with a value written to 
a hardware register, which means things should go awry (if they go) in a 
semi-predictable manner.

According to the assembly language produced, sure it is larger -- by one 
(per function) MOV that is adjacent to other initializations, making it 
highly likely the initializations are all streamed together.  I doubt 
one MOV per function will make a huge difference, considering the peace 
of mind it buys.

	Jeff

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Linus Torvalds]

On Tue, 17 Jul 2007, Roland Dreier wrote:
>
> I think this patch (on top of the previous one) actually makes the
> code clearer

Quite frankly, calling this "making the code clearer" is a bit ridiculous.

That code still is absolute *crap* from a readability angle. It doesn't 
follow any sane coding standards, and certainly not the most important 
ones ("keep the function small", "don't have tons of local variables"), 
and it has absolutely ridiculously ugly casts that get repeated over and 
over and over again.

Quite frankly, I don't quite understand where you get those enormous balls 
you have, that you can then talk about how ugly it is to just add a "= 0" 
that shuts up a compiler warning. That's the _least_ ugly part of the 
whole damn function!

So rather than sending out that idiotic patch, look at that code for five 
seconds, and ponder whether it really needs to be that ugly.

Here's a few things that you could *really* do to make it somewhat more 
readable:

 - make that whole "switch()" statement from hell another function 
   entirely, and have it return the size of the thing, so that you don't 
   need to have

	wqe += xxx
	size += xxx / 16;

   repeated fifty times (and so that it's also obvious that the xxxx 
   always matches).

 - make each switch case actually call a small function with the argument 
   cast to the right pointer type, so that you need *one* cast per case, 
   rather than a handful.

End result? More readable source code, with functions that are 20 lines 
long (or less), rather than 200 lines of spagetti-coding.

And you know what? That's actually more important than 16 bytes of object 
code, although looking at the size of the infiniband code, I *seriously* 
doubt any infiniband person has ever cared about object code size in their 
life. That thing is not for weak machines or stomachs.

The warnign (and fixing it up) is the _least_ of the problems in that 
code, methinks.

Anyway, here's a totally untested cleanup that compiles but probably 
doesn't work, because I didn't check that I did the right thing with all 
the pointer arithmetic (ie when I change "wqe" to a real structure pointer 
instead of just a "void *", maybe I left some pointer arithmetic around 
that expected it to work as a byte pointer, but now really works on the 
whole structure size instead).

So this patch is NOT meant to be applied, but it is meant to teach people 
how things like this should be done. They should *not* be one big function 
with lots of case statements. They should be lots of small functions!

		Linus
---
 drivers/infiniband/hw/mthca/mthca_qp.c |  236 ++++++++++++++++---------------
 1 files changed, 122 insertions(+), 114 deletions(-)

diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c b/drivers/infiniband/hw/mthca/mthca_qp.c
index 11f1d99..74da9bc 100644
--- a/drivers/infiniband/hw/mthca/mthca_qp.c
+++ b/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -1578,6 +1578,113 @@ static inline int mthca_wq_overflow(struct mthca_wq *wq, int nreq,
 	return cur + nreq >= wq->max;
 }
 
+static int handle_next_seg(struct ib_send_wr *wr, struct mthca_next_seg * wqe)
+{
+	wqe->nda_op = 0;
+	wqe->ee_nds = 0;
+	wqe->flags =	((wr->send_flags & IB_SEND_SIGNALED) ?
+			 cpu_to_be32(MTHCA_NEXT_CQ_UPDATE) : 0) |
+			((wr->send_flags & IB_SEND_SOLICITED) ?
+			 cpu_to_be32(MTHCA_NEXT_SOLICIT) : 0)   |
+			cpu_to_be32(1);
+
+	if (wr->opcode == IB_WR_SEND_WITH_IMM ||
+	    wr->opcode == IB_WR_RDMA_WRITE_WITH_IMM)
+		wqe->imm = wr->imm_data;
+
+	return sizeof(struct mthca_next_seg);
+}
+
+static int handle_raddr_seg(struct mthca_dev *dev, struct mthca_qp *qp, struct ib_send_wr *wr,
+	struct mthca_raddr_seg *wqe, int ind)
+{
+	switch (qp->transport) {
+	case RC:
+		switch (wr->opcode) {
+		case IB_WR_ATOMIC_CMP_AND_SWP:
+		case IB_WR_ATOMIC_FETCH_AND_ADD: {
+			struct mthca_atomic_seg *atomic;
+
+			wqe->raddr = cpu_to_be64(wr->wr.atomic.remote_addr);
+			wqe->rkey = cpu_to_be32(wr->wr.atomic.rkey);
+			wqe->reserved = 0;
+
+			atomic = (struct mthca_atomic_seg *) (wqe+1);
+
+			if (wr->opcode == IB_WR_ATOMIC_CMP_AND_SWP) {
+				atomic->swap_add = cpu_to_be64(wr->wr.atomic.swap);
+				atomic->compare = cpu_to_be64(wr->wr.atomic.compare_add);
+			} else {
+				atomic->swap_add = cpu_to_be64(wr->wr.atomic.compare_add);
+				atomic->compare = 0;
+			}
+
+			return sizeof (struct mthca_raddr_seg) +
+				sizeof (struct mthca_atomic_seg);
+		}
+
+		case IB_WR_RDMA_WRITE:
+		case IB_WR_RDMA_WRITE_WITH_IMM:
+		case IB_WR_RDMA_READ:
+			wqe->raddr = cpu_to_be64(wr->wr.rdma.remote_addr);
+			wqe->rkey = cpu_to_be32(wr->wr.rdma.rkey);
+			wqe->reserved = 0;
+			return sizeof (struct mthca_raddr_seg);
+
+		default:
+			/* No extra segments required for sends */
+			break;
+		}
+		return 0;
+
+	case UC:
+		switch (wr->opcode) {
+		case IB_WR_RDMA_WRITE:
+		case IB_WR_RDMA_WRITE_WITH_IMM:
+			wqe->raddr = cpu_to_be64(wr->wr.rdma.remote_addr);
+			wqe->rkey = cpu_to_be32(wr->wr.rdma.rkey);
+			wqe->reserved = 0;
+			return sizeof (struct mthca_raddr_seg);
+
+		default:
+			/* No extra segments required for sends */
+			break;
+		}
+
+		break;
+
+	case UD: {
+		struct mthca_tavor_ud_seg *ud = (void *)wqe;
+
+		ud->lkey = cpu_to_be32(to_mah(wr->wr.ud.ah)->key);
+		ud->av_addr = cpu_to_be64(to_mah(wr->wr.ud.ah)->avdma);
+		ud->dqpn = cpu_to_be32(wr->wr.ud.remote_qpn);
+		ud->qkey = cpu_to_be32(wr->wr.ud.remote_qkey);
+
+		return sizeof (struct mthca_tavor_ud_seg);
+	}
+
+	case MLX: {
+		int err = build_mlx_header(dev, to_msqp(qp), ind, wr,
+				       (void *) wqe - sizeof (struct mthca_next_seg),
+				       (void *) wqe);
+		if (err)
+			return err;
+
+		return sizeof (struct mthca_data_seg);
+	}
+	}
+	return 0;
+}
+
+static int handle_data_seg(struct mthca_data_seg *wqe, struct ib_sge *sge)
+{
+	wqe->byte_count = cpu_to_be32(sge->length);
+	wqe->lkey = cpu_to_be32(sge->lkey);
+	wqe->addr = cpu_to_be64(sge->addr);
+	return sizeof(struct mthca_data_seg);
+}
+
 int mthca_tavor_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 			  struct ib_send_wr **bad_wr)
 {
@@ -1616,115 +1723,18 @@ int mthca_tavor_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 		prev_wqe = qp->sq.last;
 		qp->sq.last = wqe;
 
-		((struct mthca_next_seg *) wqe)->nda_op = 0;
-		((struct mthca_next_seg *) wqe)->ee_nds = 0;
-		((struct mthca_next_seg *) wqe)->flags =
-			((wr->send_flags & IB_SEND_SIGNALED) ?
-			 cpu_to_be32(MTHCA_NEXT_CQ_UPDATE) : 0) |
-			((wr->send_flags & IB_SEND_SOLICITED) ?
-			 cpu_to_be32(MTHCA_NEXT_SOLICIT) : 0)   |
-			cpu_to_be32(1);
-		if (wr->opcode == IB_WR_SEND_WITH_IMM ||
-		    wr->opcode == IB_WR_RDMA_WRITE_WITH_IMM)
-			((struct mthca_next_seg *) wqe)->imm = wr->imm_data;
+		err = handle_next_seg(wr, (struct mthca_next_seg *) wqe);
 
-		wqe += sizeof (struct mthca_next_seg);
-		size = sizeof (struct mthca_next_seg) / 16;
+		wqe += err;
+		size = err / 16;
 
-		switch (qp->transport) {
-		case RC:
-			switch (wr->opcode) {
-			case IB_WR_ATOMIC_CMP_AND_SWP:
-			case IB_WR_ATOMIC_FETCH_AND_ADD:
-				((struct mthca_raddr_seg *) wqe)->raddr =
-					cpu_to_be64(wr->wr.atomic.remote_addr);
-				((struct mthca_raddr_seg *) wqe)->rkey =
-					cpu_to_be32(wr->wr.atomic.rkey);
-				((struct mthca_raddr_seg *) wqe)->reserved = 0;
-
-				wqe += sizeof (struct mthca_raddr_seg);
-
-				if (wr->opcode == IB_WR_ATOMIC_CMP_AND_SWP) {
-					((struct mthca_atomic_seg *) wqe)->swap_add =
-						cpu_to_be64(wr->wr.atomic.swap);
-					((struct mthca_atomic_seg *) wqe)->compare =
-						cpu_to_be64(wr->wr.atomic.compare_add);
-				} else {
-					((struct mthca_atomic_seg *) wqe)->swap_add =
-						cpu_to_be64(wr->wr.atomic.compare_add);
-					((struct mthca_atomic_seg *) wqe)->compare = 0;
-				}
-
-				wqe += sizeof (struct mthca_atomic_seg);
-				size += (sizeof (struct mthca_raddr_seg) +
-					 sizeof (struct mthca_atomic_seg)) / 16;
-				break;
-
-			case IB_WR_RDMA_WRITE:
-			case IB_WR_RDMA_WRITE_WITH_IMM:
-			case IB_WR_RDMA_READ:
-				((struct mthca_raddr_seg *) wqe)->raddr =
-					cpu_to_be64(wr->wr.rdma.remote_addr);
-				((struct mthca_raddr_seg *) wqe)->rkey =
-					cpu_to_be32(wr->wr.rdma.rkey);
-				((struct mthca_raddr_seg *) wqe)->reserved = 0;
-				wqe += sizeof (struct mthca_raddr_seg);
-				size += sizeof (struct mthca_raddr_seg) / 16;
-				break;
-
-			default:
-				/* No extra segments required for sends */
-				break;
-			}
-
-			break;
-
-		case UC:
-			switch (wr->opcode) {
-			case IB_WR_RDMA_WRITE:
-			case IB_WR_RDMA_WRITE_WITH_IMM:
-				((struct mthca_raddr_seg *) wqe)->raddr =
-					cpu_to_be64(wr->wr.rdma.remote_addr);
-				((struct mthca_raddr_seg *) wqe)->rkey =
-					cpu_to_be32(wr->wr.rdma.rkey);
-				((struct mthca_raddr_seg *) wqe)->reserved = 0;
-				wqe += sizeof (struct mthca_raddr_seg);
-				size += sizeof (struct mthca_raddr_seg) / 16;
-				break;
-
-			default:
-				/* No extra segments required for sends */
-				break;
-			}
-
-			break;
-
-		case UD:
-			((struct mthca_tavor_ud_seg *) wqe)->lkey =
-				cpu_to_be32(to_mah(wr->wr.ud.ah)->key);
-			((struct mthca_tavor_ud_seg *) wqe)->av_addr =
-				cpu_to_be64(to_mah(wr->wr.ud.ah)->avdma);
-			((struct mthca_tavor_ud_seg *) wqe)->dqpn =
-				cpu_to_be32(wr->wr.ud.remote_qpn);
-			((struct mthca_tavor_ud_seg *) wqe)->qkey =
-				cpu_to_be32(wr->wr.ud.remote_qkey);
-
-			wqe += sizeof (struct mthca_tavor_ud_seg);
-			size += sizeof (struct mthca_tavor_ud_seg) / 16;
-			break;
-
-		case MLX:
-			err = build_mlx_header(dev, to_msqp(qp), ind, wr,
-					       wqe - sizeof (struct mthca_next_seg),
-					       wqe);
-			if (err) {
-				*bad_wr = wr;
-				goto out;
-			}
-			wqe += sizeof (struct mthca_data_seg);
-			size += sizeof (struct mthca_data_seg) / 16;
-			break;
+		err = handle_raddr_seg(dev, qp, wr, wqe, ind);
+		if (err < 0) {
+			*bad_wr = wr;
+			goto out;
 		}
+		wqe += err;
+		size += err / 16;
 
 		if (wr->num_sge > qp->sq.max_gs) {
 			mthca_err(dev, "too many gathers\n");
@@ -1734,14 +1744,12 @@ int mthca_tavor_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 		}
 
 		for (i = 0; i < wr->num_sge; ++i) {
-			((struct mthca_data_seg *) wqe)->byte_count =
-				cpu_to_be32(wr->sg_list[i].length);
-			((struct mthca_data_seg *) wqe)->lkey =
-				cpu_to_be32(wr->sg_list[i].lkey);
-			((struct mthca_data_seg *) wqe)->addr =
-				cpu_to_be64(wr->sg_list[i].addr);
-			wqe += sizeof (struct mthca_data_seg);
-			size += sizeof (struct mthca_data_seg) / 16;
+			struct ib_sge *sge = wr->sg_list + i;
+			err = handle_data_seg((struct mthca_data_seg *) wqe, sge);
+
+			/* No errors possible */
+			wqe += err;
+			size += err / 16;
 		}
 
 		/* Add one more inline data segment for ICRC */

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Roland Dreier]

 > Quite frankly, I don't quite understand where you get those enormous balls 
 > you have, that you can then talk about how ugly it is to just add a "= 0" 
 > that shuts up a compiler warning. That's the _least_ ugly part of the 
 > whole damn function!

The clanking when I walk annoys people in the office too...

But you're right.  It is stupid of me to make such a big deal about
this.  My excuse is that I've seen those warnings so many times and
actually given them more thought than they deserve, and I really felt
that Jeff's change makes the admittedly already ugly code a tiny
little bit worse.

 > Anyway, here's a totally untested cleanup that compiles but probably 
 > doesn't work, because I didn't check that I did the right thing with all 
 > the pointer arithmetic (ie when I change "wqe" to a real structure pointer 
 > instead of just a "void *", maybe I left some pointer arithmetic around 
 > that expected it to work as a byte pointer, but now really works on the 
 > whole structure size instead).

Given that you took the time to do this, I'll get the patch into a
working state and apply it.  And maybe split it into reviewable chunks
while I'm at it ;)

Thanks,
  Roland

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Linus Torvalds]

On Tue, 17 Jul 2007, Roland Dreier wrote:
> 
>  > Anyway, here's a totally untested cleanup that compiles but probably 
>  > doesn't work, because I didn't check that I did the right thing with all 
>  > the pointer arithmetic (ie when I change "wqe" to a real structure pointer 
>  > instead of just a "void *", maybe I left some pointer arithmetic around 
>  > that expected it to work as a byte pointer, but now really works on the 
>  > whole structure size instead).
> 
> Given that you took the time to do this, I'll get the patch into a
> working state and apply it.  And maybe split it into reviewable chunks
> while I'm at it ;)

Hey, I appreciate it, but I really do have to warn you that I did this all 
blind, and just meant for it to be a "I think this kind of direction is 
more productive" thing. I'm not going to guarantee that it works at all.

I spent more time than I really wanted to on actually making sure the end 
result even compiles (quite often, I'm perfectly happy to just send out 
pseudo-code to indicate what I think should be done), but maybe I 
shouldn't have done that, just so that nobody thinks that the patch is 
necessarily going to *work*.

In other words, it's a totally mindless cleanup and re-factorization. It 
*may* work, but quite frankly, I did it just for that one email, and spent 
the absolutly minimum possible time thinking about it. As a result, I 
really think it needs some serious looking over.

I also think that my new "handle_raddr_seg()" function could itself be 
split up into subfunctions along the "switch()" subcases.

So not only wasn't it meant to be guaranteed correct, but it wasn't even 
meant to be seen as the best possible final situation: it could be - and 
should be - factored out even more (and the same goes for a lot of the 
other functions in that file that I didn't really look at, just glance and 
notice that they have some of the same problem patterns).

		Linus

Re: [git patches 2/2] warnings: use uninitialized_var()

[Adrian Bunk]

On Tue, Jul 17, 2007 at 05:49:52PM -0400, Jeff Garzik wrote:
> 
> For many months, I have maintained a hand-verified list of bogus "may be
> used uninitialized" warning fixes, in misc-2.6.git#gccbug.  Andrew urged
> me to head these upstream.
> 
> I have gone through and re-analyzed each warning, and verified that
> these variables are indeed initialized properly, and gcc is making
> needless noise.
>...

Some notes:
- if gcc can prove a variable gets used uninitialized it gives
  a different warning
- gcc says it may be used uninitialized - there can always be false
  positives when the correctness of the code is due to some higher
  level logic
- I've seen cases in the kernel where it was technically impossible
  for the compiler to verify a variable always gets initialized

So if we want these warnings we'll always need to silence the ones that 
are verified as being correct code like your patches do - and that's not 
gcc's fault.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Roland Dreier]

 > Hey, I appreciate it, but I really do have to warn you that I did this all 
 > blind, and just meant for it to be a "I think this kind of direction is 
 > more productive" thing. I'm not going to guarantee that it works at all.

Oh, understood, and I'm definitely planning on taking your patch as
inspiration and a kick in the pants to finally clean up the code
rather than trying to use it as is.  Obviously it was a quick hack
from someone that doesn't know the hardware -- for example
handle_raddr_seg() is at least misnamed, since UD work requests don't
have raddr segments.

But if you're going to take the time to work on the code, then I feel
like it's only polite not to drop your work on the floor.  Just like I
hope someone who works on IB and who is usually sending me stuff to
merge would give a patch from me some attention.

BTW, I noticed one interesting thing while starting on this cleanup.
I wanted to make sure that the generated code didn't change with the
first step, and I actually discovered that the patch below seems to
make the generated code *better*, maybe because some gcc alias
analysis doesn't get as paranoid without the void *:

add/remove: 0/0 grow/shrink: 0/7 up/down: 0/-85 (-85)
function                                     old     new   delta
mthca_arbel_post_srq_recv                    373     369      -4
mthca_arbel_post_receive                     570     562      -8
mthca_tavor_post_srq_recv                    520     508     -12
mthca_tavor_post_send                       1344    1330     -14
mthca_arbel_post_send                       1481    1467     -14
mlx4_ib_post_send                           1598    1582     -16
mthca_tavor_post_receive                     792     775     -17

And here's the patch itself -- I think this is a reasonable size step
to break things up into.  I assume that this is the basic form of the
cleanup that you're proposing?

diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
index 8d09aa3..a59c7f0 100644
--- a/drivers/infiniband/hw/mlx4/qp.c
+++ b/drivers/infiniband/hw/mlx4/qp.c
@@ -1183,6 +1183,14 @@ static int mlx4_wq_overflow(struct mlx4_ib_wq *wq, int nreq, struct ib_cq *ib_cq
 	return cur + nreq >= wq->max_post;
 }
 
+static void set_data_seg(struct mlx4_wqe_data_seg *dseg,
+			 struct ib_sge *sg)
+{
+	dseg->byte_count = cpu_to_be32(sg->length);
+	dseg->lkey       = cpu_to_be32(sg->lkey);
+	dseg->addr       = cpu_to_be64(sg->addr);
+}
+
 int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 		      struct ib_send_wr **bad_wr)
 {
@@ -1313,12 +1321,7 @@ int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 		}
 
 		for (i = 0; i < wr->num_sge; ++i) {
-			((struct mlx4_wqe_data_seg *) wqe)->byte_count =
-				cpu_to_be32(wr->sg_list[i].length);
-			((struct mlx4_wqe_data_seg *) wqe)->lkey =
-				cpu_to_be32(wr->sg_list[i].lkey);
-			((struct mlx4_wqe_data_seg *) wqe)->addr =
-				cpu_to_be64(wr->sg_list[i].addr);
+			set_data_seg(wqe, wr->sg_list + i);
 
 			wqe  += sizeof (struct mlx4_wqe_data_seg);
 			size += sizeof (struct mlx4_wqe_data_seg) / 16;
diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c b/drivers/infiniband/hw/mthca/mthca_qp.c
index 0e9ef24..2548250 100644
--- a/drivers/infiniband/hw/mthca/mthca_qp.c
+++ b/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -1740,13 +1740,8 @@ int mthca_tavor_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 		}
 
 		for (i = 0; i < wr->num_sge; ++i) {
-			((struct mthca_data_seg *) wqe)->byte_count =
-				cpu_to_be32(wr->sg_list[i].length);
-			((struct mthca_data_seg *) wqe)->lkey =
-				cpu_to_be32(wr->sg_list[i].lkey);
-			((struct mthca_data_seg *) wqe)->addr =
-				cpu_to_be64(wr->sg_list[i].addr);
-			wqe += sizeof (struct mthca_data_seg);
+			mthca_set_data_seg(wqe, wr->sg_list + i);
+			wqe  += sizeof (struct mthca_data_seg);
 			size += sizeof (struct mthca_data_seg) / 16;
 		}
 
@@ -1869,13 +1864,8 @@ int mthca_tavor_post_receive(struct ib_qp *ibqp, struct ib_recv_wr *wr,
 		}
 
 		for (i = 0; i < wr->num_sge; ++i) {
-			((struct mthca_data_seg *) wqe)->byte_count =
-				cpu_to_be32(wr->sg_list[i].length);
-			((struct mthca_data_seg *) wqe)->lkey =
-				cpu_to_be32(wr->sg_list[i].lkey);
-			((struct mthca_data_seg *) wqe)->addr =
-				cpu_to_be64(wr->sg_list[i].addr);
-			wqe += sizeof (struct mthca_data_seg);
+			mthca_set_data_seg(wqe, wr->sg_list + i);
+			wqe  += sizeof (struct mthca_data_seg);
 			size += sizeof (struct mthca_data_seg) / 16;
 		}
 
@@ -2125,13 +2115,8 @@ int mthca_arbel_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 		}
 
 		for (i = 0; i < wr->num_sge; ++i) {
-			((struct mthca_data_seg *) wqe)->byte_count =
-				cpu_to_be32(wr->sg_list[i].length);
-			((struct mthca_data_seg *) wqe)->lkey =
-				cpu_to_be32(wr->sg_list[i].lkey);
-			((struct mthca_data_seg *) wqe)->addr =
-				cpu_to_be64(wr->sg_list[i].addr);
-			wqe += sizeof (struct mthca_data_seg);
+			mthca_set_data_seg(wqe, wr->sg_list + i);
+			wqe  += sizeof (struct mthca_data_seg);
 			size += sizeof (struct mthca_data_seg) / 16;
 		}
 
@@ -2253,20 +2238,12 @@ int mthca_arbel_post_receive(struct ib_qp *ibqp, struct ib_recv_wr *wr,
 		}
 
 		for (i = 0; i < wr->num_sge; ++i) {
-			((struct mthca_data_seg *) wqe)->byte_count =
-				cpu_to_be32(wr->sg_list[i].length);
-			((struct mthca_data_seg *) wqe)->lkey =
-				cpu_to_be32(wr->sg_list[i].lkey);
-			((struct mthca_data_seg *) wqe)->addr =
-				cpu_to_be64(wr->sg_list[i].addr);
+			mthca_set_data_seg(wqe, wr->sg_list + i);
 			wqe += sizeof (struct mthca_data_seg);
 		}
 
-		if (i < qp->rq.max_gs) {
-			((struct mthca_data_seg *) wqe)->byte_count = 0;
-			((struct mthca_data_seg *) wqe)->lkey = cpu_to_be32(MTHCA_INVAL_LKEY);
-			((struct mthca_data_seg *) wqe)->addr = 0;
-		}
+		if (i < qp->rq.max_gs)
+			mthca_set_data_seg_inval(wqe);
 
 		qp->wrid[ind] = wr->wr_id;
 
diff --git a/drivers/infiniband/hw/mthca/mthca_srq.c b/drivers/infiniband/hw/mthca/mthca_srq.c
index b8f05a5..88d219e 100644
--- a/drivers/infiniband/hw/mthca/mthca_srq.c
+++ b/drivers/infiniband/hw/mthca/mthca_srq.c
@@ -543,20 +543,12 @@ int mthca_tavor_post_srq_recv(struct ib_srq *ibsrq, struct ib_recv_wr *wr,
 		}
 
 		for (i = 0; i < wr->num_sge; ++i) {
-			((struct mthca_data_seg *) wqe)->byte_count =
-				cpu_to_be32(wr->sg_list[i].length);
-			((struct mthca_data_seg *) wqe)->lkey =
-				cpu_to_be32(wr->sg_list[i].lkey);
-			((struct mthca_data_seg *) wqe)->addr =
-				cpu_to_be64(wr->sg_list[i].addr);
+			mthca_set_data_seg(wqe, wr->sg_list + i);
 			wqe += sizeof (struct mthca_data_seg);
 		}
 
-		if (i < srq->max_gs) {
-			((struct mthca_data_seg *) wqe)->byte_count = 0;
-			((struct mthca_data_seg *) wqe)->lkey = cpu_to_be32(MTHCA_INVAL_LKEY);
-			((struct mthca_data_seg *) wqe)->addr = 0;
-		}
+		if (i < srq->max_gs)
+			mthca_set_data_seg_inval(wqe);
 
 		((struct mthca_next_seg *) prev_wqe)->nda_op =
 			cpu_to_be32((ind << srq->wqe_shift) | 1);
@@ -662,20 +654,12 @@ int mthca_arbel_post_srq_recv(struct ib_srq *ibsrq, struct ib_recv_wr *wr,
 		}
 
 		for (i = 0; i < wr->num_sge; ++i) {
-			((struct mthca_data_seg *) wqe)->byte_count =
-				cpu_to_be32(wr->sg_list[i].length);
-			((struct mthca_data_seg *) wqe)->lkey =
-				cpu_to_be32(wr->sg_list[i].lkey);
-			((struct mthca_data_seg *) wqe)->addr =
-				cpu_to_be64(wr->sg_list[i].addr);
+			mthca_set_data_seg(wqe, wr->sg_list + i);
 			wqe += sizeof (struct mthca_data_seg);
 		}
 
-		if (i < srq->max_gs) {
-			((struct mthca_data_seg *) wqe)->byte_count = 0;
-			((struct mthca_data_seg *) wqe)->lkey = cpu_to_be32(MTHCA_INVAL_LKEY);
-			((struct mthca_data_seg *) wqe)->addr = 0;
-		}
+		if (i < srq->max_gs)
+			mthca_set_data_seg_inval(wqe);
 
 		srq->wrid[ind]  = wr->wr_id;
 		srq->first_free = next_ind;
diff --git a/drivers/infiniband/hw/mthca/mthca_wqe.h b/drivers/infiniband/hw/mthca/mthca_wqe.h
index e7d2c1e..f6a66fe 100644
--- a/drivers/infiniband/hw/mthca/mthca_wqe.h
+++ b/drivers/infiniband/hw/mthca/mthca_wqe.h
@@ -113,4 +113,19 @@ struct mthca_mlx_seg {
 	__be16 vcrc;
 };
 
+static __always_inline void mthca_set_data_seg(struct mthca_data_seg *dseg,
+					       struct ib_sge *sg)
+{
+	dseg->byte_count = cpu_to_be32(sg->length);
+	dseg->lkey       = cpu_to_be32(sg->lkey);
+	dseg->addr       = cpu_to_be64(sg->addr);
+}
+
+static __always_inline void mthca_set_data_seg_inval(struct mthca_data_seg *dseg)
+{
+	dseg->byte_count = 0;
+	dseg->lkey       = cpu_to_be32(MTHCA_INVAL_LKEY);
+	dseg->addr       = 0;
+}
+
 #endif /* MTHCA_WQE_H */

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Linus Torvalds]

On Wed, 18 Jul 2007, Roland Dreier wrote:
> 
> BTW, I noticed one interesting thing while starting on this cleanup.
> I wanted to make sure that the generated code didn't change with the
> first step, and I actually discovered that the patch below seems to
> make the generated code *better*, maybe because some gcc alias
> analysis doesn't get as paranoid without the void *:

Absolutely.

The way to get pretty much any compiler to generate better code is:
 - code it simply
 - don't have tons of variables with overlapping lifetime
 - use limited-scope variables (ie don't have the variables at the 
   outermost scope, declare them in the smallest scope you can)

and trying to split things up into functions helps all of these. 

In fact, you can often get better code even when the functions aren't even 
inlined, because of the "simpler code" issue. Gcc sometimes tries to be 
too clever with its CSE etc, and generates really nasty complex code with 
lots of register spills, just because it keeps stuff live rather than just 
regenerating them.

So inlining a function doesn't even make it faster, all the time. You want 
to inline when 

 - the function is so small that the call is literally a big part of it, 
   and it doesn't even need more than a couple of registers, so the 
   calling convention has more register pressure than inlining the 
   function itself would have.

OR

 - the callers tend to have constant arguments that can benefit from 
   constant folding inside the function

but inlining in many other circumstances actually generates *worse* code 
and just makes debugging harder and the I$ footprint bigger.

> And here's the patch itself -- I think this is a reasonable size step
> to break things up into.  I assume that this is the basic form of the
> cleanup that you're proposing?

Yes, this looks good. Doing these kinds of things for the various other 
things is likely to make the code more readable, and as you already found 
out, the generated code doesn't tend to be worse either. It might not 
_always_ be a win size and performance-wise, but it can be, and so 
readability should generally always be the #1 goal, because quite often it 
actually does help the compiler too.

		Linus

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Jeff Garzik]

Greg KH wrote:
> On Tue, Jul 17, 2007 at 05:42:39PM -0400, Jeff Garzik wrote:
>> commit ae97fec3701a559929c3529e35417fab133a4d39
>> Author: Jeff Garzik <jeff@garzik.org>
>> Date:   Tue Jul 17 01:08:29 2007 -0400
>>
>>     drivers/usb/misc/auerswald: fix status check, remove redundant check
>>     
>>     1) We should only set 'actual_length' output variable if usb length is
>>     known to be good.
>>     
>>     2) No need to check actual_length for NULL.  The only caller always
>>     passes non-NULL value.
>>     
>>     Signed-off-by: Jeff Garzik <jeff@garzik.org>
> 
> I have no objection to this patch at all, however it does not remove the
> compiler warning :(

The description doesn't say it removes a warning, so it doesn't :)

You want to look at

	drivers/*: mark variables with uninitialized_var()

which was in posting "[git patches 2/2] warnings: use 
uninitialized_var()" and is now upstream commit 
a6343afb6e16b65b9f0b264f94f8207212e7e3ae

	Jeff

Re: [git patches 1/2] warnings: attack valid cases spotted by warnings

[Greg KH]

On Wed, Jul 18, 2007 at 04:03:05PM -0400, Jeff Garzik wrote:
>  Greg KH wrote:
> > On Tue, Jul 17, 2007 at 05:42:39PM -0400, Jeff Garzik wrote:
> >> commit ae97fec3701a559929c3529e35417fab133a4d39
> >> Author: Jeff Garzik <jeff@garzik.org>
> >> Date:   Tue Jul 17 01:08:29 2007 -0400
> >>
> >>     drivers/usb/misc/auerswald: fix status check, remove redundant check
> >>         1) We should only set 'actual_length' output variable if usb 
> >> length is
> >>     known to be good.
> >>         2) No need to check actual_length for NULL.  The only caller 
> >> always
> >>     passes non-NULL value.
> >>         Signed-off-by: Jeff Garzik <jeff@garzik.org>
> > I have no objection to this patch at all, however it does not remove the
> > compiler warning :(
> 
>  The description doesn't say it removes a warning, so it doesn't :)
> 
>  You want to look at
> 
>  	drivers/*: mark variables with uninitialized_var()
> 
>  which was in posting "[git patches 2/2] warnings: use uninitialized_var()" 
>  and is now upstream commit a6343afb6e16b65b9f0b264f94f8207212e7e3ae

Ah, sorry, I just went for the first USB change entry, sorry about that
:)

And thanks for doing this, clean builds are good to have.

greg k-h