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
