Discussion:
[PATCH REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"
Brian Norris
2018-11-07 18:56:43 UTC
Permalink
This reverts commit cfb353c0dc058bc1619cc226d3cbbda1f360bdd3.

WCN3990 firmware does not yet implement this feature, and so it crashes
like this:

fatal error received: err_qdi.c:456:EX:wlan_process:1:WLAN RT:207a:PC=b001b4f0

This feature can be re-implemented with a proper service bitmap or other
feature-discovery mechanism in the future. But it should not break
working boards.

Fixes: cfb353c0dc05 ("ath10k: add quiet mode support for QCA6174/QCA9377")
Cc: Yu Wang <***@codeaurora.org>
Cc: Rakesh Pillai <***@codeaurora.org>
Cc: Govind Singh <***@codeaurora.org>
Cc: <***@vger.kernel.org>
Signed-off-by: Brian Norris <***@chromium.org>
---
drivers/net/wireless/ath/ath10k/wmi-tlv.c | 33 +----------------------
drivers/net/wireless/ath/ath10k/wmi-tlv.h | 16 -----------
2 files changed, 1 insertion(+), 48 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index ad4114a88170..099e78b5de1f 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -3209,37 +3209,6 @@ ath10k_wmi_tlv_op_gen_tdls_peer_update(struct ath10k *ar,
return skb;
}

-static struct sk_buff *
-ath10k_wmi_tlv_op_gen_pdev_set_quiet_mode(struct ath10k *ar, u32 period,
- u32 duration, u32 next_offset,
- u32 enabled)
-{
- struct wmi_tlv_set_quiet_cmd *cmd;
- struct wmi_tlv *tlv;
- struct sk_buff *skb;
-
- skb = ath10k_wmi_alloc_skb(ar, sizeof(*tlv) + sizeof(*cmd));
- if (!skb)
- return ERR_PTR(-ENOMEM);
-
- tlv = (void *)skb->data;
- tlv->tag = __cpu_to_le16(WMI_TLV_TAG_STRUCT_PDEV_SET_QUIET_CMD);
- tlv->len = __cpu_to_le16(sizeof(*cmd));
- cmd = (void *)tlv->value;
-
- /* vdev_id is not in use, set to 0 */
- cmd->vdev_id = __cpu_to_le32(0);
- cmd->period = __cpu_to_le32(period);
- cmd->duration = __cpu_to_le32(duration);
- cmd->next_start = __cpu_to_le32(next_offset);
- cmd->enabled = __cpu_to_le32(enabled);
-
- ath10k_dbg(ar, ATH10K_DBG_WMI,
- "wmi tlv quiet param: period %u duration %u enabled %d\n",
- period, duration, enabled);
- return skb;
-}
-
static struct sk_buff *
ath10k_wmi_tlv_op_gen_wow_enable(struct ath10k *ar)
{
@@ -4143,7 +4112,7 @@ static const struct wmi_ops wmi_tlv_ops = {
.gen_dbglog_cfg = ath10k_wmi_tlv_op_gen_dbglog_cfg,
.gen_pktlog_enable = ath10k_wmi_tlv_op_gen_pktlog_enable,
.gen_pktlog_disable = ath10k_wmi_tlv_op_gen_pktlog_disable,
- .gen_pdev_set_quiet_mode = ath10k_wmi_tlv_op_gen_pdev_set_quiet_mode,
+ /* .gen_pdev_set_quiet_mode not implemented */
.gen_pdev_get_temperature = ath10k_wmi_tlv_op_gen_pdev_get_temperature,
/* .gen_addba_clear_resp not implemented */
/* .gen_addba_send not implemented */
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.h b/drivers/net/wireless/ath/ath10k/wmi-tlv.h
index bf8a4320c39c..5db294558ce5 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.h
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.h
@@ -1,7 +1,6 @@
/*
* Copyright (c) 2005-2011 Atheros Communications Inc.
* Copyright (c) 2011-2017 Qualcomm Atheros, Inc.
- * Copyright (c) 2018, The Linux Foundation. All rights reserved.
*
* Permission to use, copy, modify, and/or distribute this software for any
* purpose with or without fee is hereby granted, provided that the above
@@ -1980,21 +1979,6 @@ struct wmi_tlv_wow_add_del_event_cmd {
__le32 event_bitmap;
} __packed;

-/* Command to set/unset chip in quiet mode */
-struct wmi_tlv_set_quiet_cmd {
- __le32 vdev_id;
-
- /* in TUs */
- __le32 period;
-
- /* in TUs */
- __le32 duration;
-
- /* offset in TUs */
- __le32 next_start;
- __le32 enabled;
-} __packed;
-
struct wmi_tlv_wow_enable_cmd {
__le32 enable;
} __packed;
--
2.19.1.930.g4563a0d9d0-goog
Rajkumar Manoharan
2018-11-07 21:30:23 UTC
Permalink
Post by Brian Norris
This reverts commit cfb353c0dc058bc1619cc226d3cbbda1f360bdd3.
WCN3990 firmware does not yet implement this feature, and so it crashes
fatal error received: err_qdi.c:456:EX:wlan_process:1:WLAN
RT:207a:PC=b001b4f0
This feature can be re-implemented with a proper service bitmap or other
feature-discovery mechanism in the future. But it should not break
working boards.
Brian,

The change "ath10k: add quiet mode support for QCA6174/QCA9377" was
merged even
before full WCN3990 device support was added in ath10k. How come it
could be regression
for WCN3990. I know both are sharing same WMI-TLV interface but
reverting this
will break QCA6174/QCA9377. no?

I would prefer to handle this within WMI callback or upper layer.

-Rajkumar
Govind Singh
2018-11-08 04:31:51 UTC
Permalink
Post by Rajkumar Manoharan
Post by Brian Norris
This reverts commit cfb353c0dc058bc1619cc226d3cbbda1f360bdd3.
WCN3990 firmware does not yet implement this feature, and so it crashes
fatal error received: err_qdi.c:456:EX:wlan_process:1:WLAN
RT:207a:PC=b001b4f0
This feature can be re-implemented with a proper service bitmap or other
feature-discovery mechanism in the future. But it should not break
working boards.
Brian,
The change "ath10k: add quiet mode support for QCA6174/QCA9377" was
merged even
before full WCN3990 device support was added in ath10k. How come it
could be regression
for WCN3990. I know both are sharing same WMI-TLV interface but
reverting this
will break QCA6174/QCA9377. no?
This regression is found while we switched from 4.18 + WCN3990
back-ports to 4.19.
Post by Rajkumar Manoharan
I would prefer to handle this within WMI callback or upper layer.
IMO, we should use (WMI_SERVICE_THERMAL_MGMT | WMI_SERVICE_THERM_THROT )
service bitmap check and call
ath10k_thermal_set_throttling only if fw supports THERMAL THROTTLE
feature. But we need to ensure all
available ath10k fw's are reporting this service.
Post by Rajkumar Manoharan
-Rajkumar
BR,
Govind
Brian Norris
2018-11-08 17:30:43 UTC
Permalink
Post by Govind Singh
Post by Rajkumar Manoharan
Post by Brian Norris
This reverts commit cfb353c0dc058bc1619cc226d3cbbda1f360bdd3.
WCN3990 firmware does not yet implement this feature, and so it crashes
fatal error received: err_qdi.c:456:EX:wlan_process:1:WLAN RT:207a:PC=b001b4f0
This feature can be re-implemented with a proper service bitmap or other
feature-discovery mechanism in the future. But it should not break
working boards.
Brian,
The change "ath10k: add quiet mode support for QCA6174/QCA9377" was
merged even
before full WCN3990 device support was added in ath10k. How come it
could be regression
for WCN3990. I know both are sharing same WMI-TLV interface but
reverting this
will break QCA6174/QCA9377. no?
I don't see how the revert would "break" QCA6174 -- QCA6174 worked
just fine without this feature and should continue to do so.
Post by Govind Singh
This regression is found while we switched from 4.18 + WCN3990
back-ports to 4.19.
^^ What Govind said. WCN3990 support has been trickling in over a few
releases, and it doesn't seem kosher to allow people to submit
regressions in the midst of that.
Post by Govind Singh
Post by Rajkumar Manoharan
I would prefer to handle this within WMI callback or upper layer.
IMO, we should use (WMI_SERVICE_THERMAL_MGMT | WMI_SERVICE_THERM_THROT )
service bitmap check and call
ath10k_thermal_set_throttling only if fw supports THERMAL THROTTLE
feature. But we need to ensure all
available ath10k fw's are reporting this service.
And the above notes from Govind highlight this -- if the feature was
not protected by the appropriate service flags, then we can't be sure
that you didn't break a bunch of other firmware releases out there.
Linux should not break for everyone just because you spun a firmware
release.

Of course, I'll leave it up to Kalle as to how he wants to mediate
this. And if you come up with a solid patch soon that can fix this
without dropping the feature, then so be it.

Brian
Rajkumar Manoharan
2018-11-08 19:52:53 UTC
Permalink
Post by Brian Norris
Post by Govind Singh
Post by Rajkumar Manoharan
The change "ath10k: add quiet mode support for QCA6174/QCA9377" was
merged even
before full WCN3990 device support was added in ath10k. How come it
could be regression
for WCN3990. I know both are sharing same WMI-TLV interface but
reverting this
will break QCA6174/QCA9377. no?
I don't see how the revert would "break" QCA6174 -- QCA6174 worked
just fine without this feature and should continue to do so.
I meant that the revert commit remove quiet mode support from QCA6174 &
QCA9377.
Post by Brian Norris
Post by Govind Singh
This regression is found while we switched from 4.18 + WCN3990
back-ports to 4.19.
^^ What Govind said. WCN3990 support has been trickling in over a few
releases, and it doesn't seem kosher to allow people to submit
regressions in the midst of that.
Nobody prefers regression :). WCN3990 support was still in progress, at
the time the commit got merged into upstream. My point is that we can't
expect
the community to validate the changes against in-progress platform.
Post by Brian Norris
Post by Govind Singh
IMO, we should use (WMI_SERVICE_THERMAL_MGMT | WMI_SERVICE_THERM_THROT )
service bitmap check and call
ath10k_thermal_set_throttling only if fw supports THERMAL THROTTLE
feature. But we need to ensure all
available ath10k fw's are reporting this service.
And the above notes from Govind highlight this -- if the feature was
not protected by the appropriate service flags, then we can't be sure
that you didn't break a bunch of other firmware releases out there.
Linux should not break for everyone just because you spun a firmware
release.
That is true. Any new features or interface changes in firmware will be
advertised by feature bit. But the quiet param was available in firmware
since first release.
Post by Brian Norris
Of course, I'll leave it up to Kalle as to how he wants to mediate
this. And if you come up with a solid patch soon that can fix this
without dropping the feature, then so be it.
Govind is working on to handle this properly either by instantiating
new WMI-TLV table for WCNxxxx or by adding conditional check in exiting
path.

-Rajkumar
Kalle Valo
2018-11-16 10:14:59 UTC
Permalink
Post by Brian Norris
Post by Govind Singh
Post by Rajkumar Manoharan
Post by Brian Norris
This reverts commit cfb353c0dc058bc1619cc226d3cbbda1f360bdd3.
WCN3990 firmware does not yet implement this feature, and so it crashes
fatal error received: err_qdi.c:456:EX:wlan_process:1:WLAN RT:207a:PC=b001b4f0
This feature can be re-implemented with a proper service bitmap or other
feature-discovery mechanism in the future. But it should not break
working boards.
Brian,
The change "ath10k: add quiet mode support for QCA6174/QCA9377" was
merged even
before full WCN3990 device support was added in ath10k. How come it
could be regression
for WCN3990. I know both are sharing same WMI-TLV interface but
reverting this
will break QCA6174/QCA9377. no?
I don't see how the revert would "break" QCA6174 -- QCA6174 worked
just fine without this feature and should continue to do so.
Post by Govind Singh
This regression is found while we switched from 4.18 + WCN3990
back-ports to 4.19.
^^ What Govind said. WCN3990 support has been trickling in over a few
releases, and it doesn't seem kosher to allow people to submit
regressions in the midst of that.
Yeah, I agree.
Post by Brian Norris
Post by Govind Singh
Post by Rajkumar Manoharan
I would prefer to handle this within WMI callback or upper layer.
IMO, we should use (WMI_SERVICE_THERMAL_MGMT | WMI_SERVICE_THERM_THROT )
service bitmap check and call
ath10k_thermal_set_throttling only if fw supports THERMAL THROTTLE
feature. But we need to ensure all
available ath10k fw's are reporting this service.
And the above notes from Govind highlight this -- if the feature was
not protected by the appropriate service flags, then we can't be sure
that you didn't break a bunch of other firmware releases out there.
Linux should not break for everyone just because you spun a firmware
release.
Of course, I'll leave it up to Kalle as to how he wants to mediate
this. And if you come up with a solid patch soon that can fix this
without dropping the feature, then so be it.
We should have a fix for this available next week which I'm planning to
push to 4.20. If that does not happen my plan B is to apply Brian's
revert to make wcn3990 working on 4.20.

Thanks Brian for investigating this and providing the revert!
--
Kalle Valo
Brian Norris
2018-11-16 21:15:52 UTC
Permalink
Post by Kalle Valo
Thanks Brian for investigating this and providing the revert!
I think Govind did most of the investigation, but I did at least do
the hard work of submitting the revert ;)

Loading...