Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/net/wireless/rtlwifi/cam.c | 7 ++-----
drivers/net/wireless/rtlwifi/rtl8192ce/hw.c | 6 ++----
drivers/net/wireless/rtlwifi/rtl8192cu/hw.c | 6 ++----
3 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/rtlwifi/cam.c b/drivers/net/wireless/rtlwifi/cam.c
index 5b4b4d4..6ba9f80 100644
--- a/drivers/net/wireless/rtlwifi/cam.c
+++ b/drivers/net/wireless/rtlwifi/cam.c
@@ -52,11 +52,8 @@ static void rtl_cam_program_entry(struct ieee80211_hw *hw, u32 entry_no,
u32 target_content = 0;
u8 entry_i;
- RT_TRACE(rtlpriv, COMP_SEC, DBG_LOUD,
- "key_cont_128:\n %x:%x:%x:%x:%x:%x\n",
- key_cont_128[0], key_cont_128[1],
- key_cont_128[2], key_cont_128[3],
- key_cont_128[4], key_cont_128[5]);
+ RT_TRACE(rtlpriv, COMP_SEC, DBG_LOUD, "key_cont_128: %*ph\n",
+ 6, key_cont_128);
for (entry_i = 0; entry_i < CAM_CONTENT_COUNT; entry_i++) {
target_command = entry_i + CAM_CONTENT_COUNT * entry_no;
diff --git a/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c b/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
index 86d73b3..932780d 100644
--- a/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
+++ b/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
@@ -1918,10 +1918,8 @@ static void rtl92ce_update_hal_rate_mask(struct ieee80211_hw *hw,
(ratr_index << 28);
rate_mask[4] = macid | (shortgi ? 0x20 : 0x00) | 0x80;
RT_TRACE(rtlpriv, COMP_RATR, DBG_DMESG,
- "Rate_index:%x, ratr_val:%x, %x:%x:%x:%x:%x\n",
- ratr_index, ratr_bitmap,
- rate_mask[0], rate_mask[1], rate_mask[2], rate_mask[3],
- rate_mask[4]);
+ "Rate_index:%x, ratr_val:%x, %*phC\n",
+ ratr_index, ratr_bitmap, 5, rate_mask);
rtl92c_fill_h2c_cmd(hw, H2C_RA_MASK, 5, rate_mask);
if (macid != 0)
diff --git a/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c b/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c
index 4bbb711..7554501 100644
--- a/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c
+++ b/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c
@@ -2169,10 +2169,8 @@ void rtl92cu_update_hal_rate_mask(struct ieee80211_hw *hw, u8 rssi_level)
ratr_index << 28);
rate_mask[4] = macid | (shortgi ? 0x20 : 0x00) | 0x80;
RT_TRACE(rtlpriv, COMP_RATR, DBG_DMESG,
- "Rate_index:%x, ratr_val:%x, %x:%x:%x:%x:%x\n",
- ratr_index, ratr_bitmap,
- rate_mask[0], rate_mask[1], rate_mask[2], rate_mask[3],
- rate_mask[4]);
+ "Rate_index:%x, ratr_val:%x, %*phC\n",
+ ratr_index, ratr_bitmap, 5, rate_mask);
rtl92c_fill_h2c_cmd(hw, H2C_RA_MASK, 5, rate_mask);
}
--
1.7.10.4
On Thu, 2012-09-27 at 17:28 -0500, Larry Finger wrote:
> On 09/26/2012 11:45 AM, Joe Perches wrote:
> > rate_mask uses:
> >
> > u32 ratr_bitmap = (u32) mac->basic_rates;
> > ...
> > u8 rate_mask[5];
> > ...
> > [sets ratr_bitmap as u32]
> > ...
> > *(u32 *)&rate_mask = ((ratr_bitmap & 0x0fffffff) |
> > ratr_index << 28);
> > ...
> > rtl92c_fill_h2c_cmd(hw, H2C_RA_MASK, 5, rate_mask);
> >
> > Looks like a possible endian misuse to me.
>
> After a second look at the code, I don't think this is an endian issue; however,
> I am proposing the following changes to remove any doubt:
I believe if it wasn't an endian problem, then the new
code makes it one.
> Index: wireless-testing-new/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
> ===================================================================
> --- wireless-testing-new.orig/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
> +++ wireless-testing-new/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
> @@ -1964,8 +1965,9 @@ static void rtl92ce_update_hal_rate_mask
>
> RT_TRACE(rtlpriv, COMP_RATR, DBG_DMESG,
> "ratr_bitmap :%x\n", ratr_bitmap);
> - *(u32 *)&rate_mask = (ratr_bitmap & 0x0fffffff) |
> - (ratr_index << 28);
> + for (i = 0; i < 3; i++)
> + rate_mask[i] = ratr_bitmap & (0xff << (i * 4));
rate_mask is u8, doesn't this needs (calc) >> (i * 8)
> + rate_mask[3] = (ratr_bitmap & 0x0f000000) | (ratr_index << 28);
Perhaps you meant:
((ratr_bitmap & 0x0f000000) >> 24) | (ratr_index << 4)
> rate_mask[4] = macid | (shortgi ? 0x20 : 0x00) | 0x80;
> RT_TRACE(rtlpriv, COMP_RATR, DBG_DMESG,
> "Rate_index:%x, ratr_val:%x, %*phC\n",
>
> The downstream routine uses this info as an array of u8, thus it is OK. A proper
> patch will be sent later. At least we avoid that ugly cast.
On Wed, 2012-09-26 at 16:57 +0300, Andy Shevchenko wrote:
> Signed-off-by: Andy Shevchenko <[email protected]>
Hi Andy.
(adding Larry and Chaoming to cc's)
Please use scripts/get_maintainer.pl on your patches to
see who maintains these files so you can cc them.
One comment below, it looks like a possible endian bug.
(not in your patch)
> ---
> drivers/net/wireless/rtlwifi/cam.c | 7 ++-----
> drivers/net/wireless/rtlwifi/rtl8192ce/hw.c | 6 ++----
> drivers/net/wireless/rtlwifi/rtl8192cu/hw.c | 6 ++----
> 3 files changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/wireless/rtlwifi/cam.c b/drivers/net/wireless/rtlwifi/cam.c
> index 5b4b4d4..6ba9f80 100644
> --- a/drivers/net/wireless/rtlwifi/cam.c
> +++ b/drivers/net/wireless/rtlwifi/cam.c
> @@ -52,11 +52,8 @@ static void rtl_cam_program_entry(struct ieee80211_hw *hw, u32 entry_no,
> u32 target_content = 0;
> u8 entry_i;
>
> - RT_TRACE(rtlpriv, COMP_SEC, DBG_LOUD,
> - "key_cont_128:\n %x:%x:%x:%x:%x:%x\n",
> - key_cont_128[0], key_cont_128[1],
> - key_cont_128[2], key_cont_128[3],
> - key_cont_128[4], key_cont_128[5]);
> + RT_TRACE(rtlpriv, COMP_SEC, DBG_LOUD, "key_cont_128: %*ph\n",
> + 6, key_cont_128);
>
> for (entry_i = 0; entry_i < CAM_CONTENT_COUNT; entry_i++) {
> target_command = entry_i + CAM_CONTENT_COUNT * entry_no;
> diff --git a/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c b/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
> index 86d73b3..932780d 100644
> --- a/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
> +++ b/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
> @@ -1918,10 +1918,8 @@ static void rtl92ce_update_hal_rate_mask(struct ieee80211_hw *hw,
> (ratr_index << 28);
> rate_mask[4] = macid | (shortgi ? 0x20 : 0x00) | 0x80;
> RT_TRACE(rtlpriv, COMP_RATR, DBG_DMESG,
> - "Rate_index:%x, ratr_val:%x, %x:%x:%x:%x:%x\n",
> - ratr_index, ratr_bitmap,
> - rate_mask[0], rate_mask[1], rate_mask[2], rate_mask[3],
> - rate_mask[4]);
> + "Rate_index:%x, ratr_val:%x, %*phC\n",
> + ratr_index, ratr_bitmap, 5, rate_mask);
> rtl92c_fill_h2c_cmd(hw, H2C_RA_MASK, 5, rate_mask);
>
> if (macid != 0)
> diff --git a/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c b/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c
> index 4bbb711..7554501 100644
> --- a/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c
> +++ b/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c
> @@ -2169,10 +2169,8 @@ void rtl92cu_update_hal_rate_mask(struct ieee80211_hw *hw, u8 rssi_level)
> ratr_index << 28);
> rate_mask[4] = macid | (shortgi ? 0x20 : 0x00) | 0x80;
> RT_TRACE(rtlpriv, COMP_RATR, DBG_DMESG,
> - "Rate_index:%x, ratr_val:%x, %x:%x:%x:%x:%x\n",
> - ratr_index, ratr_bitmap,
> - rate_mask[0], rate_mask[1], rate_mask[2], rate_mask[3],
> - rate_mask[4]);
> + "Rate_index:%x, ratr_val:%x, %*phC\n",
> + ratr_index, ratr_bitmap, 5, rate_mask);
> rtl92c_fill_h2c_cmd(hw, H2C_RA_MASK, 5, rate_mask);
> }
>
rate_mask uses:
u32 ratr_bitmap = (u32) mac->basic_rates;
...
u8 rate_mask[5];
...
[sets ratr_bitmap as u32]
...
*(u32 *)&rate_mask = ((ratr_bitmap & 0x0fffffff) |
ratr_index << 28);
...
rtl92c_fill_h2c_cmd(hw, H2C_RA_MASK, 5, rate_mask);
Looks like a possible endian misuse to me.
On 09/26/2012 11:45 AM, Joe Perches wrote:
>
> rate_mask uses:
>
> u32 ratr_bitmap = (u32) mac->basic_rates;
> ...
> u8 rate_mask[5];
> ...
> [sets ratr_bitmap as u32]
> ...
> *(u32 *)&rate_mask = ((ratr_bitmap & 0x0fffffff) |
> ratr_index << 28);
> ...
> rtl92c_fill_h2c_cmd(hw, H2C_RA_MASK, 5, rate_mask);
>
> Looks like a possible endian misuse to me.
Joe,
I had to track the flow through two more routines, but I think your analysis is
correct. The only BE platform I have does not have any PCIe hardware, thus I can
only test the USB version. It does not show a major problem, but this one with
the rate masks would be subtle.
Thanks,
Larry
On Fri, 2012-09-28 at 11:41 -0500, Larry Finger wrote:
> On 09/28/2012 04:04 AM, David Laight wrote:
> >>> Index: wireless-testing-new/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
[]
> >>> - *(u32 *)&rate_mask = (ratr_bitmap & 0x0fffffff) |
> >>> - (ratr_index << 28);
> >>> + for (i = 0; i < 3; i++)
> >>> + rate_mask[i] = ratr_bitmap & (0xff << (i * 4));
[]
> > So this either fixes, or adds, an endianness bug.
>
> Yes, the rate_mask array is little endian after this fragment is run, but the
> only use of the byte array is to write it to the device, and LE is what it needs
> no matter the platform. This change fixes an endianness bug.
>
> As I tend to get confused when doing these things, I wrote a small test program
> and ran it on x86_64 and PPC-32 to confirm the result.
>
> Thanks for teaching me about a = b >>= 8. I was not aware that C could do that.
The other thing that could be done is to use cpu_to_le32()
but David's method I think is superior for this use as there's
no absolute guarantee that rate_mask is aligned on a u32.
I'd add parens to make the precedence explicit.
rate_mask[0] = ratr_bitmap;
rate_mask[1] = (ratr_bitmap >>= 8);
rate_mask[2] = (ratr_bitmap >>= 8);
rate_mask[3] = ((ratr_bitmap >> 8) & 0xf) | (ratr_index << 4);
On 09/26/2012 11:45 AM, Joe Perches wrote:
> On Wed, 2012-09-26 at 16:57 +0300, Andy Shevchenko wrote:
>> Signed-off-by: Andy Shevchenko <[email protected]>
>
> Hi Andy.
>
> (adding Larry and Chaoming to cc's)
>
> Please use scripts/get_maintainer.pl on your patches to
> see who maintains these files so you can cc them.
>
> One comment below, it looks like a possible endian bug.
> (not in your patch)
>
>> ---
>> drivers/net/wireless/rtlwifi/cam.c | 7 ++-----
>> drivers/net/wireless/rtlwifi/rtl8192ce/hw.c | 6 ++----
>> drivers/net/wireless/rtlwifi/rtl8192cu/hw.c | 6 ++----
>> 3 files changed, 6 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rtlwifi/cam.c b/drivers/net/wireless/rtlwifi/cam.c
>> index 5b4b4d4..6ba9f80 100644
>> --- a/drivers/net/wireless/rtlwifi/cam.c
>> +++ b/drivers/net/wireless/rtlwifi/cam.c
>> @@ -52,11 +52,8 @@ static void rtl_cam_program_entry(struct ieee80211_hw *hw, u32 entry_no,
>> u32 target_content = 0;
>> u8 entry_i;
>>
>> - RT_TRACE(rtlpriv, COMP_SEC, DBG_LOUD,
>> - "key_cont_128:\n %x:%x:%x:%x:%x:%x\n",
>> - key_cont_128[0], key_cont_128[1],
>> - key_cont_128[2], key_cont_128[3],
>> - key_cont_128[4], key_cont_128[5]);
>> + RT_TRACE(rtlpriv, COMP_SEC, DBG_LOUD, "key_cont_128: %*ph\n",
>> + 6, key_cont_128);
>>
>> for (entry_i = 0; entry_i < CAM_CONTENT_COUNT; entry_i++) {
>> target_command = entry_i + CAM_CONTENT_COUNT * entry_no;
>> diff --git a/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c b/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
>> index 86d73b3..932780d 100644
>> --- a/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
>> +++ b/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
>> @@ -1918,10 +1918,8 @@ static void rtl92ce_update_hal_rate_mask(struct ieee80211_hw *hw,
>> (ratr_index << 28);
>> rate_mask[4] = macid | (shortgi ? 0x20 : 0x00) | 0x80;
>> RT_TRACE(rtlpriv, COMP_RATR, DBG_DMESG,
>> - "Rate_index:%x, ratr_val:%x, %x:%x:%x:%x:%x\n",
>> - ratr_index, ratr_bitmap,
>> - rate_mask[0], rate_mask[1], rate_mask[2], rate_mask[3],
>> - rate_mask[4]);
>> + "Rate_index:%x, ratr_val:%x, %*phC\n",
>> + ratr_index, ratr_bitmap, 5, rate_mask);
>> rtl92c_fill_h2c_cmd(hw, H2C_RA_MASK, 5, rate_mask);
>>
>> if (macid != 0)
>> diff --git a/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c b/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c
>> index 4bbb711..7554501 100644
>> --- a/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c
>> +++ b/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c
>> @@ -2169,10 +2169,8 @@ void rtl92cu_update_hal_rate_mask(struct ieee80211_hw *hw, u8 rssi_level)
>> ratr_index << 28);
>> rate_mask[4] = macid | (shortgi ? 0x20 : 0x00) | 0x80;
>> RT_TRACE(rtlpriv, COMP_RATR, DBG_DMESG,
>> - "Rate_index:%x, ratr_val:%x, %x:%x:%x:%x:%x\n",
>> - ratr_index, ratr_bitmap,
>> - rate_mask[0], rate_mask[1], rate_mask[2], rate_mask[3],
>> - rate_mask[4]);
>> + "Rate_index:%x, ratr_val:%x, %*phC\n",
>> + ratr_index, ratr_bitmap, 5, rate_mask);
>> rtl92c_fill_h2c_cmd(hw, H2C_RA_MASK, 5, rate_mask);
>> }
>>
>
> rate_mask uses:
>
> u32 ratr_bitmap = (u32) mac->basic_rates;
> ...
> u8 rate_mask[5];
> ...
> [sets ratr_bitmap as u32]
> ...
> *(u32 *)&rate_mask = ((ratr_bitmap & 0x0fffffff) |
> ratr_index << 28);
> ...
> rtl92c_fill_h2c_cmd(hw, H2C_RA_MASK, 5, rate_mask);
>
> Looks like a possible endian misuse to me.
After a second look at the code, I don't think this is an endian issue; however,
I am proposing the following changes to remove any doubt:
Index: wireless-testing-new/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
===================================================================
--- wireless-testing-new.orig/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
+++ wireless-testing-new/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
@@ -1964,8 +1965,9 @@ static void rtl92ce_update_hal_rate_mask
RT_TRACE(rtlpriv, COMP_RATR, DBG_DMESG,
"ratr_bitmap :%x\n", ratr_bitmap);
- *(u32 *)&rate_mask = (ratr_bitmap & 0x0fffffff) |
- (ratr_index << 28);
+ for (i = 0; i < 3; i++)
+ rate_mask[i] = ratr_bitmap & (0xff << (i * 4));
+ rate_mask[3] = (ratr_bitmap & 0x0f000000) | (ratr_index << 28);
rate_mask[4] = macid | (shortgi ? 0x20 : 0x00) | 0x80;
RT_TRACE(rtlpriv, COMP_RATR, DBG_DMESG,
"Rate_index:%x, ratr_val:%x, %*phC\n",
The downstream routine uses this info as an array of u8, thus it is OK. A proper
patch will be sent later. At least we avoid that ugly cast.
Larry
> > Index: wireless-testing-new/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
> > ===================================================================
> > --- wireless-testing-new.orig/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
> > +++ wireless-testing-new/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
> > @@ -1964,8 +1965,9 @@ static void rtl92ce_update_hal_rate_mask
> >
> > RT_TRACE(rtlpriv, COMP_RATR, DBG_DMESG,
> > "ratr_bitmap :%x\n", ratr_bitmap);
> > - *(u32 *)&rate_mask = (ratr_bitmap & 0x0fffffff) |
> > - (ratr_index << 28);
> > + for (i = 0; i < 3; i++)
> > + rate_mask[i] = ratr_bitmap & (0xff << (i * 4));
>
> rate_mask is u8, doesn't this needs (calc) >> (i * 8)
>
> > + rate_mask[3] = (ratr_bitmap & 0x0f000000) | (ratr_index << 28);
>
> Perhaps you meant:
>
> ((ratr_bitmap & 0x0f000000) >> 24) | (ratr_index << 4)
I'd just do:
rate_mask[0] = ratr_bitmap;
rate_mask[1] = ratr_bitmap >>= 8;
rate_mask[2] = ratr_bitmap >>= 8;
rate_mask[3] = (ratr_bitmap >> 8) & 0xf | ratr_index << 4;
which is, of course, little endian.
Which means it is different from the original code on big-endian systems.
So changing this here ought to require a change when the data is read.
So this either fixes, or adds, an endianness bug.
David
On 09/26/2012 08:57 AM, Andy Shevchenko wrote:
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/net/wireless/rtlwifi/cam.c | 7 ++-----
> drivers/net/wireless/rtlwifi/rtl8192ce/hw.c | 6 ++----
> drivers/net/wireless/rtlwifi/rtl8192cu/hw.c | 6 ++----
> 3 files changed, 6 insertions(+), 13 deletions(-)
Andy,
Thanks for this. I have only one suggestion. I try to include all the drivers
affected in the subject line. As this one changes rtl8192ce, and rtl8192cu, I
would like to see "rtlwifi: rtl8192ce: rtl8192cu: use ..." as the subject. Other
than that, ACKed-by: Larry Finger <[email protected]>
Larry
>
> diff --git a/drivers/net/wireless/rtlwifi/cam.c b/drivers/net/wireless/rtlwifi/cam.c
> index 5b4b4d4..6ba9f80 100644
> --- a/drivers/net/wireless/rtlwifi/cam.c
> +++ b/drivers/net/wireless/rtlwifi/cam.c
> @@ -52,11 +52,8 @@ static void rtl_cam_program_entry(struct ieee80211_hw *hw, u32 entry_no,
> u32 target_content = 0;
> u8 entry_i;
>
> - RT_TRACE(rtlpriv, COMP_SEC, DBG_LOUD,
> - "key_cont_128:\n %x:%x:%x:%x:%x:%x\n",
> - key_cont_128[0], key_cont_128[1],
> - key_cont_128[2], key_cont_128[3],
> - key_cont_128[4], key_cont_128[5]);
> + RT_TRACE(rtlpriv, COMP_SEC, DBG_LOUD, "key_cont_128: %*ph\n",
> + 6, key_cont_128);
>
> for (entry_i = 0; entry_i < CAM_CONTENT_COUNT; entry_i++) {
> target_command = entry_i + CAM_CONTENT_COUNT * entry_no;
> diff --git a/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c b/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
> index 86d73b3..932780d 100644
> --- a/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
> +++ b/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
> @@ -1918,10 +1918,8 @@ static void rtl92ce_update_hal_rate_mask(struct ieee80211_hw *hw,
> (ratr_index << 28);
> rate_mask[4] = macid | (shortgi ? 0x20 : 0x00) | 0x80;
> RT_TRACE(rtlpriv, COMP_RATR, DBG_DMESG,
> - "Rate_index:%x, ratr_val:%x, %x:%x:%x:%x:%x\n",
> - ratr_index, ratr_bitmap,
> - rate_mask[0], rate_mask[1], rate_mask[2], rate_mask[3],
> - rate_mask[4]);
> + "Rate_index:%x, ratr_val:%x, %*phC\n",
> + ratr_index, ratr_bitmap, 5, rate_mask);
> rtl92c_fill_h2c_cmd(hw, H2C_RA_MASK, 5, rate_mask);
>
> if (macid != 0)
> diff --git a/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c b/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c
> index 4bbb711..7554501 100644
> --- a/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c
> +++ b/drivers/net/wireless/rtlwifi/rtl8192cu/hw.c
> @@ -2169,10 +2169,8 @@ void rtl92cu_update_hal_rate_mask(struct ieee80211_hw *hw, u8 rssi_level)
> ratr_index << 28);
> rate_mask[4] = macid | (shortgi ? 0x20 : 0x00) | 0x80;
> RT_TRACE(rtlpriv, COMP_RATR, DBG_DMESG,
> - "Rate_index:%x, ratr_val:%x, %x:%x:%x:%x:%x\n",
> - ratr_index, ratr_bitmap,
> - rate_mask[0], rate_mask[1], rate_mask[2], rate_mask[3],
> - rate_mask[4]);
> + "Rate_index:%x, ratr_val:%x, %*phC\n",
> + ratr_index, ratr_bitmap, 5, rate_mask);
> rtl92c_fill_h2c_cmd(hw, H2C_RA_MASK, 5, rate_mask);
> }
>
>
On 09/28/2012 04:04 AM, David Laight wrote:
>>> Index: wireless-testing-new/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
>>> ===================================================================
>>> --- wireless-testing-new.orig/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
>>> +++ wireless-testing-new/drivers/net/wireless/rtlwifi/rtl8192ce/hw.c
>>> @@ -1964,8 +1965,9 @@ static void rtl92ce_update_hal_rate_mask
>>>
>>> RT_TRACE(rtlpriv, COMP_RATR, DBG_DMESG,
>>> "ratr_bitmap :%x\n", ratr_bitmap);
>>> - *(u32 *)&rate_mask = (ratr_bitmap & 0x0fffffff) |
>>> - (ratr_index << 28);
>>> + for (i = 0; i < 3; i++)
>>> + rate_mask[i] = ratr_bitmap & (0xff << (i * 4));
>>
>> rate_mask is u8, doesn't this needs (calc) >> (i * 8)
>>
>>> + rate_mask[3] = (ratr_bitmap & 0x0f000000) | (ratr_index << 28);
>>
>> Perhaps you meant:
>>
>> ((ratr_bitmap & 0x0f000000) >> 24) | (ratr_index << 4)
>
> I'd just do:
> rate_mask[0] = ratr_bitmap;
> rate_mask[1] = ratr_bitmap >>= 8;
> rate_mask[2] = ratr_bitmap >>= 8;
> rate_mask[3] = (ratr_bitmap >> 8) & 0xf | ratr_index << 4;
> which is, of course, little endian.
> Which means it is different from the original code on big-endian systems.
> So changing this here ought to require a change when the data is read.
> So this either fixes, or adds, an endianness bug.
Yes, the rate_mask array is little endian after this fragment is run, but the
only use of the byte array is to write it to the device, and LE is what it needs
no matter the platform. This change fixes an endianness bug.
As I tend to get confused when doing these things, I wrote a small test program
and ran it on x86_64 and PPC-32 to confirm the result.
Thanks for teaching me about a = b >>= 8. I was not aware that C could do that.
Larry