| Message ID | 1413941828-5547-1-git-send-email-tjakobi@math.uni-bielefeld.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tobias, On Tue, Oct 21, 2014 at 6:37 PM, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote: > EXYNOS4_MCT_L_MASK is defined as 0xffffff00, so applying this bitmask > produces a number outside the range 0x00 to 0xff, which always results > in execution of the default switch statement. > > Obviously this is wrong and git history shows that the bitmask inversion > was incorrectly set during a refactoring of the MCT code. > > Fix this by putting the inversion at the correct position again. > > Reported-by: GP Orcullo <kinsamanka@gmail.com> > Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> > --- > drivers/clocksource/exynos_mct.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Wow, the old code was pretty broken. Nice find! Was this found by code inspection / analysis or were you seeing some actual bug? I think it was broken back in March 2013 in (a1ba7a7 ARM: EXYNOS: add a register base address variable in mct controller driver), so 1.5 years. The broken code has been running for a long time, but that doesn't mean that there isn't a subtle issue... In any case, it seems like we should take this fix, assuming someone has tested it well. Reviewed-by: Doug Anderson <dianders@chromium.org> -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Doug, I didn't encounter any obvious problems with the MCT. I was made aware of the issue by kinsamanka on the Hardkernel forums, but didn't tell me through which means he/she found it. Concerning testing, I have this running since some weeks on my ODROID-X2 (Exynos4412), again, haven't encountered anything 'strange' yet. With best wishes, Tobias On 2014-10-22 17:57, Doug Anderson wrote: > Tobias, > > On Tue, Oct 21, 2014 at 6:37 PM, Tobias Jakobi > <tjakobi@math.uni-bielefeld.de> wrote: >> EXYNOS4_MCT_L_MASK is defined as 0xffffff00, so applying this bitmask >> produces a number outside the range 0x00 to 0xff, which always results >> in execution of the default switch statement. >> >> Obviously this is wrong and git history shows that the bitmask >> inversion >> was incorrectly set during a refactoring of the MCT code. >> >> Fix this by putting the inversion at the correct position again. >> >> Reported-by: GP Orcullo <kinsamanka@gmail.com> >> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >> --- >> drivers/clocksource/exynos_mct.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) > > Wow, the old code was pretty broken. Nice find! Was this found by > code inspection / analysis or were you seeing some actual bug? I > think it was broken back in March 2013 in (a1ba7a7 ARM: EXYNOS: add a > register base address variable in mct controller driver), so 1.5 > years. The broken code has been running for a long time, but that > doesn't mean that there isn't a subtle issue... > > In any case, it seems like we should take this fix, assuming someone > has tested it well. > > Reviewed-by: Doug Anderson <dianders@chromium.org> -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tobias, On Wed, Oct 22, 2014 at 9:12 AM, Tobias Jakobi <tjakobi@math.uni-bielefeld.de> wrote: > Hello Doug, > > I didn't encounter any obvious problems with the MCT. I was made aware of > the issue by kinsamanka on the Hardkernel forums, but didn't tell me through > which means he/she found it. > Looks like this came up back in February (https://lkml.org/lkml/2014/2/4/782) and the reason the code currently works is that these values are used for checking the success of the write operation. So in the current code we never check the success of the write but it hasn't been a problem because presumably the write is almost always successful. Regardless, I agree with Doug. We should take this for correctness. > Concerning testing, I have this running since some weeks on my ODROID-X2 > (Exynos4412), again, haven't encountered anything 'strange' yet. > > With best wishes, > Tobias > > Chirantan -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, just a short note that I still don't see this patch applied anywhere. Anything else I need to do here? With best wishes, Tobias -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tobias Jakobi wrote: > > Hello, > > just a short note that I still don't see this patch applied anywhere. > Anything else I need to do here? > Sorry about that and it would be handled by Daniel. Let me ping with adding him on your original patch. Thanks for your gentle reminder. - Kukjin -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tobias Jakobi wrote: > > EXYNOS4_MCT_L_MASK is defined as 0xffffff00, so applying this bitmask > produces a number outside the range 0x00 to 0xff, which always results > in execution of the default switch statement. > > Obviously this is wrong and git history shows that the bitmask inversion > was incorrectly set during a refactoring of the MCT code. > > Fix this by putting the inversion at the correct position again. > > Reported-by: GP Orcullo <kinsamanka@gmail.com> > Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> + Daniel, Thomas, adding Doug's review tag from previous his reply. Reviewed-by: Doug Anderson <dianders@chromium.org> And Acked-by: Kukjin Kim <kgene.kim@samsung.com> Daniel, Since this is obvious fix, can you please pick into your tree? If any problem, please kindly let me know. Thanks, Kukjin > --- > drivers/clocksource/exynos_mct.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c > index 9403061..83564c9 100644 > --- a/drivers/clocksource/exynos_mct.c > +++ b/drivers/clocksource/exynos_mct.c > @@ -97,8 +97,8 @@ static void exynos4_mct_write(unsigned int value, unsigned long offset) > writel_relaxed(value, reg_base + offset); > > if (likely(offset >= EXYNOS4_MCT_L_BASE(0))) { > - stat_addr = (offset & ~EXYNOS4_MCT_L_MASK) + MCT_L_WSTAT_OFFSET; > - switch (offset & EXYNOS4_MCT_L_MASK) { > + stat_addr = (offset & EXYNOS4_MCT_L_MASK) + MCT_L_WSTAT_OFFSET; > + switch (offset & ~EXYNOS4_MCT_L_MASK) { > case MCT_L_TCON_OFFSET: > mask = 1 << 3; /* L_TCON write status */ > break; > -- > 2.0.4 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/29/2014 11:18 PM, Kukjin Kim wrote: > Tobias Jakobi wrote: >> >> EXYNOS4_MCT_L_MASK is defined as 0xffffff00, so applying this bitmask >> produces a number outside the range 0x00 to 0xff, which always results >> in execution of the default switch statement. >> >> Obviously this is wrong and git history shows that the bitmask inversion >> was incorrectly set during a refactoring of the MCT code. >> >> Fix this by putting the inversion at the correct position again. >> >> Reported-by: GP Orcullo <kinsamanka@gmail.com> >> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> > > + Daniel, Thomas, > > adding Doug's review tag from previous his reply. > > Reviewed-by: Doug Anderson <dianders@chromium.org> > > And > > Acked-by: Kukjin Kim <kgene.kim@samsung.com> > > Daniel, > > Since this is obvious fix, can you please pick into your tree? > If any problem, please kindly let me know. Hi Kukjin, the patch is my tree for a 3.18 fix. Added the stable@ also in the Cc list. Thanks -- Daniel >> --- >> drivers/clocksource/exynos_mct.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c >> index 9403061..83564c9 100644 >> --- a/drivers/clocksource/exynos_mct.c >> +++ b/drivers/clocksource/exynos_mct.c >> @@ -97,8 +97,8 @@ static void exynos4_mct_write(unsigned int value, unsigned long offset) >> writel_relaxed(value, reg_base + offset); >> >> if (likely(offset >= EXYNOS4_MCT_L_BASE(0))) { >> - stat_addr = (offset & ~EXYNOS4_MCT_L_MASK) + MCT_L_WSTAT_OFFSET; >> - switch (offset & EXYNOS4_MCT_L_MASK) { >> + stat_addr = (offset & EXYNOS4_MCT_L_MASK) + MCT_L_WSTAT_OFFSET; >> + switch (offset & ~EXYNOS4_MCT_L_MASK) { >> case MCT_L_TCON_OFFSET: >> mask = 1 << 3; /* L_TCON write status */ >> break; >> -- >> 2.0.4 >
diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c index 9403061..83564c9 100644 --- a/drivers/clocksource/exynos_mct.c +++ b/drivers/clocksource/exynos_mct.c @@ -97,8 +97,8 @@ static void exynos4_mct_write(unsigned int value, unsigned long offset) writel_relaxed(value, reg_base + offset); if (likely(offset >= EXYNOS4_MCT_L_BASE(0))) { - stat_addr = (offset & ~EXYNOS4_MCT_L_MASK) + MCT_L_WSTAT_OFFSET; - switch (offset & EXYNOS4_MCT_L_MASK) { + stat_addr = (offset & EXYNOS4_MCT_L_MASK) + MCT_L_WSTAT_OFFSET; + switch (offset & ~EXYNOS4_MCT_L_MASK) { case MCT_L_TCON_OFFSET: mask = 1 << 3; /* L_TCON write status */ break;
EXYNOS4_MCT_L_MASK is defined as 0xffffff00, so applying this bitmask produces a number outside the range 0x00 to 0xff, which always results in execution of the default switch statement. Obviously this is wrong and git history shows that the bitmask inversion was incorrectly set during a refactoring of the MCT code. Fix this by putting the inversion at the correct position again. Reported-by: GP Orcullo <kinsamanka@gmail.com> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> --- drivers/clocksource/exynos_mct.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)