Re: [PATCH] ath6kl: Fix target assert in p2p bringup with multi vif

[Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>]

On Friday 06 April 2012 03:33 PM, Kalle Valo wrote:
> On 04/05/2012 09:46 AM, Vasanthakumar Thiagarajan wrote:
>> Using interface 0 for p2p causes target assert. This is because
>> interface 0 is always initialized to non-p2p operations. Fix this
>> issue by initializing all the interfaces for p2p when fw is capable
>> of dynamic interface switching. When fw is not capable of dynamic
>> switching, make sure p2p is not brought up on interface which is
>> not initialized for this purpose.
>>
>> Reported-by: Naveen Singh navesing@qca.qualcomm.com
>> Signed-off-by: Vasanthakumar Thiagarajan<vthiagar@qca.qualcomm.com>
>> ---
>>   drivers/net/wireless/ath/ath6kl/cfg80211.c |   26 +++++++++++++++++++++++
>>   drivers/net/wireless/ath/ath6kl/init.c     |   31 ++++++++++++++++++----------
>>   2 files changed, 46 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c b/drivers/net/wireless/ath/ath6kl/cfg80211.c
>> index 1272508..26fb3d7 100644
>> --- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
>> +++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
>> @@ -1451,9 +1451,35 @@ static int ath6kl_cfg80211_change_iface(struct wiphy *wiphy,
>>   					struct vif_params *params)
>>   {
>>   	struct ath6kl_vif *vif = netdev_priv(ndev);
>> +	int i;
>>
>>   	ath6kl_dbg(ATH6KL_DBG_WLAN_CFG, "%s: type %u\n", __func__, type);
>>
>> +	/*
>> +	 * Don't bring up p2p on an interface which is not initialized
>> +	 * for p2p operation where fw does not have capability to switch
>> +	 * dynamically between non-p2p and p2p type interface.
>> +	 */
>> +	if (!test_bit(ATH6KL_FW_CAPABILITY_STA_P2PDEV_DUPLEX,
>> +		      vif->ar->fw_capabilities)) {
>> +		if (type == NL80211_IFTYPE_P2P_CLIENT ||
>> +		    type == NL80211_IFTYPE_P2P_GO) {
>> +			if (vif->ar->vif_max>  1) {
>> +				for (i = vif->ar->max_norm_iface;
>> +				     i<  vif->ar->vif_max; i++) {
>> +					if (i == vif->fw_vif_idx)
>> +						break;
>> +				}
>> +				if (i == vif->ar->vif_max) {
>> +					ath6kl_err("Invalid interface to bring up P2P\n");
>> +					return -EINVAL;
>> +				}
>> +			} else
>> +				if (vif->fw_vif_idx != 0)
>> +					return -EINVAL;
>> +		}
>> +	}
>
> IMHO this is difficult to read. Could it be simplified by reducing the
> unnecessary indentation:
>
> 	if (!test_bit(ATH6KL_FW_CAPABILITY_STA_P2PDEV_DUPLEX,
> 		      vif->ar->fw_capabilities)&&
>              (type == NL80211_IFTYPE_P2P_CLIENT ||
> 	     type == NL80211_IFTYPE_P2P_GO)) {
>
> 		if (vif->ar->vif_max == 0) {
> 			if (vif->fw_vif_idx != 0)
> 				return -EINVAL;
> 			else
> 				goto foo;
> 		}
>
> 		for (i = vif->ar->max_norm_iface;
> 		     i<  vif->ar->vif_max; i++) {
> 			if (i == vif->fw_vif_idx)
> 				break;
> 			}
> 		}
>
> 		if (i == vif->ar->vif_max) {
> 			ath6kl_err("Invalid interface to bring up P2P\n");
> 			return -EINVAL;
> 		}
> 	}
>
> foo:
>
> I'm sure I added bugs above but hopefully it still shows my idea. At
> least you can combine the first two if statements and get rid of one
> indentation level.

I get your idea, i'll change it. Thanks!

Vasanth