[PATCH 0/7] trivial: fix some compilation warnings for 2.6.32-rc5

[PATCH 0/7] trivial: fix some compilation warnings for 2.6.32-rc5

[Felipe Contreras]

These fixes almost all the warnings I get while compiling a custom kernel for
my laptop.

I'm not so sure about adding __cpuinit to acpi_processor_add() but otherwise
these should be safe to apply.

Felipe Contreras (7):
  usb: trivial cleanups
  ipc: fix trivial warning
  crypto: testmgr: fix warning
  acpi: processor: fix section mismatch
  acpi: fix a bunch of style issues on 'actypes.h'
  acpi: fix trivial warning
  acpi: fix trivial warnings caused by previous commmit

 crypto/testmgr.c              |    2 +-
 drivers/acpi/acpica/exfldio.c |    8 ++++----
 drivers/acpi/processor_core.c |    2 +-
 drivers/usb/core/hcd.c        |    7 +++----
 include/acpi/actypes.h        |   36 ++++++++++++++++++------------------
 ipc/msg.c                     |    2 +-
 6 files changed, 28 insertions(+), 29 deletions(-)

[PATCH 1/7] usb: trivial cleanups

[Felipe Contreras]

It seems 'min_t' was used before, and looks cleaner, plus white-space
stuff.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 drivers/usb/core/hcd.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 34de475..e6d754e 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -396,8 +396,7 @@ rh_string(int id, struct usb_hcd const *hcd, u8 *data, unsigned len)
 	case 0:
 		/* Array of LANGID codes (0x0409 is MSFT-speak for "en-us") */
 		/* See http://www.usb.org/developers/docs/USB_LANGIDs.pdf */
-		if (len > 4)
-			len = 4;
+		len = min_t(unsigned, len, sizeof(langids));
 		memcpy(data, langids, len);
 		return len;
 	case 1:
@@ -410,8 +409,8 @@ rh_string(int id, struct usb_hcd const *hcd, u8 *data, unsigned len)
 		break;
 	case 3:
 		/* Manufacturer */
-		snprintf (buf, sizeof buf, "%s %s %s", init_utsname()->sysname,
-			init_utsname()->release, hcd->driver->description);
+		snprintf(buf, sizeof buf, "%s %s %s", init_utsname()->sysname,
+			 init_utsname()->release, hcd->driver->description);
 		s = buf;
 		break;
 	default:
-- 
1.6.5.1

[PATCH 2/7] ipc: fix trivial warning

[Felipe Contreras]

