* re: mfd: Support 88pm80x in 80x driver
@ 2015-05-14 10:31 Dan Carpenter
2015-05-14 11:35 ` Qiao Zhou
` (8 more replies)
0 siblings, 9 replies; 10+ messages in thread
From: Dan Carpenter @ 2015-05-14 10:31 UTC (permalink / raw)
To: kernel-janitors
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: mfd: Support 88pm80x in 80x driver
2015-05-14 10:31 mfd: Support 88pm80x in 80x driver Dan Carpenter
@ 2015-05-14 11:35 ` Qiao Zhou
2015-05-14 12:50 ` Lee Jones
` (7 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Qiao Zhou @ 2015-05-14 11:35 UTC (permalink / raw)
To: kernel-janitors
On 05/14/2015 06:31 PM, Dan Carpenter wrote:
> 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
Will fix this issue.
> ->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.
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.
>
> 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
>
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 <zhouqiao@marvell.com>
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 <dan.carpenter@oracle.com>
Signed-off-by: Qiao Zhou <zhouqiao@marvell.com>
---
include/linux/mfd/88pm80x.h | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
index 97cb283..a8c0318 100644
--- a/include/linux/mfd/88pm80x.h
+++ b/include/linux/mfd/88pm80x.h
@@ -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;
}
--
1.7.9.5
--
Best Regards
Qiao Zhou
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: mfd: Support 88pm80x in 80x driver
2015-05-14 10:31 mfd: Support 88pm80x in 80x driver Dan Carpenter
2015-05-14 11:35 ` Qiao Zhou
@ 2015-05-14 12:50 ` Lee Jones
2015-05-15 1:06 ` Qiao Zhou
` (6 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Lee Jones @ 2015-05-14 12:50 UTC (permalink / raw)
To: kernel-janitors
On Thu, 14 May 2015, Qiao Zhou wrote:
> On 05/14/2015 06:31 PM, Dan Carpenter wrote:
> >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
> Will fix this issue.
> >->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.
> 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.
> >
> > 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
> >
> 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 <zhouqiao@marvell.com>
> 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 <dan.carpenter@oracle.com>
> Signed-off-by: Qiao Zhou <zhouqiao@marvell.com>
> ---
> include/linux/mfd/88pm80x.h | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> index 97cb283..a8c0318 100644
> --- a/include/linux/mfd/88pm80x.h
> +++ b/include/linux/mfd/88pm80x.h
> @@ -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.
> + 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?
> + /* return 0, and do not block suspend */
This comment is not required.
> + 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?
> 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'.
> + return 0;
> + }
> +
> if (device_may_wakeup(dev))
> - clear_bit((1 << irq), &chip->wu_flag);
> + clear_bit(irq, &chip->wu_flag);
>
> return 0;
> }
--
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: mfd: Support 88pm80x in 80x driver
2015-05-14 10:31 mfd: Support 88pm80x in 80x driver Dan Carpenter
2015-05-14 11:35 ` Qiao Zhou
2015-05-14 12:50 ` Lee Jones
@ 2015-05-15 1:06 ` Qiao Zhou
2015-05-15 8:23 ` Dan Carpenter
` (5 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Qiao Zhou @ 2015-05-15 1:06 UTC (permalink / raw)
To: kernel-janitors
On 05/14/2015 08:50 PM, Lee Jones wrote:
> On Thu, 14 May 2015, Qiao Zhou wrote:
>
>> On 05/14/2015 06:31 PM, Dan Carpenter wrote:
>>> 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
>> Will fix this issue.
>>> ->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.
>> 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.
>>>
>>> 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
>>>
>> 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 <zhouqiao@marvell.com>
>> 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 <dan.carpenter@oracle.com>
>> Signed-off-by: Qiao Zhou <zhouqiao@marvell.com>
>> ---
>> include/linux/mfd/88pm80x.h | 16 ++++++++++++++--
>> 1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>> index 97cb283..a8c0318 100644
>> --- a/include/linux/mfd/88pm80x.h
>> +++ b/include/linux/mfd/88pm80x.h
>> @@ -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.
Will refine it.
>
>> + 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?
>
>> + /* return 0, and do not block suspend */
>
> This comment is not required.
Will refine it.
>
>> + 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?
>
>> 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'.
Will refine it.
>
>> + 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.
--
Best Regards
Qiao Zhou
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: mfd: Support 88pm80x in 80x driver
2015-05-14 10:31 mfd: Support 88pm80x in 80x driver Dan Carpenter
` (2 preceding siblings ...)
2015-05-15 1:06 ` Qiao Zhou
@ 2015-05-15 8:23 ` Dan Carpenter
2015-05-18 9:23 ` Lee Jones
` (4 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2015-05-15 8:23 UTC (permalink / raw)
To: kernel-janitors
On Fri, May 15, 2015 at 09:06:55AM +0800, Qiao Zhou wrote:
> >>@@ -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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: mfd: Support 88pm80x in 80x driver
2015-05-14 10:31 mfd: Support 88pm80x in 80x driver Dan Carpenter
` (3 preceding siblings ...)
2015-05-15 8:23 ` Dan Carpenter
@ 2015-05-18 9:23 ` Lee Jones
2015-05-18 9:27 ` Lee Jones
` (3 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Lee Jones @ 2015-05-18 9:23 UTC (permalink / raw)
To: kernel-janitors
On Fri, 15 May 2015, Qiao Zhou wrote:
> On 05/14/2015 08:50 PM, Lee Jones wrote:
> >On Thu, 14 May 2015, Qiao Zhou wrote:
> >
> >>On 05/14/2015 06:31 PM, Dan Carpenter wrote:
> >>>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
> >>Will fix this issue.
> >>>->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.
> >>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.
> >>>
> >>> 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
> >>>
> >>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 <zhouqiao@marvell.com>
> >>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 <dan.carpenter@oracle.com>
> >>Signed-off-by: Qiao Zhou <zhouqiao@marvell.com>
> >>---
> >> include/linux/mfd/88pm80x.h | 16 ++++++++++++++--
> >> 1 file changed, 14 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> >>index 97cb283..a8c0318 100644
> >>--- a/include/linux/mfd/88pm80x.h
> >>+++ b/include/linux/mfd/88pm80x.h
[...]
> >> 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.
--
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: mfd: Support 88pm80x in 80x driver
2015-05-14 10:31 mfd: Support 88pm80x in 80x driver Dan Carpenter
` (4 preceding siblings ...)
2015-05-18 9:23 ` Lee Jones
@ 2015-05-18 9:27 ` Lee Jones
2015-06-05 2:17 ` zhouqiao
` (2 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Lee Jones @ 2015-05-18 9:27 UTC (permalink / raw)
To: kernel-janitors
On Fri, 15 May 2015, Dan Carpenter wrote:
> On Fri, May 15, 2015 at 09:06:55AM +0800, Qiao Zhou wrote:
> > >>@@ -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?
It's not a blocker, but it is superfluous:
git grep -A2 dev_err
--
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: mfd: Support 88pm80x in 80x driver
2015-05-14 10:31 mfd: Support 88pm80x in 80x driver Dan Carpenter
` (5 preceding siblings ...)
2015-05-18 9:27 ` Lee Jones
@ 2015-06-05 2:17 ` zhouqiao
2015-06-05 7:27 ` Lee Jones
2015-06-05 7:49 ` zhouqiao
8 siblings, 0 replies; 10+ messages in thread
From: zhouqiao @ 2015-06-05 2:17 UTC (permalink / raw)
To: kernel-janitors
On 05/18/2015 05:23 PM, Lee Jones wrote:
> On Fri, 15 May 2015, Qiao Zhou wrote:
>
>> On 05/14/2015 08:50 PM, Lee Jones wrote:
>>> On Thu, 14 May 2015, Qiao Zhou wrote:
>>>
>>>> On 05/14/2015 06:31 PM, Dan Carpenter wrote:
>>>>> 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
>>>> Will fix this issue.
>>>>> ->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.
>>>> 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.
>>>>> 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
>>>>>
>>>> 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 <zhouqiao@marvell.com>
>>>> 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 <dan.carpenter@oracle.com>
>>>> Signed-off-by: Qiao Zhou <zhouqiao@marvell.com>
>>>> ---
>>>> include/linux/mfd/88pm80x.h | 16 ++++++++++++++--
>>>> 1 file changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>>>> index 97cb283..a8c0318 100644
>>>> --- a/include/linux/mfd/88pm80x.h
>>>> +++ b/include/linux/mfd/88pm80x.h
> [...]
>
>>>> 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.
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.
>
Sorry for late response. I've been trapped in some urgent issues these days.
--
Best Regards
Qiao
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: mfd: Support 88pm80x in 80x driver
2015-05-14 10:31 mfd: Support 88pm80x in 80x driver Dan Carpenter
` (6 preceding siblings ...)
2015-06-05 2:17 ` zhouqiao
@ 2015-06-05 7:27 ` Lee Jones
2015-06-05 7:49 ` zhouqiao
8 siblings, 0 replies; 10+ messages in thread
From: Lee Jones @ 2015-06-05 7:27 UTC (permalink / raw)
To: kernel-janitors
On Fri, 05 Jun 2015, zhouqiao wrote:
> On 05/18/2015 05:23 PM, Lee Jones wrote:
> >On Fri, 15 May 2015, Qiao Zhou wrote:
> >
> >>On 05/14/2015 08:50 PM, Lee Jones wrote:
> >>>On Thu, 14 May 2015, Qiao Zhou wrote:
> >>>
> >>>>On 05/14/2015 06:31 PM, Dan Carpenter wrote:
> >>>>>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
> >>>>Will fix this issue.
> >>>>>->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.
> >>>>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.
> >>>>> 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
> >>>>>
> >>>>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 <zhouqiao@marvell.com>
> >>>>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 <dan.carpenter@oracle.com>
> >>>>Signed-off-by: Qiao Zhou <zhouqiao@marvell.com>
> >>>>---
> >>>> include/linux/mfd/88pm80x.h | 16 ++++++++++++++--
> >>>> 1 file changed, 14 insertions(+), 2 deletions(-)
> >>>>
> >>>>diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> >>>>index 97cb283..a8c0318 100644
> >>>>--- a/include/linux/mfd/88pm80x.h
> >>>>+++ b/include/linux/mfd/88pm80x.h
> >[...]
> >
> >>>> 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.
> 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.
> Sorry for late response. I've been trapped in some urgent issues these days.
--
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: mfd: Support 88pm80x in 80x driver
2015-05-14 10:31 mfd: Support 88pm80x in 80x driver Dan Carpenter
` (7 preceding siblings ...)
2015-06-05 7:27 ` Lee Jones
@ 2015-06-05 7:49 ` zhouqiao
8 siblings, 0 replies; 10+ messages in thread
From: zhouqiao @ 2015-06-05 7:49 UTC (permalink / raw)
To: kernel-janitors
On 06/05/2015 03:27 PM, Lee Jones wrote:
> On Fri, 05 Jun 2015, zhouqiao wrote:
>
>> On 05/18/2015 05:23 PM, Lee Jones wrote:
>>> On Fri, 15 May 2015, Qiao Zhou wrote:
>>>
>>>> On 05/14/2015 08:50 PM, Lee Jones wrote:
>>>>> On Thu, 14 May 2015, Qiao Zhou wrote:
>>>>>
>>>>>> On 05/14/2015 06:31 PM, Dan Carpenter wrote:
>>>>>>> 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
>>>>>> Will fix this issue.
>>>>>>> ->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.
>>>>>> 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.
>>>>>>> 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
>>>>>>>
>>>>>> 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 <zhouqiao@marvell.com>
>>>>>> 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 <dan.carpenter@oracle.com>
>>>>>> Signed-off-by: Qiao Zhou <zhouqiao@marvell.com>
>>>>>> ---
>>>>>> include/linux/mfd/88pm80x.h | 16 ++++++++++++++--
>>>>>> 1 file changed, 14 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>>>>>> index 97cb283..a8c0318 100644
>>>>>> --- a/include/linux/mfd/88pm80x.h
>>>>>> +++ b/include/linux/mfd/88pm80x.h
>>> [...]
>>>
>>>>>> 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.
>> 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.
>
>> Sorry for late response. I've been trapped in some urgent issues these days.
--
Best Regards
Qiao
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-06-05 7:49 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-14 10:31 mfd: Support 88pm80x in 80x driver Dan Carpenter
2015-05-14 11:35 ` Qiao Zhou
2015-05-14 12:50 ` Lee Jones
2015-05-15 1:06 ` Qiao Zhou
2015-05-15 8:23 ` Dan Carpenter
2015-05-18 9:23 ` Lee Jones
2015-05-18 9:27 ` Lee Jones
2015-06-05 2:17 ` zhouqiao
2015-06-05 7:27 ` Lee Jones
2015-06-05 7:49 ` zhouqiao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.