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.
