[PATCH] firmware_loader: Replace simple_strtol() with kstrtoint()

[Haoyang Liu]

simple_strtol() is obsolete, use kstrtoint() instead.

Signed-off-by: Haoyang Liu <ttttur...@hust.edu.cn>
---
drivers/base/firmware_loader/sysfs.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_loader/sysfs.c b/drivers/base/firmware_loader/sysfs.c
index c9c93b47d9a5..dd08c187cc00 100644
--- a/drivers/base/firmware_loader/sysfs.c
+++ b/drivers/base/firmware_loader/sysfs.c
@@ -47,7 +47,11 @@ static ssize_t timeout_show(const struct class *class, const struct class_attrib
static ssize_t timeout_store(const struct class *class, const struct class_attribute *attr,
const char *buf, size_t count)
{
- int tmp_loading_timeout = simple_strtol(buf, NULL, 10);
+ int tmp_loading_timeout;
+ int res = kstrtoint(buf, 10, &tmp_loading_timeout);
+
+ if (res < 0)
+ return res;

if (tmp_loading_timeout < 0)
tmp_loading_timeout = 0;
@@ -157,8 +161,13 @@ static ssize_t firmware_loading_store(struct device *dev,
struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
struct fw_priv *fw_priv;
ssize_t written = count;
- int loading = simple_strtol(buf, NULL, 10);
+ int loading;
+ int res = kstrtoint(buf, 10, &loading);

+ if (res < 0) {
+ dev_err(dev, "%s: invalid value\n", __func__);
+ return res;
+ }
mutex_lock(&fw_lock);
fw_priv = fw_sysfs->fw_priv;
if (fw_state_is_aborted(fw_priv) || fw_state_is_done(fw_priv))
--
2.25.1

[Dan Carpenter]

On Wed, Apr 24, 2024 at 11:46:21PM +0800, Haoyang Liu wrote:
> simple_strtol() is obsolete, use kstrtoint() instead.
>
> Signed-off-by: Haoyang Liu <ttttur...@hust.edu.cn>
> ---
> drivers/base/firmware_loader/sysfs.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/firmware_loader/sysfs.c b/drivers/base/firmware_loader/sysfs.c
> index c9c93b47d9a5..dd08c187cc00 100644
> --- a/drivers/base/firmware_loader/sysfs.c
> +++ b/drivers/base/firmware_loader/sysfs.c
> @@ -47,7 +47,11 @@ static ssize_t timeout_show(const struct class *class, const struct class_attrib
> static ssize_t timeout_store(const struct class *class, const struct class_attribute *attr,
> const char *buf, size_t count)
> {
> - int tmp_loading_timeout = simple_strtol(buf, NULL, 10);
> + int tmp_loading_timeout;
> + int res = kstrtoint(buf, 10, &tmp_loading_timeout);
> +
> + if (res < 0)
> + return res;

Generally, don't put functions which can fail in the declaration block.
People review the declaration block less carefully. And it looks bad to
put a blank line between the function and the error checking.

int tmp_loading_timeout;
int res;

res = kstrtoint(buf, 10, &tmp_loading_timeout);

if (res)
return res;

regards,
dan carpenter

[Haoyang Liu]

simple_strtol() is obsolete, use kstrtoint() instead.

Signed-off-by: Haoyang Liu <ttttur...@hust.edu.cn>
---

v1 -> v2: remove functions which can fail in the declaration block, move it to the body of the function.

drivers/base/firmware_loader/sysfs.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_loader/sysfs.c b/drivers/base/firmware_loader/sysfs.c
index c9c93b47d9a5..23945e0df8df 100644
--- a/drivers/base/firmware_loader/sysfs.c
+++ b/drivers/base/firmware_loader/sysfs.c
@@ -47,7 +47,12 @@ static ssize_t timeout_show(const struct class *class, const struct class_attrib

static ssize_t timeout_store(const struct class *class, const struct class_attribute *attr,
const char *buf, size_t count)
{
- int tmp_loading_timeout = simple_strtol(buf, NULL, 10);
+ int tmp_loading_timeout;

+ int res;
+
+ res = kstrtoint(buf, 10, &tmp_loading_timeout);

+ if (res < 0)
+ return res;

if (tmp_loading_timeout < 0)
tmp_loading_timeout = 0;

@@ -157,8 +162,14 @@ static ssize_t firmware_loading_store(struct device *dev,

struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
struct fw_priv *fw_priv;
ssize_t written = count;

- int loading = simple_strtol(buf, NULL, 10);
+ int loading;
+ int res;

+ res = kstrtoint(buf, 10, &loading);
+ if (res < 0) {

[Dan Carpenter]

On Thu, Apr 25, 2024 at 01:56:45PM +0800, Haoyang Liu wrote:
> @@ -157,8 +162,14 @@ static ssize_t firmware_loading_store(struct device *dev,
> struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
> struct fw_priv *fw_priv;
> ssize_t written = count;
> - int loading = simple_strtol(buf, NULL, 10);
> + int loading;
> + int res;
>
> + res = kstrtoint(buf, 10, &loading);
> + if (res < 0) {
> + dev_err(dev, "%s: invalid value\n", __func__);

Wait. Sorry, I didn't look at this carefully before. Why did you add a
dev_err() here? The problem with that is that you can end up filling
the /var/log/messages file with nonsense. On the other hand, this code
is probably root only... But the message is not very useful.

regards,
dan carpenter

[Haoyang Liu]

simple_strtol() is obsolete, use kstrtoint() instead.

Signed-off-by: Haoyang Liu <ttttur...@hust.edu.cn>
---

v2 -> v3: remove dev_err().

v1 -> v2: remove functions which can fail in the declaration block, move it to the body of the function.

drivers/base/firmware_loader/sysfs.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_loader/sysfs.c b/drivers/base/firmware_loader/sysfs.c
index c9c93b47d9a5..4de1cb243bee 100644
--- a/drivers/base/firmware_loader/sysfs.c
+++ b/drivers/base/firmware_loader/sysfs.c
@@ -47,8 +47,12 @@ static ssize_t timeout_show(const struct class *class, const struct class_attrib

static ssize_t timeout_store(const struct class *class, const struct class_attribute *attr,
const char *buf, size_t count)
{

- int tmp_loading_timeout = simple_strtol(buf, NULL, 10);
+ int tmp_loading_timeout;
+ int res;

+ res = kstrtoint(buf, 10, &tmp_loading_timeout);
+ if (res < 0)

+ return res;
if (tmp_loading_timeout < 0)
tmp_loading_timeout = 0;

@@ -157,8 +161,12 @@ static ssize_t firmware_loading_store(struct device *dev,

struct fw_sysfs *fw_sysfs = to_fw_sysfs(dev);
struct fw_priv *fw_priv;
ssize_t written = count;
- int loading = simple_strtol(buf, NULL, 10);
+ int loading;
+ int res;

+ res = kstrtoint(buf, 10, &loading);
+ if (res < 0)

+ return res;

mutex_lock(&fw_lock);
fw_priv = fw_sysfs->fw_priv;
if (fw_state_is_aborted(fw_priv) || fw_state_is_done(fw_priv))

--
2.25.1

[Russ]

May I send this to LKML since it seems that nothing to be done now?

Best regards,

Haoyang

开 2024/4/25 14:56:43, Haoyang Liu <ttttur...@hust.edu.cn> 撰写:

simple_strtol() is obsolete, use kstrtoint() instead.

Signed-off-by: Haoyang Liu

[Dan Carpenter]

On Sat, Apr 27, 2024 at 04:15:17AM +0800, Russ wrote:
> May I send this to LKML since it seems that nothing to be done now?

Yes.

regards,
dan carpenter

[Haoyang Liu]

simple_strtol() is obsolete, use kstrtoint() instead.

Signed-off-by: Haoyang Liu <ttttur...@hust.edu.cn>
Reviewed-by: Dan Carpenter <dan.ca...@linaro.org>
---

[Russ Weight]

On Sat, Apr 27, 2024 at 04:25:32AM +0800, Haoyang Liu wrote:
> simple_strtol() is obsolete, use kstrtoint() instead.
>
> Signed-off-by: Haoyang Liu <ttttur...@hust.edu.cn>
> Reviewed-by: Dan Carpenter <dan.ca...@linaro.org>

Reviewed-by: Russ Weight <russ....@linux.dev>

[Greg Kroah-Hartman]

On Sat, May 04, 2024 at 09:22:08PM +0300, Andy Shevchenko wrote:

> Sat, Apr 27, 2024 at 04:25:32AM +0800, Haoyang Liu kirjoitti:
> > simple_strtol() is obsolete, use kstrtoint() instead.
>

> It's not, but kstrtox() is preferred.
>
> ...
>
>
> While I'm in support of this move, this might break userspace by making
> stricter requirement on the input.

Good point, I've dropped this so that others can test it better first.

thanks,

greg k-h