linux-kernel - Re: [PATCH v7 2/2] trigger: ledtrig-tty: add additional modes

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]

Message-ID: <1faade8f-d5e6-fd60-bd60-22e3b79c5ba4@kernel.org>
Date:   Mon, 6 Mar 2023 10:35:08 +0100
From:   Jiri Slaby <jirislaby@...nel.org>
To:     Lee Jones <lee@...nel.org>
Cc:     Florian Eckert <fe@....tdt.de>, u.kleine-koenig@...gutronix.de,
        gregkh@...uxfoundation.org, pavel@....cz,
        linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org,
        Eckert.Florian@...glemail.com
Subject: Re: [PATCH v7 2/2] trigger: ledtrig-tty: add additional modes

On 06. 03. 23, 10:04, Lee Jones wrote:
> On Mon, 06 Mar 2023, Jiri Slaby wrote:
> 
>> On 03. 03. 23, 15:11, Lee Jones wrote:
>>> On Wed, 22 Feb 2023, Florian Eckert wrote:
>>>> @@ -113,21 +207,38 @@ 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;
>>>> -	}
>>>> -
>>>> -	if (icount.rx != trigger_data->rx ||
>>>> -	    icount.tx != trigger_data->tx) {
>>>> -		led_set_brightness_sync(trigger_data->led_cdev, LED_ON);
>>>> -
>>>> -		trigger_data->rx = icount.rx;
>>>> -		trigger_data->tx = icount.tx;
>>>> -	} else {
>>>> -		led_set_brightness_sync(trigger_data->led_cdev, LED_OFF);
>>>> +	switch (trigger_data->mode) {
>>>> +	case TTY_LED_CTS:
>>>> +		ledtrig_tty_flags(trigger_data, TIOCM_CTS);
>>>> +		break;
>>>> +	case TTY_LED_DSR:
>>>> +		ledtrig_tty_flags(trigger_data, TIOCM_DSR);
>>>> +		break;
>>>> +	case TTY_LED_CAR:
>>>> +		ledtrig_tty_flags(trigger_data, TIOCM_CAR);
>>>> +		break;
>>>> +	case TTY_LED_RNG:
>>>> +		ledtrig_tty_flags(trigger_data, TIOCM_RNG);
>>>> +		break;
>>>> +	case TTY_LED_CNT:
>>>
>>> I believe this requires a 'fall-through' statement.
>>
>> I don't think this is the case. Isn't fallthrough required only in cases
>> when there is at least one statement, i.e. a block?
> 
> There's no mention of this caveat in the document.
> 
> To my untrained eyes, the rule looks fairly explicit, starting with "All".
> 
> "
>    All switch/case blocks must end in one of:
> 
>    * break;
>    * fallthrough;
>    * continue;
>    * goto <label>;
>    * return [expression];
> "
> 
> If you're aware of something I'm not, please consider updating the doc.

The magic word in the above is "block", IMO. A block is defined for me 
as a list of declarations and/or statements. Which is not the case in 
the above (i.e. in sequential "case"s).

Furthermore, the gcc docs specifically say about fallthrough attribute:
It can only be used in a switch statement (the compiler will issue an 
error otherwise), after a preceding statement and before a logically 
succeeding case label, or user-defined label.

While "case X:" is technically a (label) statement, I don't think they 
were thinking of it as such here due to following "succeeding case 
label" in the text.

So checking with the code, gcc indeed skips those 
(should_warn_for_implicit_fallthrough()):
   /* Skip all immediately following labels.  */
   while (!gsi_end_p (gsi)
          && (gimple_code (gsi_stmt (gsi)) == GIMPLE_LABEL
              || gimple_code (gsi_stmt (gsi)) == GIMPLE_PREDICT))
     gsi_next_nondebug (&gsi);


Apart from that, fallthrough only makes the code harder to read:

case X:
case Y:
case Z:
default:
   do_something();

VS

case X:
   fallthrough;
case Y:
   fallthrough;
case Z:
   fallthrough;
default:
   do_something();

The first one is a clear win, IMO, and it's pretty clear that it falls 
through on purpose. And even for compiler -- it shall not produce a 
warning in that case.

regards,
-- 
js
suse labs