[PATCH] v2: rtl8187: micro cleanup

[PATCH] v2: rtl8187: micro cleanup

[okias]

>From 3f02b3ec0c3e6d7fa0599de12af0b76150854b94 Mon Sep 17 00:00:00 2001
From: David Heidelberger <d.okias@gmail.com>
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: okias@isgeek.info
SIP VoIP: sip:17474537254@proxy01.sipphone.com

Re: [PATCH] v2: rtl8187: micro cleanup

[Larry Finger]

On 02/17/2010 12:15 PM, okias wrote:
>>From 3f02b3ec0c3e6d7fa0599de12af0b76150854b94 Mon Sep 17 00:00:00 2001
> From: David Heidelberger <d.okias@gmail.com>
> 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

Re: [PATCH] v2: rtl8187: micro cleanup

[okias]

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 <Larry.Finger@lwfinger.net>:
> On 02/17/2010 12:15 PM, okias wrote:
>>>From 3f02b3ec0c3e6d7fa0599de12af0b76150854b94 Mon Sep 17 00:00:00 2001
>> From: David Heidelberger <d.okias@gmail.com>
>> 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: okias@isgeek.info
SIP VoIP: sip:17474537254@proxy01.sipphone.com

Re: [PATCH] v2: rtl8187: micro cleanup

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 263 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] v2: rtl8187: micro cleanup

[okias]

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 <johannes@sipsolutions.net>:
> 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: okias@isgeek.info
SIP VoIP: sip:17474537254@proxy01.sipphone.com

Re: [PATCH] v2: rtl8187: micro cleanup

[Pavel Roskin]

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

Re: [PATCH] v2: rtl8187: micro cleanup

[okias]

ok, sorry I tested it on x86_64. Here is correct patch:

>From c4dd3d87e090dd0b44dc030840f2ae1fd0bff729 Mon Sep 17 00:00:00 2001
From: David Heidelberger <d.okias@gmail.com>
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

Re: [PATCH] v2: rtl8187: micro cleanup

[Sedat Dilek]

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 <d.okias@gmail.com> 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 <d.okias@gmail.com>
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] v2: rtl8187: micro cleanup

[Hin-Tak Leung]

--- On Wed, 17/2/10, okias <d.okias@gmail.com> 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 <johannes@sipsolutions.net>:
> > 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: okias@isgeek.info
> SIP VoIP: sip:17474537254@proxy01.sipphone.com
> 


      

Re: [PATCH] v2: rtl8187: micro cleanup

[Hin-Tak Leung]

--- On Wed, 17/2/10, Sedat Dilek <sedat.dilek@googlemail.com> 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. :-). 



      

Re: [PATCH] v2: rtl8187: micro cleanup

[Pavel Roskin]

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