diff mbox

[v2,1/8] ath10k: enhance swba event handler to adapt different size tim bitmap

Message ID 87h9poxjvs.fsf@kamboji.qca.qualcomm.com (mailing list archive)
State Accepted
Headers show

Commit Message

Kalle Valo July 1, 2015, 10:16 a.m. UTC 
Raja Mani <rmani@qti.qualcomm.com> writes:

> Due to 512 client support in 10.4 firmware, size of tim ie is going
> to be slightly higher than non 10.4 firmware. So, size of tim_bitmap
> what is carried in swba event from 10.4 firmware is bit higher.
>
> The only bottle neck to reuse existing swba handler
> ath10k_wmi_event_host_swba() for 10.4 is that code designed to deal
> with fixed size tim bitmap(ie, tim_info[].tim_bitmap in wmi_swba_ev_arg).
> This patch removes such size limitation and makes it more suitable
> to handle swba event which has different size tim bitmap.
>
> All existing swba event parsing functions are changed to adapt this
> change. Actual support to handle 10.4 swba event is added in next patch.
> Only preparation is made in this patch.
>
> Signed-off-by: Raja Mani <rmani@qti.qualcomm.com>

[...]

> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -2874,33 +2874,38 @@ exit:
>  static void ath10k_wmi_update_tim(struct ath10k *ar,
>  				  struct ath10k_vif *arvif,
>  				  struct sk_buff *bcn,
> -				  const struct wmi_tim_info *tim_info)
> +				  const struct wmi_tim_info_arg *tim_info)
>  {
>  	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)bcn->data;
>  	struct ieee80211_tim_ie *tim;
>  	u8 *ies, *ie;
>  	u8 ie_len, pvm_len;
>  	__le32 t;
> -	u32 v;
> +	u32 v, tim_len;
> +
> +	/* When FW reports 0 in tim_len, ensure atleast first byte
> +	 * in tim_bitmap is considered for pvm calculation.
> +	 */
> +	tim_len = tim_info->tim_len ? __le32_to_cpu(tim_info->tim_len) : 1;
>  
>  	/* if next SWBA has no tim_changed the tim_bitmap is garbage.
>  	 * we must copy the bitmap upon change and reuse it later */
>  	if (__le32_to_cpu(tim_info->tim_changed)) {
>  		int i;
>  
> -		BUILD_BUG_ON(sizeof(arvif->u.ap.tim_bitmap) !=
> -			     sizeof(tim_info->tim_bitmap));
> +		WARN_ON(sizeof(arvif->u.ap.tim_bitmap) < tim_len);

I'm worried that this WARN_ON() spams too much so I changed it to:

Comments

Raja Mani July 1, 2015, 10:21 a.m. UTC | #1 
On 07/01/2015 03:46 PM, Kalle Valo wrote:
> Raja Mani <rmani@qti.qualcomm.com> writes:
>
>> Due to 512 client support in 10.4 firmware, size of tim ie is going
>> to be slightly higher than non 10.4 firmware. So, size of tim_bitmap
>> what is carried in swba event from 10.4 firmware is bit higher.
>>
>> The only bottle neck to reuse existing swba handler
>> ath10k_wmi_event_host_swba() for 10.4 is that code designed to deal
>> with fixed size tim bitmap(ie, tim_info[].tim_bitmap in wmi_swba_ev_arg).
>> This patch removes such size limitation and makes it more suitable
>> to handle swba event which has different size tim bitmap.
>>
>> All existing swba event parsing functions are changed to adapt this
>> change. Actual support to handle 10.4 swba event is added in next patch.
>> Only preparation is made in this patch.
>>
>> Signed-off-by: Raja Mani <rmani@qti.qualcomm.com>
>
[..]
>> -		BUILD_BUG_ON(sizeof(arvif->u.ap.tim_bitmap) !=
>> -			     sizeof(tim_info->tim_bitmap));
>> +		WARN_ON(sizeof(arvif->u.ap.tim_bitmap) < tim_len);
>
> I'm worried that this WARN_ON() spams too much so I changed it to:
>
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -2893,7 +2893,7 @@ static void ath10k_wmi_update_tim(struct ath10k *ar,
>          if (__le32_to_cpu(tim_info->tim_changed)) {
>                  int i;
>
> -               WARN_ON(sizeof(arvif->u.ap.tim_bitmap) < tim_len);
> +               WARN_ON_ONCE(sizeof(arvif->u.ap.tim_bitmap) < tim_len);
>
>                  for (i = 0; i < tim_len; i++) {
>                          t = tim_info->tim_bitmap[i / 4];
>
>

The change looks good to me.

--
Raja
Kalle Valo July 1, 2015, 10:28 a.m. UTC | #2 
Kalle Valo <kvalo@qca.qualcomm.com> writes:

>>  	/* if next SWBA has no tim_changed the tim_bitmap is garbage.
>>  	 * we must copy the bitmap upon change and reuse it later */
>>  	if (__le32_to_cpu(tim_info->tim_changed)) {
>>  		int i;
>>  
>> -		BUILD_BUG_ON(sizeof(arvif->u.ap.tim_bitmap) !=
>> -			     sizeof(tim_info->tim_bitmap));
>> +		WARN_ON(sizeof(arvif->u.ap.tim_bitmap) < tim_len);
>
> I'm worried that this WARN_ON() spams too much so I changed it to:
>
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -2893,7 +2893,7 @@ static void ath10k_wmi_update_tim(struct ath10k *ar,
>         if (__le32_to_cpu(tim_info->tim_changed)) {
>                 int i;
>  
> -               WARN_ON(sizeof(arvif->u.ap.tim_bitmap) < tim_len);
> +               WARN_ON_ONCE(sizeof(arvif->u.ap.tim_bitmap) < tim_len);
>  
>                 for (i = 0; i < tim_len; i++) {
>                         t = tim_info->tim_bitmap[i / 4];

Actually I got more worried about this. If tim_len >
sizeof(arvif->u.ap.tim_bitmap) don't we read out of bounds? So we should
actually add return for that case or am I missing something?

Full code:

		WARN_ON_ONCE(sizeof(arvif->u.ap.tim_bitmap) < tim_len);

		for (i = 0; i < tim_len; i++) {
			t = tim_info->tim_bitmap[i / 4];
			v = __le32_to_cpu(t);
			arvif->u.ap.tim_bitmap[i] = (v >> ((i % 4) * 8)) & 0xFF;
		}
diff mbox

Patch

--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -2893,7 +2893,7 @@  static void ath10k_wmi_update_tim(struct ath10k *ar,
        if (__le32_to_cpu(tim_info->tim_changed)) {
                int i;
 
-               WARN_ON(sizeof(arvif->u.ap.tim_bitmap) < tim_len);
+               WARN_ON_ONCE(sizeof(arvif->u.ap.tim_bitmap) < tim_len);
 
                for (i = 0; i < tim_len; i++) {
                        t = tim_info->tim_bitmap[i / 4];