Received-SPF: pass (google.com: domain of linux-wireless+bounces-1045-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <f73b5afc-d69f-4a7c-8bf0-877a45327e0b@gmail.com>
Date: Wed, 20 Dec 2023 00:29:23 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
Subject: Re: [PATCH 18/20] wifi: rtl8xxxu: add hw crypto support for AP mode
To: Martin Kaistra <martin.kaistra@linutronix.de>,
 linux-wireless@vger.kernel.org
Cc: Jes Sorensen <Jes.Sorensen@gmail.com>, Kalle Valo <kvalo@kernel.org>,
 Ping-Ke Shih <pkshih@realtek.com>,
 Sebastian Andrzej Siewior <bigeasy@linutronix.de>
References: <20231218143645.433356-1-martin.kaistra@linutronix.de>
 <20231218143645.433356-19-martin.kaistra@linutronix.de>
Content-Language: en-US
In-Reply-To: <20231218143645.433356-19-martin.kaistra@linutronix.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 18/12/2023 16:36, Martin Kaistra wrote:
> Add a custom function for allocating entries in the sec cam. This allows
> us to store multiple keys with the same keyidx.
> 
> The maximum number of sec cam entries (for 8188f) is 16 according to the
> vendor driver.

The other chips seem to support more:

RTL8723BU, RTL8192EU, RTL8192FU: 64
RTL8188EU: 32
RTL8710BU: 32 (even though it supports only 16 macid)

I'm not sure about the RTL8723AU and RTL8192CU. Those drivers are
older and different.

Perhaps this should be a member of struct rtl8xxxu_fileops, like
max_macid_num?

> 
> Set the bssid as mac address for group keys instead of just using the
> ethernet broadcast address and use BIT(6) in the sec cam ctrl entry
> for differentiating them from pairwise keys like in the vendor driver.
> 
> Add the TXDESC_EN_DESC_ID bit and the hw_key_idx to tx
> broadcast/multicast packets in AP mode.
> 
> Finally, allow the usage of rtl8xxxu_set_key() for AP mode.
> 
> Signed-off-by: Martin Kaistra <martin.kaistra@linutronix.de>
> ---
>  .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |  4 ++
>  .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 48 +++++++++++++++----
>  2 files changed, 42 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> index 889ef7217f142..a0222d2666000 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> @@ -498,6 +498,7 @@ struct rtl8xxxu_txdesc40 {
>  #define DESC_RATE_ID_SHIFT		16
>  #define DESC_RATE_ID_MASK		0xf
>  #define TXDESC_NAVUSEHDR		BIT(20)
> +#define TXDESC_EN_DESC_ID		BIT(21)
>  #define TXDESC_SEC_RC4			0x00400000
>  #define TXDESC_SEC_AES			0x00c00000
>  #define TXDESC_PKT_OFFSET_SHIFT		26
> @@ -1774,6 +1775,7 @@ struct rtl8xxxu_cfo_tracking {
>  #define RTL8XXXU_HW_LED_CONTROL	2
>  #define RTL8XXXU_MAX_MAC_ID_NUM	128
>  #define RTL8XXXU_BC_MC_MACID	0
> +#define RTL8XXXU_MAX_SEC_CAM_NUM	16
>  
>  struct rtl8xxxu_priv {
>  	struct ieee80211_hw *hw;
> @@ -1907,6 +1909,7 @@ struct rtl8xxxu_priv {
>  	char led_name[32];
>  	struct led_classdev led_cdev;
>  	DECLARE_BITMAP(mac_id_map, RTL8XXXU_MAX_MAC_ID_NUM);
> +	DECLARE_BITMAP(cam_map, RTL8XXXU_MAX_SEC_CAM_NUM);
>  };
>  
>  struct rtl8xxxu_sta_info {
> @@ -1918,6 +1921,7 @@ struct rtl8xxxu_sta_info {
>  
>  struct rtl8xxxu_vif {
>  	int port_num;
> +	u8 hw_key_idx;
>  };
>  
>  struct rtl8xxxu_rx_urb {
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index c2a577ebd061e..88730791091a7 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -4558,8 +4558,10 @@ static void rtl8xxxu_cam_write(struct rtl8xxxu_priv *priv,
>  	 * This is a bit of a hack - the lower bits of the cipher
>  	 * suite selector happens to match the cipher index in the CAM
>  	 */
> -	addr = key->keyidx << CAM_CMD_KEY_SHIFT;
> +	addr = key->hw_key_idx << CAM_CMD_KEY_SHIFT;
>  	ctrl = (key->cipher & 0x0f) << 2 | key->keyidx | CAM_WRITE_VALID;
> +	if (!(key->flags & IEEE80211_KEY_FLAG_PAIRWISE))
> +		ctrl |= BIT(6);
>  
>  	for (j = 5; j >= 0; j--) {
>  		switch (j) {
> @@ -5545,13 +5547,14 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw,
>  	struct rtl8xxxu_tx_urb *tx_urb;
>  	struct ieee80211_sta *sta = NULL;
>  	struct ieee80211_vif *vif = tx_info->control.vif;
> +	struct rtl8xxxu_vif *rtlvif = (struct rtl8xxxu_vif *)vif->drv_priv;
>  	struct device *dev = &priv->udev->dev;
>  	u32 queue, rts_rate;
>  	u16 pktlen = skb->len;
>  	int tx_desc_size = priv->fops->tx_desc_size;
>  	u8 macid;
>  	int ret;
> -	bool ampdu_enable, sgi = false, short_preamble = false;
> +	bool ampdu_enable, sgi = false, short_preamble = false, bmc = false;
>  
>  	if (skb_headroom(skb) < tx_desc_size) {
>  		dev_warn(dev,
> @@ -5593,10 +5596,14 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw,
>  		tx_desc->txdw0 =
>  			TXDESC_OWN | TXDESC_FIRST_SEGMENT | TXDESC_LAST_SEGMENT;
>  	if (is_multicast_ether_addr(ieee80211_get_DA(hdr)) ||
> -	    is_broadcast_ether_addr(ieee80211_get_DA(hdr)))
> +	    is_broadcast_ether_addr(ieee80211_get_DA(hdr))) {
>  		tx_desc->txdw0 |= TXDESC_BROADMULTICAST;
> +		bmc = true;
> +	}
> +
>  
>  	tx_desc->txdw1 = cpu_to_le32(queue << TXDESC_QUEUE_SHIFT);
> +	macid = rtl8xxxu_get_macid(priv, sta);
>  
>  	if (tx_info->control.hw_key) {
>  		switch (tx_info->control.hw_key->cipher) {
> @@ -5611,6 +5618,10 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw,
>  		default:
>  			break;
>  		}
> +		if (bmc && rtlvif->hw_key_idx != 0xff) {
> +			tx_desc->txdw1 |= TXDESC_EN_DESC_ID;
> +			macid = rtlvif->hw_key_idx;
> +		}

cpu_to_le32(TXDESC_EN_DESC_ID)

The vendor drivers only do this for data frames. Is it the same here?

>  	}
>  
>  	/* (tx_info->flags & IEEE80211_TX_CTL_AMPDU) && */
> @@ -5654,7 +5665,6 @@ static void rtl8xxxu_tx(struct ieee80211_hw *hw,
>  	else
>  		rts_rate = 0;
>  
> -	macid = rtl8xxxu_get_macid(priv, sta);
>  	priv->fops->fill_txdesc(hw, hdr, tx_info, tx_desc, sgi, short_preamble,
>  				ampdu_enable, rts_rate, macid);
>  
> @@ -6656,6 +6666,7 @@ static int rtl8xxxu_add_interface(struct ieee80211_hw *hw,
>  
>  	priv->vifs[port_num] = vif;
>  	rtlvif->port_num = port_num;
> +	rtlvif->hw_key_idx = 0xff;
>  
>  	rtl8xxxu_set_linktype(priv, vif->type, port_num);
>  	ether_addr_copy(priv->mac_addr, vif->addr);
> @@ -6832,11 +6843,19 @@ static int rtl8xxxu_set_rts_threshold(struct ieee80211_hw *hw, u32 rts)
>  	return 0;
>  }
>  
> +static int rtl8xxxu_get_free_sec_cam(struct ieee80211_hw *hw)
> +{
> +	struct rtl8xxxu_priv *priv = hw->priv;
> +
> +	return find_first_zero_bit(priv->cam_map, RTL8XXXU_MAX_SEC_CAM_NUM);
> +}
> +
>  static int rtl8xxxu_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
>  			    struct ieee80211_vif *vif,
>  			    struct ieee80211_sta *sta,
>  			    struct ieee80211_key_conf *key)
>  {
> +	struct rtl8xxxu_vif *rtlvif = (struct rtl8xxxu_vif *)vif->drv_priv;
>  	struct rtl8xxxu_priv *priv = hw->priv;
>  	struct device *dev = &priv->udev->dev;
>  	u8 mac_addr[ETH_ALEN];
> @@ -6848,9 +6867,6 @@ static int rtl8xxxu_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
>  	dev_dbg(dev, "%s: cmd %02x, cipher %08x, index %i\n",
>  		__func__, cmd, key->cipher, key->keyidx);
>  
> -	if (vif->type != NL80211_IFTYPE_STATION)
> -		return -EOPNOTSUPP;
> -
>  	if (key->keyidx > 3)
>  		return -EOPNOTSUPP;
>  
> @@ -6874,7 +6890,7 @@ static int rtl8xxxu_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
>  		ether_addr_copy(mac_addr, sta->addr);
>  	} else {
>  		dev_dbg(dev, "%s: group key\n", __func__);
> -		eth_broadcast_addr(mac_addr);
> +		ether_addr_copy(mac_addr, vif->bss_conf.bssid);
>  	}
>  
>  	val16 = rtl8xxxu_read16(priv, REG_CR);
> @@ -6888,16 +6904,28 @@ static int rtl8xxxu_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
>  
>  	switch (cmd) {
>  	case SET_KEY:
> -		key->hw_key_idx = key->keyidx;
> +
> +		retval = rtl8xxxu_get_free_sec_cam(hw);
> +		if (retval < 0)
> +			return -EOPNOTSUPP;
> +
> +		key->hw_key_idx = retval;
> +
> +		if (vif->type == NL80211_IFTYPE_AP && !(key->flags & IEEE80211_KEY_FLAG_PAIRWISE))
> +			rtlvif->hw_key_idx = key->hw_key_idx;
> +
>  		key->flags |= IEEE80211_KEY_FLAG_GENERATE_IV;
>  		rtl8xxxu_cam_write(priv, key, mac_addr);
> +		set_bit(key->hw_key_idx, priv->cam_map);
>  		retval = 0;
>  		break;
>  	case DISABLE_KEY:
>  		rtl8xxxu_write32(priv, REG_CAM_WRITE, 0x00000000);
>  		val32 = CAM_CMD_POLLING | CAM_CMD_WRITE |
> -			key->keyidx << CAM_CMD_KEY_SHIFT;
> +			key->hw_key_idx << CAM_CMD_KEY_SHIFT;
>  		rtl8xxxu_write32(priv, REG_CAM_CMD, val32);
> +		rtlvif->hw_key_idx = 0xff;
> +		clear_bit(key->hw_key_idx, priv->cam_map);
>  		retval = 0;
>  		break;
>  	default: