linux-kernel - Re: [PATCH v5 2/2] leds: ledtrig-tty: add new line mode 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] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 04 Nov 2023 14:59:17 +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 v5 2/2] leds: ledtrig-tty: add new line mode evaluation

Florian Eckert wrote on 2023-10-30 09:15:
>>> +	/* 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.

No, you check if the *RX* path should be evaluated! On the bright side: 
this is
fixed in the new patch set.

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

Same difference.

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

I see you've dropped this from the new patch set. Thank you.

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

This explains why you don't necessarily need to invert the blink.
If E.g. both CTS and TX are configured I would expect to see the led 
turn on
once CTS actives and then blink off when something is transmitted. After 
that
I expect to see the led still on because CTS is still active.

Now only because the work interval is 2*LEDTRIG_TTY_INTERVAL and the 
blink
uses an interval of LEDTRIG_TTY_INTERVAL for both on and off the user 
doesn't
notice any difference except maybe a bit of delay of the blink.

If either the work schedule was larger than 2*LEDTRIG_TTY_INTERVAL or 
the on
interval would differ from the off interval the behaviour would differ
noticably.

This is why I recommend to use an invert variable that is set to true 
when
the previous state was TTY_LED_ENABLE.

Maarten