On Thu, May 17, 2018 at 08:16:25AM +0200, Uwe Kleine-König wrote:
> Hello Johan,
>
> 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.
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.
> > > @@ -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.
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.
> > Also note that you need to add this to tty_port_register_device() as
> > well... Serdev is currently opt-in and only enabled for serial core (due
> > to some lifetime issues and issues related to hotplug), but there's no
> > reason to limit these triggers to serial core.
>
> I cannot follow this immediatly. Will study the code a bit and then
> probably understand.
There are (at least) three ways to register a tty device of which two
set up the port link at registration and one where the tty port link is
setup when the device is opened:
tty_port_register_device_attr()
tty_port_register_device_attr_serdev()
tty_register_device_attr()
The tty_port_register_device_attr_serdev() is supposed to be a temporary
solution until we can enable serdev for all tty drivers that register
their devices along the their ports. But for now, you'd probably want to
register led triggers in both of the tty_port_register-functions.
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
