| Message ID | 20240126115231.356658-11-quic_kangyang@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: ath12k: P2P support for WCN7850 | expand |
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; > }
On 1/26/2024 8:56 AM, Jeff Johnson wrote: > 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. > */ After sending this I realized the value of ATH12K_MAX_VDEV_STATS_ID is correct, and IMO should be used by the current logic. Use of hex in the macro vs decimal in the firmware documentation confused me...
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
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
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) { vdev_stats_id = ATH12K_INVAL_VDEV_STATS_ID; break; }
During calculate vdev_stats_id, will copmare vdev_stats_id with 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") 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(-)