diff mbox series

[13/13] rtw89: prohibit mac80211 chanctx ops without HW scan

Message ID 20220617084954.61261-14-pkshih@realtek.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series rtw89: support channel context | expand

Commit Message

Ping-Ke Shih June 17, 2022, 8:49 a.m. UTC 
From: Zong-Zhe Yang <kevin_yang@realtek.com>

If a chip is configured to support mac80211 chanctx ops, we avoid
using older FW that does not support HW scan to make mac80211 stack
handle scanning as expected.

Signed-off-by: Zong-Zhe Yang <kevin_yang@realtek.com>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
 drivers/net/wireless/realtek/rtw89/fw.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Kalle Valo June 20, 2022, 10:09 a.m. UTC | #1 
Ping-Ke Shih <pkshih@realtek.com> writes:

> From: Zong-Zhe Yang <kevin_yang@realtek.com>
>
> If a chip is configured to support mac80211 chanctx ops, we avoid
> using older FW that does not support HW scan to make mac80211 stack
> handle scanning as expected.
>
> Signed-off-by: Zong-Zhe Yang <kevin_yang@realtek.com>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> ---
>  drivers/net/wireless/realtek/rtw89/fw.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/wireless/realtek/rtw89/fw.c b/drivers/net/wireless/realtek/rtw89/fw.c
> index 0e12629f789c3..a47451dc9d81d 100644
> --- a/drivers/net/wireless/realtek/rtw89/fw.c
> +++ b/drivers/net/wireless/realtek/rtw89/fw.c
> @@ -250,6 +250,7 @@ static void rtw89_fw_recognize_features(struct rtw89_dev *rtwdev)
>  
>  int rtw89_fw_recognize(struct rtw89_dev *rtwdev)
>  {
> +	const struct rtw89_chip_info *chip = rtwdev->chip;
>  	int ret;
>  
>  	ret = __rtw89_fw_recognize(rtwdev, RTW89_FW_NORMAL);
> @@ -261,6 +262,13 @@ int rtw89_fw_recognize(struct rtw89_dev *rtwdev)
>  
>  	rtw89_fw_recognize_features(rtwdev);
>  
> +	if (chip->support_chanctx_num != 0 &&
> +	    !RTW89_CHK_FW_FEATURE(SCAN_OFFLOAD, &rtwdev->fw)) {
> +		rtw89_err(rtwdev,
> +			  "require newer FW to support HW scan for chanctx\n");
> +		return -ENOENT;
> +	}

So if the user has not update the firmware a kernel upgrade will break
their internet? That's not good, we should not break existing setups. So
what firmware version is required?
Ping-Ke Shih June 20, 2022, 12:34 p.m. UTC | #2 
On Mon, 2022-06-20 at 13:09 +0300, Kalle Valo wrote:
> Ping-Ke Shih <pkshih@realtek.com> writes:
> 
> > From: Zong-Zhe Yang <kevin_yang@realtek.com>
> > 
> > If a chip is configured to support mac80211 chanctx ops, we avoid
> > using older FW that does not support HW scan to make mac80211 stack
> > handle scanning as expected.
> > 
> > Signed-off-by: Zong-Zhe Yang <kevin_yang@realtek.com>
> > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> > ---
> >  drivers/net/wireless/realtek/rtw89/fw.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/net/wireless/realtek/rtw89/fw.c b/drivers/net/wireless/realtek/rtw89/fw.c
> > index 0e12629f789c3..a47451dc9d81d 100644
> > --- a/drivers/net/wireless/realtek/rtw89/fw.c
> > +++ b/drivers/net/wireless/realtek/rtw89/fw.c
> > @@ -250,6 +250,7 @@ static void rtw89_fw_recognize_features(struct rtw89_dev *rtwdev)
> >  
> >  int rtw89_fw_recognize(struct rtw89_dev *rtwdev)
> >  {
> > +	const struct rtw89_chip_info *chip = rtwdev->chip;
> >  	int ret;
> >  
> >  	ret = __rtw89_fw_recognize(rtwdev, RTW89_FW_NORMAL);
> > @@ -261,6 +262,13 @@ int rtw89_fw_recognize(struct rtw89_dev *rtwdev)
> >  
> >  	rtw89_fw_recognize_features(rtwdev);
> >  
> > +	if (chip->support_chanctx_num != 0 &&
> > +	    !RTW89_CHK_FW_FEATURE(SCAN_OFFLOAD, &rtwdev->fw)) {
> > +		rtw89_err(rtwdev,
> > +			  "require newer FW to support HW scan for chanctx\n");
> > +		return -ENOENT;
> > +	}
> 
> So if the user has not update the firmware a kernel upgrade will break
> their internet? That's not good, we should not break existing setups. So
> what firmware version is required?
> 

Firmware version 0.13.35.0 is required. The firmware has been in
linux-firmware repository on 2022-02-18. I think people being able
to update kernel can update firmware as well.

The alternative ways could be
1. add a module parameter, like no_channel_context. We can add a
   prompt to note people can set it to 1 for old firmware.

2. wait version of request_firmware() as first step of pci probe.
   The probe could cost longer time, because currently we use
   request_firmware_nowait() and continue to initialize in parallel.
   More, hw->priv isn't allocated at that moment, so it could be not
   so straightforward.


Ping-Ke
Ping-Ke Shih June 23, 2022, 3:51 a.m. UTC | #3 
On Mon, 2022-06-20 at 12:34 +0000, Ping-Ke Shih wrote:
> On Mon, 2022-06-20 at 13:09 +0300, Kalle Valo wrote:
> > Ping-Ke Shih <pkshih@realtek.com> writes:
> > 
> > > From: Zong-Zhe Yang <kevin_yang@realtek.com>
> > > 
> > > If a chip is configured to support mac80211 chanctx ops, we avoid
> > > using older FW that does not support HW scan to make mac80211 stack
> > > handle scanning as expected.
> > > 
> > > Signed-off-by: Zong-Zhe Yang <kevin_yang@realtek.com>
> > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> > > ---
> > >  drivers/net/wireless/realtek/rtw89/fw.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/net/wireless/realtek/rtw89/fw.c b/drivers/net/wireless/realtek/rtw89/fw.c
> > > index 0e12629f789c3..a47451dc9d81d 100644
> > > --- a/drivers/net/wireless/realtek/rtw89/fw.c
> > > +++ b/drivers/net/wireless/realtek/rtw89/fw.c
> > > @@ -250,6 +250,7 @@ static void rtw89_fw_recognize_features(struct rtw89_dev *rtwdev)
> > >  
> > >  int rtw89_fw_recognize(struct rtw89_dev *rtwdev)
> > >  {
> > > +	const struct rtw89_chip_info *chip = rtwdev->chip;
> > >  	int ret;
> > >  
> > >  	ret = __rtw89_fw_recognize(rtwdev, RTW89_FW_NORMAL);
> > > @@ -261,6 +262,13 @@ int rtw89_fw_recognize(struct rtw89_dev *rtwdev)
> > >  
> > >  	rtw89_fw_recognize_features(rtwdev);
> > >  
> > > +	if (chip->support_chanctx_num != 0 &&
> > > +	    !RTW89_CHK_FW_FEATURE(SCAN_OFFLOAD, &rtwdev->fw)) {
> > > +		rtw89_err(rtwdev,
> > > +			  "require newer FW to support HW scan for chanctx\n");
> > > +		return -ENOENT;
> > > +	}
> > 
> > So if the user has not update the firmware a kernel upgrade will break
> > their internet? That's not good, we should not break existing setups. So
> > what firmware version is required?
> > 
> 
> Firmware version 0.13.35.0 is required. The firmware has been in
> linux-firmware repository on 2022-02-18. I think people being able
> to update kernel can update firmware as well.
> 
> The alternative ways could be
> 1. add a module parameter, like no_channel_context. We can add a
>    prompt to note people can set it to 1 for old firmware.
> 
> 2. wait version of request_firmware() as first step of pci probe.
>    The probe could cost longer time, because currently we use
>    request_firmware_nowait() and continue to initialize in parallel.
>    More, hw->priv isn't allocated at that moment, so it could be not
>    so straightforward.
> 

To prevent break users' existing setups, modified approach 1 is adopted
by patchset v2. We add a module parameter rtw89_use_chanctx, and disable
it by default. So, users' setups with old firmware can still work.

If a user wants channel context to support concurrency, he can set
rtw89_use_chanctx=1 and upgrade firmware. 

I think this could be a better user experience of kernel.

Ping-Ke
Ping-Ke Shih Aug. 9, 2022, 11 a.m. UTC | #4 
On Thu, 2022-06-23 at 11:50 +0800, Ping-Ke Shih wrote:
> On Mon, 2022-06-20 at 12:34 +0000, Ping-Ke Shih wrote:
> > On Mon, 2022-06-20 at 13:09 +0300, Kalle Valo wrote:
> > > Ping-Ke Shih <pkshih@realtek.com> writes:
> > > 
> > > > From: Zong-Zhe Yang <kevin_yang@realtek.com>
> > > > 
> > > > If a chip is configured to support mac80211 chanctx ops, we avoid
> > > > using older FW that does not support HW scan to make mac80211 stack
> > > > handle scanning as expected.
> > > > 
> > > > Signed-off-by: Zong-Zhe Yang <kevin_yang@realtek.com>
> > > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> > > > ---
> > > >  drivers/net/wireless/realtek/rtw89/fw.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/wireless/realtek/rtw89/fw.c
> > > > b/drivers/net/wireless/realtek/rtw89/fw.c
> > > > index 0e12629f789c3..a47451dc9d81d 100644
> > > > --- a/drivers/net/wireless/realtek/rtw89/fw.c
> > > > +++ b/drivers/net/wireless/realtek/rtw89/fw.c
> > > > @@ -250,6 +250,7 @@ static void rtw89_fw_recognize_features(struct rtw89_dev *rtwdev)
> > > >  
> > > >  int rtw89_fw_recognize(struct rtw89_dev *rtwdev)
> > > >  {
> > > > +	const struct rtw89_chip_info *chip = rtwdev->chip;
> > > >  	int ret;
> > > >  
> > > >  	ret = __rtw89_fw_recognize(rtwdev, RTW89_FW_NORMAL);
> > > > @@ -261,6 +262,13 @@ int rtw89_fw_recognize(struct rtw89_dev *rtwdev)
> > > >  
> > > >  	rtw89_fw_recognize_features(rtwdev);
> > > >  
> > > > +	if (chip->support_chanctx_num != 0 &&
> > > > +	    !RTW89_CHK_FW_FEATURE(SCAN_OFFLOAD, &rtwdev->fw)) {
> > > > +		rtw89_err(rtwdev,
> > > > +			  "require newer FW to support HW scan for chanctx\n");
> > > > +		return -ENOENT;
> > > > +	}
> > > 
> > > So if the user has not update the firmware a kernel upgrade will break
> > > their internet? That's not good, we should not break existing setups. So
> > > what firmware version is required?
> > > 
> > 
> > Firmware version 0.13.35.0 is required. The firmware has been in
> > linux-firmware repository on 2022-02-18. I think people being able
> > to update kernel can update firmware as well.
> > 
> > The alternative ways could be
> > 1. add a module parameter, like no_channel_context. We can add a
> >    prompt to note people can set it to 1 for old firmware.
> > 
> > 2. wait version of request_firmware() as first step of pci probe.
> >    The probe could cost longer time, because currently we use
> >    request_firmware_nowait() and continue to initialize in parallel.
> >    More, hw->priv isn't allocated at that moment, so it could be not
> >    so straightforward.
> > 
> 
> To prevent break users' existing setups, modified approach 1 is adopted
> by patchset v2. We add a module parameter rtw89_use_chanctx, and disable
> it by default. So, users' setups with old firmware can still work.
> 
> If a user wants channel context to support concurrency, he can set
> rtw89_use_chanctx=1 and upgrade firmware. 
> 
> I think this could be a better user experience of kernel.
> 

We have a better idea that load firmware header and parse firmware
features when probing before ieee80211_register_hw(). If firmware
doesn't support hw_scan, it still can work as original. So, users
don't need to update firmware or set module parameters.

I have sent v3 with this idea that is friendly to users.

Ping-Ke
Kalle Valo Sept. 2, 2022, 8:28 a.m. UTC | #5 
Ping-Ke Shih <pkshih@realtek.com> writes:

>> > > So if the user has not update the firmware a kernel upgrade will break
>> > > their internet? That's not good, we should not break existing setups. So
>> > > what firmware version is required?
>> > > 
>> > 
>> > Firmware version 0.13.35.0 is required. The firmware has been in
>> > linux-firmware repository on 2022-02-18. I think people being able
>> > to update kernel can update firmware as well.
>> > 
>> > The alternative ways could be
>> > 1. add a module parameter, like no_channel_context. We can add a
>> >    prompt to note people can set it to 1 for old firmware.
>> > 
>> > 2. wait version of request_firmware() as first step of pci probe.
>> >    The probe could cost longer time, because currently we use
>> >    request_firmware_nowait() and continue to initialize in parallel.
>> >    More, hw->priv isn't allocated at that moment, so it could be not
>> >    so straightforward.
>> > 
>> 
>> To prevent break users' existing setups, modified approach 1 is adopted
>> by patchset v2. We add a module parameter rtw89_use_chanctx, and disable
>> it by default. So, users' setups with old firmware can still work.
>> 
>> If a user wants channel context to support concurrency, he can set
>> rtw89_use_chanctx=1 and upgrade firmware. 
>> 
>> I think this could be a better user experience of kernel.
>> 
>
> We have a better idea that load firmware header and parse firmware
> features when probing before ieee80211_register_hw(). If firmware
> doesn't support hw_scan, it still can work as original. So, users
> don't need to update firmware or set module parameters.
>
> I have sent v3 with this idea that is friendly to users.

Sorry for the delay, I was not able to look at this in detail earlier.

Indeed v3 is much better approach and actually what we prefer in
upstream. iwlwifi, ath10k and ath11k do something similar. Forcing users
to update the firmware or fiddling with module parameters when updating
the kernel is bad from user's point of view.
diff mbox series

Patch

diff --git a/drivers/net/wireless/realtek/rtw89/fw.c b/drivers/net/wireless/realtek/rtw89/fw.c
index 0e12629f789c3..a47451dc9d81d 100644
--- a/drivers/net/wireless/realtek/rtw89/fw.c
+++ b/drivers/net/wireless/realtek/rtw89/fw.c
@@ -250,6 +250,7 @@  static void rtw89_fw_recognize_features(struct rtw89_dev *rtwdev)
 
 int rtw89_fw_recognize(struct rtw89_dev *rtwdev)
 {
+	const struct rtw89_chip_info *chip = rtwdev->chip;
 	int ret;
 
 	ret = __rtw89_fw_recognize(rtwdev, RTW89_FW_NORMAL);
@@ -261,6 +262,13 @@  int rtw89_fw_recognize(struct rtw89_dev *rtwdev)
 
 	rtw89_fw_recognize_features(rtwdev);
 
+	if (chip->support_chanctx_num != 0 &&
+	    !RTW89_CHK_FW_FEATURE(SCAN_OFFLOAD, &rtwdev->fw)) {
+		rtw89_err(rtwdev,
+			  "require newer FW to support HW scan for chanctx\n");
+		return -ENOENT;
+	}
+
 	return 0;
 }