mbox series

[net-next,v2,0/7] net: ipa: don't disable NAPI in suspend

Message ID 20210201172850.2221624-1-elder@linaro.org (mailing list archive)
Headers show
Series net: ipa: don't disable NAPI in suspend | expand

Message

Alex Elder Feb. 1, 2021, 5:28 p.m. UTC
This is version 2 of a series that reworks the order in which things
happen during channel stop and suspend (and start and resume), in
order to address a hang that has been observed during suspend.
The introductory message on the first version of the series gave
some history which is omitted here.

The end result of this series is that we only enable NAPI and the
I/O completion interrupt on a channel when we start the channel for
the first time.  And we only disable them when stopping the channel
"for good."  In other words, NAPI and the completion interrupt
remain enabled while a channel is stopped for suspend.

One comment on version 1 of the series suggested *not* returning
early on success in a function, instead having both success and
error paths return from the same point at the end of the function
block.  This has been addressed in this version.

In addition, this version consolidates things a little bit, but the
net result of the series is exactly the same as version 1 (with the
exception of the return fix mentioned above).

First, patch 6 in the first version was a small step to make patch 7
easier to understand.  The two have been combined now.

Second, previous version moved (and for suspend/resume, eliminated)
I/O completion interrupt and NAPI disable/enable control in separate
steps (patches).  Now both are moved around together in patch 5 and
6, which eliminates the need for the final (NAPI-only) patch.

I won't repeat the patch summaries provided in v1:
  https://lore.kernel.org/netdev/20210129202019.2099259-1-elder@linaro.org/

Many thanks to Willem de Bruijn for his thoughtful input.

					-Alex

Alex Elder (7):
  net: ipa: don't thaw channel if error starting
  net: ipa: introduce gsi_channel_stop_retry()
  net: ipa: introduce __gsi_channel_start()
  net: ipa: kill gsi_channel_freeze() and gsi_channel_thaw()
  net: ipa: disable interrupt and NAPI after channel stop
  net: ipa: don't disable interrupt on suspend
  net: ipa: expand last transaction check

 drivers/net/ipa/gsi.c | 138 ++++++++++++++++++++++++++----------------
 1 file changed, 85 insertions(+), 53 deletions(-)

Comments

Willem de Bruijn Feb. 1, 2021, 6:44 p.m. UTC | #1
On Mon, Feb 1, 2021 at 12:28 PM Alex Elder <elder@linaro.org> wrote:
>
> This is version 2 of a series that reworks the order in which things
> happen during channel stop and suspend (and start and resume), in
> order to address a hang that has been observed during suspend.
> The introductory message on the first version of the series gave
> some history which is omitted here.
>
> The end result of this series is that we only enable NAPI and the
> I/O completion interrupt on a channel when we start the channel for
> the first time.  And we only disable them when stopping the channel
> "for good."  In other words, NAPI and the completion interrupt
> remain enabled while a channel is stopped for suspend.
>
> One comment on version 1 of the series suggested *not* returning
> early on success in a function, instead having both success and
> error paths return from the same point at the end of the function
> block.  This has been addressed in this version.
>
> In addition, this version consolidates things a little bit, but the
> net result of the series is exactly the same as version 1 (with the
> exception of the return fix mentioned above).
>
> First, patch 6 in the first version was a small step to make patch 7
> easier to understand.  The two have been combined now.
>
> Second, previous version moved (and for suspend/resume, eliminated)
> I/O completion interrupt and NAPI disable/enable control in separate
> steps (patches).  Now both are moved around together in patch 5 and
> 6, which eliminates the need for the final (NAPI-only) patch.
>
> I won't repeat the patch summaries provided in v1:
>   https://lore.kernel.org/netdev/20210129202019.2099259-1-elder@linaro.org/
>
> Many thanks to Willem de Bruijn for his thoughtful input.
>
>                                         -Alex
>
> Alex Elder (7):
>   net: ipa: don't thaw channel if error starting
>   net: ipa: introduce gsi_channel_stop_retry()
>   net: ipa: introduce __gsi_channel_start()
>   net: ipa: kill gsi_channel_freeze() and gsi_channel_thaw()
>   net: ipa: disable interrupt and NAPI after channel stop
>   net: ipa: don't disable interrupt on suspend
>   net: ipa: expand last transaction check
>
>  drivers/net/ipa/gsi.c | 138 ++++++++++++++++++++++++++----------------
>  1 file changed, 85 insertions(+), 53 deletions(-)

Acked-by: Willem de Bruijn <willemb@google.com>
Jakub Kicinski Feb. 3, 2021, 4:08 a.m. UTC | #2
On Mon, 1 Feb 2021 13:44:20 -0500 Willem de Bruijn wrote:
> On Mon, Feb 1, 2021 at 12:28 PM Alex Elder <elder@linaro.org> wrote:
> >
> > This is version 2 of a series that reworks the order in which things
> > happen during channel stop and suspend (and start and resume), in
> > order to address a hang that has been observed during suspend.
> > The introductory message on the first version of the series gave
> > some history which is omitted here.
> >
> > The end result of this series is that we only enable NAPI and the
> > I/O completion interrupt on a channel when we start the channel for
> > the first time.  And we only disable them when stopping the channel
> > "for good."  In other words, NAPI and the completion interrupt
> > remain enabled while a channel is stopped for suspend.
> >
> > One comment on version 1 of the series suggested *not* returning
> > early on success in a function, instead having both success and
> > error paths return from the same point at the end of the function
> > block.  This has been addressed in this version.
> >
> > In addition, this version consolidates things a little bit, but the
> > net result of the series is exactly the same as version 1 (with the
> > exception of the return fix mentioned above).
> >
> > First, patch 6 in the first version was a small step to make patch 7
> > easier to understand.  The two have been combined now.
> >
> > Second, previous version moved (and for suspend/resume, eliminated)
> > I/O completion interrupt and NAPI disable/enable control in separate
> > steps (patches).  Now both are moved around together in patch 5 and
> > 6, which eliminates the need for the final (NAPI-only) patch.
>
> Acked-by: Willem de Bruijn <willemb@google.com>

Applied, thanks!