mfd: Support 88pm80x in 80x driver

DISCUSSION Dan Carpenter 3 participants 10 messages May 14, 2015 – Jun 5, 2015

Subsystems (2)

Kernel janitors Multifunction devices (MFD)

lore

Share & tools

lore

Dan Carpenter

author

2015-05-14 10:31 UTC #0 lore raw

Hello Qiao Zhou,

The patch 70c6cce04066: "mfd: Support 88pm80x in 80x driver" from Jul 9, 2012, leads to the following static checker warning:

	include/linux/mfd/88pm80x.h:352 pm80x_dev_suspend()
	warn: test_bit() takes a bit number

include/linux/mfd/88pm80x.h

   344  #ifdef CONFIG_PM
   345  static inline int pm80x_dev_suspend(struct device *dev)
   346  {
   347          struct platform_device *pdev = to_platform_device(dev);
   348          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
   349          int irq = platform_get_irq(pdev, 0);
   350  
   351          if (device_may_wakeup(dev))
   352                  set_bit((1 << irq), &chip->wu_flag);
                                ^^^^^^^^^

Smatch is complaining because it's doing a double left shift. If irq is larger than 5 then we are corrupting memory. Also we don't use ->wu_flag as a bitfield, we use it as a boolean so the name is confusing.

353
354 return 0;
355 }
356
357 static inline int pm80x_dev_resume(struct device *dev) 358 {

   359          struct platform_device *pdev = to_platform_device(dev);
   360          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
   361          int irq = platform_get_irq(pdev, 0);
   362  
   363          if (device_may_wakeup(dev))
   364                  clear_bit((1 << irq), &chip->wu_flag);
                                  ^^^^^^^^^^

Same issue.

365
366 return 0;
367 }
368 #endif

drivers/mfd/88pm80x.c

   133  #ifdef CONFIG_PM_SLEEP
   134  static int pm80x_suspend(struct device *dev)
   135  {
   136          struct i2c_client *client = container_of(dev, struct i2c_client, dev);
   137          struct pm80x_chip *chip = i2c_get_clientdata(client);
   138  
   139          if (chip && chip->wu_flag)
                            ^^^^^^^^^^^^^

Here it is used as a bool.

140 if (device_may_wakeup(chip->dev))
141 enable_irq_wake(chip->irq);
142
143 return 0;
144 }
145
146 static int pm80x_resume(struct device *dev) 147 {
148 struct i2c_client *client = container_of(dev, struct i2c_client, dev);
149 struct pm80x_chip *chip = i2c_get_clientdata(client);
150
151 if (chip && chip->wu_flag)

                            ^^^^^^^^^^^^^

This is the only other user.

   152                  if (device_may_wakeup(chip->dev))
   153                          disable_irq_wake(chip->irq);
   154  
   155          return 0;
   156  }
   157  #endif

regards,
dan carpenter

Qiao Zhou

2015-05-14 11:35 UTC #1 lore raw

Quoted 19 lines · Dan Carpenter

Hello Qiao Zhou,

The patch 70c6cce04066: "mfd: Support 88pm80x in 80x driver" from Jul 9, 2012, leads to the following static checker warning:

	include/linux/mfd/88pm80x.h:352 pm80x_dev_suspend()
	warn: test_bit() takes a bit number

include/linux/mfd/88pm80x.h

    344  #ifdef CONFIG_PM
    345  static inline int pm80x_dev_suspend(struct device *dev)
    346  {
    347          struct platform_device *pdev = to_platform_device(dev);
    348          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
    349          int irq = platform_get_irq(pdev, 0);
    350
    351          if (device_may_wakeup(dev))
    352                  set_bit((1 << irq), &chip->wu_flag);
                                 ^^^^^^^^^

Quoted 2 lines

Smatch is complaining because it's doing a double left shift. If irq is larger than 5 then we are corrupting memory. Also we don't use

Will fix this issue.

Quoted 32 lines

->wu_flag as a bitfield, we use it as a boolean so the name is confusing.

    353
    354          return 0;
    355  }
    356
    357  static inline int pm80x_dev_resume(struct device *dev)
    358  {
    359          struct platform_device *pdev = to_platform_device(dev);
    360          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
    361          int irq = platform_get_irq(pdev, 0);
    362
    363          if (device_may_wakeup(dev))
    364                  clear_bit((1 << irq), &chip->wu_flag);
                                   ^^^^^^^^^^

Same issue.

    365
    366          return 0;
    367  }
    368  #endif

drivers/mfd/88pm80x.c

    133  #ifdef CONFIG_PM_SLEEP
    134  static int pm80x_suspend(struct device *dev)
    135  {
    136          struct i2c_client *client = container_of(dev, struct i2c_client, dev);
    137          struct pm80x_chip *chip = i2c_get_clientdata(client);
    138
    139          if (chip && chip->wu_flag)
                             ^^^^^^^^^^^^^

Quoted 1 line

Here it is used as a bool.

It's designed in this way that sub device driver may use this flag. Also the bit value can tell which sub device sets the flag. However here we just check whether any bit is set.

Quoted 24 lines

    140                  if (device_may_wakeup(chip->dev))
    141                          enable_irq_wake(chip->irq);
    142
    143          return 0;
    144  }
    145
    146  static int pm80x_resume(struct device *dev)
    147  {
    148          struct i2c_client *client = container_of(dev, struct i2c_client, dev);
    149          struct pm80x_chip *chip = i2c_get_clientdata(client);
    150
    151          if (chip && chip->wu_flag)
                             ^^^^^^^^^^^^^

This is the only other user.

    152                  if (device_may_wakeup(chip->dev))
    153                          disable_irq_wake(chip->irq);
    154
    155          return 0;
    156  }
    157  #endif

