Recent "Run the driver callback directly" patch breaks libertas suspend

Recent "Run the driver callback directly" patch breaks libertas suspend

[NeilBrown]

[-- Attachment #1.1: Type: text/plain, Size: 1087 bytes --]


Hi Rafael,

 Your recent patch:
   commit 35cd133c
   PM: Run the driver callback directly if the subsystem one is not there

 breaks suspend for my libertas wifi and probably other SDIO devices.

 SDIO (and possible MMC in general) has a protocol where the suspend
 method can return -ENOSYS and this means "There is no point in suspending,
 just turn me off".

 The device itself "mmc1:0001" (I think) doesn't have any bus etc 'suspend'
 function so the new code call the device's suspend function which returns
 ENOSYS and the suspend fails.

 The previous code ignores the device as there is no bus suspend, and when it
 gets to suspend the ancestor - which for me is omap_hsmmc.1, it calls the
 device suspend function catches the ENOSYS, and turns it off.

 I suspect just reverting it isn't the right long term solution, however I
 can confirm that it works for me for now.

 I'm happy to try any alternate fixes you would like to suggest (but I cannot
 promise how quickly I will get the testing done).

 (I'm testing with 3.3)

Thanks,
NeilBrown

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [linux-pm] Recent "Run the driver callback directly" patch breaks libertas suspend

[Alan Stern]

On Sun, 25 Mar 2012, NeilBrown wrote:

> Hi Rafael,
> 
>  Your recent patch:
>    commit 35cd133c
>    PM: Run the driver callback directly if the subsystem one is not there
> 
>  breaks suspend for my libertas wifi and probably other SDIO devices.
> 
>  SDIO (and possible MMC in general) has a protocol where the suspend
>  method can return -ENOSYS and this means "There is no point in suspending,
>  just turn me off".
> 
>  The device itself "mmc1:0001" (I think) doesn't have any bus etc 'suspend'
>  function so the new code call the device's suspend function which returns
>  ENOSYS and the suspend fails.
> 
>  The previous code ignores the device as there is no bus suspend, and when it
>  gets to suspend the ancestor - which for me is omap_hsmmc.1, it calls the
>  device suspend function catches the ENOSYS, and turns it off.
> 
>  I suspect just reverting it isn't the right long term solution, however I
>  can confirm that it works for me for now.
> 
>  I'm happy to try any alternate fixes you would like to suggest (but I cannot
>  promise how quickly I will get the testing done).

With a little restructuring, this might be a good application for
pm_runtime_no_callbacks().

Alan Stern

Re: [linux-pm] Recent "Run the driver callback directly" patch breaks libertas suspend

[Rafael J. Wysocki]

On Sunday, March 25, 2012, Alan Stern wrote:
> On Sun, 25 Mar 2012, NeilBrown wrote:
> 
> > Hi Rafael,
> > 
> >  Your recent patch:
> >    commit 35cd133c
> >    PM: Run the driver callback directly if the subsystem one is not there
> > 
> >  breaks suspend for my libertas wifi and probably other SDIO devices.
> > 
> >  SDIO (and possible MMC in general) has a protocol where the suspend
> >  method can return -ENOSYS and this means "There is no point in suspending,
> >  just turn me off".
> > 
> >  The device itself "mmc1:0001" (I think) doesn't have any bus etc 'suspend'
> >  function so the new code call the device's suspend function which returns
> >  ENOSYS and the suspend fails.
> > 
> >  The previous code ignores the device as there is no bus suspend, and when it
> >  gets to suspend the ancestor - which for me is omap_hsmmc.1, it calls the
> >  device suspend function catches the ENOSYS, and turns it off.
> > 
> >  I suspect just reverting it isn't the right long term solution, however I
> >  can confirm that it works for me for now.
> > 
> >  I'm happy to try any alternate fixes you would like to suggest (but I cannot
> >  promise how quickly I will get the testing done).
> 
> With a little restructuring, this might be a good application for
> pm_runtime_no_callbacks().

That won't cover system suspend, though.

Thanks,
Rafael

Re: Recent "Run the driver callback directly" patch breaks libertas suspend

[Rafael J. Wysocki]

Hi,

