Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v6 0/4] leds: trigger: implement a tty trigger
@ 2020-02-13  9:15 Uwe Kleine-König
  2020-02-13  9:15 ` [PATCH v6 1/4] lib: new helper kstrtodev_t() Uwe Kleine-König
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: Uwe Kleine-König @ 2020-02-13  9:15 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek, Dan Murphy, Greg Kroah-Hartman,
	Jiri Slaby
  Cc: Uwe Kleine-König, linux-leds, linux-kernel, kernel, linux-serial

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Hello,

This is v6 of my quest to introduce ledtriggers for UARTs. The previous
series is available at

	http://lore.kernel.org/r/20191219093947.15502-1-u.kleine-koenig@pengutronix.de

The changes compared to that are that parsing of the dev parameter is
more strict and that I set brightness directly from the kworker instead
of using led_blink_set_oneshot which makes use of another kworker. (Both
requested by Pavel Machek.)

For the former I introduced a new helper kstrtodev_t() in the spirit of
kstrtoul() to implement the stricter parsing (instead of the lax one
using plain sscanf() in v5).

Best regards
Uwe

Uwe Kleine-König (4):
  lib: new helper kstrtodev_t()
  tty: rename tty_kopen() and add new function tty_kopen_shared()
  tty: new helper function tty_get_icount()
  leds: trigger: implement a tty trigger

 .../ABI/testing/sysfs-class-led-trigger-tty   |   6 +
 drivers/leds/trigger/Kconfig                  |   7 +
 drivers/leds/trigger/Makefile                 |   1 +
 drivers/leds/trigger/ledtrig-tty.c            | 159 ++++++++++++++++++
 drivers/staging/speakup/spk_ttyio.c           |   2 +-
 drivers/tty/tty_io.c                          |  87 +++++++---
 include/linux/kdev_t.h                        |   2 +
 include/linux/kernel.h                        |   1 +
 include/linux/tty.h                           |   7 +-
 lib/kstrtox.c                                 |  46 +++++
 10 files changed, 293 insertions(+), 25 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-tty
 create mode 100644 drivers/leds/trigger/ledtrig-tty.c


base-commit: 0bf999f9c5e74c7ecf9dafb527146601e5c848b9
-- 
2.24.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v6 1/4] lib: new helper kstrtodev_t()
  2020-02-13  9:15 [PATCH v6 0/4] leds: trigger: implement a tty trigger Uwe Kleine-König
@ 2020-02-13  9:15 ` Uwe Kleine-König
  2020-02-19 19:50   ` Andy Shevchenko
  2020-02-13  9:15 ` [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared() Uwe Kleine-König
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Uwe Kleine-König @ 2020-02-13  9:15 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek, Dan Murphy, Greg Kroah-Hartman,
	Jiri Slaby
  Cc: linux-leds, linux-kernel, kernel, linux-serial

This function is in the same spirit as the other kstrto* functions and
uses the same calling convention. It expects the input string to be in
the format %u:%u and implements stricter parsing than sscanf as it
returns an error on trailing data (other than the usual \n).

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 include/linux/kdev_t.h |  2 ++
 include/linux/kernel.h |  1 +
 lib/kstrtox.c          | 46 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+)

diff --git a/include/linux/kdev_t.h b/include/linux/kdev_t.h
index 85b5151911cf..3a5c24bd8fa4 100644
--- a/include/linux/kdev_t.h
+++ b/include/linux/kdev_t.h
@@ -4,8 +4,10 @@
 
 #include <uapi/linux/kdev_t.h>
 
+/* dev_t is 32 bit wide, 20 bits are used for MINOR, 12 for major */
 #define MINORBITS	20
 #define MINORMASK	((1U << MINORBITS) - 1)
+#define MAJORBITS	12
 
 #define MAJOR(dev)	((unsigned int) ((dev) >> MINORBITS))
 #define MINOR(dev)	((unsigned int) ((dev) & MINORMASK))
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0d9db2a14f44..9cf694c5d2c3 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -410,6 +410,7 @@ int __must_check kstrtos16(const char *s, unsigned int base, s16 *res);
 int __must_check kstrtou8(const char *s, unsigned int base, u8 *res);
 int __must_check kstrtos8(const char *s, unsigned int base, s8 *res);
 int __must_check kstrtobool(const char *s, bool *res);
+int __must_check kstrtodev_t(const char *s, dev_t *res);
 
 int __must_check kstrtoull_from_user(const char __user *s, size_t count, unsigned int base, unsigned long long *res);
 int __must_check kstrtoll_from_user(const char __user *s, size_t count, unsigned int base, long long *res);
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 1006bf70bf74..e1b896635c6a 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -19,6 +19,7 @@
 #include <linux/export.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
+#include <linux/kdev_t.h>
 #include "kstrtox.h"
 
 const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)
@@ -367,6 +368,51 @@ int kstrtobool(const char *s, bool *res)
 }
 EXPORT_SYMBOL(kstrtobool);
 
