Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: kvalo=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.4.1 smtp.codeaurora.org 1FB95C43460
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Ping-Ke Shih <pkshih@realtek.com>
Cc:     <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH v6 01/24] rtw89: add CAM files
References: <20210820043538.12424-1-pkshih@realtek.com>
        <20210820043538.12424-2-pkshih@realtek.com>
Date:   Fri, 01 Oct 2021 17:46:19 +0300
In-Reply-To: <20210820043538.12424-2-pkshih@realtek.com> (Ping-Ke Shih's
        message of "Fri, 20 Aug 2021 12:35:15 +0800")
Message-ID: <871r54kdl0.fsf@codeaurora.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Ping-Ke Shih <pkshih@realtek.com> writes:

> Three kinds of CAM, security, address and bssid, are implemented in the
> chip, and then we control them via H2C commands.
>
> Every vif contains one address and one bssid CAM entries, and both are
> allocated while vif is adding. Once any state is changed, driver notices
> firmware. For example, add/remove vif, sta assoc/disassoc, and changed
> security key.
>
> When connecting to an AP with security, a security CAM entry is allocated
> from security CAM pool and filled key values, and then attached to the
> address CAM that contains seven keys entries for unicast, group and BIP
> keys. Since the functions of these seven keys are predefined by firmware,
> we should attach key to proper position depends on security entry mode and
> key type.
>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>

[...]

> +int rtw89_cam_sec_key_add(struct rtw89_dev *rtwdev,
> +			  struct ieee80211_vif *vif,
> +			  struct ieee80211_sta *sta,
> +			  struct ieee80211_key_conf *key)
> +{
> +	u8 hw_key_type;
> +	bool ext_key = false;
> +	int ret;
> +
> +	switch (key->cipher) {
> +	case WLAN_CIPHER_SUITE_WEP40:
> +		hw_key_type = RTW89_SEC_KEY_TYPE_WEP40;
> +		break;
> +	case WLAN_CIPHER_SUITE_WEP104:
> +		hw_key_type = RTW89_SEC_KEY_TYPE_WEP104;
> +		break;
> +	case WLAN_CIPHER_SUITE_CCMP:
> +		hw_key_type = RTW89_SEC_KEY_TYPE_CCMP128;
> +		key->flags |= IEEE80211_KEY_FLAG_SW_MGMT_TX;
> +		break;
> +	case WLAN_CIPHER_SUITE_CCMP_256:
> +		hw_key_type = RTW89_SEC_KEY_TYPE_CCMP256;
> +		key->flags |= IEEE80211_KEY_FLAG_SW_MGMT_TX;
> +		ext_key = true;
> +		break;
> +	case WLAN_CIPHER_SUITE_GCMP:
> +		hw_key_type = RTW89_SEC_KEY_TYPE_GCMP128;
> +		key->flags |= IEEE80211_KEY_FLAG_SW_MGMT_TX;
> +		break;
> +	case WLAN_CIPHER_SUITE_GCMP_256:
> +		hw_key_type = RTW89_SEC_KEY_TYPE_GCMP256;
> +		key->flags |= IEEE80211_KEY_FLAG_SW_MGMT_TX;
> +		ext_key = true;
> +		break;
> +	case WLAN_CIPHER_SUITE_TKIP:
> +	case WLAN_CIPHER_SUITE_AES_CMAC:
> +	case WLAN_CIPHER_SUITE_BIP_CMAC_256:
> +	case WLAN_CIPHER_SUITE_BIP_GMAC_128:
> +	case WLAN_CIPHER_SUITE_BIP_GMAC_256:
> +		/* suppress error messages */

What error messages? I don't understand the comment.

> +		return -EOPNOTSUPP;
> +	default:
> +		return -ENOTSUPP;

I would merge default with other cases above and have just one return
call.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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