In following patch, replace b43 specific helper function with kernel
api to reduce code duplication.
Signed-off-by: ZHAO Gang <[email protected]>
---
drivers/net/wireless/b43/xmit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
index 4ae63f4..50e5ddb 100644
--- a/drivers/net/wireless/b43/xmit.c
+++ b/drivers/net/wireless/b43/xmit.c
@@ -821,10 +821,10 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
* channel number in b43. */
if (chanstat & B43_RX_CHAN_5GHZ) {
status.band = IEEE80211_BAND_5GHZ;
- status.freq = b43_freq_to_channel_5ghz(chanid);
+ status.freq = b43_channel_to_freq_5ghz(chanid);
} else {
status.band = IEEE80211_BAND_2GHZ;
- status.freq = b43_freq_to_channel_2ghz(chanid);
+ status.freq = b43_channel_to_freq_2ghz(chanid);
}
break;
default:
--
1.8.4.2
On Fri, Jan 17, 2014 at 10:01 AM, Luca Coelho <[email protected]> wrote:
> On Fri, 2014-01-17 at 09:56 +0100, Jonas Gorski wrote:
>> On Fri, Jan 17, 2014 at 8:11 AM, Rafał Miłecki <[email protected]> wrote:
>> > 2014/1/17 Luca Coelho <[email protected]>:
>> >> On Fri, 2014-01-17 at 13:27 +0800, ZHAO Gang wrote:
>> >>> In following patch, replace b43 specific helper function with kernel
>> >>> api to reduce code duplication.
>> >>>
>> >>> Signed-off-by: ZHAO Gang <[email protected]>
>> >>> ---
>> >>> drivers/net/wireless/b43/xmit.c | 4 ++--
>> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> >>>
>> >>> diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
>> >>> index 4ae63f4..50e5ddb 100644
>> >>> --- a/drivers/net/wireless/b43/xmit.c
>> >>> +++ b/drivers/net/wireless/b43/xmit.c
>> >>> @@ -821,10 +821,10 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
>> >>> * channel number in b43. */
>> >>> if (chanstat & B43_RX_CHAN_5GHZ) {
>> >>> status.band = IEEE80211_BAND_5GHZ;
>> >>> - status.freq = b43_freq_to_channel_5ghz(chanid);
>> >>> + status.freq = b43_channel_to_freq_5ghz(chanid);
>> >>> } else {
>> >>> status.band = IEEE80211_BAND_2GHZ;
>> >>> - status.freq = b43_freq_to_channel_2ghz(chanid);
>> >>> + status.freq = b43_channel_to_freq_2ghz(chanid);
>> >>> }
>> >>> break;
>> >>> default:
>> >>
>> >> Why do you need this patch if you're going to remove these calls in the
>> >> next patch anyway?
>> >
>> > I was thinking about this for a moment too. You could just make a one
>> > patch and note in commit message that "translation" was reversed.
>>
>> That would mean mixing fixes and improvements, which is something you
>> are not supposed to do, so IMHO having these split into two is
>> correct. Think about stable maintainers wanting the fix but not the
>> other change because it might introduce unknown side effects.
>
> Makes sense. In such case, the first patch should be clearly marked as
> a bug fix, so at least the commit message should be changed (ie.
> mentioning the next patch in the series is useless).
Well, it uses "fix" in the subject ;-). But I agree about the commit
message; it should describe the changes of this patch and the impact
of the fixed defect, so it's easier to decide whether to backport the
fix or not.
Jonas
On Fri, 2014-01-17 at 10:37 +0100, Rafał Miłecki wrote:
> 2014/1/17 ZHAO Gang <[email protected]>:
> > On Fri, Jan 17, 2014 at 5:01 PM, Luca Coelho <[email protected]> wrote:
> >> On Fri, 2014-01-17 at 09:56 +0100, Jonas Gorski wrote:
> >>> On Fri, Jan 17, 2014 at 8:11 AM, Rafał Miłecki <[email protected]> wrote:
> >>> > 2014/1/17 Luca Coelho <[email protected]>:
> >>> >> On Fri, 2014-01-17 at 13:27 +0800, ZHAO Gang wrote:
> >>> >>> In following patch, replace b43 specific helper function with kernel
> >>> >>> api to reduce code duplication.
> >>> >>>
> >>> >>> Signed-off-by: ZHAO Gang <[email protected]>
> >>> >>> ---
> >>> >>> drivers/net/wireless/b43/xmit.c | 4 ++--
> >>> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>> >>>
> >>> >>> diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
> >>> >>> index 4ae63f4..50e5ddb 100644
> >>> >>> --- a/drivers/net/wireless/b43/xmit.c
> >>> >>> +++ b/drivers/net/wireless/b43/xmit.c
> >>> >>> @@ -821,10 +821,10 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
> >>> >>> * channel number in b43. */
> >>> >>> if (chanstat & B43_RX_CHAN_5GHZ) {
> >>> >>> status.band = IEEE80211_BAND_5GHZ;
> >>> >>> - status.freq = b43_freq_to_channel_5ghz(chanid);
> >>> >>> + status.freq = b43_channel_to_freq_5ghz(chanid);
> >>> >>> } else {
> >>> >>> status.band = IEEE80211_BAND_2GHZ;
> >>> >>> - status.freq = b43_freq_to_channel_2ghz(chanid);
> >>> >>> + status.freq = b43_channel_to_freq_2ghz(chanid);
> >>> >>> }
> >>> >>> break;
> >>> >>> default:
> >>> >>
> >>> >> Why do you need this patch if you're going to remove these calls in the
> >>> >> next patch anyway?
> >>> >
> >>> > I was thinking about this for a moment too. You could just make a one
> >>> > patch and note in commit message that "translation" was reversed.
> >>>
> >>> That would mean mixing fixes and improvements, which is something you
> >>> are not supposed to do, so IMHO having these split into two is
> >>> correct. Think about stable maintainers wanting the fix but not the
> >>> other change because it might introduce unknown side effects.
> >>
> >> Makes sense. In such case, the first patch should be clearly marked as
> >> a bug fix, so at least the commit message should be changed (ie.
> >> mentioning the next patch in the series is useless).
> >>
> >
> > I am OK to send this fix either in one patch or two, actually I have
> > sent a version 2 which is a one patch version :-)
> >
> > I'm not sure if this patch is needed for stable, yes, as you said, if
> > it's for stable, the commit message should be changed.
>
> Thanks for your help guys.
>
> I think it may be the best idea to send
> 1/2 as fix (probably 3.14) + stable CC
> 2/2 as improvement (for next)
> Does it make sense?
Sounds good to me. The actual fix is so simple and obvious that I don't
see any reason for not sending it as a fix + stable.
--
Luca.
2014/1/17 Luca Coelho <[email protected]>:
> On Fri, 2014-01-17 at 13:27 +0800, ZHAO Gang wrote:
>> In following patch, replace b43 specific helper function with kernel
>> api to reduce code duplication.
>>
>> Signed-off-by: ZHAO Gang <[email protected]>
>> ---
>> drivers/net/wireless/b43/xmit.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
>> index 4ae63f4..50e5ddb 100644
>> --- a/drivers/net/wireless/b43/xmit.c
>> +++ b/drivers/net/wireless/b43/xmit.c
>> @@ -821,10 +821,10 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
>> * channel number in b43. */
>> if (chanstat & B43_RX_CHAN_5GHZ) {
>> status.band = IEEE80211_BAND_5GHZ;
>> - status.freq = b43_freq_to_channel_5ghz(chanid);
>> + status.freq = b43_channel_to_freq_5ghz(chanid);
>> } else {
>> status.band = IEEE80211_BAND_2GHZ;
>> - status.freq = b43_freq_to_channel_2ghz(chanid);
>> + status.freq = b43_channel_to_freq_2ghz(chanid);
>> }
>> break;
>> default:
>
> Why do you need this patch if you're going to remove these calls in the
> next patch anyway?
I was thinking about this for a moment too. You could just make a one
patch and note in commit message that "translation" was reversed.
On Fri, 2014-01-17 at 13:27 +0800, ZHAO Gang wrote:
> In following patch, replace b43 specific helper function with kernel
> api to reduce code duplication.
>
> Signed-off-by: ZHAO Gang <[email protected]>
> ---
> drivers/net/wireless/b43/xmit.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
> index 4ae63f4..50e5ddb 100644
> --- a/drivers/net/wireless/b43/xmit.c
> +++ b/drivers/net/wireless/b43/xmit.c
> @@ -821,10 +821,10 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
> * channel number in b43. */
> if (chanstat & B43_RX_CHAN_5GHZ) {
> status.band = IEEE80211_BAND_5GHZ;
> - status.freq = b43_freq_to_channel_5ghz(chanid);
> + status.freq = b43_channel_to_freq_5ghz(chanid);
> } else {
> status.band = IEEE80211_BAND_2GHZ;
> - status.freq = b43_freq_to_channel_2ghz(chanid);
> + status.freq = b43_channel_to_freq_2ghz(chanid);
> }
> break;
> default:
Why do you need this patch if you're going to remove these calls in the
next patch anyway?
--
Luca.
On Fri, Jan 17, 2014 at 5:11 PM, ZHAO Gang <[email protected]> wrote:
> The b43_freq_to_channel_{2,5}ghz() should be changed to
> b43_channel_to_freq{2,5}ghz() if we want to use b43 helper function,
> but a better way is to use kernel api ieee80211_channel_to_frequency()
> and remove these unnecessary helper function.
>
> Signed-off-by: ZHAO Gang <[email protected]>
> ---
> v2: combine two patches to one.
> suggested by Luca Coelho and Rafał Miłecki
>
The version 2 patch should be ignored. After more discussion, it seems it's
good to let the fix be two patches.
> drivers/net/wireless/b43/main.h | 35 -----------------------------------
> drivers/net/wireless/b43/xmit.c | 12 ++++++------
> 2 files changed, 6 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/net/wireless/b43/main.h b/drivers/net/wireless/b43/main.h
> index abac25e..f476fc3 100644
> --- a/drivers/net/wireless/b43/main.h
> +++ b/drivers/net/wireless/b43/main.h
> @@ -58,41 +58,6 @@ enum b43_verbosity {
> #endif
> };
>
> -
> -/* Lightweight function to convert a frequency (in Mhz) to a channel number. */
> -static inline u8 b43_freq_to_channel_5ghz(int freq)
> -{
> - return ((freq - 5000) / 5);
> -}
> -static inline u8 b43_freq_to_channel_2ghz(int freq)
> -{
> - u8 channel;
> -
> - if (freq == 2484)
> - channel = 14;
> - else
> - channel = (freq - 2407) / 5;
> -
> - return channel;
> -}
> -
> -/* Lightweight function to convert a channel number to a frequency (in Mhz). */
> -static inline int b43_channel_to_freq_5ghz(u8 channel)
> -{
> - return (5000 + (5 * channel));
> -}
> -static inline int b43_channel_to_freq_2ghz(u8 channel)
> -{
> - int freq;
> -
> - if (channel == 14)
> - freq = 2484;
> - else
> - freq = 2407 + (5 * channel);
> -
> - return freq;
> -}
> -
> static inline int b43_is_cck_rate(int rate)
> {
> return (rate == B43_CCK_RATE_1MB ||
> diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
> index 4ae63f4..218a0f3 100644
> --- a/drivers/net/wireless/b43/xmit.c
> +++ b/drivers/net/wireless/b43/xmit.c
> @@ -806,7 +806,8 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
> B43_WARN_ON(1);
> /* FIXME: We don't really know which value the "chanid" contains.
> * So the following assignment might be wrong. */
> - status.freq = b43_channel_to_freq_5ghz(chanid);
> + status.freq =
> + ieee80211_channel_to_frequency(chanid, status.band);
> break;
> case B43_PHYTYPE_G:
> status.band = IEEE80211_BAND_2GHZ;
> @@ -819,13 +820,12 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
> case B43_PHYTYPE_HT:
> /* chanid is the SHM channel cookie. Which is the plain
> * channel number in b43. */
> - if (chanstat & B43_RX_CHAN_5GHZ) {
> + if (chanstat & B43_RX_CHAN_5GHZ)
> status.band = IEEE80211_BAND_5GHZ;
> - status.freq = b43_freq_to_channel_5ghz(chanid);
> - } else {
> + else
> status.band = IEEE80211_BAND_2GHZ;
> - status.freq = b43_freq_to_channel_2ghz(chanid);
> - }
> + status.freq =
> + ieee80211_channel_to_frequency(chanid, status.band);
> break;
> default:
> B43_WARN_ON(1);
> --
> 1.8.4.2
>
On Fri, Jan 17, 2014 at 5:53 PM, Luca Coelho <[email protected]> wrote:
> On Fri, 2014-01-17 at 10:37 +0100, Rafał Miłecki wrote:
>> 2014/1/17 ZHAO Gang <[email protected]>:
>> > On Fri, Jan 17, 2014 at 5:01 PM, Luca Coelho <[email protected]> wrote:
>> >> On Fri, 2014-01-17 at 09:56 +0100, Jonas Gorski wrote:
>> >>> On Fri, Jan 17, 2014 at 8:11 AM, Rafał Miłecki <[email protected]> wrote:
>> >>> > 2014/1/17 Luca Coelho <[email protected]>:
>> >>> >> On Fri, 2014-01-17 at 13:27 +0800, ZHAO Gang wrote:
>> >>> >>> In following patch, replace b43 specific helper function with kernel
>> >>> >>> api to reduce code duplication.
>> >>> >>>
>> >>> >>> Signed-off-by: ZHAO Gang <[email protected]>
>> >>> >>> ---
>> >>> >>> drivers/net/wireless/b43/xmit.c | 4 ++--
>> >>> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> >>> >>>
>> >>> >>> diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
>> >>> >>> index 4ae63f4..50e5ddb 100644
>> >>> >>> --- a/drivers/net/wireless/b43/xmit.c
>> >>> >>> +++ b/drivers/net/wireless/b43/xmit.c
>> >>> >>> @@ -821,10 +821,10 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
>> >>> >>> * channel number in b43. */
>> >>> >>> if (chanstat & B43_RX_CHAN_5GHZ) {
>> >>> >>> status.band = IEEE80211_BAND_5GHZ;
>> >>> >>> - status.freq = b43_freq_to_channel_5ghz(chanid);
>> >>> >>> + status.freq = b43_channel_to_freq_5ghz(chanid);
>> >>> >>> } else {
>> >>> >>> status.band = IEEE80211_BAND_2GHZ;
>> >>> >>> - status.freq = b43_freq_to_channel_2ghz(chanid);
>> >>> >>> + status.freq = b43_channel_to_freq_2ghz(chanid);
>> >>> >>> }
>> >>> >>> break;
>> >>> >>> default:
>> >>> >>
>> >>> >> Why do you need this patch if you're going to remove these calls in the
>> >>> >> next patch anyway?
>> >>> >
>> >>> > I was thinking about this for a moment too. You could just make a one
>> >>> > patch and note in commit message that "translation" was reversed.
>> >>>
>> >>> That would mean mixing fixes and improvements, which is something you
>> >>> are not supposed to do, so IMHO having these split into two is
>> >>> correct. Think about stable maintainers wanting the fix but not the
>> >>> other change because it might introduce unknown side effects.
>> >>
>> >> Makes sense. In such case, the first patch should be clearly marked as
>> >> a bug fix, so at least the commit message should be changed (ie.
>> >> mentioning the next patch in the series is useless).
>> >>
>> >
>> > I am OK to send this fix either in one patch or two, actually I have
>> > sent a version 2 which is a one patch version :-)
>> >
>> > I'm not sure if this patch is needed for stable, yes, as you said, if
>> > it's for stable, the commit message should be changed.
>>
>> Thanks for your help guys.
>>
>> I think it may be the best idea to send
>> 1/2 as fix (probably 3.14) + stable CC
>> 2/2 as improvement (for next)
>> Does it make sense?
>
> Sounds good to me. The actual fix is so simple and obvious that I don't
> see any reason for not sending it as a fix + stable.
>
Hi, after reading the code, it seems that status.freq is not actually used
in rx path, so this fix has no user sensible changes. So I think it is
not needed
to send this patch to stable.
Anyway, I should mention that the version 2 patch should be ignored.
> --
> Luca.
>
Use ieee80211_channel_to_frequency() to replace b43_channel_to_freq_{2,5}ghz(),
and remove unused b43_freq_to_channel_{2,5}ghz().
Signed-off-by: ZHAO Gang <[email protected]>
---
drivers/net/wireless/b43/main.h | 35 -----------------------------------
drivers/net/wireless/b43/xmit.c | 12 ++++++------
2 files changed, 6 insertions(+), 41 deletions(-)
diff --git a/drivers/net/wireless/b43/main.h b/drivers/net/wireless/b43/main.h
index abac25e..f476fc3 100644
--- a/drivers/net/wireless/b43/main.h
+++ b/drivers/net/wireless/b43/main.h
@@ -58,41 +58,6 @@ enum b43_verbosity {
#endif
};
-
-/* Lightweight function to convert a frequency (in Mhz) to a channel number. */
-static inline u8 b43_freq_to_channel_5ghz(int freq)
-{
- return ((freq - 5000) / 5);
-}
-static inline u8 b43_freq_to_channel_2ghz(int freq)
-{
- u8 channel;
-
- if (freq == 2484)
- channel = 14;
- else
- channel = (freq - 2407) / 5;
-
- return channel;
-}
-
-/* Lightweight function to convert a channel number to a frequency (in Mhz). */
-static inline int b43_channel_to_freq_5ghz(u8 channel)
-{
- return (5000 + (5 * channel));
-}
-static inline int b43_channel_to_freq_2ghz(u8 channel)
-{
- int freq;
-
- if (channel == 14)
- freq = 2484;
- else
- freq = 2407 + (5 * channel);
-
- return freq;
-}
-
static inline int b43_is_cck_rate(int rate)
{
return (rate == B43_CCK_RATE_1MB ||
diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
index 50e5ddb..218a0f3 100644
--- a/drivers/net/wireless/b43/xmit.c
+++ b/drivers/net/wireless/b43/xmit.c
@@ -806,7 +806,8 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
B43_WARN_ON(1);
/* FIXME: We don't really know which value the "chanid" contains.
* So the following assignment might be wrong. */
- status.freq = b43_channel_to_freq_5ghz(chanid);
+ status.freq =
+ ieee80211_channel_to_frequency(chanid, status.band);
break;
case B43_PHYTYPE_G:
status.band = IEEE80211_BAND_2GHZ;
@@ -819,13 +820,12 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
case B43_PHYTYPE_HT:
/* chanid is the SHM channel cookie. Which is the plain
* channel number in b43. */
- if (chanstat & B43_RX_CHAN_5GHZ) {
+ if (chanstat & B43_RX_CHAN_5GHZ)
status.band = IEEE80211_BAND_5GHZ;
- status.freq = b43_channel_to_freq_5ghz(chanid);
- } else {
+ else
status.band = IEEE80211_BAND_2GHZ;
- status.freq = b43_channel_to_freq_2ghz(chanid);
- }
+ status.freq =
+ ieee80211_channel_to_frequency(chanid, status.band);
break;
default:
B43_WARN_ON(1);
--
1.8.4.2
On Fri, 2014-01-17 at 09:56 +0100, Jonas Gorski wrote:
> On Fri, Jan 17, 2014 at 8:11 AM, Rafał Miłecki <[email protected]> wrote:
> > 2014/1/17 Luca Coelho <[email protected]>:
> >> On Fri, 2014-01-17 at 13:27 +0800, ZHAO Gang wrote:
> >>> In following patch, replace b43 specific helper function with kernel
> >>> api to reduce code duplication.
> >>>
> >>> Signed-off-by: ZHAO Gang <[email protected]>
> >>> ---
> >>> drivers/net/wireless/b43/xmit.c | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
> >>> index 4ae63f4..50e5ddb 100644
> >>> --- a/drivers/net/wireless/b43/xmit.c
> >>> +++ b/drivers/net/wireless/b43/xmit.c
> >>> @@ -821,10 +821,10 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
> >>> * channel number in b43. */
> >>> if (chanstat & B43_RX_CHAN_5GHZ) {
> >>> status.band = IEEE80211_BAND_5GHZ;
> >>> - status.freq = b43_freq_to_channel_5ghz(chanid);
> >>> + status.freq = b43_channel_to_freq_5ghz(chanid);
> >>> } else {
> >>> status.band = IEEE80211_BAND_2GHZ;
> >>> - status.freq = b43_freq_to_channel_2ghz(chanid);
> >>> + status.freq = b43_channel_to_freq_2ghz(chanid);
> >>> }
> >>> break;
> >>> default:
> >>
> >> Why do you need this patch if you're going to remove these calls in the
> >> next patch anyway?
> >
> > I was thinking about this for a moment too. You could just make a one
> > patch and note in commit message that "translation" was reversed.
>
> That would mean mixing fixes and improvements, which is something you
> are not supposed to do, so IMHO having these split into two is
> correct. Think about stable maintainers wanting the fix but not the
> other change because it might introduce unknown side effects.
Makes sense. In such case, the first patch should be clearly marked as
a bug fix, so at least the commit message should be changed (ie.
mentioning the next patch in the series is useless).
--
Luca.
On Fri, Jan 17, 2014 at 8:56 PM, ZHAO Gang <[email protected]> wrote:
> On Fri, Jan 17, 2014 at 5:53 PM, Luca Coelho <[email protected]> wrote:
>> On Fri, 2014-01-17 at 10:37 +0100, Rafał Miłecki wrote:
>>> 2014/1/17 ZHAO Gang <[email protected]>:
>>> > On Fri, Jan 17, 2014 at 5:01 PM, Luca Coelho <[email protected]> wrote:
>>> >> On Fri, 2014-01-17 at 09:56 +0100, Jonas Gorski wrote:
>>> >>> On Fri, Jan 17, 2014 at 8:11 AM, Rafał Miłecki <[email protected]> wrote:
>>> >>> > 2014/1/17 Luca Coelho <[email protected]>:
>>> >>> >> On Fri, 2014-01-17 at 13:27 +0800, ZHAO Gang wrote:
>>> >>> >>> In following patch, replace b43 specific helper function with kernel
>>> >>> >>> api to reduce code duplication.
>>> >>> >>>
>>> >>> >>> Signed-off-by: ZHAO Gang <[email protected]>
>>> >>> >>> ---
>>> >>> >>> drivers/net/wireless/b43/xmit.c | 4 ++--
>>> >>> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>> >>> >>>
>>> >>> >>> diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
>>> >>> >>> index 4ae63f4..50e5ddb 100644
>>> >>> >>> --- a/drivers/net/wireless/b43/xmit.c
>>> >>> >>> +++ b/drivers/net/wireless/b43/xmit.c
>>> >>> >>> @@ -821,10 +821,10 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
>>> >>> >>> * channel number in b43. */
>>> >>> >>> if (chanstat & B43_RX_CHAN_5GHZ) {
>>> >>> >>> status.band = IEEE80211_BAND_5GHZ;
>>> >>> >>> - status.freq = b43_freq_to_channel_5ghz(chanid);
>>> >>> >>> + status.freq = b43_channel_to_freq_5ghz(chanid);
>>> >>> >>> } else {
>>> >>> >>> status.band = IEEE80211_BAND_2GHZ;
>>> >>> >>> - status.freq = b43_freq_to_channel_2ghz(chanid);
>>> >>> >>> + status.freq = b43_channel_to_freq_2ghz(chanid);
>>> >>> >>> }
>>> >>> >>> break;
>>> >>> >>> default:
>>> >>> >>
>>> >>> >> Why do you need this patch if you're going to remove these calls in the
>>> >>> >> next patch anyway?
>>> >>> >
>>> >>> > I was thinking about this for a moment too. You could just make a one
>>> >>> > patch and note in commit message that "translation" was reversed.
>>> >>>
>>> >>> That would mean mixing fixes and improvements, which is something you
>>> >>> are not supposed to do, so IMHO having these split into two is
>>> >>> correct. Think about stable maintainers wanting the fix but not the
>>> >>> other change because it might introduce unknown side effects.
>>> >>
>>> >> Makes sense. In such case, the first patch should be clearly marked as
>>> >> a bug fix, so at least the commit message should be changed (ie.
>>> >> mentioning the next patch in the series is useless).
>>> >>
>>> >
>>> > I am OK to send this fix either in one patch or two, actually I have
>>> > sent a version 2 which is a one patch version :-)
>>> >
>>> > I'm not sure if this patch is needed for stable, yes, as you said, if
>>> > it's for stable, the commit message should be changed.
>>>
>>> Thanks for your help guys.
>>>
>>> I think it may be the best idea to send
>>> 1/2 as fix (probably 3.14) + stable CC
>>> 2/2 as improvement (for next)
>>> Does it make sense?
>>
>> Sounds good to me. The actual fix is so simple and obvious that I don't
>> see any reason for not sending it as a fix + stable.
>>
>
> Hi, after reading the code, it seems that status.freq is not actually used
> in rx path, so this fix has no user sensible changes. So I think it is
> not needed
> to send this patch to stable.
>
Oh yes, at least it's used in ieee80211_rx -> __ieee80211_rx_handle_packet ->
ieee80211_scan_rx -> ieee80211_get_channel(local->hw.wiphy, rx_status->freq)
Now I think it should be sent to stable. I think I should resend the two patches
to update the commit message.
> Anyway, I should mention that the version 2 patch should be ignored.
>
>> --
>> Luca.
>>
On Fri, Jan 17, 2014 at 8:11 AM, Rafał Miłecki <[email protected]> wrote:
> 2014/1/17 Luca Coelho <[email protected]>:
>> On Fri, 2014-01-17 at 13:27 +0800, ZHAO Gang wrote:
>>> In following patch, replace b43 specific helper function with kernel
>>> api to reduce code duplication.
>>>
>>> Signed-off-by: ZHAO Gang <[email protected]>
>>> ---
>>> drivers/net/wireless/b43/xmit.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
>>> index 4ae63f4..50e5ddb 100644
>>> --- a/drivers/net/wireless/b43/xmit.c
>>> +++ b/drivers/net/wireless/b43/xmit.c
>>> @@ -821,10 +821,10 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
>>> * channel number in b43. */
>>> if (chanstat & B43_RX_CHAN_5GHZ) {
>>> status.band = IEEE80211_BAND_5GHZ;
>>> - status.freq = b43_freq_to_channel_5ghz(chanid);
>>> + status.freq = b43_channel_to_freq_5ghz(chanid);
>>> } else {
>>> status.band = IEEE80211_BAND_2GHZ;
>>> - status.freq = b43_freq_to_channel_2ghz(chanid);
>>> + status.freq = b43_channel_to_freq_2ghz(chanid);
>>> }
>>> break;
>>> default:
>>
>> Why do you need this patch if you're going to remove these calls in the
>> next patch anyway?
>
> I was thinking about this for a moment too. You could just make a one
> patch and note in commit message that "translation" was reversed.
That would mean mixing fixes and improvements, which is something you
are not supposed to do, so IMHO having these split into two is
correct. Think about stable maintainers wanting the fix but not the
other change because it might introduce unknown side effects.
Jonas
>
> _______________________________________________
> b43-dev mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/b43-dev
On Fri, Jan 17, 2014 at 5:01 PM, Luca Coelho <[email protected]> wrote:
> On Fri, 2014-01-17 at 09:56 +0100, Jonas Gorski wrote:
>> On Fri, Jan 17, 2014 at 8:11 AM, Rafał Miłecki <[email protected]> wrote:
>> > 2014/1/17 Luca Coelho <[email protected]>:
>> >> On Fri, 2014-01-17 at 13:27 +0800, ZHAO Gang wrote:
>> >>> In following patch, replace b43 specific helper function with kernel
>> >>> api to reduce code duplication.
>> >>>
>> >>> Signed-off-by: ZHAO Gang <[email protected]>
>> >>> ---
>> >>> drivers/net/wireless/b43/xmit.c | 4 ++--
>> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> >>>
>> >>> diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
>> >>> index 4ae63f4..50e5ddb 100644
>> >>> --- a/drivers/net/wireless/b43/xmit.c
>> >>> +++ b/drivers/net/wireless/b43/xmit.c
>> >>> @@ -821,10 +821,10 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
>> >>> * channel number in b43. */
>> >>> if (chanstat & B43_RX_CHAN_5GHZ) {
>> >>> status.band = IEEE80211_BAND_5GHZ;
>> >>> - status.freq = b43_freq_to_channel_5ghz(chanid);
>> >>> + status.freq = b43_channel_to_freq_5ghz(chanid);
>> >>> } else {
>> >>> status.band = IEEE80211_BAND_2GHZ;
>> >>> - status.freq = b43_freq_to_channel_2ghz(chanid);
>> >>> + status.freq = b43_channel_to_freq_2ghz(chanid);
>> >>> }
>> >>> break;
>> >>> default:
>> >>
>> >> Why do you need this patch if you're going to remove these calls in the
>> >> next patch anyway?
>> >
>> > I was thinking about this for a moment too. You could just make a one
>> > patch and note in commit message that "translation" was reversed.
>>
>> That would mean mixing fixes and improvements, which is something you
>> are not supposed to do, so IMHO having these split into two is
>> correct. Think about stable maintainers wanting the fix but not the
>> other change because it might introduce unknown side effects.
>
> Makes sense. In such case, the first patch should be clearly marked as
> a bug fix, so at least the commit message should be changed (ie.
> mentioning the next patch in the series is useless).
>
I am OK to send this fix either in one patch or two, actually I have
sent a version 2 which is a one patch version :-)
I'm not sure if this patch is needed for stable, yes, as you said, if
it's for stable, the commit message should be changed.
> --
> Luca.
>
2014/1/17 ZHAO Gang <[email protected]>:
> On Fri, Jan 17, 2014 at 5:01 PM, Luca Coelho <[email protected]> wrote:
>> On Fri, 2014-01-17 at 09:56 +0100, Jonas Gorski wrote:
>>> On Fri, Jan 17, 2014 at 8:11 AM, Rafał Miłecki <[email protected]> wrote:
>>> > 2014/1/17 Luca Coelho <[email protected]>:
>>> >> On Fri, 2014-01-17 at 13:27 +0800, ZHAO Gang wrote:
>>> >>> In following patch, replace b43 specific helper function with kernel
>>> >>> api to reduce code duplication.
>>> >>>
>>> >>> Signed-off-by: ZHAO Gang <[email protected]>
>>> >>> ---
>>> >>> drivers/net/wireless/b43/xmit.c | 4 ++--
>>> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>> >>>
>>> >>> diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
>>> >>> index 4ae63f4..50e5ddb 100644
>>> >>> --- a/drivers/net/wireless/b43/xmit.c
>>> >>> +++ b/drivers/net/wireless/b43/xmit.c
>>> >>> @@ -821,10 +821,10 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
>>> >>> * channel number in b43. */
>>> >>> if (chanstat & B43_RX_CHAN_5GHZ) {
>>> >>> status.band = IEEE80211_BAND_5GHZ;
>>> >>> - status.freq = b43_freq_to_channel_5ghz(chanid);
>>> >>> + status.freq = b43_channel_to_freq_5ghz(chanid);
>>> >>> } else {
>>> >>> status.band = IEEE80211_BAND_2GHZ;
>>> >>> - status.freq = b43_freq_to_channel_2ghz(chanid);
>>> >>> + status.freq = b43_channel_to_freq_2ghz(chanid);
>>> >>> }
>>> >>> break;
>>> >>> default:
>>> >>
>>> >> Why do you need this patch if you're going to remove these calls in the
>>> >> next patch anyway?
>>> >
>>> > I was thinking about this for a moment too. You could just make a one
>>> > patch and note in commit message that "translation" was reversed.
>>>
>>> That would mean mixing fixes and improvements, which is something you
>>> are not supposed to do, so IMHO having these split into two is
>>> correct. Think about stable maintainers wanting the fix but not the
>>> other change because it might introduce unknown side effects.
>>
>> Makes sense. In such case, the first patch should be clearly marked as
>> a bug fix, so at least the commit message should be changed (ie.
>> mentioning the next patch in the series is useless).
>>
>
> I am OK to send this fix either in one patch or two, actually I have
> sent a version 2 which is a one patch version :-)
>
> I'm not sure if this patch is needed for stable, yes, as you said, if
> it's for stable, the commit message should be changed.
Thanks for your help guys.
I think it may be the best idea to send
1/2 as fix (probably 3.14) + stable CC
2/2 as improvement (for next)
Does it make sense?
--
Rafał
The b43_freq_to_channel_{2,5}ghz() should be changed to
b43_channel_to_freq{2,5}ghz() if we want to use b43 helper function,
but a better way is to use kernel api ieee80211_channel_to_frequency()
and remove these unnecessary helper function.
Signed-off-by: ZHAO Gang <[email protected]>
---
v2: combine two patches to one.
suggested by Luca Coelho and Rafał Miłecki
drivers/net/wireless/b43/main.h | 35 -----------------------------------
drivers/net/wireless/b43/xmit.c | 12 ++++++------
2 files changed, 6 insertions(+), 41 deletions(-)
diff --git a/drivers/net/wireless/b43/main.h b/drivers/net/wireless/b43/main.h
index abac25e..f476fc3 100644
--- a/drivers/net/wireless/b43/main.h
+++ b/drivers/net/wireless/b43/main.h
@@ -58,41 +58,6 @@ enum b43_verbosity {
#endif
};
-
-/* Lightweight function to convert a frequency (in Mhz) to a channel number. */
-static inline u8 b43_freq_to_channel_5ghz(int freq)
-{
- return ((freq - 5000) / 5);
-}
-static inline u8 b43_freq_to_channel_2ghz(int freq)
-{
- u8 channel;
-
- if (freq == 2484)
- channel = 14;
- else
- channel = (freq - 2407) / 5;
-
- return channel;
-}
-
-/* Lightweight function to convert a channel number to a frequency (in Mhz). */
-static inline int b43_channel_to_freq_5ghz(u8 channel)
-{
- return (5000 + (5 * channel));
-}
-static inline int b43_channel_to_freq_2ghz(u8 channel)
-{
- int freq;
-
- if (channel == 14)
- freq = 2484;
- else
- freq = 2407 + (5 * channel);
-
- return freq;
-}
-
static inline int b43_is_cck_rate(int rate)
{
return (rate == B43_CCK_RATE_1MB ||
diff --git a/drivers/net/wireless/b43/xmit.c b/drivers/net/wireless/b43/xmit.c
index 4ae63f4..218a0f3 100644
--- a/drivers/net/wireless/b43/xmit.c
+++ b/drivers/net/wireless/b43/xmit.c
@@ -806,7 +806,8 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
B43_WARN_ON(1);
/* FIXME: We don't really know which value the "chanid" contains.
* So the following assignment might be wrong. */
- status.freq = b43_channel_to_freq_5ghz(chanid);
+ status.freq =
+ ieee80211_channel_to_frequency(chanid, status.band);
break;
case B43_PHYTYPE_G:
status.band = IEEE80211_BAND_2GHZ;
@@ -819,13 +820,12 @@ void b43_rx(struct b43_wldev *dev, struct sk_buff *skb, const void *_rxhdr)
case B43_PHYTYPE_HT:
/* chanid is the SHM channel cookie. Which is the plain
* channel number in b43. */
- if (chanstat & B43_RX_CHAN_5GHZ) {
+ if (chanstat & B43_RX_CHAN_5GHZ)
status.band = IEEE80211_BAND_5GHZ;
- status.freq = b43_freq_to_channel_5ghz(chanid);
- } else {
+ else
status.band = IEEE80211_BAND_2GHZ;
- status.freq = b43_freq_to_channel_2ghz(chanid);
- }
+ status.freq =
+ ieee80211_channel_to_frequency(chanid, status.band);
break;
default:
B43_WARN_ON(1);
--
1.8.4.2