+/**
+ * kstrtodev_t - convert a string in format %u:%u to a dev_t
+ * @s: input string
+ * @res: result
+ *
+ * This is the reverse of print_dev_t. The first number is interpreted as major,
+ * the second as minor.
+ */
+int kstrtodev_t(const char *s, dev_t *res)
+{
+	unsigned long long _res;
+	unsigned int ma, mi;
+	int rv;
+
+	rv = _parse_integer(s, 10, &_res);
+	if (rv < 0)
+		return rv;
+	if (rv & KSTRTOX_OVERFLOW || _res >= (1U << MAJORBITS))
+		return -ERANGE;
+	ma = _res;
+
+	s += rv;
+
+	if (*s++ != ':')
+		return -EINVAL;
+
+	rv = _parse_integer(s, 10, &_res);
+	if (rv < 0)
+		return rv;
+	if (rv & KSTRTOX_OVERFLOW || _res >= (1U << MINORBITS))
+		return -ERANGE;
+	mi = _res;
+
+	s += rv;
+
+	if (*s == '\n')
+		s++;
+	if (*s)
+		return -EINVAL;
+
+	*res = MKDEV(ma, mi);
+	return 0;
+}
+EXPORT_SYMBOL(kstrtodev_t);
+
 /*
  * Since "base" would be a nonsense argument, this open-codes the
  * _from_user helper instead of using the helper macro below.
-- 
2.24.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared()
  2020-02-13  9:15 [PATCH v6 0/4] leds: trigger: implement a tty trigger Uwe Kleine-König
  2020-02-13  9:15 ` [PATCH v6 1/4] lib: new helper kstrtodev_t() Uwe Kleine-König
@ 2020-02-13  9:15 ` Uwe Kleine-König
  2020-02-19 13:21   ` Johan Hovold
  2020-02-13  9:15 ` [PATCH v6 3/4] tty: new helper function tty_get_icount() Uwe Kleine-König
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Uwe Kleine-König @ 2020-02-13  9:15 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek, Dan Murphy, Greg Kroah-Hartman,
	Jiri Slaby
  Cc: Uwe Kleine-König, linux-leds, linux-kernel, kernel, linux-serial

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Introduce a new function tty_kopen_shared() that yields a struct
tty_struct. The semantic difference to tty_kopen() is that the tty is
expected to be used already. So rename tty_kopen() to
tty_kopen_exclusive() for clearness, adapt the single user and put the
common code in a new static helper function.

tty_kopen_shared is to be used to implement an LED trigger for tty
devices in one of the next patches.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/staging/speakup/spk_ttyio.c |  2 +-
 drivers/tty/tty_io.c                | 58 ++++++++++++++++++++---------
 include/linux/tty.h                 |  5 ++-
 3 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/drivers/staging/speakup/spk_ttyio.c b/drivers/staging/speakup/spk_ttyio.c
index 5a9eff08cb96..e9db06eae875 100644
--- a/drivers/staging/speakup/spk_ttyio.c
+++ b/drivers/staging/speakup/spk_ttyio.c
@@ -147,7 +147,7 @@ static int spk_ttyio_initialise_ldisc(struct spk_synth *synth)
 	if (ret)
 		return ret;
 
-	tty = tty_kopen(dev);
+	tty = tty_kopen_exclusive(dev);
 	if (IS_ERR(tty))
 		return PTR_ERR(tty);
 
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index a1453fe10862..b718666ce73c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1875,22 +1875,7 @@ static struct tty_driver *tty_lookup_driver(dev_t device, struct file *filp,
 	return driver;
 }
 
-/**
- *	tty_kopen	-	open a tty device for kernel
- *	@device: dev_t of device to open
- *
- *	Opens tty exclusively for kernel. Performs the driver lookup,
- *	makes sure it's not already opened and performs the first-time
- *	tty initialization.
- *
- *	Returns the locked initialized &tty_struct
- *
- *	Claims the global tty_mutex to serialize:
- *	  - concurrent first-time tty initialization
- *	  - concurrent tty driver removal w/ lookup
- *	  - concurrent tty removal from driver table
- */
-struct tty_struct *tty_kopen(dev_t device)
+static struct tty_struct *tty_kopen(dev_t device, int shared)
 {
 	struct tty_struct *tty;
 	struct tty_driver *driver;
@@ -1905,7 +1890,7 @@ struct tty_struct *tty_kopen(dev_t device)
 
 	/* check whether we're reopening an existing tty */
 	tty = tty_driver_lookup_tty(driver, NULL, index);
-	if (IS_ERR(tty))
+	if (IS_ERR(tty) || shared)
 		goto out;
 
 	if (tty) {
@@ -1923,7 +1908,44 @@ struct tty_struct *tty_kopen(dev_t device)
 	tty_driver_kref_put(driver);
 	return tty;
 }
-EXPORT_SYMBOL_GPL(tty_kopen);
+
+/**
+ *	tty_kopen_exclusive	-	open a tty device for kernel
+ *	@device: dev_t of device to open
+ *
+ *	Opens tty exclusively for kernel. Performs the driver lookup,
+ *	makes sure it's not already opened and performs the first-time
+ *	tty initialization.
+ *
+ *	Returns the locked initialized &tty_struct
+ *
+ *	Claims the global tty_mutex to serialize:
+ *	  - concurrent first-time tty initialization
+ *	  - concurrent tty driver removal w/ lookup
+ *	  - concurrent tty removal from driver table
+ */
+struct tty_struct *tty_kopen_exclusive(dev_t device)
+{
+	return tty_kopen(device, 0);
+}
+EXPORT_SYMBOL_GPL(tty_kopen_exclusive);
+
+/**
+ *	tty_kopen_shared	-	open a tty device for shared in-kernel use
+ *	@device: dev_t of device to open
+ *
+ *	Opens an already existing tty
+ *	rnel. Performs the driver lookup,
+ *	makes sure it's not already opened and performs the first-time
+ *	tty initialization.
+ *
+ *	Locking is identical to tty_kopen() above.
+ */
+struct tty_struct *tty_kopen_shared(dev_t device)
+{
+	return tty_kopen(device, 1);
+}
+EXPORT_SYMBOL_GPL(tty_kopen_shared);
 
 /**
  *	tty_open_by_driver	-	open a tty device
diff --git a/include/linux/tty.h b/include/linux/tty.h
index bfa4e2ee94a9..d0bcf3226fb2 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -411,7 +411,8 @@ extern struct tty_struct *get_current_tty(void);
 /* tty_io.c */
 extern int __init tty_init(void);
 extern const char *tty_name(const struct tty_struct *tty);
-extern struct tty_struct *tty_kopen(dev_t device);
+extern struct tty_struct *tty_kopen_exclusive(dev_t device);
+extern struct tty_struct *tty_kopen_shared(dev_t device);
 extern void tty_kclose(struct tty_struct *tty);
 extern int tty_dev_name_to_number(const char *name, dev_t *number);
 extern int tty_ldisc_lock(struct tty_struct *tty, unsigned long timeout);
@@ -436,7 +437,7 @@ static inline int __init tty_init(void)
 { return 0; }
 static inline const char *tty_name(const struct tty_struct *tty)
 { return "(none)"; }
-static inline struct tty_struct *tty_kopen(dev_t device)
+static inline struct tty_struct *tty_kopen_exclusive(dev_t device)
 { return ERR_PTR(-ENODEV); }
 static inline void tty_kclose(struct tty_struct *tty)
 { }
-- 
2.24.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v6 3/4] tty: new helper function tty_get_icount()
  2020-02-13  9:15 [PATCH v6 0/4] leds: trigger: implement a tty trigger Uwe Kleine-König
  2020-02-13  9:15 ` [PATCH v6 1/4] lib: new helper kstrtodev_t() Uwe Kleine-König
  2020-02-13  9:15 ` [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared() Uwe Kleine-König
@ 2020-02-13  9:15 ` Uwe Kleine-König
  2020-02-13  9:16 ` [PATCH v6 4/4] leds: trigger: implement a tty trigger Uwe Kleine-König
  2020-02-19 10:40 ` [PATCH v6 0/4] " Greg Kroah-Hartman
  4 siblings, 0 replies; 21+ messages in thread
From: Uwe Kleine-König @ 2020-02-13  9:15 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek, Dan Murphy, Greg Kroah-Hartman,
	Jiri Slaby
  Cc: Uwe Kleine-König, linux-leds, linux-kernel, kernel, linux-serial

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

For a given struct tty_struct this yields the corresponding statistics
about sent and received characters (and some more) which is needed to
implement an LED trigger for tty devices.

The new function is then used to simplify tty_tiocgicount().

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/tty_io.c | 29 +++++++++++++++++++++++++----
 include/linux/tty.h  |  2 ++
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b718666ce73c..3e74e6cec1a6 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2492,15 +2492,36 @@ static int tty_tiocmset(struct tty_struct *tty, unsigned int cmd,
 	return tty->ops->tiocmset(tty, set, clear);
 }
 
+/**
+ *	tty_get_icount		-	get tty statistics
+ *	@tty: tty device
+ *	@icount: output parameter
+ *
+ *	Gets a copy of the tty's icount statistics.
+ *
+ *	Locking: none (up to the driver)
+ */
+int tty_get_icount(struct tty_struct *tty,
+		   struct serial_icounter_struct *icount)
+{
+	memset(icount, 0, sizeof(*icount));
+
+	if (tty->ops->get_icount)
+		return tty->ops->get_icount(tty, icount);
+	else
+		return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(tty_get_icount);
+
 static int tty_tiocgicount(struct tty_struct *tty, void __user *arg)
 {
-	int retval = -EINVAL;
 	struct serial_icounter_struct icount;
-	memset(&icount, 0, sizeof(icount));
-	if (tty->ops->get_icount)
-		retval = tty->ops->get_icount(tty, &icount);
+	int retval;
+
+	retval = tty_get_icount(tty, &icount);
 	if (retval != 0)
 		return retval;
+
 	if (copy_to_user(arg, &icount, sizeof(icount)))
 		return -EFAULT;
 	return 0;
diff --git a/include/linux/tty.h b/include/linux/tty.h
index d0bcf3226fb2..40ea5c409619 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -495,6 +495,8 @@ extern void tty_unthrottle(struct tty_struct *tty);
 extern int tty_throttle_safe(struct tty_struct *tty);
 extern int tty_unthrottle_safe(struct tty_struct *tty);
 extern int tty_do_resize(struct tty_struct *tty, struct winsize *ws);
+extern int tty_get_icount(struct tty_struct *tty,
+			  struct serial_icounter_struct *icount);
 extern int is_current_pgrp_orphaned(void);
 extern void tty_hangup(struct tty_struct *tty);
 extern void tty_vhangup(struct tty_struct *tty);
-- 
2.24.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v6 4/4] leds: trigger: implement a tty trigger
  2020-02-13  9:15 [PATCH v6 0/4] leds: trigger: implement a tty trigger Uwe Kleine-König
                   ` (2 preceding siblings ...)
  2020-02-13  9:15 ` [PATCH v6 3/4] tty: new helper function tty_get_icount() Uwe Kleine-König
@ 2020-02-13  9:16 ` Uwe Kleine-König
  2020-02-19 10:52   ` Johan Hovold
  2020-02-19 10:40 ` [PATCH v6 0/4] " Greg Kroah-Hartman
  4 siblings, 1 reply; 21+ messages in thread
From: Uwe Kleine-König @ 2020-02-13  9:16 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek, Dan Murphy, Greg Kroah-Hartman,
	Jiri Slaby
  Cc: Uwe Kleine-König, linux-leds, linux-kernel, kernel, linux-serial

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Usage is as follows:

	myled=ledname
	tty=ttyS0

	echo tty > /sys/class/leds/$myled/trigger
	cat /sys/class/tty/$tty/dev > /sys/class/leds/$myled/dev

. When this new trigger is active it periodically checks the tty's
statistics and when it changed since the last check the led is flashed
once.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 .../ABI/testing/sysfs-class-led-trigger-tty   |   6 +
 drivers/leds/trigger/Kconfig                  |   7 +
 drivers/leds/trigger/Makefile                 |   1 +
 drivers/leds/trigger/ledtrig-tty.c            | 159 ++++++++++++++++++
 4 files changed, 173 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-tty
 create mode 100644 drivers/leds/trigger/ledtrig-tty.c

diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
new file mode 100644
index 000000000000..f56f9721c317
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
@@ -0,0 +1,6 @@
+What:		/sys/class/leds/<led>/dev
+Date:		Dec 2019
+KernelVersion:	5.6
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Specifies $major:$minor of the triggering tty
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index ce9429ca6dde..40ff08c93f56 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -144,4 +144,11 @@ config LEDS_TRIGGER_AUDIO
 	  the audio mute and mic-mute changes.
 	  If unsure, say N
 
+config LEDS_TRIGGER_TTY
+	tristate "LED Trigger for TTY devices"
+	depends on TTY
+	help
+	  This allows LEDs to be controlled by activity on ttys which includes
+	  serial devices like /dev/ttyS0.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 733a83e2a718..25c4db97cdd4 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
 obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
 obj-$(CONFIG_LEDS_TRIGGER_PATTERN)	+= ledtrig-pattern.o
 obj-$(CONFIG_LEDS_TRIGGER_AUDIO)	+= ledtrig-audio.o
+obj-$(CONFIG_LEDS_TRIGGER_TTY)		+= ledtrig-tty.o
diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
new file mode 100644
index 000000000000..e1be93c3026f
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -0,0 +1,159 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/delay.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <uapi/linux/serial.h>
+
+struct ledtrig_tty_data {
+	struct led_classdev *led_cdev;
+	struct delayed_work dwork;
+	struct tty_struct *tty;
+	dev_t device;
+	int rx, tx;
+};
+
+static void ledtrig_tty_halt(struct ledtrig_tty_data *trigger_data)
+{
+	cancel_delayed_work_sync(&trigger_data->dwork);
+}
+
+static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data)
+{
+	schedule_delayed_work(&trigger_data->dwork, 0);
+}
+
+static ssize_t dev_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
+	ssize_t len = 0;
+
+	if (trigger_data->tty)
+		len = sprintf(buf, "%d:%d\n", MAJOR(trigger_data->device),
+			      MINOR(trigger_data->device));
+
+	return len;
+}
+
+static ssize_t dev_store(struct device *dev,
+			 struct device_attribute *attr, const char *buf,
+			 size_t size)
+{
+	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
+	struct tty_struct *tty;
+	dev_t d;
+	int ret;
+
+	if (size == 0 || (size == 1 && buf[0] == '\n')) {
+		tty = NULL;
+	} else {
+		ret = kstrtodev_t(buf, &d);
+		if (ret < 0)
+			return ret;
+
+		tty = tty_kopen_shared(d);
+		if (IS_ERR(tty))
+			return PTR_ERR(tty);
+	}
+
+	ledtrig_tty_halt(trigger_data);
+
+	tty_kref_put(trigger_data->tty);
+	trigger_data->tty = tty;
+	trigger_data->device = d;
+
+	if (tty) {
+		struct serial_icounter_struct icount;
+		ret = tty_get_icount(trigger_data->tty, &icount);
+		if (!ret) {
+			trigger_data->tx = icount.tx;
+			trigger_data->rx = icount.rx;
+		}
+
+		ledtrig_tty_restart(trigger_data);
+	}
+
+	return size;
+}
+static DEVICE_ATTR_RW(dev);
+
+static void ledtrig_tty_work(struct work_struct *work)
+{
+	struct ledtrig_tty_data *trigger_data =
+		container_of(work, struct ledtrig_tty_data, dwork.work);
+	struct serial_icounter_struct icount;
+	int ret;
+
+	if (!trigger_data->tty) {
+		led_set_brightness(trigger_data->led_cdev, LED_OFF);
+		return;
+	}
+
+	ret = tty_get_icount(trigger_data->tty, &icount);
+	if (ret)
+		return;
+
+	while (icount.rx != trigger_data->rx ||
+	       icount.tx != trigger_data->tx) {
+		led_set_brightness(trigger_data->led_cdev, LED_ON);
+
+		msleep(100);
+
+		led_set_brightness(trigger_data->led_cdev, LED_OFF);
+
+		trigger_data->rx = icount.rx;
+		trigger_data->tx = icount.tx;
+
+		ret = tty_get_icount(trigger_data->tty, &icount);
+		if (ret)
+			return;
+	}
+
+	schedule_delayed_work(&trigger_data->dwork, msecs_to_jiffies(100));
+}
+
+static struct attribute *ledtrig_tty_attrs[] = {
+	&dev_attr_dev.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(ledtrig_tty);
+
+static int ledtrig_tty_activate(struct led_classdev *led_cdev)
+{
+	struct ledtrig_tty_data *trigger_data;
+
+	trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
+	if (!trigger_data)
+		return -ENOMEM;
+
+	led_set_trigger_data(led_cdev, trigger_data);
+
+	INIT_DELAYED_WORK(&trigger_data->dwork, ledtrig_tty_work);
+	trigger_data->led_cdev = led_cdev;
+
+	return 0;
+}
+
+static void ledtrig_tty_deactivate(struct led_classdev *led_cdev)
+{
+	struct ledtrig_tty_data *trigger_data = led_get_trigger_data(led_cdev);
+
+	cancel_delayed_work_sync(&trigger_data->dwork);
+
+	kfree(trigger_data);
+}
+
+struct led_trigger ledtrig_tty = {
+	.name = "tty",
+	.activate = ledtrig_tty_activate,
+	.deactivate = ledtrig_tty_deactivate,
+	.groups = ledtrig_tty_groups,
+};
+module_led_trigger(ledtrig_tty);
+
+MODULE_AUTHOR("Uwe Kleine-König <u.kleine-koenig@pengutronix.de>");
+MODULE_DESCRIPTION("UART LED trigger");
+MODULE_LICENSE("GPL v2");
-- 
2.24.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 0/4] leds: trigger: implement a tty trigger
  2020-02-13  9:15 [PATCH v6 0/4] leds: trigger: implement a tty trigger Uwe Kleine-König
                   ` (3 preceding siblings ...)
  2020-02-13  9:16 ` [PATCH v6 4/4] leds: trigger: implement a tty trigger Uwe Kleine-König
