Patchworkβ Help needed, Re: [Bug #14334] pcmcia suspend regression from 2.6.31.1 to 2.6.31.2 - Dell Inspiron 600m

login
register
about
Submitter Rafael Wysocki
Date 2009-11-02 13:39:28
Message ID <200911021439.28266.rjw@sisk.pl>
Download mbox | patch
Permalink /patch/57017/
State Not Applicable
Headers show

Comments

Rafael Wysocki - 2009-11-02 13:39:28
On Sunday 01 November 2009, Linus Torvalds wrote:
> 
> On Sun, 1 Nov 2009, Rafael J. Wysocki wrote:
> > 
> > If people don't object, I'll push it through the suspend-2.6 tree along 
> > with a few other bug fixes.
> 
> No objections, but a cleanup request:
> 
> > +static int socket_early_resume(struct pcmcia_socket *skt)
> > +{
> > +	if (skt->state & SOCKET_SUSPEND)
> > +		socket_start_resume(skt);
> > +
> > +	return 0;
> > +}
> > +
> > +static int socket_late_resume(struct pcmcia_socket *skt)
> > +{
> > +	if (!(skt->state & SOCKET_SUSPEND))
> > +		return 0;
> 
> As far as I can tell, that "SOCKET_SUSPEND" test is totally pointless. 

Right.

> That socket _is_ going to be suspended, and testing for it here just seems 
> to confuse things. 
> 
> So I'd remove it from both early_resume and late_resume, and only keep it 
> in the case of the legacy user-requested suspend/resume (do we even do 
> that any more?).

OK, updated patch is appended.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM / yenta: Split resume into early and late parts (rev. 3)

Commit 0c570cdeb8fdfcb354a3e9cd81bfc6a09c19de0c
(PM / yenta: Fix cardbus suspend/resume regression) caused resume to
fail on systems with two CardBus bridges.  While the exact nature
of the failure is not known at the moment, it can be worked around by
splitting the yenta resume into an early part, executed during the
early phase of resume, that will only resume the socket and power it
up if there was a card in it during suspend, and a late part,
executed during "regular" resume, that will carry out all of the
remaining yenta resume operations.

Fixes http://bugzilla.kernel.org/show_bug.cgi?id=14334, which is a
listed regression from 2.6.31.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pcmcia/cs.c           |   74 +++++++++++++++++++++++++++---------------
 drivers/pcmcia/yenta_socket.c |   12 ++++++
 include/pcmcia/ss.h           |    4 ++
 3 files changed, 63 insertions(+), 27 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dominik Brodowski - 2009-11-02 17:38:43
Hey,

just two minor nit-pick which we could handle post-2.6.32:

> +++ linux-2.6/drivers/pcmcia/cs.c
> @@ -98,10 +98,13 @@ EXPORT_SYMBOL(pcmcia_socket_list_rwsem);
>   * These functions check for the appropriate struct pcmcia_soket arrays,
>   * and pass them to the low-level functions pcmcia_{suspend,resume}_socket

... some documentation of the new functions, especially whether other socket
drivers should be updated?

> -static int socket_resume(struct pcmcia_socket *skt)
> +static void socket_start_resume(struct pcmcia_socket *skt)
>  {
> -	int ret;
> -
> -	if (!(skt->state & SOCKET_SUSPEND))
> -		return -EBUSY;
> -
>  	skt->socket = dead_socket;
>  	skt->ops->init(skt);
>  	skt->ops->set_socket(skt, &skt->socket);
> +	if (skt->state & SOCKET_PRESENT)
> +		skt->resume_status = socket_setup(skt, resume_delay);
> +}
>  
> +static int socket_early_resume(struct pcmcia_socket *skt)
> +{
> +	socket_start_resume(skt);
> +	return 0;
> +}

Why do we need to have two functions doing the same? Wouldn't

static int socket_early_resume(...)

suffice, with the only call to socket_start_resume() being replaced with
socket_early_resume()?

Best,
	Dominik
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Torvalds - 2009-11-02 17:50:07
On Mon, 2 Nov 2009, Rafael J. Wysocki wrote:
> 
> OK, updated patch is appended.

Looks good to me, feel free to push any time (assuming you've gotten 
testing confirmation from the people who reported it). Thanks,

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki - 2009-11-02 18:40:25
On Monday 02 November 2009, Dominik Brodowski wrote:
> Hey,
> 
> just two minor nit-pick which we could handle post-2.6.32:
> 
> > +++ linux-2.6/drivers/pcmcia/cs.c
> > @@ -98,10 +98,13 @@ EXPORT_SYMBOL(pcmcia_socket_list_rwsem);
> >   * These functions check for the appropriate struct pcmcia_soket arrays,
> >   * and pass them to the low-level functions pcmcia_{suspend,resume}_socket
> 
> ... some documentation of the new functions, especially whether other socket
> drivers should be updated?

OK, I'll post a separate patch for that for .33.

> > -static int socket_resume(struct pcmcia_socket *skt)
> > +static void socket_start_resume(struct pcmcia_socket *skt)
> >  {
> > -	int ret;
> > -
> > -	if (!(skt->state & SOCKET_SUSPEND))
> > -		return -EBUSY;
> > -
> >  	skt->socket = dead_socket;
> >  	skt->ops->init(skt);
> >  	skt->ops->set_socket(skt, &skt->socket);
> > +	if (skt->state & SOCKET_PRESENT)
> > +		skt->resume_status = socket_setup(skt, resume_delay);
> > +}
> >  
> > +static int socket_early_resume(struct pcmcia_socket *skt)
> > +{
> > +	socket_start_resume(skt);
> > +	return 0;
> > +}
> 
> Why do we need to have two functions doing the same? Wouldn't
> 
> static int socket_early_resume(...)
> 
> suffice, with the only call to socket_start_resume() being replaced with
> socket_early_resume()?

Yes, it would.  I'll do that in the final version of the patch.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Herrenschmidt - 2009-11-02 22:22:03
On Mon, 2009-11-02 at 14:39 +0100, Rafael J. Wysocki wrote:
> 
> > That socket _is_ going to be suspended, and testing for it here just seems 
> > to confuse things. 
> > 
> > So I'd remove it from both early_resume and late_resume, and only keep it 
> > in the case of the legacy user-requested suspend/resume (do we even do 
> > that any more?).
> 
> OK, updated patch is appended.

Test by me delayed to tomorrow ... hit another suspend/resume bug on
that laptop which took away the time I had to do that test yesterday and
today I'm off :-)

(Bug was simple but took a while to track down: machine was left in
storage for a while, battery ran out, RTC went back to Jan 1, 1904,
which means a negative xtime, and the new timekeeping code will do
horrible things including hanging at resume when that happens. Fix is to
make powerpc read_persistent_clock() to ignore the RTC when it contains
a date older than epoch).

Cheers,
Ben.


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pavel Machek - 2009-11-12 12:14:55
Hi!

> (Bug was simple but took a while to track down: machine was left in
> storage for a while, battery ran out, RTC went back to Jan 1, 1904,
> which means a negative xtime, and the new timekeeping code will do
> horrible things including hanging at resume when that happens. Fix is to
> make powerpc read_persistent_clock() to ignore the RTC when it contains
> a date older than epoch).

Additionaly, it would be nice if the machine would not hang, no matter
what RTC says...
									Pavel

Patch

Index: linux-2.6/drivers/pcmcia/yenta_socket.c
===================================================================
--- linux-2.6.orig/drivers/pcmcia/yenta_socket.c
+++ linux-2.6/drivers/pcmcia/yenta_socket.c
@@ -1275,16 +1275,26 @@  static int yenta_dev_resume_noirq(struct
 	if (socket->type && socket->type->restore_state)
 		socket->type->restore_state(socket);
 
-	return pcmcia_socket_dev_resume(dev);
+	pcmcia_socket_dev_early_resume(dev);
+	return 0;
+}
+
+static int yenta_dev_resume(struct device *dev)
+{
+	pcmcia_socket_dev_late_resume(dev);
+	return 0;
 }
 
 static struct dev_pm_ops yenta_pm_ops = {
 	.suspend_noirq = yenta_dev_suspend_noirq,
 	.resume_noirq = yenta_dev_resume_noirq,
+	.resume = yenta_dev_resume,
 	.freeze_noirq = yenta_dev_suspend_noirq,
 	.thaw_noirq = yenta_dev_resume_noirq,
+	.thaw = yenta_dev_resume,
 	.poweroff_noirq = yenta_dev_suspend_noirq,
 	.restore_noirq = yenta_dev_resume_noirq,
+	.restore = yenta_dev_resume,
 };
 
 #define YENTA_PM_OPS	(&yenta_pm_ops)
Index: linux-2.6/drivers/pcmcia/cs.c
===================================================================
--- linux-2.6.orig/drivers/pcmcia/cs.c
+++ linux-2.6/drivers/pcmcia/cs.c
@@ -98,10 +98,13 @@  EXPORT_SYMBOL(pcmcia_socket_list_rwsem);
  * These functions check for the appropriate struct pcmcia_soket arrays,
  * and pass them to the low-level functions pcmcia_{suspend,resume}_socket
  */
+static int socket_early_resume(struct pcmcia_socket *skt);
+static int socket_late_resume(struct pcmcia_socket *skt);
 static int socket_resume(struct pcmcia_socket *skt);
 static int socket_suspend(struct pcmcia_socket *skt);
 
-int pcmcia_socket_dev_suspend(struct device *dev)
+static void pcmcia_socket_dev_run(struct device *dev,
+				  int (*cb)(struct pcmcia_socket *))
 {
 	struct pcmcia_socket *socket;
 
@@ -110,29 +113,34 @@  int pcmcia_socket_dev_suspend(struct dev
 		if (socket->dev.parent != dev)
 			continue;
 		mutex_lock(&socket->skt_mutex);
-		socket_suspend(socket);
+		cb(socket);
 		mutex_unlock(&socket->skt_mutex);
 	}
 	up_read(&pcmcia_socket_list_rwsem);
+}
 
+int pcmcia_socket_dev_suspend(struct device *dev)
+{
+	pcmcia_socket_dev_run(dev, socket_suspend);
 	return 0;
 }
 EXPORT_SYMBOL(pcmcia_socket_dev_suspend);
 
-int pcmcia_socket_dev_resume(struct device *dev)
+void pcmcia_socket_dev_early_resume(struct device *dev)
 {
-	struct pcmcia_socket *socket;
+	pcmcia_socket_dev_run(dev, socket_early_resume);
+}
+EXPORT_SYMBOL(pcmcia_socket_dev_early_resume);
 
-	down_read(&pcmcia_socket_list_rwsem);
-	list_for_each_entry(socket, &pcmcia_socket_list, socket_list) {
-		if (socket->dev.parent != dev)
-			continue;
-		mutex_lock(&socket->skt_mutex);
-		socket_resume(socket);
-		mutex_unlock(&socket->skt_mutex);
-	}
-	up_read(&pcmcia_socket_list_rwsem);
+void pcmcia_socket_dev_late_resume(struct device *dev)
+{
+	pcmcia_socket_dev_run(dev, socket_late_resume);
+}
+EXPORT_SYMBOL(pcmcia_socket_dev_late_resume);
 
+int pcmcia_socket_dev_resume(struct device *dev)
+{
+	pcmcia_socket_dev_run(dev, socket_resume);
 	return 0;
 }
 EXPORT_SYMBOL(pcmcia_socket_dev_resume);
@@ -546,29 +554,29 @@  static int socket_suspend(struct pcmcia_
 	return 0;
 }
 
-/*
- * Resume a socket.  If a card is present, verify its CIS against
- * our cached copy.  If they are different, the card has been
- * replaced, and we need to tell the drivers.
- */
-static int socket_resume(struct pcmcia_socket *skt)
+static void socket_start_resume(struct pcmcia_socket *skt)
 {
-	int ret;
-
-	if (!(skt->state & SOCKET_SUSPEND))
-		return -EBUSY;
-
 	skt->socket = dead_socket;
 	skt->ops->init(skt);
 	skt->ops->set_socket(skt, &skt->socket);
+	if (skt->state & SOCKET_PRESENT)
+		skt->resume_status = socket_setup(skt, resume_delay);
+}
 
+static int socket_early_resume(struct pcmcia_socket *skt)
+{
+	socket_start_resume(skt);
+	return 0;
+}
+
+static int socket_late_resume(struct pcmcia_socket *skt)
+{
 	if (!(skt->state & SOCKET_PRESENT)) {
 		skt->state &= ~SOCKET_SUSPEND;
 		return socket_insert(skt);
 	}
 
-	ret = socket_setup(skt, resume_delay);
-	if (ret == 0) {
+	if (skt->resume_status == 0) {
 		/*
 		 * FIXME: need a better check here for cardbus cards.
 		 */
@@ -596,6 +604,20 @@  static int socket_resume(struct pcmcia_s
 	return 0;
 }
 
+/*
+ * Resume a socket.  If a card is present, verify its CIS against
+ * our cached copy.  If they are different, the card has been
+ * replaced, and we need to tell the drivers.
+ */
+static int socket_resume(struct pcmcia_socket *skt)
+{
+	if (!(skt->state & SOCKET_SUSPEND))
+		return -EBUSY;
+
+	socket_start_resume(skt);
+	return socket_late_resume(skt);
+}
+
 static void socket_remove(struct pcmcia_socket *skt)
 {
 	dev_printk(KERN_NOTICE, &skt->dev,
Index: linux-2.6/include/pcmcia/ss.h
===================================================================
--- linux-2.6.orig/include/pcmcia/ss.h
+++ linux-2.6/include/pcmcia/ss.h
@@ -262,6 +262,8 @@  struct pcmcia_socket {
 	struct device			dev;
 	/* data internal to the socket driver */
 	void				*driver_data;
+	/* status of the card during resume from a system sleep state */
+	int				resume_status;
 };
 
 
@@ -280,6 +282,8 @@  extern struct pccard_resource_ops pccard
 
 /* socket drivers are expected to use these callbacks in their .drv struct */
 extern int pcmcia_socket_dev_suspend(struct device *dev);
+extern void pcmcia_socket_dev_early_resume(struct device *dev);
+extern void pcmcia_socket_dev_late_resume(struct device *dev);
 extern int pcmcia_socket_dev_resume(struct device *dev);
 
 /* socket drivers use this callback in their IRQ handler */