On Fri, 6 Sep 2019 at 01:48, Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > Hi, > > On Tue, Sep 3, 2019 at 7:22 AM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote: > > > > System suspend/resume of SDIO cards, with SDIO IRQs enabled and when using > > MMC_CAP2_SDIO_IRQ_NOTHREAD is unfortunate still suffering from a fragile > > behaviour. Some problems have been taken care of so far, but more issues > > remains. > > > > For example, calling the ->ack_sdio_irq() callback to let host drivers > > re-enable the SDIO IRQs is a bad idea, unless the IRQ have been consumed, > > which may not be the case during system suspend/resume. This may lead to > > that a host driver re-signals the same SDIO IRQ over and over again, > > causing a storm of IRQs and gives a ping-pong effect towards the > > sdio_irq_work(). > > > > Moreover, calling the ->enable_sdio_irq() callback at system resume to > > re-enable already enabled SDIO IRQs for the host, causes the runtime PM > > count for some host drivers to become in-balanced. This then leads to the > > host to remain runtime resumed, no matter if it's needed or not. > > > > To fix these problems, let's check if process_sdio_pending_irqs() actually > > consumed the SDIO IRQ, before we continue to ack the IRQ by invoking the > > ->ack_sdio_irq() callback. > > > > Additionally, there should be no need to re-enable SDIO IRQs as the host > > driver already knows if they were enabled at system suspend, thus also > > whether it needs to re-enable them at system resume. For this reason, drop > > the call to ->enable_sdio_irq() during system resume. > > > > In regards to these changes there is yet another issue, which is when there > > is an SDIO IRQ being signaled by the host driver, but after the SDIO card > > has been system suspended. Currently these IRQs are just thrown away, while > > we should at least make sure to try to consume them when the SDIO card has > > been system resumed. Fix this by calling sdio_signal_irq() after system > > resumed the SDIO card. > > > > Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx> > > --- > > drivers/mmc/core/sdio.c | 2 +- > > drivers/mmc/core/sdio_irq.c | 3 ++- > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c > > index c557f1519b77..3114d496495a 100644 > > --- a/drivers/mmc/core/sdio.c > > +++ b/drivers/mmc/core/sdio.c > > @@ -1015,7 +1015,7 @@ static int mmc_sdio_resume(struct mmc_host *host) > > if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD)) > > wake_up_process(host->sdio_irq_thread); > > else if (host->caps & MMC_CAP_SDIO_IRQ) > > - host->ops->enable_sdio_irq(host, 1); > > + sdio_signal_irq(host); > > Is this always safe? On 1-function cards you won't poll CCCR_INTx so > you'll always signal an interrupt at resume time, won't you? Good point! What we really want to do is to just schedule the work and not include to set the sdio_irq_pending flag. Actually, the flag may have been set, in case a host driver have called sdio_signal_irq() when the SDIO card was suspended - but that is the intention. Thanks for pointing out the issue, I will re-spin and fix it! Kind regards Uffe
