| 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
Hello rtw88 and mac80211 maintainers/contributors, there is an ongoing effort where Jernej and I are working on adding SDIO support to the rtw88 driver. The hardware we use at the moment is RTL8822BS and RTL8822CS. We are at a point where scanning, assoc etc. works (though it's not fast yet, in my tests I got ~6Mbit/s in either direction). This series contains some preparation work for adding SDIO support. While testing our changes we found that there are some "scheduling while atomic" errors in the kernel log. These are due to the fact that the SDIO accessors (sdio_readb, sdio_writeb and friends) may sleep internally. Some background on why SDIO access (for example: sdio_writeb) cannot be done with a spinlock held (this is a copy from my previous mail, see [0]): - when using for example sdio_writeb the MMC subsystem in Linux prepares a so-called MMC request - this request is submitted to the MMC host controller hardware - the host controller hardware forwards the MMC request to the card - the card signals when it's done processing the request - the MMC subsystem in Linux waits for the card to signal that it's done processing the request in mmc_wait_for_req_done() -> this uses wait_for_completion() internally, which might sleep (which is not allowed while a spinlock is held) Based on Ping-Ke's suggestion I came up with the code in this series. The goal is to use non-atomic locking for all register access in the rtw88 driver. One patch adds a new function to mac80211 which did not have a "non-atomic" version of it's "atomic" counterpart yet. As mentioned before I don't have any rtw88 PCIe device so I am unable to test on that hardware. I am sending this as an RFC series since I am new to the mac80211 subsystem as well as the rtw88 driver. So any kind of feedback is very welcome! The actual changes for adding SDIO support will be sent separately in the future. [0] https://lore.kernel.org/linux-wireless/CAFBinCDMPPJ7qW7xTkep1Trg+zP0B9Jxei6sgjqmF4NDA1JAhQ@mail.gmail.com/ Martin Blumenstingl (7): mac80211: Add stations iterator where the iterator function may sleep rtw88: Use rtw_iterate_vifs where the iterator reads or writes registers rtw88: Use rtw_iterate_stas where the iterator reads or writes registers rtw88: Replace usage of rtw_iterate_keys_rcu() with rtw_iterate_keys() rtw88: Configure the registers from rtw_bf_assoc() outside the RCU lock rtw88: hci: Convert rf_lock from a spinlock to a mutex rtw88: fw: Convert h2c.lock from a spinlock to a mutex drivers/net/wireless/realtek/rtw88/bf.c | 8 ++++++-- drivers/net/wireless/realtek/rtw88/fw.c | 14 +++++++------- drivers/net/wireless/realtek/rtw88/hci.h | 11 ++++------- drivers/net/wireless/realtek/rtw88/mac80211.c | 2 +- drivers/net/wireless/realtek/rtw88/main.c | 16 +++++++--------- drivers/net/wireless/realtek/rtw88/main.h | 4 ++-- drivers/net/wireless/realtek/rtw88/phy.c | 4 ++-- drivers/net/wireless/realtek/rtw88/ps.c | 2 +- drivers/net/wireless/realtek/rtw88/util.h | 4 ++-- drivers/net/wireless/realtek/rtw88/wow.c | 2 +- include/net/mac80211.h | 18 ++++++++++++++++++ net/mac80211/util.c | 13 +++++++++++++ 12 files changed, 64 insertions(+), 34 deletions(-)