From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lee Jones Date: Mon, 18 May 2015 09:23:04 +0000 Subject: Re: mfd: Support 88pm80x in 80x driver Message-Id: <20150518092304.GO22418@x1> List-Id: References: <20150514103124.GA21248@mwanda> In-Reply-To: <20150514103124.GA21248@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable To: kernel-janitors@vger.kernel.org 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 =3D to_platform_device(d= ev); > >>> 348 struct pm80x_chip *chip =3D dev_get_drvdata(pdev->dev= .parent); > >>> 349 int irq =3D 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 =3D to_platform_device(d= ev); > >>> 360 struct pm80x_chip *chip =3D dev_get_drvdata(pdev->dev= .parent); > >>> 361 int irq =3D 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 =3D container_of(dev, struc= t i2c_client, dev); > >>> 137 struct pm80x_chip *chip =3D 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 =3D container_of(dev, struc= t i2c_client, dev); > >>> 149 struct pm80x_chip *chip =3D 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 > >>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 > >>Signed-off-by: Qiao Zhou > >>--- > >> 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. --=20 Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org =E2=94=82 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