ledtrig-tty: add additional tty state evaluation

Changes in v9:
==============
- As requested, I have removed the memory leak fix patch from the previous patch series v8 and have send this patch directly to the stable mailinglist [10].
- I also have removed the not needed wait for completion code path on sysfs attr store and show, for the new configuration options, as requested by greg k-h [11].
- Patch v8 3/6 dropped, because the change is not correct as mentiond by greg k-h [12] and this part of the code will be removed anyway in this patch series.
- Update naming of the function ledtrig_tty_waitforcompletion() to ledtrig_tty_wait_for_completion() which match better with the API call function wait_for_completion_timeout().

Thanks to all reviewers for the suggestions for improvement :+1: of this patch set.

Links:
[10] https://lore.kernel.org/stable/2023112718-profane-dipped-a9a8@gregkh/T/#m6b36fde65a0ddacbe1fc94a14778a304775b2e79
[11] https://lore.kernel.org/linux-leds/2023112334-unquote-robust-15b8@gregkh/
[12] https://lore.kernel.org/linux-leds/2023112300-registrar-preface-2225@gregkh/

Changes in v8:
==============
v8: https://lore.kernel.org/linux-leds/[email protected]/
- As requested by greg k-h [6], I have send the patch 2/7 of this series about the memory leak also to stable.vger.kernel.org [7]. This has already received a 'Reviewed-by' from Uwe [8].
- As requested by Maarten, I have adopted his suggestion to invert the LED blink, so that I do not have to save the 'state' in the tty data struct [9].

Links:
[6] https://lore.kernel.org/linux-leds/[email protected]/T/#me43be56f4063082e7b47858773ea8067a3846466
[7] https://lore.kernel.org/stable/[email protected]/T/#t
[8] https://lore.kernel.org/stable/[email protected]/T/#m78a946889e4722903b2a79e3a465d8da0ca16333
[9] https://lore.kernel.org/linux-leds/[email protected]/T/#m43df92b665f613fe0af7d5d003a3804404f1c494

Changes in v7:
==============
v7: https://lore.kernel.org/linux-leds/[email protected]/
- Patch 1/7 is no longer included from the previous patch set v6, as it has already been merged into the master branch [5].
- As requested by Maarten, I have added a 'Fixes:' tag to patch 2/6 of this patch set, so that this commit should also be backported to the stable branches, as it is a memory leak.
- As requested by Maarten, I added an invert flag on LED blink, so that the LED blinks in the correct order.
* LED was 'on' in the previous round, then it should first go 'off' and then 'on' again when it should blink (data has been transferred).
* LED was 'off' in the previous round, then it should first go 'on' and then 'off' again when it should blink (data has been transferred).

Links:
[5] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/tty/tty_io.c?h=next-20231106&id=838eb763c3e939a8de8d4c55a17ddcce737685c1

Changes in v6:
==============
v6: https://lore.kernel.org/linux-leds/[email protected]/
This is a paritial rewrite of the changes to make the function for setting the tty evaluation configurable. This change was requested and comment by Greg K-H at the last review [1]. The main changes are.
- Split the changes into smaller commits to make reviewing easier.
- Use a completion to sync the sysfs and the delay scheduler work on shared variables.
- Adding the base-commit to this overview, that reviewer know which base commit I am using.
Base branch is:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
Patch [1/7]:
This patch is already included in the tty branch [2], but it is still included in this patchsset, so that the following patches of the base-commit branch could be applied correctly.
Patch [2/7]:
Add a new helper function tty_get_tiocm(). This got already a 'Acked-by: Greg Kroah-Hartman' [3] and is not changed.
Patch [3/7]:
Add missing of freeing an allocated ttyname buffer on trigger deactivation. This is a memory leak fix patch and should also be backported to the 'stable' branches.
Patch [4/7]:
As requested by greg k-h this is more a 'dev_warn' instead of a 'dev_info'. This could also be backported to the 'stable' branches if needed.
Patch [5/7]:
Use a completion to sync for sysfs read/write and the delay scheduler work. I hope I am using the completion correctly. I wasn't sure if I should secure the sysfs read and write access at the same time via a mutex. With this change, the work is also not stopped as it was before when no ttyname was set via sysfs. A tty should always be set when this trigger is used. And is therefore not a problem from my point of view.
Patch [6/7]:
Make rx tx activitate configurable. In the previous implementation, there was still the ttytrigger flag variable. This flag variable was replaced by individual variables in the data struct. Now these variables can be accessed without masking. The commit was rebased and cleaned up to use the completion implementation.
Patch [7/7]:
Adding additional trigger line state sources. The commit was also rebased and cleaned up to use the completion implementation.

