Re: [PATCH net 2/2] net: ipa: prevent concurrent replenish

On Tue, Jan 11, 2022 at 02:58:16PM -0600, Alex Elder wrote:

On 1/11/22 2:20 PM, Matthias Kaehlcke wrote:

quoted

On Tue, Jan 11, 2022 at 01:21:50PM -0600, Alex Elder wrote:

quoted

We have seen cases where an endpoint RX completion interrupt arrives
while replenishing for the endpoint is underway.  This causes another
instance of replenishing to begin as part of completing the receive
transaction.  If this occurs it can lead to transaction corruption.

Use a new atomic variable to ensure only replenish instance for an
endpoint executes at a time.

Fixes: 84f9bd12d46db ("soc: qcom: ipa: IPA endpoints")
Signed-off-by: Alex Elder <redacted>
---
  drivers/net/ipa/ipa_endpoint.c | 13 +++++++++++++
  drivers/net/ipa/ipa_endpoint.h |  2 ++
  2 files changed, 15 insertions(+)

diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index 8b055885cf3cf..a1019f5fe1748 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c

@@ -1088,15 +1088,27 @@ static void ipa_endpoint_replenish(struct ipa_endpoint *endpoint, bool add_one)
  		return;
  	}
+	/* If already active, just update the backlog */
+	if (atomic_xchg(&endpoint->replenish_active, 1)) {
+		if (add_one)
+			atomic_inc(&endpoint->replenish_backlog);
+		return;
+	}
+
  	while (atomic_dec_not_zero(&endpoint->replenish_backlog))
  		if (ipa_endpoint_replenish_one(endpoint))
  			goto try_again_later;

I think there is a race here, not sure whether it's a problem: If the first
interrupt is here just when a 2nd interrupt evaluates 'replenish_active' the
latter will return, since it looks like replenishing is still active, when it
actually just finished. Would replenishing be kicked off anyway shortly after
or could the transaction be stalled until another endpoint RX completion
interrupt arrives?

I acknowledge the race you point out.  You're saying another
thread could test the flag after the while loop exits, but
before the flag gets reset to 0.  And that means that other
thread would skip the replenishing it would otherwise do.

To be honest, that is a different scenario than the one I
was trying to prevent, because it involves two threads, rather
than one thread entering this function a second time (via an
interrupt).  But regardless, I think it's OK.

The replenishing loop is intentionally tolerant of errors.
It will send as many receive buffers to the hardware as there
is room for, but will stop and "try again later" if an
error occurs.   Even if the specific case you mention
occurred it wouldn't be a problem because we'd get another
shot at it.  I'll explain.


The replenish_backlog is the number of "open slots" the hardware
has to hold a receive buffer.  When the backlog reaches 0, the
hardware is "full."

When a receive operation completes (in ipa_endpoint_rx_complete())
it calls ipa_endpoint_replenish(), requesting that we add one to
the backlog (to account for the buffer just consumed).  What this
means is that if the hardware has *any* receive buffers, a
replenish will eventually be requested (when one completes).

The logic in ipa_endpoint_replenish() tolerates an error
attempting to send a new receive buffer to the hardware.
If that happens, we just try again later--knowing that
the next RX completion will trigger that retry.

The only case where we aren't guaranteed a subsequent call
to ipa_endpoint_replenish() is when the hardware has *zero*
receive buffers, or the backlog is the maximum.  In that
case we explicitly schedule a new replenish via delayed
work.

Now, two more points.
- If the while loop exits without an error replenishing,
  the hardware is "full", so there are buffers that will
  complete, and trigger a replenish call.  So if the race
  occurred, it would be harmless.
- If the while loop exits because of an error replenishing
  one buffer, it jumps to try_again_later.  In that case,
  either the hardware has at least one receive buffer (so
  we'll get another replenish), or it has none and we will
  schedule delayed work to to schedule another replenish.

So I think that even if the race you point out occurs, the
replenish logic will eventually get another try.

I don't claim my logic is flawless; if it's wrong I can
find another solution.

Thanks for the detailed description!

I agree the race isn't an actual issue as long as it is
guaranteed that the function is called again 'soon' after
the race occurred. It seems that should be always the case.