Re: [PATCH v2 3/3] leds: trigger: implement a tty trigger

[Greg Kroah-Hartman]

On Tue, Dec 17, 2019 at 11:58:26AM +0100, Uwe Kleine-König wrote:
> Hello Greg,
> 
> On Tue, Dec 17, 2019 at 09:32:11AM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Dec 17, 2019 at 09:17:18AM +0100, Uwe Kleine-König wrote:
> > > +	ret = tty_get_icount(trigger_data->tty, &icount);
> > > +	if (icount.rx > trigger_data->icount.rx ||
> > > +	    icount.tx > trigger_data->icount.tx) {
> > 
> > What happens when icount.rx and/or icount.tx wraps?  It's "only" an int.
> 
> Good catch. I wonder why this is not an unsigned quantity. Just grepping
> through drivers/tty/serial most drivers just increment these counters
> and don't care for overflow (which is undefined for ints) either. :-\

It is not undefined for the kernel, I'm pretty sure we tell the compiler
to be sane for this type of thing.  It should "just wrap".  Oh wait
"int" is "signed int" here, hah, that's funny.  Surely someone has
noticed that in 20+ years by now?

> ..ooOO(Where is the can maintainer? --- We found a can of worms :-)

:)

> 
> > > +		unsigned long delay_on = 100, delay_off = 100;
> > > +
> > > +		led_blink_set_oneshot(trigger_data->led_cdev,
> > > +				      &delay_on, &delay_off, 0);
> > > +
> > > +		trigger_data->icount = icount;
> > 
> > Implicit memcpy of a structure?  Ick.
> 
> I'd call that elegant ;-)
> 
> > All you care about are the two integers, why not just track them instead
> > of the whole thing?
> 
> For now I only care about tx and rx, but I intend to add some bells and
> whistles to trigger on other events. (But I don't care much, can add
> that once I implement this support.)

Start small and add more as-needed, you can always move back to the full
structure later on if you really need it.

thanks,

greg k-h