Received-SPF: pass (google.com: domain of linux-wireless+bounces-8012-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <ca9a3b1e-b115-4b28-b0fc-909baab5fdc4@quicinc.com>
Date: Thu, 23 May 2024 22:04:34 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 3/5] wifi: ath12k: Fix Pdev id in HTT stats request for
 WCN7850
To: Kalle Valo <kvalo@kernel.org>
CC: Ramya Gnanasekar <quic_rgnanase@quicinc.com>, <ath12k@lists.infradead.org>,
        <linux-wireless@vger.kernel.org>
References: <20240510050806.514126-1-quic_rgnanase@quicinc.com>
 <20240510050806.514126-4-quic_rgnanase@quicinc.com>
 <87pltfd2bt.fsf@kernel.org>
 <667cea80-9b62-4ca4-9d28-c5940582832a@quicinc.com>
 <87ed9sbscj.fsf@kernel.org>
Content-Language: en-US
From: Lingbo Kong <quic_lingbok@quicinc.com>
In-Reply-To: <87ed9sbscj.fsf@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 2024/5/23 20:55, Kalle Valo wrote:
> Lingbo Kong <quic_lingbok@quicinc.com> writes:
> 
>>>> +struct ath12k_vif *ath12k_mac_get_vif_up(struct ath12k_base *ab)
>>>> +{
>>>> +	struct ath12k *ar;
>>>> +	struct ath12k_pdev *pdev;
>>>> +	struct ath12k_vif *arvif;
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < ab->num_radios; i++) {
>>>> +		pdev = &ab->pdevs[i];
>>>> +		ar = pdev->ar;
>>>> +		list_for_each_entry(arvif, &ar->arvifs, list) {
>>>> +			if (arvif->is_up)
>>>> +				return arvif;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return NULL;
>>>> +}
>>> I'm not seeing any protection here, is that on purpose?
>>
>> you means there need to add lockdep_assert_held(&ar->conf_mutex)?
> 
> I mean what's the locking design here? Is it safe to concurrectly access
> ab->pdevs and arvif->is_up?
> 

oh, i've seen other places use ar->conf_mutex to protect when accessing 
arvif->is_up.

but according to the ath12k_mac_get_vif_up()'s call stack, the 
ath12k_write_htt_stats_reset() and ath12k_open_htt_stats() have already 
executed mutex_lock(&ar->conf_mutex);

so it's best to add lockdep_assert_held(&ar->conf_mutex) here to 
determine whether conf_mutex is obtained.

/lingbo kong