| Message ID | e0de25fc-8742-4899-854e-7cbd93aaa582@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Ping-Ke Shih |
| Headers | show |
| Series | wifi: rtw88: Add support for RTL8821AU and RTL8812AU | expand |
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); >
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).
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.)
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.
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.
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;
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.
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.
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 --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,
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(-)