Subject: Re: [PATCH 2/6] brcmfmac: send port authorized event for 802.1X 4-way
 handshake offload
To:     Chi-Hsien Lin <Chi-Hsien.Lin@cypress.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc:     "brcm80211-dev-list@broadcom.com" <brcm80211-dev-list@broadcom.com>,
        brcm80211-dev-list <brcm80211-dev-list@cypress.com>,
        Franky Lin <franky.lin@broadcom.com>,
        Hante Meuleman <hante.meuleman@broadcom.com>,
        Wright Feng <Wright.Feng@cypress.com>,
        Kalle Valo <kvalo@codeaurora.org>,
        Stanley Hsu <Stanley.Hsu@cypress.com>
References: <1546582221-143220-1-git-send-email-chi-hsien.lin@cypress.com>
 <1546582221-143220-2-git-send-email-chi-hsien.lin@cypress.com>
From:   Arend Van Spriel <arend.vanspriel@broadcom.com>
Message-ID: <d89b1304-471d-f064-02f1-6a9fab0ca25d@broadcom.com>
Date:   Mon, 7 Jan 2019 10:44:01 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.4.0
MIME-Version: 1.0
In-Reply-To: <1546582221-143220-2-git-send-email-chi-hsien.lin@cypress.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On 1/4/2019 7:11 AM, Chi-Hsien Lin wrote:
> From: Chung-Hsien Hsu <stanley.hsu@cypress.com>
> 
> With 4-way handshake offload for 802.1X authentication, a port
> authorized event should be sent to user space after the completion of
> 4-way handshake. It is used to indicate that a connection is authorized
> and 802.1X authentication is no longer required.

It had been a while since I had looked at our offload code (basically 
since the initial implementation for the nl80211 work) so I was unsure 
why this would be needed.

So initially we added a PORT_AUTHORIZED *attribute* in the nl80211 api 
and later on the PORT_AUTHORIZED *event* was introduced and 4-way hs 
offload support in wpa_supplicant is ignoring the *attribute* and only 
handling the *event*. I think this information is important enough to 
add to this commit message with a reference to commit 503c1fb98ba3 
("cfg80211/nl80211: add a port authorized event") which "broke" the 
functionality in brcmfmac.

Some specific comments below...

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Chung-Hsien Hsu <stanley.hsu@cypress.com>
> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
> ---
>   .../broadcom/brcm80211/brcmfmac/cfg80211.c         | 23 +++++++++++++++-------
>   1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 35301237d435..ad0d775a1244 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -5266,10 +5266,13 @@ static bool brcmf_is_linkup(struct brcmf_cfg80211_vif *vif,
>   	u32 event = e->event_code;
>   	u32 status = e->status;
>   
> -	if (vif->profile.use_fwsup == BRCMF_PROFILE_FWSUP_PSK &&
> -	    event == BRCMF_E_PSK_SUP &&
> -	    status == BRCMF_E_STATUS_FWSUP_COMPLETED)
> +	if (event == BRCMF_E_PSK_SUP &&
> +	    status == BRCMF_E_STATUS_FWSUP_COMPLETED) {
>   		set_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &vif->sme_state);
> +		if (vif->profile.use_fwsup == BRCMF_PROFILE_FWSUP_1X)
> +			return true;
> +	}
> +

Here things get a bit tricky I think. The old behaviour was to wait for 
both PSK_SUP and SET_SSID events before calling cfg80211_connect_done(). 
If I recall correctly I did that because they can arrive in different 
order depending on the type of offload (1x or psk) but also depends on 
firmware build, so ....

>   	if (event == BRCMF_E_SET_SSID && status == BRCMF_E_STATUS_SUCCESS) {
>   		brcmf_dbg(CONN, "Processing set ssid\n");
>   		memcpy(vif->profile.bssid, e->addr, ETH_ALEN);
> @@ -5280,11 +5283,10 @@ static bool brcmf_is_linkup(struct brcmf_cfg80211_vif *vif,
>   	}
>   
>   	if (test_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &vif->sme_state) &&
> -	    test_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS, &vif->sme_state)) {
> -		clear_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &vif->sme_state);
> -		clear_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS, &vif->sme_state);
> +	    test_and_clear_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS,
> +			       &vif->sme_state))
>   		return true;
> -	}
> +
>   	return false;
>   }
>   
> @@ -5501,6 +5503,13 @@ brcmf_bss_connect_done(struct brcmf_cfg80211_info *cfg,
>   		brcmf_dbg(CONN, "Report connect result - connection %s\n",
>   			  completed ? "succeeded" : "failed");
>   	}
> +
> +	if (test_and_clear_bit(BRCMF_VIF_STATUS_EAP_SUCCESS,
> +			       &ifp->vif->sme_state) &&
> +	    profile->use_fwsup == BRCMF_PROFILE_FWSUP_1X) {
> +		cfg80211_port_authorized(ndev, profile->bssid, GFP_KERNEL);
> +		brcmf_dbg(CONN, "Report port authorized\n");
> +	}

I would leave the code in brcmf_is_linkup() as before and only check 
profile->use_fwsup here to determine whether cfg80211_port_authorized() 
should be called here.

>   	brcmf_dbg(TRACE, "Exit\n");
>   	return 0;
>   }
>