Re: [libgpiod][PATCH v2 2/4] tools: use ppoll() where higher timeout resolution makes sense

[Kent Gibson <warthog618@xxxxxxxxx>]

On Tue, Apr 16, 2024 at 11:52:20PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxx>

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
>