Commit a0d092f introduced the following warning:
ipc/msg.c: In function ‘msgctl_down’:
ipc/msg.c:415: warning: ‘msqid64’ may be used uninitialized in this function

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 ipc/msg.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/ipc/msg.c b/ipc/msg.c
index 2ceab7f..085bd58 100644
--- a/ipc/msg.c
+++ b/ipc/msg.c
@@ -412,7 +412,7 @@ static int msgctl_down(struct ipc_namespace *ns, int msqid, int cmd,
 		       struct msqid_ds __user *buf, int version)
 {
 	struct kern_ipc_perm *ipcp;
-	struct msqid64_ds msqid64;
+	struct msqid64_ds uninitialized_var(msqid64);
 	struct msg_queue *msq;
 	int err;
 
-- 
1.6.5.1

[PATCH 3/7] crypto: testmgr: fix warning

[Felipe Contreras]

crypto/testmgr.c: In function ‘test_cprng’:
crypto/testmgr.c:1204: warning: ‘err’ may be used uninitialized in this function

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 crypto/testmgr.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 6d5b746..1f2357b 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -1201,7 +1201,7 @@ static int test_cprng(struct crypto_rng *tfm, struct cprng_testvec *template,
 		      unsigned int tcount)
 {
 	const char *algo = crypto_tfm_alg_driver_name(crypto_rng_tfm(tfm));
-	int err, i, j, seedsize;
+	int err = 0, i, j, seedsize;
 	u8 *seed;
 	char result[32];
 
-- 
1.6.5.1

[PATCH 4/7] acpi: processor: fix section mismatch

[Felipe Contreras]

WARNING: drivers/acpi/processor.o(.text+0x99a): Section mismatch in
reference from the function acpi_processor_add() to the function
.cpuinit.text:acpi_processor_power_init()

acpi_processor_add replaced acpi_processor_start which had __cpuinit.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 drivers/acpi/processor_core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index c567b46..ec742a4 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -770,7 +770,7 @@ static struct notifier_block acpi_cpu_notifier =
 	    .notifier_call = acpi_cpu_soft_notify,
 };
 
-static int acpi_processor_add(struct acpi_device *device)
+static int __cpuinit acpi_processor_add(struct acpi_device *device)
 {
 	struct acpi_processor *pr = NULL;
 	int result = 0;
-- 
1.6.5.1

[PATCH 5/7] acpi: fix a bunch of style issues on 'actypes.h'

[Felipe Contreras]

Many still remain.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 include/acpi/actypes.h |   36 ++++++++++++++++++------------------
 1 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index 153f12d..c1a0eb4 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -167,7 +167,7 @@ typedef u64 acpi_physical_address;
  * Note: Em64_t and other X86-64 processors support misaligned transfers,
  * so there is no need to define this flag.
  */
-#if defined (__IA64__) || defined (__ia64__)
+#if defined(__IA64__) || defined(__ia64__)
 #define ACPI_MISALIGNMENT_NOT_SUPPORTED
 #endif
 
@@ -242,10 +242,10 @@ typedef u32 acpi_physical_address;
  * Map the OSL Mutex interfaces to binary semaphores.
  */
 #define acpi_mutex                      acpi_semaphore
-#define acpi_os_create_mutex(out_handle) acpi_os_create_semaphore (1, 1, out_handle)
-#define acpi_os_delete_mutex(handle)    (void) acpi_os_delete_semaphore (handle)
-#define acpi_os_acquire_mutex(handle,time) acpi_os_wait_semaphore (handle, 1, time)
-#define acpi_os_release_mutex(handle)   (void) acpi_os_signal_semaphore (handle, 1)
+#define acpi_os_create_mutex(out_handle) acpi_os_create_semaphore(1, 1, out_handle)
+#define acpi_os_delete_mutex(handle)    (void) acpi_os_delete_semaphore(handle)
+#define acpi_os_acquire_mutex(handle, time) acpi_os_wait_semaphore(handle, 1, time)
+#define acpi_os_release_mutex(handle)   (void) acpi_os_signal_semaphore(handle, 1)
 #endif
 
 /* Configurable types for synchronization objects */
@@ -417,7 +417,7 @@ typedef unsigned long long acpi_integer;
 /*
  * Constants with special meanings
  */
-#define ACPI_ROOT_OBJECT                ACPI_ADD_PTR (acpi_handle, NULL, ACPI_MAX_PTR)
+#define ACPI_ROOT_OBJECT                ACPI_ADD_PTR(acpi_handle, NULL, ACPI_MAX_PTR)
 #define ACPI_WAIT_FOREVER               0xFFFF	/* u16, as per ACPI spec */
 #define ACPI_DO_NOT_WAIT                0
 
@@ -438,8 +438,8 @@ typedef unsigned long long acpi_integer;
 
 #define ACPI_SET_BIT(target,bit)        ((target) |= (bit))
 #define ACPI_CLEAR_BIT(target,bit)      ((target) &= ~(bit))
-#define ACPI_MIN(a,b)                   (((a)<(b))?(a):(b))
-#define ACPI_MAX(a,b)                   (((a)>(b))?(a):(b))
+#define ACPI_MIN(a, b)                  (((a)<(b))?(a):(b))
+#define ACPI_MAX(a, b)                  (((a)>(b))?(a):(b))
 
 /* Size calculation */
 
@@ -449,21 +449,21 @@ typedef unsigned long long acpi_integer;
 
 #define ACPI_CAST_PTR(t, p)             ((t *) (acpi_uintptr_t) (p))
 #define ACPI_CAST_INDIRECT_PTR(t, p)    ((t **) (acpi_uintptr_t) (p))
-#define ACPI_ADD_PTR(t, a, b)           ACPI_CAST_PTR (t, (ACPI_CAST_PTR (u8, (a)) + (acpi_size)(b)))
-#define ACPI_PTR_DIFF(a, b)             (acpi_size) (ACPI_CAST_PTR (u8, (a)) - ACPI_CAST_PTR (u8, (b)))
+#define ACPI_ADD_PTR(t, a, b)           ACPI_CAST_PTR(t, (ACPI_CAST_PTR(u8, (a)) + (acpi_size)(b)))
+#define ACPI_PTR_DIFF(a, b)             (acpi_size) (ACPI_CAST_PTR(u8, (a)) - ACPI_CAST_PTR(u8, (b)))
 
 /* Pointer/Integer type conversions */
 
-#define ACPI_TO_POINTER(i)              ACPI_ADD_PTR (void, (void *) NULL,(acpi_size) i)
-#define ACPI_TO_INTEGER(p)              ACPI_PTR_DIFF (p, (void *) NULL)
-#define ACPI_OFFSET(d, f)               (acpi_size) ACPI_PTR_DIFF (&(((d *)0)->f), (void *) NULL)
+#define ACPI_TO_POINTER(i)              ACPI_ADD_PTR(void, (void *) NULL, (acpi_size) i)
+#define ACPI_TO_INTEGER(p)              ACPI_PTR_DIFF(p, (void *) NULL)
+#define ACPI_OFFSET(d, f)               (acpi_size) ACPI_PTR_DIFF(&(((d *)0)->f), (void *) NULL)
 #define ACPI_PHYSADDR_TO_PTR(i)         ACPI_TO_POINTER(i)
 #define ACPI_PTR_TO_PHYSADDR(i)         ACPI_TO_INTEGER(i)
 
 #ifndef ACPI_MISALIGNMENT_NOT_SUPPORTED
-#define ACPI_COMPARE_NAME(a,b)          (*ACPI_CAST_PTR (u32, (a)) == *ACPI_CAST_PTR (u32, (b)))
+#define ACPI_COMPARE_NAME(a, b)         (*ACPI_CAST_PTR(u32, (a)) == *ACPI_CAST_PTR(u32, (b)))
 #else
-#define ACPI_COMPARE_NAME(a,b)          (!ACPI_STRNCMP (ACPI_CAST_PTR (char, (a)), ACPI_CAST_PTR (char, (b)), ACPI_NAME_SIZE))
+#define ACPI_COMPARE_NAME(a, b)         (!ACPI_STRNCMP(ACPI_CAST_PTR(char, (a)), ACPI_CAST_PTR(char, (b)), ACPI_NAME_SIZE))
 #endif
 
 /*******************************************************************************
@@ -631,7 +631,7 @@ typedef u32 acpi_event_type;
 #define ACPI_EVENT_SLEEP_BUTTON         3
 #define ACPI_EVENT_RTC                  4
 #define ACPI_EVENT_MAX                  4
-#define ACPI_NUM_FIXED_EVENTS           ACPI_EVENT_MAX + 1
+#define ACPI_NUM_FIXED_EVENTS           (ACPI_EVENT_MAX + 1)
 
 /*
  * Event Status - Per event
@@ -778,7 +778,7 @@ typedef u8 acpi_adr_space_type;
 #define ACPI_BITREG_ARB_DISABLE                 0x13
 
 #define ACPI_BITREG_MAX                         0x13
-#define ACPI_NUM_BITREG                         ACPI_BITREG_MAX + 1
+#define ACPI_NUM_BITREG                         (ACPI_BITREG_MAX + 1)
 
 /* Status register values. A 1 clears a status bit. 0 = no effect */
 
@@ -945,7 +945,7 @@ typedef
 acpi_status(*acpi_adr_space_handler) (u32 function,
 				      acpi_physical_address address,
 				      u32 bit_width,
-				      acpi_integer * value,
+				      acpi_integer *value,
 				      void *handler_context,
 				      void *region_context);
 
-- 
1.6.5.1

[PATCH 6/7] acpi: fix trivial warning

[Felipe Contreras]

drivers/acpi/acpica/tbfadt.c: In function ‘acpi_tb_create_local_fadt’:
arch/x86/include/asm/string_32.h:74: warning: array subscript is above array bounds

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 include/acpi/actypes.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index c1a0eb4..7d989f1 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -438,8 +438,8 @@ typedef unsigned long long acpi_integer;
 
 #define ACPI_SET_BIT(target,bit)        ((target) |= (bit))
 #define ACPI_CLEAR_BIT(target,bit)      ((target) &= ~(bit))
-#define ACPI_MIN(a, b)                  (((a)<(b))?(a):(b))
-#define ACPI_MAX(a, b)                  (((a)>(b))?(a):(b))
+#define ACPI_MIN(a, b)                  min(a, b)
+#define ACPI_MAX(a, b)                  max(a, b)
 
 /* Size calculation */
 
-- 
1.6.5.1

[PATCH 7/7] acpi: fix trivial warnings caused by previous commmit

[Felipe Contreras]

drivers/acpi/acpica/exfldio.c: In function ‘acpi_ex_extract_from_field’:
drivers/acpi/acpica/exfldio.c:761: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:761: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:761: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:780: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:780: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:780: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c: In function ‘acpi_ex_insert_into_field’:
drivers/acpi/acpica/exfldio.c:880: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:880: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:880: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:933: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:933: warning: comparison of distinct pointer types lacks a cast
drivers/acpi/acpica/exfldio.c:933: warning: comparison of distinct pointer types lacks a cast

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 drivers/acpi/acpica/exfldio.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/acpica/exfldio.c b/drivers/acpi/acpica/exfldio.c
index d7b3b41..597bc39 100644
--- a/drivers/acpi/acpica/exfldio.c
+++ b/drivers/acpi/acpica/exfldio.c
@@ -761,7 +761,7 @@ acpi_ex_extract_from_field(union acpi_operand_object *obj_desc,
 
 		ACPI_MEMCPY(((char *)buffer) + buffer_offset, &merged_datum,
 			    ACPI_MIN(obj_desc->common_field.access_byte_width,
-				     buffer_length - buffer_offset));
+				     (u8)(buffer_length - buffer_offset)));
 
 		buffer_offset += obj_desc->common_field.access_byte_width;
 		merged_datum =
@@ -780,7 +780,7 @@ acpi_ex_extract_from_field(union acpi_operand_object *obj_desc,
 
 	ACPI_MEMCPY(((char *)buffer) + buffer_offset, &merged_datum,
 		    ACPI_MIN(obj_desc->common_field.access_byte_width,
-			     buffer_length - buffer_offset));
+			     (u8)(buffer_length - buffer_offset)));
 
 	return_ACPI_STATUS(AE_OK);
 }
