* [PATCH] cpuidle: Check cpuidle driver before add refcount @ 2013-08-30 11:48 Daniel Fu 2013-08-30 12:01 ` Rafael J. Wysocki 0 siblings, 1 reply; 6+ messages in thread From: Daniel Fu @ 2013-08-30 11:48 UTC (permalink / raw) To: rjw, daniel.lezcano; +Cc: linux-pm, linux-kernel, joqiu, Daniel Fu If the current CPU has no cpuidle driver, drv will be NULL. Check if we get drv successfully before add refount to prevent Kernel panic. Signed-off-by: Daniel Fu <danifu@nvidia.com> --- drivers/cpuidle/driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 3ac499d..6e11701 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -331,7 +331,8 @@ struct cpuidle_driver *cpuidle_driver_ref(void) spin_lock(&cpuidle_driver_lock); drv = cpuidle_get_driver(); - drv->refcnt++; + if (drv) + drv->refcnt++; spin_unlock(&cpuidle_driver_lock); return drv; -- 1.8.1.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] cpuidle: Check cpuidle driver before add refcount 2013-08-30 11:48 [PATCH] cpuidle: Check cpuidle driver before add refcount Daniel Fu @ 2013-08-30 12:01 ` Rafael J. Wysocki 2013-08-30 12:26 ` Daniel Fu 0 siblings, 1 reply; 6+ messages in thread From: Rafael J. Wysocki @ 2013-08-30 12:01 UTC (permalink / raw) To: Daniel Fu; +Cc: daniel.lezcano, linux-pm, linux-kernel, joqiu On Friday, August 30, 2013 07:48:22 PM Daniel Fu wrote: > If the current CPU has no cpuidle driver, drv will be NULL. > Check if we get drv successfully before add refount > to prevent Kernel panic. What is the actual scenario that may lead to this panic? Rafael > Signed-off-by: Daniel Fu <danifu@nvidia.com> > --- > drivers/cpuidle/driver.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c > index 3ac499d..6e11701 100644 > --- a/drivers/cpuidle/driver.c > +++ b/drivers/cpuidle/driver.c > @@ -331,7 +331,8 @@ struct cpuidle_driver *cpuidle_driver_ref(void) > spin_lock(&cpuidle_driver_lock); > > drv = cpuidle_get_driver(); > - drv->refcnt++; > + if (drv) > + drv->refcnt++; > > spin_unlock(&cpuidle_driver_lock); > return drv; > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] cpuidle: Check cpuidle driver before add refcount 2013-08-30 12:01 ` Rafael J. Wysocki @ 2013-08-30 12:26 ` Daniel Fu 2013-08-30 13:25 ` Rafael J. Wysocki 0 siblings, 1 reply; 6+ messages in thread From: Daniel Fu @ 2013-08-30 12:26 UTC (permalink / raw) To: Rafael J. Wysocki Cc: daniel.lezcano@linaro.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, Johnny Qiu X-NVConfidentiality: public In SMP, When doing the cpuidle driver registration. There maybe only 1 cpu have register cpuidle(CPU0), but In the process of registration cpuidle driver, the scheduler maybe schedule the process to the other CPU (eg. CPU0 have heay load, migrate to CPU1) cpuidle_get_driver() --> get_cpu() , will get CPU1, if CPU1 didn't register cpuidle driver, Will get NULL. I know we should prevent the registration migrate to the other CPU, but we should check drv before add refcount to prevent kernel panic at least, Right? Best Regards, Daniel -----Original Message----- From: Rafael J. Wysocki [mailto:rjw@sisk.pl] Sent: 2013年8月30日 20:01 To: Daniel Fu Cc: daniel.lezcano@linaro.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Johnny Qiu Subject: Re: [PATCH] cpuidle: Check cpuidle driver before add refcount On Friday, August 30, 2013 07:48:22 PM Daniel Fu wrote: > If the current CPU has no cpuidle driver, drv will be NULL. > Check if we get drv successfully before add refount to prevent Kernel > panic. What is the actual scenario that may lead to this panic? Rafael > Signed-off-by: Daniel Fu <danifu@nvidia.com> > --- > drivers/cpuidle/driver.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index > 3ac499d..6e11701 100644 > --- a/drivers/cpuidle/driver.c > +++ b/drivers/cpuidle/driver.c > @@ -331,7 +331,8 @@ struct cpuidle_driver *cpuidle_driver_ref(void) > spin_lock(&cpuidle_driver_lock); > > drv = cpuidle_get_driver(); > - drv->refcnt++; > + if (drv) > + drv->refcnt++; > > spin_unlock(&cpuidle_driver_lock); > return drv; > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cpuidle: Check cpuidle driver before add refcount 2013-08-30 12:26 ` Daniel Fu @ 2013-08-30 13:25 ` Rafael J. Wysocki 2013-08-30 13:39 ` Daniel Fu 0 siblings, 1 reply; 6+ messages in thread From: Rafael J. Wysocki @ 2013-08-30 13:25 UTC (permalink / raw) To: Daniel Fu Cc: daniel.lezcano@linaro.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, Johnny Qiu On Friday, August 30, 2013 08:26:34 PM Daniel Fu wrote: > X-NVConfidentiality: public > > In SMP, When doing the cpuidle driver registration. There maybe only 1 cpu have register cpuidle(CPU0), > but In the process of registration cpuidle driver, the scheduler maybe schedule the process to the other CPU (eg. CPU0 have heay load, migrate to CPU1) > cpuidle_get_driver() --> get_cpu() , will get CPU1, if CPU1 didn't register cpuidle driver, Will get NULL. > > I know we should prevent the registration migrate to the other CPU, but we should check drv before add refcount to prevent kernel panic at least, Right? Well, cpuidle_driver_ref() has only one caller, which is not the registration. However, the caller apparently assumes that that function may return NULL and cpuidle_driver_unref() check drv against NULL too, so cpuidle_driver_ref() should do that either. Did you see that problem in testing or just through code inspection? Rafael > -----Original Message----- > From: Rafael J. Wysocki [mailto:rjw@sisk.pl] > Sent: 2013年8月30日 20:01 > To: Daniel Fu > Cc: daniel.lezcano@linaro.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Johnny Qiu > Subject: Re: [PATCH] cpuidle: Check cpuidle driver before add refcount > > On Friday, August 30, 2013 07:48:22 PM Daniel Fu wrote: > > If the current CPU has no cpuidle driver, drv will be NULL. > > Check if we get drv successfully before add refount to prevent Kernel > > panic. > > What is the actual scenario that may lead to this panic? > > Rafael > > > > Signed-off-by: Daniel Fu <danifu@nvidia.com> > > --- > > drivers/cpuidle/driver.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index > > 3ac499d..6e11701 100644 > > --- a/drivers/cpuidle/driver.c > > +++ b/drivers/cpuidle/driver.c > > @@ -331,7 +331,8 @@ struct cpuidle_driver *cpuidle_driver_ref(void) > > spin_lock(&cpuidle_driver_lock); > > > > drv = cpuidle_get_driver(); > > - drv->refcnt++; > > + if (drv) > > + drv->refcnt++; > > > > spin_unlock(&cpuidle_driver_lock); > > return drv; > > > -- > I speak only for myself. > Rafael J. Wysocki, Intel Open Source Technology Center. > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] cpuidle: Check cpuidle driver before add refcount 2013-08-30 13:25 ` Rafael J. Wysocki @ 2013-08-30 13:39 ` Daniel Fu 2013-08-30 19:51 ` Rafael J. Wysocki 0 siblings, 1 reply; 6+ messages in thread From: Daniel Fu @ 2013-08-30 13:39 UTC (permalink / raw) To: Rafael J. Wysocki Cc: daniel.lezcano@linaro.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, Johnny Qiu Yes, I saw it. Thanks. found one case when Find PM domain and connect cpuidle to it , in when Kernel booting ,opening early prink testing pm_genpd_name_attach_cpuidle()->cpuidle_driver_ref(). -----Original Message----- From: Rafael J. Wysocki [mailto:rjw@sisk.pl] Sent: 2013年8月30日 21:26 To: Daniel Fu Cc: daniel.lezcano@linaro.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Johnny Qiu Subject: Re: [PATCH] cpuidle: Check cpuidle driver before add refcount On Friday, August 30, 2013 08:26:34 PM Daniel Fu wrote: > X-NVConfidentiality: public > > In SMP, When doing the cpuidle driver registration. There maybe only 1 > cpu have register cpuidle(CPU0), but In the process of registration > cpuidle driver, the scheduler maybe schedule the process to the other > CPU (eg. CPU0 have heay load, migrate to CPU1) > cpuidle_get_driver() --> get_cpu() , will get CPU1, if CPU1 didn't register cpuidle driver, Will get NULL. > > I know we should prevent the registration migrate to the other CPU, but we should check drv before add refcount to prevent kernel panic at least, Right? Well, cpuidle_driver_ref() has only one caller, which is not the registration. However, the caller apparently assumes that that function may return NULL and cpuidle_driver_unref() check drv against NULL too, so cpuidle_driver_ref() should do that either. Did you see that problem in testing or just through code inspection? Rafael > -----Original Message----- > From: Rafael J. Wysocki [mailto:rjw@sisk.pl] > Sent: 2013年8月30日 20:01 > To: Daniel Fu > Cc: daniel.lezcano@linaro.org; linux-pm@vger.kernel.org; > linux-kernel@vger.kernel.org; Johnny Qiu > Subject: Re: [PATCH] cpuidle: Check cpuidle driver before add refcount > > On Friday, August 30, 2013 07:48:22 PM Daniel Fu wrote: > > If the current CPU has no cpuidle driver, drv will be NULL. > > Check if we get drv successfully before add refount to prevent > > Kernel panic. > > What is the actual scenario that may lead to this panic? > > Rafael > > > > Signed-off-by: Daniel Fu <danifu@nvidia.com> > > --- > > drivers/cpuidle/driver.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c > > index > > 3ac499d..6e11701 100644 > > --- a/drivers/cpuidle/driver.c > > +++ b/drivers/cpuidle/driver.c > > @@ -331,7 +331,8 @@ struct cpuidle_driver *cpuidle_driver_ref(void) > > spin_lock(&cpuidle_driver_lock); > > > > drv = cpuidle_get_driver(); > > - drv->refcnt++; > > + if (drv) > > + drv->refcnt++; > > > > spin_unlock(&cpuidle_driver_lock); > > return drv; > > > -- > I speak only for myself. > Rafael J. Wysocki, Intel Open Source Technology Center. > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cpuidle: Check cpuidle driver before add refcount 2013-08-30 13:39 ` Daniel Fu @ 2013-08-30 19:51 ` Rafael J. Wysocki 0 siblings, 0 replies; 6+ messages in thread From: Rafael J. Wysocki @ 2013-08-30 19:51 UTC (permalink / raw) To: Daniel Fu Cc: daniel.lezcano@linaro.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, Johnny Qiu On Friday, August 30, 2013 09:39:27 PM Daniel Fu wrote: > Yes, I saw it. > Thanks. found one case when Find PM domain and connect cpuidle to it , in when Kernel booting ,opening early prink testing > pm_genpd_name_attach_cpuidle()->cpuidle_driver_ref(). I see. OK, I'll queue it up for the second part of the 3.12 merge window. Thanks, Rafael > -----Original Message----- > From: Rafael J. Wysocki [mailto:rjw@sisk.pl] > Sent: 2013年8月30日 21:26 > To: Daniel Fu > Cc: daniel.lezcano@linaro.org; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Johnny Qiu > Subject: Re: [PATCH] cpuidle: Check cpuidle driver before add refcount > > On Friday, August 30, 2013 08:26:34 PM Daniel Fu wrote: > > X-NVConfidentiality: public > > > > In SMP, When doing the cpuidle driver registration. There maybe only 1 > > cpu have register cpuidle(CPU0), but In the process of registration > > cpuidle driver, the scheduler maybe schedule the process to the other > > CPU (eg. CPU0 have heay load, migrate to CPU1) > > cpuidle_get_driver() --> get_cpu() , will get CPU1, if CPU1 didn't register cpuidle driver, Will get NULL. > > > > I know we should prevent the registration migrate to the other CPU, but we should check drv before add refcount to prevent kernel panic at least, Right? > > Well, cpuidle_driver_ref() has only one caller, which is not the registration. > > However, the caller apparently assumes that that function may return NULL and > cpuidle_driver_unref() check drv against NULL too, so cpuidle_driver_ref() should do that either. > > Did you see that problem in testing or just through code inspection? > > Rafael > > > > -----Original Message----- > > From: Rafael J. Wysocki [mailto:rjw@sisk.pl] > > Sent: 2013年8月30日 20:01 > > To: Daniel Fu > > Cc: daniel.lezcano@linaro.org; linux-pm@vger.kernel.org; > > linux-kernel@vger.kernel.org; Johnny Qiu > > Subject: Re: [PATCH] cpuidle: Check cpuidle driver before add refcount > > > > On Friday, August 30, 2013 07:48:22 PM Daniel Fu wrote: > > > If the current CPU has no cpuidle driver, drv will be NULL. > > > Check if we get drv successfully before add refount to prevent > > > Kernel panic. > > > > What is the actual scenario that may lead to this panic? > > > > Rafael > > > > > > > Signed-off-by: Daniel Fu <danifu@nvidia.com> > > > --- > > > drivers/cpuidle/driver.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c > > > index > > > 3ac499d..6e11701 100644 > > > --- a/drivers/cpuidle/driver.c > > > +++ b/drivers/cpuidle/driver.c > > > @@ -331,7 +331,8 @@ struct cpuidle_driver *cpuidle_driver_ref(void) > > > spin_lock(&cpuidle_driver_lock); > > > > > > drv = cpuidle_get_driver(); > > > - drv->refcnt++; > > > + if (drv) > > > + drv->refcnt++; > > > > > > spin_unlock(&cpuidle_driver_lock); > > > return drv; > > > > > -- > > I speak only for myself. > > Rafael J. Wysocki, Intel Open Source Technology Center. > > > -- > I speak only for myself. > Rafael J. Wysocki, Intel Open Source Technology Center. > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-08-30 19:40 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-30 11:48 [PATCH] cpuidle: Check cpuidle driver before add refcount Daniel Fu 2013-08-30 12:01 ` Rafael J. Wysocki 2013-08-30 12:26 ` Daniel Fu 2013-08-30 13:25 ` Rafael J. Wysocki 2013-08-30 13:39 ` Daniel Fu 2013-08-30 19:51 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox