Received-SPF: pass (google.com: domain of linux-wireless+bounces-2626-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <ef7b72b2-3a34-4e94-8552-834f24e4bddc@quicinc.com>
Date: Mon, 29 Jan 2024 10:25:11 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 10/11] wifi: ath12k: fix incorrect logic of calculating
 vdev_stats_id
Content-Language: en-US
To: Jeff Johnson <quic_jjohnson@quicinc.com>, <ath12k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>
References: <20240126115231.356658-1-quic_kangyang@quicinc.com>
 <20240126115231.356658-11-quic_kangyang@quicinc.com>
 <0dd4948b-e967-4562-b98e-2f4643205ca4@quicinc.com>
 <0b39cf96-3ed4-45b7-9254-2604716afe14@quicinc.com>
From: Kang Yang <quic_kangyang@quicinc.com>
In-Reply-To: <0b39cf96-3ed4-45b7-9254-2604716afe14@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 1/27/2024 7:05 AM, Jeff Johnson wrote:
> On 1/26/2024 8:56 AM, Jeff Johnson wrote:
>> And it seems there is another bigger issue here since, as the firmware
>> document indicates, the vdev_stats_id field should be ignored unless the
>> vdev_stats_id_valid field is non-zero, but in ath12k_wmi_vdev_create()
>> we don't set vdev_stats_id_valid -- and we cannot set it since it isn't
>> even present in the ath12k struct wmi_vdev_create_cmd! And comparing our
>> struct to the firmware definition shows we have missing fields!!!
>> Everything is correct up to pdev_id, but then there is divergence:
>>
>> our struct
>> struct wmi_vdev_create_cmd {
>> 	__le32 tlv_header;
>> 	__le32 vdev_id;
>> 	__le32 vdev_type;
>> 	__le32 vdev_subtype;
>> 	struct ath12k_wmi_mac_addr_params vdev_macaddr;
>> 	__le32 num_cfg_txrx_streams;
>> 	__le32 pdev_id;
>> 	__le32 vdev_stats_id;
>> } __packed;
>>
>> firmware definition
>> typedef struct {
>>      A_UINT32 tlv_header; /** TLV tag and len; tag equals
>> WMITLV_TAG_STRUC_wmi_vdev_create_cmd_fixed_param */
>>      /** unique id identifying the VDEV, generated by the caller */
>>      A_UINT32 vdev_id;
>>      /** VDEV type (AP,STA,IBSS,MONITOR) */
>>      A_UINT32 vdev_type;
>>      /** VDEV subtype (P2PDEV, P2PCLI, P2PGO, BT3.0, BRIDGE) */
>>      A_UINT32 vdev_subtype;
>>      /** VDEV MAC address */
>>      wmi_mac_addr vdev_macaddr;
>>      /** Number of configured txrx streams */
>>      A_UINT32 num_cfg_txrx_streams;
>>      /**
>>       * pdev_id for identifying the MAC,
>>       * See macros starting with WMI_PDEV_ID_ for values.
>>       */
>>      A_UINT32 pdev_id;
>>      /** control flags for this vdev (DEPRECATED)
>>       * Use @mbss_capability_flags in vdev start instead.
>>       */
>>      A_UINT32 flags;
>>      /**  vdevid of transmitted AP (mbssid case) (DEPRECATED)
>>       * Use @vdevid_trans in vdev start instead.
>>       */
>>      A_UINT32 vdevid_trans;
>>      /* vdev_stats_id_valid indicates whether vdev_stats_id is valid */
>>      A_UINT32 vdev_stats_id_valid;
>>      /**
>>       * vdev_stats_id indicates the ID for the REO Rx stats collection
>>       * For Beryllium: 0-47 is the valid range and >=48 is invalid
>>       * This vdev_stats_id field should be ignored unless the
>>       * vdev_stats_id_valid field is non-zero.
>>       */
>>      A_UINT32 vdev_stats_id;
>> /* This TLV is followed by another TLV of array of structures
>>   *   wmi_vdev_txrx_streams cfg_txrx_streams[];
>>   *   wmi_vdev_create_mlo_params mlo_params[0,1];
>>   *       optional TLV, only present for MLO vdev;
>>   *       if the vdev is not MLO the array length should be 0.
>>   */
>> } wmi_vdev_create_cmd_fixed_param;
>>
>> (note the deprecated fields must still have their space allocated in the
>> data structure)
>>
>> So currently when host is writing to vdev_stats_id firmware will
>> interpret this as the deprecated flags
>>
>> So it seems like we also need to fix the WMI struct to:
>> struct wmi_vdev_create_cmd {
>> 	__le32 tlv_header;
>> 	__le32 vdev_id;
>> 	__le32 vdev_type;
>> 	__le32 vdev_subtype;
>> 	struct ath12k_wmi_mac_addr_params vdev_macaddr;
>> 	__le32 num_cfg_txrx_streams;
>> 	__le32 pdev_id;
>> 	__le32 flags; /* deprecated */
>> 	__le32 vdevid_trans; /* deprecated */
>> 	__le32 vdev_stats_id_valid;
>> 	__le32 vdev_stats_id;
>> } __packed;
> 
> Sigh. I now realize that patch 7/11 in the series fixes this, and hence
> why this 10/11 patch needs to be part of the series (or the 7/11 and
> 10/11 patches should be separated from the P2P feature).
> 

They cannot be separated from the P2P feature.
Without patch 7/11, P2P won't run properly due to firmware crash.


> Let me re-review the entire series instead of just reviewing the 7/11
> patch without the associated context.

Maybe i need to move patch 10/11 to 8/11, make them closer?


> 
> Must be Friday.
> 
> /jeff