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

[NeilBrown <neilb@suse.de>]

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