linux-kernel - Re: [Patch v7 6/6] leds: ledtrig-tty: add additional line state evaluation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 06 Nov 2023 14:27:04 +0100
From:   m.brock@...mierlo.com
To:     Florian Eckert <fe@....tdt.de>
Cc:     Eckert.Florian@...glemail.com, gregkh@...uxfoundation.org,
        jirislaby@...nel.org, pavel@....cz, lee@...nel.org,
        kabel@...nel.org, u.kleine-koenig@...gutronix.de,
        linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
        linux-leds@...r.kernel.org
Subject: Re: [Patch v7 6/6] leds: ledtrig-tty: add additional line state
 evaluation

Florian Eckert wrote on 2023-11-06 13:34:
> Add 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).
> 
> In order to also evaluate the LED 'state' form the previous round, so 
> we
> could blink in the correct order, the 'state' must be saved in the 
> trigger
> data struct.
> 
> Signed-off-by: Florian Eckert <fe@....tdt.de>
> ---
> diff --git a/drivers/leds/trigger/ledtrig-tty.c
> b/drivers/leds/trigger/ledtrig-tty.c
> index 1a40a78bf1ee..107fbbca96de 100644
> --- a/drivers/leds/trigger/ledtrig-tty.c
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -17,19 +17,29 @@ struct ledtrig_tty_data {
>  	const char *ttyname;
>  	struct tty_struct *tty;
>  	int rx, tx;
> +	int state;

I don't think you need to remember the state here.

>  	bool mode_rx;
>  	bool mode_tx;
> +	bool mode_cts;
> +	bool mode_dsr;
> +	bool mode_dcd;
> +	bool mode_rng;
>  };
> @@ -167,16 +201,27 @@ 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)
>  {
>  	struct ledtrig_tty_data *trigger_data =
>  		container_of(work, struct ledtrig_tty_data, dwork.work);
>  	struct led_classdev *led_cdev = trigger_data->led_cdev;
> -	enum led_trigger_tty_state state = TTY_LED_DISABLE;

Keep this one.

>  	unsigned long interval = LEDTRIG_TTY_INTERVAL;
> +	int invert = 0;

bool invert = false;

> +	int status;
>  	int ret;
> 
> +	if (trigger_data->state == TTY_LED_ENABLE)
> +		invert = 1;

Drop the above.

> +
> +	/* Always disable the LED if no evaluation could be done */
> +	trigger_data->state = TTY_LED_DISABLE;
> +
>  	if (!trigger_data->ttyname)
>  		goto out;
> 
> @@ -202,6 +247,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)
> +				trigger_data->state = TTY_LED_ENABLE;
> +		}
> +
> +		if (trigger_data->mode_dsr) {
> +			if (status & TIOCM_DSR)
> +				trigger_data->state = TTY_LED_ENABLE;
> +		}
> +
> +		if (trigger_data->mode_dcd) {
> +			if (status & TIOCM_CAR)
> +				trigger_data->state = TTY_LED_ENABLE;
> +		}
> +
> +		if (trigger_data->mode_rng) {
> +			if (status & TIOCM_RNG)
> +				trigger_data->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,19 +283,22 @@ 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;

Keep this line.

> +			trigger_data->state = TTY_LED_BLINK;

And drop this one.

>  		}
> 
>  		if (trigger_data->mode_rx && (icount.rx != trigger_data->rx)) {
>  			trigger_data->rx = icount.rx;

invert = state == TTY_LED_ENABLE;

> -			state = TTY_LED_BLINK;
> +			trigger_data->state = TTY_LED_BLINK;
>  		}
>  	}
> 
>  out:
> -	switch (state) {
> +	switch (trigger_data->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;

Maarten