Re: mfd: Support 88pm80x in 80x driver

[zhouqiao <zhouqiao@marvell.com>]

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