On 1/25/19 12:32 PM, Jan Kiszka wrote: > On 25.01.19 11:11, Philippe Gerum wrote: >> On 1/25/19 10:48 AM, Jan Kiszka wrote: >>> On 25.01.19 10:38, Philippe Gerum wrote: >>>> On 1/25/19 10:33 AM, Jan Kiszka wrote: >>>>> On 25.01.19 10:31, Philippe Gerum wrote: >>>>>> On 1/25/19 10:23 AM, Jan Kiszka wrote: >>>>>>> On 25.01.19 10:15, Philippe Gerum wrote: >>>>>>>> On 1/24/19 7:17 PM, Jan Kiszka wrote: >>>>>>>>> On 24.01.19 16:34, Philippe Gerum wrote: >>>>>>>>>> In timestamping mode, read() returns the timestamp of the latest >>>>>>>>>> event >>>>>>>>>> receipt on the pin based on CLOCK_MONOTONIC, along with the pin >>>>>>>>>> state. This is an optional pin readout mode controlled by the >>>>>>>>>> GPIO_RTIOC_TS request, e.g.: >>>>>>>>>> >>>>>>>>>> struct rtdm_gpio_readout rdo; >>>>>>>>>> int ret, on, val; >>>>>>>>>> >>>>>>>>>> on = 1; >>>>>>>>>> ret = ioctl(pinfd, GPIO_RTIOC_TS, &on); >>>>>>>>>> ret = read(pinfd, &rdo, sizeof(rdo)); >>>>>>>>>> /* pin state changed to rdo.value at time rdo.timestamp */ >>>>>>>>>> >>>>>>>>>> on = 0; >>>>>>>>>> ret = ioctl(pinfd, GPIO_RTIOC_TS, &on); >>>>>>>>>> ret = read(pinfd, &val, sizeof(val)); >>>>>>>>>> /* pin state changed to value (time of change unspecified) */ >>>>>>>>>> >>>>>>>>>> By default, timestamping mode is disabled, which corresponds >>>>>>>>>> to the >>>>>>>>>> original behavior. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Philippe Gerum <[email protected]> >>>>>>>>>> --- >>>>>>>>>> include/cobalt/kernel/rtdm/gpio.h | 1 + >>>>>>>>>> include/rtdm/uapi/gpio.h | 18 +++++++---- >>>>>>>>>> kernel/drivers/gpio/gpio-core.c | 54 >>>>>>>>>> +++++++++++++++++++++++-------- >>>>>>>>>> 3 files changed, 54 insertions(+), 19 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/include/cobalt/kernel/rtdm/gpio.h >>>>>>>>>> b/include/cobalt/kernel/rtdm/gpio.h >>>>>>>>>> index cdb472f8a..00055ec0a 100644 >>>>>>>>>> --- a/include/cobalt/kernel/rtdm/gpio.h >>>>>>>>>> +++ b/include/cobalt/kernel/rtdm/gpio.h >>>>>>>>>> @@ -33,6 +33,7 @@ struct rtdm_gpio_pin { >>>>>>>>>> rtdm_event_t event; >>>>>>>>>> char *name; >>>>>>>>>> struct gpio_desc *desc; >>>>>>>>>> + nanosecs_abs_t timestamp; >>>>>>>>>> }; >>>>>>>>>> struct rtdm_gpio_chip { >>>>>>>>>> diff --git a/include/rtdm/uapi/gpio.h b/include/rtdm/uapi/gpio.h >>>>>>>>>> index b745f156c..ac14be66c 100644 >>>>>>>>>> --- a/include/rtdm/uapi/gpio.h >>>>>>>>>> +++ b/include/rtdm/uapi/gpio.h >>>>>>>>>> @@ -18,12 +18,18 @@ >>>>>>>>>> #ifndef _RTDM_UAPI_GPIO_H >>>>>>>>>> #define _RTDM_UAPI_GPIO_H >>>>>>>>>> -#define GPIO_RTIOC_DIR_OUT _IOW(RTDM_CLASS_GPIO, 0, >>>>>>>>>> int) >>>>>>>>>> -#define GPIO_RTIOC_DIR_IN _IO(RTDM_CLASS_GPIO, 1) >>>>>>>>>> -#define GPIO_RTIOC_IRQEN _IOW(RTDM_CLASS_GPIO, 2, int) /* >>>>>>>>>> GPIO >>>>>>>>>> trigger */ >>>>>>>>>> -#define GPIO_RTIOC_IRQDIS _IO(RTDM_CLASS_GPIO, 3) >>>>>>>>>> -#define GPIO_RTIOC_REQS _IO(RTDM_CLASS_GPIO, 4) >>>>>>>>>> -#define GPIO_RTIOC_RELS _IO(RTDM_CLASS_GPIO, 5) >>>>>>>>>> +struct rtdm_gpio_readout { >>>>>>>>>> + __u64 timestamp; >>>>>>>>> >>>>>>>>> nanosecs_abs_t - we use this type also in to userspace interface. >>>>>>>>> >>>>>>>> >>>>>>>> Nope. This is ABI stuff, nanosecs_abs_t is not guaranteed to >>>>>>>> have the >>>>>>>> same size regardless of the ABI models, which is unsafe in mixed >>>>>>>> 32/64 >>>>>>>> model configurations. OTOH, __u64 is safe. >>>>>>> >>>>>>> If nanosecs_abs_t is alread an ABI, and it is used as such for a >>>>>>> long >>>>>>> time (see include/rtdm/uapi/serial.h). If you see a bug in its >>>>>>> definition, that needs to be fixed. We should not resolve that by >>>>>>> choosing open-coded local workarounds. >>>>>>> >>>>>> >>>>>> nanosecs_t is not defined in the Xenomai ABI, never has been. Using >>>>>> nanosecs_t in structs actually abuses such ABI. So we may either keep >>>>>> adding even more nanosecs, or fix the spots where it should be >>>>>> replaced >>>>>> with __u64. >>>>> >>>>> Xenomai ABI includes the driver ABI - so your assumption might have >>>>> been >>>>> it's not while it was de facto. >>>>> >>>>> Again, let's address the issue, not work around it. >>>>> >>>> >>>> __u64 is the fix, using nanosecs*_t in uapi/ is wrong. So please merge >>>> this fix, and we'll see how we can progress fixing other abuses. >>> >>> Nope: Adjust nanosecs_*_t if it has a problem ([u]int64_t vs. __u/s64 - >>> what's the problem exactly?), and use that. That will both address >>> existing users and future ones. No special solution for the GPIO driver >>> here. >>> >> >> I'm not pushing for a GPIO-specific solution, I'm pushing for using >> ABI-specific types which state their size explicitly in uapi/ data as >> much as possible, which is safer with mixed ABI models and would have >> spared me a lot of trouble back when I implemented the 32/64 ABI support >> for Xenomai. nanosecs_* is originally a kernel-side type which slipped >> in uapi/ structs, which does not state its size. > > No, it was designed and used for both side. I know best as I wrote and > first used that abstraction. >
Please avoid the "I know best" non-argument. I don't think it is particularly relevant in this context, and the last thing we want is to lose relevance when discussing technical matters. >> >> The fact that nanosecs_* is defined as uint64_t but the kernel today >> does not address the issue at stake: there would be no obvious way to >> detect that a discrepancy might exist with what userland expects once >> such definition is modified for a different with if ever (which might be >> highly unlikely for nanosecs, but could happen for other types). > > So the good news is that a) we do not have a problem today and b) we can > safely prevent any hypothetical future by changing the definition of > nanosecs_*_t towards __u64/__s64 backing. So, if you are concerned about > the stability of standard-int, let's switch to the kernel uapi types for > nanosecs, use that also for the GPIO ABI, and call it a day. > Ok, you don't seem to understand my point. I'll switch to nanosecs*, time is a scarce resource for me too. -- Philippe.
