Re: [RESEND AGAIN][PATCH] pcmcia: move unbind/rebind into dev_pm_ops.complete

Re: [RESEND AGAIN][PATCH] pcmcia: move unbind/rebind into dev_pm_ops.complete

[Christian Lamparter]

(sorry, keyboard splat)

On Tuesday, July 10, 2012 01:01:31 AM Andrew Morton wrote:
> On Tue, 10 Jul 2012 00:54:54 +0200
> Christian Lamparter <chunkeey@googlemail.com> wrote:
> > On Monday, July 09, 2012 11:59:39 PM Andrew Morton wrote:
> > > On Fri, 6 Jul 2012 14:30:16 -0700
> > > Greg KH <gregkh@linuxfoundation.org> wrote:
> > > 
> > > > On Fri, Jul 06, 2012 at 11:23:52PM +0200, Christian Lamparter wrote:
> > > > > 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 <chunkeey@googlemail.com>
> > > > > ---
> > > > > To Greg:
> > > > > 
> > > > > 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?
> > > > 
> > > 
> > > 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 <stern@rowland.harvard.edu>
> > 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 <linux@dominikbrodowski.net>
> > 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."
> > 
> > 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".
> 
> 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).

> > 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!

Re: [RESEND AGAIN][PATCH] pcmcia: move unbind/rebind into dev_pm_ops.complete

[Alan Stern]

On Tue, 10 Jul 2012, Christian Lamparter wrote:

> > > 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")?

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

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

[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. [...]

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's full comment about the topic can
be found here: <https://lkml.org/lkml/2012/7/10/254>)

Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
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
---
diff --git a/drivers/pcmcia/cs.c b/drivers/pcmcia/cs.c
index 673c14e..18fb100 100644
--- a/drivers/pcmcia/cs.c
+++ b/drivers/pcmcia/cs.c
@@ -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)