Changes in v5:
==============
v5: https://lore.kernel.org/linux-leds/[email protected]/
- Update commit message as request by greg k-h, to make the commit message more generic and not focusing on my use case [2].
- Removing PATCH v4 1/3 from previous set. This has been already applied to tty-testing [3] by greg k-h.
- As requested by greq k-h. I have also made the following changes to PATCH v4 3/3 [4].
* Add a comment to the enum that this is already used for bit evaluation and sysfs read and write.
* Renaming the variable 'unsigned long mode' to
'unsigned long ttytrigger' in the ledtrig_tty_data structure to make it clearer that the selected triggers are stored there.
* Using sysfs_emit() function to dump the requestd ttytrigger to userland.
* Also using the kstrtobool() function to write the selected ttytrigger via the sysfs. This values are booleans.
- I also removed the function ledtrig_tty_evaluate() from my last patchses PATCH v4 3/3 [4]. The new API tty_get_tiocm() function is only called once now and checked for each ttytrigger bit. Previously this function was called for each bit, which is not necessary.

Links:
[2] https://lore.kernel.org/linux-leds/2023102115-stock-scrambled-f7d5@gregkh/
[3] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-testing&id=838eb763c3e939a8de8d4c55a17ddcce737685c1
[4] https://lore.kernel.org/linux-leds/[email protected]/

Changes in v4:
==============
v4: https://lore.kernel.org/linux-leds/[email protected]/
- Merging patch 3/4 into patch number 4/4 from previous series, because it fixes a problem that does not exist upstream. This was a note from the build robot regarding my change that I added with previous series. This change was never upstream and therefore this is not relevant.
- Update the commit message of patch 1/3 of this series, that this commit
also changes the 'ndashes' to simple dashes. There were no changes, so I add the 'Reviewed-by' that the commit received before.
- With this patchset version I have reworked my implementation for the evaluation of the additional line state, so that this changes becomes smaller. As basis I have used the staged commits from Christian Marangi that makes this changes to the netdev trigger. This has already been applied to 'for-leds-next-next' by Lee Jones. I adapted this to the tty trigger.
Convert device attr to macro:
https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git/commit/drivers/leds/trigger?h=for-leds-next-next&id=509412749002f4bac4c29f2012fff90c08d8afca
Unify sysfs and state handling:
https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git/commit/drivers/leds/trigger?h=for-leds-next-next&id=0fd93ac8582627bee9a3c824489f302dff722881

Changes in v3:
==============
v3: https://lore.kernel.org/linux-leds/[email protected]/
- Add missing 'kernel test robot' information to the commit message.
- Additional information added to the commit message

Changes in v2:
==============
v2: https://lore.kernel.org/linux-leds/[email protected]/
- rename new function from tty_get_mget() to tty_get_tiocm() as requested by 'Jiri Slaby'.
- As suggested by 'Jiri Slaby', fixed tabs in function documentation throughout the file '/drivers/tty/tty_io.c' in a separate commit.
- Move the variable definition to the top in function 'ledtrig_tty_work()'.
This was reported by the 'kernel test robot' after my change in v1.
- Also set the 'max_brightness' to 'blink_brightness' if no 'blink_brightness' was set. This fixes a problem at startup when the brightness is still set to 0 and only 'line_*' is evaluated. I looked in the netdev trigger and that's exactly how it's done there.

Changes in v1:
==============
v1: https://lore.kernel.org/linux-leds/[email protected]/
This is a follow-up patchset, based on the mailing list discussion from March 2023 based on the old patchset v8 [1]. I have changed, the LED trigger handling via the sysfs interfaces as suggested by Uwe Kleine-König.
Links:
[1] https://lore.kernel.org/linux-leds/[email protected]/

*** BLURB HERE ***

Florian Eckert (4):
tty: add new helper function tty_get_tiocm
leds: ledtrig-tty: replace mutex with completion
leds: ledtrig-tty: make rx tx activitate configurable
leds: ledtrig-tty: add additional line state evaluation

 .../ABI/testing/sysfs-class-led-trigger-tty   |  56 ++++
 drivers/leds/trigger/ledtrig-tty.c            | 243 +++++++++++++++---
 drivers/tty/tty_io.c                          |  28 +-
 include/linux/tty.h                           |   1 +
 4 files changed, 285 insertions(+), 43 deletions(-)

