Message ID | 20210717204057.67495-1-martin.blumenstingl@googlemail.com |
---|---|
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 1/7] mac80211: Add stations iterator where the iterator function may sleep > > ieee80211_iterate_active_interfaces() and > ieee80211_iterate_active_interfaces_atomic() already exist, where the > former allows the iterator function to sleep. Add > ieee80211_iterate_stations() which is similar to > ieee80211_iterate_stations_atomic() but allows the iterator to sleep. > This is needed for adding SDIO support to the rtw88 driver. Some > interators there are reading or writing registers. With the SDIO ops > (sdio_readb, sdio_writeb and friends) this means that the iterator > function may sleep. > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > include/net/mac80211.h | 18 ++++++++++++++++++ > net/mac80211/util.c | 13 +++++++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/include/net/mac80211.h b/include/net/mac80211.h > index d8a1d09a2141..77de89690008 100644 > --- a/include/net/mac80211.h > +++ b/include/net/mac80211.h > @@ -5575,6 +5575,24 @@ void ieee80211_iterate_active_interfaces_mtx(struct ieee80211_hw *hw, > struct ieee80211_vif *vif), > void *data); > > +/** > + * ieee80211_iterate_stations_atomic - iterate stations ieee80211_iterate_stations - ... [...] -- Ping-Ke
> -----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
> -----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 5/7] rtw88: Configure the registers from rtw_bf_assoc() outside the RCU lock > > Upcoming SDIO support may sleep in the read/write handlers. Configure > the chip's BFEE configuration set from rtw_bf_assoc() outside the > rcu_read_lock section to prevent a "scheduling while atomic" issue. > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > drivers/net/wireless/realtek/rtw88/bf.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtw88/bf.c b/drivers/net/wireless/realtek/rtw88/bf.c > index aff70e4ae028..06034d5d6f6c 100644 > --- a/drivers/net/wireless/realtek/rtw88/bf.c > +++ b/drivers/net/wireless/realtek/rtw88/bf.c > @@ -39,6 +39,7 @@ void rtw_bf_assoc(struct rtw_dev *rtwdev, struct ieee80211_vif *vif, > struct ieee80211_sta_vht_cap *vht_cap; > struct ieee80211_sta_vht_cap *ic_vht_cap; > const u8 *bssid = bss_conf->bssid; > + bool config_bfee = false; > u32 sound_dim; > u8 i; > The rcu_read_lock() in this function is used to access ieee80211_find_sta() and protect 'sta'. A simple way is to shrink the critical section, like: rcu_read_lock(); sta = ieee80211_find_sta(vif, bssid); if (!sta) { rtw_warn(rtwdev, "failed to find station entry for bss %pM\n", bssid); rcu_read_unlock(); } vht_cap = &sta->vht_cap; rcu_read_unlock(); -- Ping-Ke
> -----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 0/7] rtw88: prepare locking for SDIO support > [...] I have reviewed patchset v1. But, please wait a moment before sending v2 to see if other experts have better suggestions. -- Ping-Ke
> > +/** > + * ieee80211_iterate_stations_atomic - iterate stations Copy/paste issue, as PK pointed out too. > + * > + * This function iterates over all stations associated with a given > + * hardware that are currently uploaded to the driver and calls the callback > + * function for them. > + * This function allows the iterator function to sleep, when the iterator > + * function is atomic @ieee80211_iterate_stations_atomic can be used. > I have no real objections to this, but I think you should carefully document something like "the driver must not call this with a lock held that it can also take in response to callbacks from mac80211, and it must not call this within callbacks made by mac80211" or something like that, because both of those things are going to cause deadlocks. johannes
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
Hi Ping-Ke, On Mon, Jul 19, 2021 at 7:47 AM Pkshih <pkshih@realtek.com> wrote: [...] > The rcu_read_lock() in this function is used to access ieee80211_find_sta() and protect 'sta'. > A simple way is to shrink the critical section, like: > > rcu_read_lock(); > > sta = ieee80211_find_sta(vif, bssid); > if (!sta) { > rtw_warn(rtwdev, "failed to find station entry for bss %pM\n", > bssid); > rcu_read_unlock(); > } > > vht_cap = &sta->vht_cap; > > rcu_read_unlock(); I agree that reducing the amount of code under the lock will help my use-case as well in your code-example I am wondering if we should change struct ieee80211_sta_vht_cap *vht_cap; vht_cap = &sta->vht_cap; to struct ieee80211_sta_vht_cap vht_cap; vht_cap = sta->vht_cap; My thinking is that ieee80211_sta may be freed in parallel to this code running. If that cannot happen then your code will be fine. So I am hoping that you can also share your thoughts on this one. Thank you and best regards, Martin
> -----Original Message----- > From: Martin Blumenstingl [mailto:martin.blumenstingl@googlemail.com] > Sent: Monday, July 26, 2021 5:36 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 5/7] rtw88: Configure the registers from rtw_bf_assoc() outside the RCU lock > > Hi Ping-Ke, > > On Mon, Jul 19, 2021 at 7:47 AM Pkshih <pkshih@realtek.com> wrote: > [...] > > The rcu_read_lock() in this function is used to access ieee80211_find_sta() and protect 'sta'. > > A simple way is to shrink the critical section, like: > > > > rcu_read_lock(); > > > > sta = ieee80211_find_sta(vif, bssid); > > if (!sta) { > > rtw_warn(rtwdev, "failed to find station entry for bss %pM\n", > > bssid); > > rcu_read_unlock(); > > } > > > > vht_cap = &sta->vht_cap; > > > > rcu_read_unlock(); > I agree that reducing the amount of code under the lock will help my > use-case as well > in your code-example I am wondering if we should change > struct ieee80211_sta_vht_cap *vht_cap; > vht_cap = &sta->vht_cap; > to > struct ieee80211_sta_vht_cap vht_cap; > vht_cap = sta->vht_cap; > > My thinking is that ieee80211_sta may be freed in parallel to this code running. > If that cannot happen then your code will be fine. > > So I am hoping that you can also share your thoughts on this one. > When we enter rtw_bf_assoc(), the mutex rtwdev->mutex is held; as well as rtw_sta_add()/rtw_sta_remove(). So, I think it cannot happen that ieee80211_sta was freed in parallel. -- Ping-Ke