Linux-Serial Archive mirror
 help / color / mirror / Atom feed
* [Patch v8 0/6] ledtrig-tty: add additional tty state evaluation
@ 2023-11-09  8:50 Florian Eckert
  2023-11-09  8:50 ` [Patch v8 1/6] tty: add new helper function tty_get_tiocm Florian Eckert
                   ` (6 more replies)
  0 siblings, 7 replies; 26+ messages in thread
From: Florian Eckert @ 2023-11-09  8:50 UTC (permalink / raw)
  To: Eckert.Florian, gregkh, jirislaby, pavel, lee, kabel,
	u.kleine-koenig, m.brock
  Cc: linux-kernel, linux-serial, linux-leds

Changes in v8:
==============
- 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/20231106123415.3365732-1-fe@dev.tdt.de/T/#me43be56f4063082e7b47858773ea8067a3846466
[7] https://lore.kernel.org/stable/20231106144914.bflq2jxejdxs6zjb@pengutronix.de/T/#t
[8] https://lore.kernel.org/stable/20231106144914.bflq2jxejdxs6zjb@pengutronix.de/T/#m78a946889e4722903b2a79e3a465d8da0ca16333
[9] https://lore.kernel.org/linux-leds/20231106123415.3365732-1-fe@dev.tdt.de/T/#m43df92b665f613fe0af7d5d003a3804404f1c494

Changes in v7:
==============
v7: https://lore.kernel.org/linux-leds/20231106123415.3365732-1-fe@dev.tdt.de/
- 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/20231030100447.63477-1-fe@dev.tdt.de/
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/20231023094205.2706812-1-fe@dev.tdt.de/
- 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/20231019112809.881730-4-fe@dev.tdt.de/

Changes in v4:
==============
v4: https://lore.kernel.org/linux-leds/20231019112809.881730-1-fe@dev.tdt.de/
- 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/20231016071332.597654-1-fe@dev.tdt.de/
- 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/20230928132632.200263-1-fe@dev.tdt.de/
- 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/20230926093607.59536-1-fe@dev.tdt.de/
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/20230306094113.273988-1-fe@dev.tdt.de/

Florian Eckert (6):
  tty: add new helper function tty_get_tiocm
  leds: ledtrig-tty: free allocated ttyname buffer on deactivate
  leds: ledtrig-tty: change logging if get icount failed
  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            | 259 +++++++++++++++---
 drivers/tty/tty_io.c                          |  28 +-
 include/linux/tty.h                           |   1 +
 4 files changed, 301 insertions(+), 43 deletions(-)


base-commit: 6bc986ab839c844e78a2333a02e55f02c9e57935
-- 
2.30.2


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

* [Patch v8 1/6] tty: add new helper function tty_get_tiocm
  2023-11-09  8:50 [Patch v8 0/6] ledtrig-tty: add additional tty state evaluation Florian Eckert
@ 2023-11-09  8:50 ` Florian Eckert
  2023-11-20  7:21   ` Jiri Slaby
  2023-11-09  8:50 ` [Patch v8 2/6] leds: ledtrig-tty: free allocated ttyname buffer on deactivate Florian Eckert
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 26+ messages in thread
From: Florian Eckert @ 2023-11-09  8:50 UTC (permalink / raw)
  To: Eckert.Florian, gregkh, jirislaby, pavel, lee, kabel,
	u.kleine-koenig, m.brock
  Cc: linux-kernel, linux-serial, linux-leds

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.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/tty/tty_io.c | 28 ++++++++++++++++++++++------
 include/linux/tty.h  |  1 +
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 06414e43e0b5..e2e93404133e 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -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;
 }
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 4b6340ac2af2..d219a11e3fe0 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -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


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

* [Patch v8 2/6] leds: ledtrig-tty: free allocated ttyname buffer on deactivate
  2023-11-09  8:50 [Patch v8 0/6] ledtrig-tty: add additional tty state evaluation Florian Eckert
  2023-11-09  8:50 ` [Patch v8 1/6] tty: add new helper function tty_get_tiocm Florian Eckert
@ 2023-11-09  8:50 ` Florian Eckert
  2023-11-23 14:06   ` Greg KH
  2023-11-09  8:50 ` [Patch v8 3/6] leds: ledtrig-tty: change logging if get icount failed Florian Eckert
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 26+ messages in thread
From: Florian Eckert @ 2023-11-09  8:50 UTC (permalink / raw)
  To: Eckert.Florian, gregkh, jirislaby, pavel, lee, kabel,
	u.kleine-koenig, m.brock
  Cc: linux-kernel, linux-serial, linux-leds

The ttyname buffer for the ledtrig_tty_data struct is allocated in the
sysfs ttyname_store() function. This buffer must be released on trigger
deactivation. This was missing and is thus a memory leak.

While we are at it, the tty handler in the ledtrig_tty_data struct should
also be returned in case of the trigger deactivation call.

Fixes: fd4a641ac88f ("leds: trigger: implement a tty trigger")
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/leds/trigger/ledtrig-tty.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
index 8ae0d2d284af..3e69a7bde928 100644
--- a/drivers/leds/trigger/ledtrig-tty.c
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -168,6 +168,10 @@ static void ledtrig_tty_deactivate(struct led_classdev *led_cdev)
 
 	cancel_delayed_work_sync(&trigger_data->dwork);
 
+	kfree(trigger_data->ttyname);
+	tty_kref_put(trigger_data->tty);
+	trigger_data->tty = NULL;
+
 	kfree(trigger_data);
 }
 
-- 
2.30.2


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

* [Patch v8 3/6] leds: ledtrig-tty: change logging if get icount failed
  2023-11-09  8:50 [Patch v8 0/6] ledtrig-tty: add additional tty state evaluation Florian Eckert
  2023-11-09  8:50 ` [Patch v8 1/6] tty: add new helper function tty_get_tiocm Florian Eckert
  2023-11-09  8:50 ` [Patch v8 2/6] leds: ledtrig-tty: free allocated ttyname buffer on deactivate Florian Eckert
@ 2023-11-09  8:50 ` Florian Eckert
  2023-11-23 14:08   ` Greg KH
  2023-11-09  8:50 ` [Patch v8 4/6] leds: ledtrig-tty: replace mutex with completion Florian Eckert
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 26+ messages in thread
From: Florian Eckert @ 2023-11-09  8:50 UTC (permalink / raw)
  To: Eckert.Florian, gregkh, jirislaby, pavel, lee, kabel,
	u.kleine-koenig, m.brock
  Cc: linux-kernel, linux-serial, linux-leds

Change the log level from info to warn, because there is something
wrong. That is more a warn message than an info message.

While we are at it, the device prefix is also changed, as this is the
led device and not the tty device that generates this message.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/leds/trigger/ledtrig-tty.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
index 3e69a7bde928..86595add72cd 100644
--- a/drivers/leds/trigger/ledtrig-tty.c
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -83,6 +83,7 @@ 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;
+	struct led_classdev *led_cdev = trigger_data->led_cdev;
 	int ret;
 
 	mutex_lock(&trigger_data->mutex);
@@ -117,7 +118,7 @@ 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");
+		dev_warn(led_cdev->dev, "Failed to get icount, stop polling\n");
 		mutex_unlock(&trigger_data->mutex);
 		return;
 	}
@@ -126,8 +127,7 @@ static void ledtrig_tty_work(struct work_struct *work)
 	    icount.tx != trigger_data->tx) {
 		unsigned long interval = LEDTRIG_TTY_INTERVAL;
 
-		led_blink_set_oneshot(trigger_data->led_cdev, &interval,
-				      &interval, 0);
+		led_blink_set_oneshot(led_cdev, &interval, &interval, 0);
 
 		trigger_data->rx = icount.rx;
 		trigger_data->tx = icount.tx;
-- 
2.30.2


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

* [Patch v8 4/6] leds: ledtrig-tty: replace mutex with completion
  2023-11-09  8:50 [Patch v8 0/6] ledtrig-tty: add additional tty state evaluation Florian Eckert
                   ` (2 preceding siblings ...)
  2023-11-09  8:50 ` [Patch v8 3/6] leds: ledtrig-tty: change logging if get icount failed Florian Eckert
@ 2023-11-09  8:50 ` Florian Eckert
  2023-11-23 14:10   ` Greg KH
  2023-11-09  8:50 ` [Patch v8 5/6] leds: ledtrig-tty: make rx tx activitate configurable Florian Eckert
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 26+ messages in thread
From: Florian Eckert @ 2023-11-09  8:50 UTC (permalink / raw)
  To: Eckert.Florian, gregkh, jirislaby, pavel, lee, kabel,
	u.kleine-koenig, m.brock
  Cc: linux-kernel, linux-serial, linux-leds

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.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/leds/trigger/ledtrig-tty.c | 60 +++++++++++++++---------------
 1 file changed, 31 insertions(+), 29 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
index 86595add72cd..3badf74fa420 100644
--- a/drivers/leds/trigger/ledtrig-tty.c
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -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,24 @@
 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_waitforcompletion(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 +38,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_waitforcompletion(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 +58,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 +71,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_waitforcompletion(dev);
+	if (completion < 0)
+		return completion;
 
 	kfree(trigger_data->ttyname);
 	tty_kref_put(trigger_data->tty);
@@ -69,11 +82,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);
@@ -86,13 +94,8 @@ static void ledtrig_tty_work(struct work_struct *work)
 	struct led_classdev *led_cdev = trigger_data->led_cdev;
 	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) {
@@ -117,11 +120,8 @@ static void ledtrig_tty_work(struct work_struct *work)
 	}
 
 	ret = tty_get_icount(trigger_data->tty, &icount);
-	if (ret) {
-		dev_warn(led_cdev->dev, "Failed to get icount, stop 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 +134,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 +157,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


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

* [Patch v8 5/6] leds: ledtrig-tty: make rx tx activitate configurable
  2023-11-09  8:50 [Patch v8 0/6] ledtrig-tty: add additional tty state evaluation Florian Eckert
                   ` (3 preceding siblings ...)
  2023-11-09  8:50 ` [Patch v8 4/6] leds: ledtrig-tty: replace mutex with completion Florian Eckert
@ 2023-11-09  8:50 ` Florian Eckert
  2023-11-23 14:12   ` Greg KH
  2023-11-09  8:50 ` [Patch v8 6/6] leds: ledtrig-tty: add additional line state evaluation Florian Eckert
  2023-12-01 10:40 ` [Patch v8 0/6] ledtrig-tty: add additional tty " Lee Jones
  6 siblings, 1 reply; 26+ messages in thread
From: Florian Eckert @ 2023-11-09  8:50 UTC (permalink / raw)
  To: Eckert.Florian, gregkh, jirislaby, pavel, lee, kabel,
	u.kleine-koenig, m.brock
  Cc: linux-kernel, linux-serial, linux-leds

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.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 .../ABI/testing/sysfs-class-led-trigger-tty   |  16 +++
 drivers/leds/trigger/ledtrig-tty.c            | 124 ++++++++++++++++--
 2 files changed, 130 insertions(+), 10 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
index 2bf6b24e781b..504dece151b8 100644
--- a/Documentation/ABI/testing/sysfs-class-led-trigger-tty
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
@@ -4,3 +4,19 @@ KernelVersion:	5.10
 Contact:	linux-leds@vger.kernel.org
 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.
diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
index 3badf74fa420..1a40a78bf1ee 100644
--- a/drivers/leds/trigger/ledtrig-tty.c
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -17,6 +17,19 @@ struct ledtrig_tty_data {
 	const char *ttyname;
 	struct tty_struct *tty;
 	int rx, tx;
+	bool mode_rx;
+	bool mode_tx;
+};
+
+/* Indicates which state the LED should now display */
+enum led_trigger_tty_state {
+	TTY_LED_BLINK,
+	TTY_LED_DISABLE,
+};
+
+enum led_trigger_tty_modes {
+	TRIGGER_TTY_RX = 0,
+	TRIGGER_TTY_TX,
 };
 
 static int ledtrig_tty_waitforcompletion(struct device *dev)
@@ -86,12 +99,82 @@ static ssize_t ttyname_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(ttyname);
 
+static ssize_t ledtrig_tty_attr_show(struct device *dev, char *buf,
+				     enum led_trigger_tty_modes attr)
+{
+	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
+	int completion;
+	bool state;
+
+	reinit_completion(&trigger_data->sysfs);
+	completion = ledtrig_tty_waitforcompletion(dev);
+	if (completion < 0)
+		return completion;
+
+	switch (attr) {
+	case TRIGGER_TTY_RX:
+		state = trigger_data->mode_rx;
+		break;
+	case TRIGGER_TTY_TX:
+		state = trigger_data->mode_tx;
+		break;
+	}
+
+	return sysfs_emit(buf, "%u\n", state);
+}
+
+static ssize_t ledtrig_tty_attr_store(struct device *dev, const char *buf,
+				      size_t size, enum led_trigger_tty_modes attr)
+{
+	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
+	int completion;
+	bool state;
+	int ret;
+
+	ret = kstrtobool(buf, &state);
+	if (ret)
+		return ret;
+
+	reinit_completion(&trigger_data->sysfs);
+	completion = ledtrig_tty_waitforcompletion(dev);
+	if (completion < 0)
+		return completion;
+
+	switch (attr) {
+	case TRIGGER_TTY_RX:
+		trigger_data->mode_rx = state;
+		break;
+	case TRIGGER_TTY_TX:
+		trigger_data->mode_tx = state;
+		break;
+	}
+
+	return size;
+}
+
+#define DEFINE_TTY_TRIGGER(trigger_name, trigger) \
+	static ssize_t trigger_name##_show(struct device *dev, \
+		struct device_attribute *attr, char *buf) \
+	{ \
+		return ledtrig_tty_attr_show(dev, buf, trigger); \
+	} \
+	static ssize_t trigger_name##_store(struct device *dev, \
+		struct device_attribute *attr, const char *buf, size_t size) \
+	{ \
+		return ledtrig_tty_attr_store(dev, buf, size, trigger); \
+	} \
+	static DEVICE_ATTR_RW(trigger_name)
+
+DEFINE_TTY_TRIGGER(rx, TRIGGER_TTY_RX);
+DEFINE_TTY_TRIGGER(tx, TRIGGER_TTY_TX);
+
 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;
 	struct led_classdev *led_cdev = trigger_data->led_cdev;
+	enum led_trigger_tty_state state = TTY_LED_DISABLE;
+	unsigned long interval = LEDTRIG_TTY_INTERVAL;
 	int ret;
 
 	if (!trigger_data->ttyname)
@@ -119,21 +202,36 @@ static void ledtrig_tty_work(struct work_struct *work)
 		trigger_data->tty = tty;
 	}
 
-	ret = tty_get_icount(trigger_data->tty, &icount);
-	if (ret)
-		goto out;
+	if (trigger_data->mode_rx || trigger_data->mode_tx) {
+		struct serial_icounter_struct icount;
 
-	if (icount.rx != trigger_data->rx ||
-	    icount.tx != trigger_data->tx) {
-		unsigned long interval = LEDTRIG_TTY_INTERVAL;
+		ret = tty_get_icount(trigger_data->tty, &icount);
+		if (ret)
+			goto out;
 
-		led_blink_set_oneshot(led_cdev, &interval, &interval, 0);
+		if (trigger_data->mode_tx && (icount.tx != trigger_data->tx)) {
+			trigger_data->tx = icount.tx;
+			state = TTY_LED_BLINK;
+		}
 
-		trigger_data->rx = icount.rx;
-		trigger_data->tx = icount.tx;
+		if (trigger_data->mode_rx && (icount.rx != trigger_data->rx)) {
+			trigger_data->rx = icount.rx;
+			state = TTY_LED_BLINK;
+		}
 	}
 
 out:
+	switch (state) {
+	case TTY_LED_BLINK:
+		led_blink_set_oneshot(led_cdev, &interval, &interval, 0);
+		break;
+	case TTY_LED_DISABLE:
+		fallthrough;
+	default:
+		led_set_brightness(led_cdev, LED_OFF);
+		break;
+	}
+
 	complete_all(&trigger_data->sysfs);
 	schedule_delayed_work(&trigger_data->dwork,
 			      msecs_to_jiffies(LEDTRIG_TTY_INTERVAL * 2));
@@ -141,6 +239,8 @@ static void ledtrig_tty_work(struct work_struct *work)
 
 static struct attribute *ledtrig_tty_attrs[] = {
 	&dev_attr_ttyname.attr,
+	&dev_attr_rx.attr,
+	&dev_attr_tx.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(ledtrig_tty);
@@ -153,6 +253,10 @@ static int ledtrig_tty_activate(struct led_classdev *led_cdev)
 	if (!trigger_data)
 		return -ENOMEM;
 
+	/* Enable default rx/tx mode */
+	trigger_data->mode_rx = true;
+	trigger_data->mode_tx = true;
+
 	led_set_trigger_data(led_cdev, trigger_data);
 
 	INIT_DELAYED_WORK(&trigger_data->dwork, ledtrig_tty_work);
-- 
2.30.2


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

* [Patch v8 6/6] leds: ledtrig-tty: add additional line state evaluation
  2023-11-09  8:50 [Patch v8 0/6] ledtrig-tty: add additional tty state evaluation Florian Eckert
                   ` (4 preceding siblings ...)
  2023-11-09  8:50 ` [Patch v8 5/6] leds: ledtrig-tty: make rx tx activitate configurable Florian Eckert
@ 2023-11-09  8:50 ` Florian Eckert
  2023-11-09 19:03   ` m.brock
  2023-12-01 10:40 ` [Patch v8 0/6] ledtrig-tty: add additional tty " Lee Jones
  6 siblings, 1 reply; 26+ messages in thread
From: Florian Eckert @ 2023-11-09  8:50 UTC (permalink / raw)
  To: Eckert.Florian, gregkh, jirislaby, pavel, lee, kabel,
	u.kleine-koenig, m.brock
  Cc: linux-kernel, linux-serial, linux-leds

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

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 .../ABI/testing/sysfs-class-led-trigger-tty   | 40 ++++++++++
 drivers/leds/trigger/ledtrig-tty.c            | 77 ++++++++++++++++++-
 2 files changed, 116 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
index 504dece151b8..30cef9ac0f49 100644
--- a/Documentation/ABI/testing/sysfs-class-led-trigger-tty
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
@@ -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.
diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
index 1a40a78bf1ee..7291b2d970c6 100644
--- a/drivers/leds/trigger/ledtrig-tty.c
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -19,17 +19,26 @@ struct ledtrig_tty_data {
 	int rx, tx;
 	bool mode_rx;
 	bool mode_tx;
+	bool mode_cts;
+	bool mode_dsr;
+	bool mode_dcd;
+	bool mode_rng;
 };
 
 /* Indicates which state the LED should now display */
 enum led_trigger_tty_state {
 	TTY_LED_BLINK,
+	TTY_LED_ENABLE,
 	TTY_LED_DISABLE,
 };
 
 enum led_trigger_tty_modes {
 	TRIGGER_TTY_RX = 0,
 	TRIGGER_TTY_TX,
+	TRIGGER_TTY_CTS,
+	TRIGGER_TTY_DSR,
+	TRIGGER_TTY_DCD,
+	TRIGGER_TTY_RNG,
 };
 
 static int ledtrig_tty_waitforcompletion(struct device *dev)
@@ -118,6 +127,18 @@ static ssize_t ledtrig_tty_attr_show(struct device *dev, char *buf,
 	case TRIGGER_TTY_TX:
 		state = trigger_data->mode_tx;
 		break;
+	case TRIGGER_TTY_CTS:
+		state = trigger_data->mode_cts;
+		break;
+	case TRIGGER_TTY_DSR:
+		state = trigger_data->mode_dsr;
+		break;
+	case TRIGGER_TTY_DCD:
+		state = trigger_data->mode_dcd;
+		break;
+	case TRIGGER_TTY_RNG:
+		state = trigger_data->mode_rng;
+		break;
 	}
 
 	return sysfs_emit(buf, "%u\n", state);
@@ -147,6 +168,18 @@ static ssize_t ledtrig_tty_attr_store(struct device *dev, const char *buf,
 	case TRIGGER_TTY_TX:
 		trigger_data->mode_tx = state;
 		break;
+	case TRIGGER_TTY_CTS:
+		trigger_data->mode_cts = state;
+		break;
+	case TRIGGER_TTY_DSR:
+		trigger_data->mode_dsr = state;
+		break;
+	case TRIGGER_TTY_DCD:
+		trigger_data->mode_dcd = state;
+		break;
+	case TRIGGER_TTY_RNG:
+		trigger_data->mode_rng = state;
+		break;
 	}
 
 	return size;
@@ -167,6 +200,10 @@ static ssize_t ledtrig_tty_attr_store(struct device *dev, const char *buf,
 
 DEFINE_TTY_TRIGGER(rx, TRIGGER_TTY_RX);
 DEFINE_TTY_TRIGGER(tx, TRIGGER_TTY_TX);
+DEFINE_TTY_TRIGGER(cts, TRIGGER_TTY_CTS);
+DEFINE_TTY_TRIGGER(dsr, TRIGGER_TTY_DSR);
+DEFINE_TTY_TRIGGER(dcd, TRIGGER_TTY_DCD);
+DEFINE_TTY_TRIGGER(rng, TRIGGER_TTY_RNG);
 
 static void ledtrig_tty_work(struct work_struct *work)
 {
@@ -175,6 +212,8 @@ static void ledtrig_tty_work(struct work_struct *work)
 	struct led_classdev *led_cdev = trigger_data->led_cdev;
 	enum led_trigger_tty_state state = TTY_LED_DISABLE;
 	unsigned long interval = LEDTRIG_TTY_INTERVAL;
+	bool invert = false;
+	int status;
 	int ret;
 
 	if (!trigger_data->ttyname)
@@ -202,6 +241,33 @@ static void ledtrig_tty_work(struct work_struct *work)
 		trigger_data->tty = tty;
 	}
 
+	status = tty_get_tiocm(trigger_data->tty);
+	if (status > 0) {
+		if (trigger_data->mode_cts) {
+			if (status & TIOCM_CTS)
+				state = TTY_LED_ENABLE;
+		}
+
+		if (trigger_data->mode_dsr) {
+			if (status & TIOCM_DSR)
+				state = TTY_LED_ENABLE;
+		}
+
+		if (trigger_data->mode_dcd) {
+			if (status & TIOCM_CAR)
+				state = TTY_LED_ENABLE;
+		}
+
+		if (trigger_data->mode_rng) {
+			if (status & TIOCM_RNG)
+				state = TTY_LED_ENABLE;
+		}
+	}
+
+	/*
+	 * The evaluation of rx/tx must be done after the evaluation
+	 * of TIOCM_*, because rx/tx has priority.
+	 */
 	if (trigger_data->mode_rx || trigger_data->mode_tx) {
 		struct serial_icounter_struct icount;
 
@@ -211,11 +277,13 @@ static void ledtrig_tty_work(struct work_struct *work)
 
 		if (trigger_data->mode_tx && (icount.tx != trigger_data->tx)) {
 			trigger_data->tx = icount.tx;
+			invert = state == TTY_LED_ENABLE;
 			state = TTY_LED_BLINK;
 		}
 
 		if (trigger_data->mode_rx && (icount.rx != trigger_data->rx)) {
 			trigger_data->rx = icount.rx;
+			invert = state == TTY_LED_ENABLE;
 			state = TTY_LED_BLINK;
 		}
 	}
@@ -223,7 +291,10 @@ static void ledtrig_tty_work(struct work_struct *work)
 out:
 	switch (state) {
 	case TTY_LED_BLINK:
-		led_blink_set_oneshot(led_cdev, &interval, &interval, 0);
+		led_blink_set_oneshot(led_cdev, &interval, &interval, invert);
+		break;
+	case TTY_LED_ENABLE:
+		led_set_brightness(led_cdev, led_cdev->blink_brightness);
 		break;
 	case TTY_LED_DISABLE:
 		fallthrough;
@@ -241,6 +312,10 @@ static struct attribute *ledtrig_tty_attrs[] = {
 	&dev_attr_ttyname.attr,
 	&dev_attr_rx.attr,
 	&dev_attr_tx.attr,
+	&dev_attr_cts.attr,
+	&dev_attr_dsr.attr,
+	&dev_attr_dcd.attr,
+	&dev_attr_rng.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(ledtrig_tty);
-- 
2.30.2


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

* Re: [Patch v8 6/6] leds: ledtrig-tty: add additional line state evaluation
  2023-11-09  8:50 ` [Patch v8 6/6] leds: ledtrig-tty: add additional line state evaluation Florian Eckert
@ 2023-11-09 19:03   ` m.brock
  2023-11-17 12:12     ` Lee Jones
  0 siblings, 1 reply; 26+ messages in thread
From: m.brock @ 2023-11-09 19:03 UTC (permalink / raw)
  To: Florian Eckert
  Cc: Eckert.Florian, gregkh, jirislaby, pavel, lee, kabel,
	u.kleine-koenig, linux-kernel, linux-serial, linux-leds

Florian Eckert schreef op 2023-11-09 09:50:
> 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'.
> 
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ---
>  .../ABI/testing/sysfs-class-led-trigger-tty   | 40 ++++++++++
>  drivers/leds/trigger/ledtrig-tty.c            | 77 ++++++++++++++++++-
>  2 files changed, 116 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> index 504dece151b8..30cef9ac0f49 100644
> --- a/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> @@ -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.
> diff --git a/drivers/leds/trigger/ledtrig-tty.c
> b/drivers/leds/trigger/ledtrig-tty.c
> index 1a40a78bf1ee..7291b2d970c6 100644
> --- a/drivers/leds/trigger/ledtrig-tty.c
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -19,17 +19,26 @@ struct ledtrig_tty_data {
>  	int rx, tx;
>  	bool mode_rx;
>  	bool mode_tx;
> +	bool mode_cts;
> +	bool mode_dsr;
> +	bool mode_dcd;
> +	bool mode_rng;
>  };
> 
>  /* Indicates which state the LED should now display */
>  enum led_trigger_tty_state {
>  	TTY_LED_BLINK,
> +	TTY_LED_ENABLE,
>  	TTY_LED_DISABLE,
>  };
> 
>  enum led_trigger_tty_modes {
>  	TRIGGER_TTY_RX = 0,
>  	TRIGGER_TTY_TX,
> +	TRIGGER_TTY_CTS,
> +	TRIGGER_TTY_DSR,
> +	TRIGGER_TTY_DCD,
> +	TRIGGER_TTY_RNG,
>  };
> 
>  static int ledtrig_tty_waitforcompletion(struct device *dev)
> @@ -118,6 +127,18 @@ static ssize_t ledtrig_tty_attr_show(struct
> device *dev, char *buf,
>  	case TRIGGER_TTY_TX:
>  		state = trigger_data->mode_tx;
>  		break;
> +	case TRIGGER_TTY_CTS:
> +		state = trigger_data->mode_cts;
> +		break;
> +	case TRIGGER_TTY_DSR:
> +		state = trigger_data->mode_dsr;
> +		break;
> +	case TRIGGER_TTY_DCD:
> +		state = trigger_data->mode_dcd;
> +		break;
> +	case TRIGGER_TTY_RNG:
> +		state = trigger_data->mode_rng;
> +		break;
>  	}
> 
>  	return sysfs_emit(buf, "%u\n", state);
> @@ -147,6 +168,18 @@ static ssize_t ledtrig_tty_attr_store(struct
> device *dev, const char *buf,
>  	case TRIGGER_TTY_TX:
>  		trigger_data->mode_tx = state;
>  		break;
> +	case TRIGGER_TTY_CTS:
> +		trigger_data->mode_cts = state;
> +		break;
> +	case TRIGGER_TTY_DSR:
> +		trigger_data->mode_dsr = state;
> +		break;
> +	case TRIGGER_TTY_DCD:
> +		trigger_data->mode_dcd = state;
> +		break;
> +	case TRIGGER_TTY_RNG:
> +		trigger_data->mode_rng = state;
> +		break;
>  	}
> 
>  	return size;
> @@ -167,6 +200,10 @@ static ssize_t ledtrig_tty_attr_store(struct
> device *dev, const char *buf,
> 
>  DEFINE_TTY_TRIGGER(rx, TRIGGER_TTY_RX);
>  DEFINE_TTY_TRIGGER(tx, TRIGGER_TTY_TX);
> +DEFINE_TTY_TRIGGER(cts, TRIGGER_TTY_CTS);
> +DEFINE_TTY_TRIGGER(dsr, TRIGGER_TTY_DSR);
> +DEFINE_TTY_TRIGGER(dcd, TRIGGER_TTY_DCD);
> +DEFINE_TTY_TRIGGER(rng, TRIGGER_TTY_RNG);
> 
>  static void ledtrig_tty_work(struct work_struct *work)
>  {
> @@ -175,6 +212,8 @@ static void ledtrig_tty_work(struct work_struct 
> *work)
>  	struct led_classdev *led_cdev = trigger_data->led_cdev;
>  	enum led_trigger_tty_state state = TTY_LED_DISABLE;
>  	unsigned long interval = LEDTRIG_TTY_INTERVAL;
> +	bool invert = false;
> +	int status;
>  	int ret;
> 
>  	if (!trigger_data->ttyname)
> @@ -202,6 +241,33 @@ static void ledtrig_tty_work(struct work_struct 
> *work)
>  		trigger_data->tty = tty;
>  	}
> 
> +	status = tty_get_tiocm(trigger_data->tty);
> +	if (status > 0) {
> +		if (trigger_data->mode_cts) {
> +			if (status & TIOCM_CTS)
> +				state = TTY_LED_ENABLE;
> +		}
> +
> +		if (trigger_data->mode_dsr) {
> +			if (status & TIOCM_DSR)
> +				state = TTY_LED_ENABLE;
> +		}
> +
> +		if (trigger_data->mode_dcd) {
> +			if (status & TIOCM_CAR)
> +				state = TTY_LED_ENABLE;
> +		}
> +
> +		if (trigger_data->mode_rng) {
> +			if (status & TIOCM_RNG)
> +				state = TTY_LED_ENABLE;
> +		}
> +	}
> +
> +	/*
> +	 * The evaluation of rx/tx must be done after the evaluation
> +	 * of TIOCM_*, because rx/tx has priority.
> +	 */
>  	if (trigger_data->mode_rx || trigger_data->mode_tx) {
>  		struct serial_icounter_struct icount;
> 
> @@ -211,11 +277,13 @@ static void ledtrig_tty_work(struct work_struct 
> *work)
> 
>  		if (trigger_data->mode_tx && (icount.tx != trigger_data->tx)) {
>  			trigger_data->tx = icount.tx;
> +			invert = state == TTY_LED_ENABLE;
>  			state = TTY_LED_BLINK;
>  		}
> 
>  		if (trigger_data->mode_rx && (icount.rx != trigger_data->rx)) {
>  			trigger_data->rx = icount.rx;
> +			invert = state == TTY_LED_ENABLE;
>  			state = TTY_LED_BLINK;
>  		}
>  	}
> @@ -223,7 +291,10 @@ static void ledtrig_tty_work(struct work_struct 
> *work)
>  out:
>  	switch (state) {
>  	case TTY_LED_BLINK:
> -		led_blink_set_oneshot(led_cdev, &interval, &interval, 0);
> +		led_blink_set_oneshot(led_cdev, &interval, &interval, invert);
> +		break;
> +	case TTY_LED_ENABLE:
> +		led_set_brightness(led_cdev, led_cdev->blink_brightness);
>  		break;
>  	case TTY_LED_DISABLE:
>  		fallthrough;
> @@ -241,6 +312,10 @@ static struct attribute *ledtrig_tty_attrs[] = {
>  	&dev_attr_ttyname.attr,
>  	&dev_attr_rx.attr,
>  	&dev_attr_tx.attr,
> +	&dev_attr_cts.attr,
> +	&dev_attr_dsr.attr,
> +	&dev_attr_dcd.attr,
> +	&dev_attr_rng.attr,
>  	NULL
>  };
>  ATTRIBUTE_GROUPS(ledtrig_tty);

Reviewed-by: Maarten Brock <m.brock@vanmierlo.com>


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

* Re: [Patch v8 6/6] leds: ledtrig-tty: add additional line state evaluation
  2023-11-09 19:03   ` m.brock
@ 2023-11-17 12:12     ` Lee Jones
  2023-11-20  7:21       ` Florian Eckert
  0 siblings, 1 reply; 26+ messages in thread
From: Lee Jones @ 2023-11-17 12:12 UTC (permalink / raw)
  To: m.brock
  Cc: Florian Eckert, Eckert.Florian, gregkh, jirislaby, pavel, kabel,
	u.kleine-koenig, linux-kernel, linux-serial, linux-leds

On Thu, 09 Nov 2023, m.brock@vanmierlo.com wrote:

> Florian Eckert schreef op 2023-11-09 09:50:
> > 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'.
> > 
> > Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> > ---
> >  .../ABI/testing/sysfs-class-led-trigger-tty   | 40 ++++++++++
> >  drivers/leds/trigger/ledtrig-tty.c            | 77 ++++++++++++++++++-
> >  2 files changed, 116 insertions(+), 1 deletion(-)

[...]

> Reviewed-by: Maarten Brock <m.brock@vanmierlo.com>

Please snip your replies.

-- 
Lee Jones [李琼斯]

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

* Re: [Patch v8 6/6] leds: ledtrig-tty: add additional line state evaluation
  2023-11-17 12:12     ` Lee Jones
@ 2023-11-20  7:21       ` Florian Eckert
  2023-11-21 15:23         ` Lee Jones
  0 siblings, 1 reply; 26+ messages in thread
From: Florian Eckert @ 2023-11-20  7:21 UTC (permalink / raw)
  To: Lee Jones
  Cc: m.brock, Eckert.Florian, gregkh, jirislaby, pavel, kabel,
	u.kleine-koenig, linux-kernel, linux-serial, linux-leds



On 2023-11-17 13:12, Lee Jones wrote:
> On Thu, 09 Nov 2023, m.brock@vanmierlo.com wrote:
> 
>> Florian Eckert schreef op 2023-11-09 09:50:
>> > 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'.
>> >
>> > Signed-off-by: Florian Eckert <fe@dev.tdt.de>
>> > ---
>> >  .../ABI/testing/sysfs-class-led-trigger-tty   | 40 ++++++++++
>> >  drivers/leds/trigger/ledtrig-tty.c            | 77 ++++++++++++++++++-
>> >  2 files changed, 116 insertions(+), 1 deletion(-)
> 
> [...]
> 
>> Reviewed-by: Maarten Brock <m.brock@vanmierlo.com>
> 
> Please snip your replies.

Is there anything I can do? Or do I have to do something? Please give me 
more detailed instructions

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

* Re: [Patch v8 1/6] tty: add new helper function tty_get_tiocm
  2023-11-09  8:50 ` [Patch v8 1/6] tty: add new helper function tty_get_tiocm Florian Eckert
@ 2023-11-20  7:21   ` Jiri Slaby
  2023-11-21  7:35     ` Florian Eckert
  0 siblings, 1 reply; 26+ messages in thread
From: Jiri Slaby @ 2023-11-20  7:21 UTC (permalink / raw)
  To: Florian Eckert, Eckert.Florian, gregkh, pavel, lee, kabel,
	u.kleine-koenig, m.brock
  Cc: linux-kernel, linux-serial, linux-leds

On 09. 11. 23, 9:50, Florian Eckert wrote:
> 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.
> 
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ---
>   drivers/tty/tty_io.c | 28 ++++++++++++++++++++++------
>   include/linux/tty.h  |  1 +
>   2 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 06414e43e0b5..e2e93404133e 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -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;

Why not simply:
{
  if (tty->ops->tiocmget)
     return tty->ops->tiocmget(tty);

  return -ENOTTY;
}
?

thanks,
-- 
js
suse labs


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

* Re: [Patch v8 1/6] tty: add new helper function tty_get_tiocm
  2023-11-20  7:21   ` Jiri Slaby
@ 2023-11-21  7:35     ` Florian Eckert
  0 siblings, 0 replies; 26+ messages in thread
From: Florian Eckert @ 2023-11-21  7:35 UTC (permalink / raw)
  To: Jiri Slaby
  Cc: Eckert.Florian, gregkh, pavel, lee, kabel, u.kleine-koenig,
	m.brock, linux-kernel, linux-serial, linux-leds



On 2023-11-20 08:21, Jiri Slaby wrote:
> On 09. 11. 23, 9:50, Florian Eckert wrote:
>> 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.
>> 
>> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
>> ---
>>   drivers/tty/tty_io.c | 28 ++++++++++++++++++++++------
>>   include/linux/tty.h  |  1 +
>>   2 files changed, 23 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>> index 06414e43e0b5..e2e93404133e 100644
>> --- a/drivers/tty/tty_io.c
>> +++ b/drivers/tty/tty_io.c
>> @@ -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;
> 
> Why not simply:

I just did it this way because it is also done this way in other 
functions
in this file.

> {
>  if (tty->ops->tiocmget)
>     return tty->ops->tiocmget(tty);
> 
>  return -ENOTTY;
> }

Of course, we could also do it this way. If this is the C style for the 
kernel,
then I will change it. Please give me a short feedback whether I should 
change it
and send a v9, or whether it is just a comment from you.

Best regards

Florian


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

* Re: [Patch v8 6/6] leds: ledtrig-tty: add additional line state evaluation
  2023-11-20  7:21       ` Florian Eckert
@ 2023-11-21 15:23         ` Lee Jones
  2023-11-22  9:50           ` Florian Eckert
  0 siblings, 1 reply; 26+ messages in thread
From: Lee Jones @ 2023-11-21 15:23 UTC (permalink / raw)
  To: Florian Eckert
  Cc: m.brock, Eckert.Florian, gregkh, jirislaby, pavel, kabel,
	u.kleine-koenig, linux-kernel, linux-serial, linux-leds

On Mon, 20 Nov 2023, Florian Eckert wrote:

> 
> 
> On 2023-11-17 13:12, Lee Jones wrote:
> > On Thu, 09 Nov 2023, m.brock@vanmierlo.com wrote:
> > 
> > > Florian Eckert schreef op 2023-11-09 09:50:
> > > > 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'.
> > > >
> > > > Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> > > > ---
> > > >  .../ABI/testing/sysfs-class-led-trigger-tty   | 40 ++++++++++
> > > >  drivers/leds/trigger/ledtrig-tty.c            | 77 ++++++++++++++++++-
> > > >  2 files changed, 116 insertions(+), 1 deletion(-)
> > 
> > [...]
> > 
> > > Reviewed-by: Maarten Brock <m.brock@vanmierlo.com>
> > 
> > Please snip your replies.
> 
> Is there anything I can do? Or do I have to do something? Please give me
> more detailed instructions

That instruction wasn't for you.

This patch is still on my INCOMING list.

Do you have Greg's blessing yet?

-- 
Lee Jones [李琼斯]

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

* Re: [Patch v8 6/6] leds: ledtrig-tty: add additional line state evaluation
  2023-11-21 15:23         ` Lee Jones
@ 2023-11-22  9:50           ` Florian Eckert
  2023-11-22 11:28             ` Lee Jones
  0 siblings, 1 reply; 26+ messages in thread
From: Florian Eckert @ 2023-11-22  9:50 UTC (permalink / raw)
  To: Lee Jones
  Cc: m.brock, Eckert.Florian, gregkh, jirislaby, pavel, kabel,
	u.kleine-koenig, linux-kernel, linux-serial, linux-leds



On 2023-11-21 16:23, Lee Jones wrote:
> On Mon, 20 Nov 2023, Florian Eckert wrote:
> 
>> 
>> 
>> On 2023-11-17 13:12, Lee Jones wrote:
>> > On Thu, 09 Nov 2023, m.brock@vanmierlo.com wrote:
>> >
>> > > Florian Eckert schreef op 2023-11-09 09:50:
>> > > > 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'.
>> > > >
>> > > > Signed-off-by: Florian Eckert <fe@dev.tdt.de>
>> > > > ---
>> > > >  .../ABI/testing/sysfs-class-led-trigger-tty   | 40 ++++++++++
>> > > >  drivers/leds/trigger/ledtrig-tty.c            | 77 ++++++++++++++++++-
>> > > >  2 files changed, 116 insertions(+), 1 deletion(-)
>> >
>> > [...]
>> >
>> > > Reviewed-by: Maarten Brock <m.brock@vanmierlo.com>
>> >
>> > Please snip your replies.
>> 
>> Is there anything I can do? Or do I have to do something? Please give 
>> me
>> more detailed instructions
> 
> That instruction wasn't for you.
> 
> This patch is still on my INCOMING list.
> 
> Do you have Greg's blessing yet?

The summary of my v8:

The changes for the tty layer on v5 in 'drivers/tty/tty_io.c' got an
'Acked-by: Greg Kroah-Hartman' [1].
I have always added his 'Acked-by' to the following patch series.
And I did not made changes to this. So I think I have his blessing
for this changes in his maintained tty layer.

The Memory leak patch I send during v7 [2] got a comment
from Greg, that I have to send this also to 
'linux-kernel@vger-kernel.org'
So this should go into the stable branch [3]. This got an
'Reviewed-by: Uwe Kleine-König' [4]. I add this to v8 [5].
So far I don't know if this has already been merged into
the master and then backported into the stable branches?.

For the changes in the ledtrig-tty driver I am still waiting for an
complete 'ACK' or 'NOK' whether I should change something.
I have added all of Greg's requested changes in v5 [6]:
* split this series
* Add the requested change
* Switch the driver to use completion for 'sysfs'

As I understand it, he handed over the review to the LED subsystem team 
[7].

I then added a few more changes that came from Maarten in v7 [8].
I got his 'Reviewed-by: Maarten Brock' for v8 on patch 6/6.

The patches 4/6 and 5/6 of the v8 still waiting for review?

Best regards

Florian

[1] 
https://lore.kernel.org/linux-leds/2023102327-rename-kosher-bf03@gregkh/#t
[2] 
https://lore.kernel.org/linux-leds/2023110629-scenic-rounding-905f@gregkh/
[3] 
https://lore.kernel.org/linux-leds/20231106141205.3376954-1-fe@dev.tdt.de/
[4] 
https://lore.kernel.org/linux-leds/20231106144914.bflq2jxejdxs6zjb@pengutronix.de/
[5] 
https://lore.kernel.org/linux-leds/20231109085038.371977-1-fe@dev.tdt.de/T/#m1f0c4680749812f1a933667128f73995efe66bca
[6] 
https://lore.kernel.org/linux-leds/2023102341-jogger-matching-dded@gregkh/
[7] 
https://lore.kernel.org/linux-leds/2023102333-skewer-reclining-8d04@gregkh/
[8] 
https://lore.kernel.org/linux-leds/bc94f31e965be6f640c286f8c8a2cf38@vanmierlo.com/
[9] 
https://lore.kernel.org/linux-leds/39e7c892299c74821b1105a0967063ca@vanmierlo.com/

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

* Re: [Patch v8 6/6] leds: ledtrig-tty: add additional line state evaluation
  2023-11-22  9:50           ` Florian Eckert
@ 2023-11-22 11:28             ` Lee Jones
  0 siblings, 0 replies; 26+ messages in thread
From: Lee Jones @ 2023-11-22 11:28 UTC (permalink / raw)
  To: Florian Eckert
  Cc: m.brock, Eckert.Florian, gregkh, jirislaby, pavel, kabel,
	u.kleine-koenig, linux-kernel, linux-serial, linux-leds

On Wed, 22 Nov 2023, Florian Eckert wrote:

> 
> 
> On 2023-11-21 16:23, Lee Jones wrote:
> > On Mon, 20 Nov 2023, Florian Eckert wrote:
> > 
> > > 
> > > 
> > > On 2023-11-17 13:12, Lee Jones wrote:
> > > > On Thu, 09 Nov 2023, m.brock@vanmierlo.com wrote:
> > > >
> > > > > Florian Eckert schreef op 2023-11-09 09:50:
> > > > > > 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'.
> > > > > >
> > > > > > Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> > > > > > ---
> > > > > >  .../ABI/testing/sysfs-class-led-trigger-tty   | 40 ++++++++++
> > > > > >  drivers/leds/trigger/ledtrig-tty.c            | 77 ++++++++++++++++++-
> > > > > >  2 files changed, 116 insertions(+), 1 deletion(-)
> > > >
> > > > [...]
> > > >
> > > > > Reviewed-by: Maarten Brock <m.brock@vanmierlo.com>
> > > >
> > > > Please snip your replies.
> > > 
> > > Is there anything I can do? Or do I have to do something? Please
> > > give me
> > > more detailed instructions
> > 
> > That instruction wasn't for you.
> > 
> > This patch is still on my INCOMING list.
> > 
> > Do you have Greg's blessing yet?
> 
> The summary of my v8:
> 
> The changes for the tty layer on v5 in 'drivers/tty/tty_io.c' got an
> 'Acked-by: Greg Kroah-Hartman' [1].
> I have always added his 'Acked-by' to the following patch series.
> And I did not made changes to this. So I think I have his blessing
> for this changes in his maintained tty layer.
> 
> The Memory leak patch I send during v7 [2] got a comment
> from Greg, that I have to send this also to 'linux-kernel@vger-kernel.org'
> So this should go into the stable branch [3]. This got an
> 'Reviewed-by: Uwe Kleine-König' [4]. I add this to v8 [5].
> So far I don't know if this has already been merged into
> the master and then backported into the stable branches?.
> 
> For the changes in the ledtrig-tty driver I am still waiting for an
> complete 'ACK' or 'NOK' whether I should change something.
> I have added all of Greg's requested changes in v5 [6]:
> * split this series
> * Add the requested change
> * Switch the driver to use completion for 'sysfs'
> 
> As I understand it, he handed over the review to the LED subsystem team [7].
> 
> I then added a few more changes that came from Maarten in v7 [8].
> I got his 'Reviewed-by: Maarten Brock' for v8 on patch 6/6.
> 
> The patches 4/6 and 5/6 of the v8 still waiting for review?

Thanks for the update Florian.

Sounds like you're waiting on me and/or Pavel.

You're in the pile.  I'll get around to you shortly.

> [1]
> https://lore.kernel.org/linux-leds/2023102327-rename-kosher-bf03@gregkh/#t
> [2]
> https://lore.kernel.org/linux-leds/2023110629-scenic-rounding-905f@gregkh/
> [3]
> https://lore.kernel.org/linux-leds/20231106141205.3376954-1-fe@dev.tdt.de/
> [4] https://lore.kernel.org/linux-leds/20231106144914.bflq2jxejdxs6zjb@pengutronix.de/
> [5] https://lore.kernel.org/linux-leds/20231109085038.371977-1-fe@dev.tdt.de/T/#m1f0c4680749812f1a933667128f73995efe66bca
> [6]
> https://lore.kernel.org/linux-leds/2023102341-jogger-matching-dded@gregkh/
> [7]
> https://lore.kernel.org/linux-leds/2023102333-skewer-reclining-8d04@gregkh/
> [8] https://lore.kernel.org/linux-leds/bc94f31e965be6f640c286f8c8a2cf38@vanmierlo.com/
> [9] https://lore.kernel.org/linux-leds/39e7c892299c74821b1105a0967063ca@vanmierlo.com/

-- 
Lee Jones [李琼斯]

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

* Re: [Patch v8 2/6] leds: ledtrig-tty: free allocated ttyname buffer on deactivate
  2023-11-09  8:50 ` [Patch v8 2/6] leds: ledtrig-tty: free allocated ttyname buffer on deactivate Florian Eckert
@ 2023-11-23 14:06   ` Greg KH
  2023-11-27  7:13     ` Florian Eckert
  0 siblings, 1 reply; 26+ messages in thread
From: Greg KH @ 2023-11-23 14:06 UTC (permalink / raw)
  To: Florian Eckert
  Cc: Eckert.Florian, jirislaby, pavel, lee, kabel, u.kleine-koenig,
	m.brock, linux-kernel, linux-serial, linux-leds

On Thu, Nov 09, 2023 at 09:50:34AM +0100, Florian Eckert wrote:
> The ttyname buffer for the ledtrig_tty_data struct is allocated in the
> sysfs ttyname_store() function. This buffer must be released on trigger
> deactivation. This was missing and is thus a memory leak.
> 
> While we are at it, the tty handler in the ledtrig_tty_data struct should
> also be returned in case of the trigger deactivation call.
> 
> Fixes: fd4a641ac88f ("leds: trigger: implement a tty trigger")
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ---
>  drivers/leds/trigger/ledtrig-tty.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
> index 8ae0d2d284af..3e69a7bde928 100644
> --- a/drivers/leds/trigger/ledtrig-tty.c
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -168,6 +168,10 @@ static void ledtrig_tty_deactivate(struct led_classdev *led_cdev)
>  
>  	cancel_delayed_work_sync(&trigger_data->dwork);
>  
> +	kfree(trigger_data->ttyname);
> +	tty_kref_put(trigger_data->tty);
> +	trigger_data->tty = NULL;
> +

This should be a stand-alone patch with a proper cc: stable tag added as
well so that it can be accepted now, as it is independent of this new
feature you are adding.

thanks,

greg k-h

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

* Re: [Patch v8 3/6] leds: ledtrig-tty: change logging if get icount failed
  2023-11-09  8:50 ` [Patch v8 3/6] leds: ledtrig-tty: change logging if get icount failed Florian Eckert
@ 2023-11-23 14:08   ` Greg KH
  0 siblings, 0 replies; 26+ messages in thread
From: Greg KH @ 2023-11-23 14:08 UTC (permalink / raw)
  To: Florian Eckert
  Cc: Eckert.Florian, jirislaby, pavel, lee, kabel, u.kleine-koenig,
	m.brock, linux-kernel, linux-serial, linux-leds

On Thu, Nov 09, 2023 at 09:50:35AM +0100, Florian Eckert wrote:
> Change the log level from info to warn, because there is something
> wrong. That is more a warn message than an info message.
> 
> While we are at it, the device prefix is also changed, as this is the
> led device and not the tty device that generates this message.
> 
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ---
>  drivers/leds/trigger/ledtrig-tty.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
> index 3e69a7bde928..86595add72cd 100644
> --- a/drivers/leds/trigger/ledtrig-tty.c
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -83,6 +83,7 @@ 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;
> +	struct led_classdev *led_cdev = trigger_data->led_cdev;
>  	int ret;
>  
>  	mutex_lock(&trigger_data->mutex);
> @@ -117,7 +118,7 @@ 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");
> +		dev_warn(led_cdev->dev, "Failed to get icount, stop polling\n");

You changed the device that is reporting the error here.  It's the tty
device that you failed to get the icount for, not the LED device, so you
just confused the user by saying that a LED now can not get tty data :)

>  		mutex_unlock(&trigger_data->mutex);
>  		return;
>  	}
> @@ -126,8 +127,7 @@ static void ledtrig_tty_work(struct work_struct *work)
>  	    icount.tx != trigger_data->tx) {
>  		unsigned long interval = LEDTRIG_TTY_INTERVAL;
>  
> -		led_blink_set_oneshot(trigger_data->led_cdev, &interval,
> -				      &interval, 0);
> +		led_blink_set_oneshot(led_cdev, &interval, &interval, 0);

This is fine, but not needed if you fix up the above change.

thanks,

greg k-h

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

* Re: [Patch v8 4/6] leds: ledtrig-tty: replace mutex with completion
  2023-11-09  8:50 ` [Patch v8 4/6] leds: ledtrig-tty: replace mutex with completion Florian Eckert
@ 2023-11-23 14:10   ` Greg KH
  0 siblings, 0 replies; 26+ messages in thread
From: Greg KH @ 2023-11-23 14:10 UTC (permalink / raw)
  To: Florian Eckert
  Cc: Eckert.Florian, jirislaby, pavel, lee, kabel, u.kleine-koenig,
	m.brock, linux-kernel, linux-serial, linux-leds

On Thu, Nov 09, 2023 at 09:50:36AM +0100, Florian Eckert wrote:
> 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.
> 
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ---
>  drivers/leds/trigger/ledtrig-tty.c | 60 +++++++++++++++---------------
>  1 file changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
> index 86595add72cd..3badf74fa420 100644
> --- a/drivers/leds/trigger/ledtrig-tty.c
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -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,24 @@
>  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_waitforcompletion(struct device *dev)

Nit, you might want to add a few more '_' characters, right:
	ledtrig_tty_wait_for_completion()
to match up with the call to wait_for_completion_timeout() it makes.

>  {
> -	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));
> +

Nit, no blank line needed here, if you happen to redo this patch.

thanks,

greg "naming is hard" k-h

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

* Re: [Patch v8 5/6] leds: ledtrig-tty: make rx tx activitate configurable
  2023-11-09  8:50 ` [Patch v8 5/6] leds: ledtrig-tty: make rx tx activitate configurable Florian Eckert
@ 2023-11-23 14:12   ` Greg KH
  0 siblings, 0 replies; 26+ messages in thread
From: Greg KH @ 2023-11-23 14:12 UTC (permalink / raw)
  To: Florian Eckert
  Cc: Eckert.Florian, jirislaby, pavel, lee, kabel, u.kleine-koenig,
	m.brock, linux-kernel, linux-serial, linux-leds

On Thu, Nov 09, 2023 at 09:50:37AM +0100, Florian Eckert wrote:
> 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.
> 
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ---
>  .../ABI/testing/sysfs-class-led-trigger-tty   |  16 +++
>  drivers/leds/trigger/ledtrig-tty.c            | 124 ++++++++++++++++--
>  2 files changed, 130 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> index 2bf6b24e781b..504dece151b8 100644
> --- a/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> @@ -4,3 +4,19 @@ KernelVersion:	5.10
>  Contact:	linux-leds@vger.kernel.org
>  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.
> diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
> index 3badf74fa420..1a40a78bf1ee 100644
> --- a/drivers/leds/trigger/ledtrig-tty.c
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -17,6 +17,19 @@ struct ledtrig_tty_data {
>  	const char *ttyname;
>  	struct tty_struct *tty;
>  	int rx, tx;
> +	bool mode_rx;
> +	bool mode_tx;
> +};
> +
> +/* Indicates which state the LED should now display */
> +enum led_trigger_tty_state {
> +	TTY_LED_BLINK,
> +	TTY_LED_DISABLE,
> +};
> +
> +enum led_trigger_tty_modes {
> +	TRIGGER_TTY_RX = 0,
> +	TRIGGER_TTY_TX,
>  };
>  
>  static int ledtrig_tty_waitforcompletion(struct device *dev)
> @@ -86,12 +99,82 @@ static ssize_t ttyname_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(ttyname);
>  
> +static ssize_t ledtrig_tty_attr_show(struct device *dev, char *buf,
> +				     enum led_trigger_tty_modes attr)
> +{
> +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> +	int completion;
> +	bool state;
> +
> +	reinit_completion(&trigger_data->sysfs);
> +	completion = ledtrig_tty_waitforcompletion(dev);
> +	if (completion < 0)
> +		return completion;

Why do you need to wait for anything just to read the sysfs file?  What
does that sync up with?  And why would it matter?

> +
> +	switch (attr) {
> +	case TRIGGER_TTY_RX:
> +		state = trigger_data->mode_rx;
> +		break;
> +	case TRIGGER_TTY_TX:
> +		state = trigger_data->mode_tx;
> +		break;
> +	}
> +
> +	return sysfs_emit(buf, "%u\n", state);
> +}
> +
> +static ssize_t ledtrig_tty_attr_store(struct device *dev, const char *buf,
> +				      size_t size, enum led_trigger_tty_modes attr)
> +{
> +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> +	int completion;
> +	bool state;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &state);
> +	if (ret)
> +		return ret;
> +
> +	reinit_completion(&trigger_data->sysfs);
> +	completion = ledtrig_tty_waitforcompletion(dev);
> +	if (completion < 0)
> +		return completion;

Same here, why sync anything?

What am I missing as to why a completion is needed?

thanks,

greg k-h

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

* Re: [Patch v8 2/6] leds: ledtrig-tty: free allocated ttyname buffer on deactivate
  2023-11-23 14:06   ` Greg KH
@ 2023-11-27  7:13     ` Florian Eckert
  2023-11-27  8:04       ` Lee Jones
  0 siblings, 1 reply; 26+ messages in thread
From: Florian Eckert @ 2023-11-27  7:13 UTC (permalink / raw)
  To: Greg KH
  Cc: Eckert.Florian, jirislaby, pavel, lee, kabel, u.kleine-koenig,
	m.brock, linux-kernel, linux-serial, linux-leds



On 2023-11-23 15:06, Greg KH wrote:
> On Thu, Nov 09, 2023 at 09:50:34AM +0100, Florian Eckert wrote:
>> The ttyname buffer for the ledtrig_tty_data struct is allocated in the
>> sysfs ttyname_store() function. This buffer must be released on 
>> trigger
>> deactivation. This was missing and is thus a memory leak.
>> 
>> While we are at it, the tty handler in the ledtrig_tty_data struct 
>> should
>> also be returned in case of the trigger deactivation call.
>> 
>> Fixes: fd4a641ac88f ("leds: trigger: implement a tty trigger")
>> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
>> ---
>>  drivers/leds/trigger/ledtrig-tty.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/leds/trigger/ledtrig-tty.c 
>> b/drivers/leds/trigger/ledtrig-tty.c
>> index 8ae0d2d284af..3e69a7bde928 100644
>> --- a/drivers/leds/trigger/ledtrig-tty.c
>> +++ b/drivers/leds/trigger/ledtrig-tty.c
>> @@ -168,6 +168,10 @@ static void ledtrig_tty_deactivate(struct 
>> led_classdev *led_cdev)
>> 
>>  	cancel_delayed_work_sync(&trigger_data->dwork);
>> 
>> +	kfree(trigger_data->ttyname);
>> +	tty_kref_put(trigger_data->tty);
>> +	trigger_data->tty = NULL;
>> +
> 
> This should be a stand-alone patch with a proper cc: stable tag added 
> as
> well so that it can be accepted now, as it is independent of this new
> feature you are adding.

I already send this to stable@vger.kernel.org [1].
The patch already got an 'Reviewed-by:' from Uwe [2].
I hope I did everything right and it only slipped through?

I will omit the patch from the v9 patchset of 'ledtrig-tty'.
This patch set will come later today with your requested changes.

Links;
[1] 
https://lore.kernel.org/stable/20231106141205.3376954-1-fe@dev.tdt.de/
[2] 
https://lore.kernel.org/stable/20231106144914.bflq2jxejdxs6zjb@pengutronix.de/

Best regards

Florian

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

* Re: [Patch v8 2/6] leds: ledtrig-tty: free allocated ttyname buffer on deactivate
  2023-11-27  7:13     ` Florian Eckert
@ 2023-11-27  8:04       ` Lee Jones
  2023-11-27  8:19         ` Florian Eckert
  0 siblings, 1 reply; 26+ messages in thread
From: Lee Jones @ 2023-11-27  8:04 UTC (permalink / raw)
  To: Florian Eckert
  Cc: Greg KH, Eckert.Florian, jirislaby, pavel, kabel, u.kleine-koenig,
	m.brock, linux-kernel, linux-serial, linux-leds

On Mon, 27 Nov 2023, Florian Eckert wrote:

> 
> 
> On 2023-11-23 15:06, Greg KH wrote:
> > On Thu, Nov 09, 2023 at 09:50:34AM +0100, Florian Eckert wrote:
> > > The ttyname buffer for the ledtrig_tty_data struct is allocated in the
> > > sysfs ttyname_store() function. This buffer must be released on
> > > trigger
> > > deactivation. This was missing and is thus a memory leak.
> > > 
> > > While we are at it, the tty handler in the ledtrig_tty_data struct
> > > should
> > > also be returned in case of the trigger deactivation call.
> > > 
> > > Fixes: fd4a641ac88f ("leds: trigger: implement a tty trigger")
> > > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> > > ---
> > >  drivers/leds/trigger/ledtrig-tty.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/leds/trigger/ledtrig-tty.c
> > > b/drivers/leds/trigger/ledtrig-tty.c
> > > index 8ae0d2d284af..3e69a7bde928 100644
> > > --- a/drivers/leds/trigger/ledtrig-tty.c
> > > +++ b/drivers/leds/trigger/ledtrig-tty.c
> > > @@ -168,6 +168,10 @@ static void ledtrig_tty_deactivate(struct
> > > led_classdev *led_cdev)
> > > 
> > >  	cancel_delayed_work_sync(&trigger_data->dwork);
> > > 
> > > +	kfree(trigger_data->ttyname);
> > > +	tty_kref_put(trigger_data->tty);
> > > +	trigger_data->tty = NULL;
> > > +
> > 
> > This should be a stand-alone patch with a proper cc: stable tag added as
> > well so that it can be accepted now, as it is independent of this new
> > feature you are adding.
> 
> I already send this to stable@vger.kernel.org [1].
> The patch already got an 'Reviewed-by:' from Uwe [2].

But then you posted this submission which superseded it in my inbox.

Only the latest patch will be processed when this happens.

> I hope I did everything right and it only slipped through?
> 
> I will omit the patch from the v9 patchset of 'ledtrig-tty'.
> This patch set will come later today with your requested changes.
> 
> Links;
> [1] https://lore.kernel.org/stable/20231106141205.3376954-1-fe@dev.tdt.de/
> [2] https://lore.kernel.org/stable/20231106144914.bflq2jxejdxs6zjb@pengutronix.de/
> 
> Best regards
> 
> Florian

-- 
Lee Jones [李琼斯]

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

* Re: [Patch v8 2/6] leds: ledtrig-tty: free allocated ttyname buffer on deactivate
  2023-11-27  8:04       ` Lee Jones
@ 2023-11-27  8:19         ` Florian Eckert
  0 siblings, 0 replies; 26+ messages in thread
From: Florian Eckert @ 2023-11-27  8:19 UTC (permalink / raw)
  To: Lee Jones
  Cc: Greg KH, Eckert.Florian, jirislaby, pavel, kabel, u.kleine-koenig,
	m.brock, linux-kernel, linux-serial, linux-leds



On 2023-11-27 09:04, Lee Jones wrote:
> On Mon, 27 Nov 2023, Florian Eckert wrote:
> 
>> 
>> 
>> On 2023-11-23 15:06, Greg KH wrote:
>> > On Thu, Nov 09, 2023 at 09:50:34AM +0100, Florian Eckert wrote:
>> > > The ttyname buffer for the ledtrig_tty_data struct is allocated in the
>> > > sysfs ttyname_store() function. This buffer must be released on
>> > > trigger
>> > > deactivation. This was missing and is thus a memory leak.
>> > >
>> > > While we are at it, the tty handler in the ledtrig_tty_data struct
>> > > should
>> > > also be returned in case of the trigger deactivation call.
>> > >
>> > > Fixes: fd4a641ac88f ("leds: trigger: implement a tty trigger")
>> > > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> > > Signed-off-by: Florian Eckert <fe@dev.tdt.de>
>> > > ---
>> > >  drivers/leds/trigger/ledtrig-tty.c | 4 ++++
>> > >  1 file changed, 4 insertions(+)
>> > >
>> > > diff --git a/drivers/leds/trigger/ledtrig-tty.c
>> > > b/drivers/leds/trigger/ledtrig-tty.c
>> > > index 8ae0d2d284af..3e69a7bde928 100644
>> > > --- a/drivers/leds/trigger/ledtrig-tty.c
>> > > +++ b/drivers/leds/trigger/ledtrig-tty.c
>> > > @@ -168,6 +168,10 @@ static void ledtrig_tty_deactivate(struct
>> > > led_classdev *led_cdev)
>> > >
>> > >  	cancel_delayed_work_sync(&trigger_data->dwork);
>> > >
>> > > +	kfree(trigger_data->ttyname);
>> > > +	tty_kref_put(trigger_data->tty);
>> > > +	trigger_data->tty = NULL;
>> > > +
>> >
>> > This should be a stand-alone patch with a proper cc: stable tag added as
>> > well so that it can be accepted now, as it is independent of this new
>> > feature you are adding.
>> 
>> I already send this to stable@vger.kernel.org [1].
>> The patch already got an 'Reviewed-by:' from Uwe [2].
> 
> But then you posted this submission which superseded it in my inbox.
> 
> Only the latest patch will be processed when this happens.

Thanks for the clarification, I wasn't aware of that.

> 
>> I hope I did everything right and it only slipped through?
>> 
>> I will omit the patch from the v9 patchset of 'ledtrig-tty'.
>> This patch set will come later today with your requested changes.
>> 
>> Links;
>> [1] 
>> https://lore.kernel.org/stable/20231106141205.3376954-1-fe@dev.tdt.de/
>> [2] 
>> https://lore.kernel.org/stable/20231106144914.bflq2jxejdxs6zjb@pengutronix.de/
>> 
>> Best regards
>> 
>> Florian

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

* Re: [Patch v8 0/6] ledtrig-tty: add additional tty state evaluation
  2023-11-09  8:50 [Patch v8 0/6] ledtrig-tty: add additional tty state evaluation Florian Eckert
                   ` (5 preceding siblings ...)
  2023-11-09  8:50 ` [Patch v8 6/6] leds: ledtrig-tty: add additional line state evaluation Florian Eckert
@ 2023-12-01 10:40 ` Lee Jones
  2023-12-01 13:08   ` Florian Eckert
  6 siblings, 1 reply; 26+ messages in thread
From: Lee Jones @ 2023-12-01 10:40 UTC (permalink / raw)
  To: Eckert.Florian, gregkh, jirislaby, pavel, lee, kabel,
	u.kleine-koenig, m.brock, Florian Eckert
  Cc: linux-kernel, linux-serial, linux-leds

On Thu, 09 Nov 2023 09:50:32 +0100, Florian Eckert wrote:
> Changes in v8:
> ==============
> - 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].
> 
> [...]

Applied, thanks!

[1/6] tty: add new helper function tty_get_tiocm
      commit: 5d11a4709f552fa139c2439fead05daeb064a6f4
[2/6] leds: ledtrig-tty: free allocated ttyname buffer on deactivate
      (no commit info)
[3/6] leds: ledtrig-tty: change logging if get icount failed
      (no commit info)
[4/6] leds: ledtrig-tty: replace mutex with completion
      (no commit info)
[5/6] leds: ledtrig-tty: make rx tx activitate configurable
      (no commit info)
[6/6] leds: ledtrig-tty: add additional line state evaluation
      (no commit info)

--
Lee Jones [李琼斯]


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

* Re: [Patch v8 0/6] ledtrig-tty: add additional tty state evaluation
  2023-12-01 10:40 ` [Patch v8 0/6] ledtrig-tty: add additional tty " Lee Jones
@ 2023-12-01 13:08   ` Florian Eckert
  2023-12-06 13:47     ` Lee Jones
  0 siblings, 1 reply; 26+ messages in thread
From: Florian Eckert @ 2023-12-01 13:08 UTC (permalink / raw)
  To: Lee Jones
  Cc: Eckert.Florian, gregkh, jirislaby, pavel, kabel, u.kleine-koenig,
	m.brock, linux-kernel, linux-serial, linux-leds



On 2023-12-01 11:40, Lee Jones wrote:
> On Thu, 09 Nov 2023 09:50:32 +0100, Florian Eckert wrote:
>> Changes in v8:
>> ==============
>> - 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].
>> 
>> [...]
> 
> Applied, thanks!
> 
> [1/6] tty: add new helper function tty_get_tiocm
>       commit: 5d11a4709f552fa139c2439fead05daeb064a6f4
> [2/6] leds: ledtrig-tty: free allocated ttyname buffer on deactivate
>       (no commit info)
> [3/6] leds: ledtrig-tty: change logging if get icount failed
>       (no commit info)
> [4/6] leds: ledtrig-tty: replace mutex with completion
>       (no commit info)
> [5/6] leds: ledtrig-tty: make rx tx activitate configurable
>       (no commit info)
> [6/6] leds: ledtrig-tty: add additional line state evaluation
>       (no commit info)

I think that was a mistake? Patchset v9 is the correct patchset [1]?

Thanks for applying v9 [1]

Best regards

Florian

Links:

[1] 
https://lore.kernel.org/all/170142724145.3350831.5316050550655479371.b4-ty@kernel.org/

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

* Re: [Patch v8 0/6] ledtrig-tty: add additional tty state evaluation
  2023-12-01 13:08   ` Florian Eckert
@ 2023-12-06 13:47     ` Lee Jones
  2023-12-06 13:57       ` Florian Eckert
  0 siblings, 1 reply; 26+ messages in thread
From: Lee Jones @ 2023-12-06 13:47 UTC (permalink / raw)
  To: Florian Eckert
  Cc: Eckert.Florian, gregkh, jirislaby, pavel, kabel, u.kleine-koenig,
	m.brock, linux-kernel, linux-serial, linux-leds

On Fri, 01 Dec 2023, Florian Eckert wrote:

> 
> 
> On 2023-12-01 11:40, Lee Jones wrote:
> > On Thu, 09 Nov 2023 09:50:32 +0100, Florian Eckert wrote:
> > > Changes in v8:
> > > ==============
> > > - 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].
> > > 
> > > [...]
> > 
> > Applied, thanks!
> > 
> > [1/6] tty: add new helper function tty_get_tiocm
> >       commit: 5d11a4709f552fa139c2439fead05daeb064a6f4
> > [2/6] leds: ledtrig-tty: free allocated ttyname buffer on deactivate
> >       (no commit info)
> > [3/6] leds: ledtrig-tty: change logging if get icount failed
> >       (no commit info)
> > [4/6] leds: ledtrig-tty: replace mutex with completion
> >       (no commit info)
> > [5/6] leds: ledtrig-tty: make rx tx activitate configurable
> >       (no commit info)
> > [6/6] leds: ledtrig-tty: add additional line state evaluation
> >       (no commit info)
> 
> I think that was a mistake? Patchset v9 is the correct patchset [1]?
> 
> Thanks for applying v9 [1]

It's automated.  Not sure what happened now.

Please check to ensure the correct set was applied.

-- 
Lee Jones [李琼斯]

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

* Re: [Patch v8 0/6] ledtrig-tty: add additional tty state evaluation
  2023-12-06 13:47     ` Lee Jones
@ 2023-12-06 13:57       ` Florian Eckert
  0 siblings, 0 replies; 26+ messages in thread
From: Florian Eckert @ 2023-12-06 13:57 UTC (permalink / raw)
  To: Lee Jones
  Cc: Eckert.Florian, gregkh, jirislaby, pavel, kabel, u.kleine-koenig,
	m.brock, linux-kernel, linux-serial, linux-leds



On 2023-12-06 14:47, Lee Jones wrote:
> On Fri, 01 Dec 2023, Florian Eckert wrote:
>> On 2023-12-01 11:40, Lee Jones wrote:

>> > Applied, thanks!
>> >
>> > [1/6] tty: add new helper function tty_get_tiocm
>> >       commit: 5d11a4709f552fa139c2439fead05daeb064a6f4
>> > [2/6] leds: ledtrig-tty: free allocated ttyname buffer on deactivate
>> >       (no commit info)
>> > [3/6] leds: ledtrig-tty: change logging if get icount failed
>> >       (no commit info)
>> > [4/6] leds: ledtrig-tty: replace mutex with completion
>> >       (no commit info)
>> > [5/6] leds: ledtrig-tty: make rx tx activitate configurable
>> >       (no commit info)
>> > [6/6] leds: ledtrig-tty: add additional line state evaluation
>> >       (no commit info)
>> 
>> I think that was a mistake? Patchset v9 is the correct patchset [1]?
>> 
>> Thanks for applying v9 [1]
> 
> It's automated.  Not sure what happened now.
> 
> Please check to ensure the correct set was applied.

I have checked that v9 is appled to the branch for-leds-next [1].
I can confirm that v9 patchset is applied there.

Thanks

Best regards

Florian

Links:
[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git/log/?h=for-leds-next

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

end of thread, other threads:[~2023-12-06 13:57 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-09  8:50 [Patch v8 0/6] ledtrig-tty: add additional tty state evaluation Florian Eckert
2023-11-09  8:50 ` [Patch v8 1/6] tty: add new helper function tty_get_tiocm Florian Eckert
2023-11-20  7:21   ` Jiri Slaby
2023-11-21  7:35     ` Florian Eckert
2023-11-09  8:50 ` [Patch v8 2/6] leds: ledtrig-tty: free allocated ttyname buffer on deactivate Florian Eckert
2023-11-23 14:06   ` Greg KH
2023-11-27  7:13     ` Florian Eckert
2023-11-27  8:04       ` Lee Jones
2023-11-27  8:19         ` Florian Eckert
2023-11-09  8:50 ` [Patch v8 3/6] leds: ledtrig-tty: change logging if get icount failed Florian Eckert
2023-11-23 14:08   ` Greg KH
2023-11-09  8:50 ` [Patch v8 4/6] leds: ledtrig-tty: replace mutex with completion Florian Eckert
2023-11-23 14:10   ` Greg KH
2023-11-09  8:50 ` [Patch v8 5/6] leds: ledtrig-tty: make rx tx activitate configurable Florian Eckert
2023-11-23 14:12   ` Greg KH
2023-11-09  8:50 ` [Patch v8 6/6] leds: ledtrig-tty: add additional line state evaluation Florian Eckert
2023-11-09 19:03   ` m.brock
2023-11-17 12:12     ` Lee Jones
2023-11-20  7:21       ` Florian Eckert
2023-11-21 15:23         ` Lee Jones
2023-11-22  9:50           ` Florian Eckert
2023-11-22 11:28             ` Lee Jones
2023-12-01 10:40 ` [Patch v8 0/6] ledtrig-tty: add additional tty " Lee Jones
2023-12-01 13:08   ` Florian Eckert
2023-12-06 13:47     ` Lee Jones
2023-12-06 13:57       ` Florian Eckert

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).