pcmcia: move unbind/rebind into dev_pm_ops.complete

PATCH RESEND New Christian Lamparter 2 participants 3 messages Jul 9, 2012 – Jul 21, 2012 Pcmcia

lore

Share & tools

lore

Christian Lamparter

author

2012-07-09 23:19 UTC #0 lore raw

(sorry, keyboard splat)

Quoted 70 lines · Andrew Morton

Quoted 60 lines · On Tue, 10 Jul 2012 00:54:54 +0200 Christian Lamparter…
Quoted 22 lines · Andrew Morton
Quoted 10 lines · On Fri, 6 Jul 2012 14:30:16 -0700 Greg KH <[email protected]> wrote:
Quoted 9 lines · Christian Lamparter
The idea of moving rebind procedure into pm.complete was taken from the usb-subsystem, which has similar problems with reattaching devices during/after resume.
Signed-off-by Christian Lamparter <[email protected]>
---
To Greg:
Quoted 8 lines
Quoted 4 lines
I have submitted this patch back in March and again in May. As far as I can tell it was neither rejected nor was it accepted into linux-pcmcia.git since. So I'm asking you, if you could take the patch instead... please.
There is a PCMCIA "team" who should be taking these types of patches. Why are they not doing so?
Quoted 8 lines
Things are pretty quiet in pcmcia world, but Dominik does appear to still be doing stuff.
I sometimes queue PCMCIA patches for people, but not this one. The changelog is just junk. What does the patch do? Why does it do it? What problems does it solve? What are these mysterious "problems with reattaching devices" to which it refers? Useless...
Well, that can be improved, but it is a bit tricky. AFAICT the usb subsystem dealt with pm in this commit:
"commit 5096aedcd2eb70fbea83f09281f97f9ec973d9de
Author: Alan Stern <[email protected]>
Date:   Tue Aug 12 14:34:14 2008 -0400
USB: Don't rebind before "complete" callback
[...] We are not allowed to call drivers' probe routines during a system sleep transition between the "prepare" and "complete" callbacks, but that's exactly what we do when a driver doesn't have full suspend/resume support. [...]"
And on the pcmcia subsystem we have this:
"commit 88b060d6c03fcb9e4d2018b4349954c4242a5c7f
Author: Dominik Brodowski <[email protected]>
Date:   Sat Jan 2 14:14:23 2010 +0100
pcmcia: improve check for same card in slot after resume
During a suspend/resume cycle, an user may change the card in the PCMCIA/CardBus slot. [...]
For CardBus devices, the PCI hotplug interface doesn't offer a "rescan" facility which also _removes_ devices no longer to be found behind a bridge. Therefore, remove and re-add all devices unconditionally."
Quoted 7 lines
Unfortunately, the "re-add" is currently done in the *pm resume* callback (socket_late_resume), but according to "USB: Don't rebind..." is not allowed to have it there, so the patch moves it into the *pm complete* callback. The Documentation/power/* contains mostly informations for drivers developers, but AFAICT it doesn't say much about the subsystem to which the device is connected should behave, so there's a bit of a "citing-gap".

Quoted 5 lines

hm, it does seem a bit of a screwup.
What's unclear to me is whether your patch fixes any observed runtime problems. Or adds any runtime problems, which looks to be a distinct possibility.

well, the patch for the pcmcia subsystem is part of a fix for this
bug-report: <https://bugzilla.kernel.org/show_bug.cgi?id=40772>
(Note: the firmware-core changes I talk about there have been fixed by Rafael with his "firmware_class: Rework usermodehelper check" series)

But that could very well be a straw-man argument. As I can't really tell without the same HW. (I have mostly minipci hardware, and the laptop I have with a cardbus doesn't really suspend reliably).

Quoted 9 lines

Quoted 5 lines
So, my question now: Would you accept the pcmcia patch if I add the "USB: Don't rebind..." as a reference to why the re-add needs to be done in complete? Or do you think that I should bug the pm people (and Alan - since he wrote that it is "not allowed") in this case so I can link their official answer to this patch?
Well I could grab it and give it a little bit of testing in linux-next. But I'd be super-reluctant to send such a patch upstream without detailed input from Alan/Greg/Rafael/Dominik/etc.

Alright, I've added them in the 'CC'.

To Alan:
Can you please tell me, if you (still) know the details why the "re-add" is not allowed in _resume callback (see patch message & commit id from
above: USB: Don't rebind before "complete")?

Regards,
Chr

Good night!

Alan Stern

2012-07-10 14:20 UTC #1 lore raw

Quoted 9 lines · Christian Lamparter

Quoted 9 lines
Quoted 5 lines
So, my question now: Would you accept the pcmcia patch if I add the "USB: Don't rebind..." as a reference to why the re-add needs to be done in complete? Or do you think that I should bug the pm people (and Alan - since he wrote that it is "not allowed") in this case so I can link their official answer to this patch?
Well I could grab it and give it a little bit of testing in linux-next. But I'd be super-reluctant to send such a patch upstream without detailed input from Alan/Greg/Rafael/Dominik/etc.

Quoted 6 lines

Alright, I've added them in the 'CC'.
To Alan:
Can you please tell me, if you (still) know the details why the "re-add" is not allowed in _resume callback (see patch message & commit id from
above: USB: Don't rebind before "complete")?

It's a little complicated. The basic idea is that the PM core needs to send suspend and resume messages to every device in the right order, and it can't do that if new devices are being added at the same time. That's one of the main reasons we created the "prepare" and "complete" callbacks.

However the situation really isn't quite that rigid. In particular, adding new children during a resume callback shouldn't cause much of problem because the children don't need to be resumed anyway (since they were never suspended). On the other hand, if you do it you will get a dev_warn() from the PM core, something like "parent should not be sleeping".

Still, it is considered bad form and should be avoided if possible.

Alan Stern

Christian Lamparter

author

2012-07-21 23:15 UTC #2 lore raw

PATCH v2 1/1 pcmcia: move unbind/rebind into dev_pm_ops.complete

In reply to Christian Lamparter

This patch moves the device rebind procedures for cardbus devices from the pm.resume into the pm.complete callback.

The reasons for moving the code is:"
[...] The PM code needs to send suspend and resume messages to every device in the right order, and it can't do that if new devices are being added at the same time. [...]

Still, it is considered bad form and should be avoided if possible."

(Alan Stern's full comment about the topic can be found here: <https://lkml.org/lkml/2012/7/10/254>)

Cc Dominik Brodowski <[email protected]>

Signed-off-by Christian Lamparter <[email protected]>

---
I think I left everyone enough of a chance to reply and/or file complains.

The change from v1 is mainly the updated change log and I hope it now sufficiently explains why a patch like this is needed (Of course, if any of you have an alternative suggestion, then please let's hear it!).

Note: It would be nice to know what the patch does for older (not cardbus) cards in the suspend/resume case. So, if you got a machine and the hardware, please let us/me know if it works or causes crash/boom and bangs.

PS: I'm not on the pcmcia/kernel list, so please keep my address in the 'CC' at all times.

Regards,
Christian
---

drivers/pcmcia/cs.c +30 -7

index 673c14e..18fb100 100644
@@ -484,7 +484,7 @@ static int socket_early_resume(struct pcmcia_socket *skt)
 
 static int socket_late_resume(struct pcmcia_socket *skt)
 {
-	int ret;
+	int ret = 0;
 
 	mutex_lock(&skt->ops_mutex);
 	skt->state &= ~SOCKET_SUSPEND;

@@ -511,19 +511,31 @@ static int socket_late_resume(struct pcmcia_socket *skt)
 		return socket_insert(skt);
 	}
 
+	if (!(skt->state & SOCKET_CARDBUS) && (skt->callback))
+		ret = skt->callback->early_resume(skt);
+	return ret;
+}
+
+/*
+ * Finalize the resume. In case of a cardbus socket, we have
+ * to rebind the devices as we can't be certain that it has been
+ * replaced, or not.
+ */
+static int socket_complete_resume(struct pcmcia_socket *skt)
+{
+	int ret = 0;
 #ifdef CONFIG_CARDBUS
 	if (skt->state & SOCKET_CARDBUS) {
 		/* We can't be sure the CardBus card is the same
 		 * as the one previously inserted. Therefore, remove
 		 * and re-add... */
 		cb_free(skt);
-		cb_alloc(skt);
-		return 0;
+		ret = cb_alloc(skt);
+		if (ret)
+			cb_free(skt);
 	}
 #endif
-	if (!(skt->state & SOCKET_CARDBUS) && (skt->callback))
-		skt->callback->early_resume(skt);
-	return 0;
+	return ret;
 }
 
 /*

@@ -533,11 +545,15 @@ static int socket_late_resume(struct pcmcia_socket *skt)
  */
 static int socket_resume(struct pcmcia_socket *skt)
 {
+	int err;
 	if (!(skt->state & SOCKET_SUSPEND))
 		return -EBUSY;
 
 	socket_early_resume(skt);
-	return socket_late_resume(skt);
+	err = socket_late_resume(skt);
+	if (!err)
+		err = socket_complete_resume(skt);
+	return err;
 }
 
 static void socket_remove(struct pcmcia_socket *skt)

@@ -848,6 +864,12 @@ static int __used pcmcia_socket_dev_resume(struct device *dev)
 	return __pcmcia_pm_op(dev, socket_late_resume);
 }
 
+static void __used pcmcia_socket_dev_complete(struct device *dev)
+{
+	WARN(__pcmcia_pm_op(dev, socket_complete_resume),
+		"failed to complete resume");
+}
+
 static const struct dev_pm_ops pcmcia_socket_pm_ops = {
 	/* dev_resume may be called with IRQs enabled */
 	SET_SYSTEM_SLEEP_PM_OPS(NULL,

@@ -862,6 +884,7 @@ static const struct dev_pm_ops pcmcia_socket_pm_ops = {
 	.resume_noirq = pcmcia_socket_dev_resume_noirq,
 	.thaw_noirq = pcmcia_socket_dev_resume_noirq,
 	.restore_noirq = pcmcia_socket_dev_resume_noirq,
+	.complete = pcmcia_socket_dev_complete,
 };
 
 #define PCMCIA_SOCKET_CLASS_PM_OPS (&pcmcia_socket_pm_ops)