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

[Uwe Kleine-König]

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

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

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |