[PATCH 00/11] mmc: core: PM fixes/improvements for SDIO IRQs

[PATCH 00/11] mmc: core: PM fixes/improvements for SDIO IRQs

[Ulf Hansson]

The power management support for SDIO cards have slowly been improved, but
there are still quite some serious problems, especially when dealing with the
so called SDIO IRQs during system suspend/resume.

This series makes some additional improvements to this code in the mmc core,
but also includes some needed adaptations for the sdhci, the dw_mmc and the
mtk-sd host drivers.

So far the series has only been compile tested, so definitely need some help in
testing this on HW, which of course would be greatly appreciated.

The series is also available at:
git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git sdio_irq_suspend_next

Kind regards
Uffe


Matthias Kaehlcke (1):
  mmc: core: Move code to get pending SDIO IRQs to a function

Ulf Hansson (10):
  mmc: core: Add helper function to indicate if SDIO IRQs is enabled
  mmc: dw_mmc: Re-store SDIO IRQs mask at system resume
  mmc: mtk-sd: Re-store SDIO IRQs mask at system resume
  mmc: core: Clarify sdio_irq_pending flag for
    MMC_CAP2_SDIO_IRQ_NOTHREAD
  mmc: core: Clarify that the ->ack_sdio_irq() callback is mandatory
  mmc: core: WARN if SDIO IRQs are enabled for non-powered card in
    suspend
  mmc: core: Fixup processing of SDIO IRQs during system suspend/resume
  mmc: sdhci: Drop redundant check in sdhci_ack_sdio_irq()
  mmc: sdhci: Drop redundant code for SDIO IRQs
  mmc: sdhci: Convert to use sdio_irq_enabled()

 drivers/mmc/core/sdio.c            |  4 ++-
 drivers/mmc/core/sdio_irq.c        | 57 +++++++++++++++++++-----------
 drivers/mmc/host/dw_mmc.c          |  4 +++
 drivers/mmc/host/mtk-sd.c          |  3 ++
 drivers/mmc/host/sdhci-esdhc-imx.c | 34 ++++++++----------
 drivers/mmc/host/sdhci.c           | 12 ++-----
 drivers/mmc/host/sdhci.h           |  6 ----
 include/linux/mmc/host.h           | 10 ++++++
 8 files changed, 75 insertions(+), 55 deletions(-)

-- 
2.17.1

[PATCH 01/11] mmc: core: Add helper function to indicate if SDIO IRQs is enabled

[Ulf Hansson]

To avoid each host driver supporting SDIO IRQs, from keeping track
internally about if SDIO IRQs has been enabled, let's introduce a common
helper function, sdio_irq_enabled().

The function returns true if SDIO IRQs are enabled, via using the
information about the number of claimed irqs. This is safe, even without
any locks, as long as the helper function is called only from
runtime/system suspend callbacks of the host driver.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 include/linux/mmc/host.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 4a351cb7f20f..0c0a565c7ff1 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -493,6 +493,15 @@ void mmc_command_done(struct mmc_host *host, struct mmc_request *mrq);
 
 void mmc_cqe_request_done(struct mmc_host *host, struct mmc_request *mrq);
 
