[PATCH 0/5] tty: add flag to suppress ready signalling on open

[PATCH 0/5] tty: add flag to suppress ready signalling on open

[Johan Hovold]

This series adds a new NORDY port flag to suppress raising the
modem-control lines on open to signal DTE readiness.
    
This can be used to implement a NORDY termios control flag to complement
HUPCL, which controls lowering of the modem-control lines on final
close.

Initially drivers can export the flag through sysfs, which also allows
control over the lines on first open. Such an interface is implemented
for serial core and USB serial.

The motivation for this is to allow for applications where the DTR and
RTS lines are used for non-standard purposes (e.g. generating power-on
and reset pulses) to open the port without undesirable side effects.

The final patches enables this flag by default for such a USB serial
device.

Greg, are you ok we me taking this through my tree? I'm planning on some
follow ups to the ftdi driver and the tty/serial changes are fairly
self-contained.

Johan


Johan Hovold (3):
  tty: add port flag to suppress ready signalling on open
  serial: core: add sysfs attribute to suppress ready signalling on open
  USB: serial: add sysfs attribute to suppress ready signalling on open

Mychaela N. Falconia (2):
  USB: serial: ftdi_sio: pass port to quirk port_probe functions
  USB: serial: ftdi_sio: add support for FreeCalypso DUART28C adapter

 Documentation/ABI/testing/sysfs-tty |  7 +++++
 drivers/tty/serial/serial_core.c    | 29 ++++++++++++++++++++
 drivers/tty/tty_port.c              |  2 +-
 drivers/usb/serial/bus.c            | 38 ++++++++++++++++++++++++--
 drivers/usb/serial/ftdi_sio.c       | 42 +++++++++++++++++++++++------
 drivers/usb/serial/ftdi_sio_ids.h   |  1 +
 include/linux/tty.h                 | 14 ++++++++++
 7 files changed, 122 insertions(+), 11 deletions(-)

-- 
2.26.2

[PATCH 1/5] tty: add port flag to suppress ready signalling on open

[Johan Hovold]

Add a NORDY port flag to suppress raising the modem-control lines on
open to signal DTE readiness.

This can be used to implement a NORDY termios control flag to complement
HUPCL, which controls lowering of the modem-control lines on final
close.

Initially drivers can export the flag through sysfs, which also allows
control over the lines on first open.

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>
---
 drivers/tty/tty_port.c |  2 +-
 include/linux/tty.h    | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index ea80bf872f54..2613debc1d06 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -415,7 +415,7 @@ EXPORT_SYMBOL(tty_port_carrier_raised);
  */
 void tty_port_raise_dtr_rts(struct tty_port *port)
 {
-	if (port->ops->dtr_rts)
+	if (port->ops->dtr_rts && !tty_port_nordy(port))
 		port->ops->dtr_rts(port, 1);
 }
 EXPORT_SYMBOL(tty_port_raise_dtr_rts);
