Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org C754560E73
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Peter Oh <peter.oh@bowerswilkins.com>
Cc:     Rakesh Pillai <pillair@codeaurora.org>,
        "ath10k\@lists.infradead.org" <ath10k@lists.infradead.org>,
        "linux-wireless\@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH v2] ath10k: Report low ack rssi based on the reason code
References: <1554276181-7879-1-git-send-email-pillair@codeaurora.org>
        <5f25f471-5753-7d4a-91f8-9a3986117580@bowerswilkins.com>
Date:   Thu, 03 Oct 2019 16:03:33 +0300
In-Reply-To: <5f25f471-5753-7d4a-91f8-9a3986117580@bowerswilkins.com> (Peter
        Oh's message of "Wed, 3 Apr 2019 16:58:51 +0000")
Message-ID: <878sq20ymy.fsf@kamboji.qca.qualcomm.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Peter Oh <peter.oh@bowerswilkins.com> writes:

> On 04/03/2019 12:23 AM, Rakesh Pillai wrote:
>> Firmware sends peer sta kickout event to the driver
>> along with the reason code for a particular peer.
>>
>> Currently the sta kickout event is delivered to the
>> upper layer without checking if the reason code is
>> valid or not. This causes frequent disconnection of
>> the STA.
>>
>> Report low ack rssi event to mac80211 only if the reason
>> code is valid.
>>
>> Tested HW: WCN3990
>> Tested FW: WLAN.HL.2.0-01188-QCAHLSWMTPLZ-1
>>
>> Signed-off-by: Rakesh Pillai <pillair@codeaurora.org>

[...]

>> @@ -3440,6 +3437,12 @@ void ath10k_wmi_event_peer_sta_kickout(struct ath10k *ar, struct sk_buff *skb)
>>   		goto exit;
>>   	}
>>   
>> +	if (arg.reason_code_valid &&
>> +	    arg.reason == WMI_PEER_STA_KICKOUT_REASON_UNSPECIFIED)
>> +		goto exit;
>> +
>
> Why do we want this event not to be delivered to user space?

Yeah, I'm curious about that as well. If I'm guessing right,
WMI_PEER_STA_KICKOUT_REASON_UNSPECIFIED is supposed to mean that the
firmware does not support providing the reason code. Usually, but not
always, in the firmware interface value zero means unsupported. So why
would we want to ignore a kickout event which has a valid mac address
for the peer?

In what kind of cases is the firmware emitting these events? Is this
really the correct thing to do?

>> +	ath10k_dbg(ar, ATH10K_DBG_WMI, "wmi event peer sta kickout %pM reason code %d\n",
>> +		   arg.mac_addr, arg.reason);
>>   	ieee80211_report_low_ack(sta, 10);
>>   
>>   exit:
>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
>> index e1c40bb..3ccd79e 100644
>> --- a/drivers/net/wireless/ath/ath10k/wmi.h
>> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
>> @@ -6797,6 +6797,8 @@ struct wmi_vdev_start_ev_arg {
>>   
>>   struct wmi_peer_kick_ev_arg {
>>   	const u8 *mac_addr;
>> +	u32 reason;
>> +	bool reason_code_valid;
>>   };
>
> Adding extra members to this structure breaks structure consistency 
> between FW and host driver since FW doesn't have such members.

Yeah, this reason_code_valid boolean is set if WMI-TLV is used, but it
does not still mean that the reason code is valid. (There might be
WMI-TLV firmwares which do not provide the reason code.)

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches