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