+/*
+ * May be called from host driver's system/runtime suspend/resume callbacks,
+ * to know if SDIO IRQs has been enabled.
+*/
+static inline bool sdio_irq_enabled(struct mmc_host *host)
+{
+	return host->sdio_irqs > 0;
+}
+
 static inline void mmc_signal_sdio_irq(struct mmc_host *host)
 {
 	host->ops->enable_sdio_irq(host, 0);
-- 
2.17.1

Re: [PATCH 01/11] mmc: core: Add helper function to indicate if SDIO IRQs is enabled

[Matthias Kaehlcke]

Hi Ulf,

On Tue, Sep 03, 2019 at 04:21:57PM +0200, Ulf Hansson wrote:
> To avoid each host driver supporting SDIO IRQs, from keeping track
> internally about if SDIO IRQs has been enabled, let's introduce a common
> helper function, sdio_irq_enabled().
> 
> The function returns true if SDIO IRQs are enabled, via using the
> information about the number of claimed irqs. This is safe, even without
> any locks, as long as the helper function is called only from
> runtime/system suspend callbacks of the host driver.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  include/linux/mmc/host.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 4a351cb7f20f..0c0a565c7ff1 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -493,6 +493,15 @@ void mmc_command_done(struct mmc_host *host, struct mmc_request *mrq);
>  
>  void mmc_cqe_request_done(struct mmc_host *host, struct mmc_request *mrq);
>  
> +/*
> + * May be called from host driver's system/runtime suspend/resume callbacks,
> + * to know if SDIO IRQs has been enabled.
> +*/
> +static inline bool sdio_irq_enabled(struct mmc_host *host)
> +{
> +	return host->sdio_irqs > 0;
> +}
> +

The name of the function is a bit misleadling, since it indicates
if SDIO IRQs should be enabled, not whether they are actually enabled
by the host. The resulting code can look a bit confusing to the
uninstructed reader:

  if (sdio_irq_enabled(host->slot->mmc))
    __dw_mci_enable_sdio_irq(host->slot, 1);

aka 'if SDIO IRQ is enabled, enable SDIO IRQ'.

sdio_irqs_claimed() could be a possible alternative.

No biggie though, just something I noticed.

Re: [PATCH 01/11] mmc: core: Add helper function to indicate if SDIO IRQs is enabled

[Ulf Hansson]

On Thu, 5 Sep 2019 at 01:58, Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Hi Ulf,
>
> On Tue, Sep 03, 2019 at 04:21:57PM +0200, Ulf Hansson wrote:
> > To avoid each host driver supporting SDIO IRQs, from keeping track
> > internally about if SDIO IRQs has been enabled, let's introduce a common
> > helper function, sdio_irq_enabled().
> >
> > The function returns true if SDIO IRQs are enabled, via using the
> > information about the number of claimed irqs. This is safe, even without
> > any locks, as long as the helper function is called only from
> > runtime/system suspend callbacks of the host driver.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  include/linux/mmc/host.h | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> > index 4a351cb7f20f..0c0a565c7ff1 100644
> > --- a/include/linux/mmc/host.h
> > +++ b/include/linux/mmc/host.h
> > @@ -493,6 +493,15 @@ void mmc_command_done(struct mmc_host *host, struct mmc_request *mrq);
> >
> >  void mmc_cqe_request_done(struct mmc_host *host, struct mmc_request *mrq);
> >
> > +/*
> > + * May be called from host driver's system/runtime suspend/resume callbacks,
> > + * to know if SDIO IRQs has been enabled.
> > +*/
> > +static inline bool sdio_irq_enabled(struct mmc_host *host)
> > +{
> > +     return host->sdio_irqs > 0;
> > +}
> > +
>
> The name of the function is a bit misleadling, since it indicates
> if SDIO IRQs should be enabled, not whether they are actually enabled
> by the host. The resulting code can look a bit confusing to the
> uninstructed reader:
>
>   if (sdio_irq_enabled(host->slot->mmc))
>     __dw_mci_enable_sdio_irq(host->slot, 1);
>
> aka 'if SDIO IRQ is enabled, enable SDIO IRQ'.
>
> sdio_irqs_claimed() could be a possible alternative.
>
> No biggie though, just something I noticed.

Thanks for the suggestions. It makes perfect sense to me, let me rename it.

Kind regards
Uffe

[PATCH 02/11] mmc: dw_mmc: Re-store SDIO IRQs mask at system resume

[Ulf Hansson]

In cases when SDIO IRQs have been enabled, runtime suspend is prevented by
the driver. However, this still means dw_mci_runtime_suspend|resume() gets
called during system suspend/resume, via pm_runtime_force_suspend|resume().
This means during system suspend/resume, the register context of the dw_mmc
device most likely loses its register context, even in cases when SDIO IRQs
have been enabled.

To re-enable the SDIO IRQs during system resume, the dw_mmc driver
currently relies on the mmc core to re-enable the SDIO IRQs when it resumes
the SDIO card, but this isn't the recommended solution. Instead, it's
better to deal with this locally in the dw_mmc driver, so let's do that.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/dw_mmc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index eea52e2c5a0c..f114710e82b4 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -3460,6 +3460,10 @@ int dw_mci_runtime_resume(struct device *dev)
 	/* Force setup bus to guarantee available clock output */
 	dw_mci_setup_bus(host->slot, true);
 
+	/* Re-enable SDIO interrupts. */
+	if (sdio_irq_enabled(host->slot->mmc))
+		__dw_mci_enable_sdio_irq(host->slot, 1);
+
 	/* Now that slots are all setup, we can enable card detect */
 	dw_mci_enable_cd(host);
 
-- 
2.17.1

Re: [PATCH 02/11] mmc: dw_mmc: Re-store SDIO IRQs mask at system resume

[Matthias Kaehlcke]

On Tue, Sep 03, 2019 at 04:21:58PM +0200, Ulf Hansson wrote:
> In cases when SDIO IRQs have been enabled, runtime suspend is prevented by
> the driver. However, this still means dw_mci_runtime_suspend|resume() gets
> called during system suspend/resume, via pm_runtime_force_suspend|resume().
> This means during system suspend/resume, the register context of the dw_mmc
> device most likely loses its register context, even in cases when SDIO IRQs
> have been enabled.
> 
> To re-enable the SDIO IRQs during system resume, the dw_mmc driver
> currently relies on the mmc core to re-enable the SDIO IRQs when it resumes
> the SDIO card, but this isn't the recommended solution. Instead, it's
> better to deal with this locally in the dw_mmc driver, so let's do that.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/host/dw_mmc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index eea52e2c5a0c..f114710e82b4 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -3460,6 +3460,10 @@ int dw_mci_runtime_resume(struct device *dev)
>  	/* Force setup bus to guarantee available clock output */
>  	dw_mci_setup_bus(host->slot, true);
>  
> +	/* Re-enable SDIO interrupts. */
> +	if (sdio_irq_enabled(host->slot->mmc))
> +		__dw_mci_enable_sdio_irq(host->slot, 1);
> +
>  	/* Now that slots are all setup, we can enable card detect */
>  	dw_mci_enable_cd(host);

Looks reasonable to me, besides the bikeshedding over
'sdio_irq_enabled' (in "mmc: core: Add helper function to indicate
if SDIO IRQs is enabled").

One thing I wonder is why this change is only needed for dw_mmc and
mtk-sd, but not for others like sunxi_mmc. Any insights for a SDIO
newb?

Re: [PATCH 02/11] mmc: dw_mmc: Re-store SDIO IRQs mask at system resume

[Ulf Hansson]

On Thu, 5 Sep 2019 at 02:14, Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Tue, Sep 03, 2019 at 04:21:58PM +0200, Ulf Hansson wrote:
> > In cases when SDIO IRQs have been enabled, runtime suspend is prevented by
> > the driver. However, this still means dw_mci_runtime_suspend|resume() gets
> > called during system suspend/resume, via pm_runtime_force_suspend|resume().
> > This means during system suspend/resume, the register context of the dw_mmc
> > device most likely loses its register context, even in cases when SDIO IRQs
> > have been enabled.
> >
> > To re-enable the SDIO IRQs during system resume, the dw_mmc driver
> > currently relies on the mmc core to re-enable the SDIO IRQs when it resumes
> > the SDIO card, but this isn't the recommended solution. Instead, it's
> > better to deal with this locally in the dw_mmc driver, so let's do that.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/mmc/host/dw_mmc.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > index eea52e2c5a0c..f114710e82b4 100644
> > --- a/drivers/mmc/host/dw_mmc.c
> > +++ b/drivers/mmc/host/dw_mmc.c
> > @@ -3460,6 +3460,10 @@ int dw_mci_runtime_resume(struct device *dev)
> >       /* Force setup bus to guarantee available clock output */
> >       dw_mci_setup_bus(host->slot, true);
> >
> > +     /* Re-enable SDIO interrupts. */
> > +     if (sdio_irq_enabled(host->slot->mmc))
> > +             __dw_mci_enable_sdio_irq(host->slot, 1);
> > +
> >       /* Now that slots are all setup, we can enable card detect */
> >       dw_mci_enable_cd(host);
>
> Looks reasonable to me, besides the bikeshedding over
> 'sdio_irq_enabled' (in "mmc: core: Add helper function to indicate
> if SDIO IRQs is enabled").
>
> One thing I wonder is why this change is only needed for dw_mmc and
> mtk-sd, but not for others like sunxi_mmc. Any insights for a SDIO
> newb?

mtk-sd and dw_mmc is using MMC_CAP2_SDIO_IRQ_NOTHREAD and
sdio_signal_irq(). This is also the case for sdhci, but sdhci is
already internally dealing restoring SDIO IRQs during system resume.

The other host drivers haven't yet converted to
MMC_CAP2_SDIO_IRQ_NOTHREAD. I have a series for that, not yet
completed and thus not ready to be posted. Once that happens, all host
drivers needs to care about re-enabling SDIO IRQs durings system
resume as well.

For those host that currently doesn't use MMC_CAP2_SDIO_IRQ_NOTHREAD,
the core wakes up the sdio_irq_thread from mmc_sdio_resume(), which
later will calls the ->enable_sdio_irq().

Perhaps I should add some information about this in the changelog, let
me think about it for the next version.

Kind regards
Uffe

Re: [PATCH 02/11] mmc: dw_mmc: Re-store SDIO IRQs mask at system resume

[Matthias Kaehlcke]

On Thu, Sep 05, 2019 at 09:29:04AM +0200, Ulf Hansson wrote:
> On Thu, 5 Sep 2019 at 02:14, Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > On Tue, Sep 03, 2019 at 04:21:58PM +0200, Ulf Hansson wrote:
> > > In cases when SDIO IRQs have been enabled, runtime suspend is prevented by
> > > the driver. However, this still means dw_mci_runtime_suspend|resume() gets
> > > called during system suspend/resume, via pm_runtime_force_suspend|resume().
> > > This means during system suspend/resume, the register context of the dw_mmc
> > > device most likely loses its register context, even in cases when SDIO IRQs
> > > have been enabled.
> > >
> > > To re-enable the SDIO IRQs during system resume, the dw_mmc driver
> > > currently relies on the mmc core to re-enable the SDIO IRQs when it resumes
> > > the SDIO card, but this isn't the recommended solution. Instead, it's
> > > better to deal with this locally in the dw_mmc driver, so let's do that.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/mmc/host/dw_mmc.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > > index eea52e2c5a0c..f114710e82b4 100644
> > > --- a/drivers/mmc/host/dw_mmc.c
> > > +++ b/drivers/mmc/host/dw_mmc.c
> > > @@ -3460,6 +3460,10 @@ int dw_mci_runtime_resume(struct device *dev)
> > >       /* Force setup bus to guarantee available clock output */
> > >       dw_mci_setup_bus(host->slot, true);
> > >
> > > +     /* Re-enable SDIO interrupts. */
> > > +     if (sdio_irq_enabled(host->slot->mmc))
> > > +             __dw_mci_enable_sdio_irq(host->slot, 1);
> > > +
> > >       /* Now that slots are all setup, we can enable card detect */
> > >       dw_mci_enable_cd(host);
> >
> > Looks reasonable to me, besides the bikeshedding over
> > 'sdio_irq_enabled' (in "mmc: core: Add helper function to indicate
> > if SDIO IRQs is enabled").
> >
> > One thing I wonder is why this change is only needed for dw_mmc and
> > mtk-sd, but not for others like sunxi_mmc. Any insights for a SDIO
> > newb?
> 
> mtk-sd and dw_mmc is using MMC_CAP2_SDIO_IRQ_NOTHREAD and
> sdio_signal_irq(). This is also the case for sdhci, but sdhci is
> already internally dealing restoring SDIO IRQs during system resume.
> 
> The other host drivers haven't yet converted to
> MMC_CAP2_SDIO_IRQ_NOTHREAD. I have a series for that, not yet
> completed and thus not ready to be posted. Once that happens, all host
> drivers needs to care about re-enabling SDIO IRQs durings system
> resume as well.
> 
> For those host that currently doesn't use MMC_CAP2_SDIO_IRQ_NOTHREAD,
> the core wakes up the sdio_irq_thread from mmc_sdio_resume(), which
> later will calls the ->enable_sdio_irq().
> 
> Perhaps I should add some information about this in the changelog, let
> me think about it for the next version.

It makes sense now, thanks for the clarification!

Re: [PATCH 02/11] mmc: dw_mmc: Re-store SDIO IRQs mask at system resume

[Doug Anderson]

Hi,

On Tue, Sep 3, 2019 at 7:22 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> In cases when SDIO IRQs have been enabled, runtime suspend is prevented by
> the driver. However, this still means dw_mci_runtime_suspend|resume() gets
> called during system suspend/resume, via pm_runtime_force_suspend|resume().
> This means during system suspend/resume, the register context of the dw_mmc
> device most likely loses its register context, even in cases when SDIO IRQs
> have been enabled.

Even if they weren't lost the resume code currently has this statement:

mci_writel(host, INTMASK, SDMMC_INT_CMD_DONE | SDMMC_INT_DATA_OVER |
   SDMMC_INT_TXDR | SDMMC_INT_RXDR |
   DW_MCI_ERROR_FLAGS);

...so that would have clobbered any existing state even if register
state wasn't lost.  ;-)

> To re-enable the SDIO IRQs during system resume, the dw_mmc driver
> currently relies on the mmc core to re-enable the SDIO IRQs when it resumes
> the SDIO card, but this isn't the recommended solution. Instead, it's
> better to deal with this locally in the dw_mmc driver, so let's do that.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/host/dw_mmc.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index eea52e2c5a0c..f114710e82b4 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -3460,6 +3460,10 @@ int dw_mci_runtime_resume(struct device *dev)
>         /* Force setup bus to guarantee available clock output */
>         dw_mci_setup_bus(host->slot, true);
>
> +       /* Re-enable SDIO interrupts. */
> +       if (sdio_irq_enabled(host->slot->mmc))
> +               __dw_mci_enable_sdio_irq(host->slot, 1);
> +

There's a slight bit of subtleness here and I guess we need to figure
out if it matters.  From testing things seem to work OK so maybe we're
fine, but just to explain what's bugging me:

If we got an SDIO interrupt that was never ACKed then this is going to
act like an implicit ACK.  Notice that dw_mci_ack_sdio_irq() is
exactly this call because when the SDIO IRQ fires we mask it out.
...then unmask when Acked.

Specifically after your series is applied, I think this is what
happens if an interrupt fires while the SDIO bus is officially
suspended:

1. dw_mci_interrupt() will get called which will mask the SDIO IRQ and
then call sdio_signal_irq()

2. sdio_signal_irq() will queue some delayed work.

3. The work will call sdio_run_irqs()

4. sdio_run_irqs() _won't_ ack the IRQ, so it will stay disabled.

5. When we get to the resume we'll re-enable the interrupt.

I guess that's fine, but it is a little weird that we might not really
be restoring it simply because it got disabled due to the clobbering
of INTMASK but also because we implicitly skipped Acking an interrupt
that fired.


I wonder if the correct fix is to just add an explit zeroing of the
INTMASK (so mask all interrupts) in dw_mmc's suspend callback.  Then
there's no possible way we could get an interrupt during suspend...



-Doug

Re: [PATCH 02/11] mmc: dw_mmc: Re-store SDIO IRQs mask at system resume

[Ulf Hansson]

On Fri, 6 Sep 2019 at 01:47, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Sep 3, 2019 at 7:22 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > In cases when SDIO IRQs have been enabled, runtime suspend is prevented by
> > the driver. However, this still means dw_mci_runtime_suspend|resume() gets
> > called during system suspend/resume, via pm_runtime_force_suspend|resume().
> > This means during system suspend/resume, the register context of the dw_mmc
> > device most likely loses its register context, even in cases when SDIO IRQs
> > have been enabled.
>
> Even if they weren't lost the resume code currently has this statement:
>
> mci_writel(host, INTMASK, SDMMC_INT_CMD_DONE | SDMMC_INT_DATA_OVER |
>    SDMMC_INT_TXDR | SDMMC_INT_RXDR |
>    DW_MCI_ERROR_FLAGS);
>
> ...so that would have clobbered any existing state even if register
> state wasn't lost.  ;-)
>
> > To re-enable the SDIO IRQs during system resume, the dw_mmc driver
> > currently relies on the mmc core to re-enable the SDIO IRQs when it resumes
> > the SDIO card, but this isn't the recommended solution. Instead, it's
> > better to deal with this locally in the dw_mmc driver, so let's do that.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/mmc/host/dw_mmc.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > index eea52e2c5a0c..f114710e82b4 100644
> > --- a/drivers/mmc/host/dw_mmc.c
> > +++ b/drivers/mmc/host/dw_mmc.c
> > @@ -3460,6 +3460,10 @@ int dw_mci_runtime_resume(struct device *dev)
> >         /* Force setup bus to guarantee available clock output */
> >         dw_mci_setup_bus(host->slot, true);
> >
> > +       /* Re-enable SDIO interrupts. */
> > +       if (sdio_irq_enabled(host->slot->mmc))
> > +               __dw_mci_enable_sdio_irq(host->slot, 1);
> > +
>
> There's a slight bit of subtleness here and I guess we need to figure
> out if it matters.  From testing things seem to work OK so maybe we're
> fine, but just to explain what's bugging me:
>
> If we got an SDIO interrupt that was never ACKed then this is going to
> act like an implicit ACK.  Notice that dw_mci_ack_sdio_irq() is
> exactly this call because when the SDIO IRQ fires we mask it out.
> ...then unmask when Acked.
>
> Specifically after your series is applied, I think this is what
> happens if an interrupt fires while the SDIO bus is officially
> suspended:
>
> 1. dw_mci_interrupt() will get called which will mask the SDIO IRQ and
> then call sdio_signal_irq()
>
> 2. sdio_signal_irq() will queue some delayed work.
>
> 3. The work will call sdio_run_irqs()
>
> 4. sdio_run_irqs() _won't_ ack the IRQ, so it will stay disabled.
>
> 5. When we get to the resume we'll re-enable the interrupt.

Correct.

>
> I guess that's fine, but it is a little weird that we might not really
> be restoring it simply because it got disabled due to the clobbering
> of INTMASK but also because we implicitly skipped Acking an interrupt
> that fired.

Let me comment on that, because there is actually two cases that are
relevant here to be covered.

1. After the SDIO card has been system suspended, sdio_run_irqs()
doesn't call the ->ack_sdio_irq() callback, as to prevents the host
driver from re-enabling the SDIO irq (acking). This is to avoid the
host from re-signalling the same SDIO IRQ over and over again when the
SDIO card is suspended.

2. Dealing with the SDIO IRQ bit-mask when the host driver system
suspends/resumes. This is host specific, but a common behavior is that
the driver can't allow any IRQ to be managed by its IRQ handler in a
suspended state. This is because the device (MMC controller) may be
put into a low power state (no clocks enabled, register context is
lost and not accessible, etc), which makes the device non-functional.

>
>
> I wonder if the correct fix is to just add an explit zeroing of the
> INTMASK (so mask all interrupts) in dw_mmc's suspend callback.  Then
> there's no possible way we could get an interrupt during suspend...

Exactly. Other host drivers do this as well.

Although note that the host device gets system suspended after the
sdio card device, so there is still a window when an SDIO IRQ can be
signaled. This is covered by 1) above.

Also note that, in general it also depends on whether there is wakeup
IRQ configured and how that wakeup might be handled. This is another
story, which doesn't seem relevant for dw_mmc, at least not at this
point.

Kind regards
Uffe

Re: [PATCH 02/11] mmc: dw_mmc: Re-store SDIO IRQs mask at system resume

[Doug Anderson]

Hi,

On Fri, Sep 6, 2019 at 2:20 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 6 Sep 2019 at 01:47, Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Tue, Sep 3, 2019 at 7:22 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > In cases when SDIO IRQs have been enabled, runtime suspend is prevented by
> > > the driver. However, this still means dw_mci_runtime_suspend|resume() gets
> > > called during system suspend/resume, via pm_runtime_force_suspend|resume().
> > > This means during system suspend/resume, the register context of the dw_mmc
> > > device most likely loses its register context, even in cases when SDIO IRQs
> > > have been enabled.
> >
> > Even if they weren't lost the resume code currently has this statement:
> >
> > mci_writel(host, INTMASK, SDMMC_INT_CMD_DONE | SDMMC_INT_DATA_OVER |
> >    SDMMC_INT_TXDR | SDMMC_INT_RXDR |
> >    DW_MCI_ERROR_FLAGS);
> >
> > ...so that would have clobbered any existing state even if register
> > state wasn't lost.  ;-)
> >
> > > To re-enable the SDIO IRQs during system resume, the dw_mmc driver
> > > currently relies on the mmc core to re-enable the SDIO IRQs when it resumes
> > > the SDIO card, but this isn't the recommended solution. Instead, it's
> > > better to deal with this locally in the dw_mmc driver, so let's do that.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/mmc/host/dw_mmc.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > > index eea52e2c5a0c..f114710e82b4 100644
> > > --- a/drivers/mmc/host/dw_mmc.c
> > > +++ b/drivers/mmc/host/dw_mmc.c
> > > @@ -3460,6 +3460,10 @@ int dw_mci_runtime_resume(struct device *dev)
> > >         /* Force setup bus to guarantee available clock output */
> > >         dw_mci_setup_bus(host->slot, true);
> > >
> > > +       /* Re-enable SDIO interrupts. */
> > > +       if (sdio_irq_enabled(host->slot->mmc))
> > > +               __dw_mci_enable_sdio_irq(host->slot, 1);
> > > +
> >
> > There's a slight bit of subtleness here and I guess we need to figure
> > out if it matters.  From testing things seem to work OK so maybe we're
> > fine, but just to explain what's bugging me:
> >
> > If we got an SDIO interrupt that was never ACKed then this is going to
> > act like an implicit ACK.  Notice that dw_mci_ack_sdio_irq() is
> > exactly this call because when the SDIO IRQ fires we mask it out.
> > ...then unmask when Acked.
> >
> > Specifically after your series is applied, I think this is what
> > happens if an interrupt fires while the SDIO bus is officially
> > suspended:
> >
> > 1. dw_mci_interrupt() will get called which will mask the SDIO IRQ and
> > then call sdio_signal_irq()
> >
> > 2. sdio_signal_irq() will queue some delayed work.
> >
> > 3. The work will call sdio_run_irqs()
> >
> > 4. sdio_run_irqs() _won't_ ack the IRQ, so it will stay disabled.
> >
> > 5. When we get to the resume we'll re-enable the interrupt.
>
> Correct.
>
> >
> > I guess that's fine, but it is a little weird that we might not really
> > be restoring it simply because it got disabled due to the clobbering
> > of INTMASK but also because we implicitly skipped Acking an interrupt
> > that fired.
>
> Let me comment on that, because there is actually two cases that are
> relevant here to be covered.
>
> 1. After the SDIO card has been system suspended, sdio_run_irqs()
> doesn't call the ->ack_sdio_irq() callback, as to prevents the host
> driver from re-enabling the SDIO irq (acking). This is to avoid the
> host from re-signalling the same SDIO IRQ over and over again when the
> SDIO card is suspended.
>
> 2. Dealing with the SDIO IRQ bit-mask when the host driver system
> suspends/resumes. This is host specific, but a common behavior is that
> the driver can't allow any IRQ to be managed by its IRQ handler in a
> suspended state. This is because the device (MMC controller) may be
> put into a low power state (no clocks enabled, register context is
> lost and not accessible, etc), which makes the device non-functional.

Yeah, if you look at dw_mci_runtime_suspend() you can actually see
that (at least in many cases) we actually disable the "BIU" clock.  I
believe this fully turns the clock off the controller and it can't
fire interrupts.

If I remember correctly the problem I actually saw before was that the
moment we turned the BIU back on in resume the SDIO interrupt fired.
:-P


> > I wonder if the correct fix is to just add an explit zeroing of the
> > INTMASK (so mask all interrupts) in dw_mmc's suspend callback.  Then
> > there's no possible way we could get an interrupt during suspend...
>
> Exactly. Other host drivers do this as well.
>
> Although note that the host device gets system suspended after the
> sdio card device, so there is still a window when an SDIO IRQ can be
> signaled. This is covered by 1) above.
>
> Also note that, in general it also depends on whether there is wakeup
> IRQ configured and how that wakeup might be handled. This is another
> story, which doesn't seem relevant for dw_mmc, at least not at this
> point.

I guess for now things will work OK so we can leave things as they
are.  If I see problems I can try masking all the interrupts at
suspend time, making sure that I don't mess up runtime suspend in
cases where we have a removable card using the builtin CD...

Thanks!

-Doug

[PATCH 03/11] mmc: mtk-sd: Re-store SDIO IRQs mask at system resume

[Ulf Hansson]

In cases when SDIO IRQs have been enabled, runtime suspend is prevented by
the driver. However, this still means msdc_runtime_suspend|resume() gets
called during system suspend/resume, via pm_runtime_force_suspend|resume().

This means during system suspend/resume, the register context of the mtk-sd
device most likely loses its register context, even in cases when SDIO IRQs
have been enabled.

To re-enable the SDIO IRQs during system resume, the mtk-sd driver
currently relies on the mmc core to re-enable the SDIO IRQs when it resumes
the SDIO card, but this isn't the recommended solution. Instead, it's
better to deal with this locally in the mtk-sd driver, so let's do that.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/mtk-sd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index 6946bb040a28..669ea0668159 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c
@@ -2408,6 +2408,9 @@ static void msdc_save_reg(struct msdc_host *host)
 	} else {
 		host->save_para.pad_tune = readl(host->base + tune_reg);
 	}
+
+	if (sdio_irq_enabled(host->mmc))
+		__msdc_enable_sdio_irq(host, 1);
 }
 
 static void msdc_restore_reg(struct msdc_host *host)
-- 
2.17.1

[PATCH 04/11] mmc: core: Move code to get pending SDIO IRQs to a function

[Ulf Hansson]

From: Matthias Kaehlcke <mka@chromium.org>

To improve code quality, let's move the code that gets pending SDIO IRQs
from process_sdio_pending_irqs() into a dedicated function.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
[Ulf: Converted function into static]
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio_irq.c | 46 ++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index 0bcc5e83bd1a..f75043266984 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -27,6 +27,34 @@
 #include "core.h"
 #include "card.h"
 
+static int sdio_get_pending_irqs(struct mmc_host *host, u8 *pending)
+{
+	struct mmc_card *card = host->card;
+	int ret;
+
+	WARN_ON(!host->claimed);
+
+	ret = mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_INTx, 0, pending);
+	if (ret) {
+		pr_debug("%s: error %d reading SDIO_CCCR_INTx\n",
+		       mmc_card_id(card), ret);
+		return ret;
+	}
+
+	if (*pending && mmc_card_broken_irq_polling(card) &&
+	    !(host->caps & MMC_CAP_SDIO_IRQ)) {
+		unsigned char dummy;
+
+		/* A fake interrupt could be created when we poll SDIO_CCCR_INTx
+		 * register with a Marvell SD8797 card. A dummy CMD52 read to
+		 * function 0 register 0xff can avoid this.
+		 */
+		mmc_io_rw_direct(card, 0, 0, 0xff, 0, &dummy);
+	}
+
+	return 0;
+}
+
 static int process_sdio_pending_irqs(struct mmc_host *host)
 {
 	struct mmc_card *card = host->card;
@@ -49,23 +77,9 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
 		return 1;
 	}
 
