[PATCH 0/6] rtw88: minor fixes from suggestions during review

[PATCH 0/6] rtw88: minor fixes from suggestions during review

[yhchuang]

From: Yan-Hsuan Chuang <yhchuang@realtek.com>

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

[PATCH 1/6] rtw88: add license for Makefile

[yhchuang]

From: Yan-Hsuan Chuang <yhchuang@realtek.com>

Add missing license for Makefile

Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
---
 drivers/net/wireless/realtek/rtw88/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtw88/Makefile b/drivers/net/wireless/realtek/rtw88/Makefile
index da5e36e..74cd066 100644
--- a/drivers/net/wireless/realtek/rtw88/Makefile
+++ b/drivers/net/wireless/realtek/rtw88/Makefile
@@ -1,3 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
 obj-$(CONFIG_RTW88_CORE)	+= rtw88.o
 rtw88-y += main.o \
 	   mac80211.o \
-- 
2.7.4

[PATCH 2/6] rtw88: pci: use ieee80211_ac_numbers instead of 0-3

[yhchuang]

From: Yan-Hsuan Chuang <yhchuang@realtek.com>

AC numbers are defined as enum in mac80211, use them instead of bare
0-3 indexing.

Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
---
 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

[PATCH 3/6] rtw88: pci: check if queue mapping exceeds size of ac_to_hwq

[yhchuang]

From: Yan-Hsuan Chuang <yhchuang@realtek.com>

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 <yhchuang@realtek.com>
---
 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

[PATCH 4/6] rtw88: fix unassigned rssi_level in rtw_sta_info

[yhchuang]

From: Yan-Hsuan Chuang <yhchuang@realtek.com>

The new rssi_level should be stored in si, otherwise the rssi_level will
never be updated and get a wrong RA mask, which is calculated by the
rssi level

Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
---
 drivers/net/wireless/realtek/rtw88/phy.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/phy.c b/drivers/net/wireless/realtek/rtw88/phy.c
index 4381b36..7f437e2 100644
--- a/drivers/net/wireless/realtek/rtw88/phy.c
+++ b/drivers/net/wireless/realtek/rtw88/phy.c
@@ -144,10 +144,10 @@ static void rtw_phy_stat_rssi_iter(void *data, struct ieee80211_sta *sta)
 	struct rtw_phy_stat_iter_data *iter_data = data;
 	struct rtw_dev *rtwdev = iter_data->rtwdev;
 	struct rtw_sta_info *si = (struct rtw_sta_info *)sta->drv_priv;
-	u8 rssi, rssi_level;
+	u8 rssi;
 
 	rssi = ewma_rssi_read(&si->avg_rssi);
-	rssi_level = rtw_phy_get_rssi_level(si->rssi_level, rssi);
+	si->rssi_level = rtw_phy_get_rssi_level(si->rssi_level, rssi);
 
 	rtw_fw_send_rssi_info(rtwdev, si);
 
-- 
2.7.4

[PATCH 5/6] rtw88: mac: remove dangerous while (1)

[yhchuang]

From: Yan-Hsuan Chuang <yhchuang@realtek.com>

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 <yhchuang@realtek.com>
---
 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

[PATCH 6/6] rtw88: more descriptions about LPS

[yhchuang]

From: Yan-Hsuan Chuang <yhchuang@realtek.com>

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 <yhchuang@realtek.com>
---
 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

Re: [PATCH 5/6] rtw88: mac: remove dangerous while (1)

[Johannes Berg]

On Fri, 2019-05-03 at 18:31 +0800, yhchuang@realtek.com wrote:
> 
> +	while ((cmd = cmd_seq[idx])) {
...
> +	};

That semicolon is pretty pointless there :-)

johannes

RE: [PATCH 5/6] rtw88: mac: remove dangerous while (1)

[Tony Chuang]

> 
> On Fri, 2019-05-03 at 18:31 +0800, yhchuang@realtek.com wrote:
> >
> > +	while ((cmd = cmd_seq[idx])) {
> ...
> > +	};
> 
> That semicolon is pretty pointless there :-)
> 
> johannes
> 
> 

Missed it. Will send v2.
Thanks!

Yan-Hsuan

Re: [PATCH 6/6] rtw88: more descriptions about LPS

[Kalle Valo]

<yhchuang@realtek.com> writes:

> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> 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 <yhchuang@realtek.com>
> ---
>  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

Re: [PATCH 1/6] rtw88: add license for Makefile

[Kalle Valo]

<yhchuang@realtek.com> writes:

> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> Add missing license for Makefile
>
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
> ---
>  drivers/net/wireless/realtek/rtw88/Makefile | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/Makefile b/drivers/net/wireless/realtek/rtw88/Makefile
> index da5e36e..74cd066 100644
> --- a/drivers/net/wireless/realtek/rtw88/Makefile
> +++ b/drivers/net/wireless/realtek/rtw88/Makefile
> @@ -1,3 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0

Other files in the driver are "GPL-2.0 OR BSD-3-Clause", why not
Makefile? I prefer that the whole driver has the same license, keeps
things simple.

-- 
Kalle Valo

RE: [PATCH 6/6] rtw88: more descriptions about LPS

[Tony Chuang]

> Subject: Re: [PATCH 6/6] rtw88: more descriptions about LPS
> 
> <yhchuang@realtek.com> writes:
> 
> > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
> >
> > 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 <yhchuang@realtek.com>
> > ---
> >  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

RE: [PATCH 1/6] rtw88: add license for Makefile

[Tony Chuang]

> Subject: Re: [PATCH 1/6] rtw88: add license for Makefile
> 
> <yhchuang@realtek.com> writes:
> 
> > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
> >
> > Add missing license for Makefile
> >
> > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
> > ---
> >  drivers/net/wireless/realtek/rtw88/Makefile | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/Makefile
> b/drivers/net/wireless/realtek/rtw88/Makefile
> > index da5e36e..74cd066 100644
> > --- a/drivers/net/wireless/realtek/rtw88/Makefile
> > +++ b/drivers/net/wireless/realtek/rtw88/Makefile
> > @@ -1,3 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0
> 
> Other files in the driver are "GPL-2.0 OR BSD-3-Clause", why not
> Makefile? I prefer that the whole driver has the same license, keeps
> things simple.
> 
> --
> Kalle Valo
> 

You are right, different license is strange.
Will add it in v2, thanks.

Yan-Hsuan

Re: [PATCH 5/6] rtw88: mac: remove dangerous while (1)

[Kalle Valo]

<yhchuang@realtek.com> writes:

> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> 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 <yhchuang@realtek.com>
> ---
>  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

RE: [PATCH 5/6] rtw88: mac: remove dangerous while (1)

[Tony Chuang]

> Subject: Re: [PATCH 5/6] rtw88: mac: remove dangerous while (1)
> 
> <yhchuang@realtek.com> writes:
> 
> > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
> >
> > 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 <yhchuang@realtek.com>
> > ---
> >  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

Re: [PATCH 5/6] rtw88: mac: remove dangerous while (1)

[Kalle Valo]

Tony Chuang <yhchuang@realtek.com> writes:

>> Subject: Re: [PATCH 5/6] rtw88: mac: remove dangerous while (1)
>> 
>> <yhchuang@realtek.com> writes:
>> 
>> > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>> >
>> > 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 <yhchuang@realtek.com>
>> > ---
>> >  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