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

From: Farber, Eliav
Date: Thu Sep 15 2022 - 05:18:08 EST


On 9/14/2022 5:08 PM, Andy Shevchenko wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.



On Wed, Sep 14, 2022 at 05:03:09PM +0300, Andy Shevchenko wrote:
On Wed, Sep 14, 2022 at 12:32:47PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 14, 2022 at 07:26:36AM +0300, Farber, Eliav wrote:
> > 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?
>
> Meanwhile asking is not a problem, at least we will know for sure.
> And yes, leave it as is, but point to the thread where you asking
> the clarification.

For the record:

$ git grep -n -A1 -w DEFINE_DEBUGFS_ATTRIBUTE | grep ');' | sed 's,.*\(".*%.*"\).*,\1,' | sort | uniq -c
  1 "%08llx\n"
  5 "0x%016llx\n"
  5 "0x%02llx\n"
  5 "0x%04llx\n"
 13 "0x%08llx\n"
  1 "0x%4.4llx\n"
  3 "0x%.4llx\n"
  4 "0x%llx\n"
  1 "%1lld\n"
 40 "%lld\n"
  2 "%lli\n"
129 "%llu\n"
  1 "%#llx\n"
  2 "%llx\n"

means that sscanf() should work and fix the issue. You may even propose a patch
as a starter for a discussion.


I proposed a patch with the fix you suggested.

--
Thanks, Eliav