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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <39af5076-7e96-4968-943d-bb33359f0573@ti.com>
Date: Sun, 1 Dec 2024 19:09:38 +0200
From: "Nemanov, Michael" <michael.nemanov@...com>
To: Johannes Berg <johannes@...solutions.net>, Kalle Valo <kvalo@...nel.org>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Rob
 Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor
 Dooley <conor+dt@...nel.org>, <linux-wireless@...r.kernel.org>,
        <netdev@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
CC: Sabeeh Khan <sabeeh-khan@...com>
Subject: Re: [PATCH v5 09/17] wifi: cc33xx: Add main.c

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.