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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <201203262159.09335.rjw@sisk.pl>
Date:	Mon, 26 Mar 2012 21:59:09 +0200
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Chris Ball <cjb@...top.org>,
	Linux PM mailing list <linux-pm@...r.kernel.org>
Cc:	NeilBrown <neilb@...e.de>, lkml <linux-kernel@...r.kernel.org>,
	linux-mmc@...r.kernel.org
Subject: Re: [PATCH] PM / SDIO: Use empty system suspend/resume callbacks at the bus level (was: Re: Recent ...)

On Monday, March 26, 2012, Rafael J. Wysocki wrote:
> On Sunday, March 25, 2012, NeilBrown wrote:
> > On Sun, 25 Mar 2012 23:25:37 +0200 "Rafael J. Wysocki" <rjw@...k.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!
> 
> 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
> 
> ---
> From: Rafael J. Wysocki <rjw@...k.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@...e.de>
> Signed-off-by: Rafael J. Wysocki <rjw@...k.pl>

OK, I don't see any objections.  Does it mean I can push this patch as a fix
for 3.4?  Chris?

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",
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/