@@ -880,7 +880,7 @@ acpi_ex_insert_into_field(union acpi_operand_object *obj_desc,
 
 	ACPI_MEMCPY(&raw_datum, buffer,
 		    ACPI_MIN(obj_desc->common_field.access_byte_width,
-			     buffer_length - buffer_offset));
+			     (u8)(buffer_length - buffer_offset)));
 
 	merged_datum =
 	    raw_datum << obj_desc->common_field.start_field_bit_offset;
@@ -933,7 +933,7 @@ acpi_ex_insert_into_field(union acpi_operand_object *obj_desc,
 		buffer_offset += obj_desc->common_field.access_byte_width;
 		ACPI_MEMCPY(&raw_datum, ((char *)buffer) + buffer_offset,
 			    ACPI_MIN(obj_desc->common_field.access_byte_width,
-				     buffer_length - buffer_offset));
+				     (u8)(buffer_length - buffer_offset)));
 		merged_datum |=
 		    raw_datum << obj_desc->common_field.start_field_bit_offset;
 	}
-- 
1.6.5.1

Re: [PATCH 1/7] usb: trivial cleanups

[Felipe Contreras]

On Mon, Oct 19, 2009 at 1:54 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> It seems 'min_t' was used before, and looks cleaner, plus white-space
> stuff.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  drivers/usb/core/hcd.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 34de475..e6d754e 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -396,8 +396,7 @@ rh_string(int id, struct usb_hcd const *hcd, u8 *data, unsigned len)
>        case 0:
>                /* Array of LANGID codes (0x0409 is MSFT-speak for "en-us") */
>                /* See http://www.usb.org/developers/docs/USB_LANGIDs.pdf */
> -               if (len > 4)
> -                       len = 4;
> +               len = min_t(unsigned, len, sizeof(langids));
>                memcpy(data, langids, len);
>                return len;
>        case 1:

I still have an unfixed warning:
drivers/usb/core/hcd.c: In function ‘rh_string’:
/data/public/src/linux/arch/x86/include/asm/string_32.h:74: warning:
array subscript is above array bounds

Apparently there's a problem with the optimized memcpy for x86 with this code:
static char const langids[4] = {4, USB_DT_STRING, 0x09, 0x04};
len = min_t(unsigned, len, sizeof(langids));
memcpy(data, langids, len);
return len;

gcc 4.4 is trying to optimize the memcpy, but it's not able to realize
that 'len' will always be <= 4 and the memcpy will not exceed langids.
One way to solve this is by replacing len with the min_t expression:
memcpy(data, langids, min_t(unsigned, len, sizeof(langids)));

However, that looks ugly and we need the expression again for the
return. Another way is to remove 'const' from langids.

AFAIK the code is perfectly correct as it is, I think the fact that
gcc 4.4 complains is a bug on gcc side.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/7] usb: trivial cleanups

[Greg KH]

On Mon, Oct 19, 2009 at 01:54:28AM +0300, Felipe Contreras wrote:
> It seems 'min_t' was used before, and looks cleaner, plus white-space
> stuff.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

But this doesn't fix a kernel warning, does it?  If so, what one?

thanks,

greg k-h

Re: [PATCH 1/7] usb: trivial cleanups

[Felipe Contreras]

On Mon, Oct 19, 2009 at 3:02 AM, Greg KH <greg@kroah.com> wrote:
> On Mon, Oct 19, 2009 at 01:54:28AM +0300, Felipe Contreras wrote:
>> It seems 'min_t' was used before, and looks cleaner, plus white-space
>> stuff.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>
> But this doesn't fix a kernel warning, does it?  If so, what one?

No, it's just cleanups I noticed while trying to fix the warning I
described here:
http://lkml.org/lkml/2009/10/18/126

-- 
Felipe Contreras

Re: [PATCH 3/7] crypto: testmgr: fix warning

[Jiri Kosina]

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> crypto/testmgr.c: In function ?test_cprng?:
> crypto/testmgr.c:1204: warning: ?err? may be used uninitialized in this function
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  crypto/testmgr.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index 6d5b746..1f2357b 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -1201,7 +1201,7 @@ static int test_cprng(struct crypto_rng *tfm, struct cprng_testvec *template,
>  		      unsigned int tcount)
>  {
>  	const char *algo = crypto_tfm_alg_driver_name(crypto_rng_tfm(tfm));
> -	int err, i, j, seedsize;
> +	int err = 0, i, j, seedsize;
>  	u8 *seed;
>  	char result[32];

As it is not obvious to me immediately why/whether tcount couldn't be zero 
(which would cause uninitialized use of 'err'), I am not merging this 
through trivial tree. Herbert?

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 3/7] crypto: testmgr: fix warning

[Jarod Wilson]

On 10/19/09 9:52 AM, Jiri Kosina wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>
>> crypto/testmgr.c: In function ?test_cprng?:
>> crypto/testmgr.c:1204: warning: ?err? may be used uninitialized in this function
>>
>> Signed-off-by: Felipe Contreras<felipe.contreras@gmail.com>
>> ---
>>   crypto/testmgr.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
>> index 6d5b746..1f2357b 100644
>> --- a/crypto/testmgr.c
>> +++ b/crypto/testmgr.c
>> @@ -1201,7 +1201,7 @@ static int test_cprng(struct crypto_rng *tfm, struct cprng_testvec *template,
>>   		      unsigned int tcount)
>>   {
>>   	const char *algo = crypto_tfm_alg_driver_name(crypto_rng_tfm(tfm));
>> -	int err, i, j, seedsize;
>> +	int err = 0, i, j, seedsize;
>>   	u8 *seed;
>>   	char result[32];
>
> As it is not obvious to me immediately why/whether tcount couldn't be zero
> (which would cause uninitialized use of 'err'), I am not merging this
> through trivial tree. Herbert?

I believe I'm the guilty party who wrote the code in question. 
Initializing err to 0 isn't correct. tcount should always be at least 1, 
if its 0, test_cprng has been called with invalid parameters. I believe 
err would best be initialized to -EINVAL, lest the caller think they 
were successful.

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH 1/7] usb: trivial cleanups

[Alan Stern]

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> I still have an unfixed warning:
> drivers/usb/core/hcd.c: In function ‘rh_string’:
> /data/public/src/linux/arch/x86/include/asm/string_32.h:74: warning:
> array subscript is above array bounds
> 
> Apparently there's a problem with the optimized memcpy for x86 with this code:
> static char const langids[4] = {4, USB_DT_STRING, 0x09, 0x04};
> len = min_t(unsigned, len, sizeof(langids));
> memcpy(data, langids, len);
> return len;
> 
> gcc 4.4 is trying to optimize the memcpy, but it's not able to realize
> that 'len' will always be <= 4 and the memcpy will not exceed langids.
> One way to solve this is by replacing len with the min_t expression:
> memcpy(data, langids, min_t(unsigned, len, sizeof(langids)));
> 
> However, that looks ugly and we need the expression again for the
> return. Another way is to remove 'const' from langids.
> 
> AFAIK the code is perfectly correct as it is, I think the fact that
> gcc 4.4 complains is a bug on gcc side.

Yes, it is a well-known bug in gcc.  Other places in the kernel get 
similar warnings.

Alan Stern

Re: [PATCH 1/7] usb: trivial cleanups

[Felipe Contreras]

On Mon, Oct 19, 2009 at 4:59 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>
>> I still have an unfixed warning:
>> drivers/usb/core/hcd.c: In function ‘rh_string’:
>> /data/public/src/linux/arch/x86/include/asm/string_32.h:74: warning:
>> array subscript is above array bounds
>>
>> Apparently there's a problem with the optimized memcpy for x86 with this code:
>> static char const langids[4] = {4, USB_DT_STRING, 0x09, 0x04};
>> len = min_t(unsigned, len, sizeof(langids));
>> memcpy(data, langids, len);
>> return len;
>>
>> gcc 4.4 is trying to optimize the memcpy, but it's not able to realize
>> that 'len' will always be <= 4 and the memcpy will not exceed langids.
>> One way to solve this is by replacing len with the min_t expression:
>> memcpy(data, langids, min_t(unsigned, len, sizeof(langids)));
>>
>> However, that looks ugly and we need the expression again for the
>> return. Another way is to remove 'const' from langids.
>>
>> AFAIK the code is perfectly correct as it is, I think the fact that
>> gcc 4.4 complains is a bug on gcc side.
>
> Yes, it is a well-known bug in gcc.  Other places in the kernel get
> similar warnings.

So gcc guys are aware of this? Is there a bug report or something?

-- 
Felipe Contreras

Re: [PATCH 3/7] crypto: testmgr: fix warning

[Jarod Wilson]

On 10/19/09 9:58 AM, Jarod Wilson wrote:
> On 10/19/09 9:52 AM, Jiri Kosina wrote:
>> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>>
>>> crypto/testmgr.c: In function ?test_cprng?:
>>> crypto/testmgr.c:1204: warning: ?err? may be used uninitialized in
>>> this function
>>>
>>> Signed-off-by: Felipe Contreras<felipe.contreras@gmail.com>
>>> ---
>>> crypto/testmgr.c | 2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
>>> index 6d5b746..1f2357b 100644
>>> --- a/crypto/testmgr.c
>>> +++ b/crypto/testmgr.c
>>> @@ -1201,7 +1201,7 @@ static int test_cprng(struct crypto_rng *tfm,
>>> struct cprng_testvec *template,
>>> unsigned int tcount)
>>> {
>>> const char *algo = crypto_tfm_alg_driver_name(crypto_rng_tfm(tfm));
>>> - int err, i, j, seedsize;
>>> + int err = 0, i, j, seedsize;
>>> u8 *seed;
>>> char result[32];
>>
>> As it is not obvious to me immediately why/whether tcount couldn't be
>> zero
>> (which would cause uninitialized use of 'err'), I am not merging this
>> through trivial tree. Herbert?
>
> I believe I'm the guilty party who wrote the code in question.
> Initializing err to 0 isn't correct. tcount should always be at least 1,
> if its 0, test_cprng has been called with invalid parameters. I believe
> err would best be initialized to -EINVAL, lest the caller think they
> were successful.

...and I need to re-read said code more carefully. tcount is 
desc->suite.cprng.count, which is ANSI_CPRNG_AES_TEST_VECTORS, which is 
#define'd to 6, and is the count of how many cprng test vectors there 
are. If someone changes that to 0, then I guess a retval of 0 would 
actually be correct, since no tests failed...

So yeah, I rescind my claim that initializing err to 0 is incorrect, I 
think that's just fine.

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH 2/7] ipc: fix trivial warning

[Jiri Kosina]

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> Commit a0d092f introduced the following warning:
> ipc/msg.c: In function ?msgctl_down?:
> ipc/msg.c:415: warning: ?msqid64? may be used uninitialized in this function
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  ipc/msg.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/ipc/msg.c b/ipc/msg.c
> index 2ceab7f..085bd58 100644
> --- a/ipc/msg.c
> +++ b/ipc/msg.c
> @@ -412,7 +412,7 @@ static int msgctl_down(struct ipc_namespace *ns, int msqid, int cmd,
>  		       struct msqid_ds __user *buf, int version)
>  {
>  	struct kern_ipc_perm *ipcp;
> -	struct msqid64_ds msqid64;
> +	struct msqid64_ds uninitialized_var(msqid64);
>  	struct msg_queue *msq;
>  	int err;

What gcc are you using? I am not getting any warning at least with gcc 
"(SUSE Linux) 4.3.1 20080507 (prerelease) [gcc-4_3-branch revision 
135036]"

