Re: [PATCH v5 20/21] hwmon: (mr75203) add debugfs to read and write temperature coefficients

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 9/13/2022 8:01 PM, Andy Shevchenko wrote:
On Tue, Sep 13, 2022 at 05:40:16PM +0300, Farber, Eliav wrote:
On 9/13/2022 4:06 PM, Farber, Eliav wrote:

...

It seems like debugfs_attr_write() calls simple_attr_write() and it uses
kstrtoull(), which is why it fails when setting a negative value.
This is the same also in v6.0-rc5.

debugfs_attr_read() on the other hand does show the correct value also
when j is negative.

Which puzzles me since there is a few drivers that use %lld.
Yeah, changing it to

       ret = sscanf(attr->set_buf, attr->fmt, &val);
       if (ret != 1)
               ret = -EINVAL;

probably can fix that. Dunno if debugfs maintainer is okay with this.

P.S. This needs revisiting all format strings to see if there are no additional
characters, otherwise that needs to be addressed first, if feasible.

I was thinking of making such a correction:

-       ret = kstrtoull(attr->set_buf, 0, &val);
+       if (attr->set_buf[0] == '-')
+               ret = kstrtoll(attr->set_buf, 0, &val);
+       else
+               ret = kstrtoull(attr->set_buf, 0, &val);

and when I tested the change it worked, but then I noticed this commit:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs/libfs.c?h=v6.0-rc5&id=488dac0c9237647e9b8f788b6a342595bfa40bda

According to this, it previously used simple_strtoll() which supports
negative values, but was changed to use kstrtoull() to deliberately
return '-EINVAL' if it gets a negative value.

So I’m not sure debugfs maintainers will be okay with a fix that
basically reverts the commit I mentioned.
Hence, what do you suggest to do with my commit?
Is it ok to leave it as it is today?

--
Thanks, Eliav



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux