by Jeff Johnson

[permalink] [raw]

Subject: Re: [PATCH v4 11/11] wifi: ath12k: advertise P2P dev support for WCN7850

On 1/26/2024 3:52 AM, Kang Yang wrote:
> Now that all the necessary pieces are implemented we can enable P2P
> support for WCN7850.
>
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Kang Yang <[email protected]>
Acked-by: Jeff Johnson <[email protected]>

2024-01-29 02:25:32

by Kang Yang

[permalink] [raw]

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

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