Re: [PATCH RFC v2 1/4] wifi: ath12k: switch to using wiphy_lock() and remove ar->conf_mutex

[Kalle Valo <kvalo@kernel.org>]

Johannes Berg <johannes@sipsolutions.net> 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