LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [announce] new tree: "fix all build warnings, on all configs"
@ 2008-10-17 17:11 Ingo Molnar
  2008-10-17 17:59 ` Roland Dreier
  2008-10-18  7:43 ` Andi Kleen
  0 siblings, 2 replies; 23+ messages in thread
From: Ingo Molnar @ 2008-10-17 17:11 UTC (permalink / raw)
  To: Linus Torvalds, Andrew Morton
  Cc: David S. Miller, Alan Cox, linux-kernel, Peter Zijlstra,
	Thomas Gleixner, H. Peter Anvin, David Howells


I'd like to announce a new upstream -git based tree i started working on 
some time ago, the "remove all compiler warnings on x86, on any .config" 
tree.

This tree adds the CONFIG_ALLOW_WARNINGS config option - which, if 
disabled, adds -Werror to the build flags and results in a failed build 
if any compiler warning is emitted by the kernel build. I started the 
tree based on David Howells's CONFIG_ENABLE_WERROR patchset.

Right now the results are pretty good already: x86 builds with zero 
warnings both allyesconfig, allnoconfig and allmodconfig, 32-bit and 
64-bit as well - and most randconfig kernels will build fine as well. I 
continue fixing randconfig triggered warnings as i trigger them.

The main purpose of this tree is to use it in the -tip auto-testing 
randconfig based kernel testing machinery, and to fix all new warnings 
that trigger via that patch influx vector - and to help other 
maintainers fix warnings that reach the upstream kernel.

The tip/warnings tree consists of 5 topic branches comprising 83 commits 
at the moment:

 earth4:~/tip> tip-pending warnings/

   topic                              #commits
   -------------------------------------------
   warnings/bug                       :     19
   warnings/complex                   :     27
   warnings/infrastructure            :      5
   warnings/simple                    :     23
   warnings/ugly                      :      9
   ---------------------
   total topics:       5
   pending topics:     5
   pending commits:   83

tip/warnings/infrastructure:

  this topic contains the basic patches that enable this mode of 
  building.

tip/warnings/simple:

  this topic is for simple, obvious (looking) fixes. 

tip/warnings/complex:

  this topic is for fixes that i categorized as more complex - 
  they need review and more testing.

tip/warnings/ugly:

  these are workarounds for gcc bugs that i did not consider upstream 
  worthy in any way. They also contain patches that fell through my 
  review and need further improvements.

tip/warnings/bug:

  this is a special topic for a certain type of warning: BUG() turning 
  into a NOP on CONFIG_EMBEDDED + !CONFIG_BUG. In this case gcc is 
  completely right in warning us in various places that we are 
  triggering undefined behavior. This topic is fairly complete in that 
  it fixes all these warnings i could trigger on 
  allyesconfig+!CONFIG_BUG - but i'm on two minds what we should do. A 
  simple solution would be to not allow the turning off of BUG() - but 
  to turn it into something really simple, such as an infinite loop. 
  Unfortunately that increases the size of the kernel by +0.2% so i did 
  not want t do it without consideration. I kept these commits filtered 
  out.

Note that deprecated API and unused-return-value warnings 
(ENABLE_WARN_DEPRECATED and ENABLE_MUST_CHECK) are not included right 
now, there's way too many of them. If this effort works out fine the we 
could start covering those warnings too.

It is also not clear to me how i should annotate variables for gcc false 
positives. The current upstream kernel method is:

        uninitialized_var(i);

But Alan suggested that we should use __attribute__((used)) instead:

        #define __used __attribute__((used))

and that he would not accept uninitialized_var() annotations. A central 
policy decision is needed on that. I wouldnt mind to convert all 
uninitialized_var() annotations over into __used annotations - but the 
question is: is this really an equivalent transformation? Can gcc be 
fooled into not initializing an incorrect __used annotation? With 
uninitialized_var() we at least have a specific initialization to zero.

The integrated tree is available at tip/auto-warnings-next. Obviously 
the warnings/ugly and warnings/complex topics are still work in 
progress. The coordinates of the tip/warnings/simple topic can be found 
below.

Comments, suggestions are welcome.

	Ingo

-------------->
the latest warnings/simple git tree is available at:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git warnings/simple

	Ingo

------------------>
Ingo Molnar (24):
      [vfs] fs.h - fix warning in drivers/infiniband/core/uverbs_main.c
      fix warning in fs/ext4/extents.c
      work around warning in fs/xfs/xfs_rtalloc.c
      fix warning in drivers/net/mlx4/mcg.c
      fix warning in drivers/net/mlx4/profile.c
      fix warning in arch/x86/kernel/io_apic_32.c
      fix warning in arch/x86/kernel/setup.c
      fix warning in sound/soc/codecs/tlv320aic23.c
      fix warning in drivers/ide/pci/hpt366.c
      fix warning in net/mac80211/rc80211_minstrel_debugfs.c
      fix warning in drivers/media/dvb/frontends/cx24116.c
      fix warning in drivers/video/cirrusfb.c
      fix warning in drivers/isdn/i4l/isdn_ppp.c
      fix warning in fs/afs/dir.c
      fix warning in net/netlabel/netlabel_addrlist.c
      fix warning in drivers/net/wireless/b43/phy_g.c
      fix warning in arch/x86/kernel/early-quirks.c
      include/linux/fs.h: fix warning in fs/gfs2/ops_file.c
      fix warning in drivers/base/platform.c
      fix warning in drivers/pci/pci-driver.c
      fix warning in drivers/acpi/sleep/main.c
      fix warning in net/ax25/sysctl_net_ax25.c
      fix warning in arch/x86/kernel/paravirt-spinlocks.c
      fix warnings in drivers/acpi/sbs.c


 arch/x86/kernel/early-quirks.c          |    4 +++-
 arch/x86/kernel/io_apic_32.c            |    4 ++--
 arch/x86/kernel/paravirt-spinlocks.c    |    3 ++-
 arch/x86/kernel/setup.c                 |    2 ++
 drivers/acpi/sbs.c                      |    7 +++++++
 drivers/acpi/sleep/main.c               |   10 +++++-----
 drivers/base/platform.c                 |   10 ++++++----
 drivers/ide/pci/hpt366.c                |    1 -
 drivers/isdn/i4l/isdn_ppp.c             |    2 ++
 drivers/media/dvb/frontends/cx24116.c   |    3 ++-
 drivers/net/mlx4/mcg.c                  |    2 +-
 drivers/net/mlx4/profile.c              |    2 ++
 drivers/net/wireless/b43/b43.h          |    3 ++-
 drivers/pci/pci-driver.c                |    9 +++++----
 drivers/video/cirrusfb.c                |    3 +--
 fs/afs/dir.c                            |    2 +-
 fs/ext4/extents.c                       |    1 +
 fs/xfs/xfs_rtalloc.c                    |    2 +-
 include/linux/audit.h                   |    3 ++-
 include/linux/fs.h                      |    6 +++---
 net/ax25/sysctl_net_ax25.c              |    2 ++
 net/mac80211/rc80211_minstrel_debugfs.c |    2 +-
 sound/soc/codecs/tlv320aic23.c          |    2 --
 23 files changed, 53 insertions(+), 32 deletions(-)

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index 733c4f8..e587947 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -95,6 +95,7 @@ static void __init nvidia_bugs(int num, int slot, int func)
 
 }
 
+#if defined(CONFIG_ACPI) && defined(CONFIG_X86_IO_APIC)
 static u32 ati_ixp4x0_rev(int num, int slot, int func)
 {
 	u32 d;
@@ -112,10 +113,11 @@ static u32 ati_ixp4x0_rev(int num, int slot, int func)
 	d &= 0xff;
 	return d;
 }
+#endif
 
 static void __init ati_bugs(int num, int slot, int func)
 {
-#if defined(CONFIG_ACPI) && defined (CONFIG_X86_IO_APIC)
+#if defined(CONFIG_ACPI) && defined(CONFIG_X86_IO_APIC)
 	u32 d;
 	u8  b;
 
diff --git a/arch/x86/kernel/io_apic_32.c b/arch/x86/kernel/io_apic_32.c
index e710289..64f0953 100644
--- a/arch/x86/kernel/io_apic_32.c
+++ b/arch/x86/kernel/io_apic_32.c
@@ -1536,8 +1536,8 @@ __apicdebuginit(void) print_local_APIC(void *dummy)
 	}
 
 	icr = apic_icr_read();
-	printk(KERN_DEBUG "... APIC ICR: %08x\n", icr);
-	printk(KERN_DEBUG "... APIC ICR2: %08x\n", icr >> 32);
+	printk(KERN_DEBUG "... APIC ICR: %08x\n", (u32)icr);
+	printk(KERN_DEBUG "... APIC ICR2: %08x\n", (u32)(icr >> 32));
 
 	v = apic_read(APIC_LVTT);
 	printk(KERN_DEBUG "... APIC LVTT: %08x\n", v);
diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
index 0e9f198..95777b0 100644
--- a/arch/x86/kernel/paravirt-spinlocks.c
+++ b/arch/x86/kernel/paravirt-spinlocks.c
@@ -7,7 +7,8 @@
 
 #include <asm/paravirt.h>
 
-static void default_spin_lock_flags(struct raw_spinlock *lock, unsigned long flags)
+static inline void
+default_spin_lock_flags(raw_spinlock_t *lock, unsigned long flags)
 {
 	__raw_spin_lock(lock);
 }
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 2255782..44c2315 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -732,6 +732,7 @@ void start_periodic_check_for_corruption(void)
 }
 #endif
 
+#ifdef CONFIG_X86_RESERVE_LOW_64K
 static int __init dmi_low_memory_corruption(const struct dmi_system_id *d)
 {
 	printk(KERN_NOTICE
@@ -743,6 +744,7 @@ static int __init dmi_low_memory_corruption(const struct dmi_system_id *d)
 
 	return 0;
 }
+#endif
 
 /* List of systems that have known low memory corruption BIOS problems */
 static struct dmi_system_id __initdata bad_bios_dmi_table[] = {
diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
index 10a3651..2657182 100644
--- a/drivers/acpi/sbs.c
+++ b/drivers/acpi/sbs.c
@@ -389,6 +389,8 @@ static int acpi_battery_get_state(struct acpi_battery *battery)
 	return result;
 }
 
+#if defined(CONFIG_ACPI_SYSFS_POWER) || defined(CONFIG_ACPI_PROCFS_POWER)
+
 static int acpi_battery_get_alarm(struct acpi_battery *battery)
 {
 	return acpi_smbus_read(battery->sbs->hc, SMBUS_READ_WORD,
@@ -425,6 +427,8 @@ static int acpi_battery_set_alarm(struct acpi_battery *battery)
 	return ret;
 }
 
+#endif
+
 static int acpi_ac_get_present(struct acpi_sbs *sbs)
 {
 	int result;
@@ -816,7 +820,10 @@ static int acpi_battery_add(struct acpi_sbs *sbs, int id)
 
 static void acpi_battery_remove(struct acpi_sbs *sbs, int id)
 {
+#if defined(CONFIG_ACPI_SYSFS_POWER) || defined(CONFIG_ACPI_PROCFS_POWER)
 	struct acpi_battery *battery = &sbs->battery[id];
+#endif
+
 #ifdef CONFIG_ACPI_SYSFS_POWER
 	if (battery->bat.dev) {
 		if (battery->have_sysfs_alarm)
diff --git a/drivers/acpi/sleep/main.c b/drivers/acpi/sleep/main.c
index d13194a..2276d75 100644
--- a/drivers/acpi/sleep/main.c
+++ b/drivers/acpi/sleep/main.c
@@ -63,7 +63,7 @@ void __init acpi_old_suspend_ordering(void)
 /**
  *	acpi_pm_disable_gpes - Disable the GPEs.
  */
-static int acpi_pm_disable_gpes(void)
+static inline int acpi_pm_disable_gpes(void)
 {
 	acpi_hw_disable_all_gpes();
 	return 0;
@@ -75,7 +75,7 @@ static int acpi_pm_disable_gpes(void)
  *	If necessary, set the firmware waking vector and do arch-specific
  *	nastiness to get the wakeup code to the waking vector.
  */
-static int __acpi_pm_prepare(void)
+static inline int __acpi_pm_prepare(void)
 {
 	int error = acpi_sleep_prepare(acpi_target_sleep_state);
 
@@ -88,7 +88,7 @@ static int __acpi_pm_prepare(void)
  *	acpi_pm_prepare - Prepare the platform to enter the target sleep
  *		state and disable the GPEs.
  */
-static int acpi_pm_prepare(void)
+static inline int acpi_pm_prepare(void)
 {
 	int error = __acpi_pm_prepare();
 
@@ -103,7 +103,7 @@ static int acpi_pm_prepare(void)
  *	This is called after we wake back up (or if entering the sleep state
  *	failed).
  */
-static void acpi_pm_finish(void)
+static inline void acpi_pm_finish(void)
 {
 	u32 acpi_state = acpi_target_sleep_state;
 
@@ -124,7 +124,7 @@ static void acpi_pm_finish(void)
 /**
  *	acpi_pm_end - Finish up suspend sequence.
  */
-static void acpi_pm_end(void)
+static inline void acpi_pm_end(void)
 {
 	/*
 	 * This is necessary in case acpi_pm_finish() is not called during a
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index dfcbfe5..e0bcd6b 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -614,7 +614,8 @@ static int platform_match(struct device *dev, struct device_driver *drv)
 
 #ifdef CONFIG_PM_SLEEP
 
-static int platform_legacy_suspend(struct device *dev, pm_message_t mesg)
+static inline int
+platform_legacy_suspend(struct device *dev, pm_message_t mesg)
 {
 	int ret = 0;
 
@@ -624,7 +625,8 @@ static int platform_legacy_suspend(struct device *dev, pm_message_t mesg)
 	return ret;
 }
 
-static int platform_legacy_suspend_late(struct device *dev, pm_message_t mesg)
+static inline int
+platform_legacy_suspend_late(struct device *dev, pm_message_t mesg)
 {
 	struct platform_driver *drv = to_platform_driver(dev->driver);
 	struct platform_device *pdev;
@@ -637,7 +639,7 @@ static int platform_legacy_suspend_late(struct device *dev, pm_message_t mesg)
 	return ret;
 }
 
-static int platform_legacy_resume_early(struct device *dev)
+static inline int platform_legacy_resume_early(struct device *dev)
 {
 	struct platform_driver *drv = to_platform_driver(dev->driver);
 	struct platform_device *pdev;
@@ -650,7 +652,7 @@ static int platform_legacy_resume_early(struct device *dev)
 	return ret;
 }
 
-static int platform_legacy_resume(struct device *dev)
+static inline int platform_legacy_resume(struct device *dev)
 {
 	int ret = 0;
 
diff --git a/drivers/ide/pci/hpt366.c b/drivers/ide/pci/hpt366.c
index 9cf171c..ca0acb5 100644
--- a/drivers/ide/pci/hpt366.c
+++ b/drivers/ide/pci/hpt366.c
@@ -1289,7 +1289,6 @@ static u8 hpt3xx_cable_detect(ide_hwif_t *hwif)
 
 static void __devinit init_hwif_hpt366(ide_hwif_t *hwif)
 {
-	struct pci_dev *dev	= to_pci_dev(hwif->dev);
 	struct hpt_info *info	= hpt3xx_get_info(hwif->dev);
 	int serialize		= HPT_SERIALIZE_IO;
 	u8  chip_type		= info->chip_type;
diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
index 77c280e..c179360 100644
--- a/drivers/isdn/i4l/isdn_ppp.c
+++ b/drivers/isdn/i4l/isdn_ppp.c
@@ -431,6 +431,7 @@ set_arg(void __user *b, void *val,int len)
 	return 0;
 }
 
+#ifdef CONFIG_IPPP_FILTER
 static int get_filter(void __user *arg, struct sock_filter **p)
 {
 	struct sock_fprog uprog;
@@ -465,6 +466,7 @@ static int get_filter(void __user *arg, struct sock_filter **p)
 	*p = code;
 	return uprog.len;
 }
+#endif
 
 /*
  * ippp device ioctl
diff --git a/drivers/media/dvb/frontends/cx24116.c b/drivers/media/dvb/frontends/cx24116.c
index deb36f4..eb5febc 100644
--- a/drivers/media/dvb/frontends/cx24116.c
+++ b/drivers/media/dvb/frontends/cx24116.c
@@ -200,7 +200,8 @@ static int cx24116_writereg(struct cx24116_state* state, int reg, int data)
 }
 
 /* Bulk byte writes to a single I2C address, for 32k firmware load */
-static int cx24116_writeregN(struct cx24116_state* state, int reg, u8 *data, u16 len)
+static int
+cx24116_writeregN(struct cx24116_state* state, int reg, const u8 *data, u16 len)
 {
 	int ret = -EREMOTEIO;
 	struct i2c_msg msg;
diff --git a/drivers/net/mlx4/mcg.c b/drivers/net/mlx4/mcg.c
index c83f88c..d1230a8 100644
--- a/drivers/net/mlx4/mcg.c
+++ b/drivers/net/mlx4/mcg.c
@@ -215,7 +215,7 @@ int mlx4_multicast_attach(struct mlx4_dev *dev, struct mlx4_qp *qp, u8 gid[16],
 
 	if (block_mcast_loopback)
 		mgm->qp[members_count++] = cpu_to_be32((qp->qpn & MGM_QPN_MASK) |
-						       (1 << MGM_BLCK_LB_BIT));
+						       (1U << MGM_BLCK_LB_BIT));
 	else
 		mgm->qp[members_count++] = cpu_to_be32(qp->qpn & MGM_QPN_MASK);
 
diff --git a/drivers/net/mlx4/profile.c b/drivers/net/mlx4/profile.c
index 9ca42b2..ec9383d 100644
--- a/drivers/net/mlx4/profile.c
+++ b/drivers/net/mlx4/profile.c
@@ -52,6 +52,7 @@ enum {
 	MLX4_RES_NUM
 };
 
+#ifdef CONFIG_MLX4_DEBUG
 static const char *res_name[] = {
 	[MLX4_RES_QP]		= "QP",
 	[MLX4_RES_RDMARC]	= "RDMARC",
@@ -65,6 +66,7 @@ static const char *res_name[] = {
 	[MLX4_RES_MTT]		= "MTT",
 	[MLX4_RES_MCG]		= "MCG",
 };
+#endif
 
 u64 mlx4_make_profile(struct mlx4_dev *dev,
 		      struct mlx4_profile *request,
diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
index 427b820..ac55b62 100644
--- a/drivers/net/wireless/b43/b43.h
+++ b/drivers/net/wireless/b43/b43.h
@@ -853,7 +853,8 @@ void b43warn(struct b43_wl *wl, const char *fmt, ...)
 void b43dbg(struct b43_wl *wl, const char *fmt, ...)
     __attribute__ ((format(printf, 2, 3)));
 #else /* DEBUG */
-# define b43dbg(wl, fmt...) do { /* nothing */ } while (0)
+static inline void __attribute__ ((format(printf, 2, 3)))
+b43dbg(struct b43_wl *wl, const char *fmt, ...) { }
 #endif /* DEBUG */
 
 /* A WARN_ON variant that vanishes when b43 debugging is disabled.
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index a13f534..b062868 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -324,7 +324,7 @@ static int pci_default_pm_resume(struct pci_dev *pci_dev)
 	return retval;
 }
 
-static int pci_legacy_suspend(struct device *dev, pm_message_t state)
+static inline int pci_legacy_suspend(struct device *dev, pm_message_t state)
 {
 	struct pci_dev * pci_dev = to_pci_dev(dev);
 	struct pci_driver * drv = pci_dev->driver;
@@ -339,7 +339,8 @@ static int pci_legacy_suspend(struct device *dev, pm_message_t state)
 	return i;
 }
 
-static int pci_legacy_suspend_late(struct device *dev, pm_message_t state)
+static inline int
+pci_legacy_suspend_late(struct device *dev, pm_message_t state)
 {
 	struct pci_dev * pci_dev = to_pci_dev(dev);
 	struct pci_driver * drv = pci_dev->driver;
@@ -352,7 +353,7 @@ static int pci_legacy_suspend_late(struct device *dev, pm_message_t state)
 	return i;
 }
 
-static int pci_legacy_resume(struct device *dev)
+static inline int pci_legacy_resume(struct device *dev)
 {
 	int error;
 	struct pci_dev * pci_dev = to_pci_dev(dev);
@@ -365,7 +366,7 @@ static int pci_legacy_resume(struct device *dev)
 	return error;
 }
 
-static int pci_legacy_resume_early(struct device *dev)
+static inline int pci_legacy_resume_early(struct device *dev)
 {
 	int error = 0;
 	struct pci_dev * pci_dev = to_pci_dev(dev);
diff --git a/drivers/video/cirrusfb.c b/drivers/video/cirrusfb.c
index 048b139..ace4001 100644
--- a/drivers/video/cirrusfb.c
+++ b/drivers/video/cirrusfb.c
@@ -2462,8 +2462,7 @@ static int __init cirrusfb_init(void)
 
 #ifndef MODULE
 static int __init cirrusfb_setup(char *options) {
-	char *this_opt, s[32];
-	int i;
+	char *this_opt;
 
 	DPRINTK("ENTER\n");
 
diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index dfda03d..254c567 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -563,7 +563,7 @@ static struct dentry *afs_lookup(struct inode *dir, struct dentry *dentry,
 static int afs_d_revalidate(struct dentry *dentry, struct nameidata *nd)
 {
 	struct afs_vnode *vnode, *dir;
-	struct afs_fid fid;
+	struct afs_fid fid = { 0, };
 	struct dentry *parent;
 	struct key *key;
 	void *dir_version;
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index ea2ce3c..9a34682 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -1156,6 +1156,7 @@ ext4_ext_search_right(struct inode *inode, struct ext4_ext_path *path,
 		return 0;
 	}
 
+	ix = NULL; /* avoid gcc false positive warning */
 	/* go up and search for index to the right */
 	while (--depth >= 0) {
 		ix = path[depth].p_idx;
diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index e2f68de..fe5de08 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -1872,7 +1872,7 @@ xfs_growfs_rt(
 	xfs_extlen_t	rsumblocks;	/* current number of rt summary blks */
 	xfs_sb_t	*sbp;		/* old superblock */
 	xfs_fsblock_t	sumbno;		/* summary block number */
-	xfs_trans_t	*tp;		/* transaction pointer */
+	xfs_trans_t	*uninitialized_var(tp);	/* transaction pointer */
 
 	sbp = &mp->m_sb;
 	cancelflags = 0;
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 6272a39..a3f78d0 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -580,7 +580,8 @@ extern int audit_enabled;
 #define audit_log(c,g,t,f,...) do { ; } while (0)
 #define audit_log_start(c,g,t) ({ NULL; })
 #define audit_log_vformat(b,f,a) do { ; } while (0)
-#define audit_log_format(b,f,...) do { ; } while (0)
+static inline void __attribute__ ((format(printf, 2, 3)))
+audit_log_format(struct audit_buffer *ab, const char *fmt, ...) { }
 #define audit_log_end(b) do { ; } while (0)
 #define audit_log_n_hex(a,b,l) do { ; } while (0)
 #define audit_log_n_string(a,c,l) do { ; } while (0)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index a6a625b..114f469 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1597,9 +1597,9 @@ void unnamed_dev_init(void);
 
 /* Alas, no aliases. Too much hassle with bringing module.h everywhere */
 #define fops_get(fops) \
-	(((fops) && try_module_get((fops)->owner) ? (fops) : NULL))
+	(((fops != NULL) && try_module_get((fops)->owner) ? (fops) : NULL))
 #define fops_put(fops) \
-	do { if (fops) module_put((fops)->owner); } while(0)
+	do { if (fops != NULL) module_put((fops)->owner); } while(0)
 
 extern int register_filesystem(struct file_system_type *);
 extern int unregister_filesystem(struct file_system_type *);
@@ -1675,7 +1675,7 @@ static inline int break_lease(struct inode *inode, unsigned int mode)
 #else /* !CONFIG_FILE_LOCKING */
 #define locks_mandatory_locked(a) ({ 0; })
 #define locks_mandatory_area(a, b, c, d, e) ({ 0; })
-#define __mandatory_lock(a) ({ 0; })
+static inline int __mandatory_lock(struct inode *ino) { return 0; }
 #define mandatory_lock(a) ({ 0; })
 #define locks_verify_locked(a) ({ 0; })
 #define locks_verify_truncate(a, b, c) ({ 0; })
diff --git a/net/ax25/sysctl_net_ax25.c b/net/ax25/sysctl_net_ax25.c
index f288fc4..735ceef 100644
--- a/net/ax25/sysctl_net_ax25.c
+++ b/net/ax25/sysctl_net_ax25.c
@@ -24,7 +24,9 @@ static int min_idle[1],			max_idle[] = {65535000};
 static int min_n2[] = {1},		max_n2[] = {31};
 static int min_paclen[] = {1},		max_paclen[] = {512};
 static int min_proto[1],		max_proto[] = { AX25_PROTO_MAX };
+#ifdef CONFIG_AX25_DAMA_SLAVE
 static int min_ds_timeout[1],		max_ds_timeout[] = {65535000};
+#endif
 
 static struct ctl_table_header *ax25_table_header;
 
diff --git a/net/mac80211/rc80211_minstrel_debugfs.c b/net/mac80211/rc80211_minstrel_debugfs.c
index 0b024cd..26f8ffc 100644
--- a/net/mac80211/rc80211_minstrel_debugfs.c
+++ b/net/mac80211/rc80211_minstrel_debugfs.c
@@ -106,7 +106,7 @@ minstrel_stats_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static int
+static ssize_t
 minstrel_stats_read(struct file *file, char __user *buf, size_t len, loff_t *o)
 {
 	struct minstrel_stats_info *ms;
diff --git a/sound/soc/codecs/tlv320aic23.c b/sound/soc/codecs/tlv320aic23.c
index 44308da..45395d0 100644
--- a/sound/soc/codecs/tlv320aic23.c
+++ b/sound/soc/codecs/tlv320aic23.c
@@ -421,8 +421,6 @@ static int tlv320aic23_set_dai_fmt(struct snd_soc_dai *codec_dai,
 static int tlv320aic23_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 				      int clk_id, unsigned int freq, int dir)
 {
-	struct snd_soc_codec *codec = codec_dai->codec;
-
 	switch (freq) {
 	case 12000000:
 		return 0;

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-17 17:11 [announce] new tree: "fix all build warnings, on all configs" Ingo Molnar
@ 2008-10-17 17:59 ` Roland Dreier
  2008-10-17 18:05   ` Ingo Molnar
  2008-10-18  7:43 ` Andi Kleen
  1 sibling, 1 reply; 23+ messages in thread
From: Roland Dreier @ 2008-10-17 17:59 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linus Torvalds, Andrew Morton, David S. Miller, Alan Cox,
	linux-kernel, Peter Zijlstra, Thomas Gleixner, H. Peter Anvin,
	David Howells

 > diff --git a/drivers/net/mlx4/mcg.c b/drivers/net/mlx4/mcg.c
 > index c83f88c..d1230a8 100644
 > --- a/drivers/net/mlx4/mcg.c
 > +++ b/drivers/net/mlx4/mcg.c
 > @@ -215,7 +215,7 @@ int mlx4_multicast_attach(struct mlx4_dev *dev, struct mlx4_qp *qp, u8 gid[16],
 >  
 >  	if (block_mcast_loopback)
 >  		mgm->qp[members_count++] = cpu_to_be32((qp->qpn & MGM_QPN_MASK) |
 > -						       (1 << MGM_BLCK_LB_BIT));
 > +						       (1U << MGM_BLCK_LB_BIT));

This is fixing a warning caused by a gcc bug
(http://gcc.gnu.org/bugzilla/show_bug.cgi?id=37261) which is fixed
upstream.  The change is rather inoffensive but on the other hand I'm
not sure what our policy for dealing with version-specific warning bugs
in gcc should be.

 - R.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-17 17:59 ` Roland Dreier
@ 2008-10-17 18:05   ` Ingo Molnar
  2008-10-17 18:47     ` Roland Dreier
  0 siblings, 1 reply; 23+ messages in thread
From: Ingo Molnar @ 2008-10-17 18:05 UTC (permalink / raw)
  To: Roland Dreier
  Cc: Linus Torvalds, Andrew Morton, David S. Miller, Alan Cox,
	linux-kernel, Peter Zijlstra, Thomas Gleixner, H. Peter Anvin,
	David Howells


* Roland Dreier <rdreier@cisco.com> wrote:

>  > diff --git a/drivers/net/mlx4/mcg.c b/drivers/net/mlx4/mcg.c
>  > index c83f88c..d1230a8 100644
>  > --- a/drivers/net/mlx4/mcg.c
>  > +++ b/drivers/net/mlx4/mcg.c
>  > @@ -215,7 +215,7 @@ int mlx4_multicast_attach(struct mlx4_dev *dev, struct mlx4_qp *qp, u8 gid[16],
>  >  
>  >  	if (block_mcast_loopback)
>  >  		mgm->qp[members_count++] = cpu_to_be32((qp->qpn & MGM_QPN_MASK) |
>  > -						       (1 << MGM_BLCK_LB_BIT));
>  > +						       (1U << MGM_BLCK_LB_BIT));
> 
> This is fixing a warning caused by a gcc bug 
> (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=37261) which is fixed 
> upstream.  The change is rather inoffensive but on the other hand I'm 
> not sure what our policy for dealing with version-specific warning 
> bugs in gcc should be.

the full commit is below - i researched it when i created it and indeed 
it seemed like an incorrect warning.

OTOH, there should be a well-defined work flow to keep this all 
manageable: once we know why a warning triggers and it has been 
categorized by a human, we should get rid of the warning in some way. 

Applying this patch as-is would be one option. Annotating it with a 
specific gcc version would be overkill i think. Ignoring it would be 
bad, because there's real value in standardizing on a "no warnings" 
build output. Many new warnings get introduced because people do not 
notice new warnings amongst the very high baseline noise of the kernel 
build.

What do you think?

	Ingo

--------->
>From 367bb845bef83d64cfee452a18a84fd65f21d401 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Mon, 18 Aug 2008 16:18:34 +0200
Subject: [PATCH] fix warning in drivers/net/mlx4/mcg.c
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

fix warning:

  drivers/net/mlx4/mcg.c: In function ‘mlx4_multicast_attach’:
  drivers/net/mlx4/mcg.c:217: warning: integer overflow in expression

there was no real danger of overflow here though.

md5:
   db8eb55620f886c03854a2abb2ce6c3f  mcg.o.before.asm
   db8eb55620f886c03854a2abb2ce6c3f  mcg.o.after.asm

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 drivers/net/mlx4/mcg.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/mlx4/mcg.c b/drivers/net/mlx4/mcg.c
index c83f88c..d1230a8 100644
--- a/drivers/net/mlx4/mcg.c
+++ b/drivers/net/mlx4/mcg.c
@@ -215,7 +215,7 @@ int mlx4_multicast_attach(struct mlx4_dev *dev, struct mlx4_qp *qp, u8 gid[16],
 
 	if (block_mcast_loopback)
 		mgm->qp[members_count++] = cpu_to_be32((qp->qpn & MGM_QPN_MASK) |
-						       (1 << MGM_BLCK_LB_BIT));
+						       (1U << MGM_BLCK_LB_BIT));
 	else
 		mgm->qp[members_count++] = cpu_to_be32(qp->qpn & MGM_QPN_MASK);
 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-17 18:05   ` Ingo Molnar
@ 2008-10-17 18:47     ` Roland Dreier
  2008-10-17 19:12       ` Ingo Molnar
  0 siblings, 1 reply; 23+ messages in thread
From: Roland Dreier @ 2008-10-17 18:47 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linus Torvalds, Andrew Morton, David S. Miller, Alan Cox,
	linux-kernel, Peter Zijlstra, Thomas Gleixner, H. Peter Anvin,
	David Howells

 > OTOH, there should be a well-defined work flow to keep this all 
 > manageable: once we know why a warning triggers and it has been 
 > categorized by a human, we should get rid of the warning in some way. 
 > 
 > Applying this patch as-is would be one option. Annotating it with a 
 > specific gcc version would be overkill i think. Ignoring it would be 
 > bad, because there's real value in standardizing on a "no warnings" 
 > build output. Many new warnings get introduced because people do not 
 > notice new warnings amongst the very high baseline noise of the kernel 
 > build.

The specific change I noticed:

 > -						       (1 << MGM_BLCK_LB_BIT));
 > +						       (1U << MGM_BLCK_LB_BIT));

is not a problem to me -- the code is fine either way, and if we're
making an effort to kill all warnings, then I'm OK with merging it.
It's a little unfortunate to add churn due to a gcc bug that is only in
certain 4.3 releases, but this particular case doesn't seem to trigger
in many places, so the cost is low.

However I worry about warnings produced by gcc bugs forcing us to tinker
with correct code.  Maybe it just makes sense to wait and see if we ever
hit a case where a gcc bug forces us to make too many stupid changes,
and figure out what to do if and when that happens.

 - R.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-17 18:47     ` Roland Dreier
@ 2008-10-17 19:12       ` Ingo Molnar
  2008-10-17 19:36         ` Roland Dreier
  0 siblings, 1 reply; 23+ messages in thread
From: Ingo Molnar @ 2008-10-17 19:12 UTC (permalink / raw)
  To: Roland Dreier
  Cc: Linus Torvalds, Andrew Morton, David S. Miller, Alan Cox,
	linux-kernel, Peter Zijlstra, Thomas Gleixner, H. Peter Anvin,
	David Howells


* Roland Dreier <rdreier@cisco.com> wrote:

> However I worry about warnings produced by gcc bugs forcing us to 
> tinker with correct code.  Maybe it just makes sense to wait and see 
> if we ever hit a case where a gcc bug forces us to make too many 
> stupid changes, and figure out what to do if and when that happens.

i certainly have a found a couple of such cases, see tip/warnings/ugly - 
for example see the one below where gcc is not able to see through type 
width.

the threshold i'm using is: "does the code get worse". Another question 
is the rate of really ugly patches. Had i ended up with a lot of them 
i'd be worried - but right now they are in the low single digits, for a 
full 9 MLOC kernel - which seems manageable.

the drivers/net/mlx4/mcg.c commit you pointed out is one of the very few 
borderline cases: the code gets neither better, nor worse. If you look 
at the totality of fixes they are not common at all. (and almost by 
definition the 100-200 unfixed warnings that we have piled up in -git 
are the _problematic_ cases - clear-cut cases tend to be fixed.)

	Ingo

------------->
>From fbf03326a16b29f8d34a5a3883a267bac4d38fc2 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Wed, 27 Aug 2008 12:44:00 +0200
Subject: [PATCH] hack, workaround for warning drivers/acpi/tables/tbfadt.c
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

ugly workaround for this warning:

  drivers/acpi/tables/tbfadt.c: In function ‘acpi_tb_create_local_fadt’:
  include/asm/string_32.h:75: warning: array subscript is above array bounds

gcc 4.3.1 20080801 (Red Hat 4.3.1-6)

its array checks are borked. Switch the string_32.h code to short instead.

NOT-Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/asm-x86/string_32.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/asm-x86/string_32.h b/include/asm-x86/string_32.h
index 487843e..419ab10 100644
--- a/include/asm-x86/string_32.h
+++ b/include/asm-x86/string_32.h
@@ -72,7 +72,7 @@ static __always_inline void *__constant_memcpy(void *to, const void *from,
 		return to;
 	case 5:
 		*(int *)to = *(int *)from;
-		*((char *)to + 4) = *((char *)from + 4);
+		*((short *)to + 3) = *((short *)from + 3);
 		return to;
 	case 6:
 		*(int *)to = *(int *)from;

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-17 19:12       ` Ingo Molnar
@ 2008-10-17 19:36         ` Roland Dreier
  2008-10-18  8:22           ` Ingo Molnar
  0 siblings, 1 reply; 23+ messages in thread
From: Roland Dreier @ 2008-10-17 19:36 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linus Torvalds, Andrew Morton, David S. Miller, Alan Cox,
	linux-kernel, Peter Zijlstra, Thomas Gleixner, H. Peter Anvin,
	David Howells

 > the drivers/net/mlx4/mcg.c commit you pointed out is one of the very few 
 > borderline cases: the code gets neither better, nor worse.

Yes, I agree exactly.  As long as there are not too many such cases
(since every commit has some cost just from causing churn) then we are
OK, I think.

 > If you look at the totality of fixes they are not common at all. (and
 > almost by definition the 100-200 unfixed warnings that we have piled
 > up in -git are the _problematic_ cases - clear-cut cases tend to be
 > fixed.)

Yes, and I think that merging such changes makes the most sense as part
of a project such as yours that wants to kill all warnings.  I looked at
the mcg.c warning and found the same workaround, but in the context of
my maintenance work, I just reported the gcc bug and lived with the
warning when using gcc 4.3.

By the way, just out of curiousity, how are you dealing with warnings
about "format not a string literal and no format arguments" caused by
code like arch/x86/kernel/dumpstack_64.c:

static void print_trace_address(void *data, unsigned long addr, int reliable)
{
	touch_nmi_watchdog();
	printk(data);
	printk_address(addr, reliable);
}

and also cases like:

	char *name;

	//...

	kobject_set_name(obj, name);

(I get these with gcc "(Ubuntu 4.3.2-1ubuntu10) 4.3.2")

 > i certainly have a found a couple of such cases, see tip/warnings/ugly - 
 > for example see the one below where gcc is not able to see through type 
 > width.

Yes, the uninitialized variable warnings are obnoxious too.  By the way,
I think this:

@@ -72,7 +72,7 @@ static __always_inline void *__constant_memcpy(void *to, const void *from,
 		return to;
 	case 5:
 		*(int *)to = *(int *)from;
-		*((char *)to + 4) = *((char *)from + 4);
+		*((short *)to + 3) = *((short *)from + 3);
 		return to;
 	case 6:
 		*(int *)to = *(int *)from;

is actually *wrong*, because the cast operator binds tighter than
addition -- so

+		*((short *)to + 3) = *((short *)from + 3);

actually copies bytes at offset 6 and 7; I think what you intended was:

+		*((short *)(to + 3)) = *((short *)(from + 3));

which illustrates the risks in fixing warnings.

 - R.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-17 17:11 [announce] new tree: "fix all build warnings, on all configs" Ingo Molnar
  2008-10-17 17:59 ` Roland Dreier
@ 2008-10-18  7:43 ` Andi Kleen
  2008-10-21 10:30   ` [announce] new tree: "fix all build warnings, on all configs" II Andi Kleen
  2008-10-21 11:17   ` [announce] new tree: "fix all build warnings, on all configs" Ingo Molnar
  1 sibling, 2 replies; 23+ messages in thread
From: Andi Kleen @ 2008-10-18  7:43 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linus Torvalds, Andrew Morton, David S. Miller, Alan Cox,
	linux-kernel, Peter Zijlstra, Thomas Gleixner, H. Peter Anvin,
	David Howells

Ingo Molnar <mingo@elte.hu> writes:
>  		if (battery->have_sysfs_alarm)
> diff --git a/drivers/acpi/sleep/main.c b/drivers/acpi/sleep/main.c
> index d13194a..2276d75 100644
> --- a/drivers/acpi/sleep/main.c
> +++ b/drivers/acpi/sleep/main.c
> @@ -63,7 +63,7 @@ void __init acpi_old_suspend_ordering(void)
>  /**
>   *	acpi_pm_disable_gpes - Disable the GPEs.
>   */
> -static int acpi_pm_disable_gpes(void)
> +static inline int acpi_pm_disable_gpes(void)

Just to satisfy my curiosity, what compiler warning does marking functions inline 
fix?

Thanks.

-Andi

-- 
ak@linux.intel.com

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-17 19:36         ` Roland Dreier
@ 2008-10-18  8:22           ` Ingo Molnar
  2008-10-20 16:37             ` Linus Torvalds
  0 siblings, 1 reply; 23+ messages in thread
From: Ingo Molnar @ 2008-10-18  8:22 UTC (permalink / raw)
  To: Roland Dreier
  Cc: Linus Torvalds, Andrew Morton, David S. Miller, Alan Cox,
	linux-kernel, Peter Zijlstra, Thomas Gleixner, H. Peter Anvin,
	David Howells


* Roland Dreier <rdreier@cisco.com> wrote:

>  > i certainly have a found a couple of such cases, see tip/warnings/ugly - 
>  > for example see the one below where gcc is not able to see through type 
>  > width.
> 
> Yes, the uninitialized variable warnings are obnoxious too.  By the way,
> I think this:
> 
> @@ -72,7 +72,7 @@ static __always_inline void *__constant_memcpy(void *to, const void *from,
>  		return to;
>  	case 5:
>  		*(int *)to = *(int *)from;
> -		*((char *)to + 4) = *((char *)from + 4);
> +		*((short *)to + 3) = *((short *)from + 3);
>  		return to;
>  	case 6:
>  		*(int *)to = *(int *)from;
> 
> is actually *wrong*, because the cast operator binds tighter than
> addition -- so
> 
> +		*((short *)to + 3) = *((short *)from + 3);
> 
> actually copies bytes at offset 6 and 7; I think what you intended was:
> 
> +		*((short *)(to + 3)) = *((short *)(from + 3));

thx, you are right - fixed it via the patch below.

> which illustrates the risks in fixing warnings.

yeah. Note that this was not a routine case at all, i did the commit in 
the early stages when i didnt even know how much effort it all would be 
to keep the whole kernel warning-free, in all configs. It looked odd and 
ugly and was in tip/warnings/ugly rightfully.

It would be nice if you could find an outright incorrect change in 
tip/warnings/simple. The ones flagged 'simple' are the ones that have 
the highest risk of not being reviewed much beyond their initial 
addition.

	Ingo

--------------->
>From 9d8f9578ca252bf26474ed77fde7ea30e9dee595 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Sat, 18 Oct 2008 10:17:36 +0200
Subject: [PATCH] hack, workaround for warning drivers/acpi/tables/tbfadt.c, fix

Fix commit fbf03326a16b29f8d34a5a3883a267bac4d38fc2, pointed
out by Roland Dreier.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/asm-x86/string_32.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/asm-x86/string_32.h b/include/asm-x86/string_32.h
index 419ab10..be82619 100644
--- a/include/asm-x86/string_32.h
+++ b/include/asm-x86/string_32.h
@@ -72,7 +72,7 @@ static __always_inline void *__constant_memcpy(void *to, const void *from,
 		return to;
 	case 5:
 		*(int *)to = *(int *)from;
-		*((short *)to + 3) = *((short *)from + 3);
+		*((char *)(to + 3)) = *((char *)(from + 3));
 		return to;
 	case 6:
 		*(int *)to = *(int *)from;

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-18  8:22           ` Ingo Molnar
@ 2008-10-20 16:37             ` Linus Torvalds
  2008-10-20 16:49               ` H. Peter Anvin
  2008-10-20 19:21               ` Ingo Molnar
  0 siblings, 2 replies; 23+ messages in thread
From: Linus Torvalds @ 2008-10-20 16:37 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Roland Dreier, Andrew Morton, David S. Miller, Alan Cox,
	linux-kernel, Peter Zijlstra, Thomas Gleixner, H. Peter Anvin,
	David Howells



On Sat, 18 Oct 2008, Ingo Molnar wrote:
> 
> thx, you are right - fixed it via the patch below.

Hell no.

The old code was correct. Your code is shit. And you didn't fix 
_anything_.

>  	case 5:
>  		*(int *)to = *(int *)from;
> -		*((short *)to + 3) = *((short *)from + 3);
> +		*((char *)(to + 3)) = *((char *)(from + 3));
>  		return to;

Are you just making changes by randomly inserting and deleting characters 
until you don't see warnings? Or what?

That thing is supposed to be a 5-byte memcpy. Not a "take a random byte 
from a random location and move it to another random location". That would 
be "randcpy()", not "memcpy()".

I don't want to see obvious and shitty crap like this. I don't want to 
pull from people who write code with some "random walk" algorithm.

F*ck me, what's wrong with you people? THAT CODE WAS NOT BUGGY. If it 
causes a warning, it is because SOME CALLER used a 5-byte memcpy() on 
something that gcc thought was just four bytes in size.

Ingo, I'm not going to pull _anything_ from you.

			Linus

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-20 16:37             ` Linus Torvalds
@ 2008-10-20 16:49               ` H. Peter Anvin
  2008-10-20 16:57                 ` Linus Torvalds
  2008-10-20 19:21               ` Ingo Molnar
  1 sibling, 1 reply; 23+ messages in thread
From: H. Peter Anvin @ 2008-10-20 16:49 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Ingo Molnar, Roland Dreier, Andrew Morton, David S. Miller,
	Alan Cox, linux-kernel, Peter Zijlstra, Thomas Gleixner,
	David Howells

Linus Torvalds wrote:
> 
> The old code was correct. Your code is shit. And you didn't fix 
> _anything_.
> 
>>  	case 5:
>>  		*(int *)to = *(int *)from;
>> -		*((short *)to + 3) = *((short *)from + 3);
>> +		*((char *)(to + 3)) = *((char *)(from + 3));
>>  		return to;
> 
> Are you just making changes by randomly inserting and deleting characters 
> until you don't see warnings? Or what?
> 
> That thing is supposed to be a 5-byte memcpy. Not a "take a random byte 
> from a random location and move it to another random location". That would 
> be "randcpy()", not "memcpy()".
> 

That is not a 5-byte memcopy.  In *either* version!

In the "before" case, it copies bytes 0, 1, 2, 3, 6 and 7.
In the "after" case, it copies bytes 0, 1 and 2.

Presumably it *should* be:

	*((char *)to + 4) = *((char *)from + 4);

	-hpa

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-20 16:49               ` H. Peter Anvin
@ 2008-10-20 16:57                 ` Linus Torvalds
  0 siblings, 0 replies; 23+ messages in thread
From: Linus Torvalds @ 2008-10-20 16:57 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: Ingo Molnar, Roland Dreier, Andrew Morton, David S. Miller,
	Alan Cox, linux-kernel, Peter Zijlstra, Thomas Gleixner,
	David Howells



On Mon, 20 Oct 2008, H. Peter Anvin wrote:
> 
> Presumably it *should* be:
> 
> 	*((char *)to + 4) = *((char *)from + 4);

That's what it is in the kenrel. The "before and after" versions were both 
from broken Ingo code.

		Linus

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-20 16:37             ` Linus Torvalds
  2008-10-20 16:49               ` H. Peter Anvin
@ 2008-10-20 19:21               ` Ingo Molnar
  2008-10-21  6:41                 ` Jörn Engel
  1 sibling, 1 reply; 23+ messages in thread
From: Ingo Molnar @ 2008-10-20 19:21 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Roland Dreier, Andrew Morton, David S. Miller, Alan Cox,
	linux-kernel, Peter Zijlstra, Thomas Gleixner, H. Peter Anvin,
	David Howells


* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> That thing is supposed to be a 5-byte memcpy. Not a "take a random 
> byte from a random location and move it to another random location". 
> That would be "randcpy()", not "memcpy()".
> 
> I don't want to see obvious and shitty crap like this. I don't want to 
> pull from people who write code with some "random walk" algorithm.

yes, the patch is worse than crap, so i:

 1) signalled that it's crap in its branch name (warnings/ugly)
 2) named it a hack in the subject line: "hack, workaround for warning"
 3) explained it in the commit log that GCC is crap
 4) added a NOT-Signed-off

but i guess i should not even have created it, because it shows a broken 
"symptom driven" thought process when it was created.

So i zapped the 'ugly' branch completely and removed all those commits. 
Will filter out bogus warnings via different technical means. Have no 
ideas at the moment of how to do it technically though - filtering the 
compiler output does not work reliably.

Below are the kind of commits i want to end up with eventually and 
reliably.

	Ingo

------------------>

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git warnings/simple

Ingo Molnar (10):
      x86: fix default_spin_lock_flags() prototype
      drivers/ide/pci/hpt366.c: remove unused variable
      drivers/net/wireless/b43/phy_g.c: type check debug printouts
      drivers/video/cirrusfb.c: remove unused variables
      fs/afs/dir.c: fix uninitialized variable use
      net/netlabel/netlabel_addrlist.c: add type checking to audit_log_format()
      include/linux/fs.h: improve type checking of __mandatory_lock()
      [vfs] fs.h: fops_get()/fops_put(): use pointer comparison
      net/mac80211/rc80211_minstrel_debugfs.c: fix return type
      sound/soc/codecs/tlv320aic23.c: remove unused variable


 arch/x86/kernel/paravirt-spinlocks.c    |    3 ++-
 drivers/ide/pci/hpt366.c                |    1 -
 drivers/net/wireless/b43/b43.h          |    3 ++-
 drivers/video/cirrusfb.c                |    3 +--
 fs/afs/dir.c                            |    2 +-
 include/linux/audit.h                   |    3 ++-
 include/linux/fs.h                      |    6 +++---
 net/mac80211/rc80211_minstrel_debugfs.c |    2 +-
 sound/soc/codecs/tlv320aic23.c          |    2 --
 9 files changed, 12 insertions(+), 13 deletions(-)

commit 54b1d646d442289d8d49e04bc2f10ba122ff6aa4
Author: Ingo Molnar <mingo@elte.hu>
Date:   Fri Oct 17 12:07:47 2008 +0200

    sound/soc/codecs/tlv320aic23.c: remove unused variable
    
    this warning:
    
      sound/soc/codecs/tlv320aic23.c: In function ‘tlv320aic23_set_dai_sysclk’:
      sound/soc/codecs/tlv320aic23.c:424: warning: unused variable ‘codec’
    
    triggers because this variable is not used in any form.
    
    Remove it.
    
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/sound/soc/codecs/tlv320aic23.c b/sound/soc/codecs/tlv320aic23.c
index 44308da..45395d0 100644
--- a/sound/soc/codecs/tlv320aic23.c
+++ b/sound/soc/codecs/tlv320aic23.c
@@ -421,8 +421,6 @@ static int tlv320aic23_set_dai_fmt(struct snd_soc_dai *codec_dai,
 static int tlv320aic23_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 				      int clk_id, unsigned int freq, int dir)
 {
-	struct snd_soc_codec *codec = codec_dai->codec;
-
 	switch (freq) {
 	case 12000000:
 		return 0;

commit 0be735b3ff71e13e24b82420f23a1b3b0a207ffb
Author: Ingo Molnar <mingo@elte.hu>
Date:   Fri Oct 17 12:56:23 2008 +0200

    net/mac80211/rc80211_minstrel_debugfs.c: fix return type
    
    this warning:
    
      net/mac80211/rc80211_minstrel_debugfs.c:145: warning: initialization from incompatible pointer type
    
    triggers because the proper return type for file_operations::read
    is ssize_t, not int.
    
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/net/mac80211/rc80211_minstrel_debugfs.c b/net/mac80211/rc80211_minstrel_debugfs.c
index 0b024cd..26f8ffc 100644
--- a/net/mac80211/rc80211_minstrel_debugfs.c
+++ b/net/mac80211/rc80211_minstrel_debugfs.c
@@ -106,7 +106,7 @@ minstrel_stats_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static int
+static ssize_t
 minstrel_stats_read(struct file *file, char __user *buf, size_t len, loff_t *o)
 {
 	struct minstrel_stats_info *ms;

commit 04271505e03535e1fd085c96085fcee41e7c415f
Author: Ingo Molnar <mingo@elte.hu>
Date:   Mon Aug 18 15:31:58 2008 +0200

    [vfs] fs.h: fops_get()/fops_put(): use pointer comparison
    
    this warning:
    
      drivers/infiniband/core/uverbs_main.c: In function ‘ib_uverbs_alloc_event_file’:
      drivers/infiniband/core/uverbs_main.c:526: warning: the address of ‘uverbs_event_fops’ will always evaluate as ‘true’
      drivers/infiniband/core/uverbs_main.c:526: warning: assignment discards qualifiers from pointer target type
    
    triggers because we use an arithmetic comparison. Use a pointer
    comparison instead.
    
    No code changed:
       91bef63ad2e661e7adb4456b944cec7d  uverbs_main.o.before.asm
       91bef63ad2e661e7adb4456b944cec7d  uverbs_main.o.after.asm
    
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 686d46c..114f469 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1597,9 +1597,9 @@ void unnamed_dev_init(void);
 
 /* Alas, no aliases. Too much hassle with bringing module.h everywhere */
 #define fops_get(fops) \
-	(((fops) && try_module_get((fops)->owner) ? (fops) : NULL))
+	(((fops != NULL) && try_module_get((fops)->owner) ? (fops) : NULL))
 #define fops_put(fops) \
-	do { if (fops) module_put((fops)->owner); } while(0)
+	do { if (fops != NULL) module_put((fops)->owner); } while(0)
 
 extern int register_filesystem(struct file_system_type *);
 extern int unregister_filesystem(struct file_system_type *);

commit f81ee5ca545020daf0ddfff3744199b87bbd8c70
Author: Ingo Molnar <mingo@elte.hu>
Date:   Fri Oct 17 14:54:48 2008 +0200

    include/linux/fs.h: improve type checking of __mandatory_lock()
    
    this warning:
    
      fs/gfs2/ops_file.c: In function ‘gfs2_flock’:
      fs/gfs2/ops_file.c:722: warning: unused variable ‘ip’
    
    triggers because __mandatory_lock() should not be a macro in the
    !CONFIG_FILE_LOCKING case but an inline. This improves the type
    checking and also does not trigger a warning.
    
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/include/linux/fs.h b/include/linux/fs.h
index a6a625b..686d46c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1675,7 +1675,7 @@ static inline int break_lease(struct inode *inode, unsigned int mode)
 #else /* !CONFIG_FILE_LOCKING */
 #define locks_mandatory_locked(a) ({ 0; })
 #define locks_mandatory_area(a, b, c, d, e) ({ 0; })
-#define __mandatory_lock(a) ({ 0; })
+static inline int __mandatory_lock(struct inode *ino) { return 0; }
 #define mandatory_lock(a) ({ 0; })
 #define locks_verify_locked(a) ({ 0; })
 #define locks_verify_truncate(a, b, c) ({ 0; })

commit 5baf34fc63c31ef676e0a764415b10e5cc1c7a4b
Author: Ingo Molnar <mingo@elte.hu>
Date:   Fri Oct 17 14:25:36 2008 +0200

    net/netlabel/netlabel_addrlist.c: add type checking to audit_log_format()
    
    this warning:
    
      net/netlabel/netlabel_addrlist.c: In function ‘netlbl_af4list_audit_addr’:
      net/netlabel/netlabel_addrlist.c:335: warning: unused variable ‘dir’
      net/netlabel/netlabel_addrlist.c: In function ‘netlbl_af6list_audit_addr’:
      net/netlabel/netlabel_addrlist.c:369: warning: unused variable ‘dir’
    
    is caused because audit_log_format() is a macro, hence in the
    !CONFIG_AUDIT case the compiler does not know that the variables are
    used.
    
    Convert it to a proper inline instead. This improves type checking
    in the !CONFIG_AUDIT case.
    
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 6272a39..a3f78d0 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -580,7 +580,8 @@ extern int audit_enabled;
 #define audit_log(c,g,t,f,...) do { ; } while (0)
 #define audit_log_start(c,g,t) ({ NULL; })
 #define audit_log_vformat(b,f,a) do { ; } while (0)
-#define audit_log_format(b,f,...) do { ; } while (0)
+static inline void __attribute__ ((format(printf, 2, 3)))
+audit_log_format(struct audit_buffer *ab, const char *fmt, ...) { }
 #define audit_log_end(b) do { ; } while (0)
 #define audit_log_n_hex(a,b,l) do { ; } while (0)
 #define audit_log_n_string(a,c,l) do { ; } while (0)

commit d58cbf52cb25e215c0e34048bda8ec481bdce4af
Author: Ingo Molnar <mingo@elte.hu>
Date:   Fri Oct 17 14:18:31 2008 +0200

    fs/afs/dir.c: fix uninitialized variable use
    
    this warning:
    
      fs/afs/dir.c: In function ‘afs_d_revalidate’:
      fs/afs/dir.c:566: warning: ‘fid.vnode’ may be used uninitialized in this function
      fs/afs/dir.c:566: warning: ‘fid.unique’ may be used uninitialized in this function
    
    shows us a real bug: afs_dir_get_page() could use the uninitialized
    fid variable if CONFIG_AFS_DEBUG=y to print messages, and leak kernel
    stack data to the console.
    
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/fs/afs/dir.c b/fs/afs/dir.c
index dfda03d..254c567 100644
--- a/fs/afs/dir.c
+++ b/fs/afs/dir.c
@@ -563,7 +563,7 @@ static struct dentry *afs_lookup(struct inode *dir, struct dentry *dentry,
 static int afs_d_revalidate(struct dentry *dentry, struct nameidata *nd)
 {
 	struct afs_vnode *vnode, *dir;
-	struct afs_fid fid;
+	struct afs_fid fid = { 0, };
 	struct dentry *parent;
 	struct key *key;
 	void *dir_version;

commit 19be5d76a014ce0e743848a42cbb3b579c0ad8d7
Author: Ingo Molnar <mingo@elte.hu>
Date:   Fri Oct 17 13:40:00 2008 +0200

    drivers/video/cirrusfb.c: remove unused variables
    
    fix these warnings:
    
      drivers/video/cirrusfb.c: In function ‘cirrusfb_setup’:
      drivers/video/cirrusfb.c:2466: warning: unused variable ‘i’
      drivers/video/cirrusfb.c:2465: warning: unused variable ‘s’
    
    These two parameters are not used anymore, their use got removed
    in this commit:
    
      a1d35a7: cirrusfb: use modedb and add mode_option parameter
    
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/drivers/video/cirrusfb.c b/drivers/video/cirrusfb.c
index 048b139..ace4001 100644
--- a/drivers/video/cirrusfb.c
+++ b/drivers/video/cirrusfb.c
@@ -2462,8 +2462,7 @@ static int __init cirrusfb_init(void)
 
 #ifndef MODULE
 static int __init cirrusfb_setup(char *options) {
-	char *this_opt, s[32];
-	int i;
+	char *this_opt;
 
 	DPRINTK("ENTER\n");
 

commit d138e44e2f8d0f26e04b0386d328d9cc7e33d82e
Author: Ingo Molnar <mingo@elte.hu>
Date:   Fri Oct 17 14:30:37 2008 +0200

    drivers/net/wireless/b43/phy_g.c: type check debug printouts
    
    this warning:
    
      drivers/net/wireless/b43/phy_g.c: In function ‘b43_gphy_op_recalc_txpower’:
      drivers/net/wireless/b43/phy_g.c:3191: warning: unused variable ‘dbm’
    
    is caused because b43dbg() is a macro, hence in the !B43_DEBUG
    case the compiler does not know that the variables are used.
    
    Convert it to a proper inline instead. This also improves type checking
    in the !B43_DEBUG case.
    
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
index 427b820..ac55b62 100644
--- a/drivers/net/wireless/b43/b43.h
+++ b/drivers/net/wireless/b43/b43.h
@@ -853,7 +853,8 @@ void b43warn(struct b43_wl *wl, const char *fmt, ...)
 void b43dbg(struct b43_wl *wl, const char *fmt, ...)
     __attribute__ ((format(printf, 2, 3)));
 #else /* DEBUG */
-# define b43dbg(wl, fmt...) do { /* nothing */ } while (0)
+static inline void __attribute__ ((format(printf, 2, 3)))
+b43dbg(struct b43_wl *wl, const char *fmt, ...) { }
 #endif /* DEBUG */
 
 /* A WARN_ON variant that vanishes when b43 debugging is disabled.

commit f11adf3c65aff25074d5d3a90d72cdfc40d66b50
Author: Ingo Molnar <mingo@elte.hu>
Date:   Fri Oct 17 12:54:56 2008 +0200

    drivers/ide/pci/hpt366.c: remove unused variable
    
    this warning:
    
      drivers/ide/pci/hpt366.c: In function ‘init_hwif_hpt366’:
      drivers/ide/pci/hpt366.c:1292: warning: unused variable ‘dev’
    
    triggers because this variable is not used in this function at all.
    
    Remov it.
    
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/drivers/ide/pci/hpt366.c b/drivers/ide/pci/hpt366.c
index 9cf171c..ca0acb5 100644
--- a/drivers/ide/pci/hpt366.c
+++ b/drivers/ide/pci/hpt366.c
@@ -1289,7 +1289,6 @@ static u8 hpt3xx_cable_detect(ide_hwif_t *hwif)
 
 static void __devinit init_hwif_hpt366(ide_hwif_t *hwif)
 {
-	struct pci_dev *dev	= to_pci_dev(hwif->dev);
 	struct hpt_info *info	= hpt3xx_get_info(hwif->dev);
 	int serialize		= HPT_SERIALIZE_IO;
 	u8  chip_type		= info->chip_type;

commit ecd05381e26b9a61e49fa485baea1595bd3d1b40
Author: Ingo Molnar <mingo@elte.hu>
Date:   Fri Oct 17 16:09:57 2008 +0200

    x86: fix default_spin_lock_flags() prototype
    
    these warnings:
    
      arch/x86/kernel/paravirt-spinlocks.c: In function ‘default_spin_lock_flags’:
      arch/x86/kernel/paravirt-spinlocks.c:12: warning: passing argument 1 of ‘__raw_spin_lock’ from incompatible pointer type
      arch/x86/kernel/paravirt-spinlocks.c: At top level:
      arch/x86/kernel/paravirt-spinlocks.c:11: warning: ‘default_spin_lock_flags’ defined but not used
    
    showed that the prototype of default_spin_lock_flags() was confused about
    what type spinlocks have.
    
    the proper type on UP is raw_spinlock_t.
    
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
index 0e9f198..95777b0 100644
--- a/arch/x86/kernel/paravirt-spinlocks.c
+++ b/arch/x86/kernel/paravirt-spinlocks.c
@@ -7,7 +7,8 @@
 
 #include <asm/paravirt.h>
 
-static void default_spin_lock_flags(struct raw_spinlock *lock, unsigned long flags)
+static inline void
+default_spin_lock_flags(raw_spinlock_t *lock, unsigned long flags)
 {
 	__raw_spin_lock(lock);
 }

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-20 19:21               ` Ingo Molnar
@ 2008-10-21  6:41                 ` Jörn Engel
  2008-10-22  9:47                   ` Ingo Molnar
  0 siblings, 1 reply; 23+ messages in thread
From: Jörn Engel @ 2008-10-21  6:41 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linus Torvalds, Roland Dreier, Andrew Morton, David S. Miller,
	Alan Cox, linux-kernel, Peter Zijlstra, Thomas Gleixner,
	H. Peter Anvin, David Howells

On Mon, 20 October 2008 21:21:10 +0200, Ingo Molnar wrote:
>  
>  /* Alas, no aliases. Too much hassle with bringing module.h everywhere */
>  #define fops_get(fops) \
> -	(((fops) && try_module_get((fops)->owner) ? (fops) : NULL))
> +	(((fops != NULL) && try_module_get((fops)->owner) ? (fops) : NULL))
>  #define fops_put(fops) \
> -	do { if (fops) module_put((fops)->owner); } while(0)
> +	do { if (fops != NULL) module_put((fops)->owner); } while(0)

This, I would argue, makes the code worse.

> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 6272a39..a3f78d0 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -580,7 +580,8 @@ extern int audit_enabled;
>  #define audit_log(c,g,t,f,...) do { ; } while (0)
>  #define audit_log_start(c,g,t) ({ NULL; })
>  #define audit_log_vformat(b,f,a) do { ; } while (0)
> -#define audit_log_format(b,f,...) do { ; } while (0)
> +static inline void __attribute__ ((format(printf, 2, 3)))
> +audit_log_format(struct audit_buffer *ab, const char *fmt, ...) { }

Took me a moment to notice the two lines aren't independent.  A tab
would have been appreciated.

> diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
> index 427b820..ac55b62 100644
> --- a/drivers/net/wireless/b43/b43.h
> +++ b/drivers/net/wireless/b43/b43.h
> @@ -853,7 +853,8 @@ void b43warn(struct b43_wl *wl, const char *fmt, ...)
>  void b43dbg(struct b43_wl *wl, const char *fmt, ...)
>      __attribute__ ((format(printf, 2, 3)));
>  #else /* DEBUG */
> -# define b43dbg(wl, fmt...) do { /* nothing */ } while (0)
> +static inline void __attribute__ ((format(printf, 2, 3)))
> +b43dbg(struct b43_wl *wl, const char *fmt, ...) { }

Dito.

Jörn

-- 
A victorious army first wins and then seeks battle.
-- Sun Tzu

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs" II
  2008-10-18  7:43 ` Andi Kleen
@ 2008-10-21 10:30   ` Andi Kleen
  2008-10-21 11:17   ` [announce] new tree: "fix all build warnings, on all configs" Ingo Molnar
  1 sibling, 0 replies; 23+ messages in thread
From: Andi Kleen @ 2008-10-21 10:30 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linus Torvalds, Andrew Morton, David S. Miller, Alan Cox,
	linux-kernel, Peter Zijlstra, Thomas Gleixner, H. Peter Anvin,
	David Howells

Andi Kleen <andi@firstfloor.org> writes:
>>  		if (battery->have_sysfs_alarm)
>> diff --git a/drivers/acpi/sleep/main.c b/drivers/acpi/sleep/main.c
>> index d13194a..2276d75 100644
>> --- a/drivers/acpi/sleep/main.c
>> +++ b/drivers/acpi/sleep/main.c
>> @@ -63,7 +63,7 @@ void __init acpi_old_suspend_ordering(void)
>>  /**
>>   *	acpi_pm_disable_gpes - Disable the GPEs.
>>   */
>> -static int acpi_pm_disable_gpes(void)
>> +static inline int acpi_pm_disable_gpes(void)
>
> Just to satisfy my curiosity, what compiler warning does marking functions inline 
> fix?

No reply. 

General note: ignoring review comments does not make the problems go away.

The reason I asked is that the patch is very likely wrong.

AFAIK the only warning that can be fixed by this inline would
be a linker section mismatch (that is why I asked).

But for linker section mismatch this is not the correct
change:

- inline is only advisory and gcc is free to disregard it.
So you could get the warning back any time.
- If you really want inlining for correctness you need
to use __always_inline
- Or if it's really to satisfy a linker section mismatch 
it's typically better to just declare all inlined functions
in the correct section, e.g. __init

Please fix this properly.

Thanks,
-Andi

-- 
ak@linux.intel.com

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-18  7:43 ` Andi Kleen
  2008-10-21 10:30   ` [announce] new tree: "fix all build warnings, on all configs" II Andi Kleen
@ 2008-10-21 11:17   ` Ingo Molnar
  2008-10-21 12:07     ` Andi Kleen
  2008-10-21 19:37     ` Rafael J. Wysocki
  1 sibling, 2 replies; 23+ messages in thread
From: Ingo Molnar @ 2008-10-21 11:17 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Linus Torvalds, Andrew Morton, David S. Miller, Alan Cox,
	linux-kernel, Peter Zijlstra, Thomas Gleixner, H. Peter Anvin,
	David Howells


* Andi Kleen <andi@firstfloor.org> wrote:

> >   *	acpi_pm_disable_gpes - Disable the GPEs.
> >   */
> > -static int acpi_pm_disable_gpes(void)
> > +static inline int acpi_pm_disable_gpes(void)
> 
> Just to satisfy my curiosity, what compiler warning does marking 
> functions inline fix?

the commit log below explains the situation. The warning exposed a maze 
of #ifdefs in drivers/acpi/sleep/main.c. It's not the warning we need to 
"fix" but that maze, obviously.

	Ingo

-------------------------------------------->
>From 6ddae344a73fcff60c840dd4e429bf55562b41f3 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Fri, 17 Oct 2008 15:44:22 +0200
Subject: [PATCH]  #ifdef complications in drivers/acpi/sleep/main.c

this warning:

  drivers/acpi/sleep/main.c:67: warning: ‘acpi_pm_disable_gpes’ defined but not used
  drivers/acpi/sleep/main.c:92: warning: ‘acpi_pm_prepare’ defined but not used
  drivers/acpi/sleep/main.c:107: warning: ‘acpi_pm_finish’ defined but not used
  drivers/acpi/sleep/main.c:128: warning: ‘acpi_pm_end’ defined but not used

Shows that this code has an identity crisis due to a maze of #ifdefs, in
the PM_SLEEP && !SUSPEND && !HIBERNATION case for example.

Instead of complicating the code with even more #ifdefs, one option
would be to convert these rather trivial wrappers to inline functions
(implemented in this commit). Maybe there's a better solution as well -
such as to reduce the many config options we have in this area?

No size difference with SUSPEND && HIBERNATION turned back on:

drivers/acpi/sleep/main.o:

   text	   data	    bss	    dec	    hex	filename
   2717	    976	     33	   3726	    e8e	main.o.before
   2717	    976	     33	   3726	    e8e	main.o.after

md5:
   44220906eff0ea6e1a3266b35dd82ac2  main.o.before.asm
   44220906eff0ea6e1a3266b35dd82ac2  main.o.after.asm
---
 drivers/acpi/sleep/main.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/sleep/main.c b/drivers/acpi/sleep/main.c
index d13194a..2276d75 100644
--- a/drivers/acpi/sleep/main.c
+++ b/drivers/acpi/sleep/main.c
@@ -63,7 +63,7 @@ void __init acpi_old_suspend_ordering(void)
 /**
  *	acpi_pm_disable_gpes - Disable the GPEs.
  */
-static int acpi_pm_disable_gpes(void)
+static inline int acpi_pm_disable_gpes(void)
 {
 	acpi_hw_disable_all_gpes();
 	return 0;
@@ -75,7 +75,7 @@ static int acpi_pm_disable_gpes(void)
  *	If necessary, set the firmware waking vector and do arch-specific
  *	nastiness to get the wakeup code to the waking vector.
  */
-static int __acpi_pm_prepare(void)
+static inline int __acpi_pm_prepare(void)
 {
 	int error = acpi_sleep_prepare(acpi_target_sleep_state);
 
@@ -88,7 +88,7 @@ static int __acpi_pm_prepare(void)
  *	acpi_pm_prepare - Prepare the platform to enter the target sleep
  *		state and disable the GPEs.
  */
-static int acpi_pm_prepare(void)
+static inline int acpi_pm_prepare(void)
 {
 	int error = __acpi_pm_prepare();
 
@@ -103,7 +103,7 @@ static int acpi_pm_prepare(void)
  *	This is called after we wake back up (or if entering the sleep state
  *	failed).
  */
-static void acpi_pm_finish(void)
+static inline void acpi_pm_finish(void)
 {
 	u32 acpi_state = acpi_target_sleep_state;
 
@@ -124,7 +124,7 @@ static void acpi_pm_finish(void)
 /**
  *	acpi_pm_end - Finish up suspend sequence.
  */
-static void acpi_pm_end(void)
+static inline void acpi_pm_end(void)
 {
 	/*
 	 * This is necessary in case acpi_pm_finish() is not called during a

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-21 11:17   ` [announce] new tree: "fix all build warnings, on all configs" Ingo Molnar
@ 2008-10-21 12:07     ` Andi Kleen
  2008-10-21 19:39       ` Rafael J. Wysocki
  2008-10-21 19:37     ` Rafael J. Wysocki
  1 sibling, 1 reply; 23+ messages in thread
From: Andi Kleen @ 2008-10-21 12:07 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Andi Kleen, Linus Torvalds, Andrew Morton, David S. Miller,
	Alan Cox, linux-kernel, Peter Zijlstra, Thomas Gleixner,
	H. Peter Anvin, David Howells

On Tue, Oct 21, 2008 at 01:17:16PM +0200, Ingo Molnar wrote:
> 
> * Andi Kleen <andi@firstfloor.org> wrote:
> 
> > >   *	acpi_pm_disable_gpes - Disable the GPEs.
> > >   */
> > > -static int acpi_pm_disable_gpes(void)
> > > +static inline int acpi_pm_disable_gpes(void)
> > 
> > Just to satisfy my curiosity, what compiler warning does marking 
> > functions inline fix?
> 
> the commit log below explains the situation. The warning exposed a maze 
> of #ifdefs in drivers/acpi/sleep/main.c. It's not the warning we need to 
> "fix" but that maze, obviously.

Thanks. That makes sense, 

I also agree with you that the better alternative would be 
to just always force SUSPEND together with ACPI.

I suspect the code delta wouldn't be very large compared to the rest
of the ACPI code.

-Andi


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-21 11:17   ` [announce] new tree: "fix all build warnings, on all configs" Ingo Molnar
  2008-10-21 12:07     ` Andi Kleen
@ 2008-10-21 19:37     ` Rafael J. Wysocki
  2008-10-22  4:11       ` Len Brown
  1 sibling, 1 reply; 23+ messages in thread
From: Rafael J. Wysocki @ 2008-10-21 19:37 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Andi Kleen, Linus Torvalds, Andrew Morton, David S. Miller,
	Alan Cox, linux-kernel, Peter Zijlstra, Thomas Gleixner,
	H. Peter Anvin, David Howells

On Tuesday, 21 of October 2008, Ingo Molnar wrote:
> 
> * Andi Kleen <andi@firstfloor.org> wrote:
> 
> > >   *	acpi_pm_disable_gpes - Disable the GPEs.
> > >   */
> > > -static int acpi_pm_disable_gpes(void)
> > > +static inline int acpi_pm_disable_gpes(void)
> > 
> > Just to satisfy my curiosity, what compiler warning does marking 
> > functions inline fix?
> 
> the commit log below explains the situation. The warning exposed a maze 
> of #ifdefs in drivers/acpi/sleep/main.c. It's not the warning we need to 
> "fix" but that maze, obviously.

Thanks a lot for _not_ CCing me. :-(
 
> -------------------------------------------->
> From 6ddae344a73fcff60c840dd4e429bf55562b41f3 Mon Sep 17 00:00:00 2001
> From: Ingo Molnar <mingo@elte.hu>
> Date: Fri, 17 Oct 2008 15:44:22 +0200
> Subject: [PATCH]  #ifdef complications in drivers/acpi/sleep/main.c
> 
> this warning:
> 
>   drivers/acpi/sleep/main.c:67: warning: ‘acpi_pm_disable_gpes’ defined but not used
>   drivers/acpi/sleep/main.c:92: warning: ‘acpi_pm_prepare’ defined but not used
>   drivers/acpi/sleep/main.c:107: warning: ‘acpi_pm_finish’ defined but not used
>   drivers/acpi/sleep/main.c:128: warning: ‘acpi_pm_end’ defined but not used
> 
> Shows that this code has an identity crisis due to a maze of #ifdefs, in
> the PM_SLEEP && !SUSPEND && !HIBERNATION case for example.

This case is invalid, because PM_SLEEP == SUSPEND || HIBERNATION !
 
I don't know how you managed to get
(PM_SLEEP && !SUSPEND && !HIBERNATION), but _that_ shouldn'd be possible in the
first place.

If there are warnings in any other case, please let me know and I'll fix them,
but please don't mess up with that code like this without letting me know.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-21 12:07     ` Andi Kleen
@ 2008-10-21 19:39       ` Rafael J. Wysocki
  0 siblings, 0 replies; 23+ messages in thread
From: Rafael J. Wysocki @ 2008-10-21 19:39 UTC (permalink / raw)
  To: Andi Kleen
  Cc: Ingo Molnar, Linus Torvalds, Andrew Morton, David S. Miller,
	Alan Cox, linux-kernel, Peter Zijlstra, Thomas Gleixner,
	H. Peter Anvin, David Howells

On Tuesday, 21 of October 2008, Andi Kleen wrote:
> On Tue, Oct 21, 2008 at 01:17:16PM +0200, Ingo Molnar wrote:
> > 
> > * Andi Kleen <andi@firstfloor.org> wrote:
> > 
> > > >   *	acpi_pm_disable_gpes - Disable the GPEs.
> > > >   */
> > > > -static int acpi_pm_disable_gpes(void)
> > > > +static inline int acpi_pm_disable_gpes(void)
> > > 
> > > Just to satisfy my curiosity, what compiler warning does marking 
> > > functions inline fix?
> > 
> > the commit log below explains the situation. The warning exposed a maze 
> > of #ifdefs in drivers/acpi/sleep/main.c. It's not the warning we need to 
> > "fix" but that maze, obviously.
> 
> Thanks. That makes sense, 
> 
> I also agree with you that the better alternative would be 
> to just always force SUSPEND together with ACPI.
> 
> I suspect the code delta wouldn't be very large compared to the rest
> of the ACPI code.

Is that _really_ necessary?

I mean, do the warnings appear in any case that's not theoretically impossible?

Rafael

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-21 19:37     ` Rafael J. Wysocki
@ 2008-10-22  4:11       ` Len Brown
  2008-10-22 12:23         ` [PATCH] ACPI suspend: Fix CONFIG_ACPI_SLEEP dependence and some compilation warnings (was: Re: [announce] new tree: "fix all build warnings, on all configs") Rafael J. Wysocki
  0 siblings, 1 reply; 23+ messages in thread
From: Len Brown @ 2008-10-22  4:11 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Ingo Molnar, Andi Kleen, Linus Torvalds, Andrew Morton,
	David S. Miller, Alan Cox, linux-kernel, Peter Zijlstra,
	Thomas Gleixner, H. Peter Anvin, David Howells

[-- Attachment #1: Type: TEXT/PLAIN, Size: 478 bytes --]



> >   drivers/acpi/sleep/main.c:67: warning: ‘acpi_pm_disable_gpes’ defined but not used

> > 
> > Shows that this code has an identity crisis due to a maze of #ifdefs, in
> > the PM_SLEEP && !SUSPEND && !HIBERNATION case for example.
> 
> This case is invalid, because PM_SLEEP == SUSPEND || HIBERNATION !

I though so too, but 2.6.27 appears to have added XEN to the mix:

config PM_SLEEP
        bool
        depends on SUSPEND || HIBERNATION || XEN_SAVE_RESTORE

-Len

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-21  6:41                 ` Jörn Engel
@ 2008-10-22  9:47                   ` Ingo Molnar
  2008-10-22 10:10                     ` Jörn Engel
  0 siblings, 1 reply; 23+ messages in thread
From: Ingo Molnar @ 2008-10-22  9:47 UTC (permalink / raw)
  To: Jörn Engel
  Cc: Linus Torvalds, Roland Dreier, Andrew Morton, David S. Miller,
	Alan Cox, linux-kernel, Peter Zijlstra, Thomas Gleixner,
	H. Peter Anvin, David Howells


* Jörn Engel <joern@logfs.org> wrote:

> On Mon, 20 October 2008 21:21:10 +0200, Ingo Molnar wrote:
> >  
> >  /* Alas, no aliases. Too much hassle with bringing module.h everywhere */
> >  #define fops_get(fops) \
> > -	(((fops) && try_module_get((fops)->owner) ? (fops) : NULL))
> > +	(((fops != NULL) && try_module_get((fops)->owner) ? (fops) : NULL))
> >  #define fops_put(fops) \
> > -	do { if (fops) module_put((fops)->owner); } while(0)
> > +	do { if (fops != NULL) module_put((fops)->owner); } while(0)
> 
> This, I would argue, makes the code worse.

Have a look at:

   $ git log -p --grep="NULL noise"

for example:

        for (i = 0; i < MAX_FEB_SIZE; i++)
-               if (tb->FEB[i] != 0)
+               if (tb->FEB[i] != NULL)
                        break;

so checking for != NULL is a valid way of testing a pointer's existence. 
The "if (tb->FEB[i])" is a valid shortcut for the same thing as well.

In this specific case the issue is that the 'fops' parameter can 
occasionally be a constant pointer (turning the test into always-true) 
so the compiler is at least minimally correct at asking the "are you 
sure you want this" question - which we answer in the affirmative via 
the explicit NULL check. But these are really nuances.

	Ingo

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [announce] new tree: "fix all build warnings, on all configs"
  2008-10-22  9:47                   ` Ingo Molnar
@ 2008-10-22 10:10                     ` Jörn Engel
  0 siblings, 0 replies; 23+ messages in thread
From: Jörn Engel @ 2008-10-22 10:10 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Linus Torvalds, Roland Dreier, Andrew Morton, David S. Miller,
	Alan Cox, linux-kernel, Peter Zijlstra, Thomas Gleixner,
	H. Peter Anvin, David Howells

On Wed, 22 October 2008 11:47:59 +0200, Ingo Molnar wrote:
> * Jörn Engel <joern@logfs.org> wrote:
> > On Mon, 20 October 2008 21:21:10 +0200, Ingo Molnar wrote:
> > >  
> > >  /* Alas, no aliases. Too much hassle with bringing module.h everywhere */
> > >  #define fops_get(fops) \
> > > -	(((fops) && try_module_get((fops)->owner) ? (fops) : NULL))
> > > +	(((fops != NULL) && try_module_get((fops)->owner) ? (fops) : NULL))
> > >  #define fops_put(fops) \
> > > -	do { if (fops) module_put((fops)->owner); } while(0)
> > > +	do { if (fops != NULL) module_put((fops)->owner); } while(0)
> > 
> > This, I would argue, makes the code worse.
> 
> In this specific case the issue is that the 'fops' parameter can 
> occasionally be a constant pointer (turning the test into always-true) 
> so the compiler is at least minimally correct at asking the "are you 
> sure you want this" question - which we answer in the affirmative via 
> the explicit NULL check. But these are really nuances.

#define fops_put(fops) do {			\
	if (fops != NULL)			\
		module_put((fops)->owner);	\
} while 0

If the code was written like this, I wouldn't mind your patch at all.
But in the one-liner form, the ' != NULL' makes the difference between
fluent reading and having to actively sort out which piece belongs
where.

And I bet that if it hadn't been a macro but an inline function, you
and checkpatch would have complained about the formatting long ago. ;)

Jörn

-- 
To announce that there must be no criticism of the President, or that we
are to stand by the President, right or wrong, is not only unpatriotic
and servile, but is morally treasonable to the American public.
-- Theodore Roosevelt, Kansas City Star, 1918

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH] ACPI suspend: Fix CONFIG_ACPI_SLEEP dependence and some compilation warnings (was: Re: [announce] new tree: "fix all build warnings, on all configs")
  2008-10-22  4:11       ` Len Brown
@ 2008-10-22 12:23         ` Rafael J. Wysocki
  2008-10-22 18:58           ` Len Brown
  0 siblings, 1 reply; 23+ messages in thread
From: Rafael J. Wysocki @ 2008-10-22 12:23 UTC (permalink / raw)
  To: Len Brown
  Cc: Ingo Molnar, Andi Kleen, Linus Torvalds, Andrew Morton,
	David S. Miller, Alan Cox, linux-kernel, Peter Zijlstra,
	Thomas Gleixner, H. Peter Anvin, David Howells,
	ACPI Devel Maling List, pm list

On Wednesday, 22 of October 2008, Len Brown wrote:
> 
> > >   drivers/acpi/sleep/main.c:67: warning: ‘acpi_pm_disable_gpes’ defined but not used
> 
> > > 
> > > Shows that this code has an identity crisis due to a maze of #ifdefs, in
> > > the PM_SLEEP && !SUSPEND && !HIBERNATION case for example.
> > 
> > This case is invalid, because PM_SLEEP == SUSPEND || HIBERNATION !
> 
> I though so too, but 2.6.27 appears to have added XEN to the mix:
> 
> config PM_SLEEP
>         bool
>         depends on SUSPEND || HIBERNATION || XEN_SAVE_RESTORE

Yeah, which breaks things. :-(

The patch below should fix it and BTW I think ACPI_SLEEP should not depend
on XEN_SAVE_RESTORE anyway, so this is a bug fix really (please consider as
.28 material, probably -stable too).

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>

ACPI suspend: Fix CONFIG_ACPI_SLEEP dependence and some compilation warnings

Initially CONFIG_PM_SLEEP was defined as
CONFIG_SUSPEND || CONFIG_HIBERNATION and some ACPI code, most
importantly the code in drivers/acpi/main.c, was written with this
assumption.  Currently, however, CONFIG_PM_SLEEP is also set when
CONFIG_XEN_SAVE_RESTORE is set.

This causes some compilation warnings to appear in
drivers/acpi/main.c if both CONFIG_SUSPEND and CONFIG_HIBERNATION
are unset and CONFIG_PM_SLEEP is set (this was impossible before).
To fix this problem, redefine CONFIG_ACPI_SLEEP do depend directly
on CONFIG_SUSPEND || CONFIG_HIBERNATION, as originally intended, and
use it instead of CONFIG_PM_SLEEP in drivers/acpi/main.c, wherever
appropriate.

Additionally, move the acpi_target_sleep_state definition from under
the #ifdef to prevent compilation from failing in some cases. 

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/Kconfig      |    2 +-
 drivers/acpi/sleep/main.c |    7 +++----
 2 files changed, 4 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/acpi/Kconfig
===================================================================
--- linux-2.6.orig/drivers/acpi/Kconfig
+++ linux-2.6/drivers/acpi/Kconfig
@@ -42,7 +42,7 @@ if ACPI
 
 config ACPI_SLEEP
 	bool
-	depends on PM_SLEEP
+	depends on SUSPEND || HIBERNATION
 	default y
 
 config ACPI_PROCFS
Index: linux-2.6/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.orig/drivers/acpi/sleep/main.c
+++ linux-2.6/drivers/acpi/sleep/main.c
@@ -23,6 +23,7 @@
 #include "sleep.h"
 
 u8 sleep_states[ACPI_S_STATE_COUNT];
+static u32 acpi_target_sleep_state = ACPI_STATE_S0;
 
 static int acpi_sleep_prepare(u32 acpi_state)
 {
@@ -45,9 +46,7 @@ static int acpi_sleep_prepare(u32 acpi_s
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static u32 acpi_target_sleep_state = ACPI_STATE_S0;
-
+#ifdef CONFIG_ACPI_SLEEP
 /*
  * ACPI 1.0 wants us to execute _PTS before suspending devices, so we allow the
  * user to request that behavior by using the 'acpi_old_suspend_ordering'
@@ -132,7 +131,7 @@ static void acpi_pm_end(void)
 	 */
 	acpi_target_sleep_state = ACPI_STATE_S0;
 }
-#endif /* CONFIG_PM_SLEEP */
+#endif /* CONFIG_ACPI_SLEEP */
 
 #ifdef CONFIG_SUSPEND
 extern void do_suspend_lowlevel(void);

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] ACPI suspend: Fix CONFIG_ACPI_SLEEP dependence and some compilation warnings (was: Re: [announce] new tree: "fix all build warnings, on all configs")
  2008-10-22 12:23         ` [PATCH] ACPI suspend: Fix CONFIG_ACPI_SLEEP dependence and some compilation warnings (was: Re: [announce] new tree: "fix all build warnings, on all configs") Rafael J. Wysocki
@ 2008-10-22 18:58           ` Len Brown
  0 siblings, 0 replies; 23+ messages in thread
From: Len Brown @ 2008-10-22 18:58 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Ingo Molnar, Andi Kleen, Linus Torvalds, Andrew Morton,
	David S. Miller, Alan Cox, linux-kernel, Peter Zijlstra,
	Thomas Gleixner, H. Peter Anvin, David Howells,
	ACPI Devel Maling List, pm list

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3641 bytes --]

applied.

thanks,
-Len

On Wed, 22 Oct 2008, Rafael J. Wysocki wrote:

> On Wednesday, 22 of October 2008, Len Brown wrote:
> > 
> > > >   drivers/acpi/sleep/main.c:67: warning: ‘acpi_pm_disable_gpes’ defined but not used
> > 
> > > > 
> > > > Shows that this code has an identity crisis due to a maze of #ifdefs, in
> > > > the PM_SLEEP && !SUSPEND && !HIBERNATION case for example.
> > > 
> > > This case is invalid, because PM_SLEEP == SUSPEND || HIBERNATION !
> > 
> > I though so too, but 2.6.27 appears to have added XEN to the mix:
> > 
> > config PM_SLEEP
> >         bool
> >         depends on SUSPEND || HIBERNATION || XEN_SAVE_RESTORE
> 
> Yeah, which breaks things. :-(
> 
> The patch below should fix it and BTW I think ACPI_SLEEP should not depend
> on XEN_SAVE_RESTORE anyway, so this is a bug fix really (please consider as
> .28 material, probably -stable too).
> 
> Thanks,
> Rafael
> 
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> ACPI suspend: Fix CONFIG_ACPI_SLEEP dependence and some compilation warnings
> 
> Initially CONFIG_PM_SLEEP was defined as
> CONFIG_SUSPEND || CONFIG_HIBERNATION and some ACPI code, most
> importantly the code in drivers/acpi/main.c, was written with this
> assumption.  Currently, however, CONFIG_PM_SLEEP is also set when
> CONFIG_XEN_SAVE_RESTORE is set.
> 
> This causes some compilation warnings to appear in
> drivers/acpi/main.c if both CONFIG_SUSPEND and CONFIG_HIBERNATION
> are unset and CONFIG_PM_SLEEP is set (this was impossible before).
> To fix this problem, redefine CONFIG_ACPI_SLEEP do depend directly
> on CONFIG_SUSPEND || CONFIG_HIBERNATION, as originally intended, and
> use it instead of CONFIG_PM_SLEEP in drivers/acpi/main.c, wherever
> appropriate.
> 
> Additionally, move the acpi_target_sleep_state definition from under
> the #ifdef to prevent compilation from failing in some cases. 
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/acpi/Kconfig      |    2 +-
>  drivers/acpi/sleep/main.c |    7 +++----
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/Kconfig
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/Kconfig
> +++ linux-2.6/drivers/acpi/Kconfig
> @@ -42,7 +42,7 @@ if ACPI
>  
>  config ACPI_SLEEP
>  	bool
> -	depends on PM_SLEEP
> +	depends on SUSPEND || HIBERNATION
>  	default y
>  
>  config ACPI_PROCFS
> Index: linux-2.6/drivers/acpi/sleep/main.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/sleep/main.c
> +++ linux-2.6/drivers/acpi/sleep/main.c
> @@ -23,6 +23,7 @@
>  #include "sleep.h"
>  
>  u8 sleep_states[ACPI_S_STATE_COUNT];
> +static u32 acpi_target_sleep_state = ACPI_STATE_S0;
>  
>  static int acpi_sleep_prepare(u32 acpi_state)
>  {
> @@ -45,9 +46,7 @@ static int acpi_sleep_prepare(u32 acpi_s
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
> -static u32 acpi_target_sleep_state = ACPI_STATE_S0;
> -
> +#ifdef CONFIG_ACPI_SLEEP
>  /*
>   * ACPI 1.0 wants us to execute _PTS before suspending devices, so we allow the
>   * user to request that behavior by using the 'acpi_old_suspend_ordering'
> @@ -132,7 +131,7 @@ static void acpi_pm_end(void)
>  	 */
>  	acpi_target_sleep_state = ACPI_STATE_S0;
>  }
> -#endif /* CONFIG_PM_SLEEP */
> +#endif /* CONFIG_ACPI_SLEEP */
>  
>  #ifdef CONFIG_SUSPEND
>  extern void do_suspend_lowlevel(void);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2008-10-22 18:59 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-10-17 17:11 [announce] new tree: "fix all build warnings, on all configs" Ingo Molnar
2008-10-17 17:59 ` Roland Dreier
2008-10-17 18:05   ` Ingo Molnar
2008-10-17 18:47     ` Roland Dreier
2008-10-17 19:12       ` Ingo Molnar
2008-10-17 19:36         ` Roland Dreier
2008-10-18  8:22           ` Ingo Molnar
2008-10-20 16:37             ` Linus Torvalds
2008-10-20 16:49               ` H. Peter Anvin
2008-10-20 16:57                 ` Linus Torvalds
2008-10-20 19:21               ` Ingo Molnar
2008-10-21  6:41                 ` Jörn Engel
2008-10-22  9:47                   ` Ingo Molnar
2008-10-22 10:10                     ` Jörn Engel
2008-10-18  7:43 ` Andi Kleen
2008-10-21 10:30   ` [announce] new tree: "fix all build warnings, on all configs" II Andi Kleen
2008-10-21 11:17   ` [announce] new tree: "fix all build warnings, on all configs" Ingo Molnar
2008-10-21 12:07     ` Andi Kleen
2008-10-21 19:39       ` Rafael J. Wysocki
2008-10-21 19:37     ` Rafael J. Wysocki
2008-10-22  4:11       ` Len Brown
2008-10-22 12:23         ` [PATCH] ACPI suspend: Fix CONFIG_ACPI_SLEEP dependence and some compilation warnings (was: Re: [announce] new tree: "fix all build warnings, on all configs") Rafael J. Wysocki
2008-10-22 18:58           ` Len Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).