diff mbox series

[v2] mmc: sdio: add a delay to call sdio_irq_work when sdio bus resume

Message ID 1597835959-22402-1-git-send-email-haibo.chen@nxp.com (mailing list archive)
State New, archived
Headers show
Series [v2] mmc: sdio: add a delay to call sdio_irq_work when sdio bus resume | expand

Commit Message

Bough Chen Aug. 19, 2020, 11:19 a.m. UTC 
From: Haibo Chen <haibo.chen@nxp.com>

Find this issue on i.MX7D-sdb board with broadcom sdio wifi.
When system resume, sometimes this wifi meet the tuning fail issue.
All tuning command get command timeout error. This is because
sdio_irq_work on system_wq was executed before the broadcom
wifi driver resume. Due to broadcom wifi driver set the wifi in
Sleep sate in system suspend, need to set the wifi to Wake state
first. So need to make sure wifi driver resume first, then do the
sdio_irq_work.

Fixes: 51133850bce2 ("mmc: core: Fixup processing of SDIO IRQs during system suspend/resume")
Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/mmc/core/sdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ulf Hansson Aug. 21, 2020, 7:59 a.m. UTC | #1 
On Wed, 19 Aug 2020 at 13:24, <haibo.chen@nxp.com> wrote:
>
> From: Haibo Chen <haibo.chen@nxp.com>
>
> Find this issue on i.MX7D-sdb board with broadcom sdio wifi.
> When system resume, sometimes this wifi meet the tuning fail issue.
> All tuning command get command timeout error. This is because
> sdio_irq_work on system_wq was executed before the broadcom
> wifi driver resume. Due to broadcom wifi driver set the wifi in
> Sleep sate in system suspend, need to set the wifi to Wake state
> first. So need to make sure wifi driver resume first, then do the
> sdio_irq_work.
>
> Fixes: 51133850bce2 ("mmc: core: Fixup processing of SDIO IRQs during system suspend/resume")
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> ---
>  drivers/mmc/core/sdio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 7b40553d3934..101632617f69 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -1047,7 +1047,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)
> -                       queue_delayed_work(system_wq, &host->sdio_irq_work, 0);
> +                       queue_delayed_work(system_wq, &host->sdio_irq_work, 1);

This looks fragile, as there is really no guarantee that the WiFi
driver is resumed prior to the SDIO irq work being executed.

Instead, it looks like you shouldn't keep the SDIO irqs enabled during
system suspend, because those aren't system wakeup capable. Correct?

In other words, I think the WiFi driver should call sdio_release_irq()
in the ->suspend() callback and sdio_claim_irq() in the ->resume()
callback.

>         }
>
>  out:
> --
> 2.17.1
>

Kind regards
Uffe
Bough Chen Aug. 21, 2020, 8:08 a.m. UTC | #2 
> -----Original Message-----
> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> Sent: 2020年8月21日 15:59
> To: Bough Chen <haibo.chen@nxp.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>; linux-mmc@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; Matthias Kaehlcke <mka@chromium.org>;
> huyue2@yulong.com; Doug Anderson <dianders@chromium.org>; Pali Rohár
> <pali@kernel.org>
> Subject: Re: [PATCH v2] mmc: sdio: add a delay to call sdio_irq_work when sdio
> bus resume
> 
> On Wed, 19 Aug 2020 at 13:24, <haibo.chen@nxp.com> wrote:
> >
> > From: Haibo Chen <haibo.chen@nxp.com>
> >
> > Find this issue on i.MX7D-sdb board with broadcom sdio wifi.
> > When system resume, sometimes this wifi meet the tuning fail issue.
> > All tuning command get command timeout error. This is because
> > sdio_irq_work on system_wq was executed before the broadcom wifi
> > driver resume. Due to broadcom wifi driver set the wifi in Sleep sate
> > in system suspend, need to set the wifi to Wake state first. So need
> > to make sure wifi driver resume first, then do the sdio_irq_work.
> >
> > Fixes: 51133850bce2 ("mmc: core: Fixup processing of SDIO IRQs during
> > system suspend/resume")
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > ---
> >  drivers/mmc/core/sdio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c index
> > 7b40553d3934..101632617f69 100644
> > --- a/drivers/mmc/core/sdio.c
> > +++ b/drivers/mmc/core/sdio.c
> > @@ -1047,7 +1047,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)
> > -                       queue_delayed_work(system_wq,
> &host->sdio_irq_work, 0);
> > +                       queue_delayed_work(system_wq,
> > + &host->sdio_irq_work, 1);
> 
> This looks fragile, as there is really no guarantee that the WiFi driver is
> resumed prior to the SDIO irq work being executed.

Yes, agree.

> 
> Instead, it looks like you shouldn't keep the SDIO irqs enabled during system
> suspend, because those aren't system wakeup capable. Correct?
> 
> In other words, I think the WiFi driver should call sdio_release_irq() in the
> ->suspend() callback and sdio_claim_irq() in the ->resume() callback.

Let me double check the WIFI driver. Thanks for the suggestion!

Best Regards
Haibo Chen
> 
> >         }
> >
> >  out:
> > --
> > 2.17.1
> >
> 
> Kind regards
> Uffe
diff mbox series

Patch

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 7b40553d3934..101632617f69 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -1047,7 +1047,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)
-			queue_delayed_work(system_wq, &host->sdio_irq_work, 0);
+			queue_delayed_work(system_wq, &host->sdio_irq_work, 1);
 	}
 
 out: