From: Yan-Hsuan Chuang <[email protected]>
The series fix some small problems for rtw88, most of the suggestions
are from the review process.
Yan-Hsuan Chuang (6):
rtw88: add license for Makefile
rtw88: pci: use ieee80211_ac_numbers instead of 0-3
rtw88: pci: check if queue mapping exceeds size of ac_to_hwq
rtw88: fix unassigned rssi_level in rtw_sta_info
rtw88: mac: remove dangerous while (1)
rtw88: more descriptions about LPS
drivers/net/wireless/realtek/rtw88/Makefile | 2 ++
drivers/net/wireless/realtek/rtw88/mac.c | 9 +++------
drivers/net/wireless/realtek/rtw88/main.c | 2 +-
drivers/net/wireless/realtek/rtw88/pci.c | 10 ++++++----
drivers/net/wireless/realtek/rtw88/phy.c | 4 ++--
5 files changed, 14 insertions(+), 13 deletions(-)
--
2.7.4
From: Yan-Hsuan Chuang <[email protected]>
Dump warning messages when we get a q_mapping larger than the AC
numbers. And pick BE queue as default.
Signed-off-by: Yan-Hsuan Chuang <[email protected]>
---
drivers/net/wireless/realtek/rtw88/pci.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index 87bfcb3..353871c 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -504,6 +504,8 @@ static u8 rtw_hw_queue_mapping(struct sk_buff *skb)
queue = RTW_TX_QUEUE_BCN;
else if (unlikely(ieee80211_is_mgmt(fc) || ieee80211_is_ctl(fc)))
queue = RTW_TX_QUEUE_MGMT;
+ else if (WARN_ON_ONCE(q_mapping >= ARRAY_SIZE(ac_to_hwq)))
+ queue = ac_to_hwq[IEEE80211_AC_BE];
else
queue = ac_to_hwq[q_mapping];
--
2.7.4
From: Yan-Hsuan Chuang <[email protected]>
Not to use while (1) to parse power sequence commands in an array.
Put the statement (when cmd is not NULL) instead to make the loop stop
when the next fetched command is NULL.
Signed-off-by: Yan-Hsuan Chuang <[email protected]>
---
drivers/net/wireless/realtek/rtw88/mac.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/mac.c b/drivers/net/wireless/realtek/rtw88/mac.c
index 25a923b..7487b2e 100644
--- a/drivers/net/wireless/realtek/rtw88/mac.c
+++ b/drivers/net/wireless/realtek/rtw88/mac.c
@@ -203,17 +203,14 @@ static int rtw_pwr_seq_parser(struct rtw_dev *rtwdev,
return -EINVAL;
}
- do {
- cmd = cmd_seq[idx];
- if (!cmd)
- break;
-
+ while ((cmd = cmd_seq[idx])) {
ret = rtw_sub_pwr_seq_parser(rtwdev, intf_mask, cut_mask, cmd);
if (ret)
return -EBUSY;
+ /* fetch next command */
idx++;
- } while (1);
+ };
return 0;
}
--
2.7.4
From: Yan-Hsuan Chuang <[email protected]>
The LPS represents Leisure Power Save. When enabled, firmware will be in
charge of turning radio off between beacons. Also firmware should turn
on the radio when beacon is coming, and the data queued should be
transmitted in TBTT period.
Signed-off-by: Yan-Hsuan Chuang <[email protected]>
---
drivers/net/wireless/realtek/rtw88/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index f447361..6953013 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -20,7 +20,7 @@ EXPORT_SYMBOL(rtw_debug_mask);
module_param_named(support_lps, rtw_fw_support_lps, bool, 0644);
module_param_named(debug_mask, rtw_debug_mask, uint, 0644);
-MODULE_PARM_DESC(support_lps, "Set Y to enable LPS support");
+MODULE_PARM_DESC(support_lps, "Set Y to enable Leisure Power Save support, turn radio off between beacons");
MODULE_PARM_DESC(debug_mask, "Debugging mask");
static struct ieee80211_channel rtw_channeltable_2g[] = {
--
2.7.4
From: Yan-Hsuan Chuang <[email protected]>
AC numbers are defined as enum in mac80211, use them instead of bare
0-3 indexing.
Signed-off-by: Yan-Hsuan Chuang <[email protected]>
---
drivers/net/wireless/realtek/rtw88/pci.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index cfe05ba..87bfcb3 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -487,10 +487,10 @@ static void rtw_pci_stop(struct rtw_dev *rtwdev)
}
static u8 ac_to_hwq[] = {
- [0] = RTW_TX_QUEUE_VO,
- [1] = RTW_TX_QUEUE_VI,
- [2] = RTW_TX_QUEUE_BE,
- [3] = RTW_TX_QUEUE_BK,
+ [IEEE80211_AC_VO] = RTW_TX_QUEUE_VO,
+ [IEEE80211_AC_VI] = RTW_TX_QUEUE_VI,
+ [IEEE80211_AC_BE] = RTW_TX_QUEUE_BE,
+ [IEEE80211_AC_BK] = RTW_TX_QUEUE_BK,
};
static u8 rtw_hw_queue_mapping(struct sk_buff *skb)
--
2.7.4
On Fri, 2019-05-03 at 18:31 +0800, [email protected] wrote:
>
> + while ((cmd = cmd_seq[idx])) {
...
> + };
That semicolon is pretty pointless there :-)
johannes
>
> On Fri, 2019-05-03 at 18:31 +0800, [email protected] wrote:
> >
> > + while ((cmd = cmd_seq[idx])) {
> ...
> > + };
>
> That semicolon is pretty pointless there :-)
>
> johannes
>
>
Missed it. Will send v2.
Thanks!
Yan-Hsuan
<[email protected]> writes:
> From: Yan-Hsuan Chuang <[email protected]>
>
> Not to use while (1) to parse power sequence commands in an array.
> Put the statement (when cmd is not NULL) instead to make the loop stop
> when the next fetched command is NULL.
>
> Signed-off-by: Yan-Hsuan Chuang <[email protected]>
> ---
> drivers/net/wireless/realtek/rtw88/mac.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/mac.c b/drivers/net/wireless/realtek/rtw88/mac.c
> index 25a923b..7487b2e 100644
> --- a/drivers/net/wireless/realtek/rtw88/mac.c
> +++ b/drivers/net/wireless/realtek/rtw88/mac.c
> @@ -203,17 +203,14 @@ static int rtw_pwr_seq_parser(struct rtw_dev *rtwdev,
> return -EINVAL;
> }
>
> - do {
> - cmd = cmd_seq[idx];
> - if (!cmd)
> - break;
> -
> + while ((cmd = cmd_seq[idx])) {
> ret = rtw_sub_pwr_seq_parser(rtwdev, intf_mask, cut_mask, cmd);
> if (ret)
> return -EBUSY;
>
> + /* fetch next command */
> idx++;
> - } while (1);
> + };
I dount see how this is any better, with a suitable bug you can still
have a neverending loop, right? I was thinking more something like this:
count = 100;
do {
....
} while (count--);
That way the won't be more than 100 loops no matter how many bugs there
are :) Of course I have no idea what would be a good value for count.
--
Kalle Valo
> Subject: Re: [PATCH 6/6] rtw88: more descriptions about LPS
>
> <[email protected]> writes:
>
> > From: Yan-Hsuan Chuang <[email protected]>
> >
> > The LPS represents Leisure Power Save. When enabled, firmware will be in
> > charge of turning radio off between beacons. Also firmware should turn
> > on the radio when beacon is coming, and the data queued should be
> > transmitted in TBTT period.
> >
> > Signed-off-by: Yan-Hsuan Chuang <[email protected]>
> > ---
> > drivers/net/wireless/realtek/rtw88/main.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/main.c
> > b/drivers/net/wireless/realtek/rtw88/main.c
> > index f447361..6953013 100644
> > --- a/drivers/net/wireless/realtek/rtw88/main.c
> > +++ b/drivers/net/wireless/realtek/rtw88/main.c
> > @@ -20,7 +20,7 @@ EXPORT_SYMBOL(rtw_debug_mask);
> > module_param_named(support_lps, rtw_fw_support_lps, bool, 0644);
> > module_param_named(debug_mask, rtw_debug_mask, uint, 0644);
> >
> > -MODULE_PARM_DESC(support_lps, "Set Y to enable LPS support");
> > +MODULE_PARM_DESC(support_lps, "Set Y to enable Leisure Power Save
> > support, turn radio off between beacons");
>
> I think it would help to add:
>
> ", to turn radio off between beacons"
>
Looks better, will include it in v2.
Thanks.
Yan-Hsuan
<[email protected]> writes:
> From: Yan-Hsuan Chuang <[email protected]>
>
> The LPS represents Leisure Power Save. When enabled, firmware will be in
> charge of turning radio off between beacons. Also firmware should turn
> on the radio when beacon is coming, and the data queued should be
> transmitted in TBTT period.
>
> Signed-off-by: Yan-Hsuan Chuang <[email protected]>
> ---
> drivers/net/wireless/realtek/rtw88/main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/main.c
> b/drivers/net/wireless/realtek/rtw88/main.c
> index f447361..6953013 100644
> --- a/drivers/net/wireless/realtek/rtw88/main.c
> +++ b/drivers/net/wireless/realtek/rtw88/main.c
> @@ -20,7 +20,7 @@ EXPORT_SYMBOL(rtw_debug_mask);
> module_param_named(support_lps, rtw_fw_support_lps, bool, 0644);
> module_param_named(debug_mask, rtw_debug_mask, uint, 0644);
>
> -MODULE_PARM_DESC(support_lps, "Set Y to enable LPS support");
> +MODULE_PARM_DESC(support_lps, "Set Y to enable Leisure Power Save
> support, turn radio off between beacons");
I think it would help to add:
", to turn radio off between beacons"
--
Kalle Valo
> Subject: Re: [PATCH 5/6] rtw88: mac: remove dangerous while (1)
>
> <[email protected]> writes:
>
> > From: Yan-Hsuan Chuang <[email protected]>
> >
> > Not to use while (1) to parse power sequence commands in an array.
> > Put the statement (when cmd is not NULL) instead to make the loop stop
> > when the next fetched command is NULL.
> >
> > Signed-off-by: Yan-Hsuan Chuang <[email protected]>
> > ---
> > drivers/net/wireless/realtek/rtw88/mac.c | 9 +++------
> > 1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/mac.c
> b/drivers/net/wireless/realtek/rtw88/mac.c
> > index 25a923b..7487b2e 100644
> > --- a/drivers/net/wireless/realtek/rtw88/mac.c
> > +++ b/drivers/net/wireless/realtek/rtw88/mac.c
> > @@ -203,17 +203,14 @@ static int rtw_pwr_seq_parser(struct rtw_dev
> *rtwdev,
> > return -EINVAL;
> > }
> >
> > - do {
> > - cmd = cmd_seq[idx];
> > - if (!cmd)
> > - break;
> > -
> > + while ((cmd = cmd_seq[idx])) {
> > ret = rtw_sub_pwr_seq_parser(rtwdev, intf_mask, cut_mask, cmd);
> > if (ret)
> > return -EBUSY;
> >
> > + /* fetch next command */
> > idx++;
> > - } while (1);
> > + };
>
> I dount see how this is any better, with a suitable bug you can still
> have a neverending loop, right? I was thinking more something like this:
>
> count = 100;
> do {
> ....
> } while (count--);
>
> That way the won't be more than 100 loops no matter how many bugs there
> are :) Of course I have no idea what would be a good value for count.
>
To make this totally safe, I think we need to re-write the power seq parsing code.
I think I should drop this patch, and write a better code later.
And also re-write the polling command, to remove the while (1).
Yan-Hsuan
Tony Chuang <[email protected]> writes:
>> Subject: Re: [PATCH 5/6] rtw88: mac: remove dangerous while (1)
>>
>> <[email protected]> writes:
>>
>> > From: Yan-Hsuan Chuang <[email protected]>
>> >
>> > Not to use while (1) to parse power sequence commands in an array.
>> > Put the statement (when cmd is not NULL) instead to make the loop stop
>> > when the next fetched command is NULL.
>> >
>> > Signed-off-by: Yan-Hsuan Chuang <[email protected]>
>> > ---
>> > drivers/net/wireless/realtek/rtw88/mac.c | 9 +++------
>> > 1 file changed, 3 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/net/wireless/realtek/rtw88/mac.c
>> b/drivers/net/wireless/realtek/rtw88/mac.c
>> > index 25a923b..7487b2e 100644
>> > --- a/drivers/net/wireless/realtek/rtw88/mac.c
>> > +++ b/drivers/net/wireless/realtek/rtw88/mac.c
>> > @@ -203,17 +203,14 @@ static int rtw_pwr_seq_parser(struct rtw_dev
>> *rtwdev,
>> > return -EINVAL;
>> > }
>> >
>> > - do {
>> > - cmd = cmd_seq[idx];
>> > - if (!cmd)
>> > - break;
>> > -
>> > + while ((cmd = cmd_seq[idx])) {
>> > ret = rtw_sub_pwr_seq_parser(rtwdev, intf_mask, cut_mask, cmd);
>> > if (ret)
>> > return -EBUSY;
>> >
>> > + /* fetch next command */
>> > idx++;
>> > - } while (1);
>> > + };
>>
>> I dount see how this is any better, with a suitable bug you can still
>> have a neverending loop, right? I was thinking more something like this:
>>
>> count = 100;
>> do {
>> ....
>> } while (count--);
>>
>> That way the won't be more than 100 loops no matter how many bugs there
>> are :) Of course I have no idea what would be a good value for count.
>>
>
> To make this totally safe, I think we need to re-write the power seq parsing code.
> I think I should drop this patch, and write a better code later.
>
> And also re-write the polling command, to remove the while (1).
Sounds good to me.
--
Kalle Valo