Johannes Berg <johannes@xxxxxxxxxxxxxxxx> writes: > On Wed, 2024-09-18 at 21:10 +0300, Kalle Valo wrote: >> >> There is now a new sparse warning, but to keep this long patch simple the >> labels will be cleaned up in following patches: >> >> drivers/net/wireless/ath/ath12k/mac.c:6635:1: warning: statement expected after label > > I believe this is a compiler error on some compilers (in particular > clang), so you probably need to combine patches a little bit. I had actually already fixed this but forgot to update the commit message, did that now in v3. >> +++ b/drivers/net/wireless/ath/ath12k/debugfs.c >> @@ -15,14 +15,14 @@ static ssize_t ath12k_write_simulate_radar(struct file *file, >> struct ath12k *ar = file->private_data; >> int ret; >> >> - mutex_lock(&ar->conf_mutex); >> + wiphy_lock(ath12k_ar_to_hw(ar)->wiphy); > > I don't think this is an issue here, but I'm not sure if you're aware > that in general, locking the wiphy mutex in some debugfs file handlers > can lead to deadlocks, specifically if those files are later _removed_ > while holding the wiphy lock, as is e.g. the case for station, netdev > and link debugfs files. For this, we have wiphy_locked_debugfs_read() > and wiphy_locked_debugfs_write() [1]. > > [1] you are not required to understand how they are implemented ;-) Thanks, this is good to know. I'm not that worried about that, at least it's not showing up in my tests, so my plan is to fix that in a separate patchset. >> @@ -4310,7 +4301,7 @@ static void ath12k_sta_rc_update_wk(struct work_struct *wk) >> >> spin_unlock_bh(&ar->data_lock); >> >> - mutex_lock(&ar->conf_mutex); >> + wiphy_lock(ath12k_ar_to_hw(ar)->wiphy); > > Baochen already pointed out that you seem to not remove this later in > patch 4, but in this patch alone you also introduce a bug (that lockdep > should point out to you), which is that you cancel_work_sync() this in > ath12k_mac_op_sta_state(), which clearly holds the wiphy mutex already. > > This causes a deadlock. It's fine after patch 4: > >> /* cancel must be done outside the mutex to avoid deadlock */ >> if ((old_state == IEEE80211_STA_NONE && >> new_state == IEEE80211_STA_NOTEXIST)) >> - cancel_work_sync(&arsta->update_wk); >> + wiphy_work_cancel(hw->wiphy, &arsta->update_wk); > > since for wiphy work it's required to ca<ll it with the mutex held./ Excellent find! I fixed it so that I moved this patch before switching to use wiphy_lock(). There should not be a deadlock anymore, hopefully :) > You really should remove that comment too though, and perhaps then the > code can be simplified by moving this to the later code also handling > removal (none->notexist transition). Good point. In v3 I added a new patch for this. Thanks again for the review, I owe you so many beers[1] that it's starting to get difficult to store them ;) [1] https://en.wikipedia.org/wiki/Karhu_(beer) -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
