* [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, linux-pm, linux-kernel, Johnny Qiu
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1934 bytes --]
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.
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ 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, linux-pm, linux-kernel, 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, linux-pm, linux-kernel, Johnny Qiu
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2977 bytes --]
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.
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ 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, linux-pm, linux-kernel, 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;
as well as URLs for NNTP newsgroup(s).