when we cannot get a tx buffer (`get_a_tx_buf`) `rpmsg_upref_sleepers`
enables tx-complete interrupt.
however if the interrupt is executed after `get_a_tx_buf` and before
`rpmsg_upref_sleepers` we may mis the tx-complete interrupt and sleep
for the full 15 seconds.
Is there any reason why your co-processor is unable to release the TX RPMSG
buffers for 15 seconds? If not, you should first determine the reason why it is
stalled.
Arnaud's concern is valid. If the remote processor can't consume a buffer
within 15 seconds, something is probably wrong.
That said, I believe your assesment of the situation is correct. *If* the TX
callback is disabled and there is no buffer available, there is a window of
opportunity between calls to get_a_tx_buf() and rpmsg_upref_sleepers() for an
interrupt to arrive in function rpmsg_send_offchannel_raw().
the remote processor certainly releases the tx buffer and according to
my tracing the `vring_interrupt` fires immediately before `rpmsg_send`
enters the `rpmsg_upref_sleepers`.
after applying this patch we haven't been able to reproduce the 15s
timeout anymore, whereas before we could easily reproduce it with
certain workloads.
3) This patch gets applied when rc1 comes out so that it has 6 or 7 weeks to
soak. No error are locks are reported due to this patch during that time.
mentioning locks: i was a bit uncertain about a good way to implement
the retry, since both `rpmsg_upref_sleepers` and `get_a_tx_buf` both
acquire the same mutex. i briefly considered to add `get_a_tx_buf` into
`rpmsg_upref_sleepers` to avoid locking the same mutex multiple times,
though it adds a bit of complexity to the implementation and harms
readability a bit.
are there any recommendations on this topic or are (likely
non-contended) locks not expensive enough to justify the added complexity?
thanks,
tim
Regards,
Arnaud
in this case, so we re-try once before we really start to sleep
Signed-off-by: Tim Blechmann <tim@xxxxxxxxxx>
---
drivers/rpmsg/virtio_rpmsg_bus.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 905ac7910c98..2a9d42225e60 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -587,21 +587,27 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
/* no free buffer ? wait for one (but bail after 15 seconds) */
while (!msg) {
/* enable "tx-complete" interrupts, if not already enabled */
rpmsg_upref_sleepers(vrp);
- /*
- * sleep until a free buffer is available or 15 secs elapse.
- * the timeout period is not configurable because there's
- * little point in asking drivers to specify that.
- * if later this happens to be required, it'd be easy to add.
- */
- err = wait_event_interruptible_timeout(vrp->sendq,
- (msg = get_a_tx_buf(vrp)),
- msecs_to_jiffies(15000));
+ /* make sure to retry to grab tx buffer before we start waiting */
+ msg = get_a_tx_buf(vrp);
+ if (msg) {
+ err = 0;
+ } else {
+ /*
+ * sleep until a free buffer is available or 15 secs elapse.
+ * the timeout period is not configurable because there's
+ * little point in asking drivers to specify that.
+ * if later this happens to be required, it'd be easy to add.
+ */
+ err = wait_event_interruptible_timeout(vrp->sendq,
+ (msg = get_a_tx_buf(vrp)),
+ msecs_to_jiffies(15000));
+ }
/* disable "tx-complete" interrupts if we're the last sleeper */
rpmsg_downref_sleepers(vrp);
/* timeout ? */
if (!err) {
