| Message ID | 20210113182016.166049-11-pobrn@protonmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86: ideapad-laptop: cleanup, keyboard backlight and "always on USB charging" control support, reenable touchpad control | expand |
On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote: > > Do not handle zero length buffer separately. Use kstrtouint() instead > of sscanf(). ... > - int ret, state; > + int ret; > struct ideapad_private *priv = dev_get_drvdata(dev); > + unsigned int state; Reversed xmas tree order? ... > - if (sscanf(buf, "%i", &state) != 1) > - return -EINVAL; > + ret = kstrtouint(buf, 0, &state); > + if (ret) > + return ret; This seems to me a relaxing case, and should be 10 instead of 0. Am I right about %i? If it's true it's probably minor, but still an ABI breakage. P.S. Same comments for the similar cases.
Hi 2021. január 16., szombat 20:52 keltezéssel, Andy Shevchenko írta: > On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze wrote: > > > > Do not handle zero length buffer separately. Use kstrtouint() instead > > of sscanf(). > > ... > > > - int ret, state; > > + int ret; > > struct ideapad_private *priv = dev_get_drvdata(dev); > > + unsigned int state; > > Reversed xmas tree order? > I'll change the order. > ... > > > - if (sscanf(buf, "%i", &state) != 1) > > - return -EINVAL; > > + ret = kstrtouint(buf, 0, &state); > > + if (ret) > > + return ret; > > This seems to me a relaxing case, and should be 10 instead of 0. Am I > right about %i? > If it's true it's probably minor, but still an ABI breakage. > According to the latest C99 draft[1] (7.19.6.2): [The 'i' format specifier] Matches an optionally signed integer, whose format is the same as expected for the subject sequence of the strtol function with the value 0 for the base argument. The corresponding argument shall be a pointer to signed integer. Skimming over `vsscanf()`, I'm fairly sure it implements the same behaviour. So '0' as the base is correct, I believe. But technically it's still an ABI breakage since negative numbers are no longer accepted. In the case of `store_ideapad_fan()` it changes nothing since there was bounds checking in place. In the case of `store_ideapad_cam()` negative numbers are now rejected. I'm not sure if this change should be of great concern, since the the documentation only mentions '0' and '1', and I would be surprised if anyone used this interface to send negative numbers to the embedded controller. [1]: https://wg14.link/c99 Regards, Barnabás Pőcze
On Sat, Jan 16, 2021 at 11:54 PM Barnabás Pőcze <pobrn@protonmail.com> wrote: > 2021. január 16., szombat 20:52 keltezéssel, Andy Shevchenko írta: > > On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze wrote: ... > > > - if (sscanf(buf, "%i", &state) != 1) > > > - return -EINVAL; > > > + ret = kstrtouint(buf, 0, &state); > > > + if (ret) > > > + return ret; > > > > This seems to me a relaxing case, and should be 10 instead of 0. Am I > > right about %i? > > If it's true it's probably minor, but still an ABI breakage. > > According to the latest C99 draft[1] (7.19.6.2): > > [The 'i' format specifier] Matches an optionally signed integer, whose format > is the same as expected for the subject sequence of the strtol function with > the value 0 for the base argument. The corresponding argument shall be a pointer > to signed integer. > Skimming over `vsscanf()`, I'm fairly sure it implements the same behaviour. > So '0' as the base is correct, I believe. Ah, okay, good to know. I assumed that %i is decimal only. Thanks! > But technically it's still an ABI > breakage since negative numbers are no longer accepted. In the case of > `store_ideapad_fan()` it changes nothing since there was bounds checking in place. > In the case of `store_ideapad_cam()` negative numbers are now rejected. I'm not > sure if this change should be of great concern, since the the documentation only > mentions '0' and '1', and I would be surprised if anyone used this interface > to send negative numbers to the embedded controller. If it's only 0 / 1 perhaps you may go further and make it bool (kstrtobool() API)? > [1]: https://wg14.link/c99
diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c index 677d4e10352e..cba3d9419f52 100644 --- a/drivers/platform/x86/ideapad-laptop.c +++ b/drivers/platform/x86/ideapad-laptop.c @@ -365,13 +365,13 @@ static ssize_t store_ideapad_cam(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { - int ret, state; + int ret; struct ideapad_private *priv = dev_get_drvdata(dev); + unsigned int state; - if (!count) - return 0; - if (sscanf(buf, "%i", &state) != 1) - return -EINVAL; + ret = kstrtouint(buf, 0, &state); + if (ret) + return ret; ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_CAMERA, state); if (ret) return ret; @@ -398,14 +398,14 @@ static ssize_t store_ideapad_fan(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { - int ret, state; + int ret; struct ideapad_private *priv = dev_get_drvdata(dev); + unsigned int state; - if (!count) - return 0; - if (sscanf(buf, "%i", &state) != 1) - return -EINVAL; - if (state < 0 || state > 4 || state == 3) + ret = kstrtouint(buf, 0, &state); + if (ret) + return ret; + if (state > 4 || state == 3) return -EINVAL; ret = write_ec_cmd(priv->adev->handle, VPCCMD_W_FAN, state); if (ret)