From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lee Jones Date: Thu, 14 May 2015 12:50:59 +0000 Subject: Re: mfd: Support 88pm80x in 80x driver Message-Id: <20150514125059.GK22418@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 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(dev= ); > > 348 struct pm80x_chip *chip =3D dev_get_drvdata(pdev->dev.p= arent); > > 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(dev= ); > > 360 struct pm80x_chip *chip =3D dev_get_drvdata(pdev->dev.p= arent); > > 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, struct = 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, struct = 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, >=20 > 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. >=20 > 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 >=20 > 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. >=20 > Reported-by: Dan Carpenter > Signed-off-by: Qiao Zhou > --- > include/linux/mfd/88pm80x.h | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) >=20 > 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 *d= ev) > struct pm80x_chip *chip =3D dev_get_drvdata(pdev->dev.parent); > int irq =3D platform_get_irq(pdev, 0); >=20 > + if ((irq < 0) || (irq >=3D 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 *de= v) > struct pm80x_chip *chip =3D dev_get_drvdata(pdev->dev.parent); > int irq =3D platform_get_irq(pdev, 0); >=20 > + if ((irq < 0) || (irq >=3D 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); >=20 > return 0; > } --=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