[PATCH] clocksource: exynos_mct: fix exynos4_mct_write

[PATCH] clocksource: exynos_mct: fix exynos4_mct_write

[Tobias Jakobi]

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(-)

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

Re: [PATCH] clocksource: exynos_mct: fix exynos4_mct_write

[Doug Anderson]

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>

Re: [PATCH] clocksource: exynos_mct: fix exynos4_mct_write

[Tobias Jakobi]

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>

Re: [PATCH] clocksource: exynos_mct: fix exynos4_mct_write

[Chirantan Ekbote]

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

Re: [PATCH] clocksource: exynos_mct: fix exynos4_mct_write

[Tobias Jakobi]

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

RE: [PATCH] clocksource: exynos_mct: fix exynos4_mct_write

[Kukjin Kim]

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

RE: [PATCH] clocksource: exynos_mct: fix exynos4_mct_write

[Kukjin Kim]

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

Re: [PATCH] clocksource: exynos_mct: fix exynos4_mct_write

[Daniel Lezcano]

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
>


-- 
  <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