| Message ID | c64fe6a4-b48c-4a80-9d6c-5c90fb7f7bbd@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: rtw89: Add support for USB devices | expand |
Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote: > Disable rtw89_ps_power_mode_change() and rtw89_mac_send_rpwm() for > USB because they are called in atomic context and accessing hardware > registers results in "scheduling while atomic" errors. I feel rtw89_ps_power_mode_change() should be not in atomic context. Please check this. > > Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> > --- > drivers/net/wireless/realtek/rtw89/mac.c | 3 +++ > drivers/net/wireless/realtek/rtw89/ps.c | 3 +++ > 2 files changed, 6 insertions(+) > > diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c > index a316864ad137..1a03355b340f 100644 > --- a/drivers/net/wireless/realtek/rtw89/mac.c > +++ b/drivers/net/wireless/realtek/rtw89/mac.c > @@ -1338,6 +1338,9 @@ static void rtw89_mac_send_rpwm(struct rtw89_dev *rtwdev, > { > u16 request; > > + if (rtwdev->hci.type == RTW89_HCI_TYPE_USB) I think SDIO devices have the same problems as USB, so I prefer to return if "!= PCIE". > + return; > + > spin_lock_bh(&rtwdev->rpwm_lock); > > request = rtw89_read16(rtwdev, R_AX_RPWM); > diff --git a/drivers/net/wireless/realtek/rtw89/ps.c b/drivers/net/wireless/realtek/rtw89/ps.c > index ac46a7baa00d..edff9f8e1016 100644 > --- a/drivers/net/wireless/realtek/rtw89/ps.c > +++ b/drivers/net/wireless/realtek/rtw89/ps.c > @@ -56,6 +56,9 @@ static void rtw89_ps_power_mode_change_with_hci(struct rtw89_dev *rtwdev, > > static void rtw89_ps_power_mode_change(struct rtw89_dev *rtwdev, bool enter) > { > + if (rtwdev->hci.type == RTW89_HCI_TYPE_USB) > + return; > + > if (rtwdev->chip->low_power_hci_modes & BIT(rtwdev->ps_mode) && > !test_bit(RTW89_FLAG_WOWLAN, rtwdev->flags)) > rtw89_ps_power_mode_change_with_hci(rtwdev, enter); > -- > 2.49.0
On 13/05/2025 06:27, Ping-Ke Shih wrote: > Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote: >> Disable rtw89_ps_power_mode_change() and rtw89_mac_send_rpwm() for >> USB because they are called in atomic context and accessing hardware >> registers results in "scheduling while atomic" errors. > > I feel rtw89_ps_power_mode_change() should be not in atomic context. > Please check this. > I think you're right, only rtw89_mac_send_rpwm() is called in atomic context. rtw89_ps_power_mode_change() is disabled for other reasons: 1) It calls rtw89_mac_power_mode_change(), which prints errors when rtw89_mac_send_rpwm() is disabled. 2) With RTL8852CU it calls rtw89_ps_power_mode_change_with_hci() which calls napi_schedule(). That results in dereferencing a null pointer. >> >> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> >> --- >> drivers/net/wireless/realtek/rtw89/mac.c | 3 +++ >> drivers/net/wireless/realtek/rtw89/ps.c | 3 +++ >> 2 files changed, 6 insertions(+) >> >> diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c >> index a316864ad137..1a03355b340f 100644 >> --- a/drivers/net/wireless/realtek/rtw89/mac.c >> +++ b/drivers/net/wireless/realtek/rtw89/mac.c >> @@ -1338,6 +1338,9 @@ static void rtw89_mac_send_rpwm(struct rtw89_dev *rtwdev, >> { >> u16 request; >> >> + if (rtwdev->hci.type == RTW89_HCI_TYPE_USB) > > I think SDIO devices have the same problems as USB, so I prefer to return > if "!= PCIE". > >> + return; >> + >> spin_lock_bh(&rtwdev->rpwm_lock); >> >> request = rtw89_read16(rtwdev, R_AX_RPWM); >> diff --git a/drivers/net/wireless/realtek/rtw89/ps.c b/drivers/net/wireless/realtek/rtw89/ps.c >> index ac46a7baa00d..edff9f8e1016 100644 >> --- a/drivers/net/wireless/realtek/rtw89/ps.c >> +++ b/drivers/net/wireless/realtek/rtw89/ps.c >> @@ -56,6 +56,9 @@ static void rtw89_ps_power_mode_change_with_hci(struct rtw89_dev *rtwdev, >> >> static void rtw89_ps_power_mode_change(struct rtw89_dev *rtwdev, bool enter) >> { >> + if (rtwdev->hci.type == RTW89_HCI_TYPE_USB) >> + return; >> + >> if (rtwdev->chip->low_power_hci_modes & BIT(rtwdev->ps_mode) && >> !test_bit(RTW89_FLAG_WOWLAN, rtwdev->flags)) >> rtw89_ps_power_mode_change_with_hci(rtwdev, enter); >> -- >> 2.49.0 >
Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote: > On 13/05/2025 06:27, Ping-Ke Shih wrote: > > Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote: > >> Disable rtw89_ps_power_mode_change() and rtw89_mac_send_rpwm() for > >> USB because they are called in atomic context and accessing hardware > >> registers results in "scheduling while atomic" errors. > > > > I feel rtw89_ps_power_mode_change() should be not in atomic context. > > Please check this. > > > > I think you're right, only rtw89_mac_send_rpwm() is called in atomic > context. rtw89_ps_power_mode_change() is disabled for other reasons: The rtw89_ps_power_mode_change() is to enter deep power save mode, which can save more power save but need to leave the mode before you are going to do IO/TX/RX. That means if you totally disable it, code flow would be simpler, but power consumption would be higher. Since this is the first patches to enable USB, I suggest you can disable it totally for now, and add it later. > > 1) It calls rtw89_mac_power_mode_change(), which prints errors when > rtw89_mac_send_rpwm() is disabled. > > 2) With RTL8852CU it calls rtw89_ps_power_mode_change_with_hci() > which calls napi_schedule(). That results in dereferencing a null > pointer. I see. Your implementation doesn't use NAPI for USB devices. The rtw89_ps_power_mode_change_with_hci() is to switch IO address for PCI devices in deep power save mode, and USB devices don't need this. I think we can run this only if HCI is PCI.
diff --git a/drivers/net/wireless/realtek/rtw89/mac.c b/drivers/net/wireless/realtek/rtw89/mac.c index a316864ad137..1a03355b340f 100644 --- a/drivers/net/wireless/realtek/rtw89/mac.c +++ b/drivers/net/wireless/realtek/rtw89/mac.c @@ -1338,6 +1338,9 @@ static void rtw89_mac_send_rpwm(struct rtw89_dev *rtwdev, { u16 request; + if (rtwdev->hci.type == RTW89_HCI_TYPE_USB) + return; + spin_lock_bh(&rtwdev->rpwm_lock); request = rtw89_read16(rtwdev, R_AX_RPWM); diff --git a/drivers/net/wireless/realtek/rtw89/ps.c b/drivers/net/wireless/realtek/rtw89/ps.c index ac46a7baa00d..edff9f8e1016 100644 --- a/drivers/net/wireless/realtek/rtw89/ps.c +++ b/drivers/net/wireless/realtek/rtw89/ps.c @@ -56,6 +56,9 @@ static void rtw89_ps_power_mode_change_with_hci(struct rtw89_dev *rtwdev, static void rtw89_ps_power_mode_change(struct rtw89_dev *rtwdev, bool enter) { + if (rtwdev->hci.type == RTW89_HCI_TYPE_USB) + return; + if (rtwdev->chip->low_power_hci_modes & BIT(rtwdev->ps_mode) && !test_bit(RTW89_FLAG_WOWLAN, rtwdev->flags)) rtw89_ps_power_mode_change_with_hci(rtwdev, enter);
Disable rtw89_ps_power_mode_change() and rtw89_mac_send_rpwm() for USB because they are called in atomic context and accessing hardware registers results in "scheduling while atomic" errors. Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> --- drivers/net/wireless/realtek/rtw89/mac.c | 3 +++ drivers/net/wireless/realtek/rtw89/ps.c | 3 +++ 2 files changed, 6 insertions(+)