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(-)
It seems 'min_t' was used before, and looks cleaner, plus white-space
stuff.
Signed-off-by: Felipe Contreras <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Many still remain.
Signed-off-by: Felipe Contreras <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Mon, Oct 19, 2009 at 1:54 AM, Felipe Contreras
<[email protected]> wrote:
> It seems 'min_t' was used before, and looks cleaner, plus white-space
> stuff.
>
> Signed-off-by: Felipe Contreras <[email protected]>
> ---
> 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
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 <[email protected]>
But this doesn't fix a kernel warning, does it? If so, what one?
thanks,
greg k-h
On Mon, Oct 19, 2009 at 3:02 AM, Greg KH <[email protected]> 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 <[email protected]>
>
> 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
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 <[email protected]>
> ---
> 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.
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<[email protected]>
>> ---
>> 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
[email protected]
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
On Mon, Oct 19, 2009 at 4:59 PM, Alan Stern <[email protected]> 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
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<[email protected]>
>>> ---
>>> 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
[email protected]
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 <[email protected]>
> ---
> 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.
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
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.
On Mon, 19 Oct 2009, Felipe Contreras wrote:
> Many still remain.
>
> Signed-off-by: Felipe Contreras <[email protected]>
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.
On Mon, Oct 19, 2009 at 5:12 PM, Jiri Kosina <[email protected]> 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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> >> ---
> >> 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.
On Mon, Oct 19, 2009 at 5:20 PM, Jiri Kosina <[email protected]> wrote:
> On Mon, 19 Oct 2009, Felipe Contreras wrote:
>
>> Many still remain.
>>
>> Signed-off-by: Felipe Contreras <[email protected]>
>
> 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
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.
On Mon, Oct 19, 2009 at 5:57 PM, Jiri Kosina <[email protected]> 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 <[email protected]>
>> >> ---
>> >> 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
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 <[email protected]>
> >> >> ---
> >> >> 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.
On Mon, Oct 19, 2009 at 6:03 PM, Jiri Kosina <[email protected]> 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
On Mon, Oct 19, 2009 at 6:29 PM, Jiri Kosina <[email protected]> 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 <[email protected]>
>> >> >> ---
>> >> >> 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
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.
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 (Артём Битюцкий)
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 <[email protected]>
> ---
> 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.
On Tue, Oct 20, 2009 at 12:50 AM, Jiri Kosina <[email protected]> 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
On Wed, Nov 4, 2009 at 3:28 PM, Jiri Kosina <[email protected]> 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
On Mon, Oct 19, 2009 at 4:03 PM, Jarod Wilson <[email protected]> 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
On Thu, Nov 12, 2009 at 02:32:10AM +0200, Felipe Contreras wrote:
> On Mon, Oct 19, 2009 at 4:03 PM, Jarod Wilson <[email protected]> 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 <[email protected]>
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 <[email protected]>
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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.
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 <[email protected]>
>
> 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
>-----Original Message-----
>From: Daniel Walker [mailto:[email protected]]
>Sent: Friday, November 13, 2009 12:59 PM
>To: Jiri Kosina
>Cc: Felipe Contreras; [email protected]; Len Brown; Moore,
>Robert; Lin, Ming M; [email protected]
>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 <[email protected]>
>>
>> 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
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.
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