| Message ID | 20201130153742.9163-3-johan@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tty: add flag to suppress ready signalling on open | expand |
On Mon, Nov 30, 2020 at 5:42 PM Johan Hovold <johan@kernel.org> wrote: > > Add a nordy sysfs attribute to suppress raising the modem-control lines > on open to signal DTE readiness. Why not call it nomctrl ? > This can be use to prevent undesirable side-effects on open for used > applications where the DTR and RTS lines are used for non-standard > purposes such as generating power-on and reset pulses. ... > +static ssize_t nordy_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct tty_port *port = dev_get_drvdata(dev); > + unsigned int val; > + int ret; > + > + ret = kstrtouint(buf, 0, &val); > + if (ret) > + return ret; > + if (val > 1) > + return -EINVAL; Can't we utilise kstrtobool() instead? > + tty_port_set_nordy(port, val); > + > + return count; > +}
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: > > > > Add a nordy sysfs attribute to suppress raising the modem-control lines > > on open to signal DTE readiness. > > Why not call it nomctrl ? That was one of the candidates I rejected. As I hinted in the cover letter (and patch adding the flag) I chose the name to match the current termios flags (e.g. HUPCL and NOFLSH). NOMCTRL is both too general and specific; HUPCL still controls the modem-control lines on final close. Also, like HUPCL, I wanted a more general name that can be used for terminal devices which can signal readiness through other means (e.g. network). Like the other termios flags it is terse, but once you learn the meaning it's easy to remember. And I think there's value in keeping the same name throughout (cf. termios flags and stty). > > This can be use to prevent undesirable side-effects on open for > > used Thanks, I'll fix that up before applying or resending > > applications where the DTR and RTS lines are used for non-standard > > purposes such as generating power-on and reset pulses. > > ... > > > +static ssize_t nordy_store(struct device *dev, struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + struct tty_port *port = dev_get_drvdata(dev); > > + unsigned int val; > > + int ret; > > + > > + 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). Not sure how that ever made it into sysfs code... The attribute is read back as "0" or "1" and those are precisely the values that can be written back (well, modulo radix). It's not relevant in this case, but tight control over the inputs also allows for extending the range later. > > + tty_port_set_nordy(port, val); > > + > > + return count; > > +} Johan
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. > Not sure how that ever made it into sysfs code... > > The attribute is read back as "0" or "1" and those are precisely the > values that can be written back (well, modulo radix). So, how does it affect the kstrtobool() interface? You read back 0 and 1 and they are pretty much accepted by it. > It's not relevant in this case, but tight control over the inputs also > allows for extending the range later. And kstrtobool() does it. So I don't see any difference except a few less lines of code and actually *stricter* rules than kstrtouint() has.
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. > > Not sure how that ever made it into sysfs code... > > > > The attribute is read back as "0" or "1" and those are precisely the > > values that can be written back (well, modulo radix). > > So, how does it affect the kstrtobool() interface? > You read back 0 and 1 and they are pretty much accepted by it. > > > It's not relevant in this case, but tight control over the inputs also > > allows for extending the range later. > > And kstrtobool() does it. So I don't see any difference except a few > less lines of code and actually *stricter* rules than kstrtouint() > has. You miss the point; kstrobool accepts "12" today and treats it as true. You cannot extend such an interface to later accept a larger range than 0 and 1 as you didn't return an error for "12" from the start (as someone might now rely on "12" being treated as "1"). Johan
On Tue, Dec 1, 2020 at 1:04 PM Johan Hovold <johan@kernel.org> 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. Depends how you interpret this. kstrtobool() uses one character (and in some cases two) of the input. Everything else is garbage. Should we interpret garbage? > So why should a sysfs-interface accept it as valid input and treat it as > false? That's just bad design. I can agree with this. > > > Not sure how that ever made it into sysfs code... > > > > > > The attribute is read back as "0" or "1" and those are precisely the > > > values that can be written back (well, modulo radix). > > > > So, how does it affect the kstrtobool() interface? > > You read back 0 and 1 and they are pretty much accepted by it. > > > > > It's not relevant in this case, but tight control over the inputs also > > > allows for extending the range later. > > > > And kstrtobool() does it. So I don't see any difference except a few > > less lines of code and actually *stricter* rules than kstrtouint() > > has. > > You miss the point; kstrobool accepts "12" today and treats it as true. > You cannot extend such an interface to later accept a larger range than > 0 and 1 as you didn't return an error for "12" from the start (as someone > might now rely on "12" being treated as "1"). Somehow cifs uses kstrtobool() in conjunction with the wider ranges. Nobody complained so far. But maybe they had it from day 1. So, we have two issues here: kstrtobool() doesn't report an error of input when it has garbage, the user may rely on garbage to be discarded.
On Tue, Dec 01, 2020 at 01:19:30PM +0200, Andy Shevchenko wrote: > On Tue, Dec 1, 2020 at 1:04 PM Johan Hovold <johan@kernel.org> wrote: > > 0x01 is 1 and is generally treated as boolean true as you know. > > Depends how you interpret this. kstrtobool() uses one character (and > in some cases two) of the input. Everything else is garbage. > Should we interpret garbage? No, ideally we should reject the input. > > So why should a sysfs-interface accept it as valid input and treat it as > > false? That's just bad design. > > I can agree with this. Looks like part of the problem are commits like 4cc7ecb7f2a6 ("param: convert some "on"/"off" users to strtobool") which destroyed perfectly well-defined interfaces. > > You miss the point; kstrobool accepts "12" today and treats it as true. > > You cannot extend such an interface to later accept a larger range than > > 0 and 1 as you didn't return an error for "12" from the start (as someone > > might now rely on "12" being treated as "1"). > > Somehow cifs uses kstrtobool() in conjunction with the wider ranges. Nobody > complained so far. But maybe they had it from day 1. Wow, that's pretty nasty. > So, we have two issues here: kstrtobool() doesn't report an error of > input when it has garbage, the user may rely on garbage to be > discarded. Right, parsing is too allowing and there are too many ways to say true/false. The power-management attributes use 0 and 1 for boolean like I do here, and I'd prefer to stick to that until we have deprecated the current kstrtobool. Johan
On Tue, Dec 1, 2020 at 3:21 PM Johan Hovold <johan@kernel.org> wrote: > On Tue, Dec 01, 2020 at 01:19:30PM +0200, Andy Shevchenko wrote: > > On Tue, Dec 1, 2020 at 1:04 PM Johan Hovold <johan@kernel.org> wrote: ... > > > 0x01 is 1 and is generally treated as boolean true as you know. > > > > Depends how you interpret this. kstrtobool() uses one character (and > > in some cases two) of the input. Everything else is garbage. > > Should we interpret garbage? > > No, ideally we should reject the input. We can do it by the way in kstrtobool() and see if anybody complains (I believe the world is saner than relying on 0x01 for false and 123 for true. ... > > > So why should a sysfs-interface accept it as valid input and treat it as > > > false? That's just bad design. > > > > I can agree with this. > > Looks like part of the problem are commits like 4cc7ecb7f2a6 ("param: > convert some "on"/"off" users to strtobool") which destroyed perfectly > well-defined interfaces. Oh, but the strtobool() in ABI was before that, for instance % git grep -n -p -w strtobool v3.14 shows a few dozens of users and some of them looks like ABI. ... > > Somehow cifs uses kstrtobool() in conjunction with the wider ranges. Nobody > > complained so far. But maybe they had it from day 1. > > Wow, that's pretty nasty. I have checked, the wider range fits one character. So, basically they had this kind of interface from day 1. ... > > So, we have two issues here: kstrtobool() doesn't report an error of > > input when it has garbage, the user may rely on garbage to be > > discarded. > > Right, parsing is too allowing and there are too many ways to say > true/false. > > The power-management attributes use 0 and 1 for boolean like I do here, > and I'd prefer to stick to that until we have deprecated the current > kstrtobool. Okay!
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. 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 :) thanks, greg k-h
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
On Tue, Dec 01, 2020 at 03:49:19PM +0200, Andy Shevchenko wrote: > On Tue, Dec 1, 2020 at 3:21 PM Johan Hovold <johan@kernel.org> wrote: > > On Tue, Dec 01, 2020 at 01:19:30PM +0200, Andy Shevchenko wrote: > > > On Tue, Dec 1, 2020 at 1:04 PM Johan Hovold <johan@kernel.org> wrote: > > ... > > > > > 0x01 is 1 and is generally treated as boolean true as you know. > > > > > > Depends how you interpret this. kstrtobool() uses one character (and > > > in some cases two) of the input. Everything else is garbage. > > > Should we interpret garbage? > > > > No, ideally we should reject the input. > > We can do it by the way in kstrtobool() and see if anybody complains > (I believe the world is saner than relying on 0x01 for false and 123 > for true. I bet someone is using "YEAH!" just because they can. ;) > ... > > > > > So why should a sysfs-interface accept it as valid input and treat it as > > > > false? That's just bad design. > > > > > > I can agree with this. > > > > Looks like part of the problem are commits like 4cc7ecb7f2a6 ("param: > > convert some "on"/"off" users to strtobool") which destroyed perfectly > > well-defined interfaces. > > Oh, but the strtobool() in ABI was before that, for instance > % git grep -n -p -w strtobool v3.14 > shows a few dozens of users and some of them looks like ABI. Indeed, it apparently goes further back than strtobool(). The series introducing strtobool() explicitly mentions the lax parsing and for that reason wanted to keep it distinct from the other kstrto* function by dropping the k-prefix: The naming is still distinct enough from kstrtox to avoid any implication that this function has the same tight parameter passing that those functions have. https://lore.kernel.org/lkml/1303213427-12798-1-git-send-email-jic23@cam.ac.uk/#t And it was more recently renamed kstrtobool() anyway. Let's call it a feature then. Johan
diff --git a/Documentation/ABI/testing/sysfs-tty b/Documentation/ABI/testing/sysfs-tty index e157130a6792..2634b4bf9c7f 100644 --- a/Documentation/ABI/testing/sysfs-tty +++ b/Documentation/ABI/testing/sysfs-tty @@ -161,3 +161,10 @@ Contact: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Description: Allows user to detach or attach back the given device as kernel console. It shows and accepts a boolean variable. + +What: /sys/class/tty/ttyS0/nordy +Date: November 2020 +Contact: Johan Hovold <johan@kernel.org> +Description: + Show and store the port NORDY flag which suppresses raising + the modem-control lines on open to signal DTE readiness. diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index f41cba10b86b..063a617182ce 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -2805,6 +2805,33 @@ static ssize_t console_store(struct device *dev, return ret < 0 ? ret : count; } +static ssize_t nordy_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct tty_port *port = dev_get_drvdata(dev); + + return sprintf(buf, "%d\n", tty_port_nordy(port)); +} + +static ssize_t nordy_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct tty_port *port = dev_get_drvdata(dev); + unsigned int val; + int ret; + + ret = kstrtouint(buf, 0, &val); + if (ret) + return ret; + + if (val > 1) + return -EINVAL; + + tty_port_set_nordy(port, val); + + return count; +} + static DEVICE_ATTR_RO(uartclk); static DEVICE_ATTR_RO(type); static DEVICE_ATTR_RO(line); @@ -2819,6 +2846,7 @@ static DEVICE_ATTR_RO(io_type); static DEVICE_ATTR_RO(iomem_base); static DEVICE_ATTR_RO(iomem_reg_shift); static DEVICE_ATTR_RW(console); +static DEVICE_ATTR_RW(nordy); static struct attribute *tty_dev_attrs[] = { &dev_attr_uartclk.attr, @@ -2835,6 +2863,7 @@ static struct attribute *tty_dev_attrs[] = { &dev_attr_iomem_base.attr, &dev_attr_iomem_reg_shift.attr, &dev_attr_console.attr, + &dev_attr_nordy.attr, NULL };
Add a nordy sysfs attribute to suppress raising the modem-control lines on open to signal DTE readiness. This can be use to prevent undesirable side-effects on open for applications where the DTR and RTS lines are used for non-standard purposes such as generating power-on and reset pulses. Signed-off-by: Johan Hovold <johan@kernel.org> --- Documentation/ABI/testing/sysfs-tty | 7 +++++++ drivers/tty/serial/serial_core.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+)