base-commit: 2cc14f52aeb78ce3f29677c2de1f06c0e91471ab

2.30.2

There is no in-kernel function to get the status register of a tty device like the TIOCMGET ioctl returns to userspace. Create a new function, tty_get_tiocm(), to obtain the status register that other portions of the kernel can call if they need this information, and move the existing internal tty_tiocmget() function to use this interface.

---
 drivers/tty/tty_io.c | 28 ++++++++++++++++++++++------
 include/linux/tty.h  |  1 +
 2 files changed, 23 insertions(+), 6 deletions(-)

index 06414e43e0b5..e2e93404133e 100644
@@ -2498,6 +2498,24 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
 	return retval;
 }
 
+/**
+ * tty_get_tiocm - get tiocm status register
+ * @tty: tty device
+ *
+ * Obtain the modem status bits from the tty driver if the feature
+ * is supported.
+ */
+int tty_get_tiocm(struct tty_struct *tty)
+{
+	int retval = -ENOTTY;
+
+	if (tty->ops->tiocmget)
+		retval = tty->ops->tiocmget(tty);
+
+	return retval;
+}
+EXPORT_SYMBOL_GPL(tty_get_tiocm);
+
 /**
  * tty_tiocmget - get modem status
  * @tty: tty device

@@ -2510,14 +2528,12 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
  */
 static int tty_tiocmget(struct tty_struct *tty, int __user *p)
 {
-	int retval = -ENOTTY;
+	int retval;
 
-	if (tty->ops->tiocmget) {
-		retval = tty->ops->tiocmget(tty);
+	retval = tty_get_tiocm(tty);
+	if (retval >= 0)
+		retval = put_user(retval, p);
 
-		if (retval >= 0)
-			retval = put_user(retval, p);
-	}
 	return retval;
 }
 

index 4b6340ac2af2..d219a11e3fe0 100644
@@ -419,6 +419,7 @@ bool tty_unthrottle_safe(struct tty_struct *tty);
 int tty_do_resize(struct tty_struct *tty, struct winsize *ws);
 int tty_get_icount(struct tty_struct *tty,
 		struct serial_icounter_struct *icount);
+int tty_get_tiocm(struct tty_struct *tty);
 int is_current_pgrp_orphaned(void);
 void tty_hangup(struct tty_struct *tty);
 void tty_vhangup(struct tty_struct *tty);

2.30.2

With this commit, the mutex handling is replaced by the completion handling. When handling mutex, it must always be ensured that the held mutex is also released again. This is more error-prone should the number of code paths increase.

This is a preparatory commit to make the trigger more configurable via additional sysfs parameters. With this change, the worker always runs and is no longer stopped if no ttyname is set.

---
 drivers/leds/trigger/ledtrig-tty.c | 59 +++++++++++++++---------------
 1 file changed, 30 insertions(+), 29 deletions(-)

index 8ae0d2d284af..0d73bec1529f 100644
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
+#include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/leds.h>
 #include <linux/module.h>

@@ -12,15 +13,23 @@
 struct ledtrig_tty_data {
 	struct led_classdev *led_cdev;
 	struct delayed_work dwork;
-	struct mutex mutex;
+	struct completion sysfs;
 	const char *ttyname;
 	struct tty_struct *tty;
 	int rx, tx;
 };
 
