Messages in this thread | | | Date | Mon, 6 Mar 2023 10:35:08 +0100 | From | Jiri Slaby <> | 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
| |