>From 3f02b3ec0c3e6d7fa0599de12af0b76150854b94 Mon Sep 17 00:00:00 2001
From: David Heidelberger <[email protected]>
Date: Wed, 17 Feb 2010 19:13:46 +0100
Subject: [PATCH] rtl8187: micro cleanup
---
drivers/net/wireless/rtl818x/rtl8187_dev.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c
b/drivers/net/wireless/rtl818x/rtl8187_dev.c
index 7ba3052..2fe0c84 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -1175,8 +1175,6 @@ static void rtl8187_bss_info_changed(struct
ieee80211_hw *dev,
if (priv->is_rtl8187b)
reg = RTL818X_MSR_ENEDCA;
- else
- reg = 0;
if (is_valid_ether_addr(info->bssid)) {
reg |= RTL818X_MSR_INFRA;
--
1.7.0
--
Jabber/XMPP: [email protected]
SIP VoIP: sip:[email protected]
I did test on small C program, and "int a" is equal to int a = 0; so
it should be fine, but maybe I'm wrong.
David
2010/2/17 Larry Finger <[email protected]>:
> On 02/17/2010 12:15 PM, okias wrote:
>>>From 3f02b3ec0c3e6d7fa0599de12af0b76150854b94 Mon Sep 17 00:00:00 2001
>> From: David Heidelberger <[email protected]>
>> Date: Wed, 17 Feb 2010 19:13:46 +0100
>> Subject: [PATCH] rtl8187: micro cleanup
>>
>> ---
>> drivers/net/wireless/rtl818x/rtl8187_dev.c | 2 --
>> 1 files changed, 0 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c
>> b/drivers/net/wireless/rtl818x/rtl8187_dev.c
>> index 7ba3052..2fe0c84 100644
>> --- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
>> +++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
>> @@ -1175,8 +1175,6 @@ static void rtl8187_bss_info_changed(struct
>> ieee80211_hw *dev,
>>
>> if (priv->is_rtl8187b)
>> reg = RTL818X_MSR_ENEDCA;
>> - else
>> - reg = 0;
>>
>> if (is_valid_ether_addr(info->bssid)) {
>> reg |= RTL818X_MSR_INFRA;
>
> Did you forget the initialization to 0 when you declare the variable? It was in
> the first version. This change will cause the RTL8187L to fail.
>
> Larry
>
--
Jabber/XMPP: [email protected]
SIP VoIP: sip:[email protected]
You could be more precise on the commit-subject (not blaming you) next time.
Worth reading on this topic: "On Commit Messages" [1].
- Sedat -
[1] http://who-t.blogspot.com/2009/12/on-commit-messages.html
On Wed, Feb 17, 2010 at 8:04 PM, okias <[email protected]> wrote:
> ok, sorry I tested it on x86_64. Here is correct patch:
>
> From c4dd3d87e090dd0b44dc030840f2ae1fd0bff729 Mon Sep 17 00:00:00 2001
> From: David Heidelberger <[email protected]>
> Date: Wed, 17 Feb 2010 19:41:15 +0100
> Subject: [PATCH] rtl8187: micro cleanup v3
>
> ---
> drivers/net/wireless/rtl818x/rtl8187_dev.c | 4 +---
> 1 files changed, 1 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c
> b/drivers/net/wireless/rtl818x/rtl8187_dev.c
> index 7ba3052..2cc6881 100644
> --- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
> +++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
> @@ -1165,7 +1165,7 @@ static void rtl8187_bss_info_changed(struct
> ieee80211_hw *dev,
> {
> struct rtl8187_priv *priv = dev->priv;
> int i;
> - u8 reg;
> + u8 reg = 0;
>
> if (changed & BSS_CHANGED_BSSID) {
> mutex_lock(&priv->conf_mutex);
> @@ -1175,8 +1175,6 @@ static void rtl8187_bss_info_changed(struct
> ieee80211_hw *dev,
>
> if (priv->is_rtl8187b)
> reg = RTL818X_MSR_ENEDCA;
> - else
> - reg = 0;
>
> if (is_valid_ether_addr(info->bssid)) {
> reg |= RTL818X_MSR_INFRA;
> --
> 1.7.0
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
You have probably right, but when I use my testing program:
#include <stdio.h>
main() {
int a;
printf("%i\n", a);
a |= 22;
printf("%i\n", a);
}
Output is:
0
22
it look correct to me
2010/2/17 Johannes Berg <[email protected]>:
> On Wed, 2010-02-17 at 19:34 +0100, okias wrote:
>> I did test on small C program, and "int a" is equal to int a = 0; so
>> it should be fine, but maybe I'm wrong.
>
> It's not equal. It will be uninitialised stack garbage if you don't
> initialise it.
>
> johannes
>
--
Jabber/XMPP: [email protected]
SIP VoIP: sip:[email protected]
--- On Wed, 17/2/10, okias <[email protected]> wrote:
> You have probably right, but when I
> use my testing program:
>
> #include <stdio.h>
> main() {
> int a;
> printf("%i\n", a);
> a |= 22;
> printf("%i\n", a);
> }
>
> Output is:
> 0
> 22
>
> it look correct to me
It is wrong - you are relying on and depending on the compiler zero'ing uninitialized variables for you - which, depends on which compiler you are using, optimization level, etc, may or may not happen. (some compiler does that, under some conditions/optimizations, but you cannot depend on that).
>
>
> 2010/2/17 Johannes Berg <[email protected]>:
> > On Wed, 2010-02-17 at 19:34 +0100, okias wrote:
> >> I did test on small C program, and "int a" is
> equal to int a = 0; so
> >> it should be fine, but maybe I'm wrong.
> >
> > It's not equal. It will be uninitialised stack garbage
> if you don't
> > initialise it.
> >
> > johannes
> >
>
>
>
> --
> Jabber/XMPP: [email protected]
> SIP VoIP: sip:[email protected]
>
ok, sorry I tested it on x86_64. Here is correct patch:
>From c4dd3d87e090dd0b44dc030840f2ae1fd0bff729 Mon Sep 17 00:00:00 2001
From: David Heidelberger <[email protected]>
Date: Wed, 17 Feb 2010 19:41:15 +0100
Subject: [PATCH] rtl8187: micro cleanup v3
---
drivers/net/wireless/rtl818x/rtl8187_dev.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c
b/drivers/net/wireless/rtl818x/rtl8187_dev.c
index 7ba3052..2cc6881 100644
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -1165,7 +1165,7 @@ static void rtl8187_bss_info_changed(struct
ieee80211_hw *dev,
{
struct rtl8187_priv *priv = dev->priv;
int i;
- u8 reg;
+ u8 reg = 0;
if (changed & BSS_CHANGED_BSSID) {
mutex_lock(&priv->conf_mutex);
@@ -1175,8 +1175,6 @@ static void rtl8187_bss_info_changed(struct
ieee80211_hw *dev,
if (priv->is_rtl8187b)
reg = RTL818X_MSR_ENEDCA;
- else
- reg = 0;
if (is_valid_ether_addr(info->bssid)) {
reg |= RTL818X_MSR_INFRA;
--
1.7.0
On Wed, 2010-02-17 at 20:04 +0100, okias wrote:
> ok, sorry I tested it on x86_64. Here is correct patch:
I'm sorry, but it's not a cleanup. Initializing reg outside the block
gives a wrong impression that reg may be used outside the block. Code
is safer is it's more readable, and it's more readable if the logic is
not spread around, but kept in one place.
Unnecessary initialization can prevent gcc from issuing warnings about
using uninitialized variables when the code changes. Such warnings may
be useful to find real bugs. I can give you a realistic example, but it
would be off-topic here.
A real cleanup would be to give reg the block scope and to refactor the
code where reg is written to the hardware:
--- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
@@ -1163,9 +1163,10 @@ static void rtl8187_bss_info_changed(struct ieee80211_hw *dev,
{
struct rtl8187_priv *priv = dev->priv;
int i;
- u8 reg;
if (changed & BSS_CHANGED_BSSID) {
+ u8 reg;
+
mutex_lock(&priv->conf_mutex);
for (i = 0; i < ETH_ALEN; i++)
rtl818x_iowrite8(priv, &priv->map->BSSID[i],
@@ -1176,13 +1177,12 @@ static void rtl8187_bss_info_changed(struct ieee80211_hw *dev,
else
reg = 0;
- if (is_valid_ether_addr(info->bssid)) {
+ if (is_valid_ether_addr(info->bssid))
reg |= RTL818X_MSR_INFRA;
- rtl818x_iowrite8(priv, &priv->map->MSR, reg);
- } else {
+ else
reg |= RTL818X_MSR_NO_LINK;
- rtl818x_iowrite8(priv, &priv->map->MSR, reg);
- }
+
+ rtl818x_iowrite8(priv, &priv->map->MSR, reg);
mutex_unlock(&priv->conf_mutex);
}
But I would never bother with that simply because wireless developers
have more important things to do.
I respectfully suggest that you do your exercises in C programming
elsewhere.
--
Regards,
Pavel Roskin
On Wed, 2010-02-17 at 19:34 +0100, okias wrote:
> I did test on small C program, and "int a" is equal to int a = 0; so
> it should be fine, but maybe I'm wrong.
It's not equal. It will be uninitialised stack garbage if you don't
initialise it.
johannes
On 02/17/2010 12:15 PM, okias wrote:
>>From 3f02b3ec0c3e6d7fa0599de12af0b76150854b94 Mon Sep 17 00:00:00 2001
> From: David Heidelberger <[email protected]>
> Date: Wed, 17 Feb 2010 19:13:46 +0100
> Subject: [PATCH] rtl8187: micro cleanup
>
> ---
> drivers/net/wireless/rtl818x/rtl8187_dev.c | 2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/rtl818x/rtl8187_dev.c
> b/drivers/net/wireless/rtl818x/rtl8187_dev.c
> index 7ba3052..2fe0c84 100644
> --- a/drivers/net/wireless/rtl818x/rtl8187_dev.c
> +++ b/drivers/net/wireless/rtl818x/rtl8187_dev.c
> @@ -1175,8 +1175,6 @@ static void rtl8187_bss_info_changed(struct
> ieee80211_hw *dev,
>
> if (priv->is_rtl8187b)
> reg = RTL818X_MSR_ENEDCA;
> - else
> - reg = 0;
>
> if (is_valid_ether_addr(info->bssid)) {
> reg |= RTL818X_MSR_INFRA;
Did you forget the initialization to 0 when you declare the variable? It was in
the first version. This change will cause the RTL8187L to fail.
Larry
On Wed, 2010-02-17 at 19:38 +0100, okias wrote:
> You have probably right, but when I use my testing program:
>
> #include <stdio.h>
> main() {
> int a;
> printf("%i\n", a);
> a |= 22;
> printf("%i\n", a);
> }
>
> Output is:
> 0
> 22
>
> it look correct to me
That's what I get if compiling for x86_64. If compiling for i386, I get
1258024948
1258024950
It just happens that 0 is on the stack where the variable is allocated,
perhaps as a leftover from another call that used that area on the
stack.
No C standard says that automatic variables are initialized. And
indeed, they are not!
gcc will warn about it with -Wall
Please let's stop this discussion now, as it doesn't belong to the
linux-wireless mailing list.
--
Regards,
Pavel Roskin
--- On Wed, 17/2/10, Sedat Dilek <[email protected]> wrote:
> You could be more precise on the
> commit-subject (not blaming you) next time.
> Worth reading on this topic: "On Commit Messages" [1].
Ditto. Here is the definitive recommended reading for submitting patches.
BTW, you also missed a Signed-off line.
http://linuxwireless.org/en/developers/Documentation/SubmittingPatches/sourcedoc
I think normally the v1/v2/v3 part goes into the front, e.g.
[PATCH v1] ...
Things like 'ok, sorry I tested...' should probably be in a reply to your own post, or preferably, not - you either put it in the commit message itself, or leave it out.
That's because John accepts about 40 patches per day, and git provides the facility to automatically extract patches per e-mail (for all e-mails filed into a folder, for example) - anything that requires manual editing to remove is considered undesirable. Same with the ambiguous subject line, and v3, etc. About 600(?) patches goes into each kernel releases - if ever patch comes in as 'minor clean up', 'fix one bug', 'fix intermittent crash', 'tried n times now' for the patch summary, nobody would know what goes into the kernel. :-).