@ 2020-02-19 10:40 ` " Greg Kroah-Hartman
  4 siblings, 0 replies; 21+ messages in thread
From: Greg Kroah-Hartman @ 2020-02-19 10:40 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Jacek Anaszewski, Pavel Machek, Dan Murphy, Jiri Slaby,
	Uwe Kleine-König, linux-leds, linux-kernel, kernel,
	linux-serial

On Thu, Feb 13, 2020 at 10:15:56AM +0100, Uwe Kleine-König wrote:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Hello,
> 
> This is v6 of my quest to introduce ledtriggers for UARTs. The previous
> series is available at
> 
> 	http://lore.kernel.org/r/20191219093947.15502-1-u.kleine-koenig@pengutronix.de
> 
> The changes compared to that are that parsing of the dev parameter is
> more strict and that I set brightness directly from the kworker instead
> of using led_blink_set_oneshot which makes use of another kworker. (Both
> requested by Pavel Machek.)
> 
> For the former I introduced a new helper kstrtodev_t() in the spirit of
> kstrtoul() to implement the stricter parsing (instead of the lax one
> using plain sscanf() in v5).

Looks good to me, Pavel, any objection to me merging this through the
tty tree?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 4/4] leds: trigger: implement a tty trigger
  2020-02-13  9:16 ` [PATCH v6 4/4] leds: trigger: implement a tty trigger Uwe Kleine-König
@ 2020-02-19 10:52   ` Johan Hovold
  2020-02-19 11:03     ` Uwe Kleine-König
  0 siblings, 1 reply; 21+ messages in thread
From: Johan Hovold @ 2020-02-19 10:52 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Jacek Anaszewski, Pavel Machek, Dan Murphy, Greg Kroah-Hartman,
	Jiri Slaby, Uwe Kleine-König, linux-leds, linux-kernel,
	kernel, linux-serial

On Thu, Feb 13, 2020 at 10:16:00AM +0100, Uwe Kleine-König wrote:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Usage is as follows:
> 
> 	myled=ledname
> 	tty=ttyS0
> 
> 	echo tty > /sys/class/leds/$myled/trigger
> 	cat /sys/class/tty/$tty/dev > /sys/class/leds/$myled/dev
> 
> . When this new trigger is active it periodically checks the tty's
> statistics and when it changed since the last check the led is flashed
> once.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---

