[PATCH REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"

[PATCH REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"

[Brian Norris]

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(-)

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

Re: [PATCH REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"

[Rajkumar Manoharan]

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

Re: [PATCH REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"

[Govind Singh]

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

Re: [PATCH REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"

[Brian Norris]

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

Re: [PATCH REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"

[Rajkumar Manoharan]

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

Re: [PATCH REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"

[Kalle Valo]

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!

-- 
Kalle Valo

Re: [PATCH REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"

[Brian Norris]

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 ;)

Re: [PATCH REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"

[Kalle Valo]

Brian Norris <briannorris@chromium.org> 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.
> 
> 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>

This regression should be fixed by (which is in v4.20):

53884577fbce ath10k: skip sending quiet mode cmd for WCN3990

So this revert should not be needed anymore and I'll drop this.

-- 
https://patchwork.kernel.org/patch/10672903/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"

[Brian Norris]

On Mon, Feb 4, 2019 at 8:43 AM Kalle Valo <kvalo@codeaurora.org> wrote:
> Brian Norris <briannorris@chromium.org> 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.
> >
> > 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>
>
> This regression should be fixed by (which is in v4.20):
>
> 53884577fbce ath10k: skip sending quiet mode cmd for WCN3990
>
> So this revert should not be needed anymore and I'll drop this.

Yes, that seems to work for me. I'll probably replace the revert with
the above in my downstream. Thanks!

Can't really be fixed now, but it would have been nice to have a Fixes
tag on that one.

Brian

Re: [PATCH REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377"

[Kalle Valo]

Brian Norris <briannorris@chromium.org> writes:

> On Mon, Feb 4, 2019 at 8:43 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>> Brian Norris <briannorris@chromium.org> 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.
>> >
>> > 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>
>>
>> This regression should be fixed by (which is in v4.20):
>>
>> 53884577fbce ath10k: skip sending quiet mode cmd for WCN3990
>>
>> So this revert should not be needed anymore and I'll drop this.
>
> Yes, that seems to work for me. I'll probably replace the revert with
> the above in my downstream. Thanks!
>
> Can't really be fixed now, but it would have been nice to have a Fixes
> tag on that one.

Indeed. Sorry, missed that.

-- 
Kalle Valo