-	ret = mmc_io_rw_direct(card, 0, 0, SDIO_CCCR_INTx, 0, &pending);
-	if (ret) {
-		pr_debug("%s: error %d reading SDIO_CCCR_INTx\n",
-		       mmc_card_id(card), ret);
+	ret = sdio_get_pending_irqs(host, &pending);
+	if (ret)
 		return ret;
-	}
-
-	if (pending && mmc_card_broken_irq_polling(card) &&
-	    !(host->caps & MMC_CAP_SDIO_IRQ)) {
-		unsigned char dummy;
-
-		/* A fake interrupt could be created when we poll SDIO_CCCR_INTx
-		 * register with a Marvell SD8797 card. A dummy CMD52 read to
-		 * function 0 register 0xff can avoid this.
-		 */
-		mmc_io_rw_direct(card, 0, 0, 0xff, 0, &dummy);
-	}
 
 	count = 0;
 	for (i = 1; i <= 7; i++) {
-- 
2.17.1

[PATCH 05/11] mmc: core: Clarify sdio_irq_pending flag for MMC_CAP2_SDIO_IRQ_NOTHREAD

[Ulf Hansson]

In the single SDIO IRQ handler case, the sdio_irq_pending flag is used to
avoid reading the SDIO_CCCR_INTx register and instead immediately call the
SDIO func's >irq_handler() callback.

To clarify the use behind the flag for the MMC_CAP2_SDIO_IRQ_NOTHREAD case,
let's set the flag from inside sdio_signal_irq(), rather from
sdio_run_irqs(). Moreover, let's also reset the flag when the SDIO IRQ have
been properly processed.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio_irq.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index f75043266984..0962a4357d54 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -59,6 +59,7 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
 {
 	struct mmc_card *card = host->card;
 	int i, ret, count;
+	bool sdio_irq_pending = host->sdio_irq_pending;
 	unsigned char pending;
 	struct sdio_func *func;
 
@@ -66,13 +67,16 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
 	if (mmc_card_suspended(card))
 		return 0;
 
+	/* Clear the flag to indicate that we have processed the IRQ. */
+	host->sdio_irq_pending = false;
+
 	/*
 	 * Optimization, if there is only 1 function interrupt registered
 	 * and we know an IRQ was signaled then call irq handler directly.
 	 * Otherwise do the full probe.
 	 */
 	func = card->sdio_single_irq;
-	if (func && host->sdio_irq_pending) {
+	if (func && sdio_irq_pending) {
 		func->irq_handler(func);
 		return 1;
 	}
@@ -110,7 +114,6 @@ static void sdio_run_irqs(struct mmc_host *host)
 {
 	mmc_claim_host(host);
 	if (host->sdio_irqs) {
-		host->sdio_irq_pending = true;
 		process_sdio_pending_irqs(host);
 		if (host->ops->ack_sdio_irq)
 			host->ops->ack_sdio_irq(host);
@@ -128,6 +131,7 @@ void sdio_irq_work(struct work_struct *work)
 
 void sdio_signal_irq(struct mmc_host *host)
 {
+	host->sdio_irq_pending = true;
 	queue_delayed_work(system_wq, &host->sdio_irq_work, 0);
 }
 EXPORT_SYMBOL_GPL(sdio_signal_irq);
@@ -173,7 +177,6 @@ static int sdio_irq_thread(void *_host)
 		if (ret)
 			break;
 		ret = process_sdio_pending_irqs(host);
-		host->sdio_irq_pending = false;
 		mmc_release_host(host);
 
 		/*
-- 
2.17.1

Re: [PATCH 05/11] mmc: core: Clarify sdio_irq_pending flag for MMC_CAP2_SDIO_IRQ_NOTHREAD

[Matthias Kaehlcke]

On Tue, Sep 03, 2019 at 04:22:01PM +0200, Ulf Hansson wrote:
> In the single SDIO IRQ handler case, the sdio_irq_pending flag is used to
> avoid reading the SDIO_CCCR_INTx register and instead immediately call the
> SDIO func's >irq_handler() callback.
> 
> To clarify the use behind the flag for the MMC_CAP2_SDIO_IRQ_NOTHREAD case,
> let's set the flag from inside sdio_signal_irq(), rather from
> sdio_run_irqs(). Moreover, let's also reset the flag when the SDIO IRQ have
> been properly processed.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/sdio_irq.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
> index f75043266984..0962a4357d54 100644
> --- a/drivers/mmc/core/sdio_irq.c
> +++ b/drivers/mmc/core/sdio_irq.c
> @@ -59,6 +59,7 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
>  {
>  	struct mmc_card *card = host->card;
>  	int i, ret, count;
> +	bool sdio_irq_pending = host->sdio_irq_pending;
>  	unsigned char pending;
>  	struct sdio_func *func;
>  
> @@ -66,13 +67,16 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
>  	if (mmc_card_suspended(card))
>  		return 0;
>  
> +	/* Clear the flag to indicate that we have processed the IRQ. */
> +	host->sdio_irq_pending = false;
> +

It's not entirely true that we have processed the IRQ,
the sdio_get_pending_irqs() below could fail and we'd return. However
I guess if it comes to that we are in a pretty bad shape already and
the value of the flag doesn't really matter.

>  	/*
>  	 * Optimization, if there is only 1 function interrupt registered
>  	 * and we know an IRQ was signaled then call irq handler directly.
>  	 * Otherwise do the full probe.
>  	 */
>  	func = card->sdio_single_irq;
> -	if (func && host->sdio_irq_pending) {
> +	if (func && sdio_irq_pending) {
>  		func->irq_handler(func);
>  		return 1;
>  	}
> @@ -110,7 +114,6 @@ static void sdio_run_irqs(struct mmc_host *host)
>  {
>  	mmc_claim_host(host);
>  	if (host->sdio_irqs) {
> -		host->sdio_irq_pending = true;
>  		process_sdio_pending_irqs(host);
>  		if (host->ops->ack_sdio_irq)
>  			host->ops->ack_sdio_irq(host);
> @@ -128,6 +131,7 @@ void sdio_irq_work(struct work_struct *work)
>  
>  void sdio_signal_irq(struct mmc_host *host)
>  {
> +	host->sdio_irq_pending = true;
>  	queue_delayed_work(system_wq, &host->sdio_irq_work, 0);
>  }
>  EXPORT_SYMBOL_GPL(sdio_signal_irq);
> @@ -173,7 +177,6 @@ static int sdio_irq_thread(void *_host)
>  		if (ret)
>  			break;
>  		ret = process_sdio_pending_irqs(host);
> -		host->sdio_irq_pending = false;
>  		mmc_release_host(host);
>  
>  		/*

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH 05/11] mmc: core: Clarify sdio_irq_pending flag for MMC_CAP2_SDIO_IRQ_NOTHREAD

[Ulf Hansson]

On Thu, 5 Sep 2019 at 02:34, Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Tue, Sep 03, 2019 at 04:22:01PM +0200, Ulf Hansson wrote:
> > In the single SDIO IRQ handler case, the sdio_irq_pending flag is used to
> > avoid reading the SDIO_CCCR_INTx register and instead immediately call the
> > SDIO func's >irq_handler() callback.
> >
> > To clarify the use behind the flag for the MMC_CAP2_SDIO_IRQ_NOTHREAD case,
> > let's set the flag from inside sdio_signal_irq(), rather from
> > sdio_run_irqs(). Moreover, let's also reset the flag when the SDIO IRQ have
> > been properly processed.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/mmc/core/sdio_irq.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
> > index f75043266984..0962a4357d54 100644
> > --- a/drivers/mmc/core/sdio_irq.c
> > +++ b/drivers/mmc/core/sdio_irq.c
> > @@ -59,6 +59,7 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
> >  {
> >       struct mmc_card *card = host->card;
> >       int i, ret, count;
> > +     bool sdio_irq_pending = host->sdio_irq_pending;
> >       unsigned char pending;
> >       struct sdio_func *func;
> >
> > @@ -66,13 +67,16 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
> >       if (mmc_card_suspended(card))
> >               return 0;
> >
> > +     /* Clear the flag to indicate that we have processed the IRQ. */
> > +     host->sdio_irq_pending = false;
> > +
>
> It's not entirely true that we have processed the IRQ,
> the sdio_get_pending_irqs() below could fail and we'd return. However
> I guess if it comes to that we are in a pretty bad shape already and
> the value of the flag doesn't really matter.

Yes, that's my view as well.

>
> >       /*
> >        * Optimization, if there is only 1 function interrupt registered
> >        * and we know an IRQ was signaled then call irq handler directly.
> >        * Otherwise do the full probe.
> >        */
> >       func = card->sdio_single_irq;
> > -     if (func && host->sdio_irq_pending) {
> > +     if (func && sdio_irq_pending) {
> >               func->irq_handler(func);
> >               return 1;
> >       }
> > @@ -110,7 +114,6 @@ static void sdio_run_irqs(struct mmc_host *host)
> >  {
> >       mmc_claim_host(host);
> >       if (host->sdio_irqs) {
> > -             host->sdio_irq_pending = true;
> >               process_sdio_pending_irqs(host);
> >               if (host->ops->ack_sdio_irq)
> >                       host->ops->ack_sdio_irq(host);
> > @@ -128,6 +131,7 @@ void sdio_irq_work(struct work_struct *work)
> >
> >  void sdio_signal_irq(struct mmc_host *host)
> >  {
> > +     host->sdio_irq_pending = true;
> >       queue_delayed_work(system_wq, &host->sdio_irq_work, 0);
> >  }
> >  EXPORT_SYMBOL_GPL(sdio_signal_irq);
> > @@ -173,7 +177,6 @@ static int sdio_irq_thread(void *_host)
> >               if (ret)
> >                       break;
> >               ret = process_sdio_pending_irqs(host);
> > -             host->sdio_irq_pending = false;
> >               mmc_release_host(host);
> >
> >               /*
>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Thanks!

Kind regards
Uffe

Re: [PATCH 05/11] mmc: core: Clarify sdio_irq_pending flag for MMC_CAP2_SDIO_IRQ_NOTHREAD

[Doug Anderson]

Hi,

On Tue, Sep 3, 2019 at 7:22 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> In the single SDIO IRQ handler case, the sdio_irq_pending flag is used to
> avoid reading the SDIO_CCCR_INTx register and instead immediately call the
> SDIO func's >irq_handler() callback.
>
> To clarify the use behind the flag for the MMC_CAP2_SDIO_IRQ_NOTHREAD case,
> let's set the flag from inside sdio_signal_irq(), rather from
> sdio_run_irqs().

I'm having a hard time parsing the above statement...  Can you reword
and maybe I'll understand?


> Moreover, let's also reset the flag when the SDIO IRQ have
> been properly processed.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/sdio_irq.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Nice!  This looks like it addresses some of the things that came up in
the previous discussion [1] and should be a nice improvement.  From
re-reading that discussion that will probably change the behvaior
slightly (hopefully for the better) in the single-function case where
we might actually poll CCCR_INTx sometimes now.


> diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
> index f75043266984..0962a4357d54 100644
> --- a/drivers/mmc/core/sdio_irq.c
> +++ b/drivers/mmc/core/sdio_irq.c
> @@ -59,6 +59,7 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
>  {
>         struct mmc_card *card = host->card;
>         int i, ret, count;
> +       bool sdio_irq_pending = host->sdio_irq_pending;
>         unsigned char pending;
>         struct sdio_func *func;
>
> @@ -66,13 +67,16 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
>         if (mmc_card_suspended(card))
>                 return 0;
>
> +       /* Clear the flag to indicate that we have processed the IRQ. */
> +       host->sdio_irq_pending = false;
> +
>         /*
>          * Optimization, if there is only 1 function interrupt registered
>          * and we know an IRQ was signaled then call irq handler directly.
>          * Otherwise do the full probe.
>          */
>         func = card->sdio_single_irq;
> -       if (func && host->sdio_irq_pending) {
> +       if (func && sdio_irq_pending) {
>                 func->irq_handler(func);
>                 return 1;
>         }
> @@ -110,7 +114,6 @@ static void sdio_run_irqs(struct mmc_host *host)
>  {
>         mmc_claim_host(host);
>         if (host->sdio_irqs) {
> -               host->sdio_irq_pending = true;
>                 process_sdio_pending_irqs(host);
>                 if (host->ops->ack_sdio_irq)
>                         host->ops->ack_sdio_irq(host);
> @@ -128,6 +131,7 @@ void sdio_irq_work(struct work_struct *work)
>
>  void sdio_signal_irq(struct mmc_host *host)
>  {
> +       host->sdio_irq_pending = true;

Is this safe to do without claiming the host or any other type of
locking?  sdio_signal_irq() is called directly from the interrupt
handler on dw_mmc with no locks held at all.  Could we have races /
problems with weakly ordered memory?

Maybe I'm not understanding why this has to move.  It seems like it
would have been fine to leave this part in sdio_run_irqs() where it
was...


[1] https://lore.kernel.org/r/CAD=FV=XBVRsdiOD0vhgTvMXmqm=fzy9Bzd_x=E1TNPBsT_D-tQ@mail.gmail.com

-Doug

>         queue_delayed_work(system_wq, &host->sdio_irq_work, 0);
>  }
>  EXPORT_SYMBOL_GPL(sdio_signal_irq);
> @@ -173,7 +177,6 @@ static int sdio_irq_thread(void *_host)
>                 if (ret)
>                         break;
>                 ret = process_sdio_pending_irqs(host);
> -               host->sdio_irq_pending = false;
>                 mmc_release_host(host);
>
>                 /*
> --
> 2.17.1
>

Re: [PATCH 05/11] mmc: core: Clarify sdio_irq_pending flag for MMC_CAP2_SDIO_IRQ_NOTHREAD

[Ulf Hansson]

On Fri, 6 Sep 2019 at 01:47, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Sep 3, 2019 at 7:22 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > In the single SDIO IRQ handler case, the sdio_irq_pending flag is used to
> > avoid reading the SDIO_CCCR_INTx register and instead immediately call the
> > SDIO func's >irq_handler() callback.
> >
> > To clarify the use behind the flag for the MMC_CAP2_SDIO_IRQ_NOTHREAD case,
> > let's set the flag from inside sdio_signal_irq(), rather from
> > sdio_run_irqs().
>
> I'm having a hard time parsing the above statement...  Can you reword
> and maybe I'll understand?

Sure, I admit, it's not very good. :-) How about the below.

The sdio_irq_pending flag is used to let host drivers indicate that it
has signaled an IRQ. If that is the case and we only have a single
SDIO func that have claimed an SDIO IRQ, our assumption is that we can
avoid reading the SDIO_CCCR_INTx register and just call the SDIO func
irq handler immediately. This makes sense, but the flag is set/cleared
in a somewhat messy order, let's fix that up according to below.

First, the flag is currently set in sdio_run_irqs(), which is executed
as a work that was scheduled from sdio_signal_irq(). To make it more
implicit that the host have signaled an IRQ, let's instead immediately
set the flag in sdio_signal_irq(). This also makes the behavior
consistent with host drivers that uses the legacy,
mmc_signal_sdio_irq() API. This have no functional impact, because we
don't expect host drivers to call sdio_signal_irq() until after the
work (sdio_run_irqs()) have been executed anyways.

Second, currently we never clears the flag when using the
sdio_run_irqs() work, but only when using the sdio_irq_thread(). Let
make the behavior consistent, by moving the flag to be cleared inside
the common process_sdio_pending_irqs() function. Additionally, tweak
the behavior of the flag slightly, by avoiding to clear it unless we
processed the SDIO IRQ. The purpose with this at this point, is to
keep the information about whether there have been an SDIO IRQ
signaled by the host, so at system resume we can decide to process it
without reading the SDIO_CCCR_INTx register.

>
>
> > Moreover, let's also reset the flag when the SDIO IRQ have
> > been properly processed.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/mmc/core/sdio_irq.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
>
> Nice!  This looks like it addresses some of the things that came up in
> the previous discussion [1] and should be a nice improvement.  From
> re-reading that discussion that will probably change the behvaior
> slightly (hopefully for the better) in the single-function case where
> we might actually poll CCCR_INTx sometimes now.

Correct!

>
>
> > diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
> > index f75043266984..0962a4357d54 100644
> > --- a/drivers/mmc/core/sdio_irq.c
> > +++ b/drivers/mmc/core/sdio_irq.c
> > @@ -59,6 +59,7 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
> >  {
> >         struct mmc_card *card = host->card;
> >         int i, ret, count;
> > +       bool sdio_irq_pending = host->sdio_irq_pending;
> >         unsigned char pending;
> >         struct sdio_func *func;
> >
> > @@ -66,13 +67,16 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
> >         if (mmc_card_suspended(card))
> >                 return 0;
> >
> > +       /* Clear the flag to indicate that we have processed the IRQ. */
> > +       host->sdio_irq_pending = false;
> > +
> >         /*
> >          * Optimization, if there is only 1 function interrupt registered
> >          * and we know an IRQ was signaled then call irq handler directly.
> >          * Otherwise do the full probe.
> >          */
> >         func = card->sdio_single_irq;
> > -       if (func && host->sdio_irq_pending) {
> > +       if (func && sdio_irq_pending) {
> >                 func->irq_handler(func);
> >                 return 1;
> >         }
> > @@ -110,7 +114,6 @@ static void sdio_run_irqs(struct mmc_host *host)
> >  {
> >         mmc_claim_host(host);
> >         if (host->sdio_irqs) {
> > -               host->sdio_irq_pending = true;
> >                 process_sdio_pending_irqs(host);
> >                 if (host->ops->ack_sdio_irq)
> >                         host->ops->ack_sdio_irq(host);
> > @@ -128,6 +131,7 @@ void sdio_irq_work(struct work_struct *work)
> >
> >  void sdio_signal_irq(struct mmc_host *host)
> >  {
> > +       host->sdio_irq_pending = true;
>
> Is this safe to do without claiming the host or any other type of
> locking?  sdio_signal_irq() is called directly from the interrupt
> handler on dw_mmc with no locks held at all.  Could we have races /
> problems with weakly ordered memory?

At this point, for $subject patch and @subject series, I don't see any
issues. But perhaps when we go forward and start using the flag
slightly differently.

>
> Maybe I'm not understanding why this has to move.  It seems like it
> would have been fine to leave this part in sdio_run_irqs() where it
> was...

The changes later in the series relies on this change, as we start
making use of the flag to understand if we should ack an SDIO IRQ.

>
>
> [1] https://lore.kernel.org/r/CAD=FV=XBVRsdiOD0vhgTvMXmqm=fzy9Bzd_x=E1TNPBsT_D-tQ@mail.gmail.com
>
> -Doug
>
> >         queue_delayed_work(system_wq, &host->sdio_irq_work, 0);
> >  }
> >  EXPORT_SYMBOL_GPL(sdio_signal_irq);
> > @@ -173,7 +177,6 @@ static int sdio_irq_thread(void *_host)
> >                 if (ret)
> >                         break;
> >                 ret = process_sdio_pending_irqs(host);
> > -               host->sdio_irq_pending = false;
> >                 mmc_release_host(host);
> >
> >                 /*
> > --
> > 2.17.1
> >

Kind regards
Uffe

Re: [PATCH 05/11] mmc: core: Clarify sdio_irq_pending flag for MMC_CAP2_SDIO_IRQ_NOTHREAD

[Doug Anderson]

Hi,

On Fri, Sep 6, 2019 at 2:20 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 6 Sep 2019 at 01:47, Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Tue, Sep 3, 2019 at 7:22 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > In the single SDIO IRQ handler case, the sdio_irq_pending flag is used to
> > > avoid reading the SDIO_CCCR_INTx register and instead immediately call the
> > > SDIO func's >irq_handler() callback.
> > >
> > > To clarify the use behind the flag for the MMC_CAP2_SDIO_IRQ_NOTHREAD case,
> > > let's set the flag from inside sdio_signal_irq(), rather from
> > > sdio_run_irqs().
> >
> > I'm having a hard time parsing the above statement...  Can you reword
> > and maybe I'll understand?
>
> Sure, I admit, it's not very good. :-) How about the below.
>
> The sdio_irq_pending flag is used to let host drivers indicate that it
> has signaled an IRQ. If that is the case and we only have a single
> SDIO func that have claimed an SDIO IRQ, our assumption is that we can
> avoid reading the SDIO_CCCR_INTx register and just call the SDIO func
> irq handler immediately. This makes sense, but the flag is set/cleared
> in a somewhat messy order, let's fix that up according to below.
>
> First, the flag is currently set in sdio_run_irqs(), which is executed
> as a work that was scheduled from sdio_signal_irq(). To make it more
> implicit that the host have signaled an IRQ, let's instead immediately
> set the flag in sdio_signal_irq(). This also makes the behavior
> consistent with host drivers that uses the legacy,
> mmc_signal_sdio_irq() API. This have no functional impact, because we
> don't expect host drivers to call sdio_signal_irq() until after the
> work (sdio_run_irqs()) have been executed anyways.
>
> Second, currently we never clears the flag when using the
> sdio_run_irqs() work, but only when using the sdio_irq_thread(). Let
> make the behavior consistent, by moving the flag to be cleared inside
> the common process_sdio_pending_irqs() function. Additionally, tweak
> the behavior of the flag slightly, by avoiding to clear it unless we
> processed the SDIO IRQ. The purpose with this at this point, is to
> keep the information about whether there have been an SDIO IRQ
> signaled by the host, so at system resume we can decide to process it
> without reading the SDIO_CCCR_INTx register.
>
> >
> >
> > > Moreover, let's also reset the flag when the SDIO IRQ have
> > > been properly processed.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/mmc/core/sdio_irq.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > Nice!  This looks like it addresses some of the things that came up in
> > the previous discussion [1] and should be a nice improvement.  From
> > re-reading that discussion that will probably change the behvaior
> > slightly (hopefully for the better) in the single-function case where
> > we might actually poll CCCR_INTx sometimes now.
>
> Correct!
>
> >
> >
> > > diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
> > > index f75043266984..0962a4357d54 100644
> > > --- a/drivers/mmc/core/sdio_irq.c
> > > +++ b/drivers/mmc/core/sdio_irq.c
> > > @@ -59,6 +59,7 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
> > >  {
> > >         struct mmc_card *card = host->card;
> > >         int i, ret, count;
> > > +       bool sdio_irq_pending = host->sdio_irq_pending;
> > >         unsigned char pending;
> > >         struct sdio_func *func;
> > >
> > > @@ -66,13 +67,16 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
> > >         if (mmc_card_suspended(card))
> > >                 return 0;
> > >
> > > +       /* Clear the flag to indicate that we have processed the IRQ. */
> > > +       host->sdio_irq_pending = false;
> > > +
> > >         /*
> > >          * Optimization, if there is only 1 function interrupt registered
> > >          * and we know an IRQ was signaled then call irq handler directly.
> > >          * Otherwise do the full probe.
> > >          */
> > >         func = card->sdio_single_irq;
> > > -       if (func && host->sdio_irq_pending) {
> > > +       if (func && sdio_irq_pending) {
> > >                 func->irq_handler(func);
> > >                 return 1;
> > >         }
> > > @@ -110,7 +114,6 @@ static void sdio_run_irqs(struct mmc_host *host)
> > >  {
> > >         mmc_claim_host(host);
> > >         if (host->sdio_irqs) {
> > > -               host->sdio_irq_pending = true;
> > >                 process_sdio_pending_irqs(host);
> > >                 if (host->ops->ack_sdio_irq)
> > >                         host->ops->ack_sdio_irq(host);
> > > @@ -128,6 +131,7 @@ void sdio_irq_work(struct work_struct *work)
> > >
> > >  void sdio_signal_irq(struct mmc_host *host)
> > >  {
> > > +       host->sdio_irq_pending = true;
> >
> > Is this safe to do without claiming the host or any other type of
> > locking?  sdio_signal_irq() is called directly from the interrupt
> > handler on dw_mmc with no locks held at all.  Could we have races /
> > problems with weakly ordered memory?
>
> At this point, for $subject patch and @subject series, I don't see any
> issues. But perhaps when we go forward and start using the flag
> slightly differently.

Lockless concurrency always makes my head hurt (especially when I try
to consider weakly ordered memory) and I've learned that the only way
I can reason about it and have any belief that I got it right is to
always make sure I access values in a context where things are locked.
:-P

Let's see if I can figure out any actual problem, though...

Certainly the queue_delayed_work() would act as a barrier so you don't
have to worry about the worker not seeing the "= true".

I suppose it's definitely possible that (if the worker is already
running) that our "= true" will get clobbered by an "= false" from a
previous instance of the worker running.  I guess that's unlikely
because we can't get a second IRQ signaled until the "->ack_sdio_irq"
ran and presumably there's enough stuff after the "= false" that one
of them would have a barrier that made sure that the "= false" didn't
affect us in a delayed way.

So I guess we're fine...

-Doug

Re: [PATCH 05/11] mmc: core: Clarify sdio_irq_pending flag for MMC_CAP2_SDIO_IRQ_NOTHREAD

[Ulf Hansson]

On Fri, 6 Sep 2019 at 23:30, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Fri, Sep 6, 2019 at 2:20 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Fri, 6 Sep 2019 at 01:47, Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Sep 3, 2019 at 7:22 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > In the single SDIO IRQ handler case, the sdio_irq_pending flag is used to
> > > > avoid reading the SDIO_CCCR_INTx register and instead immediately call the
> > > > SDIO func's >irq_handler() callback.
> > > >
> > > > To clarify the use behind the flag for the MMC_CAP2_SDIO_IRQ_NOTHREAD case,
> > > > let's set the flag from inside sdio_signal_irq(), rather from
> > > > sdio_run_irqs().
> > >
> > > I'm having a hard time parsing the above statement...  Can you reword
> > > and maybe I'll understand?
> >
> > Sure, I admit, it's not very good. :-) How about the below.
> >
> > The sdio_irq_pending flag is used to let host drivers indicate that it
> > has signaled an IRQ. If that is the case and we only have a single
> > SDIO func that have claimed an SDIO IRQ, our assumption is that we can
> > avoid reading the SDIO_CCCR_INTx register and just call the SDIO func
> > irq handler immediately. This makes sense, but the flag is set/cleared
> > in a somewhat messy order, let's fix that up according to below.
> >
> > First, the flag is currently set in sdio_run_irqs(), which is executed
> > as a work that was scheduled from sdio_signal_irq(). To make it more
> > implicit that the host have signaled an IRQ, let's instead immediately
> > set the flag in sdio_signal_irq(). This also makes the behavior
> > consistent with host drivers that uses the legacy,
> > mmc_signal_sdio_irq() API. This have no functional impact, because we
> > don't expect host drivers to call sdio_signal_irq() until after the
> > work (sdio_run_irqs()) have been executed anyways.
> >
> > Second, currently we never clears the flag when using the
> > sdio_run_irqs() work, but only when using the sdio_irq_thread(). Let
> > make the behavior consistent, by moving the flag to be cleared inside
> > the common process_sdio_pending_irqs() function. Additionally, tweak
> > the behavior of the flag slightly, by avoiding to clear it unless we
> > processed the SDIO IRQ. The purpose with this at this point, is to
> > keep the information about whether there have been an SDIO IRQ
> > signaled by the host, so at system resume we can decide to process it
> > without reading the SDIO_CCCR_INTx register.
> >
> > >
> > >
> > > > Moreover, let's also reset the flag when the SDIO IRQ have
> > > > been properly processed.
> > > >
> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > ---
> > > >  drivers/mmc/core/sdio_irq.c | 9 ++++++---
> > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > Nice!  This looks like it addresses some of the things that came up in
> > > the previous discussion [1] and should be a nice improvement.  From
> > > re-reading that discussion that will probably change the behvaior
> > > slightly (hopefully for the better) in the single-function case where
> > > we might actually poll CCCR_INTx sometimes now.
> >
> > Correct!
> >
> > >
> > >
> > > > diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
> > > > index f75043266984..0962a4357d54 100644
> > > > --- a/drivers/mmc/core/sdio_irq.c
> > > > +++ b/drivers/mmc/core/sdio_irq.c
> > > > @@ -59,6 +59,7 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
> > > >  {
> > > >         struct mmc_card *card = host->card;
> > > >         int i, ret, count;
> > > > +       bool sdio_irq_pending = host->sdio_irq_pending;
> > > >         unsigned char pending;
> > > >         struct sdio_func *func;
> > > >
> > > > @@ -66,13 +67,16 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
> > > >         if (mmc_card_suspended(card))
> > > >                 return 0;
> > > >
> > > > +       /* Clear the flag to indicate that we have processed the IRQ. */
> > > > +       host->sdio_irq_pending = false;
> > > > +
> > > >         /*
> > > >          * Optimization, if there is only 1 function interrupt registered
> > > >          * and we know an IRQ was signaled then call irq handler directly.
> > > >          * Otherwise do the full probe.
> > > >          */
> > > >         func = card->sdio_single_irq;
> > > > -       if (func && host->sdio_irq_pending) {
> > > > +       if (func && sdio_irq_pending) {
> > > >                 func->irq_handler(func);
> > > >                 return 1;
> > > >         }
> > > > @@ -110,7 +114,6 @@ static void sdio_run_irqs(struct mmc_host *host)
> > > >  {
> > > >         mmc_claim_host(host);
> > > >         if (host->sdio_irqs) {
> > > > -               host->sdio_irq_pending = true;
> > > >                 process_sdio_pending_irqs(host);
> > > >                 if (host->ops->ack_sdio_irq)
> > > >                         host->ops->ack_sdio_irq(host);
> > > > @@ -128,6 +131,7 @@ void sdio_irq_work(struct work_struct *work)
> > > >
> > > >  void sdio_signal_irq(struct mmc_host *host)
> > > >  {
> > > > +       host->sdio_irq_pending = true;
> > >
> > > Is this safe to do without claiming the host or any other type of
> > > locking?  sdio_signal_irq() is called directly from the interrupt
> > > handler on dw_mmc with no locks held at all.  Could we have races /
> > > problems with weakly ordered memory?
> >
> > At this point, for $subject patch and @subject series, I don't see any
> > issues. But perhaps when we go forward and start using the flag
> > slightly differently.
>
> Lockless concurrency always makes my head hurt (especially when I try
> to consider weakly ordered memory) and I've learned that the only way
> I can reason about it and have any belief that I got it right is to
> always make sure I access values in a context where things are locked.
> :-P
>
> Let's see if I can figure out any actual problem, though...
>
> Certainly the queue_delayed_work() would act as a barrier so you don't
> have to worry about the worker not seeing the "= true".
>
> I suppose it's definitely possible that (if the worker is already
> running) that our "= true" will get clobbered by an "= false" from a
> previous instance of the worker running.  I guess that's unlikely
> because we can't get a second IRQ signaled until the "->ack_sdio_irq"
> ran and presumably there's enough stuff after the "= false" that one
> of them would have a barrier that made sure that the "= false" didn't
> affect us in a delayed way.
>
> So I guess we're fine...

Yeah, the trick is simply that we don't expect another IRQ being
signaled via sdio_signal_irq(), until the ->ack_sdio_irq() has been
invoked.

And even if that happens, the works case scenario would be that we
would skip the 1-func optimized pat and end up reading the poll
CCCR_INTx. This should be fine.

Kind regards
Uffe

[PATCH 06/11] mmc: core: Clarify that the ->ack_sdio_irq() callback is mandatory

[Ulf Hansson]

For the MMC_CAP2_SDIO_IRQ_NOTHREAD case and when using sdio_signal_irq(),
the ->ack_sdio_irq() is already mandatory, which was not the case for those
host drivers that called sdio_run_irqs() directly.

As there are no longer any drivers calling sdio_run_irqs(), let's clarify
the code by dropping the unnecessary check and explicitly state that the
callback is mandatory in the header file.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio_irq.c | 3 +--
 include/linux/mmc/host.h    | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index 0962a4357d54..d7965b53a6d2 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -115,8 +115,7 @@ static void sdio_run_irqs(struct mmc_host *host)
 	mmc_claim_host(host);
 	if (host->sdio_irqs) {
 		process_sdio_pending_irqs(host);
-		if (host->ops->ack_sdio_irq)
-			host->ops->ack_sdio_irq(host);
+		host->ops->ack_sdio_irq(host);
 	}
 	mmc_release_host(host);
 }
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0c0a565c7ff1..ecdc1b0b1313 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -128,6 +128,7 @@ struct mmc_host_ops {
 	int	(*get_cd)(struct mmc_host *host);
 
 	void	(*enable_sdio_irq)(struct mmc_host *host, int enable);
+	/* Mandatory callback when using MMC_CAP2_SDIO_IRQ_NOTHREAD. */
 	void	(*ack_sdio_irq)(struct mmc_host *host);
 
 	/* optional callback for HC quirks */
-- 
2.17.1

Re: [PATCH 06/11] mmc: core: Clarify that the ->ack_sdio_irq() callback is mandatory

[Matthias Kaehlcke]

On Tue, Sep 03, 2019 at 04:22:02PM +0200, Ulf Hansson wrote:
> For the MMC_CAP2_SDIO_IRQ_NOTHREAD case and when using sdio_signal_irq(),
> the ->ack_sdio_irq() is already mandatory, which was not the case for those
> host drivers that called sdio_run_irqs() directly.
> 
> As there are no longer any drivers calling sdio_run_irqs(), let's clarify
> the code by dropping the unnecessary check and explicitly state that the
> callback is mandatory in the header file.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/sdio_irq.c | 3 +--
>  include/linux/mmc/host.h    | 1 +
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
> index 0962a4357d54..d7965b53a6d2 100644
> --- a/drivers/mmc/core/sdio_irq.c
> +++ b/drivers/mmc/core/sdio_irq.c
> @@ -115,8 +115,7 @@ static void sdio_run_irqs(struct mmc_host *host)
>  	mmc_claim_host(host);
>  	if (host->sdio_irqs) {
>  		process_sdio_pending_irqs(host);
> -		if (host->ops->ack_sdio_irq)
> -			host->ops->ack_sdio_irq(host);
> +		host->ops->ack_sdio_irq(host);
>  	}
>  	mmc_release_host(host);
>  }
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 0c0a565c7ff1..ecdc1b0b1313 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -128,6 +128,7 @@ struct mmc_host_ops {
>  	int	(*get_cd)(struct mmc_host *host);
>  
>  	void	(*enable_sdio_irq)(struct mmc_host *host, int enable);
> +	/* Mandatory callback when using MMC_CAP2_SDIO_IRQ_NOTHREAD. */
>  	void	(*ack_sdio_irq)(struct mmc_host *host);
>  
>  	/* optional callback for HC quirks */

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

[PATCH 07/11] mmc: core: WARN if SDIO IRQs are enabled for non-powered card in suspend

[Ulf Hansson]

To make sure SDIO func drivers behaves correctly during system
suspend/resume, let add a WARN_ON in case the condition is a non-powered
SDIO card and there are some SDIO IRQs still being claimed.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 8dd8fc32ecca..c557f1519b77 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -951,6 +951,8 @@ static int mmc_sdio_pre_suspend(struct mmc_host *host)
  */
 static int mmc_sdio_suspend(struct mmc_host *host)
 {
+	WARN_ON(host->sdio_irqs && !mmc_card_keep_power(host));
+
 	/* Prevent processing of SDIO IRQs in suspended state. */
 	mmc_card_set_suspended(host->card);
 	cancel_delayed_work_sync(&host->sdio_irq_work);
-- 
2.17.1

Re: [PATCH 07/11] mmc: core: WARN if SDIO IRQs are enabled for non-powered card in suspend

[Matthias Kaehlcke]

On Tue, Sep 03, 2019 at 04:22:03PM +0200, Ulf Hansson wrote:
> To make sure SDIO func drivers behaves correctly during system
> suspend/resume, let add a WARN_ON in case the condition is a non-powered
> SDIO card and there are some SDIO IRQs still being claimed.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/core/sdio.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 8dd8fc32ecca..c557f1519b77 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -951,6 +951,8 @@ static int mmc_sdio_pre_suspend(struct mmc_host *host)
>   */
>  static int mmc_sdio_suspend(struct mmc_host *host)
>  {
> +	WARN_ON(host->sdio_irqs && !mmc_card_keep_power(host));
> +
>  	/* Prevent processing of SDIO IRQs in suspended state. */
>  	mmc_card_set_suspended(host->card);
>  	cancel_delayed_work_sync(&host->sdio_irq_work);

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

[PATCH 08/11] mmc: core: Fixup processing of SDIO IRQs during system suspend/resume

[Ulf Hansson]

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@linaro.org>
---
 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);
 	}
 
 out:
diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
index d7965b53a6d2..900871073bd7 100644
--- a/drivers/mmc/core/sdio_irq.c
+++ b/drivers/mmc/core/sdio_irq.c
@@ -115,7 +115,8 @@ static void sdio_run_irqs(struct mmc_host *host)
 	mmc_claim_host(host);
 	if (host->sdio_irqs) {
 		process_sdio_pending_irqs(host);
-		host->ops->ack_sdio_irq(host);
+		if (!host->sdio_irq_pending)
+			host->ops->ack_sdio_irq(host);
 	}
 	mmc_release_host(host);
 }
-- 
2.17.1

Re: [PATCH 08/11] mmc: core: Fixup processing of SDIO IRQs during system suspend/resume

[Matthias Kaehlcke]

On Tue, Sep 03, 2019 at 04:22:04PM +0200, Ulf Hansson 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@linaro.org>
> ---
>  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);

You could possibly limit this to cards that remain powered during
suspend, but doing it always should do no harm.

>  	}
>  
>  out:
> diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
> index d7965b53a6d2..900871073bd7 100644
> --- a/drivers/mmc/core/sdio_irq.c
> +++ b/drivers/mmc/core/sdio_irq.c
> @@ -115,7 +115,8 @@ static void sdio_run_irqs(struct mmc_host *host)
>  	mmc_claim_host(host);
>  	if (host->sdio_irqs) {
>  		process_sdio_pending_irqs(host);
> -		host->ops->ack_sdio_irq(host);
> +		if (!host->sdio_irq_pending)
> +			host->ops->ack_sdio_irq(host);
>  	}
>  	mmc_release_host(host);
>  }

I'm by no means a SDIO expert, but as far as I can tell this looks
good. I verified that this patch fixes a problem with SDIO interrupts
that are ignored while suspending.

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH 08/11] mmc: core: Fixup processing of SDIO IRQs during system suspend/resume

[Ulf Hansson]

On Thu, 5 Sep 2019 at 20:43, Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Tue, Sep 03, 2019 at 04:22:04PM +0200, Ulf Hansson 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@linaro.org>
> > ---
> >  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);
>
> You could possibly limit this to cards that remain powered during
> suspend, but doing it always should do no harm.

Good point. That's actually why I included the change in patch7 ("mmc:
core: WARN if SDIO IRQs are enabled for non-powered card in suspend")

In principle it means that host->sdio_irqs must not be greater than 0,
if the card isn't powered.

Hmm, perhaps we should convert the WARN to return an error code
instead, just to be really safe. What do you think?

>
> >       }
> >
> >  out:
> > diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
> > index d7965b53a6d2..900871073bd7 100644
> > --- a/drivers/mmc/core/sdio_irq.c
> > +++ b/drivers/mmc/core/sdio_irq.c
> > @@ -115,7 +115,8 @@ static void sdio_run_irqs(struct mmc_host *host)
> >       mmc_claim_host(host);
> >       if (host->sdio_irqs) {
> >               process_sdio_pending_irqs(host);
> > -             host->ops->ack_sdio_irq(host);
> > +             if (!host->sdio_irq_pending)
> > +                     host->ops->ack_sdio_irq(host);
> >       }
> >       mmc_release_host(host);
> >  }
>
> I'm by no means a SDIO expert, but as far as I can tell this looks
> good. I verified that this patch fixes a problem with SDIO interrupts
> that are ignored while suspending.
>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Thanks, I add your tested by tag as well!

Kind regards
Uffe

Re: [PATCH 08/11] mmc: core: Fixup processing of SDIO IRQs during system suspend/resume

[Doug Anderson]

Hi,

On Tue, Sep 3, 2019 at 7:22 AM Ulf Hansson <ulf.hansson@linaro.org> 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@linaro.org>
> ---
>  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?

-Doug

Re: [PATCH 08/11] mmc: core: Fixup processing of SDIO IRQs during system suspend/resume

[Ulf Hansson]

On Fri, 6 Sep 2019 at 01:48, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Sep 3, 2019 at 7:22 AM Ulf Hansson <ulf.hansson@linaro.org> 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@linaro.org>
> > ---
> >  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

[PATCH 09/11] mmc: sdhci: Drop redundant check in sdhci_ack_sdio_irq()

[Ulf Hansson]

The sdhci_ack_sdio_irq() is called only when SDIO IRQs are enabled.
Therefore, let's drop the redundant check of the internal
SDHCI_SDIO_IRQ_ENABLED flag and just re-enable the IRQs immediately.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/sdhci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c1ebc26846db..2a9be044448a 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2161,8 +2161,7 @@ static void sdhci_ack_sdio_irq(struct mmc_host *mmc)
 	unsigned long flags;
 
 	spin_lock_irqsave(&host->lock, flags);
-	if (host->flags & SDHCI_SDIO_IRQ_ENABLED)
-		sdhci_enable_sdio_irq_nolock(host, true);
+	sdhci_enable_sdio_irq_nolock(host, true);
 	spin_unlock_irqrestore(&host->lock, flags);
 }
 
-- 
2.17.1

Re: [PATCH 09/11] mmc: sdhci: Drop redundant check in sdhci_ack_sdio_irq()

[Matthias Kaehlcke]

On Tue, Sep 03, 2019 at 04:22:05PM +0200, Ulf Hansson wrote:
> The sdhci_ack_sdio_irq() is called only when SDIO IRQs are enabled.
> Therefore, let's drop the redundant check of the internal
> SDHCI_SDIO_IRQ_ENABLED flag and just re-enable the IRQs immediately.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/host/sdhci.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c1ebc26846db..2a9be044448a 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2161,8 +2161,7 @@ static void sdhci_ack_sdio_irq(struct mmc_host *mmc)
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&host->lock, flags);
> -	if (host->flags & SDHCI_SDIO_IRQ_ENABLED)
> -		sdhci_enable_sdio_irq_nolock(host, true);
> +	sdhci_enable_sdio_irq_nolock(host, true);
>  	spin_unlock_irqrestore(&host->lock, flags);
>  }


FWIW:

Reviewed-by:  Matthias Kaehlcke <mka@chromium.org>

[PATCH 10/11] mmc: sdhci: Drop redundant code for SDIO IRQs

[Ulf Hansson]

Nowadays sdhci prevents runtime suspend when SDIO IRQs are enabled.

However, some variants such as sdhci-esdhc-imx's, tries to allow runtime
suspend while having the SDIO IRQs enabled, but without supporting remote
wakeups. This support is a bit questionable, especially if the host device
have a PM domain attached that can be power gated, but more importantly,
the code have also become redundant (which was not the case when it was
introduced).

Rather than keeping the redundant code around, let's drop it and leave this
to be revisited later on.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 34 +++++++++++++-----------------
 drivers/mmc/host/sdhci.c           |  2 +-
 drivers/mmc/host/sdhci.h           |  5 -----
 3 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 776a94216248..1c988d6a2433 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1666,12 +1666,10 @@ static int sdhci_esdhc_runtime_suspend(struct device *dev)
 	if (host->tuning_mode != SDHCI_TUNING_MODE_3)
 		mmc_retune_needed(host->mmc);
 
-	if (!sdhci_sdio_irq_enabled(host)) {
-		imx_data->actual_clock = host->mmc->actual_clock;
-		esdhc_pltfm_set_clock(host, 0);
-		clk_disable_unprepare(imx_data->clk_per);
-		clk_disable_unprepare(imx_data->clk_ipg);
-	}
+	imx_data->actual_clock = host->mmc->actual_clock;
+	esdhc_pltfm_set_clock(host, 0);
+	clk_disable_unprepare(imx_data->clk_per);
+	clk_disable_unprepare(imx_data->clk_ipg);
 	clk_disable_unprepare(imx_data->clk_ahb);
 
 	if (imx_data->socdata->flags & ESDHC_FLAG_PMQOS)
@@ -1695,15 +1693,15 @@ static int sdhci_esdhc_runtime_resume(struct device *dev)
 	if (err)
 		goto remove_pm_qos_request;
 
-	if (!sdhci_sdio_irq_enabled(host)) {
-		err = clk_prepare_enable(imx_data->clk_per);
-		if (err)
-			goto disable_ahb_clk;
-		err = clk_prepare_enable(imx_data->clk_ipg);
-		if (err)
-			goto disable_per_clk;
-		esdhc_pltfm_set_clock(host, imx_data->actual_clock);
-	}
+	err = clk_prepare_enable(imx_data->clk_per);
+	if (err)
+		goto disable_ahb_clk;
+
+	err = clk_prepare_enable(imx_data->clk_ipg);
+	if (err)
+		goto disable_per_clk;
+
+	esdhc_pltfm_set_clock(host, imx_data->actual_clock);
 
 	err = sdhci_runtime_resume_host(host, 0);
 	if (err)
@@ -1715,11 +1713,9 @@ static int sdhci_esdhc_runtime_resume(struct device *dev)
 	return err;
 
 disable_ipg_clk:
-	if (!sdhci_sdio_irq_enabled(host))
-		clk_disable_unprepare(imx_data->clk_ipg);
+	clk_disable_unprepare(imx_data->clk_ipg);
 disable_per_clk:
-	if (!sdhci_sdio_irq_enabled(host))
-		clk_disable_unprepare(imx_data->clk_per);
+	clk_disable_unprepare(imx_data->clk_per);
 disable_ahb_clk:
 	clk_disable_unprepare(imx_data->clk_ahb);
 remove_pm_qos_request:
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 2a9be044448a..a7df22ed65aa 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -3046,7 +3046,7 @@ static irqreturn_t sdhci_irq(int irq, void *dev_id)
 
 	spin_lock(&host->lock);
 
-	if (host->runtime_suspended && !sdhci_sdio_irq_enabled(host)) {
+	if (host->runtime_suspended) {
 		spin_unlock(&host->lock);
 		return IRQ_NONE;
 	}
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index cf3d1ed91909..8effaac61c3a 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -753,11 +753,6 @@ static inline void sdhci_read_caps(struct sdhci_host *host)
 	__sdhci_read_caps(host, NULL, NULL, NULL);
 }
 
-static inline bool sdhci_sdio_irq_enabled(struct sdhci_host *host)
-{
-	return !!(host->flags & SDHCI_SDIO_IRQ_ENABLED);
-}
-
 u16 sdhci_calc_clk(struct sdhci_host *host, unsigned int clock,
 		   unsigned int *actual_clock);
 void sdhci_set_clock(struct sdhci_host *host, unsigned int clock);
-- 
2.17.1

[PATCH 11/11] mmc: sdhci: Convert to use sdio_irq_enabled()

[Ulf Hansson]

Instead of keeping track of whether SDIO IRQs have been enabled via an
internal sdhci status flag, avoid the open-coding and convert into using
sdio_irq_enabled().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/host/sdhci.c | 7 +------
 drivers/mmc/host/sdhci.h | 1 -
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index a7df22ed65aa..4b4db41aec50 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2142,11 +2142,6 @@ void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
 		pm_runtime_get_noresume(host->mmc->parent);
 
 	spin_lock_irqsave(&host->lock, flags);
-	if (enable)
-		host->flags |= SDHCI_SDIO_IRQ_ENABLED;
-	else
-		host->flags &= ~SDHCI_SDIO_IRQ_ENABLED;
-
 	sdhci_enable_sdio_irq_nolock(host, enable);
 	spin_unlock_irqrestore(&host->lock, flags);
 
@@ -3380,7 +3375,7 @@ int sdhci_runtime_resume_host(struct sdhci_host *host, int soft_reset)
 	host->runtime_suspended = false;
 
 	/* Enable SDIO IRQ */
-	if (host->flags & SDHCI_SDIO_IRQ_ENABLED)
+	if (sdio_irq_enabled(mmc))
 		sdhci_enable_sdio_irq_nolock(host, true);
 
 	/* Enable Card Detection */
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 8effaac61c3a..a29c4cd2d92e 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -512,7 +512,6 @@ struct sdhci_host {
 #define SDHCI_AUTO_CMD12	(1<<6)	/* Auto CMD12 support */
 #define SDHCI_AUTO_CMD23	(1<<7)	/* Auto CMD23 support */
 #define SDHCI_PV_ENABLED	(1<<8)	/* Preset value enabled */
-#define SDHCI_SDIO_IRQ_ENABLED	(1<<9)	/* SDIO irq enabled */
 #define SDHCI_USE_64_BIT_DMA	(1<<12)	/* Use 64-bit DMA */
 #define SDHCI_HS400_TUNING	(1<<13)	/* Tuning for HS400 */
 #define SDHCI_SIGNALING_330	(1<<14)	/* Host is capable of 3.3V signaling */
-- 
2.17.1

Re: [PATCH 11/11] mmc: sdhci: Convert to use sdio_irq_enabled()

[Matthias Kaehlcke]

On Tue, Sep 03, 2019 at 04:22:07PM +0200, Ulf Hansson wrote:
> Instead of keeping track of whether SDIO IRQs have been enabled via an
> internal sdhci status flag, avoid the open-coding and convert into using
> sdio_irq_enabled().
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/mmc/host/sdhci.c | 7 +------
>  drivers/mmc/host/sdhci.h | 1 -
>  2 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index a7df22ed65aa..4b4db41aec50 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2142,11 +2142,6 @@ void sdhci_enable_sdio_irq(struct mmc_host *mmc, int enable)
>  		pm_runtime_get_noresume(host->mmc->parent);
>  
>  	spin_lock_irqsave(&host->lock, flags);
> -	if (enable)
> -		host->flags |= SDHCI_SDIO_IRQ_ENABLED;
> -	else
> -		host->flags &= ~SDHCI_SDIO_IRQ_ENABLED;
> -
>  	sdhci_enable_sdio_irq_nolock(host, enable);
>  	spin_unlock_irqrestore(&host->lock, flags);
>  
> @@ -3380,7 +3375,7 @@ int sdhci_runtime_resume_host(struct sdhci_host *host, int soft_reset)
>  	host->runtime_suspended = false;
>  
>  	/* Enable SDIO IRQ */
> -	if (host->flags & SDHCI_SDIO_IRQ_ENABLED)
> +	if (sdio_irq_enabled(mmc))
>  		sdhci_enable_sdio_irq_nolock(host, true);
>  
>  	/* Enable Card Detection */
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 8effaac61c3a..a29c4cd2d92e 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -512,7 +512,6 @@ struct sdhci_host {
>  #define SDHCI_AUTO_CMD12	(1<<6)	/* Auto CMD12 support */
>  #define SDHCI_AUTO_CMD23	(1<<7)	/* Auto CMD23 support */
>  #define SDHCI_PV_ENABLED	(1<<8)	/* Preset value enabled */
> -#define SDHCI_SDIO_IRQ_ENABLED	(1<<9)	/* SDIO irq enabled */
>  #define SDHCI_USE_64_BIT_DMA	(1<<12)	/* Use 64-bit DMA */
>  #define SDHCI_HS400_TUNING	(1<<13)	/* Tuning for HS400 */
>  #define SDHCI_SIGNALING_330	(1<<14)	/* Host is capable of 3.3V signaling */

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>