linux-kernel - Re: [PATCH] mmc: core: Mark mmc_host device with pm_runtime_no

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Jun 2021 09:38:34 +0800
From:   hieagle <chgokhl@...il.com>
To:     Ulf Hansson <ulf.hansson@...aro.org>
Cc:     linux-mmc <linux-mmc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        kehuanlin@...hsemi.com
Subject: Re: [PATCH] mmc: core: Mark mmc_host device with pm_runtime_no_callbacks

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 = RPM_GET_CALLBACK(mmc_host, ...) = NULL
|                           retval = rpm_callback(callback, mmc_host) = -ENOSYS
|                           |
|                           return retval = -ENOSYS
if (retval) goto out; //skip rpm_callback()
return retval = -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@...aro.org> 于2021年3月22日周一 下午6:26写道：
>
> On Sat, 20 Mar 2021 at 05:57, kehuanlin <chgokhl@...il.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@...il.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 device *dev)
> >         host->class_dev.class = &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
> >