Quoted 3 lines

regards,
dan carpenter

Dan,

Below is the patch to fix this issue. Please have a check and I'll submit an official patch to community after you reviewed. Thanks for finding this issue.

From 96486fda25414e3b926c275b951ac1408fae7830 Mon Sep 17 00:00:00 2001
From: Qiao Zhou <[email protected]>
Date: Thu, 14 May 2015 19:00:39 +0800
Subject: [PATCH] mfd: 88pm80x: refine irq bit operation

Set_bit/clear_bit for wu_flag may be corrupted if irq > 5(or 6 for aarch64). The maximum irq number from 88pm80x chip series is 24. Here we refine the code to protect the potential memory corruption.

Reported-by Dan Carpenter <[email protected]>

Signed-off-by Qiao Zhou <[email protected]>

---
  include/linux/mfd/88pm80x.h |   16 ++++++++++++++--
  1 file changed, 14 insertions(+), 2 deletions(-)

include/linux/mfd/88pm80x.h +14 -2

index 97cb283..a8c0318 100644
@@ -348,8 +348,14 @@ static inline int pm80x_dev_suspend(struct device *dev)
  	struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
  	int irq = platform_get_irq(pdev, 0);
 
+	if ((irq < 0) || (irq >= 24)) {
+		dev_err(dev, "pm80x: wrong irq 0x%x\n", irq);
+		/* return 0, and do not block suspend */
+		return 0;
+	}
+
  	if (device_may_wakeup(dev))
-		set_bit((1 << irq), &chip->wu_flag);
+		set_bit(irq, &chip->wu_flag);
 
  	return 0;
  }

@@ -360,8 +366,14 @@ static inline int pm80x_dev_resume(struct device *dev)
  	struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
  	int irq = platform_get_irq(pdev, 0);
 
+	if ((irq < 0) || (irq >= 24)) {
+		dev_err(dev, "pm80x: wrong irq 0x%x\n", irq);
+
+		return 0;
+	}
+
  	if (device_may_wakeup(dev))
-		clear_bit((1 << irq), &chip->wu_flag);
+		clear_bit(irq, &chip->wu_flag);
 
  	return 0;
  }

Signature · 6 lines

1.7.9.5


-- 
Best Regards
Qiao Zhou

Lee Jones

2015-05-14 12:50 UTC #2 lore raw

In reply to Dan Carpenter

Quoted 112 lines · Qiao Zhou

Quoted 19 lines · Dan Carpenter
Hello Qiao Zhou,
The patch 70c6cce04066: "mfd: Support 88pm80x in 80x driver" from Jul 9, 2012, leads to the following static checker warning:
	include/linux/mfd/88pm80x.h:352 pm80x_dev_suspend()
	warn: test_bit() takes a bit number
include/linux/mfd/88pm80x.h
   344  #ifdef CONFIG_PM
   345  static inline int pm80x_dev_suspend(struct device *dev)
   346  {
   347          struct platform_device *pdev = to_platform_device(dev);
   348          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
   349          int irq = platform_get_irq(pdev, 0);
   350
   351          if (device_may_wakeup(dev))
   352                  set_bit((1 << irq), &chip->wu_flag);
                                ^^^^^^^^^
Quoted 2 lines
Smatch is complaining because it's doing a double left shift. If irq is larger than 5 then we are corrupting memory. Also we don't use
Will fix this issue.
Quoted 32 lines
->wu_flag as a bitfield, we use it as a boolean so the name is confusing.
   353
   354          return 0;
   355  }
   356
   357  static inline int pm80x_dev_resume(struct device *dev)
   358  {
   359          struct platform_device *pdev = to_platform_device(dev);
   360          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
   361          int irq = platform_get_irq(pdev, 0);
   362
   363          if (device_may_wakeup(dev))
   364                  clear_bit((1 << irq), &chip->wu_flag);
                                  ^^^^^^^^^^
Same issue.
   365
   366          return 0;
   367  }
   368  #endif
drivers/mfd/88pm80x.c
   133  #ifdef CONFIG_PM_SLEEP
   134  static int pm80x_suspend(struct device *dev)
   135  {
   136          struct i2c_client *client = container_of(dev, struct i2c_client, dev);
   137          struct pm80x_chip *chip = i2c_get_clientdata(client);
   138
   139          if (chip && chip->wu_flag)
                            ^^^^^^^^^^^^^
Quoted 1 line
Here it is used as a bool.
It's designed in this way that sub device driver may use this flag. Also the bit value can tell which sub device sets the flag. However here we just check whether any bit is set.
Quoted 24 lines
   140                  if (device_may_wakeup(chip->dev))
   141                          enable_irq_wake(chip->irq);
   142
   143          return 0;
   144  }
   145
   146  static int pm80x_resume(struct device *dev)
   147  {
   148          struct i2c_client *client = container_of(dev, struct i2c_client, dev);
   149          struct pm80x_chip *chip = i2c_get_clientdata(client);
   150
   151          if (chip && chip->wu_flag)
                            ^^^^^^^^^^^^^
This is the only other user.
   152                  if (device_may_wakeup(chip->dev))
   153                          disable_irq_wake(chip->irq);
   154
   155          return 0;
   156  }
   157  #endif
Quoted 3 lines
regards,
dan carpenter
Dan,
Below is the patch to fix this issue. Please have a check and I'll submit an official patch to community after you reviewed. Thanks for finding this issue.
From 96486fda25414e3b926c275b951ac1408fae7830 Mon Sep 17 00:00:00 2001
From: Qiao Zhou <[email protected]>
Date: Thu, 14 May 2015 19:00:39 +0800
Subject: [PATCH] mfd: 88pm80x: refine irq bit operation
Set_bit/clear_bit for wu_flag may be corrupted if irq > 5(or 6 for aarch64). The maximum irq number from 88pm80x chip series is 24. Here we refine the code to protect the potential memory corruption.
Reported-by Dan Carpenter <[email protected]>
Signed-off-by Qiao Zhou <[email protected]>
---
 include/linux/mfd/88pm80x.h |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)
include/linux/mfd/88pm80x.h +0 -0
index 97cb283..a8c0318 100644

include/linux/mfd/88pm80x.h static inline int pm80x_dev_suspend(struct device *dev) · line 351

Quoted 4 lines

include/linux/mfd/88pm80x.h +1 -0

@@ -348,8 +348,14 @@ static inline int pm80x_dev_suspend(struct device *dev)
 	struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
 	int irq = platform_get_irq(pdev, 0);
 
+	if ((irq < 0) || (irq >= 24)) {

                           irq > 23

Or, even better:

#include PM80X_MAX_IRQS 23

	if (irq < 0 || irq > PM80X_MAX_IRQS) {

Drop the parentheses.

include/linux/mfd/88pm80x.h static inline int pm80x_dev_suspend(struct device *dev) · line 352

Quoted 1 line

include/linux/mfd/88pm80x.h +1 -0
+		dev_err(dev, "pm80x: wrong irq 0x%x\n", irq);

No need to put pm80x, dev_err() will do that for you.

s/wrong irq/Invalid IRQ/

Is it really better in hex?

include/linux/mfd/88pm80x.h static inline int pm80x_dev_suspend(struct device *dev) · line 353

Quoted 1 line

include/linux/mfd/88pm80x.h +1 -0
+		/* return 0, and do not block suspend */

This comment is not required.

include/linux/mfd/88pm80x.h static inline int pm80x_dev_suspend(struct device *dev) · line 358

Quoted 6 lines

include/linux/mfd/88pm80x.h +4 -1

+		return 0;
+	}
+
 	if (device_may_wakeup(dev))
-		set_bit((1 << irq), &chip->wu_flag);
+		set_bit(irq, &chip->wu_flag);

Can you come up with a better name?

include/linux/mfd/88pm80x.h static inline int pm80x_dev_resume(struct device *dev) · line 371

Quoted 9 lines

include/linux/mfd/88pm80x.h +3 -0

 	return 0;
 }

@@ -360,8 +366,14 @@ static inline int pm80x_dev_resume(struct device *dev)
 	struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
 	int irq = platform_get_irq(pdev, 0);
 
+	if ((irq < 0) || (irq >= 24)) {
+		dev_err(dev, "pm80x: wrong irq 0x%x\n", irq);
+

Superfluous '\n'.

Quoted 9 lines

include/linux/mfd/88pm80x.h +4 -1

+		return 0;
+	}
+
 	if (device_may_wakeup(dev))
-		clear_bit((1 << irq), &chip->wu_flag);
+		clear_bit(irq, &chip->wu_flag);
 
 	return 0;
 }

Signature · 8 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â”‚ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Qiao Zhou

2015-05-15 01:06 UTC #3 lore raw

In reply to Dan Carpenter

Quoted 121 lines · Lee Jones

Quoted 112 lines · Qiao Zhou
Quoted 19 lines · Dan Carpenter
Hello Qiao Zhou,
The patch 70c6cce04066: "mfd: Support 88pm80x in 80x driver" from Jul 9, 2012, leads to the following static checker warning:
	include/linux/mfd/88pm80x.h:352 pm80x_dev_suspend()
	warn: test_bit() takes a bit number
include/linux/mfd/88pm80x.h
    344  #ifdef CONFIG_PM
    345  static inline int pm80x_dev_suspend(struct device *dev)
    346  {
    347          struct platform_device *pdev = to_platform_device(dev);
    348          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
    349          int irq = platform_get_irq(pdev, 0);
    350
    351          if (device_may_wakeup(dev))
    352                  set_bit((1 << irq), &chip->wu_flag);
                                 ^^^^^^^^^
Quoted 2 lines
Smatch is complaining because it's doing a double left shift. If irq is larger than 5 then we are corrupting memory. Also we don't use
Will fix this issue.
Quoted 32 lines
->wu_flag as a bitfield, we use it as a boolean so the name is confusing.
    353
    354          return 0;
    355  }
    356
    357  static inline int pm80x_dev_resume(struct device *dev)
    358  {
    359          struct platform_device *pdev = to_platform_device(dev);
    360          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
    361          int irq = platform_get_irq(pdev, 0);
    362
    363          if (device_may_wakeup(dev))
    364                  clear_bit((1 << irq), &chip->wu_flag);
                                   ^^^^^^^^^^
Same issue.
    365
    366          return 0;
    367  }
    368  #endif
drivers/mfd/88pm80x.c
    133  #ifdef CONFIG_PM_SLEEP
    134  static int pm80x_suspend(struct device *dev)
    135  {
    136          struct i2c_client *client = container_of(dev, struct i2c_client, dev);
    137          struct pm80x_chip *chip = i2c_get_clientdata(client);
    138
    139          if (chip && chip->wu_flag)
                             ^^^^^^^^^^^^^
Quoted 1 line
Here it is used as a bool.
It's designed in this way that sub device driver may use this flag. Also the bit value can tell which sub device sets the flag. However here we just check whether any bit is set.
Quoted 24 lines
    140                  if (device_may_wakeup(chip->dev))
    141                          enable_irq_wake(chip->irq);
    142
    143          return 0;
    144  }
    145
    146  static int pm80x_resume(struct device *dev)
    147  {
    148          struct i2c_client *client = container_of(dev, struct i2c_client, dev);
    149          struct pm80x_chip *chip = i2c_get_clientdata(client);
    150
    151          if (chip && chip->wu_flag)
                             ^^^^^^^^^^^^^
This is the only other user.
    152                  if (device_may_wakeup(chip->dev))
    153                          disable_irq_wake(chip->irq);
    154
    155          return 0;
    156  }
    157  #endif
Quoted 3 lines
regards,
dan carpenter
Dan,
Below is the patch to fix this issue. Please have a check and I'll submit an official patch to community after you reviewed. Thanks for finding this issue.
From 96486fda25414e3b926c275b951ac1408fae7830 Mon Sep 17 00:00:00 2001
From: Qiao Zhou <[email protected]>
Date: Thu, 14 May 2015 19:00:39 +0800
Subject: [PATCH] mfd: 88pm80x: refine irq bit operation
Set_bit/clear_bit for wu_flag may be corrupted if irq > 5(or 6 for aarch64). The maximum irq number from 88pm80x chip series is 24. Here we refine the code to protect the potential memory corruption.
Reported-by Dan Carpenter <[email protected]>
Signed-off-by Qiao Zhou <[email protected]>
---
  include/linux/mfd/88pm80x.h |   16 ++++++++++++++--
  1 file changed, 14 insertions(+), 2 deletions(-)
include/linux/mfd/88pm80x.h +0 -0
index 97cb283..a8c0318 100644
include/linux/mfd/88pm80x.h static inline int pm80x_dev_suspend(struct device *dev) · line 351
Quoted 4 lines
include/linux/mfd/88pm80x.h +1 -0
@@ -348,8 +348,14 @@ static inline int pm80x_dev_suspend(struct device *dev)
  	struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
  	int irq = platform_get_irq(pdev, 0);
 
+	if ((irq < 0) || (irq >= 24)) {
                            irq > 23

Quoted 7 lines

Or, even better:
#include PM80X_MAX_IRQS 23
	if (irq < 0 || irq > PM80X_MAX_IRQS) {
Drop the parentheses.

Will refine it.

Quoted 5 lines

include/linux/mfd/88pm80x.h static inline int pm80x_dev_suspend(struct device *dev) · line 352
Quoted 2 lines
include/linux/mfd/88pm80x.h +1 -0
+		dev_err(dev, "pm80x: wrong irq 0x%x\n", irq);
No need to put pm80x, dev_err() will do that for you.

Quoted 7 lines

s/wrong irq/Invalid IRQ/
Is it really better in hex?
include/linux/mfd/88pm80x.h static inline int pm80x_dev_suspend(struct device *dev) · line 353
Quoted 1 line
include/linux/mfd/88pm80x.h +1 -0
+		/* return 0, and do not block suspend */
This comment is not required.

Will refine it.

Quoted 9 lines

include/linux/mfd/88pm80x.h static inline int pm80x_dev_suspend(struct device *dev) · line 358
Quoted 7 lines
include/linux/mfd/88pm80x.h +4 -1
+		return 0;
+	}
+
  	if (device_may_wakeup(dev))
-		set_bit((1 << irq), &chip->wu_flag);
+		set_bit(irq, &chip->wu_flag);
Can you come up with a better name?

It's short for wakeup flag. I may change it to wakeup_flag. Do you have any suggestion if it's not okay?

Quoted 7 lines

include/linux/mfd/88pm80x.h +0 -0

  	return 0;
  }

@@ -360,8 +366,14 @@ static inline int pm80x_dev_resume(struct device *dev)
  	struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
  	int irq = platform_get_irq(pdev, 0);

Quoted 5 lines

include/linux/mfd/88pm80x.h static inline int pm80x_dev_resume(struct device *dev) · line 371
Quoted 3 lines
include/linux/mfd/88pm80x.h +3 -0
+	if ((irq < 0) || (irq >= 24)) {
+		dev_err(dev, "pm80x: wrong irq 0x%x\n", irq);
+
Superfluous '\n'.

Will refine it.

Quoted 11 lines

include/linux/mfd/88pm80x.h static inline int pm80x_dev_resume(struct device *dev) · line 379
Quoted 10 lines
include/linux/mfd/88pm80x.h +4 -1
+		return 0;
+	}
+
  	if (device_may_wakeup(dev))
-		clear_bit((1 << irq), &chip->wu_flag);
+		clear_bit(irq, &chip->wu_flag);
 
  	return 0;
  }

Jones,

Thanks a lot for your review & suggestions.

Signature · 2 lines

Best Regards
Qiao Zhou

Dan Carpenter

author

2015-05-15 08:23 UTC #4 lore raw

In reply to Dan Carpenter

Quoted 10 lines · Qiao Zhou

Quoted 9 lines
Review static inline int pm80x_dev_resume(struct device *dev) · line 371
Quoted 7 lines
(quoted diff) +3 -0
@@ -360,8 +366,14 @@ static inline int pm80x_dev_resume(struct device *dev)
 	struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
 	int irq = platform_get_irq(pdev, 0);
 
+	if ((irq < 0) || (irq >= 24)) {
+		dev_err(dev, "pm80x: wrong irq 0x%x\n", irq);
+
Superfluous '\n'.
Will refine it.

The \n is ok isn't it?

regards,
dan carpenter

Lee Jones

2015-05-18 09:23 UTC #5 lore raw

In reply to Dan Carpenter

Quoted 114 lines · Qiao Zhou

Quoted 113 lines · Lee Jones
Quoted 111 lines · Qiao Zhou
Quoted 19 lines · Dan Carpenter
Hello Qiao Zhou,
The patch 70c6cce04066: "mfd: Support 88pm80x in 80x driver" from Jul 9, 2012, leads to the following static checker warning:
	include/linux/mfd/88pm80x.h:352 pm80x_dev_suspend()
	warn: test_bit() takes a bit number
include/linux/mfd/88pm80x.h
   344  #ifdef CONFIG_PM
   345  static inline int pm80x_dev_suspend(struct device *dev)
   346  {
   347          struct platform_device *pdev = to_platform_device(dev);
   348          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
   349          int irq = platform_get_irq(pdev, 0);
   350
   351          if (device_may_wakeup(dev))
   352                  set_bit((1 << irq), &chip->wu_flag);
                                ^^^^^^^^^
Quoted 2 lines
Smatch is complaining because it's doing a double left shift. If irq is larger than 5 then we are corrupting memory. Also we don't use
Will fix this issue.
Quoted 32 lines
->wu_flag as a bitfield, we use it as a boolean so the name is confusing.
   353
   354          return 0;
   355  }
   356
   357  static inline int pm80x_dev_resume(struct device *dev)
   358  {
   359          struct platform_device *pdev = to_platform_device(dev);
   360          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
   361          int irq = platform_get_irq(pdev, 0);
   362
   363          if (device_may_wakeup(dev))
   364                  clear_bit((1 << irq), &chip->wu_flag);
                                  ^^^^^^^^^^
Same issue.
   365
   366          return 0;
   367  }
   368  #endif
drivers/mfd/88pm80x.c
   133  #ifdef CONFIG_PM_SLEEP
   134  static int pm80x_suspend(struct device *dev)
   135  {
   136          struct i2c_client *client = container_of(dev, struct i2c_client, dev);
   137          struct pm80x_chip *chip = i2c_get_clientdata(client);
   138
   139          if (chip && chip->wu_flag)
                            ^^^^^^^^^^^^^
Quoted 1 line
Here it is used as a bool.
It's designed in this way that sub device driver may use this flag. Also the bit value can tell which sub device sets the flag. However here we just check whether any bit is set.
Quoted 24 lines
   140                  if (device_may_wakeup(chip->dev))
   141                          enable_irq_wake(chip->irq);
   142
   143          return 0;
   144  }
   145
   146  static int pm80x_resume(struct device *dev)
   147  {
   148          struct i2c_client *client = container_of(dev, struct i2c_client, dev);
   149          struct pm80x_chip *chip = i2c_get_clientdata(client);
   150
   151          if (chip && chip->wu_flag)
                            ^^^^^^^^^^^^^
This is the only other user.
   152                  if (device_may_wakeup(chip->dev))
   153                          disable_irq_wake(chip->irq);
   154
   155          return 0;
   156  }
   157  #endif
Quoted 3 lines
regards,
dan carpenter
Dan,
Below is the patch to fix this issue. Please have a check and I'll submit an official patch to community after you reviewed. Thanks for finding this issue.
From 96486fda25414e3b926c275b951ac1408fae7830 Mon Sep 17 00:00:00 2001
From: Qiao Zhou <[email protected]>
Date: Thu, 14 May 2015 19:00:39 +0800
Subject: [PATCH] mfd: 88pm80x: refine irq bit operation
Set_bit/clear_bit for wu_flag may be corrupted if irq > 5(or 6 for aarch64). The maximum irq number from 88pm80x chip series is 24. Here we refine the code to protect the potential memory corruption.
Reported-by Dan Carpenter <[email protected]>
Signed-off-by Qiao Zhou <[email protected]>
---
 include/linux/mfd/88pm80x.h |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)
include/linux/mfd/88pm80x.h +0 -0
index 97cb283..a8c0318 100644

[...]

Quoted 7 lines

Quoted 5 lines
include/linux/mfd/88pm80x.h
Quoted 3 lines
include/linux/mfd/88pm80x.h +1 -1
 	if (device_may_wakeup(dev))
-		set_bit((1 << irq), &chip->wu_flag);
+		set_bit(irq, &chip->wu_flag);
Can you come up with a better name?
It's short for wakeup flag. I may change it to wakeup_flag. Do you have any suggestion if it's not okay?

bool wakeup; will be fine.

Signature · 8 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â”‚ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lee Jones

2015-05-18 09:27 UTC #6 lore raw

In reply to Dan Carpenter

Quoted 5 lines · Dan Carpenter

Quoted 4 lines · Qiao Zhou

Quoted 4 lines

(quoted diff) +0 -0

@@ -360,8 +366,14 @@ static inline int pm80x_dev_resume(struct device *dev)
 	struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
 	int irq = platform_get_irq(pdev, 0);

Quoted 8 lines

Quoted 6 lines
Quoted 5 lines
Review static inline int pm80x_dev_resume(struct device *dev) · line 371
Quoted 3 lines
(quoted diff) +3 -0
+	if ((irq < 0) || (irq >= 24)) {
+		dev_err(dev, "pm80x: wrong irq 0x%x\n", irq);
+
Superfluous '\n'.
Will refine it.
The \n is ok isn't it?

It's not a blocker, but it is superfluous:

git grep -A2 dev_err

Signature · 8 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â”‚ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

zhouqiao

2015-06-05 02:17 UTC #7 lore raw

In reply to Dan Carpenter

Quoted 117 lines · Lee Jones

Quoted 113 lines · Qiao Zhou
Quoted 112 lines · Lee Jones
Quoted 110 lines · Qiao Zhou
Quoted 19 lines · Dan Carpenter
Hello Qiao Zhou,
The patch 70c6cce04066: "mfd: Support 88pm80x in 80x driver" from Jul 9, 2012, leads to the following static checker warning:
	include/linux/mfd/88pm80x.h:352 pm80x_dev_suspend()
	warn: test_bit() takes a bit number
include/linux/mfd/88pm80x.h
    344  #ifdef CONFIG_PM
    345  static inline int pm80x_dev_suspend(struct device *dev)
    346  {
    347          struct platform_device *pdev = to_platform_device(dev);
    348          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
    349          int irq = platform_get_irq(pdev, 0);
    350
    351          if (device_may_wakeup(dev))
    352                  set_bit((1 << irq), &chip->wu_flag);
                                 ^^^^^^^^^
Quoted 2 lines
Smatch is complaining because it's doing a double left shift. If irq is larger than 5 then we are corrupting memory. Also we don't use
Will fix this issue.
Quoted 32 lines
->wu_flag as a bitfield, we use it as a boolean so the name is confusing.
    353
    354          return 0;
    355  }
    356
    357  static inline int pm80x_dev_resume(struct device *dev)
    358  {
    359          struct platform_device *pdev = to_platform_device(dev);
    360          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
    361          int irq = platform_get_irq(pdev, 0);
    362
    363          if (device_may_wakeup(dev))
    364                  clear_bit((1 << irq), &chip->wu_flag);
                                   ^^^^^^^^^^
Same issue.
    365
    366          return 0;
    367  }
    368  #endif
drivers/mfd/88pm80x.c
    133  #ifdef CONFIG_PM_SLEEP
    134  static int pm80x_suspend(struct device *dev)
    135  {
    136          struct i2c_client *client = container_of(dev, struct i2c_client, dev);
    137          struct pm80x_chip *chip = i2c_get_clientdata(client);
    138
    139          if (chip && chip->wu_flag)
                             ^^^^^^^^^^^^^
Quoted 1 line
Here it is used as a bool.
It's designed in this way that sub device driver may use this flag. Also the bit value can tell which sub device sets the flag. However here we just check whether any bit is set.
Quoted 23 lines
    140                  if (device_may_wakeup(chip->dev))
    141                          enable_irq_wake(chip->irq);
    142
    143          return 0;
    144  }
    145
    146  static int pm80x_resume(struct device *dev)
    147  {
    148          struct i2c_client *client = container_of(dev, struct i2c_client, dev);
    149          struct pm80x_chip *chip = i2c_get_clientdata(client);
    150
    151          if (chip && chip->wu_flag)
                             ^^^^^^^^^^^^^
This is the only other user.
    152                  if (device_may_wakeup(chip->dev))
    153                          disable_irq_wake(chip->irq);
    154
    155          return 0;
    156  }
    157  #endif
Quoted 3 lines
regards,
dan carpenter
Dan,
Below is the patch to fix this issue. Please have a check and I'll submit an official patch to community after you reviewed. Thanks for finding this issue.
From 96486fda25414e3b926c275b951ac1408fae7830 Mon Sep 17 00:00:00 2001
From: Qiao Zhou <[email protected]>
Date: Thu, 14 May 2015 19:00:39 +0800
Subject: [PATCH] mfd: 88pm80x: refine irq bit operation
Set_bit/clear_bit for wu_flag may be corrupted if irq > 5(or 6 for aarch64). The maximum irq number from 88pm80x chip series is 24. Here we refine the code to protect the potential memory corruption.
Reported-by Dan Carpenter <[email protected]>
Signed-off-by Qiao Zhou <[email protected]>
---
  include/linux/mfd/88pm80x.h |   16 ++++++++++++++--
  1 file changed, 14 insertions(+), 2 deletions(-)
include/linux/mfd/88pm80x.h +0 -0
index 97cb283..a8c0318 100644
[...]

Quoted 7 lines

Quoted 6 lines
Quoted 4 lines
include/linux/mfd/88pm80x.h
Quoted 3 lines
if (device_may_wakeup(dev))
include/linux/mfd/88pm80x.h +1 -1
-		set_bit((1 << irq), &chip->wu_flag);
+		set_bit(irq, &chip->wu_flag);
Can you come up with a better name?
It's short for wakeup flag. I may change it to wakeup_flag. Do you have any suggestion if it's not okay?
bool wakeup; will be fine.

Would it be OK to only change the same? The original purpose to use bit control is that
MFD driver may check the bits so that it knows which sub-dev is doing something special.
However, it's not pushed into mainline yet. Another concern is that Currently I've no
platform to verify the change to change the variable type.

Quoted 1 line

Sorry for late response. I've been trapped in some urgent issues these days.

Signature · 2 lines

Best Regards
Qiao

Lee Jones

2015-06-05 07:27 UTC #8 lore raw

In reply to Dan Carpenter

Quoted 124 lines · zhouqiao

Quoted 123 lines · Lee Jones
Quoted 113 lines · Qiao Zhou
Quoted 112 lines · Lee Jones
Quoted 110 lines · Qiao Zhou
Quoted 19 lines · Dan Carpenter
Hello Qiao Zhou,
The patch 70c6cce04066: "mfd: Support 88pm80x in 80x driver" from Jul 9, 2012, leads to the following static checker warning:
	include/linux/mfd/88pm80x.h:352 pm80x_dev_suspend()
	warn: test_bit() takes a bit number
include/linux/mfd/88pm80x.h
   344  #ifdef CONFIG_PM
   345  static inline int pm80x_dev_suspend(struct device *dev)
   346  {
   347          struct platform_device *pdev = to_platform_device(dev);
   348          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
   349          int irq = platform_get_irq(pdev, 0);
   350
   351          if (device_may_wakeup(dev))
   352                  set_bit((1 << irq), &chip->wu_flag);
                                ^^^^^^^^^
Quoted 2 lines
Smatch is complaining because it's doing a double left shift. If irq is larger than 5 then we are corrupting memory. Also we don't use
Will fix this issue.
Quoted 32 lines
->wu_flag as a bitfield, we use it as a boolean so the name is confusing.
   353
   354          return 0;
   355  }
   356
   357  static inline int pm80x_dev_resume(struct device *dev)
   358  {
   359          struct platform_device *pdev = to_platform_device(dev);
   360          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
   361          int irq = platform_get_irq(pdev, 0);
   362
   363          if (device_may_wakeup(dev))
   364                  clear_bit((1 << irq), &chip->wu_flag);
                                  ^^^^^^^^^^
Same issue.
   365
   366          return 0;
   367  }
   368  #endif
drivers/mfd/88pm80x.c
   133  #ifdef CONFIG_PM_SLEEP
   134  static int pm80x_suspend(struct device *dev)
   135  {
   136          struct i2c_client *client = container_of(dev, struct i2c_client, dev);
   137          struct pm80x_chip *chip = i2c_get_clientdata(client);
   138
   139          if (chip && chip->wu_flag)
                            ^^^^^^^^^^^^^
Quoted 1 line
Here it is used as a bool.
It's designed in this way that sub device driver may use this flag. Also the bit value can tell which sub device sets the flag. However here we just check whether any bit is set.
Quoted 23 lines
   140                  if (device_may_wakeup(chip->dev))
   141                          enable_irq_wake(chip->irq);
   142
   143          return 0;
   144  }
   145
   146  static int pm80x_resume(struct device *dev)
   147  {
   148          struct i2c_client *client = container_of(dev, struct i2c_client, dev);
   149          struct pm80x_chip *chip = i2c_get_clientdata(client);
   150
   151          if (chip && chip->wu_flag)
                            ^^^^^^^^^^^^^
This is the only other user.
   152                  if (device_may_wakeup(chip->dev))
   153                          disable_irq_wake(chip->irq);
   154
   155          return 0;
   156  }
   157  #endif
Quoted 3 lines
regards,
dan carpenter
Dan,
Below is the patch to fix this issue. Please have a check and I'll submit an official patch to community after you reviewed. Thanks for finding this issue.
From 96486fda25414e3b926c275b951ac1408fae7830 Mon Sep 17 00:00:00 2001
From: Qiao Zhou <[email protected]>
Date: Thu, 14 May 2015 19:00:39 +0800
Subject: [PATCH] mfd: 88pm80x: refine irq bit operation
Set_bit/clear_bit for wu_flag may be corrupted if irq > 5(or 6 for aarch64). The maximum irq number from 88pm80x chip series is 24. Here we refine the code to protect the potential memory corruption.
Reported-by Dan Carpenter <[email protected]>
Signed-off-by Qiao Zhou <[email protected]>
---
 include/linux/mfd/88pm80x.h |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)
include/linux/mfd/88pm80x.h +0 -0
index 97cb283..a8c0318 100644
[...]
Quoted 6 lines
Quoted 4 lines
include/linux/mfd/88pm80x.h
Quoted 3 lines
include/linux/mfd/88pm80x.h +1 -1
 	if (device_may_wakeup(dev))
-		set_bit((1 << irq), &chip->wu_flag);
+		set_bit(irq, &chip->wu_flag);
Can you come up with a better name?
It's short for wakeup flag. I may change it to wakeup_flag. Do you have any suggestion if it's not okay?

Quoted 8 lines

Quoted 1 line
bool wakeup; will be fine.
Would it be OK to only change the same? The original purpose to use bit control is that
MFD driver may check the bits so that it knows which sub-dev is doing something special.
However, it's not pushed into mainline yet. Another concern is that Currently I've no
platform to verify the change to change the variable type.

Fine, as long as you promise to attend to it in the future.

Quoted 1 line

Sorry for late response. I've been trapped in some urgent issues these days.

Signature · 8 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â”‚ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

zhouqiao

2015-06-05 07:49 UTC #9 lore raw

In reply to Dan Carpenter

Quoted 127 lines · Lee Jones

Quoted 125 lines · zhouqiao
Quoted 124 lines · Lee Jones
Quoted 113 lines · Qiao Zhou
Quoted 112 lines · Lee Jones
Quoted 110 lines · Qiao Zhou
Quoted 19 lines · Dan Carpenter
Hello Qiao Zhou,
The patch 70c6cce04066: "mfd: Support 88pm80x in 80x driver" from Jul 9, 2012, leads to the following static checker warning:
	include/linux/mfd/88pm80x.h:352 pm80x_dev_suspend()
	warn: test_bit() takes a bit number
include/linux/mfd/88pm80x.h
    344  #ifdef CONFIG_PM
    345  static inline int pm80x_dev_suspend(struct device *dev)
    346  {
    347          struct platform_device *pdev = to_platform_device(dev);
    348          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
    349          int irq = platform_get_irq(pdev, 0);
    350
    351          if (device_may_wakeup(dev))
    352                  set_bit((1 << irq), &chip->wu_flag);
                                 ^^^^^^^^^
Quoted 2 lines
Smatch is complaining because it's doing a double left shift. If irq is larger than 5 then we are corrupting memory. Also we don't use
Will fix this issue.
Quoted 32 lines
->wu_flag as a bitfield, we use it as a boolean so the name is confusing.
    353
    354          return 0;
    355  }
    356
    357  static inline int pm80x_dev_resume(struct device *dev)
    358  {
    359          struct platform_device *pdev = to_platform_device(dev);
    360          struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
    361          int irq = platform_get_irq(pdev, 0);
    362
    363          if (device_may_wakeup(dev))
    364                  clear_bit((1 << irq), &chip->wu_flag);
                                   ^^^^^^^^^^
Same issue.
    365
    366          return 0;
    367  }
    368  #endif
drivers/mfd/88pm80x.c
    133  #ifdef CONFIG_PM_SLEEP
    134  static int pm80x_suspend(struct device *dev)
    135  {
    136          struct i2c_client *client = container_of(dev, struct i2c_client, dev);
    137          struct pm80x_chip *chip = i2c_get_clientdata(client);
    138
    139          if (chip && chip->wu_flag)
                             ^^^^^^^^^^^^^
Quoted 1 line
Here it is used as a bool.
It's designed in this way that sub device driver may use this flag. Also the bit value can tell which sub device sets the flag. However here we just check whether any bit is set.
Quoted 23 lines
    140                  if (device_may_wakeup(chip->dev))
    141                          enable_irq_wake(chip->irq);
    142
    143          return 0;
    144  }
    145
    146  static int pm80x_resume(struct device *dev)
    147  {
    148          struct i2c_client *client = container_of(dev, struct i2c_client, dev);
    149          struct pm80x_chip *chip = i2c_get_clientdata(client);
    150
    151          if (chip && chip->wu_flag)
                             ^^^^^^^^^^^^^
This is the only other user.
    152                  if (device_may_wakeup(chip->dev))
    153                          disable_irq_wake(chip->irq);
    154
    155          return 0;
    156  }
    157  #endif
Quoted 3 lines
regards,
dan carpenter
Dan,
Below is the patch to fix this issue. Please have a check and I'll submit an official patch to community after you reviewed. Thanks for finding this issue.
From 96486fda25414e3b926c275b951ac1408fae7830 Mon Sep 17 00:00:00 2001
From: Qiao Zhou <[email protected]>
Date: Thu, 14 May 2015 19:00:39 +0800
Subject: [PATCH] mfd: 88pm80x: refine irq bit operation
Set_bit/clear_bit for wu_flag may be corrupted if irq > 5(or 6 for aarch64). The maximum irq number from 88pm80x chip series is 24. Here we refine the code to protect the potential memory corruption.
Reported-by Dan Carpenter <[email protected]>
Signed-off-by Qiao Zhou <[email protected]>
---
  include/linux/mfd/88pm80x.h |   16 ++++++++++++++--
  1 file changed, 14 insertions(+), 2 deletions(-)
include/linux/mfd/88pm80x.h +0 -0
index 97cb283..a8c0318 100644
[...]
Quoted 6 lines
Quoted 4 lines
include/linux/mfd/88pm80x.h
Quoted 3 lines
if (device_may_wakeup(dev))
include/linux/mfd/88pm80x.h +1 -1
-		set_bit((1 << irq), &chip->wu_flag);
+		set_bit(irq, &chip->wu_flag);
Can you come up with a better name?
It's short for wakeup flag. I may change it to wakeup_flag. Do you have any suggestion if it's not okay?
bool wakeup; will be fine.

Quoted 8 lines

Quoted 7 lines
Would it be OK to only change the same? The original purpose to use bit control is that
MFD driver may check the bits so that it knows which sub-dev is doing something special.
However, it's not pushed into mainline yet. Another concern is that Currently I've no
platform to verify the change to change the variable type.
Fine, as long as you promise to attend to it in the future.

OK. Thanks.

Quoted 2 lines

Quoted 2 lines
Sorry for late response. I've been trapped in some urgent issues these days.

Signature · 2 lines

Best Regards
Qiao