* [PATCH net 0/2] net: ipa: fix a suspend hang @ 2021-01-07 21:43 Alex Elder 2021-01-07 21:43 ` [PATCH net 1/2] net: ipa: introduce atomic channel STOPPING flag Alex Elder 2021-01-07 21:43 ` [PATCH net 2/2] net: ipa: re-enable NAPI before enabling interrupt Alex Elder 0 siblings, 2 replies; 5+ messages in thread From: Alex Elder @ 2021-01-07 21:43 UTC (permalink / raw) To: davem, kuba Cc: evgreen, bjorn.andersson, cpratapa, subashab, netdev, linux-kernel The IPA driver's PM suspend callback stops all channels, and its resume callback restarts them again. Part of stopping a channel is disabling NAPI and disabling its I/O completion interrupt. When stopping a channel, the IPA driver currently disables NAPI before disabling the interrupt. It also re-enables interrupts before re-enabling NAPI. The interrupt handler can therefore be called while NAPI is disabled. If the interrupt signaling a transfer completion occurs while NAPI is disabled, NAPI polling will not be scheduled to process that completion. That processing will be delayed, occuring only when a subsequent interrupt schedules NAPI polling when NAPI is enabled again. The second patch in this series reorders the NAPI and interrupt control calls. The completion interrupt is disabled before NAPI when stopping a channel, and re-enabled after NAPI when starting. This way polling to handle the completion of a transfer can begin immediately when handling its interrupt. And if a completion occurs while the interrupt is disabled, the handler will trigger polling when interrupts are enabled again. The first patch adds a flag that prevents the poll function from re-enabling the interrupt when stopping. Without this fix in place we would occasionally see a hang while stopping channels during suspend. -Alex Alex Elder (2): net: ipa: introduce atomic channel STOPPING flag net: ipa: re-enable NAPI before enabling interrupt drivers/net/ipa/gsi.c | 15 ++++++++++++--- drivers/net/ipa/gsi.h | 6 ++++++ 2 files changed, 18 insertions(+), 3 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net 1/2] net: ipa: introduce atomic channel STOPPING flag 2021-01-07 21:43 [PATCH net 0/2] net: ipa: fix a suspend hang Alex Elder @ 2021-01-07 21:43 ` Alex Elder 2021-01-07 21:43 ` [PATCH net 2/2] net: ipa: re-enable NAPI before enabling interrupt Alex Elder 1 sibling, 0 replies; 5+ messages in thread From: Alex Elder @ 2021-01-07 21:43 UTC (permalink / raw) To: davem, kuba Cc: evgreen, bjorn.andersson, cpratapa, subashab, netdev, linux-kernel Introduce a new atomic flag bit to communicate that a channel is stopping. At the end of the NAPI poll loop, we normally re-enable the IEOB interrupt, but now we won't do that if the channel is being stopped. This is required for the next patch. Fixes: 650d1603825d8 ("soc: qcom: ipa: the generic software interface") Signed-off-by: Alex Elder <elder@linaro.org> --- drivers/net/ipa/gsi.c | 11 ++++++++++- drivers/net/ipa/gsi.h | 6 ++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c index 14d9a791924bf..7e7629902911e 100644 --- a/drivers/net/ipa/gsi.c +++ b/drivers/net/ipa/gsi.c @@ -739,6 +739,10 @@ static void gsi_channel_freeze(struct gsi_channel *channel) { gsi_channel_trans_quiesce(channel); + /* Don't let the NAPI poll loop re-enable interrupts when done */ + set_bit(GSI_CHANNEL_FLAG_STOPPING, channel->flags); + smp_mb__after_atomic(); /* Ensure gsi_channel_poll() sees new value */ + napi_disable(&channel->napi); gsi_irq_ieob_disable(channel->gsi, channel->evt_ring_id); @@ -749,6 +753,10 @@ static void gsi_channel_thaw(struct gsi_channel *channel) { gsi_irq_ieob_enable(channel->gsi, channel->evt_ring_id); + /* Allow the NAPI poll loop to re-enable interrupts again */ + clear_bit(GSI_CHANNEL_FLAG_STOPPING, channel->flags); + smp_mb__after_atomic(); /* Ensure gsi_channel_poll() sees new value */ + napi_enable(&channel->napi); } @@ -1536,7 +1544,8 @@ static int gsi_channel_poll(struct napi_struct *napi, int budget) if (count < budget) { napi_complete(&channel->napi); - gsi_irq_ieob_enable(channel->gsi, channel->evt_ring_id); + if (!test_bit(GSI_CHANNEL_FLAG_STOPPING, channel->flags)) + gsi_irq_ieob_enable(channel->gsi, channel->evt_ring_id); } return count; diff --git a/drivers/net/ipa/gsi.h b/drivers/net/ipa/gsi.h index 96c9aed397aad..8f0ae97c80c6e 100644 --- a/drivers/net/ipa/gsi.h +++ b/drivers/net/ipa/gsi.h @@ -104,9 +104,15 @@ enum gsi_channel_state { GSI_CHANNEL_STATE_ERROR = 0xf, }; +enum gsi_channel_flag { + GSI_CHANNEL_FLAG_STOPPING, + GSI_CHANNEL_FLAG_COUNT, /* Last; not a flag */ +}; + /* We only care about channels between IPA and AP */ struct gsi_channel { struct gsi *gsi; + DECLARE_BITMAP(flags, GSI_CHANNEL_FLAG_COUNT); bool toward_ipa; bool command; /* AP command TX channel or not */ -- 2.20.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH net 2/2] net: ipa: re-enable NAPI before enabling interrupt 2021-01-07 21:43 [PATCH net 0/2] net: ipa: fix a suspend hang Alex Elder 2021-01-07 21:43 ` [PATCH net 1/2] net: ipa: introduce atomic channel STOPPING flag Alex Elder @ 2021-01-07 21:43 ` Alex Elder 2021-01-08 2:38 ` Jakub Kicinski 1 sibling, 1 reply; 5+ messages in thread From: Alex Elder @ 2021-01-07 21:43 UTC (permalink / raw) To: davem, kuba Cc: evgreen, bjorn.andersson, cpratapa, subashab, netdev, linux-kernel When we stop or suspend a channel, we first "freeze" it. The last part of that involves disabling NAPI, and disabling the IEOB interrupt that schedules NAPI when it occurs. On resume, a "thaw" does the inverse of these activities, in reverse order. Currently these are ordered such that NAPI is disabled before interrupts on suspend, and NAPI is re-enabled after interrupts on resume. An interrupt occurring while NAPI is disabled will request a NAPI schedule, but polling is deferred until after NAPI is enabled again. When NAPI is re-enabled, polling is allowed again, but enabling NAPI does not schedule a poll (i.e., it won't trigger polling to handle a schedule request that occurred while disabled). Polling won't commence until the next napi_schedule() request occurs. Instead, disable completion interrupts *before* disabling NAPI when stopping a channel, and re-enable interrupts *after* re-enabling NAPI. That way NAPI is always enabled when an interrupt occurs, and polling to handle the interrupt can commence immediately. The channel STOPPING flag ensures the polling function won't re-enable the completion interrupt while we are stopping. Fixes: 650d1603825d8 ("soc: qcom: ipa: the generic software interface") Signed-off-by: Alex Elder <elder@linaro.org> --- drivers/net/ipa/gsi.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c index 7e7629902911e..9bde6d02b1cd6 100644 --- a/drivers/net/ipa/gsi.c +++ b/drivers/net/ipa/gsi.c @@ -743,21 +743,21 @@ static void gsi_channel_freeze(struct gsi_channel *channel) set_bit(GSI_CHANNEL_FLAG_STOPPING, channel->flags); smp_mb__after_atomic(); /* Ensure gsi_channel_poll() sees new value */ - napi_disable(&channel->napi); - gsi_irq_ieob_disable(channel->gsi, channel->evt_ring_id); + + napi_disable(&channel->napi); } /* Allow transactions to be used on the channel again. */ static void gsi_channel_thaw(struct gsi_channel *channel) { - gsi_irq_ieob_enable(channel->gsi, channel->evt_ring_id); - /* Allow the NAPI poll loop to re-enable interrupts again */ clear_bit(GSI_CHANNEL_FLAG_STOPPING, channel->flags); smp_mb__after_atomic(); /* Ensure gsi_channel_poll() sees new value */ napi_enable(&channel->napi); + + gsi_irq_ieob_enable(channel->gsi, channel->evt_ring_id); } /* Program a channel for use */ -- 2.20.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net 2/2] net: ipa: re-enable NAPI before enabling interrupt 2021-01-07 21:43 ` [PATCH net 2/2] net: ipa: re-enable NAPI before enabling interrupt Alex Elder @ 2021-01-08 2:38 ` Jakub Kicinski 2021-01-08 20:16 ` Alex Elder 0 siblings, 1 reply; 5+ messages in thread From: Jakub Kicinski @ 2021-01-08 2:38 UTC (permalink / raw) To: Alex Elder Cc: davem, evgreen, bjorn.andersson, cpratapa, subashab, netdev, linux-kernel On Thu, 7 Jan 2021 15:43:25 -0600 Alex Elder wrote: > @@ -743,21 +743,21 @@ static void gsi_channel_freeze(struct gsi_channel *channel) > set_bit(GSI_CHANNEL_FLAG_STOPPING, channel->flags); > smp_mb__after_atomic(); /* Ensure gsi_channel_poll() sees new value */ > > - napi_disable(&channel->napi); > - > gsi_irq_ieob_disable(channel->gsi, channel->evt_ring_id); > + > + napi_disable(&channel->napi); > } So patch 1 is entirely for the purpose of keeping the code symmetric here? I can't think of other reason why masking this IRQ couldn't be left after NAPI is disabled, and that should work as you expect. > /* Allow transactions to be used on the channel again. */ > static void gsi_channel_thaw(struct gsi_channel *channel) > { > - gsi_irq_ieob_enable(channel->gsi, channel->evt_ring_id); > - > /* Allow the NAPI poll loop to re-enable interrupts again */ > clear_bit(GSI_CHANNEL_FLAG_STOPPING, channel->flags); > smp_mb__after_atomic(); /* Ensure gsi_channel_poll() sees new value */ > > napi_enable(&channel->napi); > + > + gsi_irq_ieob_enable(channel->gsi, channel->evt_ring_id); > } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net 2/2] net: ipa: re-enable NAPI before enabling interrupt 2021-01-08 2:38 ` Jakub Kicinski @ 2021-01-08 20:16 ` Alex Elder 0 siblings, 0 replies; 5+ messages in thread From: Alex Elder @ 2021-01-08 20:16 UTC (permalink / raw) To: Jakub Kicinski Cc: davem, evgreen, bjorn.andersson, cpratapa, subashab, netdev, linux-kernel On 1/7/21 8:38 PM, Jakub Kicinski wrote: > On Thu, 7 Jan 2021 15:43:25 -0600 Alex Elder wrote: >> @@ -743,21 +743,21 @@ static void gsi_channel_freeze(struct gsi_channel *channel) >> set_bit(GSI_CHANNEL_FLAG_STOPPING, channel->flags); >> smp_mb__after_atomic(); /* Ensure gsi_channel_poll() sees new value */ >> >> - napi_disable(&channel->napi); >> - >> gsi_irq_ieob_disable(channel->gsi, channel->evt_ring_id); >> + >> + napi_disable(&channel->napi); >> } > > So patch 1 is entirely for the purpose of keeping the code symmetric > here? I can't think of other reason why masking this IRQ couldn't be > left after NAPI is disabled, and that should work as you expect. No, that is not the purpose of the first patch. But regardless, I'm really glad you pushed back on this because it made me step back and re-evaluate in a different way what was happening during suspend. Your earlier response (about what happens during napi_disable()) also helped me to see there's probably something *else* wrong with how the driver is stopping channels. I was going to go into more detail here but for now let me just rescind this series. I will be reworking the channel stop/suspend logic and will send that work out when it's tested and ready. Thanks. -Alex >> /* Allow transactions to be used on the channel again. */ >> static void gsi_channel_thaw(struct gsi_channel *channel) >> { >> - gsi_irq_ieob_enable(channel->gsi, channel->evt_ring_id); >> - >> /* Allow the NAPI poll loop to re-enable interrupts again */ >> clear_bit(GSI_CHANNEL_FLAG_STOPPING, channel->flags); >> smp_mb__after_atomic(); /* Ensure gsi_channel_poll() sees new value */ >> >> napi_enable(&channel->napi); >> + >> + gsi_irq_ieob_enable(channel->gsi, channel->evt_ring_id); >> } ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-08 20:17 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-07 21:43 [PATCH net 0/2] net: ipa: fix a suspend hang Alex Elder 2021-01-07 21:43 ` [PATCH net 1/2] net: ipa: introduce atomic channel STOPPING flag Alex Elder 2021-01-07 21:43 ` [PATCH net 2/2] net: ipa: re-enable NAPI before enabling interrupt Alex Elder 2021-01-08 2:38 ` Jakub Kicinski 2021-01-08 20:16 ` Alex Elder
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).