On 11/8/2024 1:42 PM, Johannes Berg wrote:
+static void cc33xx_rc_update_work(struct work_struct *work)
+{
+ struct cc33xx_vif *wlvif = container_of(work, struct cc33xx_vif,
+ rc_update_work);
+ struct cc33xx *cc = wlvif->cc;
+ struct ieee80211_vif *vif = cc33xx_wlvif_to_vif(wlvif);
+
+ mutex_lock(&cc->mutex);
Given the way the wiphy mutex now works, I'd strongly recommend not
having your own mutex any more - it's a huge simplification in a lot of
places, and there's very little downside since everything coming from
higher layers holds the wiphy mutex already (and almost certainly needs
to acquire your own mutex.)
Yeah I see how it simplifies things. I'll get rid of cc->mutex and use
wiphy_lock() for whatever code that is not called exclusively from
ieee80211_ops.
+static void cc33xx_recovery_work(struct work_struct *work)
+{
+ struct cc33xx *cc = container_of(work, struct cc33xx, recovery_work);
+ struct cc33xx_vif *wlvif;
+ struct ieee80211_vif *vif;
+
+ cc33xx_notice("CC33xx driver attempting recovery");
+
+ if (cc->conf.core.no_recovery) {
+ cc33xx_info("Recovery disabled by configuration, driver will not restart.");
+ return;
+ }
+
+ if (test_bit(CC33XX_FLAG_DRIVER_REMOVED, &cc->flags)) {
+ cc33xx_info("Driver being removed, recovery disabled");
+ return;
+ }
+
+ cc->state = CC33XX_STATE_RESTARTING;
+ set_bit(CC33XX_FLAG_RECOVERY_IN_PROGRESS, &cc->flags);
+
+ mutex_lock(&cc->mutex);
+ while (!list_empty(&cc->wlvif_list)) {
+ wlvif = list_first_entry(&cc->wlvif_list,
+ struct cc33xx_vif, list);
+ vif = cc33xx_wlvif_to_vif(wlvif);
+
+ if (test_bit(WLVIF_FLAG_STA_ASSOCIATED, &wlvif->flags))
+ ieee80211_connection_loss(vif);
+
+ __cc33xx_op_remove_interface(cc, vif, false);
+ }
+ mutex_unlock(&cc->mutex);
+
+ cc33xx_turn_off(cc);
+ msleep(500);
+
+ mutex_lock(&cc->mutex);
+ cc33xx_init_fw(cc);
+ mutex_unlock(&cc->mutex);
+
+ ieee80211_restart_hw(cc->hw);
+
+ mutex_lock(&cc->mutex);
+ clear_bit(CC33XX_FLAG_RECOVERY_IN_PROGRESS, &cc->flags);
+ mutex_unlock(&cc->mutex);
even more so with the awful locking/unlocking/... here (also no need to
unlock to call restart_hw, I think?)
and using both a mutex and atomic ops seems ... odd?
cc33xx_turn_off() is called in the driver remove path so it expects the
mutex to be unlocked while cc33xx_init_fw() touches many driver members
and requires the lock.
OK if i keep it?
Mutex protection for the flags is indeed redundant and will be removed.
+unlock:
+ mutex_unlock(&cc->mutex);
+
+ cancel_work_sync(&wlvif->rc_update_work);
+ cancel_delayed_work_sync(&wlvif->connection_loss_work);
+ cancel_delayed_work_sync(&wlvif->channel_switch_work);
+ cancel_delayed_work_sync(&wlvif->pending_auth_complete_work);
+ cancel_delayed_work_sync(&wlvif->roc_timeout_work);
+
+ mutex_lock(&cc->mutex);
also this kind of thing ... just use wiphy mutex/work
Yeah all this work use cc->mutex so it seems safe, will do.
Thanks and regards,
Michael.
