Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Thu, 22 Feb 2018 18:32:28 +0100 | Subject | Re: [PATCH] PCMCIA / PM: Avoid noirq suspend aborts during suspend-to-idle |
| |
On Thu, Feb 22, 2018 at 5:45 PM, Dominik Brodowski <linux@dominikbrodowski.net> 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@intel.com> > 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@intel.com> > [linux@dominikbrodowski.net: follow same codepaths for both suspend variants; call ->suspend()] > Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net> > > 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
| |