* [PATCH net v2 1/3] net: ipa: fix atomic update in ipa_endpoint_replenish()
2022-01-12 13:30 [PATCH net v2 0/3] net: ipa: fix two replenish bugs Alex Elder
@ 2022-01-12 13:30 ` Alex Elder
2022-01-12 13:30 ` [PATCH net v2 2/3] net: ipa: use a bitmap for endpoint replenish_enabled Alex Elder
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Alex Elder @ 2022-01-12 13:30 UTC (permalink / raw)
To: davem, kuba
Cc: jponduru, avuyyuru, bjorn.andersson, agross, cpratapa, subashab,
mka, evgreen, elder, netdev, linux-arm-msm, linux-kernel
In ipa_endpoint_replenish(), if an error occurs when attempting to
replenish a receive buffer, we just quit and try again later. In
that case we increment the backlog count to reflect that the attempt
was unsuccessful. Then, if the add_one flag was true we increment
the backlog again.
This second increment is not included in the backlog local variable
though, and its value determines whether delayed work should be
scheduled. This is a bug.
Fix this by determining whether 1 or 2 should be added to the
backlog before adding it in a atomic_add_return() call.
Reviewed-by: Matthias Kaehlcke <mka@chromium•org>
Fixes: 84f9bd12d46db ("soc: qcom: ipa: IPA endpoints")
Signed-off-by: Alex Elder <elder@linaro•org>
---
drivers/net/ipa/ipa_endpoint.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index 49d9a077d0375..8b055885cf3cf 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -1080,6 +1080,7 @@ static void ipa_endpoint_replenish(struct ipa_endpoint *endpoint, bool add_one)
{
struct gsi *gsi;
u32 backlog;
+ int delta;
if (!endpoint->replenish_enabled) {
if (add_one)
@@ -1097,10 +1098,8 @@ static void ipa_endpoint_replenish(struct ipa_endpoint *endpoint, bool add_one)
try_again_later:
/* The last one didn't succeed, so fix the backlog */
- backlog = atomic_inc_return(&endpoint->replenish_backlog);
-
- if (add_one)
- atomic_inc(&endpoint->replenish_backlog);
+ delta = add_one ? 2 : 1;
+ backlog = atomic_add_return(delta, &endpoint->replenish_backlog);
/* Whenever a receive buffer transaction completes we'll try to
* replenish again. It's unlikely, but if we fail to supply even
--
2.32.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH net v2 2/3] net: ipa: use a bitmap for endpoint replenish_enabled
2022-01-12 13:30 [PATCH net v2 0/3] net: ipa: fix two replenish bugs Alex Elder
2022-01-12 13:30 ` [PATCH net v2 1/3] net: ipa: fix atomic update in ipa_endpoint_replenish() Alex Elder
@ 2022-01-12 13:30 ` Alex Elder
2022-01-12 13:30 ` [PATCH net v2 3/3] net: ipa: prevent concurrent replenish Alex Elder
2022-01-12 14:50 ` [PATCH net v2 0/3] net: ipa: fix two replenish bugs patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: Alex Elder @ 2022-01-12 13:30 UTC (permalink / raw)
To: davem, kuba
Cc: jponduru, avuyyuru, bjorn.andersson, agross, cpratapa, subashab,
mka, evgreen, elder, netdev, linux-arm-msm, linux-kernel
Define a new replenish_flags bitmap to contain Boolean flags
associated with an endpoint's replenishing state. Replace the
replenish_enabled field with a flag in that bitmap. This is to
prepare for the next patch, which adds another flag.
Signed-off-by: Alex Elder <elder@linaro•org>
---
v2: This change was not present in version 1
drivers/net/ipa/ipa_endpoint.c | 8 ++++----
drivers/net/ipa/ipa_endpoint.h | 15 +++++++++++++--
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c
index 8b055885cf3cf..cddddcedaf72b 100644
--- a/drivers/net/ipa/ipa_endpoint.c
+++ b/drivers/net/ipa/ipa_endpoint.c
@@ -1082,7 +1082,7 @@ static void ipa_endpoint_replenish(struct ipa_endpoint *endpoint, bool add_one)
u32 backlog;
int delta;
- if (!endpoint->replenish_enabled) {
+ if (!test_bit(IPA_REPLENISH_ENABLED, endpoint->replenish_flags)) {
if (add_one)
atomic_inc(&endpoint->replenish_saved);
return;
@@ -1119,7 +1119,7 @@ static void ipa_endpoint_replenish_enable(struct ipa_endpoint *endpoint)
u32 max_backlog;
u32 saved;
- endpoint->replenish_enabled = true;
+ set_bit(IPA_REPLENISH_ENABLED, endpoint->replenish_flags);
while ((saved = atomic_xchg(&endpoint->replenish_saved, 0)))
atomic_add(saved, &endpoint->replenish_backlog);
@@ -1133,7 +1133,7 @@ static void ipa_endpoint_replenish_disable(struct ipa_endpoint *endpoint)
{
u32 backlog;
- endpoint->replenish_enabled = false;
+ clear_bit(IPA_REPLENISH_ENABLED, endpoint->replenish_flags);
while ((backlog = atomic_xchg(&endpoint->replenish_backlog, 0)))
atomic_add(backlog, &endpoint->replenish_saved);
}
@@ -1690,7 +1690,7 @@ static void ipa_endpoint_setup_one(struct ipa_endpoint *endpoint)
/* RX transactions require a single TRE, so the maximum
* backlog is the same as the maximum outstanding TREs.
*/
- endpoint->replenish_enabled = false;
+ clear_bit(IPA_REPLENISH_ENABLED, endpoint->replenish_flags);
atomic_set(&endpoint->replenish_saved,
gsi_channel_tre_max(gsi, endpoint->channel_id));
atomic_set(&endpoint->replenish_backlog, 0);
diff --git a/drivers/net/ipa/ipa_endpoint.h b/drivers/net/ipa/ipa_endpoint.h
index 0a859d10312dc..07d5c20e5f000 100644
--- a/drivers/net/ipa/ipa_endpoint.h
+++ b/drivers/net/ipa/ipa_endpoint.h
@@ -40,6 +40,17 @@ enum ipa_endpoint_name {
#define IPA_ENDPOINT_MAX 32 /* Max supported by driver */
+/**
+ * enum ipa_replenish_flag: RX buffer replenish flags
+ *
+ * @IPA_REPLENISH_ENABLED: Whether receive buffer replenishing is enabled
+ * @IPA_REPLENISH_COUNT: Number of defined replenish flags
+ */
+enum ipa_replenish_flag {
+ IPA_REPLENISH_ENABLED,
+ IPA_REPLENISH_COUNT, /* Number of flags (must be last) */
+};
+
/**
* struct ipa_endpoint - IPA endpoint information
* @ipa: IPA pointer
@@ -51,7 +62,7 @@ enum ipa_endpoint_name {
* @trans_tre_max: Maximum number of TRE descriptors per transaction
* @evt_ring_id: GSI event ring used by the endpoint
* @netdev: Network device pointer, if endpoint uses one
- * @replenish_enabled: Whether receive buffer replenishing is enabled
+ * @replenish_flags: Replenishing state flags
* @replenish_ready: Number of replenish transactions without doorbell
* @replenish_saved: Replenish requests held while disabled
* @replenish_backlog: Number of buffers needed to fill hardware queue
@@ -72,7 +83,7 @@ struct ipa_endpoint {
struct net_device *netdev;
/* Receive buffer replenishing for RX endpoints */
- bool replenish_enabled;
+ DECLARE_BITMAP(replenish_flags, IPA_REPLENISH_COUNT);
u32 replenish_ready;
atomic_t replenish_saved;
atomic_t replenish_backlog;
--
2.32.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH net v2 3/3] net: ipa: prevent concurrent replenish
2022-01-12 13:30 [PATCH net v2 0/3] net: ipa: fix two replenish bugs Alex Elder
2022-01-12 13:30 ` [PATCH net v2 1/3] net: ipa: fix atomic update in ipa_endpoint_replenish() Alex Elder
2022-01-12 13:30 ` [PATCH net v2 2/3] net: ipa: use a bitmap for endpoint replenish_enabled Alex Elder
@ 2022-01-12 13:30 ` Alex Elder
2022-01-12 14:50 ` [PATCH net v2 0/3] net: ipa: fix two replenish bugs patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: Alex Elder @ 2022-01-12 13:30 UTC (permalink / raw)
To: davem, kuba
Cc: jponduru, avuyyuru, bjorn.andersson, agross, cpratapa, subashab,
mka, evgreen, elder, netdev, linux-arm-msm, linux-kernel
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 flag to ensure only one replenish instance for an endpoint
executes at a time.
Fixes: 84f9bd12d46db ("soc: qcom: ipa: IPA endpoints")
Signed-off-by: Alex Elder <elder@linaro•org>
---
v2: Use test_and_set_bit() and clear_bit(), as Jakub suggested
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 cddddcedaf72b..68291a3efd040 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 (test_and_set_bit(IPA_REPLENISH_ACTIVE, endpoint->replenish_flags)) {
+ 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;
+
+ clear_bit(IPA_REPLENISH_ACTIVE, endpoint->replenish_flags);
+
if (add_one)
atomic_inc(&endpoint->replenish_backlog);
return;
try_again_later:
+ clear_bit(IPA_REPLENISH_ACTIVE, endpoint->replenish_flags);
+
/* The last one didn't succeed, so fix the backlog */
delta = add_one ? 2 : 1;
backlog = atomic_add_return(delta, &endpoint->replenish_backlog);
@@ -1691,6 +1703,7 @@ static void ipa_endpoint_setup_one(struct ipa_endpoint *endpoint)
* backlog is the same as the maximum outstanding TREs.
*/
clear_bit(IPA_REPLENISH_ENABLED, endpoint->replenish_flags);
+ clear_bit(IPA_REPLENISH_ACTIVE, endpoint->replenish_flags);
atomic_set(&endpoint->replenish_saved,
gsi_channel_tre_max(gsi, endpoint->channel_id));
atomic_set(&endpoint->replenish_backlog, 0);
diff --git a/drivers/net/ipa/ipa_endpoint.h b/drivers/net/ipa/ipa_endpoint.h
index 07d5c20e5f000..0313cdc607de3 100644
--- a/drivers/net/ipa/ipa_endpoint.h
+++ b/drivers/net/ipa/ipa_endpoint.h
@@ -44,10 +44,12 @@ enum ipa_endpoint_name {
* enum ipa_replenish_flag: RX buffer replenish flags
*
* @IPA_REPLENISH_ENABLED: Whether receive buffer replenishing is enabled
+ * @IPA_REPLENISH_ACTIVE: Whether replenishing is underway
* @IPA_REPLENISH_COUNT: Number of defined replenish flags
*/
enum ipa_replenish_flag {
IPA_REPLENISH_ENABLED,
+ IPA_REPLENISH_ACTIVE,
IPA_REPLENISH_COUNT, /* Number of flags (must be last) */
};
--
2.32.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net v2 0/3] net: ipa: fix two replenish bugs
2022-01-12 13:30 [PATCH net v2 0/3] net: ipa: fix two replenish bugs Alex Elder
` (2 preceding siblings ...)
2022-01-12 13:30 ` [PATCH net v2 3/3] net: ipa: prevent concurrent replenish Alex Elder
@ 2022-01-12 14:50 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 5+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-01-12 14:50 UTC (permalink / raw)
To: Alex Elder
Cc: davem, kuba, jponduru, avuyyuru, bjorn.andersson, agross,
cpratapa, subashab, mka, evgreen, elder, netdev, linux-arm-msm,
linux-kernel
Hello:
This series was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft•net>:
On Wed, 12 Jan 2022 07:30:09 -0600 you wrote:
> This series contains two fixes for bugs in the IPA receive buffer
> replenishing code. The (new) second patch defines a bitmap to
> represent endpoint the replenish enabled flag. Its purpose is to
> prepare for the third patch, which adds an additional flag.
>
> Version 2 of this series uses bitmap operations in the second bug
> fix rather than an atomic variable, as suggested by Jakub.
>
> [...]
Here is the summary with links:
- [net,v2,1/3] net: ipa: fix atomic update in ipa_endpoint_replenish()
https://git.kernel.org/netdev/net/c/6c0e3b5ce949
- [net,v2,2/3] net: ipa: use a bitmap for endpoint replenish_enabled
https://git.kernel.org/netdev/net/c/c1aaa01dbf4c
- [net,v2,3/3] net: ipa: prevent concurrent replenish
https://git.kernel.org/netdev/net/c/998c0bd2b371
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 5+ messages in thread