[PATCH] rtlwifi: use %*ph[C] to dump small buffers

[PATCH] rtlwifi: use %*ph[C] to dump small buffers

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtlwifi: use %*ph[C] to dump small buffers

[Joe Perches]

On Wed, 2012-09-26 at 16:57 +0300, Andy Shevchenko wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

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.



--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtlwifi: use %*ph[C] to dump small buffers

[Larry Finger]

On 09/26/2012 08:57 AM, Andy Shevchenko wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   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 <Larry.Finger@lwfinger.net>

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);
>   }
>
>

Re: [PATCH] rtlwifi: use %*ph[C] to dump small buffers

[Larry Finger]

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


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtlwifi: use %*ph[C] to dump small buffers

[Larry Finger]

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 <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>
> 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


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtlwifi: use %*ph[C] to dump small buffers

[Joe Perches]

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.



--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] rtlwifi: use %*ph[C] to dump small buffers

[David Laight]

> > 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



--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtlwifi: use %*ph[C] to dump small buffers

[Larry Finger]

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


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtlwifi: use %*ph[C] to dump small buffers

[Joe Perches]

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);