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

["Nemanov, Michael" <michael.nemanov@ti.com>]

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.