From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnd Bergmann Subject: Re: [RFC] libgpiod public API reviews needed Date: Mon, 22 Jan 2018 14:46:12 +0100 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: Received: from mail-ot0-f196.google.com ([74.125.82.196]:41211 "EHLO mail-ot0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751087AbeAVNqO (ORCPT ); Mon, 22 Jan 2018 08:46:14 -0500 Received: by mail-ot0-f196.google.com with SMTP id 44so7466246otk.8 for ; Mon, 22 Jan 2018 05:46:14 -0800 (PST) In-Reply-To: Sender: linux-gpio-owner@vger.kernel.org List-Id: linux-gpio@vger.kernel.org To: Linus Walleij Cc: Bartosz Golaszewski , linux-gpio@vger.kernel.org, Andy Shevchenko , Clemens Gruber , Thierry Reding , Peter Rosin , Lars-Peter Clausen On Mon, Jan 22, 2018 at 1:02 PM, Linus Walleij wrote: > On Mon, Jan 22, 2018 at 10:25 AM, Arnd Bergmann wrote: >> On Mon, Jan 22, 2018 at 9:21 AM, Linus Walleij wrote: > >>> /** >>> * struct gpioevent_data - The actual event being pushed to userspace >>> * @timestamp: best estimate of time of event occurrence, in nanoseconds >>> * @id: event identifier >>> */ >>> struct gpioevent_data { >>> __u64 timestamp; >>> __u32 id; >>> }; >>> >>> It is the same as is used for IIO. Inside the kernel this ultimately >>> comes from ktime_get_real_ns(); >> >> Ah, too bad, that already contains two mistakes: >> >> - on x86, the structures are incompatible between 32-bit and 64-bit >> user space, as the former has no padding. > > Sigh yeah that's bad... I guess we will be saved by the 64bit > word coming first in the struct? (with the security problem > you mention appearing after the 32 bits in ID.) The main problem is the size of the data: kfifo_copy_to_user() can copy multiple records, and while the first one would remain accessible correctly, the second record contains garbage if the kernel adds 4 bytes of padding inbetween. >> - 'real' timestamps are inconvenient because time may jump in >> either direction. Time stamps should use 'monotonic' time, i.e. >> ktime_get_ns(). > > So what we have in IIO is that it is configurable what timestamp > you get. > > I've been meaning to fix this by breaking their timestamping > into lib/ and use the same configurability per-gpiochip for GPIO. > I guess this is the time to actually do it and stop talking on > my part :/ Ok, makes sense. > They also use the same default timestamp though. > > commit bc2b7dab629a ("iio:core: timestamping clock selection support") > > The code is in IIO in drivers/iio/industrialio-core.c: > > /** > * iio_get_time_ns() - utility function to get a time stamp for events etc > * @indio_dev: device > */ > s64 iio_get_time_ns(const struct iio_dev *indio_dev) > { > struct timespec tp; > > switch (iio_device_get_clock(indio_dev)) { > case CLOCK_REALTIME: > ktime_get_real_ts(&tp); > break; > case CLOCK_MONOTONIC: > ktime_get_ts(&tp); > break; > case CLOCK_MONOTONIC_RAW: > getrawmonotonic(&tp); > break; > case CLOCK_REALTIME_COARSE: > tp = current_kernel_time(); > break; > case CLOCK_MONOTONIC_COARSE: > tp = get_monotonic_coarse(); > break; > case CLOCK_BOOTTIME: > get_monotonic_boottime(&tp); > break; > case CLOCK_TAI: > timekeeping_clocktai(&tp); > break; > default: > BUG(); > } > > return timespec_to_ns(&tp); > } > EXPORT_SYMBOL(iio_get_time_ns); > > Which clock is used is configured per-device in sysfs. > > It's pretty neat I think. To be honest, I think this is overcomplicating things. While I have a patch to change the interface names and make them all ktime_get_*_ns(), most of these clocks make no sense at all for a random user space interface, mainly because I wouldn't trust user space programmers to make an informed decision which of those seven to use. Arnd