Re: [PATCH v6] wifi: mt76: mt7915: add wds support when wed is enabled

From: Shengyu Qu
Date: Sun Sep 08 2024 - 12:27:50 EST

diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/main.c b/drivers/net/wireless/mediatek/mt76/mt7915/main.c
index 049223df9beb1..dc4d87e004a0f 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7915/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7915/main.c
@@ -745,8 +745,15 @@ int mt7915_mac_sta_add(struct mt76_dev *mdev, struct ieee80211_vif *vif,
      bool ext_phy = mvif->phy != &dev->phy;
      int ret, idx;
      u32 addr;
+    u8 flags = MT76_WED_DEFAULT;
-    idx = mt76_wcid_alloc(dev->mt76.wcid_mask, MT7915_WTBL_STA);
+    if (mtk_wed_device_active(&dev->mt76.mmio.wed) &&
+        !is_mt7915(&dev->mt76)) {
+        flags = test_bit(MT_WCID_FLAG_4ADDR, &msta->wcid.flags) ?
+               MT76_WED_WDS_ACTIVE : MT76_WED_ACTIVE;
+    }
+
+    idx = __mt76_wcid_alloc(mdev->wcid_mask, MT7915_WTBL_STA, flags);
      if (idx < 0)
          return -ENOSPC;

I'd prefer to replace the mt76_wcid_alloc flags argument with an explicit start offset argument.

Hi Felix,

I'm not really familiar with mt76 code, just a enthusiast that wants to
make WDS+WED working. So I'm not sure how to do this correctly... Maybe
it's better for you to take over this patch, sorry.

@@ -1201,12 +1208,27 @@ static void mt7915_sta_set_4addr(struct ieee80211_hw *hw,
  {
      struct mt7915_dev *dev = mt7915_hw_dev(hw);
      struct mt7915_sta *msta = (struct mt7915_sta *)sta->drv_priv;
+    int min = MT76_WED_WDS_MIN, max = MT76_WED_WDS_MAX;
      if (enabled)
          set_bit(MT_WCID_FLAG_4ADDR, &msta->wcid.flags);
      else
          clear_bit(MT_WCID_FLAG_4ADDR, &msta->wcid.flags);
+    if (mtk_wed_device_active(&dev->mt76.mmio.wed) &&
+        !is_mt7915(&dev->mt76) &&
+        (msta->wcid.idx < min || msta->wcid.idx > max - 1)) {
+        struct ieee80211_sta *pre_sta;
+
+        pre_sta = kzalloc(sizeof(*sta) + sizeof(*msta), GFP_KERNEL);
+        mt76_sta_pre_rcu_remove(hw, vif, sta);
+        memmove(pre_sta, sta, sizeof(*sta) + sizeof(*msta));
+        mt7915_sta_add(hw, vif, sta);
+        synchronize_rcu();
+        mt7915_sta_remove(hw, vif, pre_sta);
+        kfree(pre_sta);
+    }
+
      mt76_connac_mcu_wtbl_update_hdr_trans(&dev->mt76, vif, sta);
  }

In order to update the code based on my latest changes and to fix potential race conditions on tx/rx packets during the transition, please change to this order:

1. copy the sta
2. allocate a new wcid
3. change the wcid index in the copied sta to the newly allocated wcid
4. call mcu functions on the duplicate sta for creating the new sta entry.
5. use rcu_assign_pointer to point dev->wcid[new_idx] at &msta->wcid
6. swap wcid index between real sta and duplicated sta
7. rcu_assign_pointer(dev->wcid[orig_idx], NULL)
8. synchronize_rcu()
9. call mcu functions to delete the duplicate sta's entry (points to old wcid after the swap)
10. free the duplicated sta

Now, the mt7915_sta_set_4addr function looks like this(__mt76_wcid_alloc
not modified), do you think it's acceptable?

static void mt7915_sta_set_4addr(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
bool enabled)
{
struct mt7915_dev *dev = mt7915_hw_dev(hw);
struct mt7915_sta *msta = (struct mt7915_sta *)sta->drv_priv;
int min = MT76_WED_WDS_MIN, max = MT76_WED_WDS_MAX;
struct ieee80211_sta *pre_sta;
u8 flags = MT76_WED_DEFAULT;
int temp_idx;

if (enabled)
set_bit(MT_WCID_FLAG_4ADDR, &msta->wcid.flags);
else
clear_bit(MT_WCID_FLAG_4ADDR, &msta->wcid.flags);

if (!msta->wcid.sta)
return;

if (mtk_wed_device_active(&dev->mt76.mmio.wed) &&
!is_mt7915(&dev->mt76) &&
(msta->wcid.idx < min || msta->wcid.idx > max - 1)) {
pre_sta = kzalloc(sizeof(*sta) + sizeof(*msta), GFP_KERNEL);
memmove(pre_sta, sta, sizeof(*sta) + sizeof(*msta));
if (mtk_wed_device_active(&dev->mt76.mmio.wed) &&
!is_mt7915(&dev->mt76)) {
flags = test_bit(MT_WCID_FLAG_4ADDR, &msta->wcid.flags) ?
MT76_WED_WDS_ACTIVE : MT76_WED_ACTIVE;
}

temp_idx = __mt76_wcid_alloc(mdev->wcid_mask, MT7915_WTBL_STA, flags);
((struct mt7915_sta *)pre_sta->drv_priv)->wcid.idx = temp_idx;
mt7915_sta_add(hw, vif, pre_sta);
rcu_assign_pointer(dev->mt76.wcid[temp_idx], &msta->wcid);
temp_idx = msta->wcid.idx;
msta->wcid.idx = ((struct mt7915_sta *)pre_sta->drv_priv)->wcid.idx;
((struct mt7915_sta *)pre_sta->drv_priv)->wcid.idx = temp_idx;
rcu_assign_pointer(dev->mt76.wcid[temp_idx], NULL);
synchronize_rcu();
mt7915_sta_remove(hw, vif, pre_sta);
kfree(pre_sta);
}

mt76_connac_mcu_wtbl_update_hdr_trans(&dev->mt76, vif, sta);
}

Best regards,
Shengyu

This should allow mgmt tx/rx to work while the sta is being migrated to the new wcid entry.

- Felix

Attachment: OpenPGP_0xE3520CC91929C8E7.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature