| Message ID | 20240416215222.175166-3-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | tools: timeout handling improvements | expand |
On Tue, Apr 16, 2024 at 11:52:20PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > We allow timeout units to be specified in microseconds but for poll() we > need to round them up to milliseconds. Switch to ppoll() to avoid losing > precision. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > tools/gpiomon.c | 12 ++++++++++-- > tools/gpionotify.c | 12 ++++++++++-- > 2 files changed, 20 insertions(+), 4 deletions(-) > > diff --git a/tools/gpiomon.c b/tools/gpiomon.c > index 40e6ac2..728a671 100644 > --- a/tools/gpiomon.c > +++ b/tools/gpiomon.c > @@ -176,7 +176,7 @@ static int parse_config(int argc, char **argv, struct config *cfg) > cfg->fmt = optarg; > break; > case 'i': > - cfg->idle_timeout = parse_period_or_die(optarg) / 1000; > + cfg->idle_timeout = parse_period_or_die(optarg); > break; > case 'l': > cfg->active_low = true; > @@ -362,6 +362,7 @@ int main(int argc, char **argv) > int num_lines, events_done = 0; > struct gpiod_edge_event *event; > struct line_resolver *resolver; > + struct timespec idle_timeout; > struct gpiod_chip *chip; > struct pollfd *pollfds; > unsigned int *offsets; > @@ -453,7 +454,14 @@ int main(int argc, char **argv) > for (;;) { > fflush(stdout); > > - ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout); > + if (cfg.idle_timeout > 0) { > + idle_timeout.tv_sec = cfg.idle_timeout / 1000000; > + idle_timeout.tv_nsec = > + (cfg.idle_timeout % 1000000) * 1000; > + } > + > + ret = ppoll(pollfds, resolver->num_chips, > + cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL); > if (ret < 0) > die_perror("error polling for events"); > One minor nit - I would introduce a timespec pointer initialised to NULL and set to point to idle_timeout within the if rather than repeat the cfg.idle_timeout > 0 test. But that is just personal preference, so either way, Reviewed-by: Kent Gibson <warthog618@gmail.com> for the series. Cheers, Kent. > diff --git a/tools/gpionotify.c b/tools/gpionotify.c > index d2aee15..962896c 100644 > --- a/tools/gpionotify.c > +++ b/tools/gpionotify.c > @@ -132,7 +132,7 @@ static int parse_config(int argc, char **argv, struct config *cfg) > cfg->fmt = optarg; > break; > case 'i': > - cfg->idle_timeout = parse_period_or_die(optarg) / 1000; > + cfg->idle_timeout = parse_period_or_die(optarg); > break; > case 'n': > cfg->events_wanted = parse_uint_or_die(optarg); > @@ -374,6 +374,7 @@ int main(int argc, char **argv) > int i, j, ret, events_done = 0, evtype; > struct line_resolver *resolver; > struct gpiod_info_event *event; > + struct timespec idle_timeout; > struct gpiod_chip **chips; > struct gpiod_chip *chip; > struct pollfd *pollfds; > @@ -422,7 +423,14 @@ int main(int argc, char **argv) > for (;;) { > fflush(stdout); > > - ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout); > + if (cfg.idle_timeout > 0) { > + idle_timeout.tv_sec = cfg.idle_timeout / 1000000; > + idle_timeout.tv_nsec = > + (cfg.idle_timeout % 1000000) * 1000; > + } > + > + ret = ppoll(pollfds, resolver->num_chips, > + cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL); > if (ret < 0) > die_perror("error polling for events"); > > -- > 2.40.1 >
On Wed, Apr 17, 2024 at 9:23 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Tue, Apr 16, 2024 at 11:52:20PM +0200, Bartosz Golaszewski wrote: > > > > - ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout); > > + if (cfg.idle_timeout > 0) { > > + idle_timeout.tv_sec = cfg.idle_timeout / 1000000; > > + idle_timeout.tv_nsec = > > + (cfg.idle_timeout % 1000000) * 1000; > > + } > > + > > + ret = ppoll(pollfds, resolver->num_chips, > > + cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL); > > if (ret < 0) > > die_perror("error polling for events"); > > > > One minor nit - I would introduce a timespec pointer initialised to NULL > and set to point to idle_timeout within the if rather than repeat the > cfg.idle_timeout > 0 test. > Actually we can avoid it by doing it once before we enter the for(;;) loop. It's passed by constant pointer to ppoll() anyway and having the struct AND pointer to it initialized to NULL sounds more complex than it needs to be. I'll do it in v2. > But that is just personal preference, so either way, > > Reviewed-by: Kent Gibson <warthog618@gmail.com> > Thanks, I'll keep the tag if you don't mind the above solution? Bart
On Mon, Apr 22, 2024 at 08:15:46PM +0200, Bartosz Golaszewski wrote: > On Wed, Apr 17, 2024 at 9:23 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Tue, Apr 16, 2024 at 11:52:20PM +0200, Bartosz Golaszewski wrote: > > > > > > - ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout); > > > + if (cfg.idle_timeout > 0) { > > > + idle_timeout.tv_sec = cfg.idle_timeout / 1000000; > > > + idle_timeout.tv_nsec = > > > + (cfg.idle_timeout % 1000000) * 1000; > > > + } > > > + > > > + ret = ppoll(pollfds, resolver->num_chips, > > > + cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL); > > > if (ret < 0) > > > die_perror("error polling for events"); > > > > > > > One minor nit - I would introduce a timespec pointer initialised to NULL > > and set to point to idle_timeout within the if rather than repeat the > > cfg.idle_timeout > 0 test. > > > > Actually we can avoid it by doing it once before we enter the for(;;) > loop. It's passed by constant pointer to ppoll() anyway and having the > struct AND pointer to it initialized to NULL sounds more complex than > it needs to be. I'll do it in v2. > Ah, I overlooked the for loop, so you are right that it should be initialised before that. > > But that is just personal preference, so either way, > > > > Reviewed-by: Kent Gibson <warthog618@gmail.com> > > > > Thanks, I'll keep the tag if you don't mind the above solution? > Sure. Cheers, Kent.
diff --git a/tools/gpiomon.c b/tools/gpiomon.c index 40e6ac2..728a671 100644 --- a/tools/gpiomon.c +++ b/tools/gpiomon.c @@ -176,7 +176,7 @@ static int parse_config(int argc, char **argv, struct config *cfg) cfg->fmt = optarg; break; case 'i': - cfg->idle_timeout = parse_period_or_die(optarg) / 1000; + cfg->idle_timeout = parse_period_or_die(optarg); break; case 'l': cfg->active_low = true; @@ -362,6 +362,7 @@ int main(int argc, char **argv) int num_lines, events_done = 0; struct gpiod_edge_event *event; struct line_resolver *resolver; + struct timespec idle_timeout; struct gpiod_chip *chip; struct pollfd *pollfds; unsigned int *offsets; @@ -453,7 +454,14 @@ int main(int argc, char **argv) for (;;) { fflush(stdout); - ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout); + if (cfg.idle_timeout > 0) { + idle_timeout.tv_sec = cfg.idle_timeout / 1000000; + idle_timeout.tv_nsec = + (cfg.idle_timeout % 1000000) * 1000; + } + + ret = ppoll(pollfds, resolver->num_chips, + cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL); if (ret < 0) die_perror("error polling for events"); diff --git a/tools/gpionotify.c b/tools/gpionotify.c index d2aee15..962896c 100644 --- a/tools/gpionotify.c +++ b/tools/gpionotify.c @@ -132,7 +132,7 @@ static int parse_config(int argc, char **argv, struct config *cfg) cfg->fmt = optarg; break; case 'i': - cfg->idle_timeout = parse_period_or_die(optarg) / 1000; + cfg->idle_timeout = parse_period_or_die(optarg); break; case 'n': cfg->events_wanted = parse_uint_or_die(optarg); @@ -374,6 +374,7 @@ int main(int argc, char **argv) int i, j, ret, events_done = 0, evtype; struct line_resolver *resolver; struct gpiod_info_event *event; + struct timespec idle_timeout; struct gpiod_chip **chips; struct gpiod_chip *chip; struct pollfd *pollfds; @@ -422,7 +423,14 @@ int main(int argc, char **argv) for (;;) { fflush(stdout); - ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout); + if (cfg.idle_timeout > 0) { + idle_timeout.tv_sec = cfg.idle_timeout / 1000000; + idle_timeout.tv_nsec = + (cfg.idle_timeout % 1000000) * 1000; + } + + ret = ppoll(pollfds, resolver->num_chips, + cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL); if (ret < 0) die_perror("error polling for events");