* [PATCH 1/5]security:key.c Fix warning: variable 'key' set but not used
2010-06-27 6:47 [PATCH 0/5] Fix gcc 4.6.0 set but unused variable warnings Justin P. Mattock
@ 2010-06-27 6:47 ` Justin P. Mattock
2010-06-28 13:05 ` [PATCH] KEYS: Use the variable 'key' in keyctl_describe_key() David Howells
2010-06-27 6:47 ` [PATCH 2/5]security:ebitmap.c Fix warning: variable 'e_sft' set but not used Justin P. Mattock
` (8 subsequent siblings)
9 siblings, 1 reply; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-27 6:47 UTC (permalink / raw)
To: linux-kernel; +Cc: dhowells, sds, lenb, linux-bluetooth, Justin P. Mattock
building with gcc 4.6 I'm getting a warning message:
CC security/keys/keyctl.o
security/keys/keyctl.c: In function 'keyctl_describe_key':
security/keys/keyctl.c:472:14: warning: variable 'key' set but not used
After reading key.h I noticed it says this:
NOTE! key_ref_t is a typedef'd pointer to a type that is not actually
defined. This is because we abuse the bottom bit of the reference to carry a
flag to indicate whether the calling process possesses that key in one of
its keyrings.
In this case the safest approach(in my mind) would be to just
mark the integer __unused. Keep in mind though Im not certain
if this is the right place for this value i.e. will this effect
*instkey or not(please check).
Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
---
security/keys/keyctl.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 13074b4..d7bb74f 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -469,7 +469,7 @@ long keyctl_describe_key(key_serial_t keyid,
char __user *buffer,
size_t buflen)
{
- struct key *key, *instkey;
+ struct key *key __attribute__((unused)), *instkey;
key_ref_t key_ref;
char *tmpbuf;
long ret;
--
1.7.1.rc1.21.gf3bd6
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH] KEYS: Use the variable 'key' in keyctl_describe_key()
2010-06-27 6:47 ` [PATCH 1/5]security:key.c Fix warning: variable 'key' set but not used Justin P. Mattock
@ 2010-06-28 13:05 ` David Howells
2010-06-28 23:01 ` [Keyrings] " James Morris
0 siblings, 1 reply; 38+ messages in thread
From: David Howells @ 2010-06-28 13:05 UTC (permalink / raw)
To: linux-security-module; +Cc: keyrings, dhowells, justinmattock, linux-kernel
keyctl_describe_key() turns the key reference it gets into a usable key pointer
and assigns that to a variable called 'key', which it then ignores in favour of
recomputing the key pointer each time it needs it. Make it use the precomputed
pointer instead.
Without this patch, gcc 4.6 reports that the variable key is set but not used:
building with gcc 4.6 I'm getting a warning message:
CC security/keys/keyctl.o
security/keys/keyctl.c: In function 'keyctl_describe_key':
security/keys/keyctl.c:472:14: warning: variable 'key' set but not used
Reported-by: Justin P. Mattock <justinmattock@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---
security/keys/keyctl.c | 12 +++++-------
1 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 639226a..b2b0998 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -505,13 +505,11 @@ okay:
ret = snprintf(tmpbuf, PAGE_SIZE - 1,
"%s;%d;%d;%08x;%s",
- key_ref_to_ptr(key_ref)->type->name,
- key_ref_to_ptr(key_ref)->uid,
- key_ref_to_ptr(key_ref)->gid,
- key_ref_to_ptr(key_ref)->perm,
- key_ref_to_ptr(key_ref)->description ?
- key_ref_to_ptr(key_ref)->description : ""
- );
+ key->type->name,
+ key->uid,
+ key->gid,
+ key->perm,
+ key->description ?: "");
/* include a NUL char at the end of the data */
if (ret > PAGE_SIZE - 1)
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [Keyrings] [PATCH] KEYS: Use the variable 'key' in keyctl_describe_key()
2010-06-28 13:05 ` [PATCH] KEYS: Use the variable 'key' in keyctl_describe_key() David Howells
@ 2010-06-28 23:01 ` James Morris
0 siblings, 0 replies; 38+ messages in thread
From: James Morris @ 2010-06-28 23:01 UTC (permalink / raw)
To: David Howells
Cc: linux-security-module, keyrings, linux-kernel, justinmattock
On Mon, 28 Jun 2010, David Howells wrote:
> keyctl_describe_key() turns the key reference it gets into a usable key pointer
> and assigns that to a variable called 'key', which it then ignores in favour of
> recomputing the key pointer each time it needs it. Make it use the precomputed
> pointer instead.
>
> Without this patch, gcc 4.6 reports that the variable key is set but not used:
>
> building with gcc 4.6 I'm getting a warning message:
> CC security/keys/keyctl.o
> security/keys/keyctl.c: In function 'keyctl_describe_key':
> security/keys/keyctl.c:472:14: warning: variable 'key' set but not used
>
> Reported-by: Justin P. Mattock <justinmattock@gmail.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 2/5]security:ebitmap.c Fix warning: variable 'e_sft' set but not used
2010-06-27 6:47 [PATCH 0/5] Fix gcc 4.6.0 set but unused variable warnings Justin P. Mattock
2010-06-27 6:47 ` [PATCH 1/5]security:key.c Fix warning: variable 'key' set but not used Justin P. Mattock
@ 2010-06-27 6:47 ` Justin P. Mattock
2010-06-27 6:47 ` [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' " Justin P. Mattock
` (7 subsequent siblings)
9 siblings, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-27 6:47 UTC (permalink / raw)
To: linux-kernel; +Cc: dhowells, sds, lenb, linux-bluetooth, Justin P. Mattock
gcc is giving me this during compiling:
CC security/selinux/ss/ebitmap.o
security/selinux/ss/ebitmap.c: In function 'ebitmap_netlbl_import':
security/selinux/ss/ebitmap.c:159:27: warning: variable 'e_sft' set but not used
The below fixes this warning for me.
(please check this whenever you have time).
Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
---
security/selinux/ss/ebitmap.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
index 04b6145..b7980ee 100644
--- a/security/selinux/ss/ebitmap.c
+++ b/security/selinux/ss/ebitmap.c
@@ -156,7 +156,7 @@ int ebitmap_netlbl_import(struct ebitmap *ebmap,
struct ebitmap_node *e_iter = NULL;
struct ebitmap_node *emap_prev = NULL;
struct netlbl_lsm_secattr_catmap *c_iter = catmap;
- u32 c_idx, c_pos, e_idx, e_sft;
+ u32 c_idx, c_pos, e_idx;
/* NetLabel's NETLBL_CATMAP_MAPTYPE is defined as an array of u64,
* however, it is not always compatible with an array of unsigned long
@@ -190,7 +190,6 @@ int ebitmap_netlbl_import(struct ebitmap *ebmap,
}
delta = c_pos - e_iter->startbit;
e_idx = delta / EBITMAP_UNIT_SIZE;
- e_sft = delta % EBITMAP_UNIT_SIZE;
while (map) {
e_iter->maps[e_idx++] |= map & (-1UL);
map = EBITMAP_SHIFT_UNIT_SIZE(map);
--
1.7.1.rc1.21.gf3bd6
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-06-27 6:47 [PATCH 0/5] Fix gcc 4.6.0 set but unused variable warnings Justin P. Mattock
2010-06-27 6:47 ` [PATCH 1/5]security:key.c Fix warning: variable 'key' set but not used Justin P. Mattock
2010-06-27 6:47 ` [PATCH 2/5]security:ebitmap.c Fix warning: variable 'e_sft' set but not used Justin P. Mattock
@ 2010-06-27 6:47 ` Justin P. Mattock
2010-06-27 6:47 ` [PATCH 4/5]block:cryptoloop Fix warning: variable 'cipher' " Justin P. Mattock
` (6 subsequent siblings)
9 siblings, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-27 6:47 UTC (permalink / raw)
To: linux-kernel; +Cc: dhowells, sds, lenb, linux-bluetooth, Justin P. Mattock
Im getting a warning message when building with gcc 4.6.0
CC drivers/acpi/glue.o
drivers/acpi/glue.c: In function 'acpi_bind_one':
drivers/acpi/glue.c:163:7: warning: variable 'ret' set but not used
The below fixes it for me. Please have a look when you have
time and let me know(bit confused with the backwardness of
&acpi_dev->dev.kobj, &dev->kobji with these two ret's)
Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
---
drivers/acpi/glue.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 4af6301..f146165 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -166,6 +166,9 @@ static int acpi_bind_one(struct device *dev, acpi_handle handle)
"firmware_node");
ret = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
"physical_node");
+ if (ret) {
+ printk(KERN_WARNING "dev%d: Failed to get exception\n", ret);
+ }
if (acpi_dev->wakeup.flags.valid) {
device_set_wakeup_capable(dev, true);
device_set_wakeup_enable(dev,
--
1.7.1.rc1.21.gf3bd6
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH 4/5]block:cryptoloop Fix warning: variable 'cipher' set but not used
2010-06-27 6:47 [PATCH 0/5] Fix gcc 4.6.0 set but unused variable warnings Justin P. Mattock
` (2 preceding siblings ...)
2010-06-27 6:47 ` [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' " Justin P. Mattock
@ 2010-06-27 6:47 ` Justin P. Mattock
2010-06-27 6:47 ` [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined Justin P. Mattock
` (5 subsequent siblings)
9 siblings, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-27 6:47 UTC (permalink / raw)
To: linux-kernel; +Cc: dhowells, sds, lenb, linux-bluetooth, Justin P. Mattock
Im getting this when compiling on gcc 4.6.0
CC [M] drivers/block/cryptoloop.o
drivers/block/cryptoloop.c: In function 'cryptoloop_init':
drivers/block/cryptoloop.c:46:8: warning: variable 'cipher' set but not used
The below fixes it for me. Please have a look and let me know.
Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
---
drivers/block/cryptoloop.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/drivers/block/cryptoloop.c b/drivers/block/cryptoloop.c
index 8b6bb76..fb8a6fd 100644
--- a/drivers/block/cryptoloop.c
+++ b/drivers/block/cryptoloop.c
@@ -43,7 +43,6 @@ cryptoloop_init(struct loop_device *lo, const struct loop_info64 *info)
int cipher_len;
int mode_len;
char cms[LO_NAME_SIZE]; /* cipher-mode string */
- char *cipher;
char *mode;
char *cmsp = cms; /* c-m string pointer */
struct crypto_blkcipher *tfm;
@@ -56,7 +55,6 @@ cryptoloop_init(struct loop_device *lo, const struct loop_info64 *info)
strncpy(cms, info->lo_crypt_name, LO_NAME_SIZE);
cms[LO_NAME_SIZE - 1] = 0;
- cipher = cmsp;
cipher_len = strcspn(cmsp, "-");
mode = cmsp + cipher_len;
--
1.7.1.rc1.21.gf3bd6
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined
2010-06-27 6:47 [PATCH 0/5] Fix gcc 4.6.0 set but unused variable warnings Justin P. Mattock
` (3 preceding siblings ...)
2010-06-27 6:47 ` [PATCH 4/5]block:cryptoloop Fix warning: variable 'cipher' " Justin P. Mattock
@ 2010-06-27 6:47 ` Justin P. Mattock
2010-06-27 7:31 ` Gustavo F. Padovan
2010-06-28 12:57 ` [PATCH] Bluetooth: Fix abuse of the preincrement operator David Howells
2010-06-28 12:38 ` [PATCH 1/5]security:key.c Fix warning: variable 'key' set but not used David Howells
` (4 subsequent siblings)
9 siblings, 2 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-27 6:47 UTC (permalink / raw)
To: linux-kernel; +Cc: dhowells, sds, lenb, linux-bluetooth, Justin P. Mattock
Im seeing this building the kernel with gcc 4.6.0
CC [M] drivers/bluetooth/hci_bcsp.o
drivers/bluetooth/hci_bcsp.c: In function 'bcsp_prepare_pkt':
drivers/bluetooth/hci_bcsp.c:247:20: warning: operation on 'bcsp->msgq_txseq' may be undefined
Hopefully the below is a fix for this. Please let me know.
Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
---
drivers/bluetooth/hci_bcsp.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
index 40aec0f..0f892e7 100644
--- a/drivers/bluetooth/hci_bcsp.c
+++ b/drivers/bluetooth/hci_bcsp.c
@@ -182,7 +182,7 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
struct sk_buff *nskb;
u8 hdr[4], chan;
u16 BCSP_CRC_INIT(bcsp_txmsg_crc);
- int rel, i;
+ int rel, i, ret;
switch (pkt_type) {
case HCI_ACLDATA_PKT:
@@ -243,8 +243,8 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
if (rel) {
hdr[0] |= 0x80 + bcsp->msgq_txseq;
- BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
- bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
+ BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq | ret);
+ ret = ++(bcsp->msgq_txseq) & 0x07;
}
if (bcsp->use_crc)
--
1.7.1.rc1.21.gf3bd6
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined
2010-06-27 6:47 ` [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined Justin P. Mattock
@ 2010-06-27 7:31 ` Gustavo F. Padovan
2010-06-28 12:57 ` [PATCH] Bluetooth: Fix abuse of the preincrement operator David Howells
1 sibling, 0 replies; 38+ messages in thread
From: Gustavo F. Padovan @ 2010-06-27 7:31 UTC (permalink / raw)
To: Justin P. Mattock; +Cc: linux-kernel, dhowells, sds, lenb, linux-bluetooth
Hi Justin,
* Justin P. Mattock <justinmattock@gmail.com> [2010-06-26 23:47:26 -0700]:
> Im seeing this building the kernel with gcc 4.6.0
> CC [M] drivers/bluetooth/hci_bcsp.o
> drivers/bluetooth/hci_bcsp.c: In function 'bcsp_prepare_pkt':
> drivers/bluetooth/hci_bcsp.c:247:20: warning: operation on 'bcsp->msgq_txseq' may be undefined
>
> Hopefully the below is a fix for this. Please let me know.
> Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
>
> ---
> drivers/bluetooth/hci_bcsp.c | 6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
> index 40aec0f..0f892e7 100644
> --- a/drivers/bluetooth/hci_bcsp.c
> +++ b/drivers/bluetooth/hci_bcsp.c
> @@ -182,7 +182,7 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
> struct sk_buff *nskb;
> u8 hdr[4], chan;
> u16 BCSP_CRC_INIT(bcsp_txmsg_crc);
> - int rel, i;
> + int rel, i, ret;
>
> switch (pkt_type) {
> case HCI_ACLDATA_PKT:
> @@ -243,8 +243,8 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
>
> if (rel) {
> hdr[0] |= 0x80 + bcsp->msgq_txseq;
> - BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
> - bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
> + BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq | ret);
> + ret = ++(bcsp->msgq_txseq) & 0x07;
> }
What are trying to do here? That is completely wrong, you are losting
the next txseq to be sent.
And please do not bother the linux-bluetooth mailing list with patches
to other subsystems. Send them in a way that only Bluetooth patches will
come to linux-bluetooth.
Regards,
--
Gustavo F. Padovan
http://padovan.org
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH] Bluetooth: Fix abuse of the preincrement operator
2010-06-27 6:47 ` [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined Justin P. Mattock
2010-06-27 7:31 ` Gustavo F. Padovan
@ 2010-06-28 12:57 ` David Howells
2010-06-28 13:12 ` Gustavo F. Padovan
2010-06-28 17:44 ` Justin P. Mattock
1 sibling, 2 replies; 38+ messages in thread
From: David Howells @ 2010-06-28 12:57 UTC (permalink / raw)
To: linux-bluetooth; +Cc: justinmattock, dhowells, linux-kernel, gustavo
Fix abuse of the preincrement operator as detected when building with gcc
4.6.0:
CC [M] drivers/bluetooth/hci_bcsp.o
drivers/bluetooth/hci_bcsp.c: In function 'bcsp_prepare_pkt':
drivers/bluetooth/hci_bcsp.c:247:20: warning: operation on 'bcsp->msgq_txseq' may be undefined
Reported-by: Justin P. Mattock <justinmattock@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---
drivers/bluetooth/hci_bcsp.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
index 40aec0f..42d69d4 100644
--- a/drivers/bluetooth/hci_bcsp.c
+++ b/drivers/bluetooth/hci_bcsp.c
@@ -244,7 +244,7 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
if (rel) {
hdr[0] |= 0x80 + bcsp->msgq_txseq;
BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
- bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
+ bcsp->msgq_txseq = (bcsp->msgq_txseq + 1) & 0x07;
}
if (bcsp->use_crc)
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH] Bluetooth: Fix abuse of the preincrement operator
2010-06-28 12:57 ` [PATCH] Bluetooth: Fix abuse of the preincrement operator David Howells
@ 2010-06-28 13:12 ` Gustavo F. Padovan
2010-06-30 20:10 ` David Miller
2010-06-28 17:44 ` Justin P. Mattock
1 sibling, 1 reply; 38+ messages in thread
From: Gustavo F. Padovan @ 2010-06-28 13:12 UTC (permalink / raw)
To: David Howells; +Cc: linux-bluetooth, justinmattock, linux-kernel
Hi David,
* David Howells <dhowells@redhat.com> [2010-06-28 13:57:52 +0100]:
> Fix abuse of the preincrement operator as detected when building with gcc
> 4.6.0:
>
> CC [M] drivers/bluetooth/hci_bcsp.o
> drivers/bluetooth/hci_bcsp.c: In function 'bcsp_prepare_pkt':
> drivers/bluetooth/hci_bcsp.c:247:20: warning: operation on 'bcsp->msgq_txseq' may be undefined
>
> Reported-by: Justin P. Mattock <justinmattock@gmail.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
> drivers/bluetooth/hci_bcsp.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
> index 40aec0f..42d69d4 100644
> --- a/drivers/bluetooth/hci_bcsp.c
> +++ b/drivers/bluetooth/hci_bcsp.c
> @@ -244,7 +244,7 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
> if (rel) {
> hdr[0] |= 0x80 + bcsp->msgq_txseq;
> BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
> - bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
> + bcsp->msgq_txseq = (bcsp->msgq_txseq + 1) & 0x07;
> }
>
> if (bcsp->use_crc)
>
Acked-by: Gustavo F. Padovan <padovan@profusion.mobi>
--
Gustavo F. Padovan
http://padovan.org
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH] Bluetooth: Fix abuse of the preincrement operator
2010-06-28 13:12 ` Gustavo F. Padovan
@ 2010-06-30 20:10 ` David Miller
0 siblings, 0 replies; 38+ messages in thread
From: David Miller @ 2010-06-30 20:10 UTC (permalink / raw)
To: gustavo; +Cc: dhowells, linux-bluetooth, justinmattock, linux-kernel
From: "Gustavo F. Padovan" <gustavo@padovan.org>
Date: Mon, 28 Jun 2010 10:12:30 -0300
> Hi David,
>
> * David Howells <dhowells@redhat.com> [2010-06-28 13:57:52 +0100]:
>
>> Fix abuse of the preincrement operator as detected when building with gcc
>> 4.6.0:
>>
>> CC [M] drivers/bluetooth/hci_bcsp.o
>> drivers/bluetooth/hci_bcsp.c: In function 'bcsp_prepare_pkt':
>> drivers/bluetooth/hci_bcsp.c:247:20: warning: operation on 'bcsp->msgq_txseq' may be undefined
>>
>> Reported-by: Justin P. Mattock <justinmattock@gmail.com>
>> Signed-off-by: David Howells <dhowells@redhat.com>
...
> Acked-by: Gustavo F. Padovan <padovan@profusion.mobi>
Applied, thanks everyone.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] Bluetooth: Fix abuse of the preincrement operator
2010-06-28 12:57 ` [PATCH] Bluetooth: Fix abuse of the preincrement operator David Howells
2010-06-28 13:12 ` Gustavo F. Padovan
@ 2010-06-28 17:44 ` Justin P. Mattock
1 sibling, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-28 17:44 UTC (permalink / raw)
To: David Howells; +Cc: linux-bluetooth, linux-kernel, gustavo
On 06/28/2010 05:57 AM, David Howells wrote:
> Fix abuse of the preincrement operator as detected when building with gcc
> 4.6.0:
>
> CC [M] drivers/bluetooth/hci_bcsp.o
> drivers/bluetooth/hci_bcsp.c: In function 'bcsp_prepare_pkt':
> drivers/bluetooth/hci_bcsp.c:247:20: warning: operation on 'bcsp->msgq_txseq' may be undefined
>
> Reported-by: Justin P. Mattock<justinmattock@gmail.com>
> Signed-off-by: David Howells<dhowells@redhat.com>
> ---
>
> drivers/bluetooth/hci_bcsp.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
> index 40aec0f..42d69d4 100644
> --- a/drivers/bluetooth/hci_bcsp.c
> +++ b/drivers/bluetooth/hci_bcsp.c
> @@ -244,7 +244,7 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
> if (rel) {
> hdr[0] |= 0x80 + bcsp->msgq_txseq;
> BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
> - bcsp->msgq_txseq = ++(bcsp->msgq_txseq)& 0x07;
> + bcsp->msgq_txseq = (bcsp->msgq_txseq + 1)& 0x07;
> }
>
> if (bcsp->use_crc)
>
>
ahh.. so it's o.k. to add the value after bcsp->msgq_txseq instead of
before. Anyways build clean over here..
Thanks!
Justin P. Mattock
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/5]security:key.c Fix warning: variable 'key' set but not used
2010-06-27 6:47 [PATCH 0/5] Fix gcc 4.6.0 set but unused variable warnings Justin P. Mattock
` (4 preceding siblings ...)
2010-06-27 6:47 ` [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined Justin P. Mattock
@ 2010-06-28 12:38 ` David Howells
2010-06-28 17:48 ` Justin P. Mattock
2010-06-28 12:44 ` [PATCH 2/5]security:ebitmap.c Fix warning: variable 'e_sft' " David Howells
` (3 subsequent siblings)
9 siblings, 1 reply; 38+ messages in thread
From: David Howells @ 2010-06-28 12:38 UTC (permalink / raw)
To: Justin P. Mattock; +Cc: dhowells, linux-kernel, sds, lenb, linux-bluetooth
Justin P. Mattock <justinmattock@gmail.com> wrote:
> In this case the safest approach(in my mind) would be to just
> mark the integer __unused. Keep in mind though Im not certain
> if this is the right place for this value i.e. will this effect
> *instkey or not(please check).
This is the wrong approach. Either the variable should be got rid of, or it
should be used to replace all the other calls to key_ref_to_ptr() in
keyctl_describe_key().
David
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/5]security:key.c Fix warning: variable 'key' set but not used
2010-06-28 12:38 ` [PATCH 1/5]security:key.c Fix warning: variable 'key' set but not used David Howells
@ 2010-06-28 17:48 ` Justin P. Mattock
0 siblings, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-28 17:48 UTC (permalink / raw)
To: David Howells; +Cc: linux-kernel, sds, lenb, linux-bluetooth
On 06/28/2010 05:38 AM, David Howells wrote:
> Justin P. Mattock<justinmattock@gmail.com> wrote:
>
>> In this case the safest approach(in my mind) would be to just
>> mark the integer __unused. Keep in mind though Im not certain
>> if this is the right place for this value i.e. will this effect
>> *instkey or not(please check).
>
> This is the wrong approach. Either the variable should be got rid of, or it
> should be used to replace all the other calls to key_ref_to_ptr() in
> keyctl_describe_key().
>
> David
>
I see your patch you sent for this.. vary nice!
Thanks!
Justin P. Mattock
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 2/5]security:ebitmap.c Fix warning: variable 'e_sft' set but not used
2010-06-27 6:47 [PATCH 0/5] Fix gcc 4.6.0 set but unused variable warnings Justin P. Mattock
` (5 preceding siblings ...)
2010-06-28 12:38 ` [PATCH 1/5]security:key.c Fix warning: variable 'key' set but not used David Howells
@ 2010-06-28 12:44 ` David Howells
2010-06-28 17:49 ` Justin P. Mattock
2010-06-28 12:48 ` [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' " David Howells
` (2 subsequent siblings)
9 siblings, 1 reply; 38+ messages in thread
From: David Howells @ 2010-06-28 12:44 UTC (permalink / raw)
To: Justin P. Mattock; +Cc: dhowells, linux-kernel, sds, lenb, linux-bluetooth
Justin P. Mattock <justinmattock@gmail.com> wrote:
> gcc is giving me this during compiling:
> CC security/selinux/ss/ebitmap.o
> security/selinux/ss/ebitmap.c: In function 'ebitmap_netlbl_import':
> security/selinux/ss/ebitmap.c:159:27: warning: variable 'e_sft' set but not used
>
> The below fixes this warning for me.
> (please check this whenever you have time).
> Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
Acked-by: David Howells <dhowells@redhat.com>
(though I wonder if e_sft should be used for something...)
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 2/5]security:ebitmap.c Fix warning: variable 'e_sft' set but not used
2010-06-28 12:44 ` [PATCH 2/5]security:ebitmap.c Fix warning: variable 'e_sft' " David Howells
@ 2010-06-28 17:49 ` Justin P. Mattock
0 siblings, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-28 17:49 UTC (permalink / raw)
To: David Howells; +Cc: linux-kernel, sds, lenb, linux-bluetooth
On 06/28/2010 05:44 AM, David Howells wrote:
> Justin P. Mattock<justinmattock@gmail.com> wrote:
>
>> gcc is giving me this during compiling:
>> CC security/selinux/ss/ebitmap.o
>> security/selinux/ss/ebitmap.c: In function 'ebitmap_netlbl_import':
>> security/selinux/ss/ebitmap.c:159:27: warning: variable 'e_sft' set but not used
>>
>> The below fixes this warning for me.
>> (please check this whenever you have time).
>> Signed-off-by: Justin P. Mattock<justinmattock@gmail.com>
>
> Acked-by: David Howells<dhowells@redhat.com>
>
> (though I wonder if e_sft should be used for something...)
>
alright..
Justin P. Mattock
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-06-27 6:47 [PATCH 0/5] Fix gcc 4.6.0 set but unused variable warnings Justin P. Mattock
` (6 preceding siblings ...)
2010-06-28 12:44 ` [PATCH 2/5]security:ebitmap.c Fix warning: variable 'e_sft' " David Howells
@ 2010-06-28 12:48 ` David Howells
2010-06-28 17:52 ` Justin P. Mattock
` (2 more replies)
2010-06-28 12:49 ` [PATCH 4/5]block:cryptoloop Fix warning: variable 'cipher' " David Howells
2010-06-28 12:52 ` [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined David Howells
9 siblings, 3 replies; 38+ messages in thread
From: David Howells @ 2010-06-28 12:48 UTC (permalink / raw)
To: Justin P. Mattock; +Cc: dhowells, linux-kernel, sds, lenb, linux-bluetooth
Justin P. Mattock <justinmattock@gmail.com> wrote:
> + if (ret) {
> + printk(KERN_WARNING "dev%d: Failed to get exception\n", ret);
> + }
That's not a good warning because it's a meaningless string and you're passing
the error number to the dev%d.
Better would perhaps be:
"dev%d: Failed to create physical_node sysfs link: %d\n"
Note also that you're only checking the result of one sysfs_create_link().
You should probably check both.
Also you're introducing a pair of unnecessary braces as there's only one
statement in the if-body.
David
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-06-28 12:48 ` [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' " David Howells
@ 2010-06-28 17:52 ` Justin P. Mattock
2010-06-28 18:47 ` David Howells
2010-06-28 19:08 ` Justin P. Mattock
2 siblings, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-28 17:52 UTC (permalink / raw)
To: David Howells; +Cc: linux-kernel, sds, lenb, linux-bluetooth
On 06/28/2010 05:48 AM, David Howells wrote:
> Justin P. Mattock<justinmattock@gmail.com> wrote:
>
>> + if (ret) {
>> + printk(KERN_WARNING "dev%d: Failed to get exception\n", ret);
>> + }
>
> That's not a good warning because it's a meaningless string and you're passing
> the error number to the dev%d.
>
> Better would perhaps be:
>
> "dev%d: Failed to create physical_node sysfs link: %d\n"
>
I'll throw that in.
> Note also that you're only checking the result of one sysfs_create_link().
> You should probably check both.
>
this is where I get confused with: &acpi_dev->dev.kobj, &dev->kobji
it's like a criss cross of code
> Also you're introducing a pair of unnecessary braces as there's only one
> statement in the if-body.
>
> David
>
would just removing ret be good or will things go out of whack because
of no ret
Justin P. Mattock
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-06-28 12:48 ` [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' " David Howells
2010-06-28 17:52 ` Justin P. Mattock
@ 2010-06-28 18:47 ` David Howells
2010-06-28 19:03 ` Justin P. Mattock
` (2 more replies)
2010-06-28 19:08 ` Justin P. Mattock
2 siblings, 3 replies; 38+ messages in thread
From: David Howells @ 2010-06-28 18:47 UTC (permalink / raw)
To: Justin P. Mattock; +Cc: dhowells, linux-kernel, sds, lenb, linux-bluetooth
Justin P. Mattock <justinmattock@gmail.com> wrote:
> would just removing ret be good or will things go out of whack because of no
> ret
Don't you then get warnings for not capturing the result?
David
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-06-28 18:47 ` David Howells
@ 2010-06-28 19:03 ` Justin P. Mattock
2010-06-29 3:23 ` Justin P. Mattock
2010-06-29 15:47 ` David Howells
2 siblings, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-28 19:03 UTC (permalink / raw)
To: David Howells; +Cc: linux-kernel, sds, lenb, linux-bluetooth
On 06/28/2010 11:47 AM, David Howells wrote:
> Justin P. Mattock<justinmattock@gmail.com> wrote:
>
>> would just removing ret be good or will things go out of whack because of no
>> ret
>
> Don't you then get warnings for not capturing the result?
>
> David
>
not sure if it would give warnings for not capturing the result.
then doing this would be the way to go.
if (ret) {
printk(KERN_WARNING "dev%d: Failed to create physical_node sysfs link:
%d\n");
}
but like you had said unnecessary braces, and only one statement
I'll look at this some more too see if I come up with anything.
Justin P. Mattock
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-06-28 18:47 ` David Howells
2010-06-28 19:03 ` Justin P. Mattock
@ 2010-06-29 3:23 ` Justin P. Mattock
2010-06-29 15:47 ` David Howells
2 siblings, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-29 3:23 UTC (permalink / raw)
To: David Howells; +Cc: linux-kernel, linux-acpi, lenb
o.k. after stepping out for a while.. I'm finally sitting down and
looking at this. below is what I came up with.. hopefully it's in the
area/vecinity of what might be a good idea for this(if not then let me know)
From da5cfa463f29ff3fe4af3874649db0809e50ab96 Mon Sep 17 00:00:00 2001
From: Justin P. Mattock <justinmattock@gmail.com>
Date: Mon, 28 Jun 2010 20:14:50 -0700
Subject: [PATCH] glue.c
Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
---
drivers/acpi/glue.c | 12 +++++++++---
1 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 4af6301..970d7f3 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -145,6 +145,7 @@ static int acpi_bind_one(struct device *dev,
acpi_handle handle)
{
struct acpi_device *acpi_dev;
acpi_status status;
+ int fn, pn;
if (dev->archdata.acpi_handle) {
dev_warn(dev, "Drivers changed 'acpi_handle'\n");
@@ -160,12 +161,17 @@ static int acpi_bind_one(struct device *dev,
acpi_handle handle)
status = acpi_bus_get_device(handle, &acpi_dev);
if (!ACPI_FAILURE(status)) {
- int ret;
- ret = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
+ fn = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
"firmware_node");
- ret = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
+ pn = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
"physical_node");
+ if (fn) {
+ printk(KERN_WARNING "dev%d: Failed to create firmware_node: %d\n",
status, fn);
+ }else if (pn) {
+ printk(KERN_WARNING "dev%d: Failed to create physical_node: %d\n",
status, pn);
+ return 0;
+ }
if (acpi_dev->wakeup.flags.valid) {
device_set_wakeup_capable(dev, true);
device_set_wakeup_enable(dev,
--
1.7.1.rc1.21.gf3bd6
keep in mind Im not exactly sure what should go into the printk
as for words to say, and functions, so I just used status, fn/pn
for the two because I was getting a not enough function error.
Justin P. Mattock
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-06-28 18:47 ` David Howells
2010-06-28 19:03 ` Justin P. Mattock
2010-06-29 3:23 ` Justin P. Mattock
@ 2010-06-29 15:47 ` David Howells
2010-06-29 17:14 ` Justin P. Mattock
` (2 more replies)
2 siblings, 3 replies; 38+ messages in thread
From: David Howells @ 2010-06-29 15:47 UTC (permalink / raw)
To: Justin P. Mattock; +Cc: dhowells, linux-kernel, linux-acpi, lenb
Justin P. Mattock <justinmattock@gmail.com> wrote:
> + if (fn) {
> + printk(KERN_WARNING "dev%d: Failed to create
> firmware_node: %d\n", status, fn);
> + }else if (pn) {
> + printk(KERN_WARNING "dev%d: Failed to create
> physical_node: %d\n", status, pn);
> + return 0;
> + }
The if-statement should be correctly indented (it's inside another
if-body, so needs to be one more tab over) and there needs to be a space
before the else.
You should probably split your printks up so they don't exceed 80 chars too,
for example:
printk(KERN_WARNING
"dev%d: Failed to create physical_node: %d\n",
status, pn);
Also 'status' is probably the wrong thing to print as the number in "dev%d".
If it worked, that should be unconditionally AE_OK, I think. Can you not use
dev_warn() or similar instead or printk?
David
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-06-29 15:47 ` David Howells
@ 2010-06-29 17:14 ` Justin P. Mattock
2010-06-29 21:53 ` Justin P. Mattock
2010-06-30 9:13 ` David Howells
2 siblings, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-29 17:14 UTC (permalink / raw)
To: David Howells; +Cc: linux-kernel, linux-acpi, lenb
On 06/29/2010 08:47 AM, David Howells wrote:
> Justin P. Mattock<justinmattock@gmail.com> wrote:
>
>> + if (fn) {
>> + printk(KERN_WARNING "dev%d: Failed to create
>> firmware_node: %d\n", status, fn);
>> + }else if (pn) {
>> + printk(KERN_WARNING "dev%d: Failed to create
>> physical_node: %d\n", status, pn);
>> + return 0;
>> + }
>
> The if-statement should be correctly indented (it's inside another
> if-body, so needs to be one more tab over) and there needs to be a space
> before the else.
>
o.k., so it should look something like this:
if (fn) {
code...
} else if (pn) {
> You should probably split your printks up so they don't exceed 80 chars too,
> for example:
>
> printk(KERN_WARNING
> "dev%d: Failed to create physical_node: %d\n",
> status, pn);
>
> Also 'status' is probably the wrong thing to print as the number in "dev%d".
> If it worked, that should be unconditionally AE_OK, I think. Can you not use
> dev_warn() or similar instead or printk?
>
> David
>
alright, I'll look at this today. I'm not the best at making printks in
fact I'm more intimidated by them..(so with this in mind, I'm going to
sit and make myself learn this, so I atleast have a better idea of doing
these than I have now.)
Justin P. Mattock
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-06-29 15:47 ` David Howells
2010-06-29 17:14 ` Justin P. Mattock
@ 2010-06-29 21:53 ` Justin P. Mattock
2010-06-30 9:13 ` David Howells
2 siblings, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-29 21:53 UTC (permalink / raw)
To: David Howells; +Cc: linux-kernel, linux-acpi, lenb
On 06/29/2010 08:47 AM, David Howells wrote:
> Justin P. Mattock<justinmattock@gmail.com> wrote:
>
>> + if (fn) {
>> + printk(KERN_WARNING "dev%d: Failed to create
>> firmware_node: %d\n", status, fn);
>> + }else if (pn) {
>> + printk(KERN_WARNING "dev%d: Failed to create
>> physical_node: %d\n", status, pn);
>> + return 0;
>> + }
>
> The if-statement should be correctly indented (it's inside another
> if-body, so needs to be one more tab over) and there needs to be a space
> before the else.
>
> You should probably split your printks up so they don't exceed 80 chars too,
> for example:
>
> printk(KERN_WARNING
> "dev%d: Failed to create physical_node: %d\n",
> status, pn);
>
> Also 'status' is probably the wrong thing to print as the number in "dev%d".
> If it worked, that should be unconditionally AE_OK, I think. Can you not use
> dev_warn() or similar instead or printk?
>
> David
>
o.k. heres another go at this.. this goes through, using dev_warn
but am uncertain about using these three for the right info dev, %p,
dev_acpi
but give it a look whenever you have the time, and let me know
then I'll go from there..
From 34485b5709dc9ad18c57be8c672236580300e05c Mon Sep 17 00:00:00 2001
From: Justin P. Mattock <justinmattock@gmail.com>
Date: Tue, 29 Jun 2010 14:47:42 -0700
Subject: [PATCH ]ACPI:glue.c
Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
---
drivers/acpi/glue.c | 15 ++++++++++++---
1 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 4af6301..69ca24d 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -145,6 +145,7 @@ static int acpi_bind_one(struct device *dev,
acpi_handle handle)
{
struct acpi_device *acpi_dev;
acpi_status status;
+ int fn, pn;
if (dev->archdata.acpi_handle) {
dev_warn(dev, "Drivers changed 'acpi_handle'\n");
@@ -160,12 +161,20 @@ static int acpi_bind_one(struct device *dev,
acpi_handle handle)
status = acpi_bus_get_device(handle, &acpi_dev);
if (!ACPI_FAILURE(status)) {
- int ret;
- ret = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
+ fn = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
"firmware_node");
- ret = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
+ pn = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
"physical_node");
+ if (fn) {
+ dev_warn(dev, "dev:%p Failed to create firmware_node: %d\n",
+ acpi_dev, fn);
+
+ } else if (pn) {
+ dev_warn(dev, "dev:%p Failed to create physical_node: %d\n",
+ acpi_dev, pn);
+ return AE_ERROR;
+ }
if (acpi_dev->wakeup.flags.valid) {
device_set_wakeup_capable(dev, true);
device_set_wakeup_enable(dev,
--
1.7.1.rc1.21.gf3bd6
Justin P. Mattock
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-06-29 15:47 ` David Howells
2010-06-29 17:14 ` Justin P. Mattock
2010-06-29 21:53 ` Justin P. Mattock
@ 2010-06-30 9:13 ` David Howells
2010-06-30 13:21 ` Justin P. Mattock
` (2 more replies)
2 siblings, 3 replies; 38+ messages in thread
From: David Howells @ 2010-06-30 9:13 UTC (permalink / raw)
To: Justin P. Mattock; +Cc: dhowells, linux-kernel, linux-acpi, lenb
Justin P. Mattock <justinmattock@gmail.com> wrote:
> if (!ACPI_FAILURE(status)) {
> - int ret;
>
> - ret = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
> + fn = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
> "firmware_node");
> - ret = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
> + pn = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
> "physical_node");
> + if (fn) {
That new if-statement still needs indenting one more tab stop. It's indented
the same as the previous if-statement, but is actually in the body of that
previous if-statement.
The body of the second if-statement should be indented one tab beyond the if,
and else/else-if statements and the final closing brace should be indented
level with the if:
if (...) {
body;
} else if (...) {
body;
} else {
body;
}
so that they line up vertically.
> + dev_warn(dev, "dev:%p Failed to create firmware_node: %d\n",
> + acpi_dev, fn);
The "dev:%p " seems like it ought to be superfluous if you're using
dev_warn(), and certainly, returning the pointer isn't really useful, I
suspect.
However, at this point you have two device struct pointers: dev and
&acip_dev->dev, so printing them both is may be good. Perhaps something like:
+ dev_warn(&acpi_dev->dev,
+ "Failed to create firmware_node link to %s %s: %d\n",
+ dev_driver_string(dev), dev_name(dev), fn);
David
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-06-30 9:13 ` David Howells
@ 2010-06-30 13:21 ` Justin P. Mattock
2010-06-30 19:47 ` Justin P. Mattock
2010-07-01 9:31 ` David Howells
2 siblings, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-30 13:21 UTC (permalink / raw)
To: David Howells; +Cc: linux-kernel, linux-acpi, lenb
On 06/30/2010 02:13 AM, David Howells wrote:
> Justin P. Mattock<justinmattock@gmail.com> wrote:
>
>> if (!ACPI_FAILURE(status)) {
>> - int ret;
>>
>> - ret = sysfs_create_link(&dev->kobj,&acpi_dev->dev.kobj,
>> + fn = sysfs_create_link(&dev->kobj,&acpi_dev->dev.kobj,
>> "firmware_node");
>> - ret = sysfs_create_link(&acpi_dev->dev.kobj,&dev->kobj,
>> + pn = sysfs_create_link(&acpi_dev->dev.kobj,&dev->kobj,
>> "physical_node");
>> + if (fn) {
>
> That new if-statement still needs indenting one more tab stop. It's indented
> the same as the previous if-statement, but is actually in the body of that
> previous if-statement.
>
> The body of the second if-statement should be indented one tab beyond the if,
> and else/else-if statements and the final closing brace should be indented
> level with the if:
>
> if (...) {
> body;
> } else if (...) {
> body;
> } else {
> body;
> }
>
> so that they line up vertically.
Thanks for the info on this, I really appreciate it. I'll look at this
today, and resend.
>
>> + dev_warn(dev, "dev:%p Failed to create firmware_node: %d\n",
>> + acpi_dev, fn);
>
> The "dev:%p " seems like it ought to be superfluous if you're using
> dev_warn(), and certainly, returning the pointer isn't really useful, I
> suspect.
>
I kept receiving an new warning for using acpi_dev the %p was the only
option I saw in the Documentation that worked
> However, at this point you have two device struct pointers: dev and
> &acip_dev->dev, so printing them both is may be good. Perhaps something like:
>
> + dev_warn(&acpi_dev->dev,
> + "Failed to create firmware_node link to %s %s: %d\n",
> + dev_driver_string(dev), dev_name(dev), fn);
>
> David
>
o.k. I'll look at this today, and see if I can find/locate the device
name and string for these.
Justin P. Mattock
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-06-30 9:13 ` David Howells
2010-06-30 13:21 ` Justin P. Mattock
@ 2010-06-30 19:47 ` Justin P. Mattock
2010-07-01 9:31 ` David Howells
2 siblings, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-30 19:47 UTC (permalink / raw)
To: David Howells; +Cc: linux-kernel, linux-acpi, lenb
o.k. hopefully this is getting close to being right.. I ended up
spending the later half of the morning looking for
dev_driver_string(dev) until I read device.h and realized you actually
use that it's self to gather the debug info etc..(I admit it, I'm a newbie!)
anyways here is the updated patch, let me know if something need a
changing..
From 16df81551d1899e650ae28ea8d41931f7c391c7a Mon Sep 17 00:00:00 2001
From: Justin P. Mattock <justinmattock@gmail.com>
Date: Wed, 30 Jun 2010 12:34:32 -0700
Subject: [PATCH]acpi:glue.c Fix Fix warning: variable 'ret' set but not used
Fix a warning message generated by gcc:
Im getting a warning message when building with gcc 4.6.0
CC drivers/acpi/glue.o
drivers/acpi/glue.c: In function 'acpi_bind_one':
drivers/acpi/glue.c:163:7: warning: variable 'ret' set but not used
Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---
drivers/acpi/glue.c | 16 +++++++++++++---
1 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 4af6301..11ad510 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -145,6 +145,7 @@ static int acpi_bind_one(struct device *dev,
acpi_handle handle)
{
struct acpi_device *acpi_dev;
acpi_status status;
+ int fn, pn;
if (dev->archdata.acpi_handle) {
dev_warn(dev, "Drivers changed 'acpi_handle'\n");
@@ -160,12 +161,21 @@ static int acpi_bind_one(struct device *dev,
acpi_handle handle)
status = acpi_bus_get_device(handle, &acpi_dev);
if (!ACPI_FAILURE(status)) {
- int ret;
- ret = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
+ fn = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
"firmware_node");
- ret = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
+ pn = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
"physical_node");
+ if (fn) {
+ dev_warn(&acpi_dev->dev,
+ "Failed to create firmware_node link to %s %s: %d\n",
+ dev_driver_string(dev), dev_name(dev), fn);
+ } else if (pn) {
+ dev_warn(&acpi_dev->dev,
+ "Failed to create physical_node link to %s %s: %d\n",
+ dev_driver_string(dev), dev_name(dev), pn);
+ return AE_ERROR;
+ }
if (acpi_dev->wakeup.flags.valid) {
device_set_wakeup_capable(dev, true);
device_set_wakeup_enable(dev,
--
1.7.1.rc1.21.gf3bd6
cheers,
Justin P. Mattock
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-06-30 9:13 ` David Howells
2010-06-30 13:21 ` Justin P. Mattock
2010-06-30 19:47 ` Justin P. Mattock
@ 2010-07-01 9:31 ` David Howells
2010-07-01 13:41 ` Justin P. Mattock
` (2 more replies)
2 siblings, 3 replies; 38+ messages in thread
From: David Howells @ 2010-07-01 9:31 UTC (permalink / raw)
To: Justin P. Mattock; +Cc: dhowells, linux-kernel, linux-acpi, lenb
Justin P. Mattock <justinmattock@gmail.com> wrote:
> + if (fn) {
> + dev_warn(&acpi_dev->dev,
> + "Failed to create firmware_node link to %s %s: %d\n",
> + dev_driver_string(dev), dev_name(dev), fn);
> + } else if (pn) {
> + dev_warn(&acpi_dev->dev,
> + "Failed to create physical_node link to %s %s: %d\n",
> + dev_driver_string(dev), dev_name(dev), pn);
> + return AE_ERROR;
> + }
There's one more question to ask yourself: do you really need two dev_warn()
statements? You could have just one that prints both error values:
if (fn || pn)
dev_warn(&acpi_dev->dev,
"Failed to create link(s) to %s %s:"
" fn=%d pn=%d\n",
dev_driver_string(dev), dev_name(dev),
fn, pn);
Not sure it's worth going that far. You could reduce it still further:
if (fn || pn)
dev_warn(&acpi_dev->dev,
"Failed to create link(s) to %s %s:"
" %d\n",
dev_driver_string(dev), dev_name(dev),
fn ?: pn);
Is it that important to know which failed to be created, or that both failed
to be created?
David
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-07-01 9:31 ` David Howells
@ 2010-07-01 13:41 ` Justin P. Mattock
2010-07-01 20:01 ` Justin P. Mattock
2010-07-02 0:10 ` David Howells
2 siblings, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-07-01 13:41 UTC (permalink / raw)
To: David Howells; +Cc: linux-kernel, linux-acpi, lenb
On 07/01/2010 02:31 AM, David Howells wrote:
> Justin P. Mattock<justinmattock@gmail.com> wrote:
>
>> + if (fn) {
>> + dev_warn(&acpi_dev->dev,
>> + "Failed to create firmware_node link to %s %s: %d\n",
>> + dev_driver_string(dev), dev_name(dev), fn);
>> + } else if (pn) {
>> + dev_warn(&acpi_dev->dev,
>> + "Failed to create physical_node link to %s %s: %d\n",
>> + dev_driver_string(dev), dev_name(dev), pn);
>> + return AE_ERROR;
>> + }
>
> There's one more question to ask yourself: do you really need two dev_warn()
> statements? You could have just one that prints both error values:
>
> if (fn || pn)
> dev_warn(&acpi_dev->dev,
> "Failed to create link(s) to %s %s:"
> " fn=%d pn=%d\n",
> dev_driver_string(dev), dev_name(dev),
> fn, pn);
ah... I did think about that a few days ago, but had no idea how to
really follow through with this.. and from looking at what you did, it's
as simple as a || b
>
> Not sure it's worth going that far. You could reduce it still further:
>
> if (fn || pn)
> dev_warn(&acpi_dev->dev,
> "Failed to create link(s) to %s %s:"
> " %d\n",
> dev_driver_string(dev), dev_name(dev),
> fn ?: pn);
I don't mind resending with your change to this.
>
> Is it that important to know which failed to be created, or that both failed
> to be created?
>
> David
>
maybe a simple test(prog) case can be created to simulate what this is
doing, just to make sure.
Justin P. Mattock
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-07-01 9:31 ` David Howells
2010-07-01 13:41 ` Justin P. Mattock
@ 2010-07-01 20:01 ` Justin P. Mattock
2010-07-02 0:10 ` David Howells
2 siblings, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-07-01 20:01 UTC (permalink / raw)
To: David Howells; +Cc: linux-kernel, linux-acpi, lenb
> Not sure it's worth going that far. You could reduce it still further:
>
> if (fn || pn)
> dev_warn(&acpi_dev->dev,
> "Failed to create link(s) to %s %s:"
> " %d\n",
> dev_driver_string(dev), dev_name(dev),
> fn ?: pn);
>
> Is it that important to know which failed to be created, or that both failed
> to be created?
>
> David
>
I like this..
fn ?: pn (will this give us the results from the above question?
_both failed to be created_)
a bit confused with the whole: "?:" though
*condition ? value if true : value if false* (what if both are true
what if both are false or does it matter?)
here is the patch itself with the change:
Fix a warning message generated by gcc:
Im getting a warning message when building with gcc 4.6.0
CC drivers/acpi/glue.o
drivers/acpi/glue.c: In function 'acpi_bind_one':
drivers/acpi/glue.c:163:7: warning: variable 'ret' set but not used
Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---
drivers/acpi/glue.c | 14 +++++++++++---
1 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 4af6301..23b16e6 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -145,6 +145,7 @@ static int acpi_bind_one(struct device *dev,
acpi_handle handle)
{
struct acpi_device *acpi_dev;
acpi_status status;
+ int fn, pn;
if (dev->archdata.acpi_handle) {
dev_warn(dev, "Drivers changed 'acpi_handle'\n");
@@ -160,12 +161,19 @@ static int acpi_bind_one(struct device *dev,
acpi_handle handle)
status = acpi_bus_get_device(handle, &acpi_dev);
if (!ACPI_FAILURE(status)) {
- int ret;
- ret = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
+ fn = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj,
"firmware_node");
- ret = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
+ pn = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj,
"physical_node");
+ if (fn || pn) {
+ dev_warn(&acpi_dev->dev,
+ "Failed to create link(s) to %s %s:"
+ " %d\n",
+ dev_driver_string(dev), dev_name(dev),
+ fn ?: pn);
+ return AE_ERROR;
+ }
if (acpi_dev->wakeup.flags.valid) {
device_set_wakeup_capable(dev, true);
device_set_wakeup_enable(dev,
-- 1.7.1.rc1.21.gf3bd6
Justin P. Mattock
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-07-01 9:31 ` David Howells
2010-07-01 13:41 ` Justin P. Mattock
2010-07-01 20:01 ` Justin P. Mattock
@ 2010-07-02 0:10 ` David Howells
2010-07-02 0:59 ` Justin P. Mattock
2 siblings, 1 reply; 38+ messages in thread
From: David Howells @ 2010-07-02 0:10 UTC (permalink / raw)
To: Justin P. Mattock; +Cc: dhowells, linux-kernel, linux-acpi, lenb
Justin P. Mattock <justinmattock@gmail.com> wrote:
> a bit confused with the whole: "?:" though
> *condition ? value if true : value if false* (what if both are true
> what if both are false or does it matter?)
If you say:
cond ?: false_value
then you'll get cond if cond is non-zero and false_value if it isn't.
I suspect it's a gccism.
David
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-07-02 0:10 ` David Howells
@ 2010-07-02 0:59 ` Justin P. Mattock
0 siblings, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-07-02 0:59 UTC (permalink / raw)
To: David Howells; +Cc: linux-kernel, linux-acpi, lenb
On 07/01/2010 05:10 PM, David Howells wrote:
> Justin P. Mattock<justinmattock@gmail.com> wrote:
>
>> a bit confused with the whole: "?:" though
>> *condition ? value if true : value if false* (what if both are true
>> what if both are false or does it matter?)
>
> If you say:
>
> cond ?: false_value
>
> then you'll get cond if cond is non-zero and false_value if it isn't.
>
> I suspect it's a gccism.
>
> David
>
I'll have to experiment with this one..
?:
Justin P. Mattock
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' set but not used
2010-06-28 12:48 ` [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' " David Howells
2010-06-28 17:52 ` Justin P. Mattock
2010-06-28 18:47 ` David Howells
@ 2010-06-28 19:08 ` Justin P. Mattock
2 siblings, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-28 19:08 UTC (permalink / raw)
To: David Howells; +Cc: linux-kernel, lenb, linux-bluetooth
no need to have extra cc's on this, so I took some people
off since they dont pertain to this area.
Justin P. Mattock
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/5]block:cryptoloop Fix warning: variable 'cipher' set but not used
2010-06-27 6:47 [PATCH 0/5] Fix gcc 4.6.0 set but unused variable warnings Justin P. Mattock
` (7 preceding siblings ...)
2010-06-28 12:48 ` [PATCH 3/5]acpi:glue.c Fix warning: variable 'ret' " David Howells
@ 2010-06-28 12:49 ` David Howells
2010-06-28 12:52 ` [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined David Howells
9 siblings, 0 replies; 38+ messages in thread
From: David Howells @ 2010-06-28 12:49 UTC (permalink / raw)
To: Justin P. Mattock; +Cc: dhowells, linux-kernel, sds, lenb, linux-bluetooth
Justin P. Mattock <justinmattock@gmail.com> wrote:
> Im getting this when compiling on gcc 4.6.0
> CC [M] drivers/block/cryptoloop.o
> drivers/block/cryptoloop.c: In function 'cryptoloop_init':
> drivers/block/cryptoloop.c:46:8: warning: variable 'cipher' set but not used
> The below fixes it for me. Please have a look and let me know.
>
> Signed-off-by: Justin P. Mattock <justinmattock@gmail.com>
Acked-by: David Howells <dhowells@redhat.com>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined
2010-06-27 6:47 [PATCH 0/5] Fix gcc 4.6.0 set but unused variable warnings Justin P. Mattock
` (8 preceding siblings ...)
2010-06-28 12:49 ` [PATCH 4/5]block:cryptoloop Fix warning: variable 'cipher' " David Howells
@ 2010-06-28 12:52 ` David Howells
2010-06-28 13:02 ` Bernd Petrovitsch
2010-06-28 17:56 ` Justin P. Mattock
9 siblings, 2 replies; 38+ messages in thread
From: David Howells @ 2010-06-28 12:52 UTC (permalink / raw)
To: Justin P. Mattock, Gustavo F. Padovan
Cc: dhowells, linux-kernel, sds, lenb, linux-bluetooth
Justin P. Mattock <justinmattock@gmail.com> wrote:
> - BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
> - bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
> + BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq | ret);
> + ret = ++(bcsp->msgq_txseq) & 0x07;
I don't know what you're trying to do here, but you seem to be trying to send
the computed value back in time.
The problem is that the compiler is confused about why a '++' operator makes
any sense here. It doesn't. It should be a '+ 1' instead. I think what you
want is:
- bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
+ bcsp->msgq_txseq = (bcsp->msgq_txseq + 1) & 0x07;
David
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined
2010-06-28 12:52 ` [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined David Howells
@ 2010-06-28 13:02 ` Bernd Petrovitsch
2010-06-28 17:56 ` Justin P. Mattock
1 sibling, 0 replies; 38+ messages in thread
From: Bernd Petrovitsch @ 2010-06-28 13:02 UTC (permalink / raw)
To: David Howells
Cc: Justin P. Mattock, Gustavo F. Padovan, linux-kernel, sds, lenb,
linux-bluetooth
On Mon, 2010-06-28 at 13:52 +0100, David Howells wrote:
> Justin P. Mattock <justinmattock@gmail.com> wrote:
>
> > - BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
> > - bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
> > + BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq | ret);
> > + ret = ++(bcsp->msgq_txseq) & 0x07;
>
> I don't know what you're trying to do here, but you seem to be trying to send
> the computed value back in time.
>
> The problem is that the compiler is confused about why a '++' operator makes
It's even worse as that expression is explicitly undefined (and should
be fixed anyways and unconditionally).
> any sense here. It doesn't. It should be a '+ 1' instead. I think what you
> want is:
>
> - bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
> + bcsp->msgq_txseq = (bcsp->msgq_txseq + 1) & 0x07;
Yes, that's looks like the most probable intention of it - at least for
one who doesn't know the bluetooth code.
Bernd
--
Bernd Petrovitsch Email : bernd@petrovitsch.priv.at
LUGA : http://www.luga.at
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined
2010-06-28 12:52 ` [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined David Howells
2010-06-28 13:02 ` Bernd Petrovitsch
@ 2010-06-28 17:56 ` Justin P. Mattock
1 sibling, 0 replies; 38+ messages in thread
From: Justin P. Mattock @ 2010-06-28 17:56 UTC (permalink / raw)
To: David Howells
Cc: Gustavo F. Padovan, linux-kernel, sds, lenb, linux-bluetooth
On 06/28/2010 05:52 AM, David Howells wrote:
> Justin P. Mattock<justinmattock@gmail.com> wrote:
>
>> - BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
>> - bcsp->msgq_txseq = ++(bcsp->msgq_txseq)& 0x07;
>> + BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq | ret);
>> + ret = ++(bcsp->msgq_txseq)& 0x07;
>
> I don't know what you're trying to do here, but you seem to be trying to send
> the computed value back in time.
>
I was under the impression that hdr[0] |= 0x80 + bcsp->msgq_txseq;
is computing a value for BT_DBG then ret = ++(bcsp->msgq_txseq)& 0x07
computes a value as well i.e.
BT_DBG("Sending packet with seqno %u", hdr[0] | ret);
> The problem is that the compiler is confused about why a '++' operator makes
> any sense here. It doesn't. It should be a '+ 1' instead. I think what you
> want is:
>
> - bcsp->msgq_txseq = ++(bcsp->msgq_txseq)& 0x07;
> + bcsp->msgq_txseq = (bcsp->msgq_txseq + 1)& 0x07;
>
> David
>
yeah I did play around with the ++ and noticed the compiler would be
satisfied if the ++ was not there, but didn't think to just add a + 1
Justin P. Mattock
^ permalink raw reply [flat|nested] 38+ messages in thread