[PATCH 02/12] drivers/gpio: provide optional timestamped readouts

Philippe Gerum rpm at xenomai.org
Fri Jan 25 12:42:41 CET 2019


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 <rpm at xenomai.org>
>>>>>>>>>> ---
>>>>>>>>>>       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.



More information about the Xenomai mailing list