[REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"
diff mbox series

Message ID 20181107185643.240346-1-briannorris@chromium.org
State New
Delegated to: Kalle Valo
Headers show
Series
  • [REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"
Related show

Commit Message

Brian Norris Nov. 7, 2018, 6:56 p.m. UTC
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 <yyuwang@codeaurora.org>
Cc: Rakesh Pillai <pillair@codeaurora.org>
Cc: Govind Singh <govinds@codeaurora.org>
Cc: <stable@vger.kernel.org>
Signed-off-by: Brian Norris <briannorris@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(-)

Comments

Rajkumar Manoharan Nov. 7, 2018, 9:30 p.m. UTC | #1
On 2018-11-07 10:56, Brian Norris wrote:
> 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.
> 
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 Nov. 8, 2018, 4:31 a.m. UTC | #2
On 2018-11-08 03:00, Rajkumar Manoharan wrote:
> On 2018-11-07 10:56, Brian Norris wrote:
>> 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.
>> 
> 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.

> 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.

> -Rajkumar

BR,
Govind
Brian Norris Nov. 8, 2018, 5:30 p.m. UTC | #3
On Wed, Nov 7, 2018 at 8:32 PM Govind Singh <govinds@codeaurora.org> wrote:
> On 2018-11-08 03:00, Rajkumar Manoharan wrote:
> > On 2018-11-07 10:56, Brian Norris wrote:
> >> 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.
> >>
> > 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.

> 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.

> > 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 Nov. 8, 2018, 7:52 p.m. UTC | #4
On 2018-11-08 09:30, Brian Norris wrote:
> On Wed, Nov 7, 2018 at 8:32 PM Govind Singh <govinds@codeaurora.org> 
> wrote:
>> On 2018-11-08 03:00, Rajkumar Manoharan wrote:
>> >
>> > 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.

>> 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.

>> 
>> 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.

> 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 Nov. 16, 2018, 10:14 a.m. UTC | #5
Brian Norris <briannorris@chromium.org> writes:

> On Wed, Nov 7, 2018 at 8:32 PM Govind Singh <govinds@codeaurora.org> wrote:
>> On 2018-11-08 03:00, Rajkumar Manoharan wrote:
>> > On 2018-11-07 10:56, Brian Norris wrote:
>> >> 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.
>> >>
>> > 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.
>
>> 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.

>> > 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!
Brian Norris Nov. 16, 2018, 9:15 p.m. UTC | #6
On Fri, Nov 16, 2018 at 2:15 AM Kalle Valo <kvalo@codeaurora.org> wrote:
> 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 ;)

Patch
diff mbox series

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;