On Tue, May 22, 2018 at 12:29:13PM +0200, Uwe Kleine-König wrote:
> Hello Johan,
>
> On Tue, May 22, 2018 at 11:51:41AM +0200, Johan Hovold wrote:
> > On Thu, May 17, 2018 at 08:16:25AM +0200, Uwe Kleine-König wrote:
> > > On Wed, May 16, 2018 at 09:54:54AM +0200, Johan Hovold wrote:
> > > > On Mon, May 14, 2018 at 12:15:05PM +0200, Uwe Kleine-König wrote:
> > > > > The rx trigger fires when data is pushed to the ldisc by the driver. This
> > > > > is a bit later than the actual receiving of data but has the nice benefit
> > > > > that it doesn't need adaption for each driver and isn't in the hot path.
> > > > >
> > > > > Similarly the tx trigger fires when data was copied from userspace and is
> > > > > given to the ldisc.
> > > >
> > > > This also has the negative effect of having the blink rate (and
> > > > pattern) be dependent on the size of the buffers provided by userspace
> > > > however.
> > > >
> > > > With buffers of 2048 bytes or more and at 115200 baud you get a 178 ms
> > > > period (rather than the "default" 100 ms used for smaller buffers and
> > > > high-enough baudrates) and this drops to 2 seconds and 68 second periods
> > > > for 9600 and 300 baud respectively.
> > >
> > > that is something I wouldn't care about. I think nobody will want to
> > > determine the transfer rate from the emitted light.
> >
> > Perhaps not, but it's an unfortunate effect of the choice of moving this
> > into the tty layer instead of having the lower level drivers implement
> > this, which I at least think you need to mention.
>
> the observability is the same, independent of where the trigger fires,
> isn't it?
Depends on how you implement it; you could start blinking when you start
tx and stop it when the write buffer is empty (or tx halted) instead of
using a one-shot trigger (with its implied variable frequencies).
> > Personally I don't understand this obsession with blinking lights at
> > all, but if this is to be supported we should at least try to do it the
> > right way (after determining what that is).
> >
> > > > Also note that by hooking into do_tty_write, for UARTs used for consoles
> > > > there will be no blink on console output, only for console input...
> > >
> > > I tested the patch with the trigger for the console device. I didn't
> > > test if kernel messages trigger the trigger, but the shell running on
> > > the console device does (e.g. when running seq 1 10000).
> >
> > You're right, a getty would open the tty device directly so then you'd
> > get blinks in both directions in a login shell.
> >
> > But printk messages won't trigger the led as consoles use a different
> > write path.
>
> I'm willing to sell this as an advantage :-)
Heh, sure, probably not a big deal, but inconsistent with serial devices
that come with hardware triggered tx/rx leds (e.g. some USB serial
devices).
> > > > > @@ -163,8 +165,31 @@ struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
> > > > > return dev;
> > > > > }
> > > > >
> > > > > - return tty_register_device_attr(driver, index, device, drvdata,
> > > > > - attr_grp);
> > > > > + if (IS_ENABLED(CONFIG_TTY_LEDS_TRIGGERS)) {
> > > >
> > > > This doesn't really improve readability much. Use compile guards in the
> > > > header file to provide dummy implementations for setup/teardown
> > > > functions when the trigger isn't selected. And yes, the actual
> > > > implementation may require ifdefs around them in the c-file as well.
> > >
> > > I like this better than cpp stuff because it respects the indention of
> > > the C source. This is also supported by Documentation/process/coding-style.rst.
> > > (Well, I have admit that it's not impossible to interpret the document
> > > to prefer dummy implementations over IS_ENABLED, but what I learned from
> > > reviews to other contributions I expected this to be the commonly
> > > accepted and expected way.).
> >
> > Yeah, and I think the following paragraph is pretty clear on this:
> >
> > Prefer to compile out entire functions, rather than portions of
> > functions or portions of expressions. Rather than putting an
> > ifdef in an expression, factor out part or all of the expression
> > into a separate helper function and apply the conditional to
> > that function.
>
> I understand "Prefer to compile out" as "Prefer to compile out using #if
> et al", so it doesn't cover if (IS_ENABLED(...)). The section title is
> "Conditional Compilation" which supports my interpretation and there is
> no "ifdef in any affected expression". The nice thing about IS_ENABLED
> is that it can be indented naturally without looking strange.
I'd argue the opposite; you get natural indentation when using stub
functions. Do you really find your version:
if (IS_ENABLED(CONFIG_TTY_LEDS_TRIGGERS)) {
int ret;
ret = led_trigger_register_format(&port->led_trigger_rx,
"%s%d-rx", driver->name, index);
if (ret < 0)
pr_warn("Failed to register rx trigger for %s%d (%d)\n",
driver->name, index, ret);
ret = led_trigger_register_format(&port->led_trigger_tx,
"%s%d-tx", driver->name, index);
if (ret < 0)
pr_warn("Failed to register tx trigger for %s%d (%d)\n",
driver->name, index, ret);
}
dev = tty_register_device_attr(driver, index,
device, drvdata, attr_grp);
if (IS_ENABLED(CONFIG_TTY_LEDS_TRIGGERS) && IS_ERR(dev)) {
led_trigger_unregister_simple(port->led_trigger_tx);
led_trigger_unregister_simple(port->led_trigger_rx);
}
easier to read (and more "naturally" indented) than:
ret = tty_led_trigger_register(port, driver, index);
if (ret)
return ret;
dev = tty_register_device_attr(driver, index,
device, drvdata, attr_grp);
if (IS_ERR(dev))
tty_led_trigger_deregister(port);
Either way, I insist that you use the latter form here.
> compared to #if it still ensures that the block is looked at by the
> compiler even when the condition is false, which is also a nice
> property.
That is true, but hardly a deal breaker as the builds bots would catch
any bit rot. And in any case, you could theoretically even use
if (!IS_ENABLED(CONFIG_TTY_LEDS_TRIGGERS))
return 0;
if that worries you.
> > And in this case I really don't want to see whatever magic is done to
> > register a led trigger so factoring that out into helper function allows
> > the tty code to remain clean. You also shouldn't keep those led triggers
> > around in struct tty_port when !CONFIG_TTY_LEDS_TRIGGERS.
>
> Having them even without CONFIG_TTY_LEDS_TRIGGERS helps to have to use
> less #ifdef in the code. Without CONFIG_LEDS_TRIGGER sizeof(struct
> led_trigger) is 0, which is an IMHO fine construct.
Ah, I had not noticed that alternate trigger struct definition, perhaps
no need for ifdefs in the header then.
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
