Re: [PATCH] PCMCIA / PM: Avoid noirq suspend aborts during suspend-to-idle

[Rafael J. Wysocki]

On Thu, Feb 22, 2018 at 5:45 PM, Dominik Brodowski
<linux@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Feb 21, 2018 at 01:24:16PM +0100, Rafael J. Wysocki wrote:
>> Avoid that by using a new socket state flag, SOCKET_IN_RESUME,
>> to indicate that socket_early_resume() has already run for the
>> socket in which case socket_suspend() will do minimum handling
>> and return 0.
>
> That looks to be *too* minimal: For example, yenta_socket does some
> socket set-up in ->init(), which is called by socket_early_resume().
> That needs to be undone by ->suspend().

I see.

> What about this variant (untested, not even compile-tested)?

It looks fine to me, I'll test it later today.

> Oh, and
>         a) does this need -stable tags, and

Not right away I guess, but I will ask the -stable maintainers to pick
it up at one point if there are no problems reported with it.

>         b) do you want to push it yourself, or should it go through the
>            PCMCIA tree?

I'll take care of it.

Thanks,
Rafael


> ---
> From: "Rafael J. Wysocki" <rafael.j.wysocki@xxxxxxxxx>
> Date: Wed, 21 Feb 2018 13:24:16 +0100
> Subject: [PATCH] PCMCIA / PM: Avoid noirq suspend aborts during
>  suspend-to-idle
>
> There is a problem with PCMCIA system resume callbacks with respect
> to suspend-to-idle in which the ->suspend_noirq() callback may be
> invoked after the ->resume_noirq() one without resuming the system
> entirely in some cases.  This doesn't work for PCMCIA because of
> the lack of symmetry between its system suspend and system resume
> "noirq" callbacks.
>
> The system resume handling in PCMCIA is split between
> socket_early_resume() and socket_late_resume() which are called in
> different phases of system resume and both need to run for
> socket_suspend() (invoked by the system suspend "noirq" callback)
> to work.  Specifically, socket_suspend() returns an error when
> called after socket_early_resume() without socket_late_resume(),
> so if the suspend-to-idle core detects a spurious wakeup event and
> attempts to put the system back to sleep, that is aborted by the
> error coming from socket_suspend().
>
> Avoid that by using a new socket state flag, SOCKET_IN_RESUME,
> to indicate that socket_early_resume() has already run for the
> socket in which case socket_suspend() will do minimum handling
> and return 0.
>
> This change has been tested on my venerable Toshiba Portege R500
> (which is where the problem has been discovered in the first place),
> but admittedly I have no PCMCIA cards to test along with the socket
> itself.
>
> Fixes: 33e4f80ee69b (ACPI / PM: Ignore spurious SCI wakeups from suspend-to-idle)
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> [linux@xxxxxxxxxxxxxxxxxxxx: follow same codepaths for both suspend variants; call ->suspend()]
> Signed-off-by: Dominik Brodowski <linux@xxxxxxxxxxxxxxxxxxxx>
>
> diff --git a/drivers/pcmcia/cs.c b/drivers/pcmcia/cs.c
> index c3b615c94b4b..513b2d4d5b20 100644
> --- a/drivers/pcmcia/cs.c
> +++ b/drivers/pcmcia/cs.c
> @@ -452,17 +452,20 @@ static int socket_insert(struct pcmcia_socket *skt)
>
>  static int socket_suspend(struct pcmcia_socket *skt)
>  {
> -       if (skt->state & SOCKET_SUSPEND)
> +       if ((skt->state & SOCKET_SUSPEND) && !(skt->state & SOCKET_IN_RESUME))
>                 return -EBUSY;
>
>         mutex_lock(&skt->ops_mutex);
> -       skt->suspended_state = skt->state;
> +       /* store state on first suspend, but not after spurious wakeups */
> +       if !(skt->state & SOCKET_IN_RESUME)
> +               skt->suspended_state = skt->state;
>
>         skt->socket = dead_socket;
>         skt->ops->set_socket(skt, &skt->socket);
>         if (skt->ops->suspend)
>                 skt->ops->suspend(skt);
>         skt->state |= SOCKET_SUSPEND;
> +       skt->state &= ~(SOCKET_IN_RESUME);
>         mutex_unlock(&skt->ops_mutex);
>         return 0;
>  }
> @@ -475,6 +478,7 @@ static int socket_early_resume(struct pcmcia_socket *skt)
>         skt->ops->set_socket(skt, &skt->socket);
>         if (skt->state & SOCKET_PRESENT)
>                 skt->resume_status = socket_setup(skt, resume_delay);
> +       skt->state |= SOCKET_IN_RESUME;
>         mutex_unlock(&skt->ops_mutex);
>         return 0;
>  }
> @@ -484,7 +488,7 @@ static int socket_late_resume(struct pcmcia_socket *skt)
>         int ret = 0;
>
>         mutex_lock(&skt->ops_mutex);
> -       skt->state &= ~SOCKET_SUSPEND;
> +       skt->state &= ~(SOCKET_SUSPEND | SOCKET_IN_RESUME);
>         mutex_unlock(&skt->ops_mutex);
>
>         if (!(skt->state & SOCKET_PRESENT)) {
> diff --git a/drivers/pcmcia/cs_internal.h b/drivers/pcmcia/cs_internal.h
> index 6765beadea95..03ec43802909 100644
> --- a/drivers/pcmcia/cs_internal.h
> +++ b/drivers/pcmcia/cs_internal.h
> @@ -70,6 +70,7 @@ struct pccard_resource_ops {
>  /* Flags in socket state */
>  #define SOCKET_PRESENT         0x0008
>  #define SOCKET_INUSE           0x0010
> +#define SOCKET_IN_RESUME       0x0040
>  #define SOCKET_SUSPEND         0x0080
>  #define SOCKET_WIN_REQ(i)      (0x0100<<(i))
>  #define SOCKET_CARDBUS         0x8000