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

[Ping-Ke Shih <pkshih@realtek.com>]

Martin Kaistra <martin.kaistra@linutronix.de> wrote:
> Am 21.02.26 um 23:09 schrieb Bitterblue Smith:
> > On 22/12/2023 12:14, 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. Add the number to rtl8xxxu_fileops, so that other chips
> >> which might support more entries, can set a different number there.
> >>
> >> 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>
> >> Reviewed-by: Ping-Ke Shih <pkshih@realtek.com>
> >> ---
> >>   .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h  |  5 ++
> >>   .../realtek/rtl8xxxu/rtl8xxxu_8188f.c         |  1 +
> >>   .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 48 +++++++++++++++----
> >>   3 files changed, 44 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> >> index c5e6d8f7d26bd..62e6318bc0924 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
> >> @@ -1775,6 +1776,7 @@ struct rtl8xxxu_cfo_tracking {
> >>   #define RTL8XXXU_MAX_MAC_ID_NUM    128
> >>   #define RTL8XXXU_BC_MC_MACID       0
> >>   #define RTL8XXXU_BC_MC_MACID1      1
> >> +#define RTL8XXXU_MAX_SEC_CAM_NUM    64
> >>
> >>   struct rtl8xxxu_priv {
> >>      struct ieee80211_hw *hw;
> >> @@ -1908,6 +1910,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 {
> >> @@ -1919,6 +1922,7 @@ struct rtl8xxxu_sta_info {
> >>
> >>   struct rtl8xxxu_vif {
> >>      int port_num;
> >> +    u8 hw_key_idx;
> >>   };
> >>
> >>   struct rtl8xxxu_rx_urb {
> >> @@ -1993,6 +1997,7 @@ struct rtl8xxxu_fileops {
> >>      u16 max_aggr_num;
> >>      u8 supports_ap:1;
> >>      u16 max_macid_num;
> >> +    u16 max_sec_cam_num;
> >>      u32 adda_1t_init;
> >>      u32 adda_1t_path_on;
> >>      u32 adda_2t_path_on_a;
> >> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8188f.c
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8188f.c
> >> index 1e1c8fa194cb8..574a5fe951543 100644
> >> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8188f.c
> >> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8188f.c
> >> @@ -1751,6 +1751,7 @@ struct rtl8xxxu_fileops rtl8188fu_fops = {
> >>      .max_aggr_num = 0x0c14,
> >>      .supports_ap = 1,
> >>      .max_macid_num = 16,
> >> +    .max_sec_cam_num = 16,
> >>      .adda_1t_init = 0x03c00014,
> >>      .adda_1t_path_on = 0x03c00014,
> >>      .trxff_boundary = 0x3f7f,
> >> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> >> index ecf54eb8dba61..7aafae9fe76b8 100644
> >> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> >> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> >> @@ -4559,8 +4559,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) {
> >> @@ -5546,13 +5548,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,
> >> @@ -5594,10 +5597,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) {
> >> @@ -5612,6 +5619,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;
> >> +            }
> >>      }
> >>
> >>      /* (tx_info->flags & IEEE80211_TX_CTL_AMPDU) && */
> >> @@ -5655,7 +5666,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);
> >>
> >> @@ -6667,6 +6677,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);
> >> @@ -6843,11 +6854,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, 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];
> >> @@ -6859,9 +6878,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;
> >>
> >> @@ -6885,7 +6901,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);
> >> @@ -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;
> >> +
> >> +            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);
> >
> > Hi Martin,
> 
> Hi Bitterblue,
> 
> >
> > It turns out RTL8188CUS and RTL8192CU don't like this patch, specifically
> > when we use iwd. After the WPA2 handshake no more data is transmitted.
> >
> > Before this patch, key->hw_key_idx was the same as key->keyidx. After
> > this patch, when we use iwd, the group key is installed first. It has
> > key->keyidx = 1, but it gets key->hw_key_idx = 0. The pairwise key is
> > installed second. It has key->keyidx = 0, but it gets key->hw_key_idx = 1.
> > Both keyidx and hw_key_idx are passed to the chip in rtl8xxxu_cam_write().
> >
> > It's fine with wpa_supplicant. wpa_supplicant installs the pairwise key
> > first, with key->keyidx = 0, then the group key, with key->keyidx = 1.
> >
> > This patch imitating rtw88 makes the old chips work again with iwd.
> > What do you think?
> 
> So you reserve the first 4 entries for group keys and use key->keyidx as
> key->hw_key_idx directly for those, right? Does that work if 2 virtual
> interfaces are used at the same time?
> I will do some tests in the next days, but I suspect this be an issue.

Ah. I'm not aware that rtl8xxxu can support 2 virtual interfaces, and
only RTL8188FU declares .supports_concurrent = 1, so maybe we can have
special handling for this chip?