diff --git a/include/linux/tty.h b/include/linux/tty.h
index a99e9b8e4e31..31414efd8661 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -267,6 +267,7 @@ struct tty_port {
 #define TTY_PORT_CHECK_CD	4	/* carrier detect enabled */
 #define TTY_PORT_KOPENED	5	/* device exclusively opened by
 					   kernel */
+#define TTY_PORT_NORDY		6	/* do not raise DTR/RTS on open */
 
 /*
  * Where all of the state associated with a tty is kept while the tty
@@ -683,6 +684,19 @@ static inline void tty_port_set_kopened(struct tty_port *port, bool val)
 		clear_bit(TTY_PORT_KOPENED, &port->iflags);
 }
 
+static inline bool tty_port_nordy(struct tty_port *port)
+{
+	return test_bit(TTY_PORT_NORDY, &port->iflags);
+}
+
+static inline void tty_port_set_nordy(struct tty_port *port, bool val)
+{
+	if (val)
+		set_bit(TTY_PORT_NORDY, &port->iflags);
+	else
+		clear_bit(TTY_PORT_NORDY, &port->iflags);
+}
+
 extern struct tty_struct *tty_port_tty_get(struct tty_port *port);
 extern void tty_port_tty_set(struct tty_port *port, struct tty_struct *tty);
 extern int tty_port_carrier_raised(struct tty_port *port);
-- 
2.26.2

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

[Johan Hovold]

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(+)

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
 };
 
-- 
2.26.2

[PATCH 3/5] USB: serial: add sysfs attribute to suppress ready signalling on open

[Johan Hovold]

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>
---
 drivers/usb/serial/bus.c | 38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/bus.c b/drivers/usb/serial/bus.c
index eb0195cf37dd..37bbeab6666e 100644
--- a/drivers/usb/serial/bus.c
+++ b/drivers/usb/serial/bus.c
@@ -35,6 +35,40 @@ static int usb_serial_device_match(struct device *dev,
 	return 0;
 }
 
+static ssize_t nordy_show(struct device *dev, struct device_attribute *attr,
+				char *buf)
+{
+	struct usb_serial_port *port = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", tty_port_nordy(&port->port));
+}
+
+static ssize_t nordy_store(struct device *dev, struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct usb_serial_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->port, val);
+
+	return count;
+}
+static DEVICE_ATTR_RW(nordy);
+
+static struct attribute *tty_attrs[] = {
+	&dev_attr_nordy.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(tty);
+
 static int usb_serial_device_probe(struct device *dev)
 {
 	struct usb_serial_driver *driver;
@@ -60,8 +94,8 @@ static int usb_serial_device_probe(struct device *dev)
 	}
 
 	minor = port->minor;
-	tty_dev = tty_port_register_device(&port->port, usb_serial_tty_driver,
-					   minor, dev);
+	tty_dev = tty_port_register_device_attr(&port->port,
+			usb_serial_tty_driver, minor, dev, port, tty_groups);
 	if (IS_ERR(tty_dev)) {
 		retval = PTR_ERR(tty_dev);
 		goto err_port_remove;
-- 
2.26.2

[PATCH 4/5] USB: serial: ftdi_sio: pass port to quirk port_probe functions

[Johan Hovold]

From: "Mychaela N. Falconia" <falcon@freecalypso.org>

The original code passed only the pointer to the ftdi_private struct
to quirk port_probe functions.  However, some quirks may need to be
applied conditionally only to some channels of a multichannel FT2232x
or FT4232H device, and if a given quirk's port_probe function needs
to figure out which channel of a multichannel device is currently
being considered, it needs access to the port pointer passed to the
ftdi_sio_port_probe() function, so it can traverse USB data structures
from there.

Signed-off-by: Mychaela N. Falconia <falcon@freecalypso.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ftdi_sio.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index e0f4c3d9649c..b69032c9ec2b 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -88,15 +88,15 @@ struct ftdi_private {
 struct ftdi_sio_quirk {
 	int (*probe)(struct usb_serial *);
 	/* Special settings for probed ports. */
-	void (*port_probe)(struct ftdi_private *);
+	void (*port_probe)(struct usb_serial_port *);
 };
 
 static int   ftdi_jtag_probe(struct usb_serial *serial);
 static int   ftdi_NDI_device_setup(struct usb_serial *serial);
 static int   ftdi_stmclite_probe(struct usb_serial *serial);
 static int   ftdi_8u2232c_probe(struct usb_serial *serial);
