| Message ID | 1249956052-21893-1-git-send-email-mike@android.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kevin Hilman |
| Headers | show |
Mike Chan <mike@android.com> writes: > Signed-off-by: Mike Chan <mike@android.com> > --- > arch/arm/mach-omap2/powerdomain.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c > index 0334609..6077629 100644 > --- a/arch/arm/mach-omap2/powerdomain.c > +++ b/arch/arm/mach-omap2/powerdomain.c > @@ -90,7 +90,7 @@ static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm, > if (!pwrdm || !deps || !omap_chip_is(pwrdm->omap_chip)) > return ERR_PTR(-EINVAL); > > - for (pd = deps; pd; pd++) { > + for (pd = deps; pd->pwrdm_name; pd++) { Maybe should be: for (pd = deps; pd && pd->pwrdm_name; pd++) { ? > > if (!omap_chip_is(pd->omap_chip)) > continue; Also, a descriptive changelog would be helpful here describing the conditions where you saw overflow etc. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 11, 2009 at 7:38 AM, Kevin Hilman<khilman@deeprootsystems.com> wrote: > Mike Chan <mike@android.com> writes: > >> Signed-off-by: Mike Chan <mike@android.com> >> --- >> Â arch/arm/mach-omap2/powerdomain.c | Â Â 2 +- >> Â 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c >> index 0334609..6077629 100644 >> --- a/arch/arm/mach-omap2/powerdomain.c >> +++ b/arch/arm/mach-omap2/powerdomain.c >> @@ -90,7 +90,7 @@ static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm, >> Â Â Â if (!pwrdm || !deps || !omap_chip_is(pwrdm->omap_chip)) >> Â Â Â Â Â Â Â return ERR_PTR(-EINVAL); >> >> - Â Â for (pd = deps; pd; pd++) { >> + Â Â for (pd = deps; pd->pwrdm_name; pd++) { > > Maybe should be: > > Â Â Â Â for (pd = deps; pd && pd->pwrdm_name; pd++) { > At the end of the list pd is a pointer to a NULL struct, so checking if the address == NULL doesn't help here. In fact the original code will just keep running past the struct to read who knows what in memory. This case manifests itself when from clkdms_setup() when enabling auto idle for a clock domain and the clockdomain usecount is greater than 0. When _clkdm_add_autodeps() tries to add the a dependency that does not exist in the powerdomain->wkdep_srcs array the for loop will run past the wkdep_srcs array. Currently in linux-omap you won't hit this because the not found case is never executed, unless you start modifying powerdomains and their wakeup/sleep deps. --Mike > ? > >> >> Â Â Â Â Â Â Â if (!omap_chip_is(pd->omap_chip)) >> Â Â Â Â Â Â Â Â Â Â Â continue; > > Also, a descriptive changelog would be helpful here describing the > conditions where you saw overflow etc. > > Kevin > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mike Chan <mike@android.com> writes: > On Tue, Aug 11, 2009 at 7:38 AM, Kevin > Hilman<khilman@deeprootsystems.com> wrote: >> Mike Chan <mike@android.com> writes: >> >>> Signed-off-by: Mike Chan <mike@android.com> >>> --- >>> Â arch/arm/mach-omap2/powerdomain.c | Â Â 2 +- >>> Â 1 files changed, 1 insertions(+), 1 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c >>> index 0334609..6077629 100644 >>> --- a/arch/arm/mach-omap2/powerdomain.c >>> +++ b/arch/arm/mach-omap2/powerdomain.c >>> @@ -90,7 +90,7 @@ static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm, >>> Â Â Â if (!pwrdm || !deps || !omap_chip_is(pwrdm->omap_chip)) >>> Â Â Â Â Â Â Â return ERR_PTR(-EINVAL); >>> >>> - Â Â for (pd = deps; pd; pd++) { >>> + Â Â for (pd = deps; pd->pwrdm_name; pd++) { >> >> Maybe should be: >> >> Â Â Â Â for (pd = deps; pd && pd->pwrdm_name; pd++) { >> > > At the end of the list pd is a pointer to a NULL struct, so checking > if the address == NULL doesn't help here. In fact the original code > will just keep running past the struct to read who knows what in > memory. I see that now, didn't notice the NULL struct terminators. > This case manifests itself when from clkdms_setup() when enabling auto > idle for a clock domain and the clockdomain usecount is greater than > 0. When _clkdm_add_autodeps() tries to add the a dependency that does > not exist in the powerdomain->wkdep_srcs array the for loop will run > past the wkdep_srcs array. Thanks, will add this as changelog and push to PM branch as well as pm-2.6.29. Kevin > Currently in linux-omap you won't hit this because the not found case > is never executed, unless you start modifying powerdomains and their > wakeup/sleep deps. > > --Mike > >> ? >> >>> >>> Â Â Â Â Â Â Â if (!omap_chip_is(pd->omap_chip)) >>> Â Â Â Â Â Â Â Â Â Â Â continue; >> >> Also, a descriptive changelog would be helpful here describing the >> conditions where you saw overflow etc. >> >> Kevin >> -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mike, On Tue, 11 Aug 2009, Mike Chan wrote: > On Tue, Aug 11, 2009 at 7:38 AM, Kevin > Hilman<khilman@deeprootsystems.com> wrote: > > Mike Chan <mike@android.com> writes: > > > >> Signed-off-by: Mike Chan <mike@android.com> > >> --- > >> Â arch/arm/mach-omap2/powerdomain.c | Â Â 2 +- > >> Â 1 files changed, 1 insertions(+), 1 deletions(-) > >> > >> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c > >> index 0334609..6077629 100644 > >> --- a/arch/arm/mach-omap2/powerdomain.c > >> +++ b/arch/arm/mach-omap2/powerdomain.c > >> @@ -90,7 +90,7 @@ static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm, > >> Â Â Â if (!pwrdm || !deps || !omap_chip_is(pwrdm->omap_chip)) > >> Â Â Â Â Â Â Â return ERR_PTR(-EINVAL); > >> > >> - Â Â for (pd = deps; pd; pd++) { > >> + Â Â for (pd = deps; pd->pwrdm_name; pd++) { > > > > Maybe should be: > > > > Â Â Â Â for (pd = deps; pd && pd->pwrdm_name; pd++) { > > > > At the end of the list pd is a pointer to a NULL struct, so checking > if the address == NULL doesn't help here. In fact the original code > will just keep running past the struct to read who knows what in > memory. > > This case manifests itself when from clkdms_setup() when enabling auto > idle for a clock domain and the clockdomain usecount is greater than > 0. When _clkdm_add_autodeps() tries to add the a dependency that does > not exist in the powerdomain->wkdep_srcs array the for loop will run > past the wkdep_srcs array. > > Currently in linux-omap you won't hit this because the not found case > is never executed, unless you start modifying powerdomains and their > wakeup/sleep deps. > > --Mike Thanks for the patch, this is correct. But the patch also should modify: if (!pd) return ERR_PTR(-ENOENT); after the loop to be if (!pd->pwrdm_name) ... Could you revise this and resend? thanks - Paul
On Tue, Aug 11, 2009 at 11:32 PM, Paul Walmsley<paul@pwsan.com> wrote: > Hi Mike, > > On Tue, 11 Aug 2009, Mike Chan wrote: > >> On Tue, Aug 11, 2009 at 7:38 AM, Kevin >> Hilman<khilman@deeprootsystems.com> wrote: >> > Mike Chan <mike@android.com> writes: >> > >> >> Signed-off-by: Mike Chan <mike@android.com> >> >> --- >> >> Â arch/arm/mach-omap2/powerdomain.c | Â Â 2 +- >> >> Â 1 files changed, 1 insertions(+), 1 deletions(-) >> >> >> >> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c >> >> index 0334609..6077629 100644 >> >> --- a/arch/arm/mach-omap2/powerdomain.c >> >> +++ b/arch/arm/mach-omap2/powerdomain.c >> >> @@ -90,7 +90,7 @@ static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm, >> >> Â Â Â if (!pwrdm || !deps || !omap_chip_is(pwrdm->omap_chip)) >> >> Â Â Â Â Â Â Â return ERR_PTR(-EINVAL); >> >> >> >> - Â Â for (pd = deps; pd; pd++) { >> >> + Â Â for (pd = deps; pd->pwrdm_name; pd++) { >> > >> > Maybe should be: >> > >> > Â Â Â Â for (pd = deps; pd && pd->pwrdm_name; pd++) { >> > >> >> At the end of the list pd is a pointer to a NULL struct, so checking >> if the address == NULL doesn't help here. In fact the original code >> will just keep running past the struct to read who knows what in >> memory. >> >> This case manifests itself when from clkdms_setup() when enabling auto >> idle for a clock domain and the clockdomain usecount is greater than >> 0. When _clkdm_add_autodeps() tries to add the a dependency that does >> not exist in the powerdomain->wkdep_srcs array the for loop will run >> past the wkdep_srcs array. >> >> Currently in linux-omap you won't hit this because the not found case >> is never executed, unless you start modifying powerdomains and their >> wakeup/sleep deps. >> >> --Mike > > Thanks for the patch, this is correct. Â But the patch also should modify: > > Â Â Â Â if (!pd) > Â Â Â Â Â Â Â Â return ERR_PTR(-ENOENT); > > after the loop to be > > Â Â Â Â if (!pd->pwrdm_name) > Â Â Â Â Â Â Â Â ... > > Could you revise this and resend? > > thanks > Good catch, looks like this was already merged into the omap-pm branch :/ Sending a followup patch fine with everyone? > > - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c index 0334609..6077629 100644 --- a/arch/arm/mach-omap2/powerdomain.c +++ b/arch/arm/mach-omap2/powerdomain.c @@ -90,7 +90,7 @@ static struct powerdomain *_pwrdm_deps_lookup(struct powerdomain *pwrdm, if (!pwrdm || !deps || !omap_chip_is(pwrdm->omap_chip)) return ERR_PTR(-EINVAL); - for (pd = deps; pd; pd++) { + for (pd = deps; pd->pwrdm_name; pd++) { if (!omap_chip_is(pd->omap_chip)) continue;
Signed-off-by: Mike Chan <mike@android.com> --- arch/arm/mach-omap2/powerdomain.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)