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.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