-static void  ftdi_USB_UIRT_setup(struct ftdi_private *priv);
-static void  ftdi_HE_TIRA1_setup(struct ftdi_private *priv);
+static void  ftdi_USB_UIRT_setup(struct usb_serial_port *port);
+static void  ftdi_HE_TIRA1_setup(struct usb_serial_port *port);
 
 static const struct ftdi_sio_quirk ftdi_jtag_quirk = {
 	.probe	= ftdi_jtag_probe,
@@ -2252,11 +2252,11 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
 
 	mutex_init(&priv->cfg_lock);
 
-	if (quirk && quirk->port_probe)
-		quirk->port_probe(priv);
-
 	usb_set_serial_port_data(port, priv);
 
+	if (quirk && quirk->port_probe)
+		quirk->port_probe(port);
+
 	ftdi_determine_type(port);
 	ftdi_set_max_packet_size(port);
 	if (read_latency_timer(port) < 0)
@@ -2277,8 +2277,10 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
 /* Setup for the USB-UIRT device, which requires hardwired
  * baudrate (38400 gets mapped to 312500) */
 /* Called from usbserial:serial_probe */
-static void ftdi_USB_UIRT_setup(struct ftdi_private *priv)
+static void ftdi_USB_UIRT_setup(struct usb_serial_port *port)
 {
+	struct ftdi_private *priv = usb_get_serial_port_data(port);
+
 	priv->flags |= ASYNC_SPD_CUST;
 	priv->custom_divisor = 77;
 	priv->force_baud = 38400;
@@ -2287,8 +2289,10 @@ static void ftdi_USB_UIRT_setup(struct ftdi_private *priv)
 /* Setup for the HE-TIRA1 device, which requires hardwired
  * baudrate (38400 gets mapped to 100000) and RTS-CTS enabled.  */
 
-static void ftdi_HE_TIRA1_setup(struct ftdi_private *priv)
+static void ftdi_HE_TIRA1_setup(struct usb_serial_port *port)
 {
+	struct ftdi_private *priv = usb_get_serial_port_data(port);
+
 	priv->flags |= ASYNC_SPD_CUST;
 	priv->custom_divisor = 240;
 	priv->force_baud = 38400;
-- 
2.26.2

[PATCH 5/5] USB: serial: ftdi_sio: add support for FreeCalypso DUART28C adapter

[Johan Hovold]

From: "Mychaela N. Falconia" <falcon@freecalypso.org>

FreeCalypso DUART28C is an FT2232D-based USB to dual UART adapter
with a special quirk: Channel B RTS and DTR outputs (BDBUS2 and BDBUS4
on the chip) have been repurposed to drive PWON and RESET controls
on Calypso targets.  The circuit is wired such that BDBUS[24] high
(RTS/DTR inactive) is the normal state with Iota VRPC controls
NOT activated, whereas BDBUS[24] low (RTS or DTR active) turn ON
the corresponding open drain control signal drivers.

A special ftdi_sio driver quirk is needed in order to suppress
automatic assertion of DTR & RTS on device open: this device's
special PWON and RESET control drivers MUST NOT be activated
when the port is ordinarily opened for plain serial communication,
instead they must only be activated when a special userspace
application explicitly requests such activation with a TIOCMBIS ioctl.
These special userspace applications are responsible for making the
needed pulse with a TIOCMBIS, delay, TIOCMBIC sequence.

The special quirk is conditionalized on the DUART28C adapter's custom
USB ID, and is further limited to FT2232D Channel B only: Channel A
is wired normally, with the chip's ADBUS2 and ADBUS4 outputs
actually being RTS and DTR rather than something else.

Signed-off-by: Mychaela N. Falconia <falcon@freecalypso.org>
[johan: reimplement using new NORDY flag, trim quirk comment]
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/ftdi_sio.c     | 22 ++++++++++++++++++++++
 drivers/usb/serial/ftdi_sio_ids.h |  1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index b69032c9ec2b..b555bbc1b0a9 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -97,6 +97,7 @@ static int   ftdi_stmclite_probe(struct usb_serial *serial);
 static int   ftdi_8u2232c_probe(struct usb_serial *serial);
 static void  ftdi_USB_UIRT_setup(struct usb_serial_port *port);
 static void  ftdi_HE_TIRA1_setup(struct usb_serial_port *port);
+static void  ftdi_duart28c_setup(struct usb_serial_port *port);
 
 static const struct ftdi_sio_quirk ftdi_jtag_quirk = {
 	.probe	= ftdi_jtag_probe,
@@ -122,6 +123,10 @@ static const struct ftdi_sio_quirk ftdi_8u2232c_quirk = {
 	.probe	= ftdi_8u2232c_probe,
 };
 
+static const struct ftdi_sio_quirk ftdi_duart28c_quirk = {
+	.port_probe = ftdi_duart28c_setup,
+};
+
 /*
  * The 8U232AM has the same API as the sio except for:
  * - it can support MUCH higher baudrates; up to:
@@ -1042,6 +1047,8 @@ static const struct usb_device_id id_table_combined[] = {
 		.driver_info = (kernel_ulong_t)&ftdi_jtag_quirk },
 	{ USB_DEVICE(FTDI_VID, FTDI_FALCONIA_JTAG_UNBUF_PID),
 		.driver_info = (kernel_ulong_t)&ftdi_jtag_quirk },
+	{ USB_DEVICE(FTDI_VID, FTDI_FALCONIA_DUART28C_PID),
+		.driver_info = (kernel_ulong_t)&ftdi_duart28c_quirk },
 	{ }					/* Terminating entry */
 };
 
@@ -2386,6 +2393,21 @@ static int ftdi_stmclite_probe(struct usb_serial *serial)
 	return 0;
 }
 
+/*
+ * FreeCalypso DUART28C is an FT2232D-based USB to dual UART adapter
+ * with a special quirk: Channel B RTS and DTR outputs (BDBUS2 and BDBUS4
+ * on the chip) have been repurposed to drive PWON and RESET controls.
+ */
+static void ftdi_duart28c_setup(struct usb_serial_port *port)
+{
+	struct usb_serial *serial = port->serial;
+	struct usb_interface *intf = serial->interface;
+	int ifnum = intf->cur_altsetting->desc.bInterfaceNumber;
+
+	if (ifnum == 1)
+		tty_port_set_nordy(&port->port, 1);
+}
+
 static int ftdi_sio_port_remove(struct usb_serial_port *port)
 {
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
diff --git a/drivers/usb/serial/ftdi_sio_ids.h b/drivers/usb/serial/ftdi_sio_ids.h
index 3d47c6d72256..3081b8916a0a 100644
--- a/drivers/usb/serial/ftdi_sio_ids.h
+++ b/drivers/usb/serial/ftdi_sio_ids.h
@@ -45,6 +45,7 @@
  */
 #define FTDI_FALCONIA_JTAG_BUF_PID	0x7150
 #define FTDI_FALCONIA_JTAG_UNBUF_PID	0x7151
+#define FTDI_FALCONIA_DUART28C_PID	0x7152
 
 /* Sienna Serial Interface by Secyourit GmbH */
 #define FTDI_SIENNA_PID		0x8348
-- 
2.26.2

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

[Andy Shevchenko]

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;
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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.
>
> 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.

...

> +       ret = kstrtouint(buf, 0, &val);
> +       if (ret)
> +               return ret;
> +
> +       if (val > 1)
> +               return -EINVAL;

kstrtobool() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/5] tty: add flag to suppress ready signalling on open

[Mychaela Falconia]

A quick background for Greg and others who haven't seen the Sept-Oct
discussion between me and Johan on the linux-usb ML: I am the hardware
engineer who designed the FT2232D-based DUART28C adapter board, and it
was my desire to have this custom FT2232D adapter supported in mainline
Linux that triggered the chain of discussions and patch revisions that
led to the present patch series by Johan.

My FTDI-based USB to dual UART adapter is special in that the DTR &
RTS signals on one of the two UART channels (FT2232D Channel B
specifically) have been repurposed for a non-serial, non-modem-control
use: they control open drain drivers (a Nexperia 74LVC2G07 buffer IC)
which drive PWON & RESET control signals that would otherwise be
driven by short-to-ground human-finger-operated pushbutton switches.
The standard Unix/POSIX/Linux behaviour (going all the way back to
1970s Original UNIX) of automatically asserting DTR & RTS on serial
port open is a killer for custom hw in which these signals have been
repurposed, thus I need some way to suppress this automatic DTR & RTS
assertion on tty device open.  With automatic assertion on open
suppressed, these two signals can then be freely manipulated by
userspace via TIOCMBIS and TIOCMBIC.

There have certainly been other serial devices in the past (whether
true RS-232 or USB-serial) in which DTR and/or RTS has been repurposed
for some non-standard use that does not tolerate unwanted auto-assert
on every device open, and to the best of my knowledge this problem
does not occur on Windows - thus it is quite possible that some other
hw engineer somewhere out there could design and build a custom serial
or USB-serial device with repurposed DTR/RTS that works fine under
Windows, but the moment someone in Linux community tries to get it
working under our free OS, they will run into the problem of unwanted
DTR & RTS auto-assertion on device open.

In our previous discussion Johan said "this has come up the in past",
referring to repurposed DTR and/or RTS signals doing non-standard
things, thus I find it a little surprising that this issue hasn't been
solved long before my time - but I guess I must be the first to
complain loudly enough to get something done about it, and someone
always has to be the first...

Johan's patch series provides two workable solutions to the problem of
unwanted DTR & RTS auto-assertion:

1) For hardware engineers like me who design and build their own
boards with the USB-serial chip fully embedded and who have their own
custom USB IDs, a driver quirk can be applied (as part of adding
support for the new USB ID) that sets the new tty port flag upon
detecting the USB ID of the custom hw device for which it is required
- this approach provides the highest level of friendliness to the
ultimate end user of the finished hardware product.

2) For situations in which the luxury of a custom USB ID is not
available, e.g., a situation where the device that does not tolerate
automatic DTR/RTS assertion on open is a physical RS-232 device that
can be connected to "any" serial port, the new sysfs attribute comes
to the rescue.

