From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [PATCH 02/12] drivers/gpio: provide optional timestamped readouts References: <20190124153428.21006-1-rpm@xenomai.org> <20190124153428.21006-3-rpm@xenomai.org> <4d1c39af-7a6f-1637-7727-ab6ad6d533c1@siemens.com> <40b168af-98d1-955d-ff01-ae5e70113744@xenomai.org> <197faf1a-c056-36fd-28ce-d8eee333b9cd@xenomai.org> <214e962e-f725-f056-7153-7225b21a4112@siemens.com> <1215ea5d-5ba7-348e-20b9-7d46cc77c183@xenomai.org> <76d6c198-2c6a-fa2c-8e0e-3ae92f7c2210@siemens.com> From: Jan Kiszka Message-ID: <015cdb61-fdbe-81e7-f609-9c9c795977bd@siemens.com> Date: Fri, 25 Jan 2019 12:32:16 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit List-Id: Discussions about the Xenomai project List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Philippe Gerum , xenomai@xenomai.org 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 >>>>>>>>> --- >>>>>>>>>      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. > > 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. Jan -- Siemens AG, Corporate Technology, CT RDA IOT SES-DE Corporate Competence Center Embedded Linux