> +static ssize_t dev_store(struct device *dev,
> +			 struct device_attribute *attr, const char *buf,
> +			 size_t size)
> +{
> +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> +	struct tty_struct *tty;
> +	dev_t d;
> +	int ret;
> +
> +	if (size == 0 || (size == 1 && buf[0] == '\n')) {
> +		tty = NULL;
> +	} else {
> +		ret = kstrtodev_t(buf, &d);
> +		if (ret < 0)
> +			return ret;
> +
> +		tty = tty_kopen_shared(d);

I really don't have time to look at this, but having the led-trigger
keep the port open looks fundamentally broken (consider modem-control
signals, power, etc).

Johan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 4/4] leds: trigger: implement a tty trigger
  2020-02-19 10:52   ` Johan Hovold
@ 2020-02-19 11:03     ` Uwe Kleine-König
  2020-02-19 11:19       ` Johan Hovold
  0 siblings, 1 reply; 21+ messages in thread
From: Uwe Kleine-König @ 2020-02-19 11:03 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Jacek Anaszewski, Pavel Machek, Dan Murphy, Greg Kroah-Hartman,
	Jiri Slaby, linux-leds, linux-kernel, kernel, linux-serial

On Wed, Feb 19, 2020 at 11:52:39AM +0100, Johan Hovold wrote:
> On Thu, Feb 13, 2020 at 10:16:00AM +0100, Uwe Kleine-König wrote:
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > Usage is as follows:
> > 
> > 	myled=ledname
> > 	tty=ttyS0
> > 
> > 	echo tty > /sys/class/leds/$myled/trigger
> > 	cat /sys/class/tty/$tty/dev > /sys/class/leds/$myled/dev
> > 
> > . When this new trigger is active it periodically checks the tty's
> > statistics and when it changed since the last check the led is flashed
> > once.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> 
> > +static ssize_t dev_store(struct device *dev,
> > +			 struct device_attribute *attr, const char *buf,
> > +			 size_t size)
> > +{
> > +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> > +	struct tty_struct *tty;
> > +	dev_t d;
> > +	int ret;
> > +
> > +	if (size == 0 || (size == 1 && buf[0] == '\n')) {
> > +		tty = NULL;
> > +	} else {
> > +		ret = kstrtodev_t(buf, &d);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		tty = tty_kopen_shared(d);
> 
> I really don't have time to look at this, but having the led-trigger
> keep the port open looks fundamentally broken (consider modem-control
> signals, power, etc).

If I understand correctly calling tty_kopen_shared() doesn't open the
device, just keep it referenced which prevents it to disappear. Unless I
miss something it doesn't result in the tty's .open() being called.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 4/4] leds: trigger: implement a tty trigger
  2020-02-19 11:03     ` Uwe Kleine-König
@ 2020-02-19 11:19       ` Johan Hovold
  2020-02-19 12:48         ` Johan Hovold
  0 siblings, 1 reply; 21+ messages in thread
From: Johan Hovold @ 2020-02-19 11:19 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Johan Hovold, Jacek Anaszewski, Pavel Machek, Dan Murphy,
	Greg Kroah-Hartman, Jiri Slaby, linux-leds, linux-kernel, kernel,
	linux-serial

On Wed, Feb 19, 2020 at 12:03:06PM +0100, Uwe Kleine-König wrote:
> On Wed, Feb 19, 2020 at 11:52:39AM +0100, Johan Hovold wrote:
> > On Thu, Feb 13, 2020 at 10:16:00AM +0100, Uwe Kleine-König wrote:
> > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > 
> > > Usage is as follows:
> > > 
> > > 	myled=ledname
> > > 	tty=ttyS0
> > > 
> > > 	echo tty > /sys/class/leds/$myled/trigger
> > > 	cat /sys/class/tty/$tty/dev > /sys/class/leds/$myled/dev
> > > 
> > > . When this new trigger is active it periodically checks the tty's
> > > statistics and when it changed since the last check the led is flashed
> > > once.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > 
> > > +static ssize_t dev_store(struct device *dev,
> > > +			 struct device_attribute *attr, const char *buf,
> > > +			 size_t size)
> > > +{
> > > +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> > > +	struct tty_struct *tty;
> > > +	dev_t d;
> > > +	int ret;
> > > +
> > > +	if (size == 0 || (size == 1 && buf[0] == '\n')) {
> > > +		tty = NULL;
> > > +	} else {
> > > +		ret = kstrtodev_t(buf, &d);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		tty = tty_kopen_shared(d);
> > 
> > I really don't have time to look at this, but having the led-trigger
> > keep the port open looks fundamentally broken (consider modem-control
> > signals, power, etc).
> 
> If I understand correctly calling tty_kopen_shared() doesn't open the
> device, just keep it referenced which prevents it to disappear. Unless I
> miss something it doesn't result in the tty's .open() being called.

So tty_kopen_shared() is something you added. Judging from a quick look
it seems you can only attach a trigger to an already open port, but the
trigger will then keep the port open (again, consider modem control,
power).

I'm sorry I don't have time to review this myself, but this probably
needs some more eyes on it before being merged.

Johan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 4/4] leds: trigger: implement a tty trigger
  2020-02-19 11:19       ` Johan Hovold
@ 2020-02-19 12:48         ` Johan Hovold
  0 siblings, 0 replies; 21+ messages in thread
From: Johan Hovold @ 2020-02-19 12:48 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Johan Hovold, Jacek Anaszewski, Pavel Machek, Dan Murphy,
	Greg Kroah-Hartman, Jiri Slaby, linux-leds, linux-kernel, kernel,
	linux-serial

On Wed, Feb 19, 2020 at 12:19:13PM +0100, Johan Hovold wrote:
> On Wed, Feb 19, 2020 at 12:03:06PM +0100, Uwe Kleine-König wrote:
> > On Wed, Feb 19, 2020 at 11:52:39AM +0100, Johan Hovold wrote:

> > If I understand correctly calling tty_kopen_shared() doesn't open the
> > device, just keep it referenced which prevents it to disappear. Unless I
> > miss something it doesn't result in the tty's .open() being called.
> 
> So tty_kopen_shared() is something you added. Judging from a quick look
> it seems you can only attach a trigger to an already open port, but the
> trigger will then keep the port open (again, consider modem control,
> power).

Sorry, my bad; this reference shouldn't prevent the port from being
closed.

The fact that you need the port to be open before you can attach a
trigger does sound like a problem though (e.g. consider udev rules).

Johan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared()
  2020-02-13  9:15 ` [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared() Uwe Kleine-König
@ 2020-02-19 13:21   ` Johan Hovold
  2020-02-19 16:37     ` Uwe Kleine-König
  0 siblings, 1 reply; 21+ messages in thread
From: Johan Hovold @ 2020-02-19 13:21 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Jacek Anaszewski, Pavel Machek, Dan Murphy, Greg Kroah-Hartman,
	Jiri Slaby, Uwe Kleine-König, linux-leds, linux-kernel,
	kernel, linux-serial

On Thu, Feb 13, 2020 at 10:15:58AM +0100, Uwe Kleine-König wrote:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Introduce a new function tty_kopen_shared() that yields a struct
> tty_struct. The semantic difference to tty_kopen() is that the tty is
> expected to be used already. So rename tty_kopen() to
> tty_kopen_exclusive() for clearness, adapt the single user and put the
> common code in a new static helper function.
> 
> tty_kopen_shared is to be used to implement an LED trigger for tty
> devices in one of the next patches.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
 
> -/**
> - *	tty_kopen	-	open a tty device for kernel
> - *	@device: dev_t of device to open
> - *
> - *	Opens tty exclusively for kernel. Performs the driver lookup,
> - *	makes sure it's not already opened and performs the first-time
> - *	tty initialization.
> - *
> - *	Returns the locked initialized &tty_struct
> - *
> - *	Claims the global tty_mutex to serialize:
> - *	  - concurrent first-time tty initialization
> - *	  - concurrent tty driver removal w/ lookup
> - *	  - concurrent tty removal from driver table
> - */
> -struct tty_struct *tty_kopen(dev_t device)
> +static struct tty_struct *tty_kopen(dev_t device, int shared)
>  {
>  	struct tty_struct *tty;
>  	struct tty_driver *driver;
> @@ -1905,7 +1890,7 @@ struct tty_struct *tty_kopen(dev_t device)
>  
>  	/* check whether we're reopening an existing tty */
>  	tty = tty_driver_lookup_tty(driver, NULL, index);
> -	if (IS_ERR(tty))
> +	if (IS_ERR(tty) || shared)

So here you skip initialisation and return NULL if the tty isn't already
in use (e.g. is open) when shared is set.

>  		goto out;
>  
>  	if (tty) {
> @@ -1923,7 +1908,44 @@ struct tty_struct *tty_kopen(dev_t device)
>  	tty_driver_kref_put(driver);
>  	return tty;
>  }
> -EXPORT_SYMBOL_GPL(tty_kopen);
> +
> +/**
> + *	tty_kopen_exclusive	-	open a tty device for kernel
> + *	@device: dev_t of device to open
> + *
> + *	Opens tty exclusively for kernel. Performs the driver lookup,
> + *	makes sure it's not already opened and performs the first-time
> + *	tty initialization.
> + *
> + *	Returns the locked initialized &tty_struct
> + *
> + *	Claims the global tty_mutex to serialize:
> + *	  - concurrent first-time tty initialization
> + *	  - concurrent tty driver removal w/ lookup
> + *	  - concurrent tty removal from driver table
> + */
> +struct tty_struct *tty_kopen_exclusive(dev_t device)
> +{
> +	return tty_kopen(device, 0);
> +}
> +EXPORT_SYMBOL_GPL(tty_kopen_exclusive);
> +
> +/**
> + *	tty_kopen_shared	-	open a tty device for shared in-kernel use
> + *	@device: dev_t of device to open
> + *
> + *	Opens an already existing tty
> + *	rnel. Performs the driver lookup,

"rnel"?

> + *	makes sure it's not already opened and performs the first-time
> + *	tty initialization.

Yet, you claim to do initialisation here, which isn't the case.

> + *
> + *	Locking is identical to tty_kopen() above.
> + */
> +struct tty_struct *tty_kopen_shared(dev_t device)
> +{
> +	return tty_kopen(device, 1);
> +}
> +EXPORT_SYMBOL_GPL(tty_kopen_shared);

This "kopen" naming is unfortunate as the tty isn't really opened by
either of these functions, but that's not something you introduced.

Johan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared()
  2020-02-19 13:21   ` Johan Hovold
@ 2020-02-19 16:37     ` Uwe Kleine-König
  2020-02-19 17:17       ` Johan Hovold
  0 siblings, 1 reply; 21+ messages in thread
From: Uwe Kleine-König @ 2020-02-19 16:37 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Jacek Anaszewski, Pavel Machek, Dan Murphy, Greg Kroah-Hartman,
	Jiri Slaby, linux-leds, linux-kernel, kernel, linux-serial

On Wed, Feb 19, 2020 at 02:21:13PM +0100, Johan Hovold wrote:
> On Thu, Feb 13, 2020 at 10:15:58AM +0100, Uwe Kleine-König wrote:
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > Introduce a new function tty_kopen_shared() that yields a struct
> > tty_struct. The semantic difference to tty_kopen() is that the tty is
> > expected to be used already. So rename tty_kopen() to
> > tty_kopen_exclusive() for clearness, adapt the single user and put the
> > common code in a new static helper function.
> > 
> > tty_kopen_shared is to be used to implement an LED trigger for tty
> > devices in one of the next patches.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
>  
> > -/**
> > - *	tty_kopen	-	open a tty device for kernel
> > - *	@device: dev_t of device to open
> > - *
> > - *	Opens tty exclusively for kernel. Performs the driver lookup,
> > - *	makes sure it's not already opened and performs the first-time
> > - *	tty initialization.
> > - *
> > - *	Returns the locked initialized &tty_struct
> > - *
> > - *	Claims the global tty_mutex to serialize:
> > - *	  - concurrent first-time tty initialization
> > - *	  - concurrent tty driver removal w/ lookup
> > - *	  - concurrent tty removal from driver table
> > - */
> > -struct tty_struct *tty_kopen(dev_t device)
> > +static struct tty_struct *tty_kopen(dev_t device, int shared)
> >  {
> >  	struct tty_struct *tty;
> >  	struct tty_driver *driver;
> > @@ -1905,7 +1890,7 @@ struct tty_struct *tty_kopen(dev_t device)
> >  
> >  	/* check whether we're reopening an existing tty */
> >  	tty = tty_driver_lookup_tty(driver, NULL, index);
> > -	if (IS_ERR(tty))
> > +	if (IS_ERR(tty) || shared)
> 
> So here you skip initialisation and return NULL if the tty isn't already
> in use (e.g. is open) when shared is set.

Which is good, right? If I remember my tests correctly this even works
if the tty isn't opened but just "exists".

> >  		goto out;
> >  
> >  	if (tty) {
> > @@ -1923,7 +1908,44 @@ struct tty_struct *tty_kopen(dev_t device)
> >  	tty_driver_kref_put(driver);
> >  	return tty;
> >  }
> > -EXPORT_SYMBOL_GPL(tty_kopen);
> > +
> > +/**
> > + *	tty_kopen_exclusive	-	open a tty device for kernel
> > + *	@device: dev_t of device to open
> > + *
> > + *	Opens tty exclusively for kernel. Performs the driver lookup,
> > + *	makes sure it's not already opened and performs the first-time
> > + *	tty initialization.
> > + *
> > + *	Returns the locked initialized &tty_struct
> > + *
> > + *	Claims the global tty_mutex to serialize:
> > + *	  - concurrent first-time tty initialization
> > + *	  - concurrent tty driver removal w/ lookup
> > + *	  - concurrent tty removal from driver table
> > + */
> > +struct tty_struct *tty_kopen_exclusive(dev_t device)
> > +{
> > +	return tty_kopen(device, 0);
> > +}
> > +EXPORT_SYMBOL_GPL(tty_kopen_exclusive);
> > +
> > +/**
> > + *	tty_kopen_shared	-	open a tty device for shared in-kernel use
> > + *	@device: dev_t of device to open
> > + *
> > + *	Opens an already existing tty
> > + *	rnel. Performs the driver lookup,
> 
> "rnel"?
> 
> > + *	makes sure it's not already opened and performs the first-time
> > + *	tty initialization.
> 
> Yet, you claim to do initialisation here, which isn't the case.

Yeah, wrong (and incomplete) copy of the tty_kopen_exclusive docstring.

> > + *
> > + *	Locking is identical to tty_kopen() above.
> > + */
> > +struct tty_struct *tty_kopen_shared(dev_t device)
> > +{
> > +	return tty_kopen(device, 1);
> > +}
> > +EXPORT_SYMBOL_GPL(tty_kopen_shared);
> 
> This "kopen" naming is unfortunate as the tty isn't really opened by
> either of these functions, but that's not something you introduced.

Ack, will send a v7 with the doc string fixed.

Thanks for taking the time to look over 
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared()
  2020-02-19 16:37     ` Uwe Kleine-König
@ 2020-02-19 17:17       ` Johan Hovold
  2020-02-20 11:04         ` Uwe Kleine-König
  0 siblings, 1 reply; 21+ messages in thread
From: Johan Hovold @ 2020-02-19 17:17 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Johan Hovold, Jacek Anaszewski, Pavel Machek, Dan Murphy,
	Greg Kroah-Hartman, Jiri Slaby, linux-leds, linux-kernel, kernel,
	linux-serial

On Wed, Feb 19, 2020 at 05:37:58PM +0100, Uwe Kleine-König wrote:
> On Wed, Feb 19, 2020 at 02:21:13PM +0100, Johan Hovold wrote:
> > On Thu, Feb 13, 2020 at 10:15:58AM +0100, Uwe Kleine-König wrote:
> > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > 
> > > Introduce a new function tty_kopen_shared() that yields a struct
> > > tty_struct. The semantic difference to tty_kopen() is that the tty is
> > > expected to be used already. So rename tty_kopen() to
> > > tty_kopen_exclusive() for clearness, adapt the single user and put the
> > > common code in a new static helper function.
> > > 
> > > tty_kopen_shared is to be used to implement an LED trigger for tty
> > > devices in one of the next patches.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> >  
> > > -/**
> > > - *	tty_kopen	-	open a tty device for kernel
> > > - *	@device: dev_t of device to open
> > > - *
> > > - *	Opens tty exclusively for kernel. Performs the driver lookup,
> > > - *	makes sure it's not already opened and performs the first-time
> > > - *	tty initialization.
> > > - *
> > > - *	Returns the locked initialized &tty_struct
> > > - *
> > > - *	Claims the global tty_mutex to serialize:
> > > - *	  - concurrent first-time tty initialization
> > > - *	  - concurrent tty driver removal w/ lookup
> > > - *	  - concurrent tty removal from driver table
> > > - */
> > > -struct tty_struct *tty_kopen(dev_t device)
> > > +static struct tty_struct *tty_kopen(dev_t device, int shared)
> > >  {
> > >  	struct tty_struct *tty;
> > >  	struct tty_driver *driver;
> > > @@ -1905,7 +1890,7 @@ struct tty_struct *tty_kopen(dev_t device)
> > >  
> > >  	/* check whether we're reopening an existing tty */
> > >  	tty = tty_driver_lookup_tty(driver, NULL, index);
> > > -	if (IS_ERR(tty))
> > > +	if (IS_ERR(tty) || shared)
> > 
> > So here you skip initialisation and return NULL if the tty isn't already
> > in use (e.g. is open) when shared is set.
> 
> Which is good, right? If I remember my tests correctly this even works
> if the tty isn't opened but just "exists".

No, this means that your trigger will never be installed unless the port
is already open, yet the sysfs interface still returns success (see
patch 4/4 dev_store()).

Note that the struct tty doesn't exist until the port is opened; it's
allocated in tty_init_dev() that you skip above when "shared" is set.

Johan

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()
  2020-02-13  9:15 ` [PATCH v6 1/4] lib: new helper kstrtodev_t() Uwe Kleine-König
@ 2020-02-19 19:50   ` Andy Shevchenko
  2020-02-20  7:49     ` Uwe Kleine-König
  0 siblings, 1 reply; 21+ messages in thread
From: Andy Shevchenko @ 2020-02-19 19:50 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Jacek Anaszewski, Pavel Machek, Dan Murphy, Greg Kroah-Hartman,
	Jiri Slaby, Linux LED Subsystem, Linux Kernel Mailing List,
	Sascha Hauer, open list:SERIAL DRIVERS

On Thu, Feb 13, 2020 at 11:27 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
>
> This function is in the same spirit as the other kstrto* functions and
> uses the same calling convention. It expects the input string to be in
> the format %u:%u and implements stricter parsing than sscanf as it
> returns an error on trailing data (other than the usual \n).

Can we first split the kstrotox* (and simple_strto*) to the separate
header first?

On top of that, why kstrtodev_t is so important? How many users are
already in the kernel to get an advantage out of it?
What to do with all other possible variants ("%d:%d", "%dx%d" and its
%u variant, etc)?

Why simple_strto*() can't be used?

>  #include <linux/export.h>
>  #include <linux/types.h>
>  #include <linux/uaccess.h>

> +#include <linux/kdev_t.h>

Perhaps preserve order? (It's for the future, since I doubt we will
get this in upstream anyway).

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()
  2020-02-19 19:50   ` Andy Shevchenko
@ 2020-02-20  7:49     ` Uwe Kleine-König
  2020-02-20 10:22       ` Andy Shevchenko
  0 siblings, 1 reply; 21+ messages in thread
From: Uwe Kleine-König @ 2020-02-20  7:49 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jacek Anaszewski, Pavel Machek, Dan Murphy, Greg Kroah-Hartman,
	Jiri Slaby, Linux LED Subsystem, Linux Kernel Mailing List,
	Sascha Hauer, open list:SERIAL DRIVERS

On Wed, Feb 19, 2020 at 09:50:54PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 13, 2020 at 11:27 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> >
> > This function is in the same spirit as the other kstrto* functions and
> > uses the same calling convention. It expects the input string to be in
> > the format %u:%u and implements stricter parsing than sscanf as it
> > returns an error on trailing data (other than the usual \n).
> 
> Can we first split the kstrotox* (and simple_strto*) to the separate
> header first?

I don't feel strong here what is right. But I hesitate to create another
pre-condition for this patch set.

> On top of that, why kstrtodev_t is so important? How many users are
> already in the kernel to get an advantage out of it?

Does it need to be important? It matches the other kstrto* functions and
so it seemed more natural to me to put it near the other functions. I'm
not aware of other potential users and surprised you seem to suggest
this as a requirement.

> What to do with all other possible variants ("%d:%d", "%dx%d" and its
> %u variant, etc)?

I don't see how %d:%d is relevant, major and minor cannot be negative
can they? I never saw 'x' as separator between major and minor. I
considered shortly parsing %u, but given that (I think) this is an
internal representation only I chose to not make it more visible than it
already is.

> Why simple_strto*() can't be used?

I didn't really consider it, but looking in more detail I don't like it
much. Without having tried it I think simple_strtoull accepts
"1000000000000000000000000000000000000000000" returning some arbitrary
value without an error indication. And given that I was asked for strict
parsing (i.e. not accepting 2:4:something) I'd say using simple_strto*
is a step backwards. Also simple_strtoul() has "This function is obsolete.
Please use kstrtoul instead." in its docstring which seems to apply to
the other simple_strto*() functions, too.

> >  #include <linux/export.h>
> >  #include <linux/types.h>
> >  #include <linux/uaccess.h>
> 
> > +#include <linux/kdev_t.h>
> 
> Perhaps preserve order?

Can do.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()
  2020-02-20  7:49     ` Uwe Kleine-König
@ 2020-02-20 10:22       ` Andy Shevchenko
  2020-02-20 10:57         ` Uwe Kleine-König
  0 siblings, 1 reply; 21+ messages in thread
From: Andy Shevchenko @ 2020-02-20 10:22 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Jacek Anaszewski, Pavel Machek, Dan Murphy, Greg Kroah-Hartman,
	Jiri Slaby, Linux LED Subsystem, Linux Kernel Mailing List,
	Sascha Hauer, open list:SERIAL DRIVERS

On Thu, Feb 20, 2020 at 9:49 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Wed, Feb 19, 2020 at 09:50:54PM +0200, Andy Shevchenko wrote:
> > On Thu, Feb 13, 2020 at 11:27 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > >
> > > This function is in the same spirit as the other kstrto* functions and
> > > uses the same calling convention. It expects the input string to be in
> > > the format %u:%u and implements stricter parsing than sscanf as it
> > > returns an error on trailing data (other than the usual \n).

...

> > On top of that, why kstrtodev_t is so important? How many users are
> > already in the kernel to get an advantage out of it?
>
> Does it need to be important? It matches the other kstrto* functions and
> so it seemed more natural to me to put it near the other functions. I'm
> not aware of other potential users and surprised you seem to suggest
> this as a requirement.

Yes it does. The kstrtox() are quite generic, what you are proposing
is rather one particular case with blurry understanding how many users
will be out of it.
If you had told "look, we have 1234 users which may benefit out of
it", I would have given no comment against.

> > What to do with all other possible variants ("%d:%d", "%dx%d" and its
> > %u variant, etc)?
>
> I don't see how %d:%d is relevant, major and minor cannot be negative
> can they? I never saw 'x' as separator between major and minor. I
> considered shortly parsing %u, but given that (I think) this is an
> internal representation only I chose to not make it more visible than it
> already is.

See above, if we are going to make it generic, perhaps better to cover
more possible users, right?
Otherwise your change provokes pile of (replaced)
kstrto_resolution() /* %ux:%u */
kstrto_range() /* %d:%d */
kstrto_you_name_it()

> > Why simple_strto*() can't be used?
>
> I didn't really consider it, but looking in more detail I don't like it
> much. Without having tried it I think simple_strtoull accepts
> "1000000000000000000000000000000000000000000" returning some arbitrary
> value without an error indication.

So what? User has a lot of possibilities to shoot into the foot.
Since you interpret this as device major:minor, not founding a device
will be first level of error, next one when your code will try to do
something out of it. It shouldn't be a problem of kstrtox generic
helpers.

> And given that I was asked for strict
> parsing (i.e. not accepting 2:4:something) I'd say using simple_strto*
> is a step backwards. Also simple_strtoul() has "This function is obsolete.
> Please use kstrtoul instead." in its docstring which seems to apply to
> the other simple_strto*() functions, too.

I specifically fixed a doc string to approve its use in the precisely
cases you have here.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()
  2020-02-20 10:22       ` Andy Shevchenko
@ 2020-02-20 10:57         ` Uwe Kleine-König
  2020-02-20 11:46           ` Andy Shevchenko
  0 siblings, 1 reply; 21+ messages in thread
From: Uwe Kleine-König @ 2020-02-20 10:57 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Sascha Hauer, open list:SERIAL DRIVERS, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Jacek Anaszewski, Pavel Machek,
	Jiri Slaby, Linux LED Subsystem, Dan Murphy

Hello Andy,

On Thu, Feb 20, 2020 at 12:22:36PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 20, 2020 at 9:49 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Wed, Feb 19, 2020 at 09:50:54PM +0200, Andy Shevchenko wrote:
> > > On Thu, Feb 13, 2020 at 11:27 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > >
> > > > This function is in the same spirit as the other kstrto* functions and
> > > > uses the same calling convention. It expects the input string to be in
> > > > the format %u:%u and implements stricter parsing than sscanf as it
> > > > returns an error on trailing data (other than the usual \n).
> 
> ...
> 
> > > On top of that, why kstrtodev_t is so important? How many users are
> > > already in the kernel to get an advantage out of it?
> >
> > Does it need to be important? It matches the other kstrto* functions and
> > so it seemed more natural to me to put it near the other functions. I'm
> > not aware of other potential users and surprised you seem to suggest
> > this as a requirement.
> 
> Yes it does. The kstrtox() are quite generic, what you are proposing
> is rather one particular case with blurry understanding how many users
> will be out of it.

In my understanding one user is a hard requirement.

> If you had told "look, we have 1234 users which may benefit out of
> it", I would have given no comment against.

Sure, having >1000 potential users would be a good argument pro this
function. But having only one isn't a good contra IMHO.

> > > What to do with all other possible variants ("%d:%d", "%dx%d" and its
> > > %u variant, etc)?
> >
> > I don't see how %d:%d is relevant, major and minor cannot be negative
> > can they? I never saw 'x' as separator between major and minor. I
> > considered shortly parsing %u, but given that (I think) this is an
> > internal representation only I chose to not make it more visible than it
> > already is.
> 
> See above, if we are going to make it generic, perhaps better to cover
> more possible users, right?
> Otherwise your change provokes pile of (replaced)
> kstrto_resolution() /* %ux:%u */
> kstrto_range() /* %d:%d */
> kstrto_you_name_it()

Given there are respective types that this can be stored to, I don't
object more functions of this type and don't see a good reason to not
add such a function. And in my eyes I prefer to have such a function in
a visible place (i.e. where all the other kstrto* functions are) to
prevent code duplication.

Also I don't understand yet, what you want me to do. Assume I'd be
willing to use simple_strtoul, I'd still want to have a function that
gives me a dev_t from a given string. Should I put this directly in my
led-trigger driver?

> > > Why simple_strto*() can't be used?
> >
> > I didn't really consider it, but looking in more detail I don't like it
> > much. Without having tried it I think simple_strtoull accepts
> > "1000000000000000000000000000000000000000000" returning some arbitrary
> > value without an error indication.
> 
> So what? User has a lot of possibilities to shoot into the foot.
> Since you interpret this as device major:minor, not founding a device
> will be first level of error, next one when your code will try to do
> something out of it. It shouldn't be a problem of kstrtox generic
> helpers.

I fail to follow your argument here. In my eyes if the user writes a
valid major:minor it should work, and if they write an invalid one this
should result in an error and not a usage of a device that just happens
to have the major:minor that simple_strtoull happens to return for the
two components.

> > And given that I was asked for strict
> > parsing (i.e. not accepting 2:4:something) I'd say using simple_strto*
> > is a step backwards. Also simple_strtoul() has "This function is obsolete.
> > Please use kstrtoul instead." in its docstring which seems to apply to
> > the other simple_strto*() functions, too.
> 
> I specifically fixed a doc string to approve its use in the precisely
> cases you have here.

Can you please be a bit more constructive here and point to the change
you talk about? I didn't find a commit in next.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared()
  2020-02-19 17:17       ` Johan Hovold
@ 2020-02-20 11:04         ` Uwe Kleine-König
  0 siblings, 0 replies; 21+ messages in thread
From: Uwe Kleine-König @ 2020-02-20 11:04 UTC (permalink / raw)
  To: Johan Hovold
  Cc: kernel, linux-serial, Greg Kroah-Hartman, linux-kernel,
	Dan Murphy, Pavel Machek, Jiri Slaby, linux-leds,
	Jacek Anaszewski

On Wed, Feb 19, 2020 at 06:17:59PM +0100, Johan Hovold wrote:
> On Wed, Feb 19, 2020 at 05:37:58PM +0100, Uwe Kleine-König wrote:
> > On Wed, Feb 19, 2020 at 02:21:13PM +0100, Johan Hovold wrote:
> > > On Thu, Feb 13, 2020 at 10:15:58AM +0100, Uwe Kleine-König wrote:
> > > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > 
> > > > Introduce a new function tty_kopen_shared() that yields a struct
> > > > tty_struct. The semantic difference to tty_kopen() is that the tty is
> > > > expected to be used already. So rename tty_kopen() to
> > > > tty_kopen_exclusive() for clearness, adapt the single user and put the
> > > > common code in a new static helper function.
> > > > 
> > > > tty_kopen_shared is to be used to implement an LED trigger for tty
> > > > devices in one of the next patches.
> > > > 
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > ---
> > >  
> > > > -/**
> > > > - *	tty_kopen	-	open a tty device for kernel
> > > > - *	@device: dev_t of device to open
> > > > - *
> > > > - *	Opens tty exclusively for kernel. Performs the driver lookup,
> > > > - *	makes sure it's not already opened and performs the first-time
> > > > - *	tty initialization.
> > > > - *
> > > > - *	Returns the locked initialized &tty_struct
> > > > - *
> > > > - *	Claims the global tty_mutex to serialize:
> > > > - *	  - concurrent first-time tty initialization
> > > > - *	  - concurrent tty driver removal w/ lookup
> > > > - *	  - concurrent tty removal from driver table
> > > > - */
> > > > -struct tty_struct *tty_kopen(dev_t device)
> > > > +static struct tty_struct *tty_kopen(dev_t device, int shared)
> > > >  {
> > > >  	struct tty_struct *tty;
> > > >  	struct tty_driver *driver;
> > > > @@ -1905,7 +1890,7 @@ struct tty_struct *tty_kopen(dev_t device)
> > > >  
> > > >  	/* check whether we're reopening an existing tty */
> > > >  	tty = tty_driver_lookup_tty(driver, NULL, index);
> > > > -	if (IS_ERR(tty))
> > > > +	if (IS_ERR(tty) || shared)
> > > 
> > > So here you skip initialisation and return NULL if the tty isn't already
> > > in use (e.g. is open) when shared is set.
> > 
> > Which is good, right? If I remember my tests correctly this even works
> > if the tty isn't opened but just "exists".
> 
> No, this means that your trigger will never be installed unless the port
> is already open, yet the sysfs interface still returns success (see
> patch 4/4 dev_store()).

Ah I think I see. tty_driver_lookup_tty() might return NULL which for
the trigger driver indicates that tty_kopen_shared should be retried,
right?

> Note that the struct tty doesn't exist until the port is opened; it's
> allocated in tty_init_dev() that you skip above when "shared" is set.

That needs fixing. I will try to resolve the dialog with Andy before
addressing that in the next iteration.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()
  2020-02-20 10:57         ` Uwe Kleine-König
@ 2020-02-20 11:46           ` Andy Shevchenko
  2020-02-20 11:57             ` Andy Shevchenko
  2020-02-20 14:01             ` Uwe Kleine-König
  0 siblings, 2 replies; 21+ messages in thread
From: Andy Shevchenko @ 2020-02-20 11:46 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Sascha Hauer, open list:SERIAL DRIVERS, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Jacek Anaszewski, Pavel Machek,
	Jiri Slaby, Linux LED Subsystem, Dan Murphy

On Thu, Feb 20, 2020 at 12:57 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Thu, Feb 20, 2020 at 12:22:36PM +0200, Andy Shevchenko wrote:
> > On Thu, Feb 20, 2020 at 9:49 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Wed, Feb 19, 2020 at 09:50:54PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Feb 13, 2020 at 11:27 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > > >
> > > > > This function is in the same spirit as the other kstrto* functions and
> > > > > uses the same calling convention. It expects the input string to be in
> > > > > the format %u:%u and implements stricter parsing than sscanf as it
> > > > > returns an error on trailing data (other than the usual \n).
> >
> > ...
> >
> > > > On top of that, why kstrtodev_t is so important? How many users are
> > > > already in the kernel to get an advantage out of it?
> > >
> > > Does it need to be important? It matches the other kstrto* functions and
> > > so it seemed more natural to me to put it near the other functions. I'm
> > > not aware of other potential users and surprised you seem to suggest
> > > this as a requirement.
> >
> > Yes it does. The kstrtox() are quite generic, what you are proposing
> > is rather one particular case with blurry understanding how many users
> > will be out of it.
>
> In my understanding one user is a hard requirement.

Yes. But looking at the LOCs you introduce to entire kernel in such
generic area (I wouldn't tell you anything if, for instance, you
introduced a support for hypothetical S2P bus with one host controller
driver) like lib/.

> > If you had told "look, we have 1234 users which may benefit out of
> > it", I would have given no comment against.
>
> Sure, having >1000 potential users would be a good argument pro this
> function. But having only one isn't a good contra IMHO.

For lib/ is a good argument in my opinion.

> > > > What to do with all other possible variants ("%d:%d", "%dx%d" and its
> > > > %u variant, etc)?
> > >
> > > I don't see how %d:%d is relevant, major and minor cannot be negative
> > > can they? I never saw 'x' as separator between major and minor. I
> > > considered shortly parsing %u, but given that (I think) this is an
> > > internal representation only I chose to not make it more visible than it
> > > already is.
> >
> > See above, if we are going to make it generic, perhaps better to cover
> > more possible users, right?
> > Otherwise your change provokes pile of (replaced)
> > kstrto_resolution() /* %ux:%u */
> > kstrto_range() /* %d:%d */
> > kstrto_you_name_it()
>
> Given there are respective types that this can be stored to, I don't
> object more functions of this type and don't see a good reason to not
> add such a function. And in my eyes I prefer to have such a function in
> a visible place (i.e. where all the other kstrto* functions are) to
> prevent code duplication.

You can easily satisfy above by adding a function parameter 'char
*delim', right?

> Also I don't understand yet, what you want me to do.

I have issues with kstrto() not playing with simple numbers (boolean
is a special case, but still a number at the end).
I also don't feel good with too narrow usage of the newly introduced helper

> Assume I'd be
> willing to use simple_strtoul, I'd still want to have a function that
> gives me a dev_t from a given string. Should I put this directly in my
> led-trigger driver?

I see the following possibilities:
a) put it inside the caller and forget about generic helper
b) do a generic helper, but 1/ in string_*() namespace, 2/ with a
delimiter parameter and 3/ possibility to take negative numbers

In b) case, add to the commit message how many potential _existing_
users may be converted to this.
Also it would be good to have two versions strict (only \n at the end
is allowed) and non-strict (based on the amount of users for each
group).

> > > And given that I was asked for strict
> > > parsing (i.e. not accepting 2:4:something) I'd say using simple_strto*
> > > is a step backwards. Also simple_strtoul() has "This function is obsolete.
> > > Please use kstrtoul instead." in its docstring which seems to apply to
> > > the other simple_strto*() functions, too.
> >
> > I specifically fixed a doc string to approve its use in the precisely
> > cases you have here.
>
> Can you please be a bit more constructive here and point to the change
> you talk about? I didn't find a commit in next.

https://elixir.bootlin.com/linux/v5.6-rc2/source/include/linux/kernel.h#L446

Note, there is no more word 'obsolete' there.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()
  2020-02-20 11:46           ` Andy Shevchenko
@ 2020-02-20 11:57             ` Andy Shevchenko
  2020-02-20 14:01             ` Uwe Kleine-König
  1 sibling, 0 replies; 21+ messages in thread
From: Andy Shevchenko @ 2020-02-20 11:57 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Sascha Hauer, open list:SERIAL DRIVERS, Greg Kroah-Hartman,
	Linux Kernel Mailing List, Jacek Anaszewski, Pavel Machek,
	Jiri Slaby, Linux LED Subsystem, Dan Murphy

On Thu, Feb 20, 2020 at 1:46 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Feb 20, 2020 at 12:57 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Thu, Feb 20, 2020 at 12:22:36PM +0200, Andy Shevchenko wrote:

...

> > Also I don't understand yet, what you want me to do.
>
> I have issues with kstrto() not playing with simple numbers (boolean

s/simple/simple and single/

> is a special case, but still a number at the end).
> I also don't feel good with too narrow usage of the newly introduced helper
>
> > Assume I'd be
> > willing to use simple_strtoul, I'd still want to have a function that
> > gives me a dev_t from a given string. Should I put this directly in my
> > led-trigger driver?
>
> I see the following possibilities:

(above doesn't imply the necessity of simple_strto*() use)

> a) put it inside the caller and forget about generic helper
> b) do a generic helper, but 1/ in string_*() namespace, 2/ with a
> delimiter parameter and 3/ possibility to take negative numbers
>
> In b) case, add to the commit message how many potential _existing_
> users may be converted to this.
> Also it would be good to have two versions strict (only \n at the end
> is allowed) and non-strict (based on the amount of users for each
> group).

And don't forget to extend lib/test_string.c accordingly.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v6 1/4] lib: new helper kstrtodev_t()
  2020-02-20 11:46           ` Andy Shevchenko
  2020-02-20 11:57             ` Andy Shevchenko
@ 2020-02-20 14:01             ` Uwe Kleine-König
  1 sibling, 0 replies; 21+ messages in thread
From: Uwe Kleine-König @ 2020-02-20 14:01 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Pengutronix Kernel Team, open list:SERIAL DRIVERS,
	Greg Kroah-Hartman, Linux Kernel Mailing List, Jacek Anaszewski,
	Pavel Machek, Jiri Slaby, Linux LED Subsystem, Dan Murphy

Hello,

On Thu, Feb 20, 2020 at 01:46:00PM +0200, Andy Shevchenko wrote (with
the additions written in a later mail inserted as if he said them
already then):
> On Thu, Feb 20, 2020 at 12:57 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Thu, Feb 20, 2020 at 12:22:36PM +0200, Andy Shevchenko wrote:
> > > On Thu, Feb 20, 2020 at 9:49 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Wed, Feb 19, 2020 at 09:50:54PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Feb 13, 2020 at 11:27 AM Uwe Kleine-König <uwe@kleine-koenig.org> wrote:
> > > > > >
> > > > > > This function is in the same spirit as the other kstrto* functions and
> > > > > > uses the same calling convention. It expects the input string to be in
> > > > > > the format %u:%u and implements stricter parsing than sscanf as it
> > > > > > returns an error on trailing data (other than the usual \n).
> > >
> > > ...
> > >
> > > > > On top of that, why kstrtodev_t is so important? How many users are
> > > > > already in the kernel to get an advantage out of it?
> > > >
> > > > Does it need to be important? It matches the other kstrto* functions and
> > > > so it seemed more natural to me to put it near the other functions. I'm
> > > > not aware of other potential users and surprised you seem to suggest
> > > > this as a requirement.
> > >
> > > Yes it does. The kstrtox() are quite generic, what you are proposing
> > > is rather one particular case with blurry understanding how many users
> > > will be out of it.
> >
> > In my understanding one user is a hard requirement.
> 
> Yes. But looking at the LOCs you introduce to entire kernel in such
> generic area (I wouldn't tell you anything if, for instance, you
> introduced a support for hypothetical S2P bus with one host controller
> driver) like lib/.

Why are added LOCs bad? Increased compile time? Increased binary size?
More memory usage when the file is opened in an editor?

> > > If you had told "look, we have 1234 users which may benefit out of
> > > it", I would have given no comment against.
> >
> > Sure, having >1000 potential users would be a good argument pro this
> > function. But having only one isn't a good contra IMHO.
> 
> For lib/ is a good argument in my opinion.

So while I agree (as I wrote) that many users would be great, not having
many users isn't bad enough to justify putting this function somewhere
separated from the other kstrto*() functions. I assume we won't be able
to sort out this difference of opinion.

> > > > > What to do with all other possible variants ("%d:%d", "%dx%d" and its
> > > > > %u variant, etc)?
> > > >
> > > > I don't see how %d:%d is relevant, major and minor cannot be negative
> > > > can they? I never saw 'x' as separator between major and minor. I
> > > > considered shortly parsing %u, but given that (I think) this is an
> > > > internal representation only I chose to not make it more visible than it
> > > > already is.
> > >
> > > See above, if we are going to make it generic, perhaps better to cover
> > > more possible users, right?
> > > Otherwise your change provokes pile of (replaced)
> > > kstrto_resolution() /* %ux:%u */
> > > kstrto_range() /* %d:%d */
> > > kstrto_you_name_it()
> >
> > Given there are respective types that this can be stored to, I don't
> > object more functions of this type and don't see a good reason to not
> > add such a function. And in my eyes I prefer to have such a function in
> > a visible place (i.e. where all the other kstrto* functions are) to
> > prevent code duplication.
> 
> You can easily satisfy above by adding a function parameter 'char
> *delim', right?

Well, not completely as major and minor have a different domain range
compared to unsigned ints (or any other integer type).
 
> > Also I don't understand yet, what you want me to do.
> 
> I have issues with kstrto() not playing with simple and single numbers
> (boolean is a special case, but still a number at the end).

A dev_t is also a number in the end.

> I also don't feel good with too narrow usage of the newly introduced helper

I don't see how a helper that should provide a valid dev_t and other
types use a generic function. The specification for parsing a dev_t is:
"A non-negative number that fits in 20 bits, followed by a colon,
followed by a non-negative number that fits in 12 bits." So even if we
had a generic function that could parse (in scanf-semantics) "%u:%u",
we'd still need a more specialized helper that ensures the range of the
two integers. (And I suggest to call this helper kstrtodev_t. :-)

> > Assume I'd be
> > willing to use simple_strtoul, I'd still want to have a function that
> > gives me a dev_t from a given string. Should I put this directly in my
> > led-trigger driver?
> 
> I see the following possibilities:
> a) put it inside the caller and forget about generic helper
> b) do a generic helper, but 1/ in string_*() namespace, 2/ with a
> delimiter parameter and 3/ possibility to take negative numbers

I wonder about 1/. Are there already other (and similar) functions in
the string_* namespace? IMHO kstrto* is fine. These all take a string
and provide a value of a given type with strict parsing. As pointed out
above, 2/ isn't enough. I don't care much about 3/.

> In b) case, add to the commit message how many potential _existing_
> users may be converted to this.

<sarcasm>Will use 9f6158946987a5ce3f16da097d18f240a89db417 as a good
example how to do that.</sarcasm>

> Also it would be good to have two versions strict (only \n at the end
> is allowed) and non-strict (based on the amount of users for each
> group). And don't forget to extend lib/test_string.c accordingly.
> 
> > > > And given that I was asked for strict
> > > > parsing (i.e. not accepting 2:4:something) I'd say using simple_strto*
> > > > is a step backwards. Also simple_strtoul() has "This function is obsolete.
> > > > Please use kstrtoul instead." in its docstring which seems to apply to
> > > > the other simple_strto*() functions, too.
> > >
> > > I specifically fixed a doc string to approve its use in the precisely
> > > cases you have here.
> >
> > Can you please be a bit more constructive here and point to the change
> > you talk about? I didn't find a commit in next.
> 
> https://elixir.bootlin.com/linux/v5.6-rc2/source/include/linux/kernel.h#L446
> 
> Note, there is no more word 'obsolete' there.

I talked about

https://elixir.bootlin.com/linux/v5.6-rc2/source/lib/vsprintf.c#L61

which still tells to not use it.

I think what is needed here to satisfy us both is a set of functions like:

	int buftoul(const char *buf, char **endp, unsigned long *result)

which does proper range checking (by not consuming chars that are too
much) and still provides an endp pointer that allows to make use of this
function to parse types that are represented by more than a plain
integer. Currently this functionality is provided by _parse_integer
(with a different API and slightly different semantic). For my purpose
_parse_integer is good enough, so I'd like to leave introduction and
identification plus conversion of already existing potential users to
you.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, back to index

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-13  9:15 [PATCH v6 0/4] leds: trigger: implement a tty trigger Uwe Kleine-König
2020-02-13  9:15 ` [PATCH v6 1/4] lib: new helper kstrtodev_t() Uwe Kleine-König
2020-02-19 19:50   ` Andy Shevchenko
2020-02-20  7:49     ` Uwe Kleine-König
2020-02-20 10:22       ` Andy Shevchenko
2020-02-20 10:57         ` Uwe Kleine-König
2020-02-20 11:46           ` Andy Shevchenko
2020-02-20 11:57             ` Andy Shevchenko
2020-02-20 14:01             ` Uwe Kleine-König
2020-02-13  9:15 ` [PATCH v6 2/4] tty: rename tty_kopen() and add new function tty_kopen_shared() Uwe Kleine-König
2020-02-19 13:21   ` Johan Hovold
2020-02-19 16:37     ` Uwe Kleine-König
2020-02-19 17:17       ` Johan Hovold
2020-02-20 11:04         ` Uwe Kleine-König
2020-02-13  9:15 ` [PATCH v6 3/4] tty: new helper function tty_get_icount() Uwe Kleine-König
2020-02-13  9:16 ` [PATCH v6 4/4] leds: trigger: implement a tty trigger Uwe Kleine-König
2020-02-19 10:52   ` Johan Hovold
2020-02-19 11:03     ` Uwe Kleine-König
2020-02-19 11:19       ` Johan Hovold
2020-02-19 12:48         ` Johan Hovold
2020-02-19 10:40 ` [PATCH v6 0/4] " Greg Kroah-Hartman

Linux-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org
	public-inbox-index linux-leds

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-leds


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git