Received-SPF: pass (google.com: domain of linux-wireless+bounces-4450-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <4536614a-7f4c-420f-b042-f71cf46ba08d@quicinc.com>
Date: Thu, 7 Mar 2024 17:46:45 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 08/11] wifi: ath12k: allow specific mgmt frame tx while
 vdev is not up
Content-Language: en-US
To: Kalle Valo <kvalo@kernel.org>
CC: <ath12k@lists.infradead.org>, <linux-wireless@vger.kernel.org>
References: <20240130040303.370590-1-quic_kangyang@quicinc.com>
 <20240130040303.370590-9-quic_kangyang@quicinc.com>
 <87il31r26k.fsf@kernel.org>
From: Kang Yang <quic_kangyang@quicinc.com>
In-Reply-To: <87il31r26k.fsf@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


On 2/7/2024 12:24 AM, Kalle Valo wrote:
> Kang Yang <quic_kangyang@quicinc.com> writes:
> 
>> In current code, the management frames must be sent after vdev is started.
>> But for P2P device, vdev won't start until P2P negotiation is done. So
>> this logic doesn't make sense for P2P device.
>>
>> Also use ar->conf_mutex to synchronize ar to avoid potential conflicts.
> 
> Please do locking changes in a separate followup patch, I removed this
> in the pending branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=d357dcb3cd0cd3bf57064dc673b5477d884454b3
> 
> I assume you are referring to ar->allocated_vdev_map and access to that
> indeed doesn't look consistent. Most of the places take conf->mutex but
> I see some places which it's accessed without the mutex, for example
> ath12k_mac_get_arvif_by_vdev_id() and ath12k_mac_get_ar_by_vdev_id().
> 


Hi, kalle:

I just take a look for ath12k_mac_get_arvif_by_vdev_id() and 
ath12k_mac_get_ar_by_vdev_id.

Both of them use rcu_read_lock(), so we don't need mutex_lock() anymore, 
right?

I also tried to add mutex_lock() for them, cannot work well:
[ 7804.291286] BUG: sleeping function called from invalid context at 
kernel/locking/mutex.c:585
[ 7804.291349] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 0, 
name: swapper/8
[ 7804.291358] preempt_count: 101, expected: 0
[ 7804.291366] RCU nest depth: 1, expected: 0
[ 7804.291374] 1 lock held by swapper/8/0:
……


> I recommend in the followup patch checking all the access to
> ar->allocated_vdev_map, fixing that if needed and adding documentation
> to struct ath12k::allocated_vdev_map how it's supposed to be protected.
> 


So i think the initial change is sufficient: just use mutex_lock() in
ath12k_mgmt_over_wmi_tx_work().