[PATCH 1/2] b43: fix the wrong assignment of status.freq in b43_rx()

[ZHAO Gang]

On Fri, Jan 17, 2014 at 8:56 PM, ZHAO Gang <gamerh2o at gmail.com> wrote:
> On Fri, Jan 17, 2014 at 5:53 PM, Luca Coelho <luca at coelho.fi> wrote:
>> On Fri, 2014-01-17 at 10:37 +0100, Rafał Miłecki wrote:
>>> 2014/1/17 ZHAO Gang <gamerh2o at gmail.com>:
>>> > On Fri, Jan 17, 2014 at 5:01 PM, Luca Coelho <luca at coelho.fi> 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 <zajec5 at gmail.com> wrote:
>>> >>> > 2014/1/17 Luca Coelho <luca at coelho.fi>:
>>> >>> >> 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 <gamerh2o at gmail.com>
>>> >>> >>> ---
>>> >>> >>>  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.
>>