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

[Bitterblue Smith <rtl8821cerfe2@gmail.com>]

On 22/08/2024 09:58, Kalle Valo wrote:
> 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.
> 

Okay, I will use a struct. This is similar to the solution
already accepted in rtl8xxxu:

struct rtw_c2h_ra_rpt {
	u8 rate_sgi;
	u8 mac_id;
	u8 byte2;
	u8 status;
	u8 byte4;
	u8 ra_ratio;
	u8 bw;
	u8 txcls_rate;
} __packed;

#define RTW_C2H_RA_RPT_RATE	GENMASK(6, 0)
#define RTW_C2H_RA_RPT_SGI	BIT(7)


	mac_id = ra_rpt->mac_id;
	if (si->mac_id != mac_id)
		return;

	si->ra_report.txrate.flags = 0;

	rate = u8_get_bits(ra_rpt->rate_sgi, RTW_C2H_RA_RPT_RATE);
	sgi = u8_get_bits(ra_rpt->rate_sgi, RTW_C2H_RA_RPT_SGI);
	if (ra_data->length >= offsetofend(typeof(*ra_rpt), bw))
		bw = ra_rpt->bw;
	else
		bw = si->bw_mode;