On 22/08/2024 09:58, Kalle Valo wrote:
> Ping-Ke Shih <pkshih@xxxxxxxxxxx> writes:
>
>> 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.
>
> 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;