On Sunday, March 25, 2012, NeilBrown wrote:
> 
> Hi Rafael,
> 
>  Your recent patch:
>    commit 35cd133c
>    PM: Run the driver callback directly if the subsystem one is not there
> 
>  breaks suspend for my libertas wifi and probably other SDIO devices.

Well, the patch is not recent.  The _commit_ is more than three months old
and the patch has been around since the last November (at least).

>  SDIO (and possible MMC in general) has a protocol where the suspend
>  method can return -ENOSYS and this means "There is no point in suspending,
>  just turn me off".
>
>  The device itself "mmc1:0001" (I think) doesn't have any bus etc 'suspend'
>  function so the new code call the device's suspend function which returns
>  ENOSYS and the suspend fails.
> 
>  The previous code ignores the device as there is no bus suspend, and when it
>  gets to suspend the ancestor - which for me is omap_hsmmc.1, it calls the
>  device suspend function catches the ENOSYS, and turns it off.

Well, I can only call that a blatant abuse of the PM infrastructure.

>  I suspect just reverting it isn't the right long term solution, however I
>  can confirm that it works for me for now.

It's not a solution at all, because there's code that depends on it already in
the tree and the fact that it works for you doesn't mean it won't break other
systems.  So no, it's not an option.

>  I'm happy to try any alternate fixes you would like to suggest (but I cannot
>  promise how quickly I will get the testing done).
> 
>  (I'm testing with 3.3)

The only fix I can think of is to rework SDIO to stop abusing the PM callbacks.
I'll have a look at that next week, although I can't promise anything any time
soon, because I'm heading to San Francisco on Saturday.

Thanks,
Rafael

Re: Recent "Run the driver callback directly" patch breaks libertas suspend

[Rafael J. Wysocki]

On Sunday, March 25, 2012, Rafael J. Wysocki wrote:
> Hi,
> 
> On Sunday, March 25, 2012, NeilBrown wrote:
> > 
> > Hi Rafael,
> > 
> >  Your recent patch:
> >    commit 35cd133c
> >    PM: Run the driver callback directly if the subsystem one is not there
> > 
> >  breaks suspend for my libertas wifi and probably other SDIO devices.
> 
> Well, the patch is not recent.  The _commit_ is more than three months old
> and the patch has been around since the last November (at least).
> 
> >  SDIO (and possible MMC in general) has a protocol where the suspend
> >  method can return -ENOSYS and this means "There is no point in suspending,
> >  just turn me off".
> >
> >  The device itself "mmc1:0001" (I think) doesn't have any bus etc 'suspend'
> >  function so the new code call the device's suspend function which returns
> >  ENOSYS and the suspend fails.
> > 
> >  The previous code ignores the device as there is no bus suspend, and when it
> >  gets to suspend the ancestor - which for me is omap_hsmmc.1, it calls the
> >  device suspend function catches the ENOSYS, and turns it off.
> 
> Well, I can only call that a blatant abuse of the PM infrastructure.
> 
> >  I suspect just reverting it isn't the right long term solution, however I
> >  can confirm that it works for me for now.
> 
> It's not a solution at all, because there's code that depends on it already in
> the tree and the fact that it works for you doesn't mean it won't break other
> systems.  So no, it's not an option.
> 
> >  I'm happy to try any alternate fixes you would like to suggest (but I cannot
> >  promise how quickly I will get the testing done).
> > 
> >  (I'm testing with 3.3)
> 
> The only fix I can think of is to rework SDIO to stop abusing the PM callbacks.
> I'll have a look at that next week, although I can't promise anything any time
> soon, because I'm heading to San Francisco on Saturday.

Well, this is kind of a long shot, but I wonder if the patch below makes
any difference?

Rafael

---
 drivers/mmc/core/sdio_bus.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Index: linux/drivers/mmc/core/sdio_bus.c
===================================================================
--- linux.orig/drivers/mmc/core/sdio_bus.c
+++ linux/drivers/mmc/core/sdio_bus.c
@@ -192,9 +192,15 @@ static int sdio_bus_remove(struct device
 	return ret;
 }
 
-#ifdef CONFIG_PM_RUNTIME
+#ifdef CONFIG_PM
+
+static int pm_no_operation(struct device *dev)
+{
+	return 0;
+}
 
 static const struct dev_pm_ops sdio_bus_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_no_operation, pm_no_operation)
 	SET_RUNTIME_PM_OPS(
 		pm_generic_runtime_suspend,
 		pm_generic_runtime_resume,
@@ -204,11 +210,11 @@ static const struct dev_pm_ops sdio_bus_
 
 #define SDIO_PM_OPS_PTR	(&sdio_bus_pm_ops)
 
-#else /* !CONFIG_PM_RUNTIME */
+#else /* !CONFIG_PM */
 
 #define SDIO_PM_OPS_PTR	NULL
 
-#endif /* !CONFIG_PM_RUNTIME */
+#endif /* !CONFIG_PM */
 
 static struct bus_type sdio_bus_type = {
 	.name		= "sdio",

Re: Recent "Run the driver callback directly" patch breaks libertas suspend

[NeilBrown]

[-- Attachment #1.1: Type: text/plain, Size: 3671 bytes --]

On Sun, 25 Mar 2012 23:25:37 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Sunday, March 25, 2012, Rafael J. Wysocki wrote:
> > Hi,
> > 
> > On Sunday, March 25, 2012, NeilBrown wrote:
> > > 
> > > Hi Rafael,
> > > 
> > >  Your recent patch:
> > >    commit 35cd133c
> > >    PM: Run the driver callback directly if the subsystem one is not there
> > > 
> > >  breaks suspend for my libertas wifi and probably other SDIO devices.
> > 
> > Well, the patch is not recent.  The _commit_ is more than three months old
> > and the patch has been around since the last November (at least).
> > 
> > >  SDIO (and possible MMC in general) has a protocol where the suspend
> > >  method can return -ENOSYS and this means "There is no point in suspending,
> > >  just turn me off".
> > >
> > >  The device itself "mmc1:0001" (I think) doesn't have any bus etc 'suspend'
> > >  function so the new code call the device's suspend function which returns
> > >  ENOSYS and the suspend fails.
> > > 
> > >  The previous code ignores the device as there is no bus suspend, and when it
> > >  gets to suspend the ancestor - which for me is omap_hsmmc.1, it calls the
> > >  device suspend function catches the ENOSYS, and turns it off.
> > 
> > Well, I can only call that a blatant abuse of the PM infrastructure.
> > 
> > >  I suspect just reverting it isn't the right long term solution, however I
> > >  can confirm that it works for me for now.
> > 
> > It's not a solution at all, because there's code that depends on it already in
> > the tree and the fact that it works for you doesn't mean it won't break other
> > systems.  So no, it's not an option.
> > 
> > >  I'm happy to try any alternate fixes you would like to suggest (but I cannot
> > >  promise how quickly I will get the testing done).
> > > 
> > >  (I'm testing with 3.3)
> > 
> > The only fix I can think of is to rework SDIO to stop abusing the PM callbacks.
> > I'll have a look at that next week, although I can't promise anything any time
> > soon, because I'm heading to San Francisco on Saturday.
> 
> Well, this is kind of a long shot, but I wonder if the patch below makes
> any difference?

Hi Rafael,
 thanks for looking into this so quickly.

I removed the revert and applied this patch instead and can confirm that
suspend completes now (and resume works and the wifi works after resume).

Further, this patch fits with my (fairly shallow) understanding of the
problem.

Thanks!

NeilBrown

> 
> Rafael
> 
> ---
>  drivers/mmc/core/sdio_bus.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> Index: linux/drivers/mmc/core/sdio_bus.c
> ===================================================================
> --- linux.orig/drivers/mmc/core/sdio_bus.c
> +++ linux/drivers/mmc/core/sdio_bus.c
> @@ -192,9 +192,15 @@ static int sdio_bus_remove(struct device
>  	return ret;
>  }
>  
> -#ifdef CONFIG_PM_RUNTIME
> +#ifdef CONFIG_PM
> +
> +static int pm_no_operation(struct device *dev)
> +{
> +	return 0;
> +}
>  
>  static const struct dev_pm_ops sdio_bus_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_no_operation, pm_no_operation)
>  	SET_RUNTIME_PM_OPS(
>  		pm_generic_runtime_suspend,
>  		pm_generic_runtime_resume,
> @@ -204,11 +210,11 @@ static const struct dev_pm_ops sdio_bus_
>  
>  #define SDIO_PM_OPS_PTR	(&sdio_bus_pm_ops)
>  
> -#else /* !CONFIG_PM_RUNTIME */
> +#else /* !CONFIG_PM */
>  
>  #define SDIO_PM_OPS_PTR	NULL
>  
> -#endif /* !CONFIG_PM_RUNTIME */
> +#endif /* !CONFIG_PM */
>  
>  static struct bus_type sdio_bus_type = {
>  	.name		= "sdio",


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] PM / SDIO: Use empty system suspend/resume callbacks at the bus level (was: Re: Recent ...)

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4064 bytes --]

On Mon, 26 Mar 2012 00:29:24 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:


> Thanks for the confirmation.
> 
> Below it goes again with a changelog and tags.
> 
> I don't really think that SDIO does the right thing here overall, but that's
> all I can do to address the problem timely.
> 
> Thanks,
> Rafael

Hi Rafael,
 I just discovered that this patch has since been reverted - with an 'ack'
 from you:
----------
commit d8e2ac330f65bcf47e8894fe5331a7e8ee019c06
Author: Thierry Reding <thierry.reding@avionic-design.de>
Date:   Thu Aug 9 09:32:21 2012 +0000

    mmc: sdio: Fix PM_SLEEP related build warnings
    
    Power management callbacks defined by SIMPLE_DEV_PM_OPS are only used if
    the PM_SLEEP Kconfig symbol has been defined. If not, the compiler will
    complain about them being unused. However, since the callback for this
    driver doesn't do anything it can just as well be dropped.
    
    Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
    Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
    Signed-off-by: Chris Ball <cjb@laptop.org>
-----------

Unsurprisingly the problem which your patch fixed has come back.

Do you think we could get the patch back in again.  This time maybe we should
put some comments in there pointing out that having a function which does
nothing is very different from not having any function at all?

Thanks,
NeilBrown


> 
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: PM / SDIO: Use empty system suspend/resume callbacks at the bus level
> 
> Neil Brown reports that commit 35cd133c
> 
>    PM: Run the driver callback directly if the subsystem one is not there
> 
> breaks suspend for his libertas wifi, because SDIO has a protocol
> where the suspend method can return -ENOSYS and this means "There is
> no point in suspending, just turn me off".  Moreover, the suspend
> methods provided by SDIO drivers are not supposed to be called by
> the PM core or bus-level suspend routines (which aren't presend for
> SDIO).  Instead, when the SDIO core gets to suspend the device's
> ancestor, it calls the device driver's suspend function, catches the
> ENOSYS, and turns the device off.
> 
> The commit above breaks the SDIO core's assumption that the device
> drivers' callbacks won't be executed if it doesn't provide any
> bus-level callbacks.  If fact, however, this assumption has never
> been really satisfied, because device class or device type suspend
> might very well use the driver's callback even without that commit.
> 
> The simplest way to address this problem is to make the SDIO core
> tell the PM core to ignore driver callbacks, for example by providing
> no-operation suspend/resume callbacks at the bus level for it,
> which is implemented by this change.
> 
> Reported-and-tested-by: Neil Brown <neilb@suse.de>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/mmc/core/sdio_bus.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> Index: linux/drivers/mmc/core/sdio_bus.c
> ===================================================================
> --- linux.orig/drivers/mmc/core/sdio_bus.c
> +++ linux/drivers/mmc/core/sdio_bus.c
> @@ -192,9 +192,15 @@ static int sdio_bus_remove(struct device
>  	return ret;
>  }
>  
> -#ifdef CONFIG_PM_RUNTIME
> +#ifdef CONFIG_PM
> +
> +static int pm_no_operation(struct device *dev)
> +{
> +	return 0;
> +}
>  
>  static const struct dev_pm_ops sdio_bus_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_no_operation, pm_no_operation)
>  	SET_RUNTIME_PM_OPS(
>  		pm_generic_runtime_suspend,
>  		pm_generic_runtime_resume,
> @@ -204,11 +210,11 @@ static const struct dev_pm_ops sdio_bus_
>  
>  #define SDIO_PM_OPS_PTR	(&sdio_bus_pm_ops)
>  
> -#else /* !CONFIG_PM_RUNTIME */
> +#else /* !CONFIG_PM */
>  
>  #define SDIO_PM_OPS_PTR	NULL
>  
> -#endif /* !CONFIG_PM_RUNTIME */
> +#endif /* !CONFIG_PM */
>  
>  static struct bus_type sdio_bus_type = {
>  	.name		= "sdio",


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

[PATCH] SDIO / PM: Add empty bus-level suspend/resume callbacks

[Rafael J. Wysocki]

On Sunday, December 02, 2012 07:46:25 PM NeilBrown wrote:
> On Mon, 26 Mar 2012 00:29:24 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> 
> > Thanks for the confirmation.
> > 
> > Below it goes again with a changelog and tags.
> > 
> > I don't really think that SDIO does the right thing here overall, but that's
> > all I can do to address the problem timely.
> > 
> > Thanks,
> > Rafael
> 
> Hi Rafael,
>  I just discovered that this patch has since been reverted - with an 'ack'
>  from you:
> ----------
> commit d8e2ac330f65bcf47e8894fe5331a7e8ee019c06
> Author: Thierry Reding <thierry.reding@avionic-design.de>
> Date:   Thu Aug 9 09:32:21 2012 +0000
> 
>     mmc: sdio: Fix PM_SLEEP related build warnings
>     
>     Power management callbacks defined by SIMPLE_DEV_PM_OPS are only used if
>     the PM_SLEEP Kconfig symbol has been defined. If not, the compiler will
>     complain about them being unused. However, since the callback for this
>     driver doesn't do anything it can just as well be dropped.
>     
>     Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
>     Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
>     Signed-off-by: Chris Ball <cjb@laptop.org>
> -----------
> 
> Unsurprisingly the problem which your patch fixed has come back.
> 
> Do you think we could get the patch back in again.  This time maybe we should
> put some comments in there pointing out that having a function which does
> nothing is very different from not having any function at all?

Well, I agree.  I didn't remember that the callback had been added for
a purpose and hence my "ack" for that patch.

What about applying the appended patch (hopefully, the build warnings should
be fixed properly this time)?

Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: SDIO / PM: Add empty bus-level suspend/resume callbacks

Suspend methods provided by SDIO drivers are not supposed to be
called by the PM core.  Instead, when the SDIO core gets to suspend
a device's ancestor, it calls the device driver's suspend routine.
However, the PM core executes suspend callback routines directly for
device drivers whose bus types don't provide suspend callbacks.
In consequece, because the SDIO bus type doesn't provide a suspend
callback, the SDIO drivers' suspend routines will be executed by the
PM core (which shouldn't happen).

To prevent this from happening, add empty system suspend/resume
callbacks for the SDIO bus type.

An analogous change had been made already by commit (e841a7c mmc:
sdio: Use empty system suspend/resume callbacks at the bus level),
but then it was reverted inadvertently by commit (d8e2ac3 mmc: sdio:
Fix PM_SLEEP related build warnings) that attempted to fix build
warnings introduced by commit e841a7c.

Reported-by: NeilBrown <neilb@suse.de>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/mmc/core/sdio_bus.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

Index: linux/drivers/mmc/core/sdio_bus.c
===================================================================
--- linux.orig/drivers/mmc/core/sdio_bus.c
+++ linux/drivers/mmc/core/sdio_bus.c
@@ -193,7 +193,21 @@ static int sdio_bus_remove(struct device
 }
 
 #ifdef CONFIG_PM
+
+#ifdef CONFIG_PM_SLEEP
+static int pm_no_operation(struct device *dev)
+{
+	/*
+	 * Prevent the PM core from calling SDIO device drivers' suspend
+	 * callback routines, which it is not supposed to do, by using this
+	 * empty function as the bus type suspend callaback for SDIO.
+	 */
+	return 0;
+}
+#endif
+
 static const struct dev_pm_ops sdio_bus_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_no_operation, pm_no_operation)
 	SET_RUNTIME_PM_OPS(
 		pm_generic_runtime_suspend,
 		pm_generic_runtime_resume,


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] SDIO / PM: Add empty bus-level suspend/resume callbacks

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4191 bytes --]

On Sun, 02 Dec 2012 14:48:50 +0100 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Sunday, December 02, 2012 07:46:25 PM NeilBrown wrote:
> > On Mon, 26 Mar 2012 00:29:24 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > 
> > 
> > > Thanks for the confirmation.
> > > 
> > > Below it goes again with a changelog and tags.
> > > 
> > > I don't really think that SDIO does the right thing here overall, but that's
> > > all I can do to address the problem timely.
> > > 
> > > Thanks,
> > > Rafael
> > 
> > Hi Rafael,
> >  I just discovered that this patch has since been reverted - with an 'ack'
> >  from you:
> > ----------
> > commit d8e2ac330f65bcf47e8894fe5331a7e8ee019c06
> > Author: Thierry Reding <thierry.reding@avionic-design.de>
> > Date:   Thu Aug 9 09:32:21 2012 +0000
> > 
> >     mmc: sdio: Fix PM_SLEEP related build warnings
> >     
> >     Power management callbacks defined by SIMPLE_DEV_PM_OPS are only used if
> >     the PM_SLEEP Kconfig symbol has been defined. If not, the compiler will
> >     complain about them being unused. However, since the callback for this
> >     driver doesn't do anything it can just as well be dropped.
> >     
> >     Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> >     Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> >     Signed-off-by: Chris Ball <cjb@laptop.org>
> > -----------
> > 
> > Unsurprisingly the problem which your patch fixed has come back.
> > 
> > Do you think we could get the patch back in again.  This time maybe we should
> > put some comments in there pointing out that having a function which does
> > nothing is very different from not having any function at all?
> 
> Well, I agree.  I didn't remember that the callback had been added for
> a purpose and hence my "ack" for that patch.
> 
> What about applying the appended patch (hopefully, the build warnings should
> be fixed properly this time)?

Looks good to me - thanks!

NeilBrown

> 
> Rafael
> 
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: SDIO / PM: Add empty bus-level suspend/resume callbacks
> 
> Suspend methods provided by SDIO drivers are not supposed to be
> called by the PM core.  Instead, when the SDIO core gets to suspend
> a device's ancestor, it calls the device driver's suspend routine.
> However, the PM core executes suspend callback routines directly for
> device drivers whose bus types don't provide suspend callbacks.
> In consequece, because the SDIO bus type doesn't provide a suspend
> callback, the SDIO drivers' suspend routines will be executed by the
> PM core (which shouldn't happen).
> 
> To prevent this from happening, add empty system suspend/resume
> callbacks for the SDIO bus type.
> 
> An analogous change had been made already by commit (e841a7c mmc:
> sdio: Use empty system suspend/resume callbacks at the bus level),
> but then it was reverted inadvertently by commit (d8e2ac3 mmc: sdio:
> Fix PM_SLEEP related build warnings) that attempted to fix build
> warnings introduced by commit e841a7c.
> 
> Reported-by: NeilBrown <neilb@suse.de>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/mmc/core/sdio_bus.c |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> Index: linux/drivers/mmc/core/sdio_bus.c
> ===================================================================
> --- linux.orig/drivers/mmc/core/sdio_bus.c
> +++ linux/drivers/mmc/core/sdio_bus.c
> @@ -193,7 +193,21 @@ static int sdio_bus_remove(struct device
>  }
>  
>  #ifdef CONFIG_PM
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int pm_no_operation(struct device *dev)
> +{
> +	/*
> +	 * Prevent the PM core from calling SDIO device drivers' suspend
> +	 * callback routines, which it is not supposed to do, by using this
> +	 * empty function as the bus type suspend callaback for SDIO.
> +	 */
> +	return 0;
> +}
> +#endif
> +
>  static const struct dev_pm_ops sdio_bus_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_no_operation, pm_no_operation)
>  	SET_RUNTIME_PM_OPS(
>  		pm_generic_runtime_suspend,
>  		pm_generic_runtime_resume,
> 
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] SDIO / PM: Add empty bus-level suspend/resume callbacks

[Chris Ball]

Hi,

On Sun, Dec 02 2012, NeilBrown wrote:
>> What about applying the appended patch (hopefully, the build warnings should
>> be fixed properly this time)?
>
> Looks good to me - thanks!

Thanks, both of you, pushed to mmc-next for 3.8.

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] SDIO / PM: Add empty bus-level suspend/resume callbacks

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4339 bytes --]

