Received-SPF: pass (google.com: domain of linux-wireless+bounces-2570-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <0dd4948b-e967-4562-b98e-2f4643205ca4@quicinc.com>
Date: Fri, 26 Jan 2024 08:56:04 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 10/11] wifi: ath12k: fix incorrect logic of calculating
 vdev_stats_id
Content-Language: en-US
To: Kang Yang <quic_kangyang@quicinc.com>, <ath12k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>
References: <20240126115231.356658-1-quic_kangyang@quicinc.com>
 <20240126115231.356658-11-quic_kangyang@quicinc.com>
From: Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20240126115231.356658-11-quic_kangyang@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

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@quicinc.com>
> ---
> 
> 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;
>  			}