| 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
| ||
|
Message-ID: <2951fd563fc6a364d8cddfb7ec17808b@dev.tdt.de>
Date: Mon, 30 Oct 2023 09:15:28 +0100
From: Florian Eckert <fe@....tdt.de>
To: m.brock@...mierlo.com
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 v5 2/2] leds: ledtrig-tty: add new line mode evaluation
On 2023-10-28 12:43, m.brock@...mierlo.com wrote:
> Florian Eckert wrote on 2023-10-23 11:42:
>
>> @@ -16,6 +16,28 @@ struct ledtrig_tty_data {
>> const char *ttyname;
>> struct tty_struct *tty;
>> int rx, tx;
>> + unsigned long ttytrigger;
>> +};
>
> ttytriggers ?
Yes that would be nicer name. thanks
> [...]
>
>> 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;
>> + unsigned long interval = LEDTRIG_TTY_INTERVAL;
>> struct serial_icounter_struct icount;
>> + enum led_trigger_tty_state state;
>> + int current_brightness;
>> + int status;
>> int ret;
>>
>> + state = TTY_LED_DISABLE;
>> mutex_lock(&trigger_data->mutex);
>>
>> if (!trigger_data->ttyname) {
>> @@ -115,22 +218,74 @@ static void ledtrig_tty_work(struct work_struct
>> *work)
>> trigger_data->tty = tty;
>> }
>>
>> - ret = tty_get_icount(trigger_data->tty, &icount);
>> - if (ret) {
>> - dev_info(trigger_data->tty->dev, "Failed to get icount, stopped
>> polling\n");
>> - mutex_unlock(&trigger_data->mutex);
>> - return;
>> + status = tty_get_tiocm(trigger_data->tty);
>> + if (status > 0) {
>> + if (test_bit(TRIGGER_TTY_CTS, &trigger_data->ttytrigger)) {
>> + if (status & TIOCM_CTS)
>> + state = TTY_LED_ENABLE;
>> + }
>> +
>> + if (test_bit(TRIGGER_TTY_DSR, &trigger_data->ttytrigger)) {
>> + if (status & TIOCM_DSR)
>> + state = TTY_LED_ENABLE;
>> + }
>> +
>> + if (test_bit(TRIGGER_TTY_CAR, &trigger_data->ttytrigger)) {
>> + if (status & TIOCM_CAR)
>> + state = TTY_LED_ENABLE;
>> + }
>> +
>> + if (test_bit(TRIGGER_TTY_RNG, &trigger_data->ttytrigger)) {
>> + if (status & TIOCM_RNG)
>> + state = TTY_LED_ENABLE;
>> + }
>> + }
>> +
>> + /* The rx/tx handling must come after the evaluation of TIOCM_*,
>> + * since the display for rx/tx has priority
>> + */
>> + if (test_bit(TRIGGER_TTY_RX, &trigger_data->ttytrigger) ||
>> + test_bit(TRIGGER_TTY_TX, &trigger_data->ttytrigger)) {
>> + ret = tty_get_icount(trigger_data->tty, &icount);
>> + if (ret) {
>> + dev_info(trigger_data->tty->dev, "Failed to get icount, stopped
>> polling\n");
>> + mutex_unlock(&trigger_data->mutex);
>> + return;
>> + }
>> +
>> + if (test_bit(TRIGGER_TTY_RX, &trigger_data->ttytrigger) &&
>> + (icount.tx != trigger_data->tx)) {
>
> You check for TRIGGER_TTY_RX and then compare icount.tx, is that
> correct?
I would say this is correct. At first I check if the tx path should be
evaluated
and if this is correct I check if there was a tx transmission during the
last run.
>> + trigger_data->tx = icount.tx;
>> + state = TTY_LED_BLINK;
>> + }
>> +
>> + if (test_bit(TRIGGER_TTY_TX, &trigger_data->ttytrigger) &&
>> + (icount.rx != trigger_data->rx)) {
>
> You check for TRIGGER_TTY_TX and then compare icount.rx, is that
> correct?
I would say this is correct. At first I check if the rx path should be
evaluated
and if this is correct I check if there was a rx transmission during the
last run.
>> + trigger_data->rx = icount.rx;
>> + state = TTY_LED_BLINK;
>> + }
>> }
>>
>> - if (icount.rx != trigger_data->rx ||
>> - icount.tx != trigger_data->tx) {
>> - unsigned long interval = LEDTRIG_TTY_INTERVAL;
>> + current_brightness = led_cdev->brightness;
>> + if (current_brightness)
>> + led_cdev->blink_brightness = current_brightness;
>>
>> + if (!led_cdev->blink_brightness)
>> + led_cdev->blink_brightness = led_cdev->max_brightness;
>
> Is it OK to override the chosen brightness here?
In my setup my brightness in the sysfs path of the LED ist set to '0'.
Even though the tty trigger was configured correctly the LED was not
turned on. If I set max_brightness in this path the LED works correctly.
I would check this a gain if this is still needed.
>> +
>> + switch (state) {
>> + case TTY_LED_BLINK:
>> led_blink_set_oneshot(trigger_data->led_cdev, &interval,
>> &interval, 0);
>
> Change trigger_data->led_cdev to simply led_cdev
Thanks for the hint. I will change this.
> Shouldn't the led return to the line controlled steady state?
Sorry I do not understand your question.
> Set an invert variable to true if state was TTY_LED_ENABLE before it
> got set
> to TTY_LED_BLINK
No matter whether the LED is on or off beforehand. I understand that the
LED is always on for the first half of the period and off for the rest
of
the period. I think that is correct and I don't need to make a
distinction
via invert here. I hope I have understood your comment correctly here.
> How do interval and the frequency of ledtrig_tty_work() relate?
The work is twice as long as of the interval. So the variable
LEDTRIG_TTY_INTERVAL = 50 and the work is scheduled LEDTRIG_TTY_INTERVAL
* 2.
But that was also before my change.
>> -
>> - trigger_data->rx = icount.rx;
>> - trigger_data->tx = icount.tx;
>> + break;
>> + case TTY_LED_ENABLE:
>> + led_set_brightness(led_cdev, led_cdev->blink_brightness);
>> + break;
>> + case TTY_LED_DISABLE:
>> + fallthrough;
>> + default:
>> + led_set_brightness(led_cdev, LED_OFF);
>> + break;
>> }
>
> Maarten
Thank you for your feedback. I must say, however, that I am currently in
the process of preparing v6, which will implement the comments and
change requests from 'greg k-h' [1]. The big change here in v6 is, that
I have
switched to completion and split the change in more reviewable commits.
I will see if your comments can also be incorporated into the new
approach.
---
Florian
[1]
https://lore.kernel.org/linux-leds/2023102341-jogger-matching-dded@gregkh/
Powered by blists - more mailing lists