Re: [PATCH] clocksource: exynos_mct: fix exynos4_mct_write
On 11/29/2014 11:18 PM, Kukjin Kim wrote:
Tobias Jakobi wrote:
EXYNOS4_MCT_L_MASK is defined as 0xff00, 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 kinsama...@gmail.com
Signed-off-by: Tobias Jakobi tjak...@math.uni-bielefeld.de
+ Daniel, Thomas,
adding Doug's review tag from previous his reply.
Reviewed-by: Doug Anderson diand...@chromium.org
And
Acked-by: Kukjin Kim kgene@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
--
http://www.linaro.org/ Linaro.org │ Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro Facebook |
http://twitter.com/#!/linaroorg Twitter |
http://www.linaro.org/linaro-blog/ Blog
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clocksource: exynos_mct: fix exynos4_mct_write
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] clocksource: exynos_mct: fix exynos4_mct_write
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] clocksource: exynos_mct: fix exynos4_mct_write
Tobias Jakobi wrote:
EXYNOS4_MCT_L_MASK is defined as 0xff00, 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 kinsama...@gmail.com
Signed-off-by: Tobias Jakobi tjak...@math.uni-bielefeld.de
+ Daniel, Thomas,
adding Doug's review tag from previous his reply.
Reviewed-by: Doug Anderson diand...@chromium.org
And
Acked-by: Kukjin Kim kgene@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clocksource: exynos_mct: fix exynos4_mct_write
Tobias, On Tue, Oct 21, 2014 at 6:37 PM, Tobias Jakobi tjak...@math.uni-bielefeld.de wrote: EXYNOS4_MCT_L_MASK is defined as 0xff00, 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 kinsama...@gmail.com Signed-off-by: Tobias Jakobi tjak...@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 diand...@chromium.org -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clocksource: exynos_mct: fix exynos4_mct_write
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 tjak...@math.uni-bielefeld.de wrote: EXYNOS4_MCT_L_MASK is defined as 0xff00, 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 kinsama...@gmail.com Signed-off-by: Tobias Jakobi tjak...@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 diand...@chromium.org -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clocksource: exynos_mct: fix exynos4_mct_write
Hi Tobias, On Wed, Oct 22, 2014 at 9:12 AM, Tobias Jakobi tjak...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] clocksource: exynos_mct: fix exynos4_mct_write
EXYNOS4_MCT_L_MASK is defined as 0xff00, 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 kinsama...@gmail.com
Signed-off-by: Tobias Jakobi tjak...@math.uni-bielefeld.de
---
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
