| Message ID | 20210717204057.67495-3-martin.blumenstingl@googlemail.com |
|---|---|
| State | New |
| Headers | show |
| Series | rtw88: prepare locking for SDIO support | expand |
> -----Original Message----- > From: Martin Blumenstingl [mailto:martin.blumenstingl@googlemail.com] > Sent: Sunday, July 18, 2021 4:41 AM > To: linux-wireless@vger.kernel.org > Cc: tony0620emma@gmail.com; kvalo@codeaurora.org; johannes@sipsolutions.net; netdev@vger.kernel.org; > linux-kernel@vger.kernel.org; Neo Jou; Jernej Skrabec; Martin Blumenstingl > Subject: [PATCH RFC v1 2/7] rtw88: Use rtw_iterate_vifs where the iterator reads or writes registers > > Upcoming SDIO support may sleep in the read/write handlers. Switch > all users of rtw_iterate_vifs_atomic() which are either reading or > writing a register to rtw_iterate_vifs(). > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > drivers/net/wireless/realtek/rtw88/main.c | 6 +++--- > drivers/net/wireless/realtek/rtw88/ps.c | 2 +- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c > index c6364837e83b..207161a8f5bd 100644 > --- a/drivers/net/wireless/realtek/rtw88/main.c > +++ b/drivers/net/wireless/realtek/rtw88/main.c > @@ -229,8 +229,8 @@ static void rtw_watch_dog_work(struct work_struct *work) > rtw_phy_dynamic_mechanism(rtwdev); > > data.rtwdev = rtwdev; > - /* use atomic version to avoid taking local->iflist_mtx mutex */ > - rtw_iterate_vifs_atomic(rtwdev, rtw_vif_watch_dog_iter, &data); > + > + rtw_iterate_vifs(rtwdev, rtw_vif_watch_dog_iter, &data); You revert the fix of [1]. I think we can move out rtw_chip_cfg_csi_rate() from rtw_dynamic_csi_rate(), and add/set a field cfg_csi_rate to itera data. Then, we do rtw_chip_cfg_csi_rate() outside iterate function. Therefore, we can keep the atomic version of iterate_vifs. [1] https://lore.kernel.org/linux-wireless/1556886547-23632-1-git-send-email-sgruszka@redhat.com/ -- Ping-Ke
Hello Ping-Ke, On Mon, Jul 19, 2021 at 7:47 AM Pkshih <pkshih@realtek.com> wrote: > > > > > -----Original Message----- > > From: Martin Blumenstingl [mailto:martin.blumenstingl@googlemail.com] > > Sent: Sunday, July 18, 2021 4:41 AM > > To: linux-wireless@vger.kernel.org > > Cc: tony0620emma@gmail.com; kvalo@codeaurora.org; johannes@sipsolutions.net; netdev@vger.kernel.org; > > linux-kernel@vger.kernel.org; Neo Jou; Jernej Skrabec; Martin Blumenstingl > > Subject: [PATCH RFC v1 2/7] rtw88: Use rtw_iterate_vifs where the iterator reads or writes registers > > > > Upcoming SDIO support may sleep in the read/write handlers. Switch > > all users of rtw_iterate_vifs_atomic() which are either reading or > > writing a register to rtw_iterate_vifs(). > > > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > --- > > drivers/net/wireless/realtek/rtw88/main.c | 6 +++--- > > drivers/net/wireless/realtek/rtw88/ps.c | 2 +- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c > > index c6364837e83b..207161a8f5bd 100644 > > --- a/drivers/net/wireless/realtek/rtw88/main.c > > +++ b/drivers/net/wireless/realtek/rtw88/main.c > > @@ -229,8 +229,8 @@ static void rtw_watch_dog_work(struct work_struct *work) > > rtw_phy_dynamic_mechanism(rtwdev); > > > > data.rtwdev = rtwdev; > > - /* use atomic version to avoid taking local->iflist_mtx mutex */ > > - rtw_iterate_vifs_atomic(rtwdev, rtw_vif_watch_dog_iter, &data); > > + > > + rtw_iterate_vifs(rtwdev, rtw_vif_watch_dog_iter, &data); > > You revert the fix of [1]. Thanks for bringing this to my attention! > I think we can move out rtw_chip_cfg_csi_rate() from rtw_dynamic_csi_rate(), and > add/set a field cfg_csi_rate to itera data. Then, we do rtw_chip_cfg_csi_rate() > outside iterate function. Therefore, we can keep the atomic version of iterate_vifs. just to make sure that I understand this correctly: rtw_iterate_vifs_atomic can be the iterator as it was before inside the iterator func I use something like: iter_data->cfg_csi_rate = rtwvif->bfee.role == RTW_BFEE_SU || rtwvif->bfee.role == RTW_BFEE_MU || iter_data->cfg_csi_rate; (the last iter_data->cfg_csi_rate may read a bit strange, but I think it's needed because there can be multiple interfaces and if any of them has cfg_csi_rate true then we need to remember that) then move the rtw_chip_cfg_csi_rate outside the iterator function, taking iter_data->cfg_csi_rate to decide whether it needs to be called Best regards, Martin
> -----Original Message----- > From: Martin Blumenstingl [mailto:martin.blumenstingl@googlemail.com] > Sent: Monday, July 26, 2021 5:31 AM > To: Pkshih > Cc: linux-wireless@vger.kernel.org; tony0620emma@gmail.com; kvalo@codeaurora.org; > johannes@sipsolutions.net; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Neo Jou; Jernej > Skrabec > Subject: Re: [PATCH RFC v1 2/7] rtw88: Use rtw_iterate_vifs where the iterator reads or writes registers > > Hello Ping-Ke, > > On Mon, Jul 19, 2021 at 7:47 AM Pkshih <pkshih@realtek.com> wrote: > > > > > > > > > -----Original Message----- > > > From: Martin Blumenstingl [mailto:martin.blumenstingl@googlemail.com] > > > Sent: Sunday, July 18, 2021 4:41 AM > > > To: linux-wireless@vger.kernel.org > > > Cc: tony0620emma@gmail.com; kvalo@codeaurora.org; johannes@sipsolutions.net; > netdev@vger.kernel.org; > > > linux-kernel@vger.kernel.org; Neo Jou; Jernej Skrabec; Martin Blumenstingl > > > Subject: [PATCH RFC v1 2/7] rtw88: Use rtw_iterate_vifs where the iterator reads or writes registers > > > > > > Upcoming SDIO support may sleep in the read/write handlers. Switch > > > all users of rtw_iterate_vifs_atomic() which are either reading or > > > writing a register to rtw_iterate_vifs(). > > > > > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > --- > > > drivers/net/wireless/realtek/rtw88/main.c | 6 +++--- > > > drivers/net/wireless/realtek/rtw88/ps.c | 2 +- > > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/net/wireless/realtek/rtw88/main.c > b/drivers/net/wireless/realtek/rtw88/main.c > > > index c6364837e83b..207161a8f5bd 100644 > > > --- a/drivers/net/wireless/realtek/rtw88/main.c > > > +++ b/drivers/net/wireless/realtek/rtw88/main.c > > > @@ -229,8 +229,8 @@ static void rtw_watch_dog_work(struct work_struct *work) > > > rtw_phy_dynamic_mechanism(rtwdev); > > > > > > data.rtwdev = rtwdev; > > > - /* use atomic version to avoid taking local->iflist_mtx mutex */ > > > - rtw_iterate_vifs_atomic(rtwdev, rtw_vif_watch_dog_iter, &data); > > > + > > > + rtw_iterate_vifs(rtwdev, rtw_vif_watch_dog_iter, &data); > > > > You revert the fix of [1]. > Thanks for bringing this to my attention! > > > I think we can move out rtw_chip_cfg_csi_rate() from rtw_dynamic_csi_rate(), and > > add/set a field cfg_csi_rate to itera data. Then, we do rtw_chip_cfg_csi_rate() > > outside iterate function. Therefore, we can keep the atomic version of iterate_vifs. > just to make sure that I understand this correctly: > rtw_iterate_vifs_atomic can be the iterator as it was before > inside the iterator func I use something like: > iter_data->cfg_csi_rate = rtwvif->bfee.role == RTW_BFEE_SU || > rtwvif->bfee.role == RTW_BFEE_MU || iter_data->cfg_csi_rate; > (the last iter_data->cfg_csi_rate may read a bit strange, but I think > it's needed because there can be multiple interfaces and if any of > them has cfg_csi_rate true then we need to remember that) > then move the rtw_chip_cfg_csi_rate outside the iterator function, > taking iter_data->cfg_csi_rate to decide whether it needs to be called > Yes, you understand correctly. For the strange part that you mentioned, how about this? iter_data->cfg_csi_rate |= rtwvif->bfee.role == RTW_BFEE_SU || rtwvif->bfee.role == RTW_BFEE_MU; -- Ping-Ke
diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c index c6364837e83b..207161a8f5bd 100644 --- a/drivers/net/wireless/realtek/rtw88/main.c +++ b/drivers/net/wireless/realtek/rtw88/main.c @@ -229,8 +229,8 @@ static void rtw_watch_dog_work(struct work_struct *work) rtw_phy_dynamic_mechanism(rtwdev); data.rtwdev = rtwdev; - /* use atomic version to avoid taking local->iflist_mtx mutex */ - rtw_iterate_vifs_atomic(rtwdev, rtw_vif_watch_dog_iter, &data); + + rtw_iterate_vifs(rtwdev, rtw_vif_watch_dog_iter, &data); /* fw supports only one station associated to enter lps, if there are * more than two stations associated to the AP, then we can not enter @@ -578,7 +578,7 @@ static void __fw_recovery_work(struct rtw_dev *rtwdev) rtw_iterate_keys_rcu(rtwdev, NULL, rtw_reset_key_iter, rtwdev); rcu_read_unlock(); rtw_iterate_stas_atomic(rtwdev, rtw_reset_sta_iter, rtwdev); - rtw_iterate_vifs_atomic(rtwdev, rtw_reset_vif_iter, rtwdev); + rtw_iterate_vifs(rtwdev, rtw_reset_vif_iter, rtwdev); rtw_enter_ips(rtwdev); } diff --git a/drivers/net/wireless/realtek/rtw88/ps.c b/drivers/net/wireless/realtek/rtw88/ps.c index 3f0ac33156d6..95f9060b083f 100644 --- a/drivers/net/wireless/realtek/rtw88/ps.c +++ b/drivers/net/wireless/realtek/rtw88/ps.c @@ -58,7 +58,7 @@ int rtw_leave_ips(struct rtw_dev *rtwdev) return ret; } - rtw_iterate_vifs_atomic(rtwdev, rtw_restore_port_cfg_iter, rtwdev); + rtw_iterate_vifs(rtwdev, rtw_restore_port_cfg_iter, rtwdev); rtw_coex_ips_notify(rtwdev, COEX_IPS_LEAVE);
Upcoming SDIO support may sleep in the read/write handlers. Switch all users of rtw_iterate_vifs_atomic() which are either reading or writing a register to rtw_iterate_vifs(). Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- drivers/net/wireless/realtek/rtw88/main.c | 6 +++--- drivers/net/wireless/realtek/rtw88/ps.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)