Johan's patch comments say that the new flag can also be brought out
to termios in the future, similarly to HUPCL, but I question the
usefulness of doing so, as it is a chicken and egg problem: one needs
to open the tty device in order to do termios ioctls on it, and if
that initial open triggers DTR/RTS hardware actions, then the end user
is still screwed.  If Johan or someone else can see a potential use
case for manipulating this new flag via termios (as opposed to sysfs
or USB-ID-based driver quirks), perhaps you could elaborate on it?

Andy Shevchenko wrote:

> > Add a nordy sysfs attribute to suppress raising the modem-control lines
> > on open to signal DTE readiness.
>
> Why not call it nomctrl ?

I have no opinion one way or another as to what the new sysfs attribute
should be called - my use case won't involve this sysfs mechanism at
all, instead I care much more about the path where the tty port flag
gets set via a driver quirk upon seeing my custom USB ID. :)

The naming of the internal tty port flag is likewise a matter which I
gladly leave to more qualified kernel developers like Johan and Greg.

In any case, it would be really awesome if this patch series (with or
without further modifications) can make it into 5.10 - any chance of
such happening, or will it have to be pushed out to 5.11?

Sincerely,
Mychaela,
freelance hardware and software engineer,
she/her/hers

Re: [PATCH 1/5] tty: add port flag to suppress ready signalling on open

