diff mbox series

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

Message ID e0de25fc-8742-4899-854e-7cbd93aaa582@gmail.com
State New
Headers show
Series wifi: rtw88: Add support for RTL8821AU and RTL8812AU | expand

Commit Message

Bitterblue Smith Aug. 11, 2024, 8:55 p.m. UTC
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(-)

Comments

Ping-Ke Shih Aug. 15, 2024, 6:14 a.m. UTC | #1
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);
>
Bitterblue Smith Aug. 19, 2024, 5:52 p.m. UTC | #2
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).
Ping-Ke Shih Aug. 20, 2024, 1:10 a.m. UTC | #3
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.)
Bitterblue Smith Aug. 20, 2024, 9:44 p.m. UTC | #4
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.
Ping-Ke Shih Aug. 21, 2024, 12:31 a.m. UTC | #5
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.
Bitterblue Smith Aug. 21, 2024, 11:13 a.m. UTC | #6
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;
Ping-Ke Shih Aug. 22, 2024, 12:33 a.m. UTC | #7
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.
Kalle Valo Aug. 22, 2024, 6:58 a.m. UTC | #8
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.
Bitterblue Smith Aug. 22, 2024, 2:04 p.m. UTC | #9
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;
diff mbox series

Patch

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)
+		bw = si->bw_mode;
+	else
+		bw = GET_RA_REPORT_BW(ra_data->payload);
 
 	if (rate < DESC_RATEMCS0) {
 		si->ra_report.txrate.legacy = rtw_desc_to_bitrate(rate);
@@ -199,7 +202,8 @@  static void rtw_fw_ra_report_handle(struct rtw_dev *rtwdev, u8 *payload,
 {
 	struct rtw_fw_iter_ra_data ra_data;
 
-	if (WARN(length < 7, "invalid ra report c2h length\n"))
+	if (WARN(length < rtwdev->chip->c2h_ra_report_size,
+		 "invalid ra report c2h length %d\n", length))
 		return;
 
 	rtwdev->dm_info.tx_rate = GET_RA_REPORT_RATE(payload);
diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h
index 16619432f450..baf3098e93ba 100644
--- a/drivers/net/wireless/realtek/rtw88/main.h
+++ b/drivers/net/wireless/realtek/rtw88/main.h
@@ -1203,6 +1203,7 @@  struct rtw_chip_info {
 
 	u8 usb_tx_agg_desc_num;
 	bool hw_feature_report;
+	u8 c2h_ra_report_size;
 
 	u8 default_1ss_tx_path;
 
diff --git a/drivers/net/wireless/realtek/rtw88/rtw8703b.c b/drivers/net/wireless/realtek/rtw88/rtw8703b.c
index c9bb779812b6..8f90320e1c51 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8703b.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8703b.c
@@ -2015,6 +2015,7 @@  const struct rtw_chip_info rtw8703b_hw_spec = {
 	.ampdu_density = IEEE80211_HT_MPDU_DENSITY_16,
 	.usb_tx_agg_desc_num = 1, /* Not sure if this chip has USB interface */
 	.hw_feature_report = true,
+	.c2h_ra_report_size = 7,
 
 	.path_div_supported = false,
 	.ht_supported = true,
diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723d.c b/drivers/net/wireless/realtek/rtw88/rtw8723d.c
index fea327e5a474..85f3abee32fc 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8723d.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8723d.c
@@ -2173,6 +2173,7 @@  const struct rtw_chip_info rtw8723d_hw_spec = {
 	.dig_min = 0x20,
 	.usb_tx_agg_desc_num = 1,
 	.hw_feature_report = true,
+	.c2h_ra_report_size = 7,
 	.ht_supported = true,
 	.vht_supported = false,
 	.lps_deep_mode_supported = 0,
diff --git a/drivers/net/wireless/realtek/rtw88/rtw8821c.c b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
index a95bca79ce02..76726632c048 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
@@ -2010,6 +2010,7 @@  const struct rtw_chip_info rtw8821c_hw_spec = {
 	.dig_min = 0x1c,
 	.usb_tx_agg_desc_num = 3,
 	.hw_feature_report = true,
+	.c2h_ra_report_size = 7,
 	.ht_supported = true,
 	.vht_supported = true,
 	.lps_deep_mode_supported = BIT(LPS_DEEP_MODE_LCLK),
diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822b.c b/drivers/net/wireless/realtek/rtw88/rtw8822b.c
index 42f055eec16b..9dde02dbbb62 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8822b.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8822b.c
@@ -2551,6 +2551,7 @@  const struct rtw_chip_info rtw8822b_hw_spec = {
 	.dig_min = 0x1c,
 	.usb_tx_agg_desc_num = 3,
 	.hw_feature_report = true,
+	.c2h_ra_report_size = 7,
 	.ht_supported = true,
 	.vht_supported = true,
 	.lps_deep_mode_supported = BIT(LPS_DEEP_MODE_LCLK),
diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822c.c b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
index c646bd4ec5e2..5dabcd0efb1d 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8822c.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8822c.c
@@ -5372,6 +5372,7 @@  const struct rtw_chip_info rtw8822c_hw_spec = {
 	.dig_min = 0x20,
 	.usb_tx_agg_desc_num = 3,
 	.hw_feature_report = true,
+	.c2h_ra_report_size = 7,
 	.default_1ss_tx_path = BB_PATH_A,
 	.path_div_supported = true,
 	.ht_supported = true,