$ make ipc/msg.o
  CHK     include/linux/version.h
  CHK     include/linux/utsrelease.h
  SYMLINK include/asm -> include/asm-x86
  CALL    scripts/checksyscalls.sh
  CC      ipc/msg.o
$

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 1/7] usb: trivial cleanups

[Alan Stern]

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> >> AFAIK the code is perfectly correct as it is, I think the fact that
> >> gcc 4.4 complains is a bug on gcc side.
> >
> > Yes, it is a well-known bug in gcc.  Other places in the kernel get
> > similar warnings.
> 
> So gcc guys are aware of this? Is there a bug report or something?

I don't know.  You should ask them.

Alan Stern

Re: [PATCH 6/7] acpi: fix trivial warning

[Jiri Kosina]

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> drivers/acpi/acpica/tbfadt.c: In function ?acpi_tb_create_local_fadt?:
> arch/x86/include/asm/string_32.h:74: warning: array subscript is above array bounds

Same is in your previous patch -- what gcc is that? I don't see any 
warning with 4.3.1

$ make drivers/acpi/acpica/tbfadt.o
  CHK     include/linux/version.h
  CHK     include/linux/utsrelease.h
  SYMLINK include/asm -> include/asm-x86
  CALL    scripts/checksyscalls.sh
  CC      drivers/acpi/acpica/tbfadt.o
$

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 5/7] acpi: fix a bunch of style issues on 'actypes.h'

[Jiri Kosina]

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> Many still remain.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

I have never been in favor of merging whitespace-only patches (in a sense 
that the sole purpose of them being to change whitespaces, but no else 
value added). 

And after today's discussion on kernel summit on this topic, I wouldn't 
expect any maintainer to merge it, sorry :)

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 2/7] ipc: fix trivial warning

[Felipe Contreras]

On Mon, Oct 19, 2009 at 5:12 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>
>> Commit a0d092f introduced the following warning:
>> ipc/msg.c: In function ?msgctl_down?:
>> ipc/msg.c:415: warning: ?msqid64? may be used uninitialized in this function
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  ipc/msg.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/ipc/msg.c b/ipc/msg.c
>> index 2ceab7f..085bd58 100644
>> --- a/ipc/msg.c
>> +++ b/ipc/msg.c
>> @@ -412,7 +412,7 @@ static int msgctl_down(struct ipc_namespace *ns, int msqid, int cmd,
>>                      struct msqid_ds __user *buf, int version)
>>  {
>>       struct kern_ipc_perm *ipcp;
>> -     struct msqid64_ds msqid64;
>> +     struct msqid64_ds uninitialized_var(msqid64);
>>       struct msg_queue *msq;
>>       int err;
>
> What gcc are you using? I am not getting any warning at least with gcc
> "(SUSE Linux) 4.3.1 20080507 (prerelease) [gcc-4_3-branch revision
> 135036]"

gcc (GCC) 4.4.1 20090725 (Red Hat 4.4.1-2)

Since I moved to Fedora 11 I get more warnings than other people,
possibly because gcc 4.4.

-- 
Felipe Contreras

Re: [PATCH 2/7] ipc: fix trivial warning

[Jiri Kosina]

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

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> >> ipc/msg.c: In function ?msgctl_down?:
> >> ipc/msg.c:415: warning: ?msqid64? may be used uninitialized in this function
> >>
> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >> ---
> >>  ipc/msg.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/ipc/msg.c b/ipc/msg.c
> >> index 2ceab7f..085bd58 100644
> >> --- a/ipc/msg.c
> >> +++ b/ipc/msg.c
> >> @@ -412,7 +412,7 @@ static int msgctl_down(struct ipc_namespace *ns, int msqid, int cmd,
> >>                      struct msqid_ds __user *buf, int version)
> >>  {
> >>       struct kern_ipc_perm *ipcp;
> >> -     struct msqid64_ds msqid64;
> >> +     struct msqid64_ds uninitialized_var(msqid64);
> >>       struct msg_queue *msq;
> >>       int err;
> >
> > What gcc are you using? I am not getting any warning at least with gcc
> > "(SUSE Linux) 4.3.1 20080507 (prerelease) [gcc-4_3-branch revision
> > 135036]"
> 
> gcc (GCC) 4.4.1 20090725 (Red Hat 4.4.1-2)
> 
> Since I moved to Fedora 11 I get more warnings than other people,
> possibly because gcc 4.4.

Wouldn't it be better just to report this to gcc developers as a bug 
instead?

I have verified both with 4.1 and 4.3, and it doesn't emit this 
false-positive warning, so there have been gcc versions getting this 
right. Ergo gcc developers should rather fix this "regression" and revert 
to the old behavior, methinks.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 5/7] acpi: fix a bunch of style issues on 'actypes.h'

[Felipe Contreras]

On Mon, Oct 19, 2009 at 5:20 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>
>> Many still remain.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>
> I have never been in favor of merging whitespace-only patches (in a sense
> that the sole purpose of them being to change whitespaces, but no else
> value added).

If somebody tries to send a patch for that file that doesn't fix the
white-space, checkpatch will complain, and people will complain that
checkpatch complains, which is precisely what happened, and I was
requested to write this patch by Daniel Walker (final mail wasn't on
the ml):

http://lkml.org/lkml/2009/9/14/183

> And after today's discussion on kernel summit on this topic, I wouldn't
> expect any maintainer to merge it, sorry :)

What are you talking about?

-- 
Felipe Contreras

Re: [PATCH 5/7] acpi: fix a bunch of style issues on 'actypes.h'

[Jiri Kosina]

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> > I have never been in favor of merging whitespace-only patches (in a 
> > sense that the sole purpose of them being to change whitespaces, but 
> > no else value added).
> If somebody tries to send a patch for that file that doesn't fix the
> white-space, checkpatch will complain, and people will complain that
> checkpatch complains, which is precisely what happened, 

Oh, well ... checkpatch warning about this is somewhat controversial. My 
preferred way would be that it warns about whitespace only if there are 
also some other (non-whitespace) changes.

> and I was requested to write this patch by Daniel Walker (final mail 
> wasn't on the ml):
> 
> http://lkml.org/lkml/2009/9/14/183

This is something slightly different -- he asks you to fixup whitespace 
issue in the code you are newly introducing, right?

> > And after today's discussion on kernel summit on this topic, I wouldn't
> > expect any maintainer to merge it, sorry :)
> What are you talking about?

Seems like many kernel maintainers are just tired of 
'whitespace-cleanup-only' patches that bring no real added value 
otherwise.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 2/7] ipc: fix trivial warning

[Felipe Contreras]

On Mon, Oct 19, 2009 at 5:57 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>
>> >> ipc/msg.c: In function ?msgctl_down?:
>> >> ipc/msg.c:415: warning: ?msqid64? may be used uninitialized in this function
>> >>
>> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >> ---
>> >>  ipc/msg.c |    2 +-
>> >>  1 files changed, 1 insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/ipc/msg.c b/ipc/msg.c
>> >> index 2ceab7f..085bd58 100644
>> >> --- a/ipc/msg.c
>> >> +++ b/ipc/msg.c
>> >> @@ -412,7 +412,7 @@ static int msgctl_down(struct ipc_namespace *ns, int msqid, int cmd,
>> >>                      struct msqid_ds __user *buf, int version)
>> >>  {
>> >>       struct kern_ipc_perm *ipcp;
>> >> -     struct msqid64_ds msqid64;
>> >> +     struct msqid64_ds uninitialized_var(msqid64);
>> >>       struct msg_queue *msq;
>> >>       int err;
>> >
>> > What gcc are you using? I am not getting any warning at least with gcc
>> > "(SUSE Linux) 4.3.1 20080507 (prerelease) [gcc-4_3-branch revision
>> > 135036]"
>>
>> gcc (GCC) 4.4.1 20090725 (Red Hat 4.4.1-2)
>>
>> Since I moved to Fedora 11 I get more warnings than other people,
>> possibly because gcc 4.4.
>
> Wouldn't it be better just to report this to gcc developers as a bug
> instead?

If it's a gcc bug, yes.

> I have verified both with 4.1 and 4.3, and it doesn't emit this
> false-positive warning, so there have been gcc versions getting this
> right. Ergo gcc developers should rather fix this "regression" and revert
> to the old behavior, methinks.

The other possibility is that the bug was in gcc 4.1/4.3, and now 4.4
finds an actual problem in the code. I will try to dig deeper to see
what's actually happening... at first glance I don't see who is
initializing msqid64.

-- 
Felipe Contreras

Re: [PATCH 2/7] ipc: fix trivial warning

[Jiri Kosina]

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

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> >> >> ipc/msg.c: In function ?msgctl_down?:
> >> >> ipc/msg.c:415: warning: ?msqid64? may be used uninitialized in this function
> >> >>
> >> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >> >> ---
> >> >>  ipc/msg.c |    2 +-
> >> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >> >>
> >> >> diff --git a/ipc/msg.c b/ipc/msg.c
> >> >> index 2ceab7f..085bd58 100644
> >> >> --- a/ipc/msg.c
> >> >> +++ b/ipc/msg.c
> >> >> @@ -412,7 +412,7 @@ static int msgctl_down(struct ipc_namespace *ns, int msqid, int cmd,
> >> >>                      struct msqid_ds __user *buf, int version)
> >> >>  {
> >> >>       struct kern_ipc_perm *ipcp;
> >> >> -     struct msqid64_ds msqid64;
> >> >> +     struct msqid64_ds uninitialized_var(msqid64);
> >> >>       struct msg_queue *msq;
> >> >>       int err;
[ ... snip ... ]
> > I have verified both with 4.1 and 4.3, and it doesn't emit this
> > false-positive warning, so there have been gcc versions getting this
> > right. Ergo gcc developers should rather fix this "regression" and revert
> > to the old behavior, methinks.
> 
> The other possibility is that the bug was in gcc 4.1/4.3, and now 4.4
> finds an actual problem in the code. I will try to dig deeper to see
> what's actually happening... at first glance I don't see who is
> initializing msqid64.

This statement of your makes me wonder why you have submitted the patch in 
the first place, as you are apparently not sure whether adding 
uninitialized_var() is a valid thing to do or not.

The gcc warning in this case is actually bogus, as msqid64 is touched only 
iff cmd == IPC_SET, and in such case, copy_msqid_from_user() initializes 
it properly.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 5/7] acpi: fix a bunch of style issues on 'actypes.h'

[Felipe Contreras]

On Mon, Oct 19, 2009 at 6:03 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>
>> > I have never been in favor of merging whitespace-only patches (in a
>> > sense that the sole purpose of them being to change whitespaces, but
>> > no else value added).
>> If somebody tries to send a patch for that file that doesn't fix the
>> white-space, checkpatch will complain, and people will complain that
>> checkpatch complains, which is precisely what happened,
>
> Oh, well ... checkpatch warning about this is somewhat controversial. My
> preferred way would be that it warns about whitespace only if there are
> also some other (non-whitespace) changes.

Huh? I think we are talking about different things. See the next comment.

>> and I was requested to write this patch by Daniel Walker (final mail
>> wasn't on the ml):
>>
>> http://lkml.org/lkml/2009/9/14/183
>
> This is something slightly different -- he asks you to fixup whitespace
> issue in the code you are newly introducing, right?

No, did you read the thread?

This was my patch:
-#define ACPI_MIN(a,b)                   (((a)<(b))?(a):(b))
-#define ACPI_MAX(a,b)                   (((a)>(b))?(a):(b))
+#define ACPI_MIN(a,b)                   min(a, b)
+#define ACPI_MAX(a,b)                   max(a, b)

Checkpatch complains, even though my changes are ok. So that's what I
mean, if somebody wants to do a similar patch in the future so that
checkpatch doesn't complain; they would have to fix the white-spaces
again.

Or checkpatch should be fixed.

>> > And after today's discussion on kernel summit on this topic, I wouldn't
>> > expect any maintainer to merge it, sorry :)
>> What are you talking about?
>
> Seems like many kernel maintainers are just tired of
> 'whitespace-cleanup-only' patches that bring no real added value
> otherwise.

Hm, I wonder what would happen to the current badly formatted code.
Stay there forever?

-- 
Felipe Contreras

Re: [PATCH 2/7] ipc: fix trivial warning

[Felipe Contreras]

On Mon, Oct 19, 2009 at 6:29 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>
>> >> >> ipc/msg.c: In function ?msgctl_down?:
>> >> >> ipc/msg.c:415: warning: ?msqid64? may be used uninitialized in this function
>> >> >>
>> >> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >> >> ---
>> >> >>  ipc/msg.c |    2 +-
>> >> >>  1 files changed, 1 insertions(+), 1 deletions(-)
>> >> >>
>> >> >> diff --git a/ipc/msg.c b/ipc/msg.c
>> >> >> index 2ceab7f..085bd58 100644
>> >> >> --- a/ipc/msg.c
>> >> >> +++ b/ipc/msg.c
>> >> >> @@ -412,7 +412,7 @@ static int msgctl_down(struct ipc_namespace *ns, int msqid, int cmd,
>> >> >>                      struct msqid_ds __user *buf, int version)
>> >> >>  {
>> >> >>       struct kern_ipc_perm *ipcp;
>> >> >> -     struct msqid64_ds msqid64;
>> >> >> +     struct msqid64_ds uninitialized_var(msqid64);
>> >> >>       struct msg_queue *msq;
>> >> >>       int err;
> [ ... snip ... ]
>> > I have verified both with 4.1 and 4.3, and it doesn't emit this
>> > false-positive warning, so there have been gcc versions getting this
>> > right. Ergo gcc developers should rather fix this "regression" and revert
>> > to the old behavior, methinks.
>>
>> The other possibility is that the bug was in gcc 4.1/4.3, and now 4.4
>> finds an actual problem in the code. I will try to dig deeper to see
>> what's actually happening... at first glance I don't see who is
>> initializing msqid64.
>
> This statement of your makes me wonder why you have submitted the patch in
> the first place, as you are apparently not sure whether adding
> uninitialized_var() is a valid thing to do or not.

Because I want to get rid of the warning?

Also, if you look at the commit I mentioned in the commit message
(a0d092f), you'll see this change:
-	struct msq_setbuf uninitialized_var(setbuf);
+	struct msq_setbuf setbuf;

Which is ok, because in that version msgctl_down is calling
audit_ipc_set_perm, directly, but only when cmd == IPC_SET. However,
later on (a5f75e7), ipcctl_pre_down was introduced, and the check was
delegated, and that most probably introduced the warning.

In any case, if the patch is not correct, then somebody should point
that out, which is what the patch review process is for. Alternatively
I could have sent an email asking what is happening here, but from my
point of view this patch serves exactly the same purpose, and has the
advantage that it might turn out to be correct.

It's not as if I didn't do any homework while writing this patch.

> The gcc warning in this case is actually bogus, as msqid64 is touched only
> iff cmd == IPC_SET, and in such case, copy_msqid_from_user() initializes
> it properly.

Yes, but it's used by ipcctl_pre_down, which from what I can see is
only using those arguments when cmd == IPC_SET, so everything is ok.
But still, I don't think the compiler _should_ know what
ipcctl_pre_down is going to do, if you look _only_ at msgctl_down,
then uninitialized_var is OK.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 2/7] ipc: fix trivial warning

[Jiri Kosina]

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> > This statement of your makes me wonder why you have submitted the patch in
> > the first place, as you are apparently not sure whether adding
> > uninitialized_var() is a valid thing to do or not.
[ ... ]
> In any case, if the patch is not correct, then somebody should point
> that out, which is what the patch review process is for. Alternatively
> I could have sent an email asking what is happening here, but from my
> point of view this patch serves exactly the same purpose, and has the
> advantage that it might turn out to be correct.
>
> It's not as if I didn't do any homework while writing this patch.

Sorry if it sounded to offensive, I have probably been a little bit too 
tired ysterday.

> > The gcc warning in this case is actually bogus, as msqid64 is touched only
> > iff cmd == IPC_SET, and in such case, copy_msqid_from_user() initializes
> > it properly.
> 
> Yes, but it's used by ipcctl_pre_down, which from what I can see is
> only using those arguments when cmd == IPC_SET, so everything is ok.
> But still, I don't think the compiler _should_ know what
> ipcctl_pre_down is going to do, if you look _only_ at msgctl_down,
> then uninitialized_var is OK.

Well at least older compilers were able to see this, only 4.4 seems to be 
warning here ... I have applied the patch for now, but I am really not 
fully convinced about it yet, we should probably report it to gcc folks 
anyway.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 5/7] acpi: fix a bunch of style issues on 'actypes.h'

[Artem Bityutskiy]

On Mon, 2009-10-19 at 17:03 +0200, Jiri Kosina wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
> 
> > > I have never been in favor of merging whitespace-only patches (in a 
> > > sense that the sole purpose of them being to change whitespaces, but 
> > > no else value added).
> > If somebody tries to send a patch for that file that doesn't fix the
> > white-space, checkpatch will complain, and people will complain that
> > checkpatch complains, which is precisely what happened, 
> 
> Oh, well ... checkpatch warning about this is somewhat controversial. My 
> preferred way would be that it warns about whitespace only if there are 
> also some other (non-whitespace) changes.
> 
> > and I was requested to write this patch by Daniel Walker (final mail 
> > wasn't on the ml):
> > 
> > http://lkml.org/lkml/2009/9/14/183
> 
> This is something slightly different -- he asks you to fixup whitespace 
> issue in the code you are newly introducing, right?
> 
> > > And after today's discussion on kernel summit on this topic, I wouldn't
> > > expect any maintainer to merge it, sorry :)
> > What are you talking about?
> 
> Seems like many kernel maintainers are just tired of 
> 'whitespace-cleanup-only' patches that bring no real added value 
> otherwise.

May be some are tired, but others just say thanks and apply them,
because it is easier to apply than complain, and because they do not
mind if their subsystem becomes a tiny bit cleaner. Sometimes it may
cause troubles, but hey, development is not easy and we are accustomed
to fix conflict and amend patches. But the include/acpi/actypes.h does
not seem to be changing very often anyway.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 4/7] acpi: processor: fix section mismatch

[Jiri Kosina]

On Mon, 19 Oct 2009, Felipe Contreras wrote:

> WARNING: drivers/acpi/processor.o(.text+0x99a): Section mismatch in
> reference from the function acpi_processor_add() to the function
> .cpuinit.text:acpi_processor_power_init()
> 
> acpi_processor_add replaced acpi_processor_start which had __cpuinit.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  drivers/acpi/processor_core.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
> index c567b46..ec742a4 100644
> --- a/drivers/acpi/processor_core.c
> +++ b/drivers/acpi/processor_core.c
> @@ -770,7 +770,7 @@ static struct notifier_block acpi_cpu_notifier =
>  	    .notifier_call = acpi_cpu_soft_notify,
>  };
>  
> -static int acpi_processor_add(struct acpi_device *device)
> +static int __cpuinit acpi_processor_add(struct acpi_device *device)
>  {
>  	struct acpi_processor *pr = NULL;
>  	int result = 0;

I don't see this patch in today's linux-next ... ACPI guys, are you going 
to merge it?

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 2/7] ipc: fix trivial warning

