| Message ID | 20240111170629.1257217-1-benjamin@sipsolutions.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: ath11k: rely on mac80211 debugfs handling for vif | expand |
benjamin@sipsolutions.net writes: > From: Benjamin Berg <benjamin.berg@intel.com> > > mac80211 started to delete debugfs entries in certain cases, causing a > conflict between ath11k also trying to delete them later on. Fix this by > relying on mac80211 to delete the entries when appropriate and adding > them from the vif_add_debugfs handler. > > Fixes: 0a3d898ee9a8 ("wifi: mac80211: add/remove driver debugfs entries as appropriate") > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=218364 > Signed-off-by: Benjamin Berg <benjamin.berg@intel.com> Adding ath11k list so that the whole team sees this.
Kalle Valo <kvalo@kernel.org> writes: > benjamin@sipsolutions.net writes: > >> From: Benjamin Berg <benjamin.berg@intel.com> >> >> mac80211 started to delete debugfs entries in certain cases, causing a >> conflict between ath11k also trying to delete them later on. Fix this by >> relying on mac80211 to delete the entries when appropriate and adding >> them from the vif_add_debugfs handler. >> >> Fixes: 0a3d898ee9a8 ("wifi: mac80211: add/remove driver debugfs entries as appropriate") >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=218364 >> Signed-off-by: Benjamin Berg <benjamin.berg@intel.com> > > Adding ath11k list so that the whole team sees this. Thanks, this patch passes my ath11k tests and I don't see crashes anymore. But what about other drivers, can we trust that they don't have similar problems? I'm just wondering should we consider reverting the mac80211 commit for the time being?
On 1/11/2024 9:06 AM, benjamin@sipsolutions.net wrote: > From: Benjamin Berg <benjamin.berg@intel.com> ... > #ifdef CONFIG_ATH11K_DEBUGFS > + .vif_add_debugfs = ath11k_debugfs_op_add_interface, nit: can we rename to ath11k_debugfs_op_vif_add()? this would follow the convention used by almost all of the other methods to use the name ath11k_<component>_op_<method>() but in this case dropping the redundant "debugfs" from the method I'll submit a separate patch to rename the method below to also align with that naming, ath11k_debugfs_op_sta_add() > .sta_add_debugfs = ath11k_debugfs_sta_op_add, > #endif >
Kalle Valo <kvalo@kernel.org> writes: > benjamin@sipsolutions.net writes: > >> From: Benjamin Berg <benjamin.berg@intel.com> >> >> mac80211 started to delete debugfs entries in certain cases, causing a >> conflict between ath11k also trying to delete them later on. Instead of calling it a conflict I would prefer to clearly document that ath11k was crashing so badly that it was unusable. >> >> Fixes: 0a3d898ee9a8 ("wifi: mac80211: add/remove driver debugfs entries as appropriate") >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=218364 s/Bugzilla:/Closes:/
On Fri, 2024-01-12 at 10:24 +0200, Kalle Valo wrote: > Kalle Valo <kvalo@kernel.org> writes: > > > benjamin@sipsolutions.net writes: > > > > > From: Benjamin Berg <benjamin.berg@intel.com> > > > > > > mac80211 started to delete debugfs entries in certain cases, > > > causing a > > > conflict between ath11k also trying to delete them later on. Fix > > > this by > > > relying on mac80211 to delete the entries when appropriate and > > > adding > > > them from the vif_add_debugfs handler. > > > > > > Fixes: 0a3d898ee9a8 ("wifi: mac80211: add/remove driver debugfs > > > entries as appropriate") > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=218364 > > > Signed-off-by: Benjamin Berg <benjamin.berg@intel.com> > > > > Adding ath11k list so that the whole team sees this. > > Thanks, this patch passes my ath11k tests and I don't see crashes > anymore. But what about other drivers, can we trust that they don't > have > similar problems? I'm just wondering should we consider reverting the > mac80211 commit for the time being? Good question. I did have a look originally and I just went over all drivers (i.e. debugfs_remove calls). None of these seem to be vif related. As such, I would say we could just go ahead with this patch and leave in the fix. That said, we could also revert 0a3d898ee9a8 for now. I don't think that has any severe side effects other than a warning from debugfs reappearing again for iwlwifi. Benjamin
diff --git a/drivers/net/wireless/ath/ath11k/core.h b/drivers/net/wireless/ath/ath11k/core.h index 7e3b6779f4e9..02e160d831be 100644 --- a/drivers/net/wireless/ath/ath11k/core.h +++ b/drivers/net/wireless/ath/ath11k/core.h @@ -368,10 +368,6 @@ struct ath11k_vif { struct ieee80211_chanctx_conf chanctx; struct ath11k_arp_ns_offload arp_ns_offload; struct ath11k_rekey_data rekey_data; - -#ifdef CONFIG_ATH11K_DEBUGFS - struct dentry *debugfs_twt; -#endif /* CONFIG_ATH11K_DEBUGFS */ }; struct ath11k_vif_iter { diff --git a/drivers/net/wireless/ath/ath11k/debugfs.c b/drivers/net/wireless/ath/ath11k/debugfs.c index a847bc0d50c0..af6fdb4b6497 100644 --- a/drivers/net/wireless/ath/ath11k/debugfs.c +++ b/drivers/net/wireless/ath/ath11k/debugfs.c @@ -1894,35 +1894,30 @@ static const struct file_operations ath11k_fops_twt_resume_dialog = { .open = simple_open }; -void ath11k_debugfs_add_interface(struct ath11k_vif *arvif) +void ath11k_debugfs_op_add_interface(struct ieee80211_hw *hw, + struct ieee80211_vif *vif) { + struct ath11k_vif *arvif = ath11k_vif_to_arvif(vif); struct ath11k_base *ab = arvif->ar->ab; + struct dentry *debugfs_twt; if (arvif->vif->type != NL80211_IFTYPE_AP && !(arvif->vif->type == NL80211_IFTYPE_STATION && test_bit(WMI_TLV_SERVICE_STA_TWT, ab->wmi_ab.svc_map))) return; - arvif->debugfs_twt = debugfs_create_dir("twt", - arvif->vif->debugfs_dir); - debugfs_create_file("add_dialog", 0200, arvif->debugfs_twt, + debugfs_twt = debugfs_create_dir("twt", + arvif->vif->debugfs_dir); + debugfs_create_file("add_dialog", 0200, debugfs_twt, arvif, &ath11k_fops_twt_add_dialog); - debugfs_create_file("del_dialog", 0200, arvif->debugfs_twt, + debugfs_create_file("del_dialog", 0200, debugfs_twt, arvif, &ath11k_fops_twt_del_dialog); - debugfs_create_file("pause_dialog", 0200, arvif->debugfs_twt, + debugfs_create_file("pause_dialog", 0200, debugfs_twt, arvif, &ath11k_fops_twt_pause_dialog); - debugfs_create_file("resume_dialog", 0200, arvif->debugfs_twt, + debugfs_create_file("resume_dialog", 0200, debugfs_twt, arvif, &ath11k_fops_twt_resume_dialog); } -void ath11k_debugfs_remove_interface(struct ath11k_vif *arvif) -{ - if (!arvif->debugfs_twt) - return; - - debugfs_remove_recursive(arvif->debugfs_twt); - arvif->debugfs_twt = NULL; -} diff --git a/drivers/net/wireless/ath/ath11k/debugfs.h b/drivers/net/wireless/ath/ath11k/debugfs.h index 44d15845f39a..325151ec8d4f 100644 --- a/drivers/net/wireless/ath/ath11k/debugfs.h +++ b/drivers/net/wireless/ath/ath11k/debugfs.h @@ -307,8 +307,8 @@ static inline int ath11k_debugfs_rx_filter(struct ath11k *ar) return ar->debug.rx_filter; } -void ath11k_debugfs_add_interface(struct ath11k_vif *arvif); -void ath11k_debugfs_remove_interface(struct ath11k_vif *arvif); +void ath11k_debugfs_op_add_interface(struct ieee80211_hw *hw, + struct ieee80211_vif *vif); void ath11k_debugfs_add_dbring_entry(struct ath11k *ar, enum wmi_direct_buffer_module id, enum ath11k_dbg_dbr_event event, @@ -387,14 +387,6 @@ static inline int ath11k_debugfs_get_fw_stats(struct ath11k *ar, return 0; } -static inline void ath11k_debugfs_add_interface(struct ath11k_vif *arvif) -{ -} - -static inline void ath11k_debugfs_remove_interface(struct ath11k_vif *arvif) -{ -} - static inline void ath11k_debugfs_add_dbring_entry(struct ath11k *ar, enum wmi_direct_buffer_module id, diff --git a/drivers/net/wireless/ath/ath11k/mac.c b/drivers/net/wireless/ath/ath11k/mac.c index db241589424d..cd99246303dd 100644 --- a/drivers/net/wireless/ath/ath11k/mac.c +++ b/drivers/net/wireless/ath/ath11k/mac.c @@ -6756,13 +6756,6 @@ static int ath11k_mac_op_add_interface(struct ieee80211_hw *hw, goto err; } - /* In the case of hardware recovery, debugfs files are - * not deleted since ieee80211_ops.remove_interface() is - * not invoked. In such cases, try to delete the files. - * These will be re-created later. - */ - ath11k_debugfs_remove_interface(arvif); - memset(arvif, 0, sizeof(*arvif)); arvif->ar = ar; @@ -6939,8 +6932,6 @@ static int ath11k_mac_op_add_interface(struct ieee80211_hw *hw, ath11k_dp_vdev_tx_attach(ar, arvif); - ath11k_debugfs_add_interface(arvif); - if (vif->type != NL80211_IFTYPE_MONITOR && test_bit(ATH11K_FLAG_MONITOR_CONF_ENABLED, &ar->monitor_flags)) { ret = ath11k_mac_monitor_vdev_create(ar); @@ -7056,8 +7047,6 @@ static void ath11k_mac_op_remove_interface(struct ieee80211_hw *hw, /* Recalc txpower for remaining vdev */ ath11k_mac_txpower_recalc(ar); - ath11k_debugfs_remove_interface(arvif); - /* TODO: recal traffic pause state based on the available vdevs */ mutex_unlock(&ar->conf_mutex); @@ -9153,6 +9142,7 @@ static const struct ieee80211_ops ath11k_ops = { #endif #ifdef CONFIG_ATH11K_DEBUGFS + .vif_add_debugfs = ath11k_debugfs_op_add_interface, .sta_add_debugfs = ath11k_debugfs_sta_op_add, #endif