Re: [PATCH rtw-next v3 2/5] wifi: rtw88: 88xxa: receive VHT SU beamformed transmissions

[Luka Gejak <luka.gejak@linux.dev>]

On November 13, 2026 9:12:13 AM GMT+01:00, Jie Feng <hjf3108@gmail.com>
wrote:
>On Fri, 13 Nov 2026 at 16:07, Luka Gejak <luka.gejak@linux.dev> wrote:
>>
>> On November 13, 2026 9:01:47 AM GMT+01:00, Jie Feng <hjf3108@gmail.com>
>> wrote:
>> >On Fri, 13 Nov 2026 at 15:46, Luka Gejak <luka.gejak@linux.dev> wrote:
>> >>
>> >> Hi Jie Feng,
>> >>
>> >> Thanks for the new version. A question and a comment below.
>> >>
>> >> On November 12, 2026 6:32:41 PM GMT+01:00, Ho Jie Feng
>> ><hjf3108@gmail.com>
>> >> wrote:
>> >>
>> >> > diff --git a/drivers/net/wireless/realtek/rtw88/main.c
>> >b/drivers/net/wireless/realtek/rtw88/main.c
>> >> > index e243738809..fe93c93b1a 100644
>> >> > --- a/drivers/net/wireless/realtek/rtw88/main.c
>> >> > +++ b/drivers/net/wireless/realtek/rtw88/main.c
>> >>
>> >> [ ... ]
>> >>
>> >> > @@ -1621,10 +1622,13 @@ static void rtw_init_vht_cap(struct rtw_dev
>> >*rtwdev,
>> >> >                      0;
>> >> >       if (rtwdev->hal.rf_path_num > 1)
>> >> >               vht_cap->cap |= IEEE80211_VHT_CAP_TXSTBC;
>> >> > -     vht_cap->cap |= IEEE80211_VHT_CAP_MU_BEAMFORMEE_CAPABLE |
>> >> > -                     IEEE80211_VHT_CAP_SU_BEAMFORMEE_CAPABLE;
>> >> > -     vht_cap->cap |= (rtwdev->hal.bfee_sts_cap <<
>> >> > -                     IEEE80211_VHT_CAP_BEAMFORMEE_STS_SHIFT);
>> >> > +     if (rtw_bf_support && chip->bfer_su_max_num) {
>> >> > +             vht_cap->cap |=
>IEEE80211_VHT_CAP_SU_BEAMFORMEE_CAPABLE;
>> >> > +             vht_cap->cap |= rtwdev->hal.bfee_sts_cap <<
>> >> > +                            
>IEEE80211_VHT_CAP_BEAMFORMEE_STS_SHIFT;
>> >> > +     }
>> >> > +     if (rtw_bf_support && chip->bfer_mu_max_num)
>> >> > +             vht_cap->cap |=
>IEEE80211_VHT_CAP_MU_BEAMFORMEE_CAPABLE;
>> >>
>> >> In v2 this was its own patch, 3/7 "correctly expose beamformee
>> >> capabilities", after Bitterblue asked on v1 for it to be split out.
>Was
>> >> folding it back in here intended? I couldn't find it in the v3
>> >changelog.
>> >Sorry, looks like I used the wrong commit on my local tree, will resend
>as
>> >a v4.
>> >Please ignore this v3.
>> >
>>
>> Can you address all comments I have send, as they will most likely apply
>> to v4 as well?
>Yes v4 will address both the csi and cfo comments. I will send it out
>after figuring
>out the correct fix.
>
>> >> I can see a reason for it. Without this part, an AP that is also a MU
>> >> beamformer makes rtw_bf_assoc() take the MU branch and return at
>> >> bfer_mu_max_num, so the new config_bfee code would never run for it.
>If
>> >> that is why, maybe mention it in the changelog, and in the commit
>> >message
>> >> too, since this is core code and the support_bf check now hides the
>> >> beamformee caps on every chip when beamforming is turned off.
>> >>
>> >> [ ... ]
>> >>
>> >> > diff --git a/drivers/net/wireless/realtek/rtw88/rtw8812a.c
>> >b/drivers/net/wireless/realtek/rtw88/rtw8812a.c
>> >> > index 55f1d3d324..5d58dff4fc 100644
>> >> > --- a/drivers/net/wireless/realtek/rtw88/rtw8812a.c
>> >> > +++ b/drivers/net/wireless/realtek/rtw88/rtw8812a.c
>> >> > @@ -931,7 +931,7 @@ static const struct rtw_chip_ops rtw8812a_ops =
>{
>> >> >       .phy_calibration        = rtw8812a_phy_calibration,
>> >> >       .cck_pd_set             = rtw_phy_cck_pd_set,
>> >> >       .pwr_track              = rtw8812a_pwr_track,
>> >> > -     .config_bfee            = NULL,
>> >> > +     .config_bfee            = rtw88xxa_bf_config_bfee,
>> >> >       .set_gid_table          = NULL,
>> >> >       .cfg_csi_rate           = NULL,
>> >>
>> >> As far as I can see, 8812a, 8821a and 8814a are now the first chips
>that
>> >> can become a SU beamformee without a cfg_csi_rate op. Once
>> >rtw_bf_assoc()
>> >> has set bfee.role, the watchdog goes through
>> >>
>> >>         rtw_vif_watch_dog_iter()
>> >>           rtw_dynamic_csi_rate()
>> >>
>> >> and there new_csi_rate_idx is only written by that op:
>> >>
>> >>         u8 new_csi_rate_idx;
>> >>         ...
>> >>         rtw_chip_cfg_csi_rate(rtwdev, rtwdev->dm_info.min_rssi,
>> >>                               bf_info->cur_csi_rpt_rate,
>> >>                               fix_rate_enable, &new_csi_rate_idx);
>> >>
>> >>         if (new_csi_rate_idx != bf_info->cur_csi_rpt_rate)
>> >>                 bf_info->cur_csi_rpt_rate = new_csi_rate_idx;
>> >>
>> >> So if I read it right, it compares and stores an uninitialized value
>> >> there. cur_csi_rpt_rate has no other user on these chips, so it seems
>> >> harmless for now, but I think KMSAN would report it. Maybe
>> >> rtw_dynamic_csi_rate() could return early when the chip has no
>> >> cfg_csi_rate op?
>This looks correct, will put in v4.
>
>Another alternative would be to do:
>u8 new_csi_rate_idx = bf_info->cur_csi_rpt_rate
>and the != will never pass.
>

Either works for me. The initialization is the smaller change and doesn't
repeat the op check that rtw_chip_cfg_csi_rate() already does, so I would
go with that. Since it is core code, maybe as its own small patch before
the beamformee one.

Best regards,
Luka Gejak