Re: mfd: Support 88pm80x in 80x driver

[zhouqiao <zhouqiao@marvell.com>]

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