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

[Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx>]

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;