Received-SPF: pass (google.com: domain of linux-wireless+bounces-4077-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <9f26a75a-df9a-405a-b1ac-2e457eef8416@quicinc.com>
Date: Tue, 27 Feb 2024 19:35:00 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 3/6] wifi: ath11k: implement handling of P2P NoA event
Content-Language: en-US
To: Jeff Johnson <quic_jjohnson@quicinc.com>, <ath11k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>
References: <20240226060203.2040444-1-quic_kangyang@quicinc.com>
 <20240226060203.2040444-4-quic_kangyang@quicinc.com>
 <032375ce-7dc0-4d93-9f26-12a6bb89dbe0@quicinc.com>
From: Kang Yang <quic_kangyang@quicinc.com>
In-Reply-To: <032375ce-7dc0-4d93-9f26-12a6bb89dbe0@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


>> +
>> +static void ath11k_p2p_noa_ie_fill(u8 *data, size_t len,
>> +				   const struct ath11k_wmi_p2p_noa_info *noa)
>> +{
>> +	struct ieee80211_p2p_noa_attr *noa_attr;
>> +	u8 ctwindow = u32_get_bits(noa->noa_attr, WMI_P2P_NOA_INFO_CTWIN_TU);
>> +	bool oppps = u32_get_bits(noa->noa_attr, WMI_P2P_NOA_INFO_OPP_PS);
>> +	__le16 *noa_attr_len;
>> +	u16 attr_len;
>> +	u8 noa_descriptors = u32_get_bits(noa->noa_attr,
>> +					   WMI_P2P_NOA_INFO_DESC_NUM);
> 
> do you need to validate that this doesn't exceed
> WMI_P2P_MAX_NOA_DESCRIPTORS?
> 
>> +	int i;
>> +
>> +	/* P2P IE */
……

>> +
>> +static size_t
>> +ath11k_p2p_noa_ie_len_compute(const struct ath11k_wmi_p2p_noa_info *noa)
>> +{
>> +	size_t len = 0;
>> +
>> +	if (!(u32_get_bits(noa->noa_attr, WMI_P2P_NOA_INFO_DESC_NUM)) &&
>> +	    !(u32_get_bits(noa->noa_attr, WMI_P2P_NOA_INFO_OPP_PS)))
>> +		return 0;
>> +
>> +	len += 1 + 1 + 4; /* EID + len + OUI */
>> +	len += 1 + 2; /* noa attr + attr len */
>> +	len += 1 + 1; /* index + oppps_ctwindow */
>> +	len += u32_get_bits(noa->noa_attr, WMI_P2P_NOA_INFO_DESC_NUM) *
> 
> here again do you need to validate that this doesn't exceed
> WMI_P2P_MAX_NOA_DESCRIPTORS?
> 
> also rather than call u32_get_bits() twice for the same field, call it
> once and cache the result


Will add this check in ath11k_wmi_p2p_noa_event(), cause 
ath11k_p2p_noa_ie_fill() and ath11k_p2p_noa_ie_len_compute will only be 
called through ath11k_wmi_p2p_noa_event().


> 
>> +			sizeof(struct ieee80211_p2p_noa_desc);
>> +
>> +	return len;
>> +}
>> +

……

>>   
>> +static int ath11k_wmi_p2p_noa_event(struct ath11k_base *ab,
>> +				    struct sk_buff *skb)
>> +{
>> +	const void **tb;
>> +	const struct wmi_p2p_noa_event *ev;
>> +	const struct ath11k_wmi_p2p_noa_info *noa;
>> +	struct ath11k *ar;
>> +	int ret, vdev_id;
>> +
>> +	tb = ath11k_wmi_tlv_parse_alloc(ab, skb, GFP_ATOMIC);
>> +	if (IS_ERR(tb)) {
>> +		ret = PTR_ERR(tb);
>> +		ath11k_warn(ab, "failed to parse tlv: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ev = tb[WMI_TAG_P2P_NOA_EVENT];
>> +	noa = tb[WMI_TAG_P2P_NOA_INFO];
>> +
>> +	if (!ev || !noa) {
>> +		ret = -EPROTO;
>> +		goto out;
>> +	}
>> +
>> +	vdev_id = ev->vdev_id;
>> +
>> +	ath11k_dbg(ab, ATH11K_DBG_WMI,
>> +		   "wmi tlv p2p noa vdev_id %i descriptors %u\n",
>> +		   vdev_id, u32_get_bits(noa->noa_attr, WMI_P2P_NOA_INFO_DESC_NUM));
>> +	rcu_read_lock();
>> +	ar = ath11k_mac_get_ar_by_vdev_id(ab, vdev_id);
>> +	if (!ar) {
>> +		ath11k_warn(ab, "invalid vdev id %d in P2P NoA event\n",
>> +			    vdev_id);
>> +		ret = -EINVAL;
>> +		goto unlock;
>> +	}
>> +
>> +	ath11k_p2p_noa_update_by_vdev_id(ar, vdev_id, noa);
>> +
>> +unlock:
>> +	rcu_read_unlock();
>> +out:
>> +	kfree(tb);
>> +	return 0;
>> +}
>> +
>>   static void ath11k_wmi_tlv_op_rx(struct ath11k_base *ab, struct sk_buff *skb)
>>   {
>>   	struct wmi_cmd_hdr *cmd_hdr;
>> @@ -8772,6 +8824,9 @@ static void ath11k_wmi_tlv_op_rx(struct ath11k_base *ab, struct sk_buff *skb)
>>   	case WMI_GTK_OFFLOAD_STATUS_EVENTID:
>>   		ath11k_wmi_gtk_offload_status_event(ab, skb);
>>   		break;
>> +	case WMI_P2P_NOA_EVENTID:
>> +		ath11k_wmi_p2p_noa_event(ab, skb);
>> +		break;
>>   	default:
>>   		ath11k_dbg(ab, ATH11K_DBG_WMI, "unsupported event id 0x%x\n", id);
>>   		break;
>> diff --git a/drivers/net/wireless/ath/ath11k/wmi.h b/drivers/net/wireless/ath/ath11k/wmi.h
>> index 4c20202947c7..564f4a9ac8ce 100644
>> --- a/drivers/net/wireless/ath/ath11k/wmi.h
>> +++ b/drivers/net/wireless/ath/ath11k/wmi.h
> 
> drivers/net/wireless/ath/ath11k/wmi.h QuIC copyright missing 2024
> 
>> @@ -3630,6 +3630,37 @@ struct wmi_ftm_event_msg {
>>   	u8 data[];
>>   } __packed;
>>   
>> +#define WMI_P2P_MAX_NOA_DESCRIPTORS		4
>> +
>> +struct wmi_p2p_noa_event {
>> +	u32 vdev_id;
>> +} __packed;
>> +
>> +struct ath11k_wmi_p2p_noa_descriptor {
>> +	u32 type_count; /* 255: continuous schedule, 0: reserved */
>> +	u32 duration;  /* Absent period duration in micro seconds */
>> +	u32 interval;   /* Absent period interval in micro seconds */
>> +	u32 start_time; /* 32 bit tsf time when in starts */
>> +} __packed;
>> +
>> +#define WMI_P2P_NOA_INFO_CHANGED_FLAG		BIT(0)
>> +#define WMI_P2P_NOA_INFO_INDEX			GENMASK(15, 8)
>> +#define WMI_P2P_NOA_INFO_OPP_PS			BIT(16)
>> +#define WMI_P2P_NOA_INFO_CTWIN_TU		GENMASK(23, 17)
>> +#define WMI_P2P_NOA_INFO_DESC_NUM		GENMASK(31, 24)
>> +
>> +struct ath11k_wmi_p2p_noa_info {
>> +	/* Bit 0 - Flag to indicate an update in NOA schedule
>> +	 * Bits 7-1 - Reserved
>> +	 * Bits 15-8 - Index (identifies the instance of NOA sub element)
>> +	 * Bit  16 - Opp PS state of the AP
>> +	 * Bits 23-17 -  Ctwindow in TUs
>> +	 * Bits 31-24 -  Number of NOA descriptors
>> +	 */
>> +	u32 noa_attr;
>> +	struct ath11k_wmi_p2p_noa_descriptor descriptors[WMI_P2P_MAX_NOA_DESCRIPTORS];
> 
> so firmware always sends the max number of records even if Bits 31-24 -
> Number of NOA descriptors indicates a smaller number?
> 

I just checked this, firmware will fill this according to valid number, 
not max number.
But when fill WMI event for P2P NOA, the TLV length is based on max number.

Even if the valid number does not reach the max number, the remain part 
is just empty.


So i think this definition is OK.


>> +} __packed;
>> +
>>   #define WMI_BEACON_TX_BUFFER_SIZE	512
>>   
>>   #define WMI_EMA_TMPL_IDX_SHIFT            8
> 
> I'm seeing things today I probably missed in the ath12k review, so let's
> make sure that when this is merged that ath12k si updated as well.

> /jeff