[Felipe Contreras]

On Tue, Oct 20, 2009 at 12:50 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>> Yes, but it's used by ipcctl_pre_down, which from what I can see is
>> only using those arguments when cmd == IPC_SET, so everything is ok.
>> But still, I don't think the compiler _should_ know what
>> ipcctl_pre_down is going to do, if you look _only_ at msgctl_down,
>> then uninitialized_var is OK.
>
> Well at least older compilers were able to see this, only 4.4 seems to be
> warning here ... I have applied the patch for now, but I am really not
> fully convinced about it yet, we should probably report it to gcc folks
> anyway.

I have in my to-do list to report this to the gcc guys, but in the
meantime I don't see this patch applied.

-- 
Felipe Contreras

Re: [PATCH 4/7] acpi: processor: fix section mismatch

[Felipe Contreras]

On Wed, Nov 4, 2009 at 3:28 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> I don't see this patch in today's linux-next ... ACPI guys, are you going
> to merge it?

How critical are these section mismatch issues? Should this go into 2.6.32?

-- 
Felipe Contreras

Re: [PATCH 3/7] crypto: testmgr: fix warning

[Felipe Contreras]

On Mon, Oct 19, 2009 at 4:03 PM, Jarod Wilson <jarod@redhat.com> wrote:
> So yeah, I rescind my claim that initializing err to 0 is incorrect, I think
> that's just fine.

So is this acked? Who will merge it?

-- 
Felipe Contreras

Re: [PATCH 3/7] crypto: testmgr: fix warning

[Herbert Xu]

On Thu, Nov 12, 2009 at 02:32:10AM +0200, Felipe Contreras wrote:
> On Mon, Oct 19, 2009 at 4:03 PM, Jarod Wilson <jarod@redhat.com> wrote:
> > So yeah, I rescind my claim that initializing err to 0 is incorrect, I think
> > that's just fine.
> 
> So is this acked? Who will merge it?

It's already in cryptodev-2.6:

commit fa4ef8a6af4745bbf3a25789bc7d4f14a3a6d803
Author: Felipe Contreras <felipe.contreras@gmail.com>
Date:   Tue Oct 27 19:04:42 2009 +0800

    crypto: testmgr - Fix warning

    crypto/testmgr.c: In function ‘test_cprng’:
    crypto/testmgr.c:1204: warning: ‘err’ may be used uninitialized in this function

    Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 2/7] ipc: fix trivial warning

[Jiri Kosina]

On Thu, 12 Nov 2009, Felipe Contreras wrote:

> > Well at least older compilers were able to see this, only 4.4 seems to be
> > warning here ... I have applied the patch for now, but I am really not
> > fully convinced about it yet, we should probably report it to gcc folks
> > anyway.
> 
> I have in my to-do list to report this to the gcc guys, but in the
> meantime I don't see this patch applied.

It is applied in my tree, scheduled for next merge window.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 5/7] acpi: fix a bunch of style issues on 'actypes.h'

[Daniel Walker]

On Mon, 2009-10-19 at 16:20 +0200, Jiri Kosina wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
> 
> > Many still remain.
> > 
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> 
> I have never been in favor of merging whitespace-only patches (in a sense 
> that the sole purpose of them being to change whitespaces, but no else 
> value added). 
> 
> And after today's discussion on kernel summit on this topic, I wouldn't 
> expect any maintainer to merge it, sorry :)

could you be more specific about what "discussion" you are talking
about.

Daniel

RE: [PATCH 5/7] acpi: fix a bunch of style issues on 'actypes.h'

[Moore, Robert]

>-----Original Message-----
>From: Daniel Walker [mailto:dwalker@fifo99.com]
>Sent: Friday, November 13, 2009 12:59 PM
>To: Jiri Kosina
>Cc: Felipe Contreras; linux-kernel@vger.kernel.org; Len Brown; Moore,
>Robert; Lin, Ming M; linux-acpi@vger.kernel.org
>Subject: Re: [PATCH 5/7] acpi: fix a bunch of style issues on 'actypes.h'
>
>On Mon, 2009-10-19 at 16:20 +0200, Jiri Kosina wrote:
>> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>>
>> > Many still remain.
>> >
>> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>
>> I have never been in favor of merging whitespace-only patches (in a sense
>> that the sole purpose of them being to change whitespaces, but no else
>> value added).
>>
>> And after today's discussion on kernel summit on this topic, I wouldn't
>> expect any maintainer to merge it, sorry :)
>
>could you be more specific about what "discussion" you are talking
>about.
>

Yes, do tell.


>Daniel

RE: [PATCH 5/7] acpi: fix a bunch of style issues on 'actypes.h'

[Jiri Kosina]

On Fri, 13 Nov 2009, Moore, Robert wrote:

> >> And after today's discussion on kernel summit on this topic, I 
> >> wouldn't expect any maintainer to merge it, sorry :)
> >could you be more specific about what "discussion" you are talking
> >about.
> Yes, do tell.

I have probably lost the track of the context here already, sorry.

But it probably had something to do with the fact that whitespace-only 
patches (which [PATCH 5/7] acpi: fix a bunch of style issues on 
'actypes.h' has been, as far as I remember), are unnecessary noise.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

RE: [PATCH 5/7] acpi: fix a bunch of style issues on 'actypes.h'

[Daniel Walker]

On Fri, 2009-11-13 at 22:41 +0100, Jiri Kosina wrote:
> On Fri, 13 Nov 2009, Moore, Robert wrote:
> 
> > >> And after today's discussion on kernel summit on this topic, I 
> > >> wouldn't expect any maintainer to merge it, sorry :)
> > >could you be more specific about what "discussion" you are talking
> > >about.
> > Yes, do tell.
> 
> I have probably lost the track of the context here already, sorry.
> 
> But it probably had something to do with the fact that whitespace-only 
> patches (which [PATCH 5/7] acpi: fix a bunch of style issues on 
> 'actypes.h' has been, as far as I remember), are unnecessary noise.

I don't think it was clear from the description , but I think this is
actually a checkpatch cleanup. So it's not just random whitespace
changes. AFAIK, maintainers do accept those.

Daniel