* [PATCH] v2: rtl8187: micro cleanup @ 2010-02-17 18:15 okias 2010-02-17 18:30 ` Larry Finger 0 siblings, 1 reply; 11+ messages in thread From: okias @ 2010-02-17 18:15 UTC (permalink / raw) To: linville; +Cc: linux-wireless, herton, htl10, Larry.Finger >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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] v2: rtl8187: micro cleanup 2010-02-17 18:15 [PATCH] v2: rtl8187: micro cleanup okias @ 2010-02-17 18:30 ` Larry Finger 2010-02-17 18:34 ` okias 0 siblings, 1 reply; 11+ messages in thread From: Larry Finger @ 2010-02-17 18:30 UTC (permalink / raw) To: okias; +Cc: linville, linux-wireless, herton, htl10 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] v2: rtl8187: micro cleanup 2010-02-17 18:30 ` Larry Finger @ 2010-02-17 18:34 ` okias 2010-02-17 18:36 ` Johannes Berg 0 siblings, 1 reply; 11+ messages in thread From: okias @ 2010-02-17 18:34 UTC (permalink / raw) To: Larry Finger; +Cc: linville, linux-wireless, herton, htl10 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] v2: rtl8187: micro cleanup 2010-02-17 18:34 ` okias @ 2010-02-17 18:36 ` Johannes Berg 2010-02-17 18:38 ` okias 0 siblings, 1 reply; 11+ messages in thread From: Johannes Berg @ 2010-02-17 18:36 UTC (permalink / raw) To: okias; +Cc: Larry Finger, linville, linux-wireless, herton, htl10 [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] v2: rtl8187: micro cleanup 2010-02-17 18:36 ` Johannes Berg @ 2010-02-17 18:38 ` okias 2010-02-17 18:58 ` Pavel Roskin 2010-02-17 20:47 ` Hin-Tak Leung 0 siblings, 2 replies; 11+ messages in thread From: okias @ 2010-02-17 18:38 UTC (permalink / raw) To: Johannes Berg; +Cc: Larry Finger, linville, linux-wireless, herton, htl10 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] v2: rtl8187: micro cleanup 2010-02-17 18:38 ` okias @ 2010-02-17 18:58 ` Pavel Roskin 2010-02-17 19:04 ` okias 2010-02-17 20:47 ` Hin-Tak Leung 1 sibling, 1 reply; 11+ messages in thread From: Pavel Roskin @ 2010-02-17 18:58 UTC (permalink / raw) To: okias; +Cc: linux-wireless 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] v2: rtl8187: micro cleanup 2010-02-17 18:58 ` Pavel Roskin @ 2010-02-17 19:04 ` okias 2010-02-17 19:13 ` Sedat Dilek 2010-02-17 22:44 ` Pavel Roskin 0 siblings, 2 replies; 11+ messages in thread From: okias @ 2010-02-17 19:04 UTC (permalink / raw) To: Pavel Roskin; +Cc: Larry Finger, linville, linux-wireless, herton, htl10 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] v2: rtl8187: micro cleanup 2010-02-17 19:04 ` okias @ 2010-02-17 19:13 ` Sedat Dilek 2010-02-17 21:14 ` Hin-Tak Leung 2010-02-17 22:44 ` Pavel Roskin 1 sibling, 1 reply; 11+ messages in thread From: Sedat Dilek @ 2010-02-17 19:13 UTC (permalink / raw) To: okias; +Cc: Pavel Roskin, Larry Finger, linville, linux-wireless, herton, htl10 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] v2: rtl8187: micro cleanup 2010-02-17 19:13 ` Sedat Dilek @ 2010-02-17 21:14 ` Hin-Tak Leung 0 siblings, 0 replies; 11+ messages in thread From: Hin-Tak Leung @ 2010-02-17 21:14 UTC (permalink / raw) To: okias, sedat.dilek Cc: Pavel Roskin, Larry Finger, linville, linux-wireless, herton --- 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. :-). ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] v2: rtl8187: micro cleanup 2010-02-17 19:04 ` okias 2010-02-17 19:13 ` Sedat Dilek @ 2010-02-17 22:44 ` Pavel Roskin 1 sibling, 0 replies; 11+ messages in thread From: Pavel Roskin @ 2010-02-17 22:44 UTC (permalink / raw) To: okias; +Cc: linux-wireless 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] v2: rtl8187: micro cleanup 2010-02-17 18:38 ` okias 2010-02-17 18:58 ` Pavel Roskin @ 2010-02-17 20:47 ` Hin-Tak Leung 1 sibling, 0 replies; 11+ messages in thread From: Hin-Tak Leung @ 2010-02-17 20:47 UTC (permalink / raw) To: Johannes Berg, okias; +Cc: Larry Finger, linville, linux-wireless, herton --- 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-02-17 22:44 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-02-17 18:15 [PATCH] v2: rtl8187: micro cleanup okias 2010-02-17 18:30 ` Larry Finger 2010-02-17 18:34 ` okias 2010-02-17 18:36 ` Johannes Berg 2010-02-17 18:38 ` okias 2010-02-17 18:58 ` Pavel Roskin 2010-02-17 19:04 ` okias 2010-02-17 19:13 ` Sedat Dilek 2010-02-17 21:14 ` Hin-Tak Leung 2010-02-17 22:44 ` Pavel Roskin 2010-02-17 20:47 ` Hin-Tak Leung
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.