Re: [PATCH 2/5] serial: core: add sysfs attribute to suppress ready signalling on open

[Johan Hovold <johan@kernel.org>]

On Tue, Dec 01, 2020 at 05:44:10PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 01, 2020 at 12:05:23PM +0100, Johan Hovold wrote:
> > On Tue, Dec 01, 2020 at 12:55:54PM +0200, Andy Shevchenko wrote:
> > > On Tue, Dec 1, 2020 at 10:20 AM Johan Hovold <johan@kernel.org> wrote:
> > > > On Mon, Nov 30, 2020 at 08:27:54PM +0200, Andy Shevchenko wrote:
> > > > > On Mon, Nov 30, 2020 at 5:42 PM Johan Hovold <johan@kernel.org> wrote:
> > > 
> > > > > > +       ret = kstrtouint(buf, 0, &val);
> > > > > > +       if (ret)
> > > > > > +               return ret;
> > > > >
> > > > > > +       if (val > 1)
> > > > > > +               return -EINVAL;
> > > > >
> > > > > Can't we utilise kstrtobool() instead?
> > > >
> > > > I chose not to as kstrtobool() results in a horrid interface. To many
> > > > options to do the same thing and you end up with confusing things like
> > > > "0x01" being accepted but treated as false (as only the first character
> > > > is considered).
> > > 
> > > And this is perfectly fine. 0x01 is not boolean.
> > 
> > 0x01 is 1 and is generally treated as boolean true as you know.
> > 
> > So why should a sysfs-interface accept it as valid input and treat it as
> > false? That's just bad design.
> 
> The "design" was to accept "sane" flags here:
> 	1, y, Y mean "enable"
> 	0, n, N mean "disable"
> 
> We never thought someone would try to write "0x01" as "enable" for a
> boolean flag :)
>
> So it's not a bad design, it works well what it was designed for.  It
> just is NOT designed for hex values.

I'd still say it was a bad idea to accept just about anything like
"yoghurt" or "0x01" as valid input. And having some attributes accept
"0x01" or "01" as true while other consider it false as is the case
today is less than ideal.

For sysfs we should be able to pick and enforce a representation, or
three, for example, by adding a 1-character length check for the above
examples (which have since been extended to accept "Often" and
"ontology" and what not). 

> If your sysfs file is "enable/disable", then please, use kstrtobool, as
> that is the standard way of doing this, and don't expect 0x01 to work :)

A quick grep shows we have about 55 attributes using [k]strtobool and 35
or so which expects integers and reject malformed input like "1what". So
perhaps not too late to fix. ;)

But ok, I'll use kstrtobool().

Johan