[Mychaela Falconia]

> Add a NORDY port flag to suppress raising the modem-control lines on
> open to signal DTE readiness.
>
> This can be used to implement a NORDY termios control flag to complement
> HUPCL, which controls lowering of the modem-control lines on final
> close.
>
> Initially drivers can export the flag through sysfs, which also allows
> control over the lines on first open.
>
> 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>

Reviewed-by: Mychaela N. Falconia <falcon@freecalypso.org>

Re: [PATCH 1/5] tty: add port flag to suppress ready signalling on open

[Jiri Slaby]

On 30. 11. 20, 16:37, Johan Hovold wrote:
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -683,6 +684,19 @@ static inline void tty_port_set_kopened(struct tty_port *port, bool val)
>   		clear_bit(TTY_PORT_KOPENED, &port->iflags);
>   }
>   
> +static inline bool tty_port_nordy(struct tty_port *port)

port can be const here.

> +{
> +	return test_bit(TTY_PORT_NORDY, &port->iflags);
> +}
> +
> +static inline void tty_port_set_nordy(struct tty_port *port, bool val)
> +{
> +	if (val)
> +		set_bit(TTY_PORT_NORDY, &port->iflags);
> +	else
> +		clear_bit(TTY_PORT_NORDY, &port->iflags);

We have assign_bit() for these cases these days.

thanks,
-- 
js

Re: [PATCH 5/5] USB: serial: ftdi_sio: add support for FreeCalypso DUART28C adapter

[Jiri Slaby]

On 30. 11. 20, 16:37, Johan Hovold wrote:
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
...
> @@ -2386,6 +2393,21 @@ static int ftdi_stmclite_probe(struct usb_serial *serial)
>   	return 0;
>   }
>   
> +/*
> + * FreeCalypso DUART28C is an FT2232D-based USB to dual UART adapter
> + * with a special quirk: Channel B RTS and DTR outputs (BDBUS2 and BDBUS4
> + * on the chip) have been repurposed to drive PWON and RESET controls.
> + */
> +static void ftdi_duart28c_setup(struct usb_serial_port *port)
> +{
> +	struct usb_serial *serial = port->serial;
> +	struct usb_interface *intf = serial->interface;
> +	int ifnum = intf->cur_altsetting->desc.bInterfaceNumber;
> +
> +	if (ifnum == 1)
> +		tty_port_set_nordy(&port->port, 1);

So s/1/true, provided the parameter is defined as bool now.

thanks,
-- 
js

Re: [PATCH 1/5] tty: add port flag to suppress ready signalling on open

[Mychaela Falconia]

On 11/30/20, Jiri Slaby <jirislaby@kernel.org> wrote:
> port can be const here.
> [...]
> We have assign_bit() for these cases these days.

Johan's patch adding test and set accessor inline functions for the
new flag follows the style of the existing accessor inline functions
for previously existing flags, for the sake of consistency. If we are
going to use the new style (const for test functions, assign_bit() for
set functions) for the new flag, then we should also change all
existing ones for consistency. In terms of patch splitting, would it
be most kosher to have one patch that updates the style of existing
accessor inline functions, and then the interesting patch that adds
the new flag?

M~

Re: [PATCH 0/5] tty: add flag to suppress ready signalling on open

[Jiri Slaby]

On 30. 11. 20, 22:22, Mychaela Falconia wrote:
> 2) For situations in which the luxury of a custom USB ID is not
> available, e.g., a situation where the device that does not tolerate
> automatic DTR/RTS assertion on open is a physical RS-232 device that
> can be connected to "any" serial port, the new sysfs attribute comes
> to the rescue.
> 
> Johan's patch comments say that the new flag can also be brought out
> to termios in the future, similarly to HUPCL,

