Received-SPF: pass (google.com: domain of linux-wireless+bounces-1924-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <87e04cfe-f8ed-40da-bb2c-edccfe385b6e@linutronix.de>
Date: Mon, 15 Jan 2024 14:12:39 +0100
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH v2 19/21] wifi: rtl8xxxu: add hw crypto support for AP
 mode
Content-Language: de-DE
To: Ping-Ke Shih <pkshih@realtek.com>,
 "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc: Jes Sorensen <Jes.Sorensen@gmail.com>, Kalle Valo <kvalo@kernel.org>,
 Bitterblue Smith <rtl8821cerfe2@gmail.com>,
 Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
 Zenm Chen <zenmchen@gmail.com>
References: <20231221164353.603258-1-martin.kaistra@linutronix.de>
 <20231221164353.603258-20-martin.kaistra@linutronix.de>
 <92fa5949e8094739883e665abb23ac01@realtek.com>
From: Martin Kaistra <martin.kaistra@linutronix.de>
In-Reply-To: <92fa5949e8094739883e665abb23ac01@realtek.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Hi Ping-Ke,

Am 12.01.24 um 07:52 schrieb Ping-Ke Shih:
> Hi Martin,
> 
>> -----Original Message-----
>> From: Martin Kaistra <martin.kaistra@linutronix.de>
>> Sent: Friday, December 22, 2023 12:44 AM
>> To: linux-wireless@vger.kernel.org
>> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>; Kalle Valo <kvalo@kernel.org>; Ping-Ke Shih
>> <pkshih@realtek.com>; Bitterblue Smith <rtl8821cerfe2@gmail.com>; Sebastian Andrzej Siewior
>> <bigeasy@linutronix.de>
>> Subject: [PATCH v2 19/21] wifi: rtl8xxxu: add hw crypto support for AP mode
>>
> 
> [...]
> 
> Zenm reported [1] his RTL8192EU and RTL8192FU don't work in station mode,
> and cause is this patch. Please try if you can reproduce the symptom, and
> apply my suggestion to see if help.
> 
> [1] https://lore.kernel.org/linux-wireless/20240112045104.12282-1-zenmchen@gmail.com/T/#me0940f522249becf49f25bc281f1992c523673f6

I managed to find two other Realtek USB Wifi devices that are supported by the 
rtl8xxxu driver (RTL8188EU and RTL8192CU) and I can reproduce the issue with 
both of them.

I also tried creating a patch with your suggestions and this seems to help.

Looking at it more closely however, I think the main problem is, that 
fops->max_sec_cam_num is not set for the other variants. Without the additional 
patch, this causes rtl8xxxu_get_free_sec_cam() to return 0 for pairwise and 
group key and so using the same spot for both key entries.

I then created a patch using the numbers suggested by Bitterblue Smith in [1] 
and using 32 for RTL8723AU and RTL8192CU like the rtlwifi driver seems to do. 
This also seems to solve the issue reported, even without reserving the first 4 
slots for group keys.

Do you think we need both patches?

[1] 
https://lore.kernel.org/linux-wireless/f73b5afc-d69f-4a7c-8bf0-877a45327e0b@gmail.com/

> 
>>
>> +static int rtl8xxxu_get_free_sec_cam(struct ieee80211_hw *hw)
>> +{
>> +       struct rtl8xxxu_priv *priv = hw->priv;
> 
> We need to reserve entries 0~3 for keys that aren't pairwise key.
> 
>> +
>> +       return find_first_zero_bit(priv->cam_map, priv->fops->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];
> 
> [...]
> 
>> @@ -6899,16 +6915,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;
>> +
> 
> if (key->flags & IEEE80211_KEY_FLAG_PAIRWISE)
> 	key->hw_key_idx = retval;
> else
> 	key->hw_key_idx = key->keyidx;
> 
>> +               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);
> 
> Shouldn't swap these two statements? I missed that during reviewing.

I don't think that would make a difference. rtlvif->hw_key_idx is set for use in 
rtl8xxxu_tx() and the second line uses key->hw_key_idx to clear the map entry.

> 
> 
>>                  retval = 0;
>>                  break;
>>          default:
>> --
>> 2.39.2
>