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;
> 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.
* Roland Dreier <[email protected]> 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 <[email protected]>
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 <[email protected]>
---
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);
> 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.
* Roland Dreier <[email protected]> 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 <[email protected]>
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 <[email protected]>
---
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;
> 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.
Ingo Molnar <[email protected]> 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
--
[email protected]
* Roland Dreier <[email protected]> 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 <[email protected]>
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 <[email protected]>
---
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;
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
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
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
* Linus Torvalds <[email protected]> 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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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);
}
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
Andi Kleen <[email protected]> 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
--
[email protected]
* Andi Kleen <[email protected]> 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 <[email protected]>
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
On Tue, Oct 21, 2008 at 01:17:16PM +0200, Ingo Molnar wrote:
>
> * Andi Kleen <[email protected]> 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
On Tuesday, 21 of October 2008, Ingo Molnar wrote:
>
> * Andi Kleen <[email protected]> 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 <[email protected]>
> 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
On Tuesday, 21 of October 2008, Andi Kleen wrote:
> On Tue, Oct 21, 2008 at 01:17:16PM +0200, Ingo Molnar wrote:
> >
> > * Andi Kleen <[email protected]> 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
> > 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
* J?rn Engel <[email protected]> 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
On Wed, 22 October 2008 11:47:59 +0200, Ingo Molnar wrote:
> * Jörn Engel <[email protected]> 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
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 <[email protected]>
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 <[email protected]>
---
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);
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 <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>