[PATCH 1/6] Werror: Fix casting wrong size integer to pointer

[PATCH 1/6] Werror: Fix casting wrong size integer to pointer

[David Howells]

Fix casting wrong size integer to pointer.  Smaller integers must be cast to
unsigned long then cast to a pointer.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 drivers/char/drm/sis_mm.c        |    2 +-
 drivers/message/i2o/i2o_config.c |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)


diff --git a/drivers/char/drm/sis_mm.c b/drivers/char/drm/sis_mm.c
index b387877..378b275 100644
--- a/drivers/char/drm/sis_mm.c
+++ b/drivers/char/drm/sis_mm.c
@@ -57,7 +57,7 @@ static void *sis_sman_mm_allocate(void *private, unsigned long size,
 	if (req.size == 0)
 		return NULL;
 	else
-		return (void *)~req.offset;
+		return (void *)(unsigned long)~req.offset;
 }
 
 static void sis_sman_mm_free(void *private, void *ref)
diff --git a/drivers/message/i2o/i2o_config.c b/drivers/message/i2o/i2o_config.c
index c0fb77d..a1c83af 100644
--- a/drivers/message/i2o/i2o_config.c
+++ b/drivers/message/i2o/i2o_config.c
@@ -886,7 +886,7 @@ static int i2o_cfg_passthru(unsigned long arg)
 			    flag_count & 0x04000000 /*I2O_SGL_FLAGS_DIR */ ) {
 				// TODO 64bit fix
 				if (copy_from_user
-				    (p, (void __user *)sg[i].addr_bus,
+				    (p, (void __user *)(unsigned long)sg[i].addr_bus,
 				     sg_size)) {
 					printk(KERN_DEBUG
 					       "%s: Could not copy SG buf %d FROM user\n",
@@ -942,7 +942,7 @@ static int i2o_cfg_passthru(unsigned long arg)
 				sg_size = sg[j].flag_count & 0xffffff;
 				// TODO 64bit fix
 				if (copy_to_user
-				    ((void __user *)sg[j].addr_bus, sg_list[j],
+				    ((void __user *)(unsigned long)sg[j].addr_bus, sg_list[j],
 				     sg_size)) {
 					printk(KERN_WARNING
 					       "%s: Could not copy %p TO user %x\n",

[PATCH 2/6] Werror: Hide epca_setup() as it's unused

[David Howells]

Hide epca_setup() by encasing it in #if 0 as it's unused.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 drivers/char/epca.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)


diff --git a/drivers/char/epca.c b/drivers/char/epca.c
index 60a4df7..0d3f685 100644
--- a/drivers/char/epca.c
+++ b/drivers/char/epca.c
@@ -186,7 +186,9 @@ static void pc_throttle(struct tty_struct *tty);
 static void pc_unthrottle(struct tty_struct *tty);
 static void digi_send_break(struct channel *ch, int msec);
 static void setup_empty_event(struct tty_struct *tty, struct channel *ch);
+#if 0 // UNUSED
 static void epca_setup(char *, int *);
+#endif
 
 static int pc_write(struct tty_struct *, const unsigned char *, int);
 static int pc_init(void);
@@ -2538,6 +2540,7 @@ static void setup_empty_event(struct tty_struct *tty, struct channel *ch)
 	memoff(ch);
 }
 
+#if 0 // UNUSED
 static void epca_setup(char *str, int *ints)
 {
 	struct board_info board;
@@ -2791,6 +2794,7 @@ static void epca_setup(char *str, int *ints)
 		board.numports, (int)board.port, (unsigned int) board.membase);
 	num_cards++;
 }
+#endif
 
 enum epic_board_types {
 	brd_xr = 0,

Re: [PATCH 2/6] Werror: Hide epca_setup() as it's unused

[Alan Cox]

On Thu, 29 May 2008 19:17:31 +0100
David Howells <dhowells@redhat.com> wrote:

> Hide epca_setup() by encasing it in #if 0 as it's unused.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>

NAK - we don't hide bugs, we fix em.

EPCA setup either needs calling, or as I suspect is the case epca needs
deleting. Hiding the warning isn't useful

[PATCH 3/6] Werror: Remove iiEllisCleanup() as it's unused.

[David Howells]

Remove iiEllisCleanup() as it's unused.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 drivers/char/ip2/i2ellis.c |   16 ----------------
 1 files changed, 0 insertions(+), 16 deletions(-)


diff --git a/drivers/char/ip2/i2ellis.c b/drivers/char/ip2/i2ellis.c
index 3601017..a7c89d8 100644
--- a/drivers/char/ip2/i2ellis.c
+++ b/drivers/char/ip2/i2ellis.c
@@ -85,22 +85,6 @@ iiEllisInit(void)
 }
 
 //******************************************************************************
-// Function:   iiEllisCleanup()
-// Parameters: None
-//
-// Returns:    Nothing
-//
-// Description:
-//
-// This routine performs any required cleanup of the iiEllis subsystem.
-//
-//******************************************************************************
-static void
-iiEllisCleanup(void)
-{
-}
-
-//******************************************************************************
 // Function:   iiSetAddress(pB, address, delay)
 // Parameters: pB      - pointer to the board structure
 //             address - the purported I/O address of the board

Re: [PATCH 3/6] Werror: Remove iiEllisCleanup() as it's unused.

[Adrian Bunk]

On Thu, May 29, 2008 at 07:17:36PM +0100, David Howells wrote:
> Remove iiEllisCleanup() as it's unused.


It is used with CONFIG_COMPUTONE=m - this empty function is kinda 
pointless but you also have to remove the caller from ip2main.c if
you want to remove it.


> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  drivers/char/ip2/i2ellis.c |   16 ----------------
>  1 files changed, 0 insertions(+), 16 deletions(-)
> 
> 
> diff --git a/drivers/char/ip2/i2ellis.c b/drivers/char/ip2/i2ellis.c
> index 3601017..a7c89d8 100644
> --- a/drivers/char/ip2/i2ellis.c
> +++ b/drivers/char/ip2/i2ellis.c
> @@ -85,22 +85,6 @@ iiEllisInit(void)
>  }
>  
>  //******************************************************************************
> -// Function:   iiEllisCleanup()
> -// Parameters: None
> -//
> -// Returns:    Nothing
> -//
> -// Description:
> -//
> -// This routine performs any required cleanup of the iiEllis subsystem.
> -//
> -//******************************************************************************
> -static void
> -iiEllisCleanup(void)
> -{
> -}
> -
> -//******************************************************************************
>  // Function:   iiSetAddress(pB, address, delay)
>  // Parameters: pB      - pointer to the board structure
>  //             address - the purported I/O address of the board

cu
Adrian

-- 

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

Re: [PATCH 3/6] Werror: Remove iiEllisCleanup() as it's unused.

[David Howells]

Adrian Bunk <bunk@kernel.org> wrote:

> It is used with CONFIG_COMPUTONE=m - this empty function is kinda 
> pointless but you also have to remove the caller from ip2main.c if
> you want to remove it.

Ah...  I see.  i2ellis.c is #included by ip2main.c, but iiEllisCleanup() isn't
called unless actually in a module.

David

Re: [PATCH 3/6] Werror: Remove iiEllisCleanup() as it's unused.

[Alan Cox]

On Thu, 29 May 2008 19:17:36 +0100
David Howells <dhowells@redhat.com> wrote:

> Remove iiEllisCleanup() as it's unused.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>

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

Surplus from the merge of the two bits

[PATCH 4/6] Werror: Hide warnings on static module device tables

[David Howells]

Hide warnings on static module device tables that are produced when compiling a
driver in to the core kernel rather than compiling it as a module.

This is done by introducing a MODULE_STATIC_DEVICE_TABLE() version of
MODULE_DEVICE_TABLE() for device ID tables that are declared static.  This
tells the compiler that the table is unused, thus suppressing warnings of the
type:

	mod.c:37: warning: 'id_table' defined but not used

MODULE_STATIC_DEVICE_TABLE() should not be used for non-static tables.

A MODULE_STATIC_GENERIC_TABLE() is also added for the same reason as a version
of MODULE_GENERIC_TABLE().

This does not automatically cause the table to be emitted by the compiler; that
will only happen if something references it.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 drivers/char/ip2/ip2main.c       |    2 +-
 drivers/char/rocket.c            |    2 +-
 drivers/char/specialix.c         |    2 +-
 drivers/isdn/hisax/config.c      |    2 +-
 drivers/media/video/zoran_card.c |    2 +-
 drivers/scsi/dpt_i2o.c           |    2 +-
 drivers/scsi/fdomain.c           |    2 +-
 drivers/scsi/initio.c            |    2 +-
 drivers/telephony/ixj.c          |    2 +-
 drivers/watchdog/alim1535_wdt.c  |    2 +-
 drivers/watchdog/alim7101_wdt.c  |    2 +-
 include/linux/module.h           |   28 +++++++++++++++++++++++-----
 sound/oss/ad1848.c               |    2 +-
 13 files changed, 35 insertions(+), 17 deletions(-)


diff --git a/drivers/char/ip2/ip2main.c b/drivers/char/ip2/ip2main.c
index c12cf8f..0580122 100644
--- a/drivers/char/ip2/ip2main.c
+++ b/drivers/char/ip2/ip2main.c
@@ -3181,4 +3181,4 @@ static struct pci_device_id ip2main_pci_tbl[] __devinitdata = {
 	{ }
 };
 
-MODULE_DEVICE_TABLE(pci, ip2main_pci_tbl);
+MODULE_STATIC_DEVICE_TABLE(pci, ip2main_pci_tbl);
diff --git a/drivers/char/rocket.c b/drivers/char/rocket.c
index 743dc80..27155ce 100644
--- a/drivers/char/rocket.c
+++ b/drivers/char/rocket.c
@@ -1872,7 +1872,7 @@ static struct pci_device_id __devinitdata rocket_pci_ids[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_RP, PCI_ANY_ID) },
 	{ }
 };
-MODULE_DEVICE_TABLE(pci, rocket_pci_ids);
+MODULE_STATIC_DEVICE_TABLE(pci, rocket_pci_ids);
 
 /*
  *  Called when a PCI card is found.  Retrieves and stores model information,
diff --git a/drivers/char/specialix.c b/drivers/char/specialix.c
index 2ee4d98..83b3968 100644
--- a/drivers/char/specialix.c
+++ b/drivers/char/specialix.c
@@ -2451,7 +2451,7 @@ static struct pci_device_id specialx_pci_tbl[] __devinitdata = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_SPECIALIX, PCI_DEVICE_ID_SPECIALIX_IO8) },
 	{ }
 };
-MODULE_DEVICE_TABLE(pci, specialx_pci_tbl);
+MODULE_STATIC_DEVICE_TABLE(pci, specialx_pci_tbl);
 
 module_init(specialix_init_module);
 module_exit(specialix_exit_module);
diff --git a/drivers/isdn/hisax/config.c b/drivers/isdn/hisax/config.c
index 84d75a3..63932c5 100644
--- a/drivers/isdn/hisax/config.c
+++ b/drivers/isdn/hisax/config.c
@@ -1970,7 +1970,7 @@ static struct pci_device_id hisax_pci_tbl[] __devinitdata = {
 	{ }				/* Terminating entry */
 };
 
-MODULE_DEVICE_TABLE(pci, hisax_pci_tbl);
+MODULE_STATIC_DEVICE_TABLE(pci, hisax_pci_tbl);
 #endif /* CONFIG_PCI */
 
 module_init(HiSax_init);
diff --git a/drivers/media/video/zoran_card.c b/drivers/media/video/zoran_card.c
index 006d488..231fa72 100644
--- a/drivers/media/video/zoran_card.c
+++ b/drivers/media/video/zoran_card.c
@@ -160,7 +160,7 @@ static struct pci_device_id zr36067_pci_tbl[] = {
 	 PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
 	{0}
 };
-MODULE_DEVICE_TABLE(pci, zr36067_pci_tbl);
+MODULE_STATIC_DEVICE_TABLE(pci, zr36067_pci_tbl);
 
 int zoran_num;			/* number of Buzs in use */
 struct zoran zoran[BUZ_MAX];
diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index 8508816..f98ce25 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -182,7 +182,7 @@ static struct pci_device_id dptids[] = {
 	{ PCI_DPT_VENDOR_ID, PCI_DPT_RAPTOR_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID,},
 	{ 0, }
 };
-MODULE_DEVICE_TABLE(pci,dptids);
+MODULE_STATIC_DEVICE_TABLE(pci,dptids);
 
 static int adpt_detect(struct scsi_host_template* sht)
 {
diff --git a/drivers/scsi/fdomain.c b/drivers/scsi/fdomain.c
index c33bcb2..d7c3752 100644
--- a/drivers/scsi/fdomain.c
+++ b/drivers/scsi/fdomain.c
@@ -1772,7 +1772,7 @@ static struct pci_device_id fdomain_pci_tbl[] __devinitdata = {
 	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0UL },
 	{ }
 };
-MODULE_DEVICE_TABLE(pci, fdomain_pci_tbl);
+MODULE_STATIC_DEVICE_TABLE(pci, fdomain_pci_tbl);
 #endif
 #define driver_template fdomain_driver_template
 #include "scsi_module.c"
diff --git a/drivers/scsi/initio.c b/drivers/scsi/initio.c
index e3f7397..3d6bbc0 100644
--- a/drivers/scsi/initio.c
+++ b/drivers/scsi/initio.c
@@ -136,7 +136,7 @@ static struct pci_device_id i91u_pci_devices[] = {
 	{ PCI_VENDOR_ID_DOMEX, I920_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0},
 	{ }
 };
-MODULE_DEVICE_TABLE(pci, i91u_pci_devices);
+MODULE_STATIC_DEVICE_TABLE(pci, i91u_pci_devices);
 
 #define DEBUG_INTERRUPT 0
 #define DEBUG_QUEUE     0
diff --git a/drivers/telephony/ixj.c b/drivers/telephony/ixj.c
index 49cd979..1eea44f 100644
--- a/drivers/telephony/ixj.c
+++ b/drivers/telephony/ixj.c
@@ -290,7 +290,7 @@ static struct pci_device_id ixj_pci_tbl[] __devinitdata = {
 	{ }
 };
 
-MODULE_DEVICE_TABLE(pci, ixj_pci_tbl);
+MODULE_STATIC_DEVICE_TABLE(pci, ixj_pci_tbl);
 
 /************************************************************************
 *
diff --git a/drivers/watchdog/alim1535_wdt.c b/drivers/watchdog/alim1535_wdt.c
index 2b1fbdb..fedf9b9 100644
--- a/drivers/watchdog/alim1535_wdt.c
+++ b/drivers/watchdog/alim1535_wdt.c
@@ -316,7 +316,7 @@ static struct pci_device_id ali_pci_tbl[] = {
 	{ PCI_VENDOR_ID_AL, 0x1535, PCI_ANY_ID, PCI_ANY_ID,},
 	{ 0, },
 };
-MODULE_DEVICE_TABLE(pci, ali_pci_tbl);
+MODULE_STATIC_DEVICE_TABLE(pci, ali_pci_tbl);
 
 /*
  *	ali_find_watchdog	-	find a 1535 and 7101
diff --git a/drivers/watchdog/alim7101_wdt.c b/drivers/watchdog/alim7101_wdt.c
index 238273c..b8fccd2 100644
--- a/drivers/watchdog/alim7101_wdt.c
+++ b/drivers/watchdog/alim7101_wdt.c
@@ -415,7 +415,7 @@ static struct pci_device_id alim7101_pci_tbl[] __devinitdata = {
 	{ }
 };
 
-MODULE_DEVICE_TABLE(pci, alim7101_pci_tbl);
+MODULE_STATIC_DEVICE_TABLE(pci, alim7101_pci_tbl);
 
 MODULE_AUTHOR("Steve Hill");
 MODULE_DESCRIPTION("ALi M7101 PMU Computer Watchdog Timer driver");
diff --git a/include/linux/module.h b/include/linux/module.h
index 3e03b1a..9e1b041 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -76,18 +76,34 @@ void sort_extable(struct exception_table_entry *start,
 		  struct exception_table_entry *finish);
 void sort_main_extable(void);
 
+/*
+ * Return a pointer to the current module, but only if within a module
+ */
 #ifdef MODULE
-#define MODULE_GENERIC_TABLE(gtype,name)			\
-extern const struct gtype##_id __mod_##gtype##_table		\
-  __attribute__ ((unused, alias(__stringify(name))))
-
 extern struct module __this_module;
 #define THIS_MODULE (&__this_module)
 #else  /* !MODULE */
-#define MODULE_GENERIC_TABLE(gtype,name)
 #define THIS_MODULE ((struct module *)0)
 #endif
 
+/*
+ * Declare a module table
+ * - this suppresses "'name' defined but not used" warnings from the compiler
+ *   as the table may not actually be used by the code within the module
+ */
+#ifdef MODULE
+#define MODULE_GENERIC_TABLE(gtype,name)			\
+extern const struct gtype##_id __mod_##gtype##_table		\
+  __attribute__ ((unused, alias(__stringify(name))))
+#define MODULE_STATIC_GENERIC_TABLE(gtype,name)			\
+extern const struct gtype##_id __mod_##gtype##_table		\
+  __attribute__ ((unused, alias(__stringify(name))))
+#else
+#define MODULE_GENERIC_TABLE(gtype,name)
+#define MODULE_STATIC_GENERIC_TABLE(gtype,name) \
+static __typeof__((name)) name __attribute__((unused));
+#endif
+
 /* Generic info of form tag = "info" */
 #define MODULE_INFO(tag, info) __MODULE_INFO(tag, tag, info)
 
@@ -137,6 +153,8 @@ extern struct module __this_module;
 
 #define MODULE_DEVICE_TABLE(type,name)		\
   MODULE_GENERIC_TABLE(type##_device,name)
+#define MODULE_STATIC_DEVICE_TABLE(type,name)		\
+  MODULE_STATIC_GENERIC_TABLE(type##_device,name)
 
 /* Version of form [<epoch>:]<version>[-<extra-version>].
    Or for CVS/RCS ID version, everything but the number is stripped.
diff --git a/sound/oss/ad1848.c b/sound/oss/ad1848.c
index 7cf9913..2e586f4 100644
--- a/sound/oss/ad1848.c
+++ b/sound/oss/ad1848.c
@@ -2879,7 +2879,7 @@ static struct isapnp_device_id id_table[] __devinitdata = {
 	{0}
 };
 
-MODULE_DEVICE_TABLE(isapnp, id_table);
+MODULE_STATIC_DEVICE_TABLE(isapnp, id_table);
 
 static struct pnp_dev *activate_dev(char *devname, char *resname, struct pnp_dev *dev)
 {

Re: [PATCH 4/6] Werror: Hide warnings on static module device tables

[Adrian Bunk]

On Thu, May 29, 2008 at 07:17:41PM +0100, David Howells wrote:
> Hide warnings on static module device tables that are produced when compiling a
> driver in to the core kernel rather than compiling it as a module.
> 
> This is done by introducing a MODULE_STATIC_DEVICE_TABLE() version of
> MODULE_DEVICE_TABLE() for device ID tables that are declared static.  This
> tells the compiler that the table is unused, thus suppressing warnings of the
> type:
> 
> 	mod.c:37: warning: 'id_table' defined but not used
> 
> MODULE_STATIC_DEVICE_TABLE() should not be used for non-static tables.
> 
> A MODULE_STATIC_GENERIC_TABLE() is also added for the same reason as a version
> of MODULE_GENERIC_TABLE().
> 
> This does not automatically cause the table to be emitted by the compiler; that
> will only happen if something references it.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  drivers/char/ip2/ip2main.c       |    2 +-
>  drivers/char/rocket.c            |    2 +-
>  drivers/char/specialix.c         |    2 +-
>  drivers/isdn/hisax/config.c      |    2 +-
>  drivers/media/video/zoran_card.c |    2 +-
>  drivers/scsi/dpt_i2o.c           |    2 +-
>  drivers/scsi/fdomain.c           |    2 +-
>  drivers/scsi/initio.c            |    2 +-
>  drivers/telephony/ixj.c          |    2 +-
>  drivers/watchdog/alim1535_wdt.c  |    2 +-
>  drivers/watchdog/alim7101_wdt.c  |    2 +-
>  include/linux/module.h           |   28 +++++++++++++++++++++++-----
>  sound/oss/ad1848.c               |    2 +-
>...

Isn't the warning often a reminder that a driver should be updated to 
use pci_register_driver() etc. ?

cu
Adrian

-- 

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

Re: [PATCH 4/6] Werror: Hide warnings on static module device tables

[David Howells]

Adrian Bunk <bunk@kernel.org> wrote:

> Isn't the warning often a reminder that a driver should be updated to 
> use pci_register_driver() etc. ?

If it is, it's not mentioned in Documentation/ or at the definition of
MODULE_DEVICE_TABLE.

Also, does that apply to the isapnp driver?

David

Re: [PATCH 4/6] Werror: Hide warnings on static module device tables

[Adrian Bunk]

On Thu, May 29, 2008 at 08:41:47PM +0100, David Howells wrote:
> Adrian Bunk <bunk@kernel.org> wrote:
> 
> > Isn't the warning often a reminder that a driver should be updated to 
> > use pci_register_driver() etc. ?
> 
> If it is, it's not mentioned in Documentation/ or at the definition of
> MODULE_DEVICE_TABLE.

It might not be intentionally, but the warning goes away when you stuff 
the table into a struct pci_driver.

> Also, does that apply to the isapnp driver?

There's struct pnp_driver together with pnp_register_driver() etc.

> David

cu
Adrian

-- 

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

[PATCH 5/6] Werror: Remove the packed attribute from PofTimStamp_tag in the hysdn driver

[David Howells]

Remove the packed attribute from PofTimStamp_tag in the hysdn driver as the
thing being packed is just an array of chars.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 drivers/isdn/hysdn/hysdn_pof.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)


diff --git a/drivers/isdn/hysdn/hysdn_pof.h b/drivers/isdn/hysdn/hysdn_pof.h
index a368d6c..3a72b90 100644
--- a/drivers/isdn/hysdn/hysdn_pof.h
+++ b/drivers/isdn/hysdn/hysdn_pof.h
@@ -60,7 +60,7 @@ typedef struct PofRecHdr_tag {	/* Pof record header */
 
 typedef struct PofTimeStamp_tag {
 /*00 */ unsigned long UnixTime __attribute__((packed));
-	/*04 */ unsigned char DateTimeText[0x28] __attribute__((packed));
+	/*04 */ unsigned char DateTimeText[0x28];
 	/* =40 */
 /*2C */
 } tPofTimeStamp;

Re: [PATCH 5/6] Werror: Remove the packed attribute from PofTimStamp_tag in the hysdn driver

[Andrew Morton]

On Thu, 29 May 2008 19:17:46 +0100 David Howells <dhowells@redhat.com> wrote:

> Remove the packed attribute from PofTimStamp_tag in the hysdn driver as the
> thing being packed is just an array of chars.

When fixing warnings and build errors, please do quote the compiler
output in the changelog.  Ditto sparse, etc.

> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  drivers/isdn/hysdn/hysdn_pof.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> 
> diff --git a/drivers/isdn/hysdn/hysdn_pof.h b/drivers/isdn/hysdn/hysdn_pof.h
> index a368d6c..3a72b90 100644
> --- a/drivers/isdn/hysdn/hysdn_pof.h
> +++ b/drivers/isdn/hysdn/hysdn_pof.h
> @@ -60,7 +60,7 @@ typedef struct PofRecHdr_tag {	/* Pof record header */
>  
>  typedef struct PofTimeStamp_tag {
>  /*00 */ unsigned long UnixTime __attribute__((packed));
> -	/*04 */ unsigned char DateTimeText[0x28] __attribute__((packed));
> +	/*04 */ unsigned char DateTimeText[0x28];
>  	/* =40 */
>  /*2C */
>  } tPofTimeStamp;

I was admiring this the other day.  The compiler (my one at least) says

drivers/isdn/hysdn/hysdn_pof.h:63: warning: 'packed' attribute ignored for field of type 'unsigned char[39u]'

39u != 0x28.  What's up with that?

[PATCH 6/6] Werror: Provide a configuration option to enable -Werror

[David Howells]

Provide a configuration option to enable -Werror and make it work for x86_64
allyesconfig with __deprecated and __must_check checking disabled.

Note that it _won't_ be turned on for allyesconfig as it is suppressed by
ENABLE_WARN_DEPRECATED or ENABLE_MUST_CHECK being enabled.

Additionally, the advansys driver has its complaint about not being coverted
to the DMA API suppressed if this option is enabled.

Subject to those conditions, x86_64 allyesconfig will build with this option
enabled, if ENABLE_WARN_DEPRECATED and ENABLE_MUST_CHECK are manually disabled
and ENABLE_WERROR enabled.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 Makefile                |    4 ++++
 drivers/scsi/advansys.c |    2 ++
 lib/Kconfig.debug       |    8 ++++++++
 3 files changed, 14 insertions(+), 0 deletions(-)


diff --git a/Makefile b/Makefile
index 8db70fe..f40d5c1 100644
--- a/Makefile
+++ b/Makefile
@@ -528,6 +528,10 @@ KBUILD_CFLAGS	+= -g
 KBUILD_AFLAGS	+= -gdwarf-2
 endif
 
+ifdef CONFIG_ENABLE_WERROR
+KBUILD_CFLAGS	+= -Werror
+endif
+
 # We trigger additional mismatches with less inlining
 ifdef CONFIG_DEBUG_SECTION_MISMATCH
 KBUILD_CFLAGS += $(call cc-option, -fno-inline-functions-called-once)
diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index 8591585..539d22f 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -68,7 +68,9 @@
  *  7. advansys_info is not safe against multiple simultaneous callers
  *  8. Add module_param to override ISA/VLB ioport array
  */
+#ifndef CONFIG_ENABLE_WERROR
 #warning this driver is still not properly converted to the DMA API
+#endif
 
 /* Enable driver /proc statistics. */
 #define ADVANSYS_STATS
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index d2099f4..3efca43 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -25,6 +25,14 @@ config ENABLE_MUST_CHECK
 	  suppress the "warning: ignoring return value of 'foo', declared with
 	  attribute warn_unused_result" messages.
 
+config ENABLE_WERROR
+	bool "Enable -Werror"
+	default n
+	depends on !ENABLE_WARN_DEPRECATED && !ENABLE_MUST_CHECK
+	help
+	  Enable -Werror on building C files.  This causes all warnings to
+	  abort the compilation, just as errors do.
+
 config FRAME_WARN
 	int "Warn for stack frames larger than (needs gcc 4.4)"
 	range 0 8192

Re: [PATCH 6/6] Werror: Provide a configuration option to enable -Werror

[Linus Torvalds]

On Thu, 29 May 2008, David Howells wrote:
>
> Provide a configuration option to enable -Werror and make it work for x86_64
> allyesconfig with __deprecated and __must_check checking disabled.

No.

This just results in all the the other kinds of broken patches you have.

If anybody thinks that a compiler warning is the same thing as a compiler 
error, that person shouldn't be compiling.

			Linus

Re: [PATCH 1/6] Werror: Fix casting wrong size integer to pointer

[Linus Torvalds]

On Thu, 29 May 2008, David Howells wrote:
>
> Fix casting wrong size integer to pointer.  Smaller integers must be cast to
> unsigned long then cast to a pointer.

I do not want to apply patches that just hide compiler warnings.

Quite frankly, if the code looks worse (and it does), and the warning is 
valid (and in two out of three cases it *was*), then the patch that 
removes the warning is MUCH MUCH WORSE than the warning ever was!

So in this case, the "sis_mm.c" change *may* be fine. I say *may*, because 
quite frankly, I don't think it really is. The cast looks like it may be 
at the wrogn point, and the code really looks like crap. In particular, 
notice how the "sis_sman_mm_allocate()" and "sis_sman_mm_free()" (and 
the _offset() function) use casts on different sides of the '~' operator.

I think that thing should be cleaned up to actually have a function to 
turn the offset into a pointer and back. Because right now it looks buggy.

The two changes to drivers/message/i2o/i2o_config.c are utter shit. That 
warning should *stay*, because it damn well is correct. The code simply 
doesn't work right on a 64-bit architecture, and even has a comment to 
that effect.

Don't send patches to remove warnings. Send patches to FIX PROBLEMS. If 
the warning goes away because the _problem_ went away, it's a good patch. 
Otherwise the patch is just a total piece of crap.

			Linus

Re: [PATCH 1/6] Werror: Fix casting wrong size integer to pointer

[David Howells]

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

> I do not want to apply patches that just hide compiler warnings.

Well, will you at least consider patch 4?

Subject: [PATCH 4/6] Werror: Hide warnings on static module device tables

David

Re: [PATCH 1/6] Werror: Fix casting wrong size integer to pointer

[Andrew Morton]

On Thu, 29 May 2008 19:17:26 +0100 David Howells <dhowells@redhat.com> wrote:

> --- a/drivers/message/i2o/i2o_config.c
> +++ b/drivers/message/i2o/i2o_config.c
> @@ -886,7 +886,7 @@ static int i2o_cfg_passthru(unsigned long arg)
>  			    flag_count & 0x04000000 /*I2O_SGL_FLAGS_DIR */ ) {
>  				// TODO 64bit fix
>  				if (copy_from_user
> -				    (p, (void __user *)sg[i].addr_bus,
> +				    (p, (void __user *)(unsigned long)sg[i].addr_bus,
>  				     sg_size)) {
>  					printk(KERN_DEBUG
>  					       "%s: Could not copy SG buf %d FROM user\n",
> @@ -942,7 +942,7 @@ static int i2o_cfg_passthru(unsigned long arg)
>  				sg_size = sg[j].flag_count & 0xffffff;
>  				// TODO 64bit fix
>  				if (copy_to_user
> -				    ((void __user *)sg[j].addr_bus, sg_list[j],
> +				    ((void __user *)(unsigned long)sg[j].addr_bus, sg_list[j],
>  				     sg_size)) {
>  					printk(KERN_WARNING
>  					       "%s: Could not copy %p TO user %x\n",

yup, what Linus said.  This one's been irritating me for years, and I
got close to tempting Markus into fixing it before he departed the
scene.  But it remains, as a little "might be busted on 64 bit" reminder.

Re: [PATCH 1/6] Werror: Fix casting wrong size integer to pointer

[David Howells]

Andrew Morton <akpm@linux-foundation.org> wrote:

> yup, what Linus said.  This one's been irritating me for years, and I
> got close to tempting Markus into fixing it before he departed the
> scene.  But it remains, as a little "might be busted on 64 bit" reminder.

Is it worth making the driver only available for non-64-bit arches?

David

Re: [PATCH 1/6] Werror: Fix casting wrong size integer to pointer

[Andrew Morton]

On Thu, 29 May 2008 20:25:24 +0100
David Howells <dhowells@redhat.com> wrote:

> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > yup, what Linus said.  This one's been irritating me for years, and I
> > got close to tempting Markus into fixing it before he departed the
> > scene.  But it remains, as a little "might be busted on 64 bit" reminder.
> 
> Is it worth making the driver only available for non-64-bit arches?
> 

I don't know whether anyone is using it on 64-bit machines.  If they
are, disabling it would probably wake someone up.

Perhaps it all happens to work OK on 64-bit.  I just don't know, sorry.
A bit of lkml/bugzilla/google data mining would reveal the status, I
guess.