| Message ID | 20181107185643.240346-1-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [REGRESSION] Revert "ath10k: add quiet mode support for QCA6174/QCA9377" | expand |
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
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
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
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
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!
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 ;)
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.
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
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.
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;
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(-)