DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 344E120862
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Tue, 06 Nov 2018 14:38:22 +0200
From:   merez@codeaurora.org
To:     Kalle Valo <kvalo@codeaurora.org>
Cc:     Ahmad Masri <amasri@codeaurora.org>,
        linux-wireless@vger.kernel.org, wil6210@qti.qualcomm.com,
        linux-wireless-owner@vger.kernel.org
Subject: Re: [PATCH 07/16] wil6210: refactor disconnect flow
In-Reply-To: <874lcue6ft.fsf@codeaurora.org>
References: <1540975944-30576-1-git-send-email-merez@codeaurora.org>
 <1540975944-30576-8-git-send-email-merez@codeaurora.org>
 <871s7ybir0.fsf@purkki.adurom.net>
 <6852209b6e110737dc189ec89a49078f@codeaurora.org>
 <874lcue6ft.fsf@codeaurora.org>
Message-ID: <98ff59749bbf386ba92faec2203ac3e9@codeaurora.org>
User-Agent: Roundcube Webmail/1.2.5
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On 2018-11-06 14:28, Kalle Valo wrote:
> merez@codeaurora.org writes:
> 
>> On 2018-11-06 12:30, Kalle Valo wrote:
>>> Maya Erez <merez@codeaurora.org> writes:
>>> 
>>>> From: Ahmad Masri <amasri@codeaurora.org>
>>>> 
>>>> Separate sending command to the fw from the event handling function 
>>>> to
>>>> simplify the disconnect flow and track the from_event flag 
>>>> correctly.
>>>> 
>>>> Signed-off-by: Ahmad Masri <amasri@codeaurora.org>
>>>> Signed-off-by: Maya Erez <merez@codeaurora.org>
>>> 
>>> [...]
>>> 
>>>> +static int wil_disconnect_cid(struct wil6210_vif *vif, int cid,
>>>> +			      u16 reason_code)
>>>> +__acquires(&sta->tid_rx_lock) __releases(&sta->tid_rx_lock)
>>>> +{
>>>> +	struct wil6210_priv *wil = vif_to_wil(vif);
>>>> +	struct wireless_dev *wdev = vif_to_wdev(vif);
>>>> +	struct wil_sta_info *sta = &wil->sta[cid];
>>>> +	bool del_sta = false;
>>>> +
>>>> +	might_sleep();
>>>> +	wil_dbg_misc(wil, "disconnect_cid: CID %d, MID %d, status %d\n",
>>>> +		     cid, sta->mid, sta->status);
>>>> +
>>>> +	if (sta->status == wil_sta_unused)
>>>> +		return 0;
>>>> +
>>>> +	if (vif->mid != sta->mid) {
>>>> +		wil_err(wil, "STA MID mismatch with VIF MID(%d)\n", vif->mid);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	/* inform lower layers */
>>>> +	if (wdev->iftype == NL80211_IFTYPE_AP && disable_ap_sme)
>>>> +		del_sta = true;
>>>> +
>>>> +	/* disconnect by sending command disconnect/del_sta and wait
>>>> +	 * synchronously for WMI_DISCONNECT_EVENTID event.
>>>> +	 */
>>>> +	return wmi_disconnect_sta(vif, sta->addr, reason_code, del_sta);
>>>> +}
>>> 
>>> I don't get use of __acquires() and __releases() in this function. I
>>> see
>>> similar pattern already in wil6210 but care to explain why this is
>>> needed? I don't see the function even accessing tid_rx_lock so I'm 
>>> very
>>> confused.
>> 
>> I assume it is a copy / paste leftover that we missed in the code
>> review. We will remove it.
> 
> Actually I already removed the annotations from the pending branch and
> no need to resend, it's faster that way. Please double check if you 
> can,
> unfortunately I cannot provide a direct link cgit doesn't show the new
> commit yet.

In such a case you can go ahead and apply the patches without
"wil6210: ignore HALP ICR if already handled". I'll upstream its fixed 
version
in the next set of wil6210 patches.

-- 
Maya Erez
Qualcomm Israel, Inc. on behalf of Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project