| Message ID | 20210107214325.7077-3-elder@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ipa: fix a suspend hang | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: elder@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 25 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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); > }
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); >> }
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 */
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(-)