* [PATCH net 0/3] net: ipa: GSI interrupt handling fixes
@ 2020-12-22 18:00 Alex Elder
2020-12-22 18:00 ` [PATCH net 1/3] net: ipa: clear pending interrupts before enabling Alex Elder
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Alex Elder @ 2020-12-22 18:00 UTC (permalink / raw)
To: davem, kuba
Cc: evgreen, cpratapa, bjorn.andersson, subashab, netdev, linux-kernel
This series implements fixes for some issues related to handling
interrupts when GSI channel and event ring commands complete.
The first issue is that the completion condition for an event ring
or channel command could occur while the associated interrupt is
disabled. This would cause the interrupt to fire when it is
subsequently enabled, even if the condition it signals had already
been handled. The fix is to clear any pending interrupt conditions
before re-enabling the interrupt.
The second and third patches change how the success of an event ring
or channel command is determined. These commands change the state
of an event ring or channel. Previously the receipt of a completion
interrupt was required to consider a command successful. Instead, a
command is successful if it changes the state of the target event
ring or channel in the way expected. This way the command can
succeed even if the completion interrupt did not arrive while it was
enabled.
-Alex
Alex Elder (3):
net: ipa: clear pending interrupts before enabling
net: ipa: use state to determine channel command success
net: ipa: use state to determine event ring command success
drivers/net/ipa/gsi.c | 89 +++++++++++++++++++++++++++----------------
1 file changed, 56 insertions(+), 33 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH net 1/3] net: ipa: clear pending interrupts before enabling
2020-12-22 18:00 [PATCH net 0/3] net: ipa: GSI interrupt handling fixes Alex Elder
@ 2020-12-22 18:00 ` Alex Elder
2020-12-22 18:00 ` [PATCH net 2/3] net: ipa: use state to determine channel command success Alex Elder
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Alex Elder @ 2020-12-22 18:00 UTC (permalink / raw)
To: davem, kuba
Cc: evgreen, cpratapa, bjorn.andersson, subashab, netdev, linux-kernel
We enable the completion interrupt for channel or event ring
commands only when we issue them. The interrupt is disabled after
the interrupt has fired, or after we have timed out waiting for it.
If we time out, the command could complete after the interrupt has
been disabled, causing a state change in the channel or event ring.
The interrupt associated with that state change would be delivered
the next time the completion interrupt is enabled.
To avoid previous command completions interfering with new commands,
clear all pending completion interrupts before re-enabling them for
a new command.
Fixes: b4175f8731f78 ("net: ipa: only enable GSI event control IRQs when needed")
Signed-off-by: Alex Elder <elder@linaro.org>
---
drivers/net/ipa/gsi.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index c4795249719d4..4aee60d62ab09 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -340,7 +340,13 @@ static int evt_ring_command(struct gsi *gsi, u32 evt_ring_id,
* is issued here. Only permit *this* event ring to trigger
* an interrupt, and only enable the event control IRQ type
* when we expect it to occur.
+ *
+ * There's a small chance that a previous command completed
+ * after the interrupt was disabled, so make sure we have no
+ * pending interrupts before we enable them.
*/
+ iowrite32(~0, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_CLR_OFFSET);
+
val = BIT(evt_ring_id);
iowrite32(val, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_MSK_OFFSET);
gsi_irq_type_enable(gsi, GSI_EV_CTRL);
@@ -453,7 +459,13 @@ gsi_channel_command(struct gsi_channel *channel, enum gsi_ch_cmd_opcode opcode)
* issued here. So we only permit *this* channel to trigger
* an interrupt and only enable the channel control IRQ type
* when we expect it to occur.
+ *
+ * There's a small chance that a previous command completed
+ * after the interrupt was disabled, so make sure we have no
+ * pending interrupts before we enable them.
*/
+ iowrite32(~0, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_CLR_OFFSET);
+
val = BIT(channel_id);
iowrite32(val, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_MSK_OFFSET);
gsi_irq_type_enable(gsi, GSI_CH_CTRL);
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net 2/3] net: ipa: use state to determine channel command success
2020-12-22 18:00 [PATCH net 0/3] net: ipa: GSI interrupt handling fixes Alex Elder
2020-12-22 18:00 ` [PATCH net 1/3] net: ipa: clear pending interrupts before enabling Alex Elder
@ 2020-12-22 18:00 ` Alex Elder
2020-12-26 18:51 ` kernel test robot
2020-12-22 18:00 ` [PATCH net 3/3] net: ipa: use state to determine event ring " Alex Elder
2020-12-23 20:20 ` [PATCH net 0/3] net: ipa: GSI interrupt handling fixes patchwork-bot+netdevbpf
3 siblings, 1 reply; 6+ messages in thread
From: Alex Elder @ 2020-12-22 18:00 UTC (permalink / raw)
To: davem, kuba
Cc: evgreen, cpratapa, bjorn.andersson, subashab, netdev, linux-kernel
The result of issuing a channel control command should be that the
channel changes state. If enabled, a completion interrupt signals
that the channel state has changed. This interrupt is enabled by
gsi_channel_command() and disabled again after the command has
completed (or we time out).
There is a window of time--after the completion interrupt is disabled
but before the channel state is read--during which the command could
complete successfully without interrupting. This would cause the
channel to transition to the desired new state.
So whether a channel command ends via completion interrupt or
timeout, we can consider the command successful if the channel
has entered the desired state (and a failure if it has not,
regardless of the cause).
Fixes: d6c9e3f506ae8 ("net: ipa: only enable generic command completion IRQ when needed");
Signed-off-by: Alex Elder <elder@linaro.org>
---
drivers/net/ipa/gsi.c | 43 ++++++++++++++++++++++---------------------
1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 4aee60d62ab09..4f0e791764237 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -505,15 +505,15 @@ static int gsi_channel_alloc_command(struct gsi *gsi, u32 channel_id)
ret = gsi_channel_command(channel, GSI_CH_ALLOCATE);
- /* Channel state will normally have been updated */
+ /* If successful the channel state will have changed */
state = gsi_channel_state(channel);
- if (!ret && state != GSI_CHANNEL_STATE_ALLOCATED) {
- dev_err(dev, "channel %u bad state %u after alloc\n",
- channel_id, state);
- ret = -EIO;
- }
+ if (state == GSI_CHANNEL_STATE_ALLOCATED)
+ return 0;
- return ret;
+ dev_err(dev, "channel %u bad state %u after alloc\n",
+ channel_id, state);
+
+ return -EIO;
}
/* Start an ALLOCATED channel */
@@ -533,15 +533,15 @@ static int gsi_channel_start_command(struct gsi_channel *channel)
ret = gsi_channel_command(channel, GSI_CH_START);
- /* Channel state will normally have been updated */
+ /* If successful the channel state will have changed */
state = gsi_channel_state(channel);
- if (!ret && state != GSI_CHANNEL_STATE_STARTED) {
- dev_err(dev, "channel %u bad state %u after start\n",
- gsi_channel_id(channel), state);
- ret = -EIO;
- }
+ if (state == GSI_CHANNEL_STATE_STARTED)
+ return 0;
- return ret;
+ dev_err(dev, "channel %u bad state %u after start\n",
+ gsi_channel_id(channel), state);
+
+ return -EIO;
}
/* Stop a GSI channel in STARTED state */
@@ -568,10 +568,10 @@ static int gsi_channel_stop_command(struct gsi_channel *channel)
ret = gsi_channel_command(channel, GSI_CH_STOP);
- /* Channel state will normally have been updated */
+ /* If successful the channel state will have changed */
state = gsi_channel_state(channel);
- if (ret || state == GSI_CHANNEL_STATE_STOPPED)
- return ret;
+ if (state == GSI_CHANNEL_STATE_STOPPED)
+ return 0;
/* We may have to try again if stop is in progress */
if (state == GSI_CHANNEL_STATE_STOP_IN_PROC)
@@ -604,9 +604,9 @@ static void gsi_channel_reset_command(struct gsi_channel *channel)
ret = gsi_channel_command(channel, GSI_CH_RESET);
- /* Channel state will normally have been updated */
+ /* If successful the channel state will have changed */
state = gsi_channel_state(channel);
- if (!ret && state != GSI_CHANNEL_STATE_ALLOCATED)
+ if (state != GSI_CHANNEL_STATE_ALLOCATED)
dev_err(dev, "channel %u bad state %u after reset\n",
gsi_channel_id(channel), state);
}
@@ -628,9 +628,10 @@ static void gsi_channel_de_alloc_command(struct gsi *gsi, u32 channel_id)
ret = gsi_channel_command(channel, GSI_CH_DE_ALLOC);
- /* Channel state will normally have been updated */
+ /* If successful the channel state will have changed */
state = gsi_channel_state(channel);
- if (!ret && state != GSI_CHANNEL_STATE_NOT_ALLOCATED)
+
+ if (state != GSI_CHANNEL_STATE_NOT_ALLOCATED)
dev_err(dev, "channel %u bad state %u after dealloc\n",
channel_id, state);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH net 3/3] net: ipa: use state to determine event ring command success
2020-12-22 18:00 [PATCH net 0/3] net: ipa: GSI interrupt handling fixes Alex Elder
2020-12-22 18:00 ` [PATCH net 1/3] net: ipa: clear pending interrupts before enabling Alex Elder
2020-12-22 18:00 ` [PATCH net 2/3] net: ipa: use state to determine channel command success Alex Elder
@ 2020-12-22 18:00 ` Alex Elder
2020-12-23 20:20 ` [PATCH net 0/3] net: ipa: GSI interrupt handling fixes patchwork-bot+netdevbpf
3 siblings, 0 replies; 6+ messages in thread
From: Alex Elder @ 2020-12-22 18:00 UTC (permalink / raw)
To: davem, kuba
Cc: evgreen, cpratapa, bjorn.andersson, subashab, netdev, linux-kernel
This patch implements the same basic fix for event rings as the
previous one does for channels.
The result of issuing an event ring control command should be that
the event ring changes state. If enabled, a completion interrupt
signals that the event ring state has changed. This interrupt is
enabled by gsi_evt_ring_command() and disabled again after the
command has completed (or we time out).
There is a window of time during which the command could complete
successfully without interrupting. This would cause the event ring
to transition to the desired new state.
So whether a event ring command ends via completion interrupt or
timeout, we can consider the command successful if the event ring
has entered the desired state (and a failure if it has not,
regardless of the cause).
Fixes: b4175f8731f78 ("net: ipa: only enable GSI event control IRQs when needed")
Signed-off-by: Alex Elder <elder@linaro.org>
---
drivers/net/ipa/gsi.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 4f0e791764237..579cc3e516775 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -384,13 +384,15 @@ static int gsi_evt_ring_alloc_command(struct gsi *gsi, u32 evt_ring_id)
}
ret = evt_ring_command(gsi, evt_ring_id, GSI_EVT_ALLOCATE);
- if (!ret && evt_ring->state != GSI_EVT_RING_STATE_ALLOCATED) {
- dev_err(gsi->dev, "event ring %u bad state %u after alloc\n",
- evt_ring_id, evt_ring->state);
- ret = -EIO;
- }
- return ret;
+ /* If successful the event ring state will have changed */
+ if (evt_ring->state == GSI_EVT_RING_STATE_ALLOCATED)
+ return 0;
+
+ dev_err(gsi->dev, "event ring %u bad state %u after alloc\n",
+ evt_ring_id, evt_ring->state);
+
+ return -EIO;
}
/* Reset a GSI event ring in ALLOCATED or ERROR state. */
@@ -408,9 +410,13 @@ static void gsi_evt_ring_reset_command(struct gsi *gsi, u32 evt_ring_id)
}
ret = evt_ring_command(gsi, evt_ring_id, GSI_EVT_RESET);
- if (!ret && evt_ring->state != GSI_EVT_RING_STATE_ALLOCATED)
- dev_err(gsi->dev, "event ring %u bad state %u after reset\n",
- evt_ring_id, evt_ring->state);
+
+ /* If successful the event ring state will have changed */
+ if (evt_ring->state == GSI_EVT_RING_STATE_ALLOCATED)
+ return;
+
+ dev_err(gsi->dev, "event ring %u bad state %u after reset\n",
+ evt_ring_id, evt_ring->state);
}
/* Issue a hardware de-allocation request for an allocated event ring */
@@ -426,9 +432,13 @@ static void gsi_evt_ring_de_alloc_command(struct gsi *gsi, u32 evt_ring_id)
}
ret = evt_ring_command(gsi, evt_ring_id, GSI_EVT_DE_ALLOC);
- if (!ret && evt_ring->state != GSI_EVT_RING_STATE_NOT_ALLOCATED)
- dev_err(gsi->dev, "event ring %u bad state %u after dealloc\n",
- evt_ring_id, evt_ring->state);
+
+ /* If successful the event ring state will have changed */
+ if (evt_ring->state == GSI_EVT_RING_STATE_NOT_ALLOCATED)
+ return;
+
+ dev_err(gsi->dev, "event ring %u bad state %u after dealloc\n",
+ evt_ring_id, evt_ring->state);
}
/* Fetch the current state of a channel from hardware */
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net 0/3] net: ipa: GSI interrupt handling fixes
2020-12-22 18:00 [PATCH net 0/3] net: ipa: GSI interrupt handling fixes Alex Elder
` (2 preceding siblings ...)
2020-12-22 18:00 ` [PATCH net 3/3] net: ipa: use state to determine event ring " Alex Elder
@ 2020-12-23 20:20 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2020-12-23 20:20 UTC (permalink / raw)
To: Alex Elder
Cc: davem, kuba, evgreen, cpratapa, bjorn.andersson, subashab,
netdev, linux-kernel
Hello:
This series was applied to netdev/net.git (refs/heads/master):
On Tue, 22 Dec 2020 12:00:09 -0600 you wrote:
> This series implements fixes for some issues related to handling
> interrupts when GSI channel and event ring commands complete.
>
> The first issue is that the completion condition for an event ring
> or channel command could occur while the associated interrupt is
> disabled. This would cause the interrupt to fire when it is
> subsequently enabled, even if the condition it signals had already
> been handled. The fix is to clear any pending interrupt conditions
> before re-enabling the interrupt.
>
> [...]
Here is the summary with links:
- [net,1/3] net: ipa: clear pending interrupts before enabling
https://git.kernel.org/netdev/net/c/94ad8f3ac6af
- [net,2/3] net: ipa: use state to determine channel command success
https://git.kernel.org/netdev/net/c/6ffddf3b3d18
- [net,3/3] net: ipa: use state to determine event ring command success
https://git.kernel.org/netdev/net/c/428b448ee764
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] 6+ messages in thread
* Re: [PATCH net 2/3] net: ipa: use state to determine channel command success
2020-12-22 18:00 ` [PATCH net 2/3] net: ipa: use state to determine channel command success Alex Elder
@ 2020-12-26 18:51 ` kernel test robot
0 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2020-12-26 18:51 UTC (permalink / raw)
To: Alex Elder, davem, kuba
Cc: kbuild-all, evgreen, cpratapa, bjorn.andersson, subashab, netdev,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4502 bytes --]
Hi Alex,
I love your patch! Perhaps something to improve:
[auto build test WARNING on net/master]
url: https://github.com/0day-ci/linux/commits/Alex-Elder/net-ipa-GSI-interrupt-handling-fixes/20201223-020409
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git 2575bc1aa9d52a62342b57a0b7d0a12146cf6aed
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/4073b68cee8a9d0dbb83080db22255fc9b9d7fd5
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Alex-Elder/net-ipa-GSI-interrupt-handling-fixes/20201223-020409
git checkout 4073b68cee8a9d0dbb83080db22255fc9b9d7fd5
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/net/ipa/gsi.c: In function 'gsi_channel_alloc_command':
>> drivers/net/ipa/gsi.c:496:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
496 | int ret;
| ^~~
drivers/net/ipa/gsi.c: In function 'gsi_channel_start_command':
drivers/net/ipa/gsi.c:524:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
524 | int ret;
| ^~~
drivers/net/ipa/gsi.c: In function 'gsi_channel_stop_command':
drivers/net/ipa/gsi.c:552:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
552 | int ret;
| ^~~
drivers/net/ipa/gsi.c: In function 'gsi_channel_reset_command':
drivers/net/ipa/gsi.c:591:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
591 | int ret;
| ^~~
drivers/net/ipa/gsi.c: In function 'gsi_channel_de_alloc_command':
drivers/net/ipa/gsi.c:620:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
620 | int ret;
| ^~~
vim +/ret +496 drivers/net/ipa/gsi.c
650d1603825d8ba Alex Elder 2020-03-05 489
650d1603825d8ba Alex Elder 2020-03-05 490 /* Allocate GSI channel in NOT_ALLOCATED state */
650d1603825d8ba Alex Elder 2020-03-05 491 static int gsi_channel_alloc_command(struct gsi *gsi, u32 channel_id)
650d1603825d8ba Alex Elder 2020-03-05 492 {
650d1603825d8ba Alex Elder 2020-03-05 493 struct gsi_channel *channel = &gsi->channel[channel_id];
a442b3c7554898f Alex Elder 2020-06-30 494 struct device *dev = gsi->dev;
a2003b303875b41 Alex Elder 2020-04-30 495 enum gsi_channel_state state;
650d1603825d8ba Alex Elder 2020-03-05 @496 int ret;
650d1603825d8ba Alex Elder 2020-03-05 497
650d1603825d8ba Alex Elder 2020-03-05 498 /* Get initial channel state */
a2003b303875b41 Alex Elder 2020-04-30 499 state = gsi_channel_state(channel);
a442b3c7554898f Alex Elder 2020-06-30 500 if (state != GSI_CHANNEL_STATE_NOT_ALLOCATED) {
f8d3bdd561a7c95 Alex Elder 2020-11-19 501 dev_err(dev, "channel %u bad state %u before alloc\n",
f8d3bdd561a7c95 Alex Elder 2020-11-19 502 channel_id, state);
650d1603825d8ba Alex Elder 2020-03-05 503 return -EINVAL;
a442b3c7554898f Alex Elder 2020-06-30 504 }
650d1603825d8ba Alex Elder 2020-03-05 505
650d1603825d8ba Alex Elder 2020-03-05 506 ret = gsi_channel_command(channel, GSI_CH_ALLOCATE);
a2003b303875b41 Alex Elder 2020-04-30 507
4073b68cee8a9d0 Alex Elder 2020-12-22 508 /* If successful the channel state will have changed */
a2003b303875b41 Alex Elder 2020-04-30 509 state = gsi_channel_state(channel);
4073b68cee8a9d0 Alex Elder 2020-12-22 510 if (state == GSI_CHANNEL_STATE_ALLOCATED)
4073b68cee8a9d0 Alex Elder 2020-12-22 511 return 0;
4073b68cee8a9d0 Alex Elder 2020-12-22 512
f8d3bdd561a7c95 Alex Elder 2020-11-19 513 dev_err(dev, "channel %u bad state %u after alloc\n",
f8d3bdd561a7c95 Alex Elder 2020-11-19 514 channel_id, state);
650d1603825d8ba Alex Elder 2020-03-05 515
4073b68cee8a9d0 Alex Elder 2020-12-22 516 return -EIO;
650d1603825d8ba Alex Elder 2020-03-05 517 }
650d1603825d8ba Alex Elder 2020-03-05 518
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 76283 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-12-26 18:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-22 18:00 [PATCH net 0/3] net: ipa: GSI interrupt handling fixes Alex Elder
2020-12-22 18:00 ` [PATCH net 1/3] net: ipa: clear pending interrupts before enabling Alex Elder
2020-12-22 18:00 ` [PATCH net 2/3] net: ipa: use state to determine channel command success Alex Elder
2020-12-26 18:51 ` kernel test robot
2020-12-22 18:00 ` [PATCH net 3/3] net: ipa: use state to determine event ring " Alex Elder
2020-12-23 20:20 ` [PATCH net 0/3] net: ipa: GSI interrupt handling fixes patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).