| Message ID | 20250115042532.1509956-5-quic_aarasahu@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: ath12k: Factory test mode support | expand |
On 1/15/25 09:55, Aaradhana Sahu wrote: > Currently pdev id is not set properly. That can cause a crash > if pdev id is not equal to the pdev id when firmware test > command is run during AP bring up or ping. > > Set pdev id in function ath12k_tm_cmd_wmi to resolve this > issue. > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1 > > Co-developed-by: Rajat Soni <quic_rajson@quicinc.com> > Signed-off-by: Rajat Soni <quic_rajson@quicinc.com> > Signed-off-by: Aaradhana Sahu <quic_aarasahu@quicinc.com> > --- > drivers/net/wireless/ath/ath12k/testmode.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > Previous patch "[PATCH v9 3/5] wifi: ath12k: add factory test mode support" only added testmode.c file isn't it? So can't we squash this patch in that? Let's not introduce a bug in patch X and then in same series fix it at patch Y.
On 1/15/2025 10:56 AM, Aditya Kumar Singh wrote: > On 1/15/25 09:55, Aaradhana Sahu wrote: >> Currently pdev id is not set properly. That can cause a crash >> if pdev id is not equal to the pdev id when firmware test >> command is run during AP bring up or ping. >> >> Set pdev id in function ath12k_tm_cmd_wmi to resolve this >> issue. >> >> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1 >> >> Co-developed-by: Rajat Soni <quic_rajson@quicinc.com> >> Signed-off-by: Rajat Soni <quic_rajson@quicinc.com> >> Signed-off-by: Aaradhana Sahu <quic_aarasahu@quicinc.com> >> --- >> drivers/net/wireless/ath/ath12k/testmode.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> > > Previous patch "[PATCH v9 3/5] wifi: ath12k: add factory test mode support" only added testmode.c file isn't it? So can't we squash this patch in that? > > Let's not introduce a bug in patch X and then in same series fix it at patch Y. > This patch does not address any issues related to Factory Test Mode (FTM). Instead, it focuses only on the ath11k-fwtest command, which is used for certification testing and is distinct from FTM. Initially, this patch was submitted independently as '[PATCH v2] wifi: ath12k: Fill pdev id for fw test cmd' during the internal review. However, Kalle suggested incorporating this patch with the FTM patches. As this patch addresses ath11k-fwtest command issue, that's why this is not merge with [PATCH v9 3/5].
On 1/15/25 11:11, Aaradhana Sahu wrote: > > > On 1/15/2025 10:56 AM, Aditya Kumar Singh wrote: >> On 1/15/25 09:55, Aaradhana Sahu wrote: >>> Currently pdev id is not set properly. That can cause a crash >>> if pdev id is not equal to the pdev id when firmware test >>> command is run during AP bring up or ping. >>> >>> Set pdev id in function ath12k_tm_cmd_wmi to resolve this >>> issue. >>> >>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1 >>> >>> Co-developed-by: Rajat Soni <quic_rajson@quicinc.com> >>> Signed-off-by: Rajat Soni <quic_rajson@quicinc.com> >>> Signed-off-by: Aaradhana Sahu <quic_aarasahu@quicinc.com> >>> --- >>> drivers/net/wireless/ath/ath12k/testmode.c | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >> >> Previous patch "[PATCH v9 3/5] wifi: ath12k: add factory test mode support" only added testmode.c file isn't it? So can't we squash this patch in that? >> >> Let's not introduce a bug in patch X and then in same series fix it at patch Y. >> > > This patch does not address any issues related to Factory Test Mode (FTM). > Instead, it focuses only on the ath11k-fwtest command, which is used for > certification testing and is distinct from FTM. > > Initially, this patch was submitted independently as '[PATCH v2] wifi: ath12k: Fill pdev id for fw test cmd' > during the internal review. > However, Kalle suggested incorporating this patch with the FTM patches. That is good. But that comment does not mean it needs to be a *separate* patch? It should be in this series that's what Kalle wanted to ensure. > > As this patch addresses ath11k-fwtest command issue, that's why this is not merge with [PATCH v9 3/5]. > That is fine, but you are adding the file in 3/5 so why not have this part as well in same patch only? And let me ask this way, if you don't apply 3/5, will your issue exist? Probably No? Since ath12k/testmode.c file itself is not there before 3/5. So if you know something is incorrect, why introduce it in the first place?
On 1/15/2025 12:10 PM, Aditya Kumar Singh wrote: > On 1/15/25 11:11, Aaradhana Sahu wrote: >> >> >> On 1/15/2025 10:56 AM, Aditya Kumar Singh wrote: >>> On 1/15/25 09:55, Aaradhana Sahu wrote: >>>> Currently pdev id is not set properly. That can cause a crash >>>> if pdev id is not equal to the pdev id when firmware test >>>> command is run during AP bring up or ping. >>>> >>>> Set pdev id in function ath12k_tm_cmd_wmi to resolve this >>>> issue. >>>> >>>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.3.1-00173-QCAHKSWPL_SILICONZ-1 >>>> >>>> Co-developed-by: Rajat Soni <quic_rajson@quicinc.com> >>>> Signed-off-by: Rajat Soni <quic_rajson@quicinc.com> >>>> Signed-off-by: Aaradhana Sahu <quic_aarasahu@quicinc.com> >>>> --- >>>> drivers/net/wireless/ath/ath12k/testmode.c | 11 +++++++++-- >>>> 1 file changed, 9 insertions(+), 2 deletions(-) >>>> >>> >>> Previous patch "[PATCH v9 3/5] wifi: ath12k: add factory test mode support" only added testmode.c file isn't it? So can't we squash this patch in that? >>> >>> Let's not introduce a bug in patch X and then in same series fix it at patch Y. >>> >> >> This patch does not address any issues related to Factory Test Mode (FTM). >> Instead, it focuses only on the ath11k-fwtest command, which is used for >> certification testing and is distinct from FTM. >> >> Initially, this patch was submitted independently as '[PATCH v2] wifi: ath12k: Fill pdev id for fw test cmd' >> during the internal review. >> However, Kalle suggested incorporating this patch with the FTM patches. > > That is good. But that comment does not mean it needs to be a *separate* patch? It should be in this series that's what Kalle wanted to ensure. > >> >> As this patch addresses ath11k-fwtest command issue, that's why this is not merge with [PATCH v9 3/5]. >> > > That is fine, but you are adding the file in 3/5 so why not have this part as well in same patch only? And let me ask this way, if you don't apply 3/5, will your issue exist? Probably No? Since ath12k/testmode.c file itself is not there before 3/5. So if you know something is incorrect, why introduce it in the first place? > > During the internal review of this patch, the leads explicitly stated that it should be separate. Based on their feedback, I made the necessary changes. Since this was previously discussed, I would prefer to keep this patch separate.
diff --git a/drivers/net/wireless/ath/ath12k/testmode.c b/drivers/net/wireless/ath/ath12k/testmode.c index ac9ffa765ef6..18d56a976dc7 100644 --- a/drivers/net/wireless/ath/ath12k/testmode.c +++ b/drivers/net/wireless/ath/ath12k/testmode.c @@ -301,9 +301,10 @@ static int ath12k_tm_cmd_wmi(struct ath12k *ar, struct nlattr *tb[]) { struct ath12k_wmi_pdev *wmi = ar->wmi; struct sk_buff *skb; - u32 cmd_id, buf_len; - int ret = 0; + struct wmi_pdev_set_param_cmd *cmd; + int ret = 0, tag; void *buf; + u32 cmd_id, buf_len; if (!tb[ATH_TM_ATTR_DATA]) return -EINVAL; @@ -321,6 +322,12 @@ static int ath12k_tm_cmd_wmi(struct ath12k *ar, struct nlattr *tb[]) cmd_id = nla_get_u32(tb[ATH_TM_ATTR_WMI_CMDID]); + cmd = buf; + tag = le32_get_bits(cmd->tlv_header, WMI_TLV_TAG); + + if (tag == WMI_TAG_PDEV_SET_PARAM_CMD) + cmd->pdev_id = cpu_to_le32(ar->pdev->pdev_id); + ath12k_dbg(ar->ab, ATH12K_DBG_TESTMODE, "testmode cmd wmi cmd_id %d buf length %d\n", cmd_id, buf_len);