by Martin Kaistra

[permalink] [raw]

Subject: Re: [PATCH v2 19/21] wifi: rtl8xxxu: add hw crypto support for AP mode

Hi Ping-Ke,

Am 12.01.24 um 07:52 schrieb Ping-Ke Shih:
> Hi Martin,
>
>> -----Original Message-----
>> From: Martin Kaistra <[email protected]>
>> Sent: Friday, December 22, 2023 12:44 AM
>> To: [email protected]
>> Cc: Jes Sorensen <[email protected]>; Kalle Valo <[email protected]>; Ping-Ke Shih
>> <[email protected]>; Bitterblue Smith <[email protected]>; Sebastian Andrzej Siewior
>> <[email protected]>
>> 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/[email protected]/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/[email protected]/

>
>>
>> +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
>

2024-01-16 01:54:46

by Ping-Ke Shih

[permalink] [raw]

Subject: RE: [PATCH v2 19/21] wifi: rtl8xxxu: add hw crypto support for AP mode

> -----Original Message-----
> From: Martin Kaistra <[email protected]>
> Sent: Monday, January 15, 2024 9:13 PM
> To: Ping-Ke Shih <[email protected]>; [email protected]
> Cc: Jes Sorensen <[email protected]>; Kalle Valo <[email protected]>; Bitterblue Smith
> <[email protected]>; Sebastian Andrzej Siewior <[email protected]>; Zenm Chen
> <[email protected]>
> Subject: Re: [PATCH v2 19/21] wifi: rtl8xxxu: add hw crypto support for AP mode
>
> Hi Ping-Ke,
>
> Am 12.01.24 um 07:52 schrieb Ping-Ke Shih:
> > Hi Martin,
> >
> >> -----Original Message-----
> >> From: Martin Kaistra <[email protected]>
> >> Sent: Friday, December 22, 2023 12:44 AM
> >> To: [email protected]
> >> Cc: Jes Sorensen <[email protected]>; Kalle Valo <[email protected]>; Ping-Ke Shih
> >> <[email protected]>; Bitterblue Smith <[email protected]>; Sebastian Andrzej Siewior
> >> <[email protected]>
> >> 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/[email protected]/T/#me0940f522249becf4
> 9f25bc281f1992c523673f6
>
> 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?

I think you only need the one that fills fops->max_sec_cam_num.

The search rule of hardware security CAM is to find an entry that matches address
and key_idx, and then use it to encrypt/decrypt packets. If no entry found, it
select entry by key_idx (i.e. key_idx=0/1/2/3 use entry=0/1/2/3). As I can
remember, WEP needs to reserve 4 entries, but I believe no one uses it now.

>
> [1]
> https://lore.kernel.org/linux-wireless/[email protected]/
>
> >> 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.
>

Sorry, I misread the "rtlvif->..." and "key->..." that are different.