Date:   Mon, 6 Mar 2023 10:04:00 +0000
From:   Lee Jones <lee@kernel.org>
To:     Jiri Slaby <jirislaby@kernel.org>
Cc:     Florian Eckert <fe@dev.tdt.de>, u.kleine-koenig@pengutronix.de,
        gregkh@linuxfoundation.org, pavel@ucw.cz,
        linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org,
        Eckert.Florian@googlemail.com
Subject: Re: [PATCH v7 2/2] trigger: ledtrig-tty: add additional modes
Message-ID: <20230306100400.GF9667@google.com>
References: <20230222083335.847655-1-fe@dev.tdt.de>
 <20230222083335.847655-3-fe@dev.tdt.de>
 <20230303141139.GP2420672@google.com>
 <be7c90cf-4c65-1cf0-3001-8706415c3d34@kernel.org>
 <20230306090456.GA9667@google.com>
 <1faade8f-d5e6-fd60-bd60-22e3b79c5ba4@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1faade8f-d5e6-fd60-bd60-22e3b79c5ba4@kernel.org>
Precedence: bulk

On Mon, 06 Mar 2023, Jiri Slaby wrote:

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

Works for me.  Thanks for the clear explanation, Jiri and Uwe.

And yes Uwe, it would be good if we could make that clearer in the doc.

-- 
Lee Jones [李琼斯]