linux-kernel - Re: 2.6.35.6 fails to suspend (pxa2xx-mci.0)

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: <1286788315.3101.11.camel@sven>
Date:	Mon, 11 Oct 2010 11:11:55 +0200
From:	Sven Neumann <s.neumann@...mfeld.com>
To:	Ohad Ben-Cohen <ohad@...ery.com>, libertas-dev@...ts.infradead.org
Cc:	Chris Ball <cjb@...top.org>, "Rafael J. Wysocki" <rjw@...k.pl>,
	Daniel Mack <daniel@...aq.de>,
	Colin Cross <ccross@...roid.com>,
	Greg Kroah-Hartman <gregkh@...e.de>, linux-mmc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Maxim Levitsky <maximlevitsky@...il.com>
Subject: Re: 2.6.35.6 fails to suspend (pxa2xx-mci.0)

On Mon, 2010-10-11 at 10:45 +0200, Ohad Ben-Cohen wrote:
> On Mon, Oct 11, 2010 at 10:31 AM, Sven Neumann <s.neumann@...mfeld.com> wrote:
> > On Sat, 2010-10-09 at 03:07 +0200, Ohad Ben-Cohen wrote:
> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> >> index c94565d..515ff39 100644
> >> --- a/drivers/mmc/core/core.c
> >> +++ b/drivers/mmc/core/core.c
> >> @@ -1682,6 +1682,19 @@ int mmc_suspend_host(struct mmc_host *host)
> >>         if (host->bus_ops && !host->bus_dead) {
> >>                 if (host->bus_ops->suspend)
> >>                         err = host->bus_ops->suspend(host);
> >> +               if (err == -ENOSYS || !host->bus_ops->resume) {
> >> +                       /*
> >> +                        * We simply "remove" the card in this case.
> >> +                        * It will be redetected on resume.
> >> +                        */
> >> +                       if (host->bus_ops->remove)
> >> +                               host->bus_ops->remove(host);
> >> +                       mmc_claim_host(host);
> >> +                       mmc_detach_bus(host);
> >> +                       mmc_release_host(host);
> >> +                       host->pm_flags = 0;
> >> +                       err = 0;
> >> +               }
> >>         }
> >>         mmc_bus_put(host);
> >>
> >> The reason I'm asking this is because it seems like commit
> >> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs related
> >> to mmc/sd card insert/removal during suspend/resume" has broken
> >> suspending SDIO function drivers which don't have a suspend handler
> >> (or do have one, but for some reason it returns -ENOSYS, like
> >> libertas_sdio's does).
> >
> > Yes, this patch fixes the problem. Tested with v2.6.35-rc7. With above
> > patch applied, suspend (and resume!) work again with and without
> > pm_async=0.
> >
> > Thanks a lot to all of you for your help with this problem.
> 
> Thanks a lot for testing this Sven !
> 
> It is completely fixing the regression, but I must say the end result
> is not very beautiful. We may eventually still use it, at least as an
> interim solution, but I'd at least like to explain what's going on
> here.
> 
> Patch 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e "mmc: fix all hangs
> related to mmc/sd card insert/removal during suspend/resume" (which
> was introduced in 2.6.36 and backported to stable kernels) moved the
> card remove/insert mechanism out of the suspend/resume path and into
> mmc_pm_notify (which uses pm notifiers; needed in order to let user
> space sync the card upon removal).
> 
> It broke (part of) SDIO suspend because SDIO relied on
> mmc_suspend_host() to remove the card, and squash the error, in case
> -ENOSYS is returned from the bus suspend handler (mmc_sdio_suspend()
> in this case).
> 
> mmc_sdio_suspend() is using this whenever one of the card's SDIO
> function drivers does not have suspend/resume handlers - in that case
> it is agreed to force removal of the entire card.
> 
> So the trivial thing to do here was to bring back that part of
> mmc_suspend_host(), which was removed by
> 4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e.
> 
> The reason I said the end result is not too beautiful is because now
> we have two places in the mmc suspend execution path that can remove
> the card: mmc_pm_notify and mmc_suspend_host, where the second is
> probably useful only for SDIO. The card re-insertion, btw, will only
> take place in mmc_pm_notify, and be used by all scenarios (so it's
> becoming asymmetric for SDIO).
> 
> To use mmc_pm_notify's card-removal code also for SDIO, we need to
> check there if all the SDIO functions have suspend handlers. That
> would probably make us add a new bus_ops handler (something like
> host->bus_ops->remove_card_on_suspend ?).
> 
> It's starting to be a bit complicated though, and I'm not sure it
> would make the code a lot more readable.
> 
> And, it would still not work for drivers like libertas sdio, which do
> have a suspend hander, but sometimes let it return -ENOSYS, expecting
> the MMC core to remove the card and squash the error. For those cases,
> we still need the old card-removal logic in mmc_suspend_host().
> 
> Btw, it makes me wonder: does libertas_sdio really needs this
> functionality ? When MMC_PM_KEEP_POWER is not needed, why can't
> libertas_sdio just return 0 (and as a result the card will be powered
> off, but not removed) ?

Perhaps we should bring up this question on the libertas-dev list. I am
Cc'ing them in the hope that someone can answer this question.


Sven




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