Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210320045720.11872-1-chgokhl@gmail.com> <CAPDyKFqXtKPngfnQZXXaF=rvqw0=nWzDc7P++QxNHPwGHHSxUA@mail.gmail.com>
In-Reply-To: <CAPDyKFqXtKPngfnQZXXaF=rvqw0=nWzDc7P++QxNHPwGHHSxUA@mail.gmail.com>
Reply-To: chgokhl@163.com
From:   hieagle <chgokhl@gmail.com>
Date:   Mon, 21 Jun 2021 09:38:34 +0800
Message-ID: <CAMjpFAUTsei-PWLDdM+_78qXTo=QFdw-Wbgo9QA2Jnss+i4yRA@mail.gmail.com>
Subject: Re: [PATCH] mmc: core: Mark mmc_host device with pm_runtime_no_callbacks
To:     Ulf Hansson <ulf.hansson@linaro.org>
Cc:     linux-mmc <linux-mmc@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        kehuanlin@fishsemi.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Sorry, I don't receive the reply email in my gmail.

Normally the mmc_host's power.disable_depth is large than zero, the
rpm_resume(mmc:0001) will not be called recursively for parent. This is
the most case.

Although the mmc class device never calls pm_runtime_enable() directly,
there are still some cases as below to call pm_runtime_enable(), which
may cause it's power.disable_depth decremented to zero.
  case1: device_resume_early->pm_runtime_enable
  case2: device_resume->pm_runtime_enable

Anything that can go wrong will go wrong. Unfortunately we meet the case.
If you trigger to set the mmc_host's power.disable_depth value to zero
after mmc suspended, you can find the issue.

In our platform the mmc device's parent list is as below:
     mmc0:0001->mmc_host mmc0->fa630000.mmc->soc.
The rpm_resume call trace is as below in our scenario:

rpm_resume(mmc0:0001)
|
if (!parent && dev->parent) //true
if (!parent->power.disable_depth
   && !parent->power.ignore_children) //true
rpm_resume(parent, 0) --->  rpm_resume(mmc_host, 0)
|                           |
|                           callback =3D RPM_GET_CALLBACK(mmc_host, ...) =
=3D NULL
|                           retval =3D rpm_callback(callback, mmc_host) =3D=
 -ENOSYS
|                           |
|                           return retval =3D -ENOSYS
if (retval) goto out; //skip rpm_callback()
return retval =3D -ENOSYS

The scenario is rare, but anything that can go wrong will go wrong.
The patch can enhance the code to avoid this scenario.

Ulf Hansson <ulf.hansson@linaro.org> =E4=BA=8E2021=E5=B9=B43=E6=9C=8822=E6=
=97=A5=E5=91=A8=E4=B8=80 =E4=B8=8B=E5=8D=886:26=E5=86=99=E9=81=93=EF=BC=9A
>
> On Sat, 20 Mar 2021 at 05:57, kehuanlin <chgokhl@gmail.com> wrote:
> >
> > The rpm_resume() will call parent's resume callback recursively.
> > Since mmc_host has no its own pm_runtime callbacks, the mmc devices
> > may fail to resume (-ENOSYS in rpm_callback) sometimes. Mark mmc_host
> > device with pm_runtime_no_callbacks can fix the issue.
>
> Can you please elaborate more on this? What do you mean by "sometimes"?
>
> More precisely, how do you trigger the rpm_callback() for mmc class
> device to return -ENOSYS?
>
> Don't get me wrong, the patch is fine, but I want to understand if it
> actually solves a problem for you - or that it's better considered as
> an optimization?
>
> Kind regards
> Uffe
>
> >
> > Signed-off-by: kehuanlin <chgokhl@gmail.com>
> > ---
> >  drivers/mmc/core/host.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> > index 9b89a91b6b47..177bebd9a6c4 100644
> > --- a/drivers/mmc/core/host.c
> > +++ b/drivers/mmc/core/host.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/of.h>
> >  #include <linux/of_gpio.h>
> >  #include <linux/pagemap.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/pm_wakeup.h>
> >  #include <linux/export.h>
> >  #include <linux/leds.h>
> > @@ -480,6 +481,7 @@ struct mmc_host *mmc_alloc_host(int extra, struct d=
evice *dev)
> >         host->class_dev.class =3D &mmc_host_class;
> >         device_initialize(&host->class_dev);
> >         device_enable_async_suspend(&host->class_dev);
> > +       pm_runtime_no_callbacks(&host->class_dev);
> >
> >         if (mmc_gpio_alloc(host)) {
> >                 put_device(&host->class_dev);
> > --
> > 2.30.0
> >