| Message ID | 20220617084954.61261-14-pkshih@realtek.com |
|---|---|
| State | New |
| Headers | show |
| Series | rtw89: support channel context | expand |
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?
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
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
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
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 --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; }