[PATCH] ARM:OMAP2: an issue about curr_pwrst, u8 is never < 0

Subsystems: arm port, omap2+ support, the rest

STALE4877d

3 messages, 2 authors, 2013-04-01 · open the first message on its own page

[PATCH] ARM:OMAP2: an issue about curr_pwrst, u8 is never < 0

From: Chen Gang <hidden>
Date: 2013-03-14 04:21:28

  if pwrdm_read_pwrst returns negative number, curr_pwrst can not notice it.
  since really need check curr_pwrst whether is negative,
    need let the check valid in _pwrdm_save_clkdm_state_and_activate.
    and also better to check the return value of pwrdm_read_pwrst, firstly.

Signed-off-by: Chen Gang <redacted>
---
 arch/arm/mach-omap2/powerdomain.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 43ac20c..876d067 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c

@@ -233,7 +233,7 @@ static u8 _pwrdm_save_clkdm_state_and_activate(struct powerdomain *pwrdm,
 {
 	u8 sleep_switch;
 
-	if (curr_pwrst < 0) {
+	if ((char)curr_pwrst < 0) {
 		WARN_ON(1);
 		sleep_switch = ERROR_SWITCH;
 	} else if (curr_pwrst < PWRDM_POWER_ON) {

@@ -1106,7 +1106,10 @@ int omap_set_pwrdm_state(struct powerdomain *pwrdm, u8 pwrst)
 
 	pwrdm_lock(pwrdm);
 
-	curr_pwrst = pwrdm_read_pwrst(pwrdm);
+	ret = pwrdm_read_pwrst(pwrdm);
+	if (ret < 0)
+		goto osps_out;
+	curr_pwrst = (u8)(unsigned int)ret;
 	next_pwrst = pwrdm_read_next_pwrst(pwrdm);
 	if (curr_pwrst == pwrst && next_pwrst == pwrst)
 		goto osps_out;

-- 
1.7.7.6

[PATCH] ARM:OMAP2: an issue about curr_pwrst, u8 is never < 0

From: paul@pwsan.com (Paul Walmsley)
Date: 2013-04-01 10:25:29

Hi,

On Thu, 14 Mar 2013, Chen Gang wrote:

  if pwrdm_read_pwrst returns negative number, curr_pwrst can not notice it.
  since really need check curr_pwrst whether is negative,
    need let the check valid in _pwrdm_save_clkdm_state_and_activate.
    and also better to check the return value of pwrdm_read_pwrst, firstly.

Signed-off-by: Chen Gang <redacted>

thanks for reporting this and sending a suggested fix.  The following 
patch is what I've queued here, which should deal with the root cause 
without the casts.

- Paul

From: Paul Walmsley <paul@pwsan.com>
Date: Sun, 31 Mar 2013 20:22:22 -0600
Subject: [PATCH] ARM: OMAP2+: powerdomain: avoid testing whether an unsigned
 char is less than 0

_pwrdm_save_clkdm_state_and_activate() tried to test one of its
unsigned arguments to determine whether it was less than zero.  Fix by
moving the error test to the caller.

Reported-by: Chen Gang <redacted>
Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
 arch/arm/mach-omap2/powerdomain.c |   18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 8e61d80..89cad4a 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c

@@ -52,7 +52,6 @@ enum {
 #define ALREADYACTIVE_SWITCH		0
 #define FORCEWAKEUP_SWITCH		1
 #define LOWPOWERSTATE_SWITCH		2
-#define ERROR_SWITCH			3
 
 /* pwrdm_list contains all registered struct powerdomains */
 static LIST_HEAD(pwrdm_list);

@@ -233,10 +232,7 @@ static u8 _pwrdm_save_clkdm_state_and_activate(struct powerdomain *pwrdm,
 {
 	u8 sleep_switch;
 
-	if (curr_pwrst < 0) {
-		WARN_ON(1);
-		sleep_switch = ERROR_SWITCH;
-	} else if (curr_pwrst < PWRDM_POWER_ON) {
+	if (curr_pwrst < PWRDM_POWER_ON) {
 		if (curr_pwrst > pwrst &&
 		    pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE &&
 		    arch_pwrdm->pwrdm_set_lowpwrstchange) {

@@ -1091,7 +1087,8 @@ int pwrdm_post_transition(struct powerdomain *pwrdm)
  */
 int omap_set_pwrdm_state(struct powerdomain *pwrdm, u8 pwrst)
 {
-	u8 curr_pwrst, next_pwrst, sleep_switch;
+	u8 next_pwrst, sleep_switch;
+	int curr_pwrst;
 	int ret = 0;
 	bool hwsup = false;

@@ -1107,16 +1104,17 @@ int omap_set_pwrdm_state(struct powerdomain *pwrdm, u8 pwrst)
 	pwrdm_lock(pwrdm);
 
 	curr_pwrst = pwrdm_read_pwrst(pwrdm);
+	if (curr_pwrst < 0) {
+		ret = -EINVAL;
+		goto osps_out;
+	}
+
 	next_pwrst = pwrdm_read_next_pwrst(pwrdm);
 	if (curr_pwrst == pwrst && next_pwrst == pwrst)
 		goto osps_out;
 
 	sleep_switch = _pwrdm_save_clkdm_state_and_activate(pwrdm, curr_pwrst,
 							    pwrst, &hwsup);
-	if (sleep_switch == ERROR_SWITCH) {
-		ret = -EINVAL;
-		goto osps_out;
-	}
 
 	ret = pwrdm_set_next_pwrst(pwrdm, pwrst);
 	if (ret)

-- 
1.7.10.4

[PATCH] ARM:OMAP2: an issue about curr_pwrst, u8 is never < 0

From: Chen Gang <hidden>
Date: 2013-04-01 10:41:11

On 2013?04?01? 18:25, Paul Walmsley wrote:

Hi,

On Thu, 14 Mar 2013, Chen Gang wrote:

quoted

  if pwrdm_read_pwrst returns negative number, curr_pwrst can not notice it.
  since really need check curr_pwrst whether is negative,
    need let the check valid in _pwrdm_save_clkdm_state_and_activate.
    and also better to check the return value of pwrdm_read_pwrst, firstly.

Signed-off-by: Chen Gang <redacted>

thanks for reporting this and sending a suggested fix.  The following 
patch is what I've queued here, which should deal with the root cause 
without the casts.

  yours (patch below) is simpler and clearer than mine.

  thanks.

  :-)

quoted hunk

- Paul

From: Paul Walmsley <paul@pwsan.com>
Date: Sun, 31 Mar 2013 20:22:22 -0600
Subject: [PATCH] ARM: OMAP2+: powerdomain: avoid testing whether an unsigned
 char is less than 0

_pwrdm_save_clkdm_state_and_activate() tried to test one of its
unsigned arguments to determine whether it was less than zero.  Fix by
moving the error test to the caller.

Reported-by: Chen Gang <redacted>
Signed-off-by: Paul Walmsley <paul@pwsan.com>
---
 arch/arm/mach-omap2/powerdomain.c |   18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 8e61d80..89cad4a 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c

@@ -52,7 +52,6 @@ enum {
 #define ALREADYACTIVE_SWITCH		0
 #define FORCEWAKEUP_SWITCH		1
 #define LOWPOWERSTATE_SWITCH		2
-#define ERROR_SWITCH			3
 
 /* pwrdm_list contains all registered struct powerdomains */
 static LIST_HEAD(pwrdm_list);

@@ -233,10 +232,7 @@ static u8 _pwrdm_save_clkdm_state_and_activate(struct powerdomain *pwrdm,
 {
 	u8 sleep_switch;
 
-	if (curr_pwrst < 0) {
-		WARN_ON(1);
-		sleep_switch = ERROR_SWITCH;
-	} else if (curr_pwrst < PWRDM_POWER_ON) {
+	if (curr_pwrst < PWRDM_POWER_ON) {
 		if (curr_pwrst > pwrst &&
 		    pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE &&
 		    arch_pwrdm->pwrdm_set_lowpwrstchange) {

@@ -1091,7 +1087,8 @@ int pwrdm_post_transition(struct powerdomain *pwrdm)
  */
 int omap_set_pwrdm_state(struct powerdomain *pwrdm, u8 pwrst)
 {
-	u8 curr_pwrst, next_pwrst, sleep_switch;
+	u8 next_pwrst, sleep_switch;
+	int curr_pwrst;
 	int ret = 0;
 	bool hwsup = false;

@@ -1107,16 +1104,17 @@ int omap_set_pwrdm_state(struct powerdomain *pwrdm, u8 pwrst)
 	pwrdm_lock(pwrdm);
 
 	curr_pwrst = pwrdm_read_pwrst(pwrdm);
+	if (curr_pwrst < 0) {
+		ret = -EINVAL;
+		goto osps_out;
+	}
+
 	next_pwrst = pwrdm_read_next_pwrst(pwrdm);
 	if (curr_pwrst == pwrst && next_pwrst == pwrst)
 		goto osps_out;
 
 	sleep_switch = _pwrdm_save_clkdm_state_and_activate(pwrdm, curr_pwrst,
 							    pwrst, &hwsup);
-	if (sleep_switch == ERROR_SWITCH) {
-		ret = -EINVAL;
-		goto osps_out;
-	}
 
 	ret = pwrdm_set_next_pwrst(pwrdm, pwrst);
 	if (ret)


-- 
Chen Gang

Asianux Corporation

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help