On Sun, Dec 02, 2012 at 02:48:50PM +0100, Rafael J. Wysocki wrote:
> On Sunday, December 02, 2012 07:46:25 PM NeilBrown wrote:
> > On Mon, 26 Mar 2012 00:29:24 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > 
> > 
> > > Thanks for the confirmation.
> > > 
> > > Below it goes again with a changelog and tags.
> > > 
> > > I don't really think that SDIO does the right thing here overall, but that's
> > > all I can do to address the problem timely.
> > > 
> > > Thanks,
> > > Rafael
> > 
> > Hi Rafael,
> >  I just discovered that this patch has since been reverted - with an 'ack'
> >  from you:
> > ----------
> > commit d8e2ac330f65bcf47e8894fe5331a7e8ee019c06
> > Author: Thierry Reding <thierry.reding@avionic-design.de>
> > Date:   Thu Aug 9 09:32:21 2012 +0000
> > 
> >     mmc: sdio: Fix PM_SLEEP related build warnings
> >     
> >     Power management callbacks defined by SIMPLE_DEV_PM_OPS are only used if
> >     the PM_SLEEP Kconfig symbol has been defined. If not, the compiler will
> >     complain about them being unused. However, since the callback for this
> >     driver doesn't do anything it can just as well be dropped.
> >     
> >     Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> >     Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> >     Signed-off-by: Chris Ball <cjb@laptop.org>
> > -----------
> > 
> > Unsurprisingly the problem which your patch fixed has come back.
> > 
> > Do you think we could get the patch back in again.  This time maybe we should
> > put some comments in there pointing out that having a function which does
> > nothing is very different from not having any function at all?
> 
> Well, I agree.  I didn't remember that the callback had been added for
> a purpose and hence my "ack" for that patch.
> 
> What about applying the appended patch (hopefully, the build warnings should
> be fixed properly this time)?
> 
> Rafael
> 
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: SDIO / PM: Add empty bus-level suspend/resume callbacks
> 
> Suspend methods provided by SDIO drivers are not supposed to be
> called by the PM core.  Instead, when the SDIO core gets to suspend
> a device's ancestor, it calls the device driver's suspend routine.
> However, the PM core executes suspend callback routines directly for
> device drivers whose bus types don't provide suspend callbacks.
> In consequece, because the SDIO bus type doesn't provide a suspend
> callback, the SDIO drivers' suspend routines will be executed by the
> PM core (which shouldn't happen).
> 
> To prevent this from happening, add empty system suspend/resume
> callbacks for the SDIO bus type.
> 
> An analogous change had been made already by commit (e841a7c mmc:
> sdio: Use empty system suspend/resume callbacks at the bus level),
> but then it was reverted inadvertently by commit (d8e2ac3 mmc: sdio:
> Fix PM_SLEEP related build warnings) that attempted to fix build
> warnings introduced by commit e841a7c.
> 
> Reported-by: NeilBrown <neilb@suse.de>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/mmc/core/sdio_bus.c |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> Index: linux/drivers/mmc/core/sdio_bus.c
> ===================================================================
> --- linux.orig/drivers/mmc/core/sdio_bus.c
> +++ linux/drivers/mmc/core/sdio_bus.c
> @@ -193,7 +193,21 @@ static int sdio_bus_remove(struct device
>  }
>  
>  #ifdef CONFIG_PM
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int pm_no_operation(struct device *dev)
> +{
> +	/*
> +	 * Prevent the PM core from calling SDIO device drivers' suspend
> +	 * callback routines, which it is not supposed to do, by using this
> +	 * empty function as the bus type suspend callaback for SDIO.
> +	 */
> +	return 0;
> +}
> +#endif
> +
>  static const struct dev_pm_ops sdio_bus_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_no_operation, pm_no_operation)
>  	SET_RUNTIME_PM_OPS(
>  		pm_generic_runtime_suspend,
>  		pm_generic_runtime_resume,
> 

Hehe... if my memory serves me well, that's exactly (well, modulo the
comment) what my initial patch did before somebody suggested that the
empty callbacks should just be removed altogether. =)

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]