DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org BC2BF602FC
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Maya Erez <merez@codeaurora.org>
Cc:     Ahmad Masri <amasri@codeaurora.org>,
        linux-wireless@vger.kernel.org, wil6210@qti.qualcomm.com
Subject: Re: [PATCH 03/21] wil6210: support up to 20 stations in AP mode
References: <1550845279-16103-1-git-send-email-merez@codeaurora.org>
        <1550845279-16103-4-git-send-email-merez@codeaurora.org>
Date:   Thu, 28 Feb 2019 11:17:23 +0200
In-Reply-To: <1550845279-16103-4-git-send-email-merez@codeaurora.org> (Maya
        Erez's message of "Fri, 22 Feb 2019 16:21:01 +0200")
Message-ID: <87va14z2zg.fsf@purkki.adurom.net>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.4 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Maya Erez <merez@codeaurora.org> writes:

> From: Ahmad Masri <amasri@codeaurora.org>
>
> New FW added support for upto 20 clients in AP mode. Change the driver
> to support this as well. FW reports it's max supported associations in
> WMI_READY_EVENT. Some WMI commands/events use cidxtid field which is
> limited to 16 cids. Use new cid/tid fields instead.
>
> For Rx packets cid from rx descriptor is limited to 3 bits (0..7),
> to find the real cid, compare transmitter address with the stored
> stations mac address in the driver sta array.
>
> EDMA FW still supports 8 stations. Extending the support to 20
> stations will come later.
>
> Signed-off-by: Ahmad Masri <amasri@codeaurora.org>
> Signed-off-by: Maya Erez <merez@codeaurora.org>

[...]


> --- a/drivers/net/wireless/ath/wil6210/wmi.c
> +++ b/drivers/net/wireless/ath/wil6210/wmi.c
> @@ -24,8 +24,9 @@
>  #include "wmi.h"
>  #include "trace.h"
>  
> -static uint max_assoc_sta = WIL6210_MAX_CID;
> -module_param(max_assoc_sta, uint, 0644);
> +/* set the default max assoc sta to max supported by driver */
> +uint max_assoc_sta = WIL6210_MAX_CID;
> +module_param(max_assoc_sta, uint, 0444);
>  MODULE_PARM_DESC(max_assoc_sta, " Max number of stations associated to the AP");
>  
>  int agg_wsize; /* = 0; */
> @@ -770,6 +771,7 @@ static void wmi_evt_ready(struct wil6210_vif *vif, int id, void *d, int len)
>  	struct wil6210_priv *wil = vif_to_wil(vif);
>  	struct wiphy *wiphy = wil_to_wiphy(wil);
>  	struct wmi_ready_event *evt = d;
> +	u8 fw_max_assoc_sta;
>  
>  	wil_info(wil, "FW ver. %s(SW %d); MAC %pM; %d MID's\n",
>  		 wil->fw_version, le32_to_cpu(evt->sw_version),
> @@ -787,6 +789,25 @@ static void wmi_evt_ready(struct wil6210_vif *vif, int id, void *d, int len)
>  			    evt->rfc_read_calib_result);
>  		wil->fw_calib_result = evt->rfc_read_calib_result;
>  	}
> +
> +	fw_max_assoc_sta = WIL6210_RX_DESC_MAX_CID;
> +	if (len > offsetof(struct wmi_ready_event, max_assoc_sta) &&
> +	    evt->max_assoc_sta > 0) {
> +		fw_max_assoc_sta = evt->max_assoc_sta;
> +		wil_dbg_wmi(wil, "fw reported max assoc sta %d\n",
> +			    fw_max_assoc_sta);
> +
> +		if (fw_max_assoc_sta > WIL6210_MAX_CID) {
> +			wil_dbg_wmi(wil,
> +				    "fw max assoc sta %d exceeds max driver supported %d\n",
> +				    fw_max_assoc_sta, WIL6210_MAX_CID);
> +			fw_max_assoc_sta = WIL6210_MAX_CID;
> +		}
> +	}
> +
> +	max_assoc_sta = min_t(uint, max_assoc_sta, fw_max_assoc_sta);

You shouldn't modify max_assoc_sta like this. If you have two wil6210
devices on the same host they will share that variable and there's a
race condition. Sure, I guess currently they will use the same firmware
so the values will be the same, but still this is a bug. I think you
should have wil->max_assoc_sta to make this device specific value.

I'll apply this anyway as it breaks all other patches if I remove this
patch. But please fix this in a follow up patch.

-- 
Kalle Valo