The difference to other control flags is that open raises DTR/RTS in any 
case (i.e. including O_NONBLOCK) -- provided baud rate is set (and it is 
for casual serials). That means you cannot open a port to configure it 
(using e.g. setserial) without actually raising the DTR/RTS.

> but I question the
> usefulness of doing so, as it is a chicken and egg problem: one needs
> to open the tty device in order to do termios ioctls on it, and if
> that initial open triggers DTR/RTS hardware actions, then the end user
> is still screwed.  If Johan or someone else can see a potential use
> case for manipulating this new flag via termios (as opposed to sysfs
> or USB-ID-based driver quirks), perhaps you could elaborate on it?

We would need to (ab)use another open flag (e.g. O_DIRECT). I am not 
biased to either of solutions.

thanks,
-- 
js

Re: [PATCH 1/5] tty: add port flag to suppress ready signalling on open

[Jiri Slaby]

On 01. 12. 20, 8:09, Mychaela Falconia wrote:
> On 11/30/20, Jiri Slaby <jirislaby@kernel.org> wrote:
>> port can be const here.
>> [...]
>> We have assign_bit() for these cases these days.
> 
> Johan's patch adding test and set accessor inline functions for the
> new flag follows the style of the existing accessor inline functions
> for previously existing flags, for the sake of consistency. If we are
> going to use the new style (const for test functions, assign_bit() for
> set functions) for the new flag, then we should also change all
> existing ones for consistency. In terms of patch splitting, would it
> be most kosher to have one patch that updates the style of existing
> accessor inline functions, and then the interesting patch that adds
> the new flag?

Yes. Or the other way around. Add this new using const+assign_bit and 
convert the rest on the top of the series.

thanks,
-- 
js

Re: [PATCH 0/5] tty: add flag to suppress ready signalling on open

[Mychaela Falconia]

On 11/30/20, Jiri Slaby <jirislaby@kernel.org> wrote:
> The difference to other control flags is that open raises DTR/RTS in any
> case (i.e. including O_NONBLOCK)

Yes, this is the exact root-cause problem I am trying to fix, with Johan's help.

> -- provided baud rate is set (and it is
> for casual serials). That means you cannot open a port to configure it
> (using e.g. setserial) without actually raising the DTR/RTS.

Exactly.

M~

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

[Johan Hovold]

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

Re: [PATCH 0/5] tty: add flag to suppress ready signalling on open

[Johan Hovold]

On Mon, Nov 30, 2020 at 01:22:07PM -0800, Mychaela Falconia wrote:

> Johan's patch comments say that the new flag can also be brought out
> to termios in the future, similarly to HUPCL, but I question the
> usefulness of doing so, as it is a chicken and egg problem: one needs
> to open the tty device in order to do termios ioctls on it, and if
> that initial open triggers DTR/RTS hardware actions, then the end user
> is still screwed.  If Johan or someone else can see a potential use
> case for manipulating this new flag via termios (as opposed to sysfs
> or USB-ID-based driver quirks), perhaps you could elaborate on it?

Depending on the application it may be acceptable to assert the
modem-control lines on first open (e.g. before initialisation). 

A new termios flag allows for a generic interface which could be shared
with other OSes and controlled through stty.

In case that isn't sufficient and you need control over the default port
setting, you can always fall back to the Linux-specific sysfs interface.

> In any case, it would be really awesome if this patch series (with or
> without further modifications) can make it into 5.10 - any chance of
> such happening, or will it have to be pushed out to 5.11?

Let's see, but I don't think this necessarily has to take that long to
get merged.

Johan

Re: [PATCH 1/5] tty: add port flag to suppress ready signalling on open

[Johan Hovold]

On Tue, Dec 01, 2020 at 06:49:07AM +0100, Jiri Slaby wrote:
> On 30. 11. 20, 16:37, Johan Hovold wrote:
> > --- a/include/linux/tty.h
> > +++ b/include/linux/tty.h
> > @@ -683,6 +684,19 @@ static inline void tty_port_set_kopened(struct tty_port *port, bool val)
> >   		clear_bit(TTY_PORT_KOPENED, &port->iflags);
> >   }
> >   
> > +static inline bool tty_port_nordy(struct tty_port *port)
> 
> port can be const here.

Sure, but see below.

> > +{
> > +	return test_bit(TTY_PORT_NORDY, &port->iflags);
> > +}
> > +
> > +static inline void tty_port_set_nordy(struct tty_port *port, bool val)
> > +{
> > +	if (val)
> > +		set_bit(TTY_PORT_NORDY, &port->iflags);
> > +	else
> > +		clear_bit(TTY_PORT_NORDY, &port->iflags);
> 
> We have assign_bit() for these cases these days.

Right, but for both your comments this follows the pattern used by the
other port-flag helpers.

I can add a preparatory patch updating the current helpers, but I don't
think this needs to be a blocker.

Johan

Re: [PATCH 5/5] USB: serial: ftdi_sio: add support for FreeCalypso DUART28C adapter

[Johan Hovold]

On Tue, Dec 01, 2020 at 07:54:10AM +0100, Jiri Slaby wrote:
> On 30. 11. 20, 16:37, Johan Hovold wrote:
> > --- a/drivers/usb/serial/ftdi_sio.c
> > +++ b/drivers/usb/serial/ftdi_sio.c
> ...
> > @@ -2386,6 +2393,21 @@ static int ftdi_stmclite_probe(struct usb_serial *serial)
> >   	return 0;
> >   }
> >   
> > +/*
> > + * FreeCalypso DUART28C is an FT2232D-based USB to dual UART adapter
> > + * with a special quirk: Channel B RTS and DTR outputs (BDBUS2 and BDBUS4
> > + * on the chip) have been repurposed to drive PWON and RESET controls.
> > + */
> > +static void ftdi_duart28c_setup(struct usb_serial_port *port)
> > +{
> > +	struct usb_serial *serial = port->serial;
> > +	struct usb_interface *intf = serial->interface;
> > +	int ifnum = intf->cur_altsetting->desc.bInterfaceNumber;
> > +
> > +	if (ifnum == 1)
> > +		tty_port_set_nordy(&port->port, 1);
> 
> So s/1/true, provided the parameter is defined as bool now.

Also here I'm following the convention used for the other port-flag
helper which are used with numerical constants (just about) consistently
throughout the tree. 

Johan

Re: [PATCH 0/5] tty: add flag to suppress ready signalling on open

[Andy Shevchenko]

On Mon, Nov 30, 2020 at 11:25 PM Mychaela Falconia
<mychaela.falconia@gmail.com> wrote:

...

> Johan's patch comments say that the new flag can also be brought out
> to termios in the future, similarly to HUPCL, but I question the
> usefulness of doing so, as it is a chicken and egg problem: one needs
> to open the tty device in order to do termios ioctls on it, and if
> that initial open triggers DTR/RTS hardware actions, then the end user
> is still screwed.  If Johan or someone else can see a potential use
> case for manipulating this new flag via termios (as opposed to sysfs
> or USB-ID-based driver quirks), perhaps you could elaborate on it?

Thanks for the very detailed description of what you are working on.
Unfortunately I have no thoughts about alternative solutions.

> Andy Shevchenko wrote:
>
> > > Add a nordy sysfs attribute to suppress raising the modem-control lines
> > > on open to signal DTE readiness.
> >
> > Why not call it nomctrl ?
>
> I have no opinion one way or another as to what the new sysfs attribute
> should be called - my use case won't involve this sysfs mechanism at
> all, instead I care much more about the path where the tty port flag
> gets set via a driver quirk upon seeing my custom USB ID. :)

Then why do we bother with sysfs right now? It's an ABI and Johan is
completely aware and knows that once it's in the kernel it is close to
being carved in stone.
I would vote to remove sysfs from now and see if we really need it in
the future.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/5] tty: add flag to suppress ready signalling on open

[Johan Hovold]

On Tue, Dec 01, 2020 at 12:48:48PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 11:25 PM Mychaela Falconia
> <mychaela.falconia@gmail.com> wrote:

> > > Why not call it nomctrl ?
> >
> > I have no opinion one way or another as to what the new sysfs attribute
> > should be called - my use case won't involve this sysfs mechanism at
> > all, instead I care much more about the path where the tty port flag
> > gets set via a driver quirk upon seeing my custom USB ID. :)
> 
> Then why do we bother with sysfs right now? It's an ABI and Johan is
> completely aware and knows that once it's in the kernel it is close to
> being carved in stone.
> I would vote to remove sysfs from now and see if we really need it in
> the future.

Eh, because this is generally useful and has come up in the past. I'm
not interested in adding quirks for odd devices that want non-standard
behaviour that we need to maintain indefinitely; that's precisely why I
proposed a general interface that can be use with any serial port.

Johan

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

[Johan Hovold]

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

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Johan Hovold]

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

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

[Andy Shevchenko]

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!


-- 
With Best Regards,
Andy Shevchenko

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

[Greg Kroah-Hartman]

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

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

[Johan Hovold]

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

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

[Johan Hovold]

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

Re: [PATCH 0/5] tty: add flag to suppress ready signalling on open

[Johan Hovold]

On Tue, Dec 01, 2020 at 08:14:07AM +0100, Jiri Slaby wrote:
> On 30. 11. 20, 22:22, Mychaela Falconia wrote:
> > 2) For situations in which the luxury of a custom USB ID is not
> > available, e.g., a situation where the device that does not tolerate
> > automatic DTR/RTS assertion on open is a physical RS-232 device that
> > can be connected to "any" serial port, the new sysfs attribute comes
> > to the rescue.
> > 
> > Johan's patch comments say that the new flag can also be brought out
> > to termios in the future, similarly to HUPCL,
> 
> The difference to other control flags is that open raises DTR/RTS in any 
> case (i.e. including O_NONBLOCK) -- provided baud rate is set (and it is 
> for casual serials). That means you cannot open a port to configure it 
> (using e.g. setserial) without actually raising the DTR/RTS.

Right, but depending on the application this may be ok (e.g. reset and
initialise on first open after boot, which may have triggered a reset
anyway).

If control over first open is needed, the sysfs interface provides that
out-of-band.

> > but I question the
> > usefulness of doing so, as it is a chicken and egg problem: one needs
> > to open the tty device in order to do termios ioctls on it, and if
> > that initial open triggers DTR/RTS hardware actions, then the end user
> > is still screwed.  If Johan or someone else can see a potential use
> > case for manipulating this new flag via termios (as opposed to sysfs
> > or USB-ID-based driver quirks), perhaps you could elaborate on it?
> 
> We would need to (ab)use another open flag (e.g. O_DIRECT). I am not 
> biased to either of solutions.

Forgot to mention that using open-flags would prevent using standard
utilities like cat, echo and terminal programs. So for that reason a
termios and/or sysfs interface is also preferred.

Johan

Re: [PATCH 0/5] tty: add flag to suppress ready signalling on open

[Jiri Slaby]

On 02. 12. 20, 12:48, Johan Hovold wrote:
>>> but I question the
>>> usefulness of doing so, as it is a chicken and egg problem: one needs
>>> to open the tty device in order to do termios ioctls on it, and if
>>> that initial open triggers DTR/RTS hardware actions, then the end user
>>> is still screwed.  If Johan or someone else can see a potential use
>>> case for manipulating this new flag via termios (as opposed to sysfs
>>> or USB-ID-based driver quirks), perhaps you could elaborate on it?
>>
>> We would need to (ab)use another open flag (e.g. O_DIRECT). I am not
>> biased to either of solutions.
> 
> Forgot to mention that using open-flags would prevent using standard
> utilities like cat, echo and terminal programs. So for that reason a
> termios and/or sysfs interface is also preferred.

Nope, I meant it differently. You set it up once using the special open 
flag. Like with setserial, one sets I/O port, irqs etc. and then uses 
standard tools as the port is already set up (marked as NORDY in this case).

thanks,
-- 
js

Re: [PATCH 0/5] tty: add flag to suppress ready signalling on open

[Johan Hovold]

On Fri, Dec 04, 2020 at 07:58:53AM +0100, Jiri Slaby wrote:
> On 02. 12. 20, 12:48, Johan Hovold wrote:
> >>> but I question the
> >>> usefulness of doing so, as it is a chicken and egg problem: one needs
> >>> to open the tty device in order to do termios ioctls on it, and if
> >>> that initial open triggers DTR/RTS hardware actions, then the end user
> >>> is still screwed.  If Johan or someone else can see a potential use
> >>> case for manipulating this new flag via termios (as opposed to sysfs
> >>> or USB-ID-based driver quirks), perhaps you could elaborate on it?
> >>
> >> We would need to (ab)use another open flag (e.g. O_DIRECT). I am not
> >> biased to either of solutions.
> > 
> > Forgot to mention that using open-flags would prevent using standard
> > utilities like cat, echo and terminal programs. So for that reason a
> > termios and/or sysfs interface is also preferred.
> 
> Nope, I meant it differently. You set it up once using the special open 
> flag. Like with setserial, one sets I/O port, irqs etc. and then uses 
> standard tools as the port is already set up (marked as NORDY in this
> case).

Ok, but leaving the open flag abuse aside, that would still require a
custom tool to do the setup.

There are also no other examples of such an interface with a "sticky"
open flag affecting later opens. But you probably meant that the open
flag only affects the current open, and then the termios flag is used
to make the setting stick. 

Note that having a udev rule handle this at boot using a sysfs interface
does not require any custom tools at all.

And in theory nothing prevents extending/abusing POSIX with such an open
behaviour later.

Johan