| Message ID | 1395428150-31996-1-git-send-email-greearb@candelatech.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
greearb@candelatech.com writes: > From: Ben Greear <greearb@candelatech.com> > > This helps track tx credits accounting and usage. If > firmware hangs or otherwise fails to return credits, one > can more easily see the last few command types that > was send to the firmware. > > Signed-off-by: Ben Greear <greearb@candelatech.com> This patchset didn't apply, and the sha1 ids were useless so that 3-way merge didn't work. Can you rebase, please? And better yet if you rebase in top of my master branch. > int ath10k_htc_send(struct ath10k_htc *htc, > enum ath10k_htc_ep_id eid, > - struct sk_buff *skb) > + struct sk_buff *skb, int dbg) > { > struct ath10k_htc_ep *ep = &htc->endpoint[eid]; > struct ath10k_skb_cb *skb_cb = ATH10K_SKB_CB(skb); > @@ -167,6 +167,10 @@ int ath10k_htc_send(struct ath10k_htc *htc, > goto err_pull; > } > ep->tx_credits -= credits; > + ath10k_dbg(ATH10K_DBG_HTC, > + "ep %d used %d credits, remaining %d dbg %d (0x%x)\n", > + eid, credits, ep->tx_credits, dbg, dbg); > + > spin_unlock_bh(&htc->tx_lock); > } [...] > @@ -680,7 +684,7 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc, > > reinit_completion(&htc->ctl_resp); > > - status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb); > + status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb, __LINE__); Using line numbers in debug messages is very cumbersome. Some people cherry pick patches, have their own changes and whatnot which will make it more difficult to read the debug logs. Isn't there any better way to do this? > --- a/drivers/net/wireless/ath/ath10k/wmi.c > +++ b/drivers/net/wireless/ath/ath10k/wmi.c > @@ -544,7 +544,7 @@ static int ath10k_wmi_cmd_send_nowait(struct ath10k *ar, struct sk_buff *skb, > cmd_hdr->cmd_id = __cpu_to_le32(cmd); > > memset(skb_cb, 0, sizeof(*skb_cb)); > - ret = ath10k_htc_send(&ar->htc, ar->wmi.eid, skb); > + ret = ath10k_htc_send(&ar->htc, ar->wmi.eid, skb, 0x71100000 | cmd_id); > trace_ath10k_wmi_cmd(cmd_id, skb->data, skb->len, ret); What does 0x71100000 stand for?
On 24 March 2014 12:12, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > greearb@candelatech.com writes: > >> From: Ben Greear <greearb@candelatech.com> >> >> This helps track tx credits accounting and usage. If >> firmware hangs or otherwise fails to return credits, one >> can more easily see the last few command types that >> was send to the firmware. >> >> Signed-off-by: Ben Greear <greearb@candelatech.com> > > This patchset didn't apply, and the sha1 ids were useless so that 3-way > merge didn't work. Can you rebase, please? And better yet if you rebase > in top of my master branch. > >> int ath10k_htc_send(struct ath10k_htc *htc, >> enum ath10k_htc_ep_id eid, >> - struct sk_buff *skb) >> + struct sk_buff *skb, int dbg) >> { >> struct ath10k_htc_ep *ep = &htc->endpoint[eid]; >> struct ath10k_skb_cb *skb_cb = ATH10K_SKB_CB(skb); >> @@ -167,6 +167,10 @@ int ath10k_htc_send(struct ath10k_htc *htc, >> goto err_pull; >> } >> ep->tx_credits -= credits; >> + ath10k_dbg(ATH10K_DBG_HTC, >> + "ep %d used %d credits, remaining %d dbg %d (0x%x)\n", >> + eid, credits, ep->tx_credits, dbg, dbg); >> + >> spin_unlock_bh(&htc->tx_lock); >> } > > [...] > >> @@ -680,7 +684,7 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc, >> >> reinit_completion(&htc->ctl_resp); >> >> - status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb); >> + status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb, __LINE__); > > Using line numbers in debug messages is very cumbersome. Some people > cherry pick patches, have their own changes and whatnot which will make > it more difficult to read the debug logs. Isn't there any better way to > do this? I would prefer to explicitly wait for tx credit replenishment in ath10k_wmi_cmd_send() after a command is sent instead of all these prints. This way you can get a full call trace if it times out. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/24/2014 04:21 AM, Michal Kazior wrote: > On 24 March 2014 12:12, Kalle Valo <kvalo@qca.qualcomm.com> wrote: >>> - status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb); >>> + status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb, __LINE__); >> >> Using line numbers in debug messages is very cumbersome. Some people >> cherry pick patches, have their own changes and whatnot which will make >> it more difficult to read the debug logs. Isn't there any better way to >> do this? > > I would prefer to explicitly wait for tx credit replenishment in > ath10k_wmi_cmd_send() after a command is sent instead of all these > prints. This way you can get a full call trace if it times out. Would that be a performance problem (by effectively allowing only a single item to be sent to the firmware at any given time?) Thanks, Ben
On 03/24/2014 04:12 AM, Kalle Valo wrote: > greearb@candelatech.com writes: > >> From: Ben Greear <greearb@candelatech.com> >> >> This helps track tx credits accounting and usage. If >> firmware hangs or otherwise fails to return credits, one >> can more easily see the last few command types that >> was send to the firmware. >> >> Signed-off-by: Ben Greear <greearb@candelatech.com> > > This patchset didn't apply, and the sha1 ids were useless so that 3-way > merge didn't work. Can you rebase, please? And better yet if you rebase > in top of my master branch. I will do this. I must have some conflicting patch between your top of tree and this patch in my tree... >> - status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb); >> + status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb, __LINE__); > > Using line numbers in debug messages is very cumbersome. Some people > cherry pick patches, have their own changes and whatnot which will make > it more difficult to read the debug logs. Isn't there any better way to > do this? It is nasty, but I wanted to keep performance overhead light. I could do an enum instead if you think that would be an improvement. >> --- a/drivers/net/wireless/ath/ath10k/wmi.c >> +++ b/drivers/net/wireless/ath/ath10k/wmi.c >> @@ -544,7 +544,7 @@ static int ath10k_wmi_cmd_send_nowait(struct ath10k *ar, struct sk_buff *skb, >> cmd_hdr->cmd_id = __cpu_to_le32(cmd); >> >> memset(skb_cb, 0, sizeof(*skb_cb)); >> - ret = ath10k_htc_send(&ar->htc, ar->wmi.eid, skb); >> + ret = ath10k_htc_send(&ar->htc, ar->wmi.eid, skb, 0x71100000 | cmd_id); >> trace_ath10k_wmi_cmd(cmd_id, skb->data, skb->len, ret); > > What does 0x71100000 stand for? It's just something that is easily grepped for, and is obviously not a line number. Hopefully someone will have a better idea how to do this sort of thing, as my patch is pretty far down the slope towards total hack :) Thanks, Ben
On 24 March 2014 17:50, Ben Greear <greearb@candelatech.com> wrote: > On 03/24/2014 04:21 AM, Michal Kazior wrote: >> On 24 March 2014 12:12, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > >>>> - status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb); >>>> + status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb, __LINE__); >>> >>> Using line numbers in debug messages is very cumbersome. Some people >>> cherry pick patches, have their own changes and whatnot which will make >>> it more difficult to read the debug logs. Isn't there any better way to >>> do this? >> >> I would prefer to explicitly wait for tx credit replenishment in >> ath10k_wmi_cmd_send() after a command is sent instead of all these >> prints. This way you can get a full call trace if it times out. > > Would that be a performance problem (by effectively allowing only a single > item to be sent to the firmware at any given time?) The only thing that may suffer here is wmi mgmt tx but I wouldn't consider that performance critical. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/25/2014 12:27 AM, Michal Kazior wrote: > On 24 March 2014 17:50, Ben Greear <greearb@candelatech.com> wrote: >> On 03/24/2014 04:21 AM, Michal Kazior wrote: >>> On 24 March 2014 12:12, Kalle Valo <kvalo@qca.qualcomm.com> wrote: >> >>>>> - status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb); >>>>> + status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb, __LINE__); >>>> >>>> Using line numbers in debug messages is very cumbersome. Some people >>>> cherry pick patches, have their own changes and whatnot which will make >>>> it more difficult to read the debug logs. Isn't there any better way to >>>> do this? >>> >>> I would prefer to explicitly wait for tx credit replenishment in >>> ath10k_wmi_cmd_send() after a command is sent instead of all these >>> prints. This way you can get a full call trace if it times out. >> >> Would that be a performance problem (by effectively allowing only a single >> item to be sent to the firmware at any given time?) > > The only thing that may suffer here is wmi mgmt tx but I wouldn't > consider that performance critical. In that case, why use credits at all? Thanks, Ben > > > Micha? > -- > To unsubscribe from this list: send the line "unsubscribe linux-wireless" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On 25 March 2014 16:14, Ben Greear <greearb@candelatech.com> wrote: > On 03/25/2014 12:27 AM, Michal Kazior wrote: >> >> On 24 March 2014 17:50, Ben Greear <greearb@candelatech.com> wrote: >>> >>> On 03/24/2014 04:21 AM, Michal Kazior wrote: >>>> >>>> On 24 March 2014 12:12, Kalle Valo <kvalo@qca.qualcomm.com> wrote: >>> >>> >>>>>> - status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb); >>>>>> + status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb, __LINE__); >>>>> >>>>> >>>>> Using line numbers in debug messages is very cumbersome. Some people >>>>> cherry pick patches, have their own changes and whatnot which will make >>>>> it more difficult to read the debug logs. Isn't there any better way to >>>>> do this? >>>> >>>> >>>> I would prefer to explicitly wait for tx credit replenishment in >>>> ath10k_wmi_cmd_send() after a command is sent instead of all these >>>> prints. This way you can get a full call trace if it times out. >>> >>> >>> Would that be a performance problem (by effectively allowing only a >>> single >>> item to be sent to the firmware at any given time?) >> >> >> The only thing that may suffer here is wmi mgmt tx but I wouldn't >> consider that performance critical. > > > In that case, why use credits at all? Excellent question. Firmware provides 2 HTC tx credits for WMI. I guess this is on purpose. I imagine WMI mgmt tx can hang (i.e. not replenish tx credits). In that case the remaining tx credit could be used to try recovering, e.g. down/stop vdev (I think it should drop all pending tx and thus free the stuck HTC tx credit). It might be a good idea to keep this recovery strategy in mind when implementing the "wait for tx replenishment" wmi tx blocking. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/26/2014 12:18 AM, Michal Kazior wrote: > On 25 March 2014 16:14, Ben Greear <greearb@candelatech.com> wrote: >> On 03/25/2014 12:27 AM, Michal Kazior wrote: >>> >>> On 24 March 2014 17:50, Ben Greear <greearb@candelatech.com> wrote: >>>> >>>> On 03/24/2014 04:21 AM, Michal Kazior wrote: >>>>> >>>>> On 24 March 2014 12:12, Kalle Valo <kvalo@qca.qualcomm.com> wrote: >>>> >>>> >>>>>>> - status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb); >>>>>>> + status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb, __LINE__); >>>>>> >>>>>> >>>>>> Using line numbers in debug messages is very cumbersome. Some people >>>>>> cherry pick patches, have their own changes and whatnot which will make >>>>>> it more difficult to read the debug logs. Isn't there any better way to >>>>>> do this? >>>>> >>>>> >>>>> I would prefer to explicitly wait for tx credit replenishment in >>>>> ath10k_wmi_cmd_send() after a command is sent instead of all these >>>>> prints. This way you can get a full call trace if it times out. >>>> >>>> >>>> Would that be a performance problem (by effectively allowing only a >>>> single >>>> item to be sent to the firmware at any given time?) >>> >>> >>> The only thing that may suffer here is wmi mgmt tx but I wouldn't >>> consider that performance critical. >> >> >> In that case, why use credits at all? > > Excellent question. > > Firmware provides 2 HTC tx credits for WMI. I guess this is on > purpose. I imagine WMI mgmt tx can hang (i.e. not replenish tx > credits). In that case the remaining tx credit could be used to try > recovering, e.g. down/stop vdev (I think it should drop all pending tx > and thus free the stuck HTC tx credit). > > It might be a good idea to keep this recovery strategy in mind when > implementing the "wait for tx replenishment" wmi tx blocking. Seems to me we should just fix the firmware to respond quickly..and if we know exactly which command is not being answered (quickly) in the firmware, it might be easier to debug it. That said, while debugging I think I have seen cases where one message would be consumed for a long time, and then suddenly it frees up again. So, I'm not sure what to do. Maybe could improve firmware to return an id so we know exactly which of the messages was processed, and then the driver could do the work of figuring out what commands are outstanding by just keeping a copy in RAM.... Thanks, Ben > > > Micha? >
On 26 March 2014 15:40, Ben Greear <greearb@candelatech.com> wrote: > On 03/26/2014 12:18 AM, Michal Kazior wrote: >> >> On 25 March 2014 16:14, Ben Greear <greearb@candelatech.com> wrote: >>> >>> On 03/25/2014 12:27 AM, Michal Kazior wrote: >>>> >>>> >>>> On 24 March 2014 17:50, Ben Greear <greearb@candelatech.com> wrote: >>>>> >>>>> >>>>> On 03/24/2014 04:21 AM, Michal Kazior wrote: >>>>>> >>>>>> >>>>>> On 24 March 2014 12:12, Kalle Valo <kvalo@qca.qualcomm.com> wrote: >>>>> >>>>> >>>>> >>>>>>>> - status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb); >>>>>>>> + status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb, __LINE__); >>>>>>> >>>>>>> >>>>>>> >>>>>>> Using line numbers in debug messages is very cumbersome. Some people >>>>>>> cherry pick patches, have their own changes and whatnot which will >>>>>>> make >>>>>>> it more difficult to read the debug logs. Isn't there any better way >>>>>>> to >>>>>>> do this? >>>>>> >>>>>> >>>>>> >>>>>> I would prefer to explicitly wait for tx credit replenishment in >>>>>> ath10k_wmi_cmd_send() after a command is sent instead of all these >>>>>> prints. This way you can get a full call trace if it times out. >>>>> >>>>> >>>>> >>>>> Would that be a performance problem (by effectively allowing only a >>>>> single >>>>> item to be sent to the firmware at any given time?) >>>> >>>> >>>> >>>> The only thing that may suffer here is wmi mgmt tx but I wouldn't >>>> consider that performance critical. >>> >>> >>> >>> In that case, why use credits at all? >> >> >> Excellent question. >> >> Firmware provides 2 HTC tx credits for WMI. I guess this is on >> purpose. I imagine WMI mgmt tx can hang (i.e. not replenish tx >> credits). In that case the remaining tx credit could be used to try >> recovering, e.g. down/stop vdev (I think it should drop all pending tx >> and thus free the stuck HTC tx credit). >> >> It might be a good idea to keep this recovery strategy in mind when >> implementing the "wait for tx replenishment" wmi tx blocking. > > > Seems to me we should just fix the firmware to respond quickly..and if we > know exactly which > command is not being answered (quickly) in the firmware, it might be easier > to debug > it. > > That said, while debugging I think I have seen cases where one message would > be consumed for a long time, and then suddenly it frees up again. > > So, I'm not sure what to do. Maybe could improve firmware to return an id > so we know exactly which of the messages was processed, and then the driver > could do the work of figuring out what commands are outstanding by just > keeping > a copy in RAM.... > I suspect mgmt_tx frames (for AP FW we send them using WMI) without ACK couse this problem (disassoc + deauth probably). Question if we saw this problem with 636 firmware? In 636 we send mgmt frames using HTT, so we don't consume WMI credits. I will try to reproduce this tommorow. BR Janusz -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c index ba3084c..bca5c41 100644 --- a/drivers/net/wireless/ath/ath10k/htc.c +++ b/drivers/net/wireless/ath/ath10k/htc.c @@ -121,7 +121,7 @@ static void ath10k_htc_prepare_tx_skb(struct ath10k_htc_ep *ep, int ath10k_htc_send(struct ath10k_htc *htc, enum ath10k_htc_ep_id eid, - struct sk_buff *skb) + struct sk_buff *skb, int dbg) { struct ath10k_htc_ep *ep = &htc->endpoint[eid]; struct ath10k_skb_cb *skb_cb = ATH10K_SKB_CB(skb); @@ -167,6 +167,10 @@ int ath10k_htc_send(struct ath10k_htc *htc, goto err_pull; } ep->tx_credits -= credits; + ath10k_dbg(ATH10K_DBG_HTC, + "ep %d used %d credits, remaining %d dbg %d (0x%x)\n", + eid, credits, ep->tx_credits, dbg, dbg); + spin_unlock_bh(&htc->tx_lock); } @@ -244,13 +248,13 @@ ath10k_htc_process_credit_report(struct ath10k_htc *htc, if (report->eid >= ATH10K_HTC_EP_COUNT) break; - ath10k_dbg(ATH10K_DBG_HTC, "ep %d got %d credits\n", - report->eid, report->credits); - ep = &htc->endpoint[report->eid]; ep->tx_credits += report->credits; htc->ar->no_tx_credits_at = 0; + ath10k_dbg(ATH10K_DBG_HTC, "ep %d got %d credits tot %d\n", + report->eid, report->credits, ep->tx_credits); + if (ep->ep_ops.ep_tx_credits) { spin_unlock_bh(&htc->tx_lock); ep->ep_ops.ep_tx_credits(htc->ar); @@ -680,7 +684,7 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc, reinit_completion(&htc->ctl_resp); - status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb); + status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb, __LINE__); if (status) { kfree_skb(skb); return status; @@ -826,7 +830,7 @@ int ath10k_htc_start(struct ath10k_htc *htc) ath10k_dbg(ATH10K_DBG_HTC, "HTC is using TX credit flow control\n"); - status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb); + status = ath10k_htc_send(htc, ATH10K_HTC_EP_0, skb, __LINE__); if (status) { kfree_skb(skb); return status; diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h index 4716d33..b86dc2b 100644 --- a/drivers/net/wireless/ath/ath10k/htc.h +++ b/drivers/net/wireless/ath/ath10k/htc.h @@ -356,7 +356,7 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc, struct ath10k_htc_svc_conn_req *conn_req, struct ath10k_htc_svc_conn_resp *conn_resp); int ath10k_htc_send(struct ath10k_htc *htc, enum ath10k_htc_ep_id eid, - struct sk_buff *packet); + struct sk_buff *packet, int dbg); void ath10k_htc_stop(struct ath10k_htc *htc); struct sk_buff *ath10k_htc_alloc_skb(int size); diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c index 7a3e2e4..22a4542 100644 --- a/drivers/net/wireless/ath/ath10k/htt_tx.c +++ b/drivers/net/wireless/ath/ath10k/htt_tx.c @@ -173,7 +173,7 @@ int ath10k_htt_h2t_ver_req_msg(struct ath10k_htt *htt) cmd = (struct htt_cmd *)skb->data; cmd->hdr.msg_type = HTT_H2T_MSG_TYPE_VERSION_REQ; - ret = ath10k_htc_send(&htt->ar->htc, htt->eid, skb); + ret = ath10k_htc_send(&htt->ar->htc, htt->eid, skb, __LINE__); if (ret) { dev_kfree_skb_any(skb); return ret; @@ -212,7 +212,7 @@ int ath10k_htt_h2t_stats_req(struct ath10k_htt *htt, u8 mask, u64 cookie) req->cookie_lsb = cpu_to_le32(cookie & 0xffffffff); req->cookie_msb = cpu_to_le32((cookie & 0xffffffff00000000ULL) >> 32); - ret = ath10k_htc_send(&htt->ar->htc, htt->eid, skb); + ret = ath10k_htc_send(&htt->ar->htc, htt->eid, skb, __LINE__); if (ret) { ath10k_warn("failed to send htt type stats request: %d", ret); dev_kfree_skb_any(skb); @@ -298,7 +298,7 @@ int ath10k_htt_send_rx_ring_cfg_ll(struct ath10k_htt *htt) #undef desc_offset - ret = ath10k_htc_send(&htt->ar->htc, htt->eid, skb); + ret = ath10k_htc_send(&htt->ar->htc, htt->eid, skb, __LINE__); if (ret) { dev_kfree_skb_any(skb); return ret; @@ -360,7 +360,7 @@ int ath10k_htt_mgmt_tx(struct ath10k_htt *htt, struct sk_buff *msdu) skb_cb->htt.txbuf = NULL; - res = ath10k_htc_send(&htt->ar->htc, htt->eid, txdesc); + res = ath10k_htc_send(&htt->ar->htc, htt->eid, txdesc, __LINE__); if (res) goto err_unmap_msdu; diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c index 8b99e17..a8827e2 100644 --- a/drivers/net/wireless/ath/ath10k/wmi.c +++ b/drivers/net/wireless/ath/ath10k/wmi.c @@ -544,7 +544,7 @@ static int ath10k_wmi_cmd_send_nowait(struct ath10k *ar, struct sk_buff *skb, cmd_hdr->cmd_id = __cpu_to_le32(cmd); memset(skb_cb, 0, sizeof(*skb_cb)); - ret = ath10k_htc_send(&ar->htc, ar->wmi.eid, skb); + ret = ath10k_htc_send(&ar->htc, ar->wmi.eid, skb, 0x71100000 | cmd_id); trace_ath10k_wmi_cmd(cmd_id, skb->data, skb->len, ret); if (ret)