[PATCH v4 10/11] wifi: ath12k: fix incorrect logic of calculating vdev_stats_id

[Kang Yang]

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