On 1/26/2024 3:52 AM, Kang Yang wrote:
> During calculate vdev_stats_id, will copmare vdev_stats_id with
s/copmare /compare /
> ATH12K_INVAL_VDEV_STATS_ID. If vdev_stats_id is relatively small, then
> assign ATH12K_INVAL_VDEV_STATS_ID to vdev_stats_id.
>
> Obviously, this logic is incorrect. ATH12K_INVAL_VDEV_STATS_ID is 0xff,
> and the data type of this variable is u8. Which means this judgement
> will always be true. So will get 0xff for every vdev except the first
> one.
>
> Correct this logic and replace it with the maximum value
> ATH12K_MAX_VDEV_STATS_ID.
>
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
Fixes is an upstream tag so it should be grouped with the SOB
Since this is a preexisting issue that is unrelated to P2P I'm thinking
you should remove it from the P2P series and send it separately?
>
> Signed-off-by: Kang Yang <quic_kangyang@xxxxxxxxxxx>
> ---
>
> v4: new patch.
>
> ---
> drivers/net/wireless/ath/ath12k/mac.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index d8c8bd420aa2..6b8b92d22553 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -5520,7 +5520,7 @@ ath12k_mac_get_vdev_stats_id(struct ath12k_vif *arvif)
> do {
> if (ab->free_vdev_stats_id_map & (1LL << vdev_stats_id)) {
> vdev_stats_id++;
> - if (vdev_stats_id <= ATH12K_INVAL_VDEV_STATS_ID) {
> + if (vdev_stats_id >= ATH12K_MAX_VDEV_STATS_ID) {
as you already noted it can't be > so just make this ==
but why isn't this instead using ATH12K_MAX_VDEV_STATS_ID (which is
currently unused)
But even the current value for that seems wrong based upon the firmware
documentation:
/**
* 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.
*/
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;
> vdev_stats_id = ATH12K_INVAL_VDEV_STATS_ID;
> break;
> }