I'd share some information about security CAM for reference. Since data is
quite old (10+ years), I can't guarantee all are correct.

1. default key switch in rtl8xxxu_set_key()
   SEC_CFG_TXBC_USE_DEFKEY and SEC_CFG_RXBC_USE_DEFKEY mean that
   broadcast/multicast packets use default key (CAM entries 0~3 for
   keyidx 0~3).
   For two interfaces case, that'd a problem, so maybe we should use
   software encryption/decryption for non-pairwise key.

	val8 = SEC_CFG_TX_SEC_ENABLE | SEC_CFG_TXBC_USE_DEFKEY |
		SEC_CFG_RX_SEC_ENABLE | SEC_CFG_RXBC_USE_DEFKEY;
	val8 |= SEC_CFG_TX_USE_DEFKEY | SEC_CFG_RX_USE_DEFKEY;
	rtl8xxxu_write8(priv, REG_SECURITY_CFG, val8);

2. group key (GK) field of security CAM in rtl8xxxu_cam_write()
   The group key field of security CAM is BIT(6) which isn't supported
   by earlier chips (sorry I have no data about the exact chips).

   If a chip can support group key field, the CAM layout with default key
   enabled for two interfaces (STA-1+AP-1) can be

   STA-1 <-> AP-10
   AP-1  <-> STA-20

   CAM  GK   MAC    KEY
    0
    1   1    00:    AP-1 (GTK)  // should be on entry 0~3 depends on key_idx
    2
    3
    4
    5   1    AP-10  AP-10 (GTK)  // can be on any entry after 4
    6

    (pairwise key can be on any entry after 4)

	if (!(key->flags & IEEE80211_KEY_FLAG_PAIRWISE))
		ctrl |= BIT(6);

My perspective are
1. currently default key is enabled, so we should treat CAM entry 0~3 as
   special cases. That means rtl8xxxu_get_free_sec_cam() should be modified
   as Bitterblue's version.

2. For two interfaces, I guess RTL8188FU can support GK bit, so it'd be worth
   to try the method of example 2 above.

   If we want earlier chips can support two interfaces, I think we should 
   disable default key. Seemingly, it is not possible that using hardware
   crypto when operating one interface and then switching to use software
   crypto when operating two interfaces. That means broadcast/multicast
   packets should be by software crypto entirely if we want to support two
   interfaces.

3. I think rtw88 has similar problem for two interfaces.

> 
> >
> > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/core.c
> b/drivers/net/wireless/realtek/rtl8xxxu/core.c
> > index ee278f0548e4..f7b35655bec5 100644
> > --- a/drivers/net/wireless/realtek/rtl8xxxu/core.c
> > +++ b/drivers/net/wireless/realtek/rtl8xxxu/core.c
> > @@ -6942,7 +6942,8 @@ 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, priv->fops->max_sec_cam_num);
> > +     return find_next_zero_bit(priv->cam_map, priv->fops->max_sec_cam_num,
> > +                               RTL8XXXU_SEC_DEFAULT_KEY_NUM);
> >   }
> >
> >   static int rtl8xxxu_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> > @@ -6999,12 +7000,15 @@ static int rtl8xxxu_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> >
> >       switch (cmd) {
> >       case SET_KEY:
> > +             if (key->flags & IEEE80211_KEY_FLAG_PAIRWISE) {
> > +                     retval = rtl8xxxu_get_free_sec_cam(hw);
> > +                     if (retval < 0)
> > +                             return -EOPNOTSUPP;
> >
> > -             retval = rtl8xxxu_get_free_sec_cam(hw);
> > -             if (retval < 0)
> > -                     return -EOPNOTSUPP;
> > -
> > -             key->hw_key_idx = retval;
> > +                     key->hw_key_idx = retval;
> > +             } else {
> > +                     key->hw_key_idx = key->keyidx;
> > +             }
> >
> >               if (vif->type == NL80211_IFTYPE_AP && !(key->flags & IEEE80211_KEY_FLAG_PAIRWISE))
> >                       rtlvif->hw_key_idx = key->hw_key_idx;
> > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> > index 4b05dba22e67..188f4bbe99cd 100644
> > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> > @@ -1788,6 +1788,7 @@ struct rtl8xxxu_cfo_tracking {
> >   #define RTL8XXXU_BC_MC_MACID        0
> >   #define RTL8XXXU_BC_MC_MACID1       1
> >   #define RTL8XXXU_MAX_SEC_CAM_NUM    64
> > +#define RTL8XXXU_SEC_DEFAULT_KEY_NUM 4
> >
> >   struct rtl8xxxu_priv {
> >       struct ieee80211_hw *hw;