-static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data)
+static int ledtrig_tty_wait_for_completion(struct device *dev)
 {
-	schedule_delayed_work(&trigger_data->dwork, 0);
+	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
+	int ret;
+
+	ret = wait_for_completion_timeout(&trigger_data->sysfs,
+					  msecs_to_jiffies(LEDTRIG_TTY_INTERVAL * 20));
+	if (ret == 0)
+		return -ETIMEDOUT;
+
+	return ret;
 }
 
 static ssize_t ttyname_show(struct device *dev,

@@ -28,14 +37,16 @@ static ssize_t ttyname_show(struct device *dev,
 {
 	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
 	ssize_t len = 0;
+	int completion;
 
-	mutex_lock(&trigger_data->mutex);
+	reinit_completion(&trigger_data->sysfs);
+	completion = ledtrig_tty_wait_for_completion(dev);
+	if (completion < 0)
+		return completion;
 
 	if (trigger_data->ttyname)
 		len = sprintf(buf, "%s\n", trigger_data->ttyname);
 
-	mutex_unlock(&trigger_data->mutex);
-
 	return len;
 }
 

@@ -46,7 +57,7 @@ static ssize_t ttyname_store(struct device *dev,
 	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
 	char *ttyname;
 	ssize_t ret = size;
-	bool running;
+	int completion;
 
 	if (size > 0 && buf[size - 1] == '\n')
 		size -= 1;

@@ -59,9 +70,10 @@ static ssize_t ttyname_store(struct device *dev,
 		ttyname = NULL;
 	}
 
-	mutex_lock(&trigger_data->mutex);
-
-	running = trigger_data->ttyname != NULL;
+	reinit_completion(&trigger_data->sysfs);
+	completion = ledtrig_tty_wait_for_completion(dev);
+	if (completion < 0)
+		return completion;
 
 	kfree(trigger_data->ttyname);
 	tty_kref_put(trigger_data->tty);

@@ -69,11 +81,6 @@ static ssize_t ttyname_store(struct device *dev,
 
 	trigger_data->ttyname = ttyname;
 
-	mutex_unlock(&trigger_data->mutex);
-
-	if (ttyname && !running)
-		ledtrig_tty_restart(trigger_data);
-
 	return ret;
 }
 static DEVICE_ATTR_RW(ttyname);

@@ -85,13 +92,8 @@ static void ledtrig_tty_work(struct work_struct *work)
 	struct serial_icounter_struct icount;
 	int ret;
 
-	mutex_lock(&trigger_data->mutex);
-
-	if (!trigger_data->ttyname) {
-		/* exit without rescheduling */
-		mutex_unlock(&trigger_data->mutex);
-		return;
-	}
+	if (!trigger_data->ttyname)
+		goto out;
 
 	/* try to get the tty corresponding to $ttyname */
 	if (!trigger_data->tty) {

@@ -116,11 +118,8 @@ static void ledtrig_tty_work(struct work_struct *work)
 	}
 
 	ret = tty_get_icount(trigger_data->tty, &icount);
-	if (ret) {
-		dev_info(trigger_data->tty->dev, "Failed to get icount, stopped polling\n");
-		mutex_unlock(&trigger_data->mutex);
-		return;
-	}
+	if (ret)
+		goto out;
 
 	if (icount.rx != trigger_data->rx ||
 	    icount.tx != trigger_data->tx) {

@@ -134,7 +133,7 @@ static void ledtrig_tty_work(struct work_struct *work)
 	}
 
 out:
-	mutex_unlock(&trigger_data->mutex);
+	complete_all(&trigger_data->sysfs);
 	schedule_delayed_work(&trigger_data->dwork,
 			      msecs_to_jiffies(LEDTRIG_TTY_INTERVAL * 2));
 }

@@ -157,7 +156,9 @@ static int ledtrig_tty_activate(struct led_classdev *led_cdev)
 
 	INIT_DELAYED_WORK(&trigger_data->dwork, ledtrig_tty_work);
 	trigger_data->led_cdev = led_cdev;
-	mutex_init(&trigger_data->mutex);
+	init_completion(&trigger_data->sysfs);
+
+	schedule_delayed_work(&trigger_data->dwork, 0);
 
 	return 0;
 }

2.30.2

Until now, the LED blinks when data is sent via the tty (rx/tx). This is not configurable.

This change adds the possibility to make the indication for the direction of the transmitted data independently controllable via the new rx and tx sysfs entries.

- rx:
Signal reception (rx) of data on the named tty device. If set to 0, the LED will not blink on reception. If set to 1 (default), the LED will blink on reception.

- tx:
Signal transmission (tx) of data on the named tty device. If set to 0, the LED will not blink on transmission. If set to 1 (default), the LED will blink on transmission.

This new sysfs entry are on by default. Thus the trigger behaves as before this change.

---
 .../ABI/testing/sysfs-class-led-trigger-tty   |  16 +++
 drivers/leds/trigger/ledtrig-tty.c            | 114 ++++++++++++++++--
 2 files changed, 119 insertions(+), 11 deletions(-)

index 2bf6b24e781b..504dece151b8 100644
@@ -4,3 +4,19 @@ KernelVersion:	5.10
 Contact:	[email protected]
 Description:
 		Specifies the tty device name of the triggering tty
+
+What:		/sys/class/leds/<led>/rx
+Date:		February 2024
+KernelVersion:	6.8
+Description:
+		Signal reception (rx) of data on the named tty device.
+		If set to 0, the LED will not blink on reception.
+		If set to 1 (default), the LED will blink on reception.
+
+What:		/sys/class/leds/<led>/tx
+Date:		February 2024
+KernelVersion:	6.8
+Description:
+		Signal transmission (tx) of data on the named tty device.
+		If set to 0, the LED will not blink on transmission.
+		If set to 1 (default), the LED will blink on transmission.

2.30.2

The serial tty interface also supports additional input signals, that can also be evaluated within this trigger. This change is adding the following additional input sources, which could be controlled via the '/sys/class/<leds>/' sysfs interface.

Explanation:
DCE = Data Communication Equipment (Modem)
DTE = Data Terminal Equipment (Computer)

- cts:
DCE is ready to accept data from the DTE (CTS = Clear To Send). If the line state is detected, the LED is switched on.
If set to 0 (default), the LED will not evaluate CTS. If set to 1, the LED will evaluate CTS.

- dsr:
DCE is ready to receive and send data (DSR = Data Set Ready). If the line state is detected, the LED is switched on.
If set to 0 (default), the LED will not evaluate DSR.
If set to 1, the LED will evaluate DSR.

- dcd:
DTE is receiving a carrier from the DCE (DCD = Data Carrier Detect). If the line state is detected, the LED is switched on.
If set to 0 (default), the LED will not evaluate DCD.
If set to 1, the LED will evaluate DCD.

- rng:
DCE has detected an incoming ring signal on the telephone line (RNG = Ring Indicator). If the line state is detected, the LED is switched on.
If set to 0 (default), the LED will not evaluate RNG. If set to 1, the LED will evaluate RNG.

Also add an invert flag on LED blink, so that the LED blinks in the correct order.

* If one off the new enabled input signals are evaluatet as 'enabled', and data are transmitted, then the LED should first blink 'off' and then 'on' (invert).
* If all the new enabled input signals are evaluatet as 'disabled', and data are transmitted, then the LED should first blink 'on' and then 'off'.

---
 .../ABI/testing/sysfs-class-led-trigger-tty   | 40 ++++++++++
 drivers/leds/trigger/ledtrig-tty.c            | 78 ++++++++++++++++++-
 2 files changed, 117 insertions(+), 1 deletion(-)

index 504dece151b8..30cef9ac0f49 100644
@@ -20,3 +20,43 @@ Description:
 		Signal transmission (tx) of data on the named tty device.
 		If set to 0, the LED will not blink on transmission.
 		If set to 1 (default), the LED will blink on transmission.
+
+What:		/sys/class/leds/<led>/cts
+Date:		February 2024
+KernelVersion:	6.8
+Description:
+		CTS = Clear To Send
+		DCE is ready to accept data from the DTE.
+		If the line state is detected, the LED is switched on.
+		If set to 0 (default), the LED will not evaluate CTS.
+		If set to 1, the LED will evaluate CTS.
+
+What:		/sys/class/leds/<led>/dsr
+Date:		February 2024
+KernelVersion:	6.8
+Description:
+		DSR = Data Set Ready
+		DCE is ready to receive and send data.
+		If the line state is detected, the LED is switched on.
+		If set to 0 (default), the LED will not evaluate DSR.
+		If set to 1, the LED will evaluate DSR.
+
+What:		/sys/class/leds/<led>/dcd
+Date:		February 2024
+KernelVersion:	6.8
+Description:
+		DCD = Data Carrier Detect
+		DTE is receiving a carrier from the DCE.
+		If the line state is detected, the LED is switched on.
+		If set to 0 (default), the LED will not evaluate CAR (DCD).
+		If set to 1, the LED will evaluate CAR (DCD).
+
+What:		/sys/class/leds/<led>/rng
+Date:		February 2024
+KernelVersion:	6.8
+Description:
+		RNG = Ring Indicator
+		DCE has detected an incoming ring signal on the telephone
+		line. If the line state is detected, the LED is switched on.
+		If set to 0 (default), the LED will not evaluate RNG.
+		If set to 1, the LED will evaluate RNG.

2.30.2

Nice work, thanks for sticking with it:

Applied, thanks!

[1/4] tty: add new helper function tty_get_tiocm

      commit: 5d11a4709f552fa139c2439fead05daeb064a6f4
[2/4] leds: ledtrig-tty: replace mutex with completion
      commit: 50744fe65a4c8b78eff9a4b70825dd7c768f9f69
[3/4] leds: ledtrig-tty: make rx tx activitate configurable
      commit: 8e7b857c0eafdc49ff0e63e9252ed0273b9efdbd
[4/4] leds: ledtrig-tty: add additional line state evaluation
      commit: 7f5a46671b2253989b2674407d624f1fcc77d60f

--
Lee Jones [李琼斯]