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

[Kalle Valo <kvalo@qca.qualcomm.com>]

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.

Kalle