ledtrig-tty: add new state evaluation

PATCH Superseded Florian Eckert 3 participants 6 messages Sep 26, 2023

Subsystems (2)

TTY layer and serial drivers LED

lore

Share & tools

lore

Florian Eckert

author

2023-09-26 09:56 UTC #0 lore raw

cover ledtrig-tty: add new state evaluation

This is a follow-up patchset, based on the mailing list discussion from March 2023 based on the old patchset v7 [1]. I have changed, the LED trigger handling via the sysfs interfaces as suggested by Uwe Kleine-König.

[1] https://lore.kernel.org/linux-leds/[email protected]/

Florian Eckert (2):
tty: add new helper function tty_get_mget
trigger: ledtrig-tty: add new line mode to triggers

 .../ABI/testing/sysfs-class-led-trigger-tty   |  54 ++++
 drivers/leds/trigger/ledtrig-tty.c            | 272 +++++++++++++++++-
 drivers/tty/tty_io.c                          |  29 +-
 include/linux/tty.h                           |   1 +
 4 files changed, 339 insertions(+), 17 deletions(-)

Signature · 1 line

2.30.2

Florian Eckert

author

2023-09-26 09:59 UTC #1 lore raw

PATCH 1/2 tty: add new helper function tty_get_mget

The struct 'tty_struct' has a callback to read the status flags of the tty if the tty driver provides them. So fare, the data is transferred directly to userspace with the function 'tty_tiocmget'. This function cannot be used to evaluate the status line of the tty interface in the ledtrig-tty trigger. To make this possible, a new function must be added that does not immediately pass the data on to userspace.

The new function 'tty_get_mget' only returns the status register. This information can then be processed further in the ledtrig-tty trigger.

Signed-off-by Florian Eckert <[email protected]>

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

drivers/tty/tty_io.c +23 -6

index 8a94e5a43c6d..8070ed0ce41f 100644
@@ -2494,6 +2494,25 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
 	return retval;
 }
 
+/**
+ * tty_get_mget		-	get modem status
+ * @tty: tty device
+ *
+ * Obtain the modem status bits from the tty driver if the feature
+ * is supported.
+ *
+ */
+int tty_get_mget(struct tty_struct *tty)
+{
+	int retval = -ENOTTY;
+
+	if (tty->ops->tiocmget)
+		retval = tty->ops->tiocmget(tty);
+
+	return retval;
+}
+EXPORT_SYMBOL_GPL(tty_get_mget);
+
 /**
  * tty_tiocmget		-	get modem status
  * @tty: tty device

@@ -2506,14 +2525,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_mget(tty);
+	if (retval >= 0)
+		retval = put_user(retval, p);
 
-		if (retval >= 0)
-			retval = put_user(retval, p);
-	}
 	return retval;
 }

include/linux/tty.h +1 -0

index f002d0f25db7..7b9edd4a7007 100644
@@ -421,6 +421,7 @@ int 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_mget(struct tty_struct *tty);
 int is_current_pgrp_orphaned(void);
 void tty_hangup(struct tty_struct *tty);
 void tty_vhangup(struct tty_struct *tty);

Signature · 1 line

2.30.2

Jiri Slaby

maintainer

2023-09-26 09:48 UTC #2 lore raw

Re: [PATCH 1/2] tty: add new helper function tty_get_mget

Quoted 22 lines · Florian Eckert

The struct 'tty_struct' has a callback to read the status flags of the tty if the tty driver provides them. So fare, the data is transferred directly to userspace with the function 'tty_tiocmget'. This function cannot be used to evaluate the status line of the tty interface in the ledtrig-tty trigger. To make this possible, a new function must be added that does not immediately pass the data on to userspace.
The new function 'tty_get_mget' only returns the status register. This information can then be processed further in the ledtrig-tty trigger.
Signed-off-by Florian Eckert <[email protected]>
---
  drivers/tty/tty_io.c | 29 +++++++++++++++++++++++------
  include/linux/tty.h  |  1 +
  2 files changed, 24 insertions(+), 6 deletions(-)
drivers/tty/tty_io.c +0 -0
index 8a94e5a43c6d..8070ed0ce41f 100644

drivers/tty/tty_io.c static int send_break(struct tty_struct *tty, unsigned int duration) · line 2498

Quoted 5 lines

drivers/tty/tty_io.c +2 -0

@@ -2494,6 +2494,25 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
  	return retval;
  }
 
+/**
+ * tty_get_mget		-	get modem status

Heh, the naming is funny. It apparently comes from tiocmget. But that comes from:
tty ioctl modem get (TIOCMGET)
tty ioctl modem set (TIOCMSET)

So you should name it like tty_get_modem() not get_mget().

Also those extra spaces around "-" caused some issues in the generated output and should be removed (everywhere).

drivers/tty/tty_io.c static int send_break(struct tty_struct *tty, unsigned int duration) · line 2503

Quoted 5 lines

drivers/tty/tty_io.c +5 -0

+ * @tty: tty device
+ *
+ * Obtain the modem status bits from the tty driver if the feature
+ * is supported.
+ *

Superfluous empty line here.

drivers/tty/tty_io.c static int send_break(struct tty_struct *tty, unsigned int duration) · line 2514

Quoted 11 lines

drivers/tty/tty_io.c +11 -0

+ */
+int tty_get_mget(struct tty_struct *tty)
+{
+	int retval = -ENOTTY;
+
+	if (tty->ops->tiocmget)
+		retval = tty->ops->tiocmget(tty);
+
+	return retval;
+}
+EXPORT_SYMBOL_GPL(tty_get_mget);

