Re: [PATCH 03/20] wifi: rtw88: Allow different C2H RA report sizes

[Kalle Valo <kvalo@kernel.org>]

Ping-Ke Shih <pkshih@realtek.com> writes:

> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>
>> On 21/08/2024 03:31, Ping-Ke Shih wrote:
>> > Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>> >> On 20/08/2024 04:10, Ping-Ke Shih wrote:
>> >>> Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
>> >>>> On 15/08/2024 09:14, Ping-Ke Shih wrote:
>> >>>>> Bitterblue Smith <rtl8821cerfe2@gmail.com> 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@gmail.com>
>> >>>>>> ---
>> >>>>>>  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.

Yeah, please avoid that payload[6] stuff for parsing firmware commands
and events. It just makes the code harder to read and more fragile.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches