Re: [PATCH v5 09/17] wifi: cc33xx: Add main.c

[Nemanov, Michael]

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.