thanks,

Signature · 2 lines

js
suse labs

Florian Eckert

author

2023-09-26 12:04 UTC #3 lore raw

Re: [PATCH 1/2] tty: add new helper function tty_get_mget

Thanks for your fast respond!

Quoted 12 lines

drivers/tty/tty_io.c static int send_break(struct tty_struct *tty, · line 2498
Quoted 6 lines
drivers/tty/tty_io.c +1 -0
@@ -2494,6 +2494,25 @@ static int send_break(struct tty_struct *tty, 
 unsigned int duration)
  	return retval;
  }
  +/**
+ * tty_get_mget		-	get modem status
Heh, the naming is funny. It apparently comes from tiocmget. But that comes from:
tty ioctl modem get (TIOCMGET)
tty ioctl modem set (TIOCMSET)

Quoted 1 line

So you should name it like tty_get_modem() not get_mget().

I didn't like the name too, but I couldn't think of another. The function is returning the state of serial control and status signals.

From your suggestion for the name, however, you can not deduce that at all. How would it be then with the following name?

tty_tioctl_state() ?

Quoted 3 lines

Also those extra spaces around "-" caused some issues in the generated output and should be removed (everywhere).

Ok, I will change this in an own commit throughout the file.

Thanks

Florian

@jirislaby: Forgot to send this message to the mailing list as well!

Florian Eckert

author

2023-09-26 09:55 UTC #4 lore raw

PATCH 2/2 trigger: ledtrig-tty: add new line mode to triggers

In reply to Florian Eckert

Until now, the LED blinks when data is sent via the tty (rx/tx). 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.

- line_cts:
DCE is ready to accept data from the DTE (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.

- line_dsr:
DCE is ready to receive and send data (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.

- line_car:
DTE is receiving a carrier from the DCE (Data Carrier Detect). 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).

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

In addition to the new line_* entries in sysfs, the indication for the direction of the transmitted data is independently controllable via the new rx and tx sysfs entrie now too. These are on by default. Thus the trigger behaves as before this change.

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

Signed-off-by Florian Eckert <[email protected]>

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

Documentation/ABI/testing/sysfs-class-led-trigger-tty +54 -0

index 2bf6b24e781b..14c38a2370ea 100644
@@ -4,3 +4,57 @@ KernelVersion:	5.10
 Contact:	[email protected]
 Description:
 		Specifies the tty device name of the triggering tty
+
+What:		/sys/class/leds/<led>/rx
+Date:		September 2023
+KernelVersion:	6.7
+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:		September 2023
+KernelVersion:	6.7
+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.
+
+car rng
+What:		/sys/class/leds/<led>/line_cts
+Date:		September 2023
+KernelVersion:	6.7
+Description:
+		DCE is ready to accept data from the DTE (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.
+
+What:		/sys/class/leds/<led>/line_dsr
+Date:		September 2023
+KernelVersion:	6.7
+Description:
+		DCE is ready to receive and send data (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.
+
+What:		/sys/class/leds/<led>/line_car
+Date:		September 2023
+KernelVersion:	6.7
+Description:
+		DTE is receiving a carrier from the DCE (Data Carrier Detect).
+		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>/line_cts
+Date:		September 2023
+KernelVersion:	6.7
+Description:
+		DCE has detected an incoming ring signal on the telephone
+		line (Ring Indicator). If the line state is detected, the
+		LED is switched on.
+		If set to 0 (default), the LED will not evaluate RNG (RI).
+		If set to 1, the LED will evaluate RNG (RI).

drivers/leds/trigger/ledtrig-tty.c +261 ~~-11~~

314 lines not on this page · raw message

Signature · 1 line

2.30.2

kernel test robot

build bot

2023-09-26 20:34 UTC #5 lore raw

Re: [PATCH 2/2] trigger: ledtrig-tty: add new line mode to triggers

kernel test robot noticed the following build errors:

Hi Florian,

kernel test robot noticed the following build errors:

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on tty/tty-next tty/tty-linus staging/staging-testing staging/staging-next staging/staging-linus linus/master v6.6-rc3 next-20230926] [cannot apply to pavel-leds/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Florian-Eckert/tty-add-new-helper-function-tty_get_mget/20230926-180154
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link:    https://lore.kernel.org/r/20230926093607.59536-3-fe%40dev.tdt.de
patch subject: [PATCH 2/2] trigger: ledtrig-tty: add new line mode to triggers
config: x86_64-randconfig-161-20230927 (https://download.01.org/0day-ci/archive/20230927/[email protected]/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230927/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

drivers/leds/trigger/ledtrig-tty.c: In function 'ledtrig_tty_work':

>> drivers/leds/trigger/ledtrig-tty.c:362:3: error: a label can only be part of a statement and a declaration is not a statement
      unsigned long interval = LEDTRIG_TTY_INTERVAL;
      ^~~~~~~~

vim +362 drivers/leds/trigger/ledtrig-tty.c

270
271 static void ledtrig_tty_work(struct work_struct *work)
272 {
273 struct ledtrig_tty_data *trigger_data =
274 container_of(work, struct ledtrig_tty_data, dwork.work);
275 struct led_classdev *led_cdev = trigger_data->led_cdev;
276 struct serial_icounter_struct icount;
277 enum tty_led_state state;
278 int ret;
279
280 state = TTY_LED_DISABLE;
281 mutex_lock(&trigger_data->mutex);
282
283 if (!trigger_data->ttyname) {
284 /* exit without rescheduling */
285 mutex_unlock(&trigger_data->mutex);
286 return;
287 }
288
289 /* try to get the tty corresponding to $ttyname */
290 if (!trigger_data->tty) {
291 dev_t devno;
292 struct tty_struct *tty;
293 int ret;
294
295 ret = tty_dev_name_to_number(trigger_data->ttyname, &devno);
296 if (ret < 0)
297 /*
298 * A device with this name might appear later, so keep
299 * retrying.
300 */
301 goto out;
302
303 tty = tty_kopen_shared(devno);
304 if (IS_ERR(tty) || !tty)
305 /* What to do? retry or abort */
306 goto out;
307
308 trigger_data->tty = tty;
309 }
310
311 if (test_bit(LEDTRIG_TTY_MODE_CTS, &trigger_data->mode)) {
312 ret = ledtrig_tty_flag(trigger_data, TIOCM_CTS);
313 if (ret)
314 state = TTY_LED_ENABLE;
315 }
316
317 if (test_bit(LEDTRIG_TTY_MODE_DSR, &trigger_data->mode)) {
318 ret = ledtrig_tty_flag(trigger_data, TIOCM_DSR);
319 if (ret)
320 state = TTY_LED_ENABLE;
321 }
322
323 if (test_bit(LEDTRIG_TTY_MODE_CAR, &trigger_data->mode)) {
324 ret = ledtrig_tty_flag(trigger_data, TIOCM_CAR);
325 if (ret)
326 state = TTY_LED_ENABLE;
327 }
328
329 if (test_bit(LEDTRIG_TTY_MODE_RNG, &trigger_data->mode)) {
330 ret = ledtrig_tty_flag(trigger_data, TIOCM_RNG);
331 if (ret)
332 state = TTY_LED_ENABLE;
333 }
334
335 /* The rx/tx handling must come after the evaluation of TIOCM_*,
336 * since the display for rx/tx has priority
337 */
338 if (test_bit(LEDTRIG_TTY_MODE_TX, &trigger_data->mode) ||
339 test_bit(LEDTRIG_TTY_MODE_RX, &trigger_data->mode)) {
340 ret = tty_get_icount(trigger_data->tty, &icount);
341 if (ret) {
342 dev_info(trigger_data->tty->dev, "Failed to get icount, stopped polling\n");
343 mutex_unlock(&trigger_data->mutex);
344 return;
345 }
346
347 if (test_bit(LEDTRIG_TTY_MODE_TX, &trigger_data->mode) &&
348 (icount.tx != trigger_data->tx)) {
349 trigger_data->tx = icount.tx;
350 state = TTY_LED_BLINK;
351 }
352
353 if (test_bit(LEDTRIG_TTY_MODE_RX, &trigger_data->mode) &&
354 (icount.rx != trigger_data->rx)) {
355 trigger_data->rx = icount.rx;
356 state = TTY_LED_BLINK;
357 }
358 }
359
360 switch (state) {
361 case TTY_LED_BLINK:

 > 362			unsigned long interval = LEDTRIG_TTY_INTERVAL;

363 led_blink_set_oneshot(trigger_data->led_cdev, &interval,
364 &interval, 0);
365 break;
366 case TTY_LED_ENABLE:
367 led_set_brightness(led_cdev, led_cdev->blink_brightness);
368 break;
369 case TTY_LED_DISABLE:
370 fallthrough;
371 default:
372 led_set_brightness(led_cdev, 0);
373 break;
374 }
375
376 out:
377 mutex_unlock(&trigger_data->mutex);
378 schedule_delayed_work(&trigger_data->dwork,
379 msecs_to_jiffies(LEDTRIG_TTY_INTERVAL * 2));
380 }
381

Signature · 2 lines

0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki