| Message ID | 1459368643-13300-1-git-send-email-greearb@candelatech.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On 30 March 2016 at 22:10, <greearb@candelatech.com> wrote: > From: Ben Greear <greearb@candelatech.com> > > I believe the CE tx buffer reaping logic may be able to fall > behind in certain cases (lots of serial console logging, lots > of WMI messages). > > Dropping WMI messages is a very serious problem, so it is worth > waiting a bit in hopes the tx buffers become available again. > > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > > Probably the ath10k_err should be made dbg or rate-limited before > this goes upstream..in meantime, it might help shed some light on > this problem. > > drivers/net/wireless/ath/ath10k/wmi.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c > index f042711..43d23fc 100644 > --- a/drivers/net/wireless/ath/ath10k/wmi.c > +++ b/drivers/net/wireless/ath/ath10k/wmi.c > @@ -1819,6 +1819,7 @@ static void ath10k_wmi_op_ep_tx_credits(struct ath10k *ar) > int ath10k_wmi_cmd_send(struct ath10k *ar, struct sk_buff *skb, u32 cmd_id) > { > int ret = -EOPNOTSUPP; > + int retry = 1000; > > might_sleep(); > > @@ -1832,7 +1833,19 @@ int ath10k_wmi_cmd_send(struct ath10k *ar, struct sk_buff *skb, u32 cmd_id) > /* try to send pending beacons first. they take priority */ > ath10k_wmi_tx_beacons_nowait(ar); > > - ret = ath10k_wmi_cmd_send_nowait(ar, skb, cmd_id); > + while (--retry) { > + ret = ath10k_wmi_cmd_send_nowait(ar, skb, cmd_id); > + if ((ret == -ENOBUFS) && > + !test_bit(ATH10K_FLAG_CRASH_FLUSH, &ar->dev_flags)) { > + /* CE transport logic is full, maybe we cannot reap entries fast > + * enough? > + */ > + ath10k_err(ar, "CE transport is full, sleeping for 1ms\n"); > + msleep(1); > + continue; > + } > + break; > + } This looks like a workaround to me. This problem shouldn't be happening in the first place as far as design is concerned. If it does the only reason I can think of is if MSI-range support is exercised. Anyway, It'd be a lot more sane to instead poll WMI's CE Tx pipe when processing WMI's CE Rx pipe (but that's still -arguably- unnecessary) instead of retrying in WMI.. Micha?
On 30 March 2016 at 23:55, Michal Kazior <michal.kazior@tieto.com> wrote: > On 30 March 2016 at 22:10, <greearb@candelatech.com> wrote: >> From: Ben Greear <greearb@candelatech.com> >> >> I believe the CE tx buffer reaping logic may be able to fall >> behind in certain cases (lots of serial console logging, lots >> of WMI messages). >> >> Dropping WMI messages is a very serious problem, so it is worth >> waiting a bit in hopes the tx buffers become available again. >> >> Signed-off-by: Ben Greear <greearb@candelatech.com> >> --- >> >> Probably the ath10k_err should be made dbg or rate-limited before >> this goes upstream..in meantime, it might help shed some light on >> this problem. >> >> drivers/net/wireless/ath/ath10k/wmi.c | 15 ++++++++++++++- >> 1 file changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c >> index f042711..43d23fc 100644 >> --- a/drivers/net/wireless/ath/ath10k/wmi.c >> +++ b/drivers/net/wireless/ath/ath10k/wmi.c >> @@ -1819,6 +1819,7 @@ static void ath10k_wmi_op_ep_tx_credits(struct ath10k *ar) >> int ath10k_wmi_cmd_send(struct ath10k *ar, struct sk_buff *skb, u32 cmd_id) >> { >> int ret = -EOPNOTSUPP; >> + int retry = 1000; >> >> might_sleep(); >> >> @@ -1832,7 +1833,19 @@ int ath10k_wmi_cmd_send(struct ath10k *ar, struct sk_buff *skb, u32 cmd_id) >> /* try to send pending beacons first. they take priority */ >> ath10k_wmi_tx_beacons_nowait(ar); >> >> - ret = ath10k_wmi_cmd_send_nowait(ar, skb, cmd_id); >> + while (--retry) { >> + ret = ath10k_wmi_cmd_send_nowait(ar, skb, cmd_id); >> + if ((ret == -ENOBUFS) && >> + !test_bit(ATH10K_FLAG_CRASH_FLUSH, &ar->dev_flags)) { >> + /* CE transport logic is full, maybe we cannot reap entries fast >> + * enough? >> + */ >> + ath10k_err(ar, "CE transport is full, sleeping for 1ms\n"); >> + msleep(1); >> + continue; >> + } >> + break; >> + } > > This looks like a workaround to me. This problem shouldn't be > happening in the first place as far as design is concerned. > > If it does the only reason I can think of is if MSI-range support is exercised. > > Anyway, It'd be a lot more sane to instead poll WMI's CE Tx pipe when > processing WMI's CE Rx pipe (but that's still -arguably- unnecessary) > instead of retrying in WMI.. I think polling TX when processing WMI RX isn't a bad idea. If Ben's doing a lot of WMI stuff then the interrupt latency or scheduling may be getting in the way. But I'd like to first verify that the problem isn't something silly like you're not seeing the interrupt because it didn't fire in the first place... -adrian
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c index f042711..43d23fc 100644 --- a/drivers/net/wireless/ath/ath10k/wmi.c +++ b/drivers/net/wireless/ath/ath10k/wmi.c @@ -1819,6 +1819,7 @@ static void ath10k_wmi_op_ep_tx_credits(struct ath10k *ar) int ath10k_wmi_cmd_send(struct ath10k *ar, struct sk_buff *skb, u32 cmd_id) { int ret = -EOPNOTSUPP; + int retry = 1000; might_sleep(); @@ -1832,7 +1833,19 @@ int ath10k_wmi_cmd_send(struct ath10k *ar, struct sk_buff *skb, u32 cmd_id) /* try to send pending beacons first. they take priority */ ath10k_wmi_tx_beacons_nowait(ar); - ret = ath10k_wmi_cmd_send_nowait(ar, skb, cmd_id); + while (--retry) { + ret = ath10k_wmi_cmd_send_nowait(ar, skb, cmd_id); + if ((ret == -ENOBUFS) && + !test_bit(ATH10K_FLAG_CRASH_FLUSH, &ar->dev_flags)) { + /* CE transport logic is full, maybe we cannot reap entries fast + * enough? + */ + ath10k_err(ar, "CE transport is full, sleeping for 1ms\n"); + msleep(1); + continue; + } + break; + } if (ret && test_bit(ATH10K_FLAG_CRASH_FLUSH, &ar->dev_flags)) ret = -ESHUTDOWN;