Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> wrote:
> On 21/08/2024 03:31, Ping-Ke Shih wrote:
> > Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> wrote:
> >> On 20/08/2024 04:10, Ping-Ke Shih wrote:
> >>> Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> wrote:
> >>>> On 15/08/2024 09:14, Ping-Ke Shih wrote:
> >>>>> Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx> wrote:
> >>>>>> The RTL8821AU and RTL8812AU have smaller RA report size, only 4 bytes.
> >>>>>> Avoid the "invalid ra report c2h length" error.
> >>>>>>
> >>>>>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx>
> >>>>>> ---
> >>>>>> drivers/net/wireless/realtek/rtw88/fw.c | 8 ++++++--
> >>>>>> drivers/net/wireless/realtek/rtw88/main.h | 1 +
> >>>>>> drivers/net/wireless/realtek/rtw88/rtw8703b.c | 1 +
> >>>>>> drivers/net/wireless/realtek/rtw88/rtw8723d.c | 1 +
> >>>>>> drivers/net/wireless/realtek/rtw88/rtw8821c.c | 1 +
> >>>>>> drivers/net/wireless/realtek/rtw88/rtw8822b.c | 1 +
> >>>>>> drivers/net/wireless/realtek/rtw88/rtw8822c.c | 1 +
> >>>>>> 7 files changed, 12 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
> >>>>>> index 782f3776e0a0..ac53e3e30af0 100644
> >>>>>> --- a/drivers/net/wireless/realtek/rtw88/fw.c
> >>>>>> +++ b/drivers/net/wireless/realtek/rtw88/fw.c
> >>>>>> @@ -157,7 +157,10 @@ static void rtw_fw_ra_report_iter(void *data, struct ieee80211_sta *sta)
> >>>>>>
> >>>>>> rate = GET_RA_REPORT_RATE(ra_data->payload);
> >>>>>> sgi = GET_RA_REPORT_SGI(ra_data->payload);
> >>>>>> - bw = GET_RA_REPORT_BW(ra_data->payload);
> >>>>>> + if (si->rtwdev->chip->c2h_ra_report_size < 7)
> >>>>>
> >>>>> Explicitly specify '== 4' for the case of RTL8821AU and RTL8812AU.
> >>>>>
> >>>>>> + bw = si->bw_mode;
> >>>>>> + else
> >>>>>> + bw = GET_RA_REPORT_BW(ra_data->payload);
> >>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>> Would that make sense? I check for less than 7 because the size
> >>>> has to be at least 7 in order to access payload[6] (GET_RA_REPORT_BW).
> >>>
> >>> As you did "WARN(length < rtwdev->chip->c2h_ra_report_size)", I assume you
> >>> expect "< 7" cases is only for coming chips RTL8821AU and RTL8812AU.
> >>>
> >>> Maybe explicitly specifying chips ID would be easier to understand:
> >>> if (chip == RTL8821A || chip == RTL8812A)
> >>> bw = si->bw_mode;
> >>> else
> >>> bw = GET_RA_REPORT_BW(ra_data->payload);
> >>>
> >>> That's why I want "== 4". (but it seems implicitly not explicitly though.)
> >>>
> >>
> >> I just checked, the RA report size of RTL8814AU is 6.
> >
> > Could you also check if the report format is compatible?
> > I mean definition of first 4 bytes are the same for all chips? and
> > definition of first 6 bytes are the same for RTL8814AU and current
> > exiting chips?
> >
> > By the way, I think we should struct with w0, w1, ... fields instead.
> > struct rtw_ra_report {
> > __le32 w0;
> > __le32 w1;
> > __le32 w2;
> > __le32 w3;
> > __le32 w4;
> > __le32 w5;
> > __le32 w6;
> > } __packed;
> >
> > Then, we can be easier to avoid accessing out of range. GET_RA_REPORT_BW()
> > hides something, no help to read the code.
> >
>
> The report format looks compatible.
>
> I'm not sure how a struct with __le32 members would help here.
> I agree that the current macros hide things. We could access payload
> directly. The variable names already make it clear what each byte is:
>
> mac_id = ra_data->payload[1];
> if (si->mac_id != mac_id)
> return;
>
> si->ra_report.txrate.flags = 0;
>
> rate = u8_get_bits(ra_data->payload[0], GENMASK(6, 0));
> sgi = u8_get_bits(ra_data->payload[0], BIT(7));
> if (si->rtwdev->chip->c2h_ra_report_size >= 7)
> bw = ra_data->payload[6];
> else
> bw = si->bw_mode;
Yes, this is also clear to me to avoid accessing out of range.
Another advantage of a struct is to explicitly tell us the total size of a
C2H event.
