Received-SPF: pass (google.com: domain of linux-wireless+bounces-2579-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <0b39cf96-3ed4-45b7-9254-2604716afe14@quicinc.com>
Date: Fri, 26 Jan 2024 15:05:24 -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
From: Jeff Johnson <quic_jjohnson@quicinc.com>
To: Kang Yang <quic_kangyang@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>
In-Reply-To: <0dd4948b-e967-4562-b98e-2f4643205ca4@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

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).

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

Must be Friday.

/jeff