[PATCH 00/12] Assorted updates: RTnet, GPIO, smokey tests

[PATCH 00/12] Assorted updates: RTnet, GPIO, smokey tests

[Philippe Gerum]

Patch 1 addresses the issue in the posix_clock fix as discussed in
[1]. Patch 6 works around a recent regression in RTnet as mentioned in
[2] (tested with the IGB driver which implements a map_rtskb
handler). Both commits are also -stable candidates.

A couple of other changes extend the GPIO support with a timestamping
feature, so that we may ask for the date of the latest state change of
an input pin when reported by an interrupt event.

The rest is RTnet-related. In addition to patch 6, I would recommend
picking 7 and 8 for -stable.

[1] https://www.xenomai.org/pipermail/xenomai/2019-January/040225.html
[2] https://www.xenomai.org/pipermail/xenomai/2019-January/040230.html

Philippe Gerum (12):
  testsuite/smokey: posix_clock: prevent false positive in
    time-dependent test
  drivers/gpio: provide optional timestamped readouts
  testsuite/gpiotest: enable timestamping on 'timestamp' argument
  net/stack: allow initializing pre-allocated device structs
  net/stack: fresh rtskb should have ip_summed set to CHECKSUM_NONE
  net/rtdev: ensure per-device skbs get mapped at registration
  net/udp: getfrag: fix frag preparation status
  net/udp: getfrag: remove direct reference to user memory
  testsuite/smokey: net: do not unload pre-loaded modules
  testsuite/smokey: net: do not down a previously running test interface
  net/stack: rtskb: do not run nop locking calls
  testsuite/smokey: net_client: improve stats readability

 include/cobalt/kernel/rtdm/gpio.h          |   1 +
 include/rtdm/uapi/gpio.h                   |  18 ++-
 kernel/drivers/gpio/gpio-core.c            |  54 +++++--
 kernel/drivers/net/stack/include/rtdev.h   |   9 ++
 kernel/drivers/net/stack/ipv4/udp/udp.c    |  21 +--
 kernel/drivers/net/stack/rtdev.c           | 167 ++++++++++++++-------
 kernel/drivers/net/stack/rtskb.c           |  26 +---
 testsuite/gpiotest/gpiotest.c              |  37 ++++-
 testsuite/smokey/net_common/client.c       |  26 ++--
 testsuite/smokey/net_common/setup.c        | 108 ++++++++++---
 testsuite/smokey/posix-clock/posix-clock.c |   2 +-
 11 files changed, 325 insertions(+), 144 deletions(-)

-- 
2.17.2

[PATCH 01/12] testsuite/smokey: posix_clock: prevent false positive in time-dependent test

[Philippe Gerum]

clock_decrease_after_periodic_timer_first_tick checks that periodic
interval timers based on CLOCK_REALTIME are not (pathologically)
affected by the epoch going backwards.

To this end, we measure the actual time observed between two ticks of
a periodic timer based on CLOCK_REALTIME with a call to
clock_settime() injecting a negative offset in between, equivalent to
five ticks.

Due to processing delays induced by clock_settime() and other latency,
we could observe a duration which exceeds a tick by a few tenths of
microseconds. Since we can't anticipate the amount of latency
involved, let's accept a longer delay of at most two ticks.

This is still correct from the standpoint of the test, which verifies
that no correlation exists between the clock offset injected by
clock_settime() and the delay until the next tick generated by the
affected clock.

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 testsuite/smokey/posix-clock/posix-clock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/testsuite/smokey/posix-clock/posix-clock.c b/testsuite/smokey/posix-clock/posix-clock.c
index f672a9d52..3a638d41f 100644
--- a/testsuite/smokey/posix-clock/posix-clock.c
+++ b/testsuite/smokey/posix-clock/posix-clock.c
@@ -417,7 +417,7 @@ static int clock_decrease_after_periodic_timer_first_tick(void)
 
 	diff = now.tv_sec * 1000000000ULL + now.tv_nsec -
 		(timer.it_value.tv_sec * 1000000000ULL + timer.it_value.tv_nsec);
-	if (!smokey_assert(diff < 1000000000))
+	if (!smokey_assert(diff < 2000000000))
 		return -EINVAL;
 	
 	ret = smokey_check_errno(read(t, &ticks, sizeof(ticks)));
-- 
2.17.2

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

[Philippe Gerum]

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@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;
+	__s32 value;
+};
+
+#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)
+#define GPIO_RTIOC_TS		_IOR(RTDM_CLASS_GPIO, 7, int)
 
 #define GPIO_TRIGGER_NONE		0x0 /* unspecified */
 #define GPIO_TRIGGER_EDGE_RISING	0x1
diff --git a/kernel/drivers/gpio/gpio-core.c b/kernel/drivers/gpio/gpio-core.c
index 3ce73dbd5..81f9653f1 100644
--- a/kernel/drivers/gpio/gpio-core.c
+++ b/kernel/drivers/gpio/gpio-core.c
@@ -28,7 +28,8 @@ struct rtdm_gpio_chan {
 	int requested : 1,
 		has_direction : 1,
 		is_output : 1,
-		is_interrupt : 1;
+	        is_interrupt : 1,
+		want_timestamp : 1;
 };
 
 static LIST_HEAD(rtdm_gpio_chips);
@@ -41,6 +42,7 @@ static int gpio_pin_interrupt(rtdm_irq_t *irqh)
 
 	pin = rtdm_irq_get_arg(irqh, struct rtdm_gpio_pin);
 
+	pin->timestamp = rtdm_clock_read_monotonic();
 	rtdm_event_signal(&pin->event);
 
 	return RTDM_IRQ_HANDLED;
@@ -187,6 +189,12 @@ static int gpio_pin_ioctl_nrt(struct rtdm_fd *fd,
 		gpio_free(gpio);
 		chan->requested = false;
 		break;
+	case GPIO_RTIOC_TS:
+		ret = rtdm_safe_copy_from_user(fd, &val, arg, sizeof(val));
+		if (ret)
+			return ret;
+		chan->want_timestamp = !!val;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -199,11 +207,9 @@ static ssize_t gpio_pin_read_rt(struct rtdm_fd *fd,
 {
 	struct rtdm_gpio_chan *chan = rtdm_fd_to_private(fd);
 	struct rtdm_device *dev = rtdm_fd_device(fd);
+	struct rtdm_gpio_readout rdo;
 	struct rtdm_gpio_pin *pin;
-	int value, ret;
-
-	if (len < sizeof(value))
-		return -EINVAL;
+	int ret;
 
 	if (!chan->has_direction)
 		return -EAGAIN;
@@ -213,16 +219,37 @@ static ssize_t gpio_pin_read_rt(struct rtdm_fd *fd,
 
 	pin = container_of(dev, struct rtdm_gpio_pin, dev);
 
-	if (!(fd->oflags & O_NONBLOCK)) {
-		ret = rtdm_event_wait(&pin->event);
-		if (ret)
-			return ret;
-	}
+	if (chan->want_timestamp) {
+		if (len < sizeof(rdo))
+			return -EINVAL;
+
+		if (!(fd->oflags & O_NONBLOCK)) {
+			ret = rtdm_event_wait(&pin->event);
+			if (ret)
+				return ret;
+			rdo.timestamp = pin->timestamp;
+		} else
+			rdo.timestamp = rtdm_clock_read_monotonic();
+
+		len = sizeof(rdo);
+		rdo.value = gpiod_get_raw_value(pin->desc);
+		ret = rtdm_safe_copy_to_user(fd, buf, &rdo, len);
+	} else {
+		if (len < sizeof(rdo.value))
+			return -EINVAL;
 
-	value = gpiod_get_raw_value(pin->desc);
-	ret = rtdm_safe_copy_to_user(fd, buf, &value, sizeof(value));
+		if (!(fd->oflags & O_NONBLOCK)) {
+			ret = rtdm_event_wait(&pin->event);
+			if (ret)
+				return ret;
+		}
+
+		len = sizeof(rdo.value);
+		rdo.value = gpiod_get_raw_value(pin->desc);
+		ret = rtdm_safe_copy_to_user(fd, buf, &rdo.value, len);
+	}
 	
-	return ret ?: sizeof(value);
+	return ret ?: len;
 }
 
 static ssize_t gpio_pin_write_rt(struct rtdm_fd *fd,
@@ -462,6 +489,7 @@ int rtdm_gpiochip_post_event(struct rtdm_gpio_chip *rgc,
 		return -EINVAL;
 
 	pin = rgc->pins + offset;
+	pin->timestamp = rtdm_clock_read_monotonic();
 	rtdm_event_signal(&pin->event);
 	
 	return 0;
-- 
2.17.2

[PATCH 03/12] testsuite/gpiotest: enable timestamping on 'timestamp' argument

[Philippe Gerum]

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 testsuite/gpiotest/gpiotest.c | 37 +++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/testsuite/gpiotest/gpiotest.c b/testsuite/gpiotest/gpiotest.c
index 3c8c6c318..7347ac506 100644
--- a/testsuite/gpiotest/gpiotest.c
+++ b/testsuite/gpiotest/gpiotest.c
@@ -28,6 +28,7 @@
 #include <fcntl.h>
 #include <smokey/smokey.h>
 #include <rtdm/gpio.h>
+#include <boilerplate/time.h>
 
 smokey_test_plugin(interrupt,
 		   SMOKEY_ARGLIST(
@@ -39,6 +40,7 @@ smokey_test_plugin(interrupt,
    "\tdevice=<device-path>\n"
    "\trigger={edge[-rising/falling/both], level[-low/high]}\n"
    "\tselect, wait on select(2)."
+   "\ttimestamp, enable timestamping."
 );
 
 smokey_test_plugin(read_value,
@@ -72,8 +74,10 @@ static int run_interrupt(struct smokey_test *t, int argc, char *const argv[])
 		{ .name = "level-high", .flag = GPIO_TRIGGER_LEVEL_HIGH },
 		{ NULL, 0 },
 	};
-	int do_select = 0, fd, ret, trigger, n, value;
+	int do_select = 0, fd, ret, trigger, n, value, do_timestamp = 0;
 	const char *device = NULL, *trigname;
+	struct rtdm_gpio_readout rdo;
+	struct timespec now;
 	fd_set set;
 	
 	smokey_parse_args(t, argc, argv);
@@ -95,6 +99,9 @@ static int run_interrupt(struct smokey_test *t, int argc, char *const argv[])
 	if (SMOKEY_ARG_ISSET(interrupt, select))
 		do_select = SMOKEY_ARG_BOOL(interrupt, select);
 
+	if (SMOKEY_ARG_ISSET(interrupt, timestamp))
+		do_timestamp = SMOKEY_ARG_BOOL(interrupt, timestamp);
+
 	trigger = GPIO_TRIGGER_NONE;
 	if (SMOKEY_ARG_ISSET(interrupt, trigger)) {
 		trigname = SMOKEY_ARG_STRING(interrupt, trigger);
@@ -131,14 +138,28 @@ static int run_interrupt(struct smokey_test *t, int argc, char *const argv[])
 				return ret;
 			}
 		}
-		ret = read(fd, &value, sizeof(value));
-		if (ret < 0) {
-			ret = -errno;
-			warning("failed reading from %s [%s]",
-				device, symerror(ret));
-			return ret;
+		if (do_timestamp) {
+			ret = read(fd, &rdo, sizeof(rdo));
+			if (ret < 0) {
+				ret = -errno;
+				warning("failed reading from %s [%s]",
+					device, symerror(ret));
+				return ret;
+			}
+			clock_gettime(CLOCK_MONOTONIC, &now);
+			printf("received irq %llu us from now, GPIO state=%d\n",
+			       (timespec_scalar(&now) - rdo.timestamp) / 1000ULL,
+			       rdo.value);
+		} else {
+			ret = read(fd, &value, sizeof(value));
+			if (ret < 0) {
+				ret = -errno;
+				warning("failed reading from %s [%s]",
+					device, symerror(ret));
+				return ret;
+			}
+			printf("received irq, GPIO state=%d\n", value);
 		}
-		printf("received irq, GPIO state=%d\n", value);
 	}
 
 	close(fd);
-- 
2.17.2

[PATCH 04/12] net/stack: allow initializing pre-allocated device structs

[Philippe Gerum]

We may want to embed struct rtnet_device into a pre-allocated parent
container struct, which makes rtdev_alloc() and rt_alloc_etherdev()
useless for the purpose.

The new rtdev_init() and rt_init_etherdev() calls can be used to
initialize a pre-allocated device structure and set it up with default
ethernet settings resp. The caller should set the private area pointer
manually (defaults to NULL).

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 kernel/drivers/net/stack/include/rtdev.h |   9 ++
 kernel/drivers/net/stack/rtdev.c         | 130 +++++++++++++++--------
 2 files changed, 96 insertions(+), 43 deletions(-)

diff --git a/kernel/drivers/net/stack/include/rtdev.h b/kernel/drivers/net/stack/include/rtdev.h
index ddc7aebcd..8d37b6709 100644
--- a/kernel/drivers/net/stack/include/rtdev.h
+++ b/kernel/drivers/net/stack/include/rtdev.h
@@ -180,12 +180,21 @@ extern struct mutex rtnet_devices_nrt_lock;
 extern struct rtnet_device *rtnet_devices[];
 
 
+int __rt_init_etherdev(struct rtnet_device *rtdev,
+		       unsigned int dev_pool_size,
+		       struct module *module);
+
+#define rt_init_etherdev(__rtdev, __dev_pool_size)	\
+    __rt_init_etherdev(__rtdev, __dev_pool_size, THIS_MODULE)
+
 struct rtnet_device *__rt_alloc_etherdev(unsigned sizeof_priv,
 					unsigned dev_pool_size,
 					struct module *module);
 #define rt_alloc_etherdev(priv_size, rx_size) \
     __rt_alloc_etherdev(priv_size, rx_size, THIS_MODULE)
 
+void rtdev_destroy(struct rtnet_device *rtdev);
+
 void rtdev_free(struct rtnet_device *rtdev);
 
 int rt_register_rtnetdev(struct rtnet_device *rtdev);
diff --git a/kernel/drivers/net/stack/rtdev.c b/kernel/drivers/net/stack/rtdev.c
index e76d1debf..631ca1804 100644
--- a/kernel/drivers/net/stack/rtdev.c
+++ b/kernel/drivers/net/stack/rtdev.c
@@ -251,6 +251,39 @@ static const struct rtskb_pool_lock_ops rtdev_ops = {
     .unlock = rtdev_pool_unlock,
 };
 
+int rtdev_init(struct rtnet_device *rtdev, unsigned dev_pool_size)
+{
+    int ret;
+
+    ret = rtskb_pool_init(&rtdev->dev_pool, dev_pool_size, &rtdev_ops, rtdev);
+    if (ret < dev_pool_size) {
+	printk(KERN_ERR "RTnet: cannot allocate rtnet device pool\n");
+	rtskb_pool_release(&rtdev->dev_pool);
+	return -ENOMEM;
+    }
+
+    rtdm_mutex_init(&rtdev->xmit_mutex);
+    rtdm_lock_init(&rtdev->rtdev_lock);
+    mutex_init(&rtdev->nrt_lock);
+
+    atomic_set(&rtdev->refcount, 0);
+
+    /* scale global rtskb pool */
+    rtdev->add_rtskbs = rtskb_pool_extend(&global_pool, device_rtskbs);
+
+    return 0;
+}
+EXPORT_SYMBOL_GPL(rtdev_init);
+
+void rtdev_destroy(struct rtnet_device *rtdev)
+{
+    rtskb_pool_release(&rtdev->dev_pool);
+    rtskb_pool_shrink(&global_pool, rtdev->add_rtskbs);
+    rtdev->stack_event = NULL;
+    rtdm_mutex_destroy(&rtdev->xmit_mutex);
+}
+EXPORT_SYMBOL_GPL(rtdev_destroy);
+
 /***
  *  rtdev_alloc
  *  @int sizeof_priv:
@@ -267,70 +300,94 @@ struct rtnet_device *rtdev_alloc(unsigned sizeof_priv, unsigned dev_pool_size)
     /* ensure 32-byte alignment of the private area */
     alloc_size = sizeof (*rtdev) + sizeof_priv + 31;
 
-    rtdev = (struct rtnet_device *)kmalloc(alloc_size, GFP_KERNEL);
+    rtdev = kzalloc(alloc_size, GFP_KERNEL);
     if (rtdev == NULL) {
 	printk(KERN_ERR "RTnet: cannot allocate rtnet device\n");
 	return NULL;
     }
 
-    memset(rtdev, 0, alloc_size);
-
-    ret = rtskb_pool_init(&rtdev->dev_pool, dev_pool_size, &rtdev_ops, rtdev);
-    if (ret < dev_pool_size) {
-	printk(KERN_ERR "RTnet: cannot allocate rtnet device pool\n");
-	rtskb_pool_release(&rtdev->dev_pool);
+    ret = rtdev_init(rtdev, dev_pool_size);
+    if (ret) {
 	kfree(rtdev);
 	return NULL;
     }
 
-    rtdm_mutex_init(&rtdev->xmit_mutex);
-    rtdm_lock_init(&rtdev->rtdev_lock);
-    mutex_init(&rtdev->nrt_lock);
-
-    atomic_set(&rtdev->refcount, 0);
-
-    /* scale global rtskb pool */
-    rtdev->add_rtskbs = rtskb_pool_extend(&global_pool, device_rtskbs);
-
     if (sizeof_priv)
 	rtdev->priv = (void *)(((long)(rtdev + 1) + 31) & ~31);
 
     return rtdev;
 }
 
-
-
 /***
  *  rtdev_free
  */
 void rtdev_free (struct rtnet_device *rtdev)
 {
     if (rtdev != NULL) {
-	rtskb_pool_release(&rtdev->dev_pool);
-	rtskb_pool_shrink(&global_pool, rtdev->add_rtskbs);
-	rtdev->stack_event = NULL;
-	rtdm_mutex_destroy(&rtdev->xmit_mutex);
+	rtdev_destroy(rtdev);
 	kfree(rtdev);
     }
 }
+EXPORT_SYMBOL_GPL(rtdev_free);
+
+
+static void init_etherdev(struct rtnet_device *rtdev,
+			  struct module *module)
+{
+    rtdev->hard_header     = rt_eth_header;
+    rtdev->type            = ARPHRD_ETHER;
+    rtdev->hard_header_len = ETH_HLEN;
+    rtdev->mtu             = 1500; /* eth_mtu */
+    rtdev->addr_len        = ETH_ALEN;
+    rtdev->flags           = IFF_BROADCAST; /* TODO: IFF_MULTICAST; */
+    rtdev->get_mtu         = rt_hard_mtu;
+    rtdev->rt_owner	   = module;
+
+    memset(rtdev->broadcast, 0xFF, ETH_ALEN);
+    strcpy(rtdev->name, "rteth%d");
+}
+
+/**
+ * rt_init_etherdev - sets up an ethernet device
+ * @module: module initializing the device
+ *
+ * Fill in the fields of the device structure with ethernet-generic
+ * values. This routine can be used to set up a pre-allocated device
+ * structure. The device still needs to be registered afterwards.
+ */
+int __rt_init_etherdev(struct rtnet_device *rtdev,
+			unsigned dev_pool_size,
+			struct module *module)
+{
+    int ret;
 
+    ret = rtdev_init(rtdev, dev_pool_size);
+    if (ret)
+	    return ret;
+
+    init_etherdev(rtdev, module);
+
+    return 0;
+}
+EXPORT_SYMBOL_GPL(__rt_init_etherdev);
 
 
 /**
- * rtalloc_etherdev - Allocates and sets up an ethernet device
+ * rt_alloc_etherdev - Allocates and sets up an ethernet device
  * @sizeof_priv: size of additional driver-private structure to
  *               be allocated for this ethernet device
  * @dev_pool_size: size of the rx pool
- * @module: module creating the deivce
+ * @module: module creating the device
  *
- * Fill in the fields of the device structure with ethernet-generic
- * values. Basically does everything except registering the device.
+ * Allocates then fills in the fields of a new device structure with
+ * ethernet-generic values. Basically does everything except
+ * registering the device.
  *
  * A 32-byte alignment is enforced for the private data area.
  */
 struct rtnet_device *__rt_alloc_etherdev(unsigned sizeof_priv,
-					unsigned dev_pool_size,
-					struct module *module)
+					 unsigned dev_pool_size,
+					 struct module *module)
 {
     struct rtnet_device *rtdev;
 
@@ -338,21 +395,11 @@ struct rtnet_device *__rt_alloc_etherdev(unsigned sizeof_priv,
     if (!rtdev)
 	return NULL;
 
-    rtdev->hard_header     = rt_eth_header;
-    rtdev->type            = ARPHRD_ETHER;
-    rtdev->hard_header_len = ETH_HLEN;
-    rtdev->mtu             = 1500; /* eth_mtu */
-    rtdev->addr_len        = ETH_ALEN;
-    rtdev->flags           = IFF_BROADCAST; /* TODO: IFF_MULTICAST; */
-    rtdev->get_mtu         = rt_hard_mtu;
-    rtdev->rt_owner	   = module;
-
-    memset(rtdev->broadcast, 0xFF, ETH_ALEN);
-    strcpy(rtdev->name, "rteth%d");
+    init_etherdev(rtdev, module);
 
     return rtdev;
 }
-
+EXPORT_SYMBOL_GPL(__rt_alloc_etherdev);
 
 
 static inline int __rtdev_new_index(void)
@@ -907,9 +954,6 @@ unsigned int rt_hard_mtu(struct rtnet_device *rtdev, unsigned int priority)
 }
 
 
-EXPORT_SYMBOL_GPL(__rt_alloc_etherdev);
-EXPORT_SYMBOL_GPL(rtdev_free);
-
 EXPORT_SYMBOL_GPL(rtdev_alloc_name);
 
 EXPORT_SYMBOL_GPL(rt_register_rtnetdev);
-- 
2.17.2

[PATCH 05/12] net/stack: fresh rtskb should have ip_summed set to CHECKSUM_NONE

[Philippe Gerum]

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 kernel/drivers/net/stack/rtskb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/drivers/net/stack/rtskb.c b/kernel/drivers/net/stack/rtskb.c
index f5609027b..cc7563719 100644
--- a/kernel/drivers/net/stack/rtskb.c
+++ b/kernel/drivers/net/stack/rtskb.c
@@ -223,6 +223,7 @@ struct rtskb *alloc_rtskb(unsigned int size, struct rtskb_pool *pool)
     skb->len = 0;
     skb->pkt_type = PACKET_HOST;
     skb->xmit_stamp = NULL;
+    skb->ip_summed = CHECKSUM_NONE;
 
 #if IS_ENABLED(CONFIG_XENO_DRIVERS_NET_ADDON_RTCAP)
     skb->cap_flags = 0;
-- 
2.17.2

[PATCH 06/12] net/rtdev: ensure per-device skbs get mapped at registration

[Philippe Gerum]

This patch works around a regression introduced by #74464ee37d0,
causing a new device's skbs not to be passed to its ->map_rtskb()
handler when registered, breaking further DMA inits in the driver.

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 kernel/drivers/net/stack/rtdev.c | 37 +++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/kernel/drivers/net/stack/rtdev.c b/kernel/drivers/net/stack/rtdev.c
index 631ca1804..286d08cb1 100644
--- a/kernel/drivers/net/stack/rtdev.c
+++ b/kernel/drivers/net/stack/rtdev.c
@@ -45,6 +45,7 @@ struct rtnet_device         *rtnet_devices[MAX_RT_DEVICES];
 static struct rtnet_device  *loopback_device;
 static DEFINE_RTDM_LOCK(rtnet_devices_rt_lock);
 static LIST_HEAD(rtskb_mapped_list);
+static LIST_HEAD(rtskb_mapwait_list);
 
 LIST_HEAD(event_hook_list);
 DEFINE_MUTEX(rtnet_devices_nrt_lock);
@@ -459,8 +460,12 @@ int rtdev_map_rtskb(struct rtskb *skb)
 	}
     }
 
-    if (!err && skb->buf_dma_addr != RTSKB_UNMAPPED)
-	list_add(&skb->entry, &rtskb_mapped_list);
+    if (!err) {
+	    if (skb->buf_dma_addr != RTSKB_UNMAPPED)
+		    list_add(&skb->entry, &rtskb_mapped_list);
+	    else
+		    list_add(&skb->entry, &rtskb_mapwait_list);
+    }
 
     mutex_unlock(&rtnet_devices_nrt_lock);
 
@@ -471,19 +476,31 @@ int rtdev_map_rtskb(struct rtskb *skb)
 
 static int rtdev_map_all_rtskbs(struct rtnet_device *rtdev)
 {
-    struct rtskb *skb;
-    int err = 0;
+    struct rtskb *skb, *n;
+    int err;
 
     if (!rtdev->map_rtskb)
-	return 0;
+	    return 0;
 
-    list_for_each_entry(skb, &rtskb_mapped_list, entry) {
-	err = rtskb_map(rtdev, skb);
-	if (err)
-	   break;
+    if (!list_empty(&rtskb_mapped_list)) {
+	    list_for_each_entry(skb, &rtskb_mapped_list, entry) {
+		    err = rtskb_map(rtdev, skb);
+		    if (err)
+			    return err;
+	    }
     }
 
-    return err;
+    if (!list_empty(&rtskb_mapwait_list)) {
+	    list_for_each_entry_safe(skb, n, &rtskb_mapwait_list, entry) {
+		    err = rtskb_map(rtdev, skb);
+		    if (err)
+			    return err;
+		    list_del(&skb->entry);
+		    list_add(&skb->entry, &rtskb_mapped_list);
+	    }
+    }
+
+    return 0;
 }
 
 
-- 
2.17.2

[PATCH 07/12] net/udp: getfrag: fix frag preparation status

[Philippe Gerum]

This fixes a regression introduced by #a69c4ac5d, making the UDP stack
basically unusable since no xmit could ever take place due to the
wrong return value.

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 kernel/drivers/net/stack/ipv4/udp/udp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/drivers/net/stack/ipv4/udp/udp.c b/kernel/drivers/net/stack/ipv4/udp/udp.c
index 8e80d3e0b..e580dac08 100644
--- a/kernel/drivers/net/stack/ipv4/udp/udp.c
+++ b/kernel/drivers/net/stack/ipv4/udp/udp.c
@@ -553,8 +553,10 @@ static int rt_udp_getfrag(const void *p, unsigned char *to,
 
 
     // We should optimize this function a bit (copy+csum...)!
-    if (offset)
-	    return rtnet_read_from_iov(ufh->fd, ufh->iov, ufh->iovlen, to, fraglen);
+    if (offset) {
+	    ret = rtnet_read_from_iov(ufh->fd, ufh->iov, ufh->iovlen, to, fraglen);
+	    return ret < 0 ? ret : 0;
+    }
 
     /* Checksum of the complete data part of the UDP message: */
     for (i = 0; i < ufh->iovlen; i++) {
@@ -565,7 +567,7 @@ static int rt_udp_getfrag(const void *p, unsigned char *to,
     ret = rtnet_read_from_iov(ufh->fd, ufh->iov, ufh->iovlen,
 			      to + sizeof(struct udphdr),
 			      fraglen - sizeof(struct udphdr));
-    if (ret)
+    if (ret < 0)
 	    return ret;
 
     /* Checksum of the udp header: */
-- 
2.17.2

[PATCH 08/12] net/udp: getfrag: remove direct reference to user memory

[Philippe Gerum]

This issue triggers domain violation on ARM, SMAP violation on x86.

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 kernel/drivers/net/stack/ipv4/udp/udp.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/kernel/drivers/net/stack/ipv4/udp/udp.c b/kernel/drivers/net/stack/ipv4/udp/udp.c
index e580dac08..d0d35c324 100644
--- a/kernel/drivers/net/stack/ipv4/udp/udp.c
+++ b/kernel/drivers/net/stack/ipv4/udp/udp.c
@@ -549,7 +549,7 @@ static int rt_udp_getfrag(const void *p, unsigned char *to,
                           unsigned int offset, unsigned int fraglen)
 {
     struct udpfakehdr *ufh = (struct udpfakehdr *)p;
-    int i, ret;
+    int ret;
 
 
     // We should optimize this function a bit (copy+csum...)!
@@ -558,18 +558,17 @@ static int rt_udp_getfrag(const void *p, unsigned char *to,
 	    return ret < 0 ? ret : 0;
     }
 
-    /* Checksum of the complete data part of the UDP message: */
-    for (i = 0; i < ufh->iovlen; i++) {
-            ufh->wcheck = csum_partial(ufh->iov[i].iov_base, ufh->iov[i].iov_len,
-                                       ufh->wcheck);
-    }
-
     ret = rtnet_read_from_iov(ufh->fd, ufh->iov, ufh->iovlen,
 			      to + sizeof(struct udphdr),
 			      fraglen - sizeof(struct udphdr));
     if (ret < 0)
 	    return ret;
 
+    /* Checksum of the complete data part of the UDP message: */
+    ufh->wcheck = csum_partial(to + sizeof(struct udphdr),
+			       fraglen - sizeof(struct udphdr),
+			       ufh->wcheck);
+
     /* Checksum of the udp header: */
     ufh->wcheck = csum_partial((unsigned char *)ufh,
 			       sizeof(struct udphdr), ufh->wcheck);
@@ -684,7 +683,7 @@ ssize_t rt_udp_sendmsg(struct rtdm_fd *fd, const struct user_msghdr *msg, int ms
     ufh.uh.len    = htons(ulen);
     ufh.uh.check  = 0;
     ufh.fd        = fd;
-    ufh.iov       = msg->msg_iov;
+    ufh.iov       = iov;
     ufh.iovlen    = msg->msg_iovlen;
     ufh.wcheck    = 0;
 
-- 
2.17.2

[PATCH 09/12] testsuite/smokey: net: do not unload pre-loaded modules

[Philippe Gerum]

We may want to use those tests in pre-configured situations, where a
set of RTnet modules is already in place at startup, which we don't
want the test to remove on exit.

Those changes ensure that only the modules we had to push for running
the test are actually removed on exit, leaving the pre-loaded ones
running.

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 testsuite/smokey/net_common/setup.c | 97 +++++++++++++++++++++++------
 1 file changed, 77 insertions(+), 20 deletions(-)

diff --git a/testsuite/smokey/net_common/setup.c b/testsuite/smokey/net_common/setup.c
index ea3daceca..d3791038f 100644
--- a/testsuite/smokey/net_common/setup.c
+++ b/testsuite/smokey/net_common/setup.c
@@ -28,6 +28,7 @@
 struct module {
 	int option;
 	const char *name;
+	bool loaded;
 };
 
 #define TIMEOUT 10
@@ -37,6 +38,15 @@ static int fd;
 static pthread_t loopback_server_tid;
 static bool loopback_thread_created;
 static struct module modules[] = {
+	{
+		.name = "rtnet",
+	},
+	{
+		.name = "rtipv4",
+	},
+	{
+		.name = "rtcfg",
+	},
 	{
 		.option = _CC_COBALT_NET_UDP,
 		.name = "rtudp",
@@ -45,9 +55,19 @@ static struct module modules[] = {
 		.option = _CC_COBALT_NET_AF_PACKET,
 		.name = "rtpacket",
 	},
+	{
+		.name = NULL,	/* driver */
+	},
 };
 
-static const char *option_to_module(int option)
+#define MODID_RTNET  0
+#define MODID_IPV4   1
+#define MODID_CFG    2
+#define MODID_UDP    3
+#define MODID_PACKET 4
+#define MODID_DRIVER 5
+
+static int option_to_modid(int option)
 {
 	unsigned i;
 
@@ -55,10 +75,10 @@ static const char *option_to_module(int option)
 		if (modules[i].option != option)
 			continue;
 
-		return modules[i].name;
+		return i;
 	}
 
-	return NULL;
+	return -1;
 }
 
 static int get_info(const char *intf)
@@ -118,13 +138,37 @@ static int do_down(const char *intf)
 	return 0;
 }
 
-static int smokey_net_modprobe(const char *mod)
+static int smokey_net_modprobe(int modid)
 {
+	struct module *m = modules + modid;
 	char buffer[128];
-	int err;
+	int err, len;
+	FILE *fp;
+
+	if (modid < 0)
+		return -EINVAL;
+
+	fp = fopen("/proc/modules", "r");
+	if (fp == NULL)
+		return -errno;
+
+	len = strlen(m->name);
+
+	while (fgets(buffer, sizeof(buffer), fp)) {
+		if (strncmp(buffer, m->name, len) == 0 &&
+		    len < sizeof(buffer) && buffer[len] == ' ') {
+			smokey_trace("%s module already loaded", m->name);
+			fclose(fp);
+			return 0;
+		}
+	}
+
+	fclose(fp);
+
+	smokey_trace("%s module not there: modprobing", m->name);
 
 	err = smokey_check_errno(
-		snprintf(buffer, sizeof(buffer), "modprobe %s", mod));
+		snprintf(buffer, sizeof(buffer), "modprobe %s", m->name));
 	if (err < 0)
 		return err;
 
@@ -137,16 +181,26 @@ static int smokey_net_modprobe(const char *mod)
 		return -EINVAL;
 	}
 
+	m->loaded = true;
+
 	return err;
 }
 
-static int smokey_net_rmmod(const char *mod)
+static int smokey_net_rmmod(int modid)
 {
+	struct module *m = modules + modid;
 	char buffer[128];
 	int err;
 
+	if (!m->loaded) {
+		smokey_trace("%s module was there on entry, keeping it", m->name);
+		return 0;
+	}
+
+	smokey_trace("unloading %s module", m->name);
+
 	err = smokey_check_errno(
-		snprintf(buffer, sizeof(buffer), "rmmod %s", mod));
+		snprintf(buffer, sizeof(buffer), "rmmod %s", m->name));
 	if (err < 0)
 		return err;
 
@@ -170,7 +224,7 @@ static int smokey_net_setup_rtcfg_client(const char *intf, int net_config)
 	if ((net_config & _CC_COBALT_NET_CFG) == 0)
 		return -ENOSYS;
 
-	err = smokey_net_modprobe("rtcfg");
+	err = smokey_net_modprobe(MODID_CFG);
 	if (err < 0)
 		return err;
 
@@ -213,11 +267,13 @@ static int smokey_net_teardown_rtcfg(const char *intf)
 	if (err < 0)
 		return err;
 
-	err = smokey_check_errno(ioctl(fd, RTCFG_IOC_DETACH, &cmd));
-	if (err < 0)
-		return err;
+	/*
+	 * We may or may not be acting as a server; don't check the
+	 * status.
+	 */
+	ioctl(fd, RTCFG_IOC_DETACH, &cmd);
 
-	return smokey_net_rmmod("rtcfg");
+	return smokey_net_rmmod(MODID_CFG);
 }
 
 static int find_peer(const char *intf, void *vpeer)
@@ -366,15 +422,16 @@ int smokey_net_setup(const char *driver, const char *intf, int tested_config,
 	if ((net_config & tested_config) == 0)
 		return -ENOSYS;
 
-	err = smokey_net_modprobe(driver);
+	modules[MODID_DRIVER].name = driver;
+	err = smokey_net_modprobe(MODID_DRIVER);
 	if (err < 0)
 		return err;
 
-	err = smokey_net_modprobe("rtipv4");
+	err = smokey_net_modprobe(MODID_IPV4);
 	if (err < 0)
 		return err;
 
-	err = smokey_net_modprobe(option_to_module(tested_config));
+	err = smokey_net_modprobe(option_to_modid(tested_config));
 	if (err < 0)
 		return err;
 
@@ -494,19 +551,19 @@ int smokey_net_teardown(const char *driver, const char *intf, int tested_config)
 	} else
 		err = tmp;
 
-	tmp = smokey_net_rmmod(option_to_module(tested_config));
+	tmp = smokey_net_rmmod(option_to_modid(tested_config));
 	if (err == 0)
 		err = tmp;
 
-	tmp = smokey_net_rmmod(driver);
+	tmp = smokey_net_rmmod(MODID_DRIVER);
 	if (err == 0)
 		err = tmp;
 
-	tmp = smokey_net_rmmod("rtipv4");
+	tmp = smokey_net_rmmod(MODID_IPV4);
 	if (err == 0)
 		err = tmp;
 
-	tmp = smokey_net_rmmod("rtnet");
+	tmp = smokey_net_rmmod(MODID_RTNET);
 	if (err == 0)
 		err = tmp;
 
-- 
2.17.2

[PATCH 10/12] testsuite/smokey: net: do not down a previously running test interface

[Philippe Gerum]

If the test interface is already up on entry, keep it that way on
exit, so that we may run this test multiple times without having to
reconfigure.

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 testsuite/smokey/net_common/setup.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/testsuite/smokey/net_common/setup.c b/testsuite/smokey/net_common/setup.c
index d3791038f..1badabdfb 100644
--- a/testsuite/smokey/net_common/setup.c
+++ b/testsuite/smokey/net_common/setup.c
@@ -36,7 +36,7 @@ struct module {
 static struct rtnet_core_cmd cmd;
 static int fd;
 static pthread_t loopback_server_tid;
-static bool loopback_thread_created;
+static bool loopback_thread_created, ifup;
 static struct module modules[] = {
 	{
 		.name = "rtnet",
@@ -447,6 +447,7 @@ int smokey_net_setup(const char *driver, const char *intf, int tested_config,
 		err = do_up(intf);
 		if (err < 0)
 			goto err;
+		ifup = true;
 	}
 
 	smokey_trace("Waiting for interface %s to be running", intf);
@@ -543,9 +544,11 @@ int smokey_net_teardown(const char *driver, const char *intf, int tested_config)
 				err = tmp;
 		}
 
-		tmp = do_down(intf);
-		if (err == 0)
-			err = tmp;
+		if (ifup) {
+			tmp = do_down(intf);
+			if (err == 0)
+				err = tmp;
+		}
 
 		close(fd);
 	} else
-- 
2.17.2

[PATCH 11/12] net/stack: rtskb: do not run nop locking calls

[Philippe Gerum]

Checking whether calling a lock operation is actually needed is better
wrt I-cache performance.

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 kernel/drivers/net/stack/rtskb.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/kernel/drivers/net/stack/rtskb.c b/kernel/drivers/net/stack/rtskb.c
index cc7563719..a19ef9f2d 100644
--- a/kernel/drivers/net/stack/rtskb.c
+++ b/kernel/drivers/net/stack/rtskb.c
@@ -156,10 +156,11 @@ static struct rtskb *__rtskb_pool_dequeue(struct rtskb_pool *pool)
     struct rtskb_queue *queue = &pool->queue;
     struct rtskb *skb;
 
-    if (!pool->lock_ops->trylock(pool->lock_cookie))
+    if (pool->lock_ops &&
+	!pool->lock_ops->trylock(pool->lock_cookie))
 	    return NULL;
     skb = __rtskb_dequeue(queue);
-    if (skb == NULL)
+    if (skb == NULL && pool->lock_ops)
 	    pool->lock_ops->unlock(pool->lock_cookie);
 
     return skb;
@@ -184,7 +185,8 @@ static void __rtskb_pool_queue_tail(struct rtskb_pool *pool, struct rtskb *skb)
     struct rtskb_queue *queue = &pool->queue;
 
     __rtskb_queue_tail(queue,skb);
-    pool->lock_ops->unlock(pool->lock_cookie);
+    if (pool->lock_ops)
+	    pool->lock_ops->unlock(pool->lock_cookie);
 }
 
 void rtskb_pool_queue_tail(struct rtskb_pool *pool, struct rtskb *skb)
@@ -292,21 +294,6 @@ void kfree_rtskb(struct rtskb *skb)
 EXPORT_SYMBOL_GPL(kfree_rtskb);
 
 
-static int rtskb_nop_pool_trylock(void *cookie)
-{
-    return 1;
-}
-
-static void rtskb_nop_pool_unlock(void *cookie)
-{
-}
-
-static const struct rtskb_pool_lock_ops rtskb_nop_pool_lock_ops = {
-    .trylock = rtskb_nop_pool_trylock,
-    .unlock = rtskb_nop_pool_unlock,
-};
-
-
 /***
  *  rtskb_pool_init
  *  @pool: pool to be initialized
@@ -328,7 +315,7 @@ unsigned int rtskb_pool_init(struct rtskb_pool *pool,
     if (rtskb_pools > rtskb_pools_max)
 	rtskb_pools_max = rtskb_pools;
 
-    pool->lock_ops = lock_ops ?: &rtskb_nop_pool_lock_ops;
+    pool->lock_ops = lock_ops;
     pool->lock_cookie = lock_cookie;
 
     return i;
-- 
2.17.2

[PATCH 12/12] testsuite/smokey: net_client: improve stats readability

[Philippe Gerum]

Visibale with --verbose=2.

Signed-off-by: Philippe Gerum <rpm@xenomai.org>
---
 testsuite/smokey/net_common/client.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/testsuite/smokey/net_common/client.c b/testsuite/smokey/net_common/client.c
index 093282547..d13d72918 100644
--- a/testsuite/smokey/net_common/client.c
+++ b/testsuite/smokey/net_common/client.c
@@ -17,7 +17,7 @@ static pthread_t tid;
 static unsigned long long glost, glate;
 
 static int rcv_packet(struct smokey_net_client *client, int sock, unsigned seq,
-		struct timespec *next_shot, bool last)
+		      struct timespec *next_shot, bool last, int *linesout)
 {
 	static unsigned long long gmin = ~0ULL, gmax = 0, gsum = 0, gcount = 0;
 	static unsigned long long min = ~0ULL, max = 0, sum = 0, count = 0,
@@ -115,16 +115,19 @@ static int rcv_packet(struct smokey_net_client *client, int sock, unsigned seq,
 	glost += lost - late;
 	glate += late;
 
-	smokey_trace("%g pps\t%Lu\t%Lu\t%.03gus\t%.03gus\t%.03gus\t"
-		"| %Lu\t%Lu\t%.03gus\t%.03gus\t%.03gus",
+	if (((*linesout)++ % 20) == 0) {
+		smokey_trace("\n   %-7s%6s%8s%8s%8s%8s%8s%10s",
+			     "PPS", "LOST", "LATE", "MIN", "MAX",
+			     "BEST", "AVG", "WORST");
+		smokey_trace("------------------------------------------------------------------");
+	}
+
+	smokey_trace("%8.2f  %6Ld  %6Ld     %.03g     %.03g     %.03g     %.03g     %.03g",
 		count / (diff / 1000000000.0),
-		lost - late,
-		late,
-		count ? min / 1000.0 : 0,
-		count ? (sum / (double)count) / 1000 : 0,
-		count ? max / 1000.0 : 0,
 		glost,
 		glate,
+		count ? min / 1000.0 : 0,
+		count ? max / 1000.0 : 0,
 		gcount ? gmin / 1000.0 : 0,
 		gcount ? (gsum / (double)gcount) / 1000 : 0,
 		gcount ? gmax / 1000.0 : 0);
@@ -143,11 +146,11 @@ static int rcv_packet(struct smokey_net_client *client, int sock, unsigned seq,
 static int smokey_net_client_loop(struct smokey_net_client *client)
 {
 	struct smokey_net_payload payload;
+	int sock, err, linesout = 0;
 	struct timespec next_shot;
 	struct sched_param prio;
 	char packet[256];
 	long long limit;
-	int sock, err;
 
 	sock = client->create_socket(client);
 	if (sock < 0)
@@ -164,6 +167,9 @@ static int smokey_net_client_loop(struct smokey_net_client *client)
 	if (err < 0)
 		goto err;
 
+	smokey_trace("\nPPS, LOST, LATE: packet count");
+	smokey_trace("MIN, MAX, BEST, AVG, WORST: microseconds");
+
 	limit = (long long)rate * duration;
 	for (payload.seq = 1;
 	     limit <= 0 || payload.seq < limit + 1; payload.seq++) {
@@ -192,7 +198,7 @@ static int smokey_net_client_loop(struct smokey_net_client *client)
 
 		do {
 			err = rcv_packet(client, sock, seq, &next_shot,
-					payload.seq == limit);
+					 payload.seq == limit, &linesout);
 			if (!err)
 				seq = 0;
 		} while (err != -ETIMEDOUT);
-- 
2.17.2

Re: [PATCH 00/12] Assorted updates: RTnet, GPIO, smokey tests

[Jan Kiszka]

On 24.01.19 16:34, Philippe Gerum wrote:
> Patch 1 addresses the issue in the posix_clock fix as discussed in

And I'm still seeing a 100% identical patch and commit log...

> [1]. Patch 6 works around a recent regression in RTnet as mentioned in
> [2] (tested with the IGB driver which implements a map_rtskb
> handler). Both commits are also -stable candidates.
> 
> A couple of other changes extend the GPIO support with a timestamping
> feature, so that we may ask for the date of the latest state change of
> an input pin when reported by an interrupt event.
> 
> The rest is RTnet-related. In addition to patch 6, I would recommend
> picking 7 and 8 for -stable.
> 

Thanks, looking through them now.

Jan

> [1] https://www.xenomai.org/pipermail/xenomai/2019-January/040225.html
> [2] https://www.xenomai.org/pipermail/xenomai/2019-January/040230.html
> 
> Philippe Gerum (12):
>    testsuite/smokey: posix_clock: prevent false positive in
>      time-dependent test
>    drivers/gpio: provide optional timestamped readouts
>    testsuite/gpiotest: enable timestamping on 'timestamp' argument
>    net/stack: allow initializing pre-allocated device structs
>    net/stack: fresh rtskb should have ip_summed set to CHECKSUM_NONE
>    net/rtdev: ensure per-device skbs get mapped at registration
>    net/udp: getfrag: fix frag preparation status
>    net/udp: getfrag: remove direct reference to user memory
>    testsuite/smokey: net: do not unload pre-loaded modules
>    testsuite/smokey: net: do not down a previously running test interface
>    net/stack: rtskb: do not run nop locking calls
>    testsuite/smokey: net_client: improve stats readability
> 
>   include/cobalt/kernel/rtdm/gpio.h          |   1 +
>   include/rtdm/uapi/gpio.h                   |  18 ++-
>   kernel/drivers/gpio/gpio-core.c            |  54 +++++--
>   kernel/drivers/net/stack/include/rtdev.h   |   9 ++
>   kernel/drivers/net/stack/ipv4/udp/udp.c    |  21 +--
>   kernel/drivers/net/stack/rtdev.c           | 167 ++++++++++++++-------
>   kernel/drivers/net/stack/rtskb.c           |  26 +---
>   testsuite/gpiotest/gpiotest.c              |  37 ++++-
>   testsuite/smokey/net_common/client.c       |  26 ++--
>   testsuite/smokey/net_common/setup.c        | 108 ++++++++++---
>   testsuite/smokey/posix-clock/posix-clock.c |   2 +-
>   11 files changed, 325 insertions(+), 144 deletions(-)
> 

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

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

[Jan Kiszka]

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@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.

> +	__s32 value;
> +};
> +
> +#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)
> +#define GPIO_RTIOC_TS		_IOR(RTDM_CLASS_GPIO, 7, int)
>   
>   #define GPIO_TRIGGER_NONE		0x0 /* unspecified */
>   #define GPIO_TRIGGER_EDGE_RISING	0x1
> diff --git a/kernel/drivers/gpio/gpio-core.c b/kernel/drivers/gpio/gpio-core.c
> index 3ce73dbd5..81f9653f1 100644
> --- a/kernel/drivers/gpio/gpio-core.c
> +++ b/kernel/drivers/gpio/gpio-core.c
> @@ -28,7 +28,8 @@ struct rtdm_gpio_chan {
>   	int requested : 1,
>   		has_direction : 1,
>   		is_output : 1,
> -		is_interrupt : 1;
> +	        is_interrupt : 1,
> +		want_timestamp : 1;
>   };
>   
>   static LIST_HEAD(rtdm_gpio_chips);
> @@ -41,6 +42,7 @@ static int gpio_pin_interrupt(rtdm_irq_t *irqh)
>   
>   	pin = rtdm_irq_get_arg(irqh, struct rtdm_gpio_pin);
>   
> +	pin->timestamp = rtdm_clock_read_monotonic();
>   	rtdm_event_signal(&pin->event);
>   
>   	return RTDM_IRQ_HANDLED;
> @@ -187,6 +189,12 @@ static int gpio_pin_ioctl_nrt(struct rtdm_fd *fd,
>   		gpio_free(gpio);
>   		chan->requested = false;
>   		break;
> +	case GPIO_RTIOC_TS:
> +		ret = rtdm_safe_copy_from_user(fd, &val, arg, sizeof(val));
> +		if (ret)
> +			return ret;
> +		chan->want_timestamp = !!val;
> +		break;
>   	default:
>   		return -EINVAL;
>   	}
> @@ -199,11 +207,9 @@ static ssize_t gpio_pin_read_rt(struct rtdm_fd *fd,
>   {
>   	struct rtdm_gpio_chan *chan = rtdm_fd_to_private(fd);
>   	struct rtdm_device *dev = rtdm_fd_device(fd);
> +	struct rtdm_gpio_readout rdo;
>   	struct rtdm_gpio_pin *pin;
> -	int value, ret;
> -
> -	if (len < sizeof(value))
> -		return -EINVAL;
> +	int ret;
>   
>   	if (!chan->has_direction)
>   		return -EAGAIN;
> @@ -213,16 +219,37 @@ static ssize_t gpio_pin_read_rt(struct rtdm_fd *fd,
>   
>   	pin = container_of(dev, struct rtdm_gpio_pin, dev);
>   
> -	if (!(fd->oflags & O_NONBLOCK)) {
> -		ret = rtdm_event_wait(&pin->event);
> -		if (ret)
> -			return ret;
> -	}
> +	if (chan->want_timestamp) {
> +		if (len < sizeof(rdo))
> +			return -EINVAL;
> +
> +		if (!(fd->oflags & O_NONBLOCK)) {
> +			ret = rtdm_event_wait(&pin->event);
> +			if (ret)
> +				return ret;
> +			rdo.timestamp = pin->timestamp;
> +		} else
> +			rdo.timestamp = rtdm_clock_read_monotonic();
> +
> +		len = sizeof(rdo);
> +		rdo.value = gpiod_get_raw_value(pin->desc);
> +		ret = rtdm_safe_copy_to_user(fd, buf, &rdo, len);
> +	} else {
> +		if (len < sizeof(rdo.value))
> +			return -EINVAL;
>   
> -	value = gpiod_get_raw_value(pin->desc);
> -	ret = rtdm_safe_copy_to_user(fd, buf, &value, sizeof(value));
> +		if (!(fd->oflags & O_NONBLOCK)) {
> +			ret = rtdm_event_wait(&pin->event);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		len = sizeof(rdo.value);
> +		rdo.value = gpiod_get_raw_value(pin->desc);
> +		ret = rtdm_safe_copy_to_user(fd, buf, &rdo.value, len);
> +	}
>   	
> -	return ret ?: sizeof(value);
> +	return ret ?: len;
>   }
>   
>   static ssize_t gpio_pin_write_rt(struct rtdm_fd *fd,
> @@ -462,6 +489,7 @@ int rtdm_gpiochip_post_event(struct rtdm_gpio_chip *rgc,
>   		return -EINVAL;
>   
>   	pin = rgc->pins + offset;
> +	pin->timestamp = rtdm_clock_read_monotonic();
>   	rtdm_event_signal(&pin->event);
>   	
>   	return 0;
> 

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 05/12] net/stack: fresh rtskb should have ip_summed set to CHECKSUM_NONE

[Jan Kiszka]

Cleanup or fix?

I agree with the change, but if it's a fix, it should also go into stable.

On 24.01.19 16:34, Philippe Gerum wrote:
> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
> ---
>   kernel/drivers/net/stack/rtskb.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/drivers/net/stack/rtskb.c b/kernel/drivers/net/stack/rtskb.c
> index f5609027b..cc7563719 100644
> --- a/kernel/drivers/net/stack/rtskb.c
> +++ b/kernel/drivers/net/stack/rtskb.c
> @@ -223,6 +223,7 @@ struct rtskb *alloc_rtskb(unsigned int size, struct rtskb_pool *pool)
>       skb->len = 0;
>       skb->pkt_type = PACKET_HOST;
>       skb->xmit_stamp = NULL;
> +    skb->ip_summed = CHECKSUM_NONE;
>   
>   #if IS_ENABLED(CONFIG_XENO_DRIVERS_NET_ADDON_RTCAP)
>       skb->cap_flags = 0;
> 

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 06/12] net/rtdev: ensure per-device skbs get mapped at registration

[Jan Kiszka]

On 24.01.19 16:34, Philippe Gerum wrote:
> This patch works around a regression introduced by #74464ee37d0,
> causing a new device's skbs not to be passed to its ->map_rtskb()
> handler when registered, breaking further DMA inits in the driver.
> 
> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
> ---
>   kernel/drivers/net/stack/rtdev.c | 37 +++++++++++++++++++++++---------
>   1 file changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/drivers/net/stack/rtdev.c b/kernel/drivers/net/stack/rtdev.c
> index 631ca1804..286d08cb1 100644
> --- a/kernel/drivers/net/stack/rtdev.c
> +++ b/kernel/drivers/net/stack/rtdev.c
> @@ -45,6 +45,7 @@ struct rtnet_device         *rtnet_devices[MAX_RT_DEVICES];
>   static struct rtnet_device  *loopback_device;
>   static DEFINE_RTDM_LOCK(rtnet_devices_rt_lock);
>   static LIST_HEAD(rtskb_mapped_list);
> +static LIST_HEAD(rtskb_mapwait_list);
>   
>   LIST_HEAD(event_hook_list);
>   DEFINE_MUTEX(rtnet_devices_nrt_lock);
> @@ -459,8 +460,12 @@ int rtdev_map_rtskb(struct rtskb *skb)
>   	}
>       }
>   
> -    if (!err && skb->buf_dma_addr != RTSKB_UNMAPPED)
> -	list_add(&skb->entry, &rtskb_mapped_list);
> +    if (!err) {
> +	    if (skb->buf_dma_addr != RTSKB_UNMAPPED)
> +		    list_add(&skb->entry, &rtskb_mapped_list);
> +	    else
> +		    list_add(&skb->entry, &rtskb_mapwait_list);
> +    }
>   
>       mutex_unlock(&rtnet_devices_nrt_lock);
>   
> @@ -471,19 +476,31 @@ int rtdev_map_rtskb(struct rtskb *skb)
>   
>   static int rtdev_map_all_rtskbs(struct rtnet_device *rtdev)
>   {
> -    struct rtskb *skb;
> -    int err = 0;
> +    struct rtskb *skb, *n;
> +    int err;
>   
>       if (!rtdev->map_rtskb)
> -	return 0;
> +	    return 0;
>   
> -    list_for_each_entry(skb, &rtskb_mapped_list, entry) {
> -	err = rtskb_map(rtdev, skb);
> -	if (err)
> -	   break;
> +    if (!list_empty(&rtskb_mapped_list)) {

Why this extra check? list_for_each_entry should just do nothing if the list is 
empty.

> +	    list_for_each_entry(skb, &rtskb_mapped_list, entry) {
> +		    err = rtskb_map(rtdev, skb);
> +		    if (err)
> +			    return err;
> +	    }
>       }
>   
> -    return err;
> +    if (!list_empty(&rtskb_mapwait_list)) {

Same here.

> +	    list_for_each_entry_safe(skb, n, &rtskb_mapwait_list, entry) {
> +		    err = rtskb_map(rtdev, skb);
> +		    if (err)
> +			    return err;
> +		    list_del(&skb->entry);
> +		    list_add(&skb->entry, &rtskb_mapped_list);
> +	    }
> +    }
> +
> +    return 0;
>   }
>   
>   
> 

Style mix: Eventually, we should switch all RTnet code to kernel style. For now, 
we have 4-space-indention, and we should keep it until then. Mixing things will 
only make it more messy.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 00/12] Assorted updates: RTnet, GPIO, smokey tests

[Philippe Gerum]

On 1/24/19 7:10 PM, Jan Kiszka wrote:
> On 24.01.19 16:34, Philippe Gerum wrote:
>> Patch 1 addresses the issue in the posix_clock fix as discussed in
> 
> And I'm still seeing a 100% identical patch and commit log...


Please check the short log again:

https://www.xenomai.org/pipermail/xenomai/2019-January/040215.html
vs
https://www.xenomai.org/pipermail/xenomai/2019-January/040238.html

-- 
Philippe.

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

[Philippe Gerum]

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@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.

-- 
Philippe.

Re: [PATCH 00/12] Assorted updates: RTnet, GPIO, smokey tests

[Jan Kiszka]

On 25.01.19 10:12, Philippe Gerum wrote:
> On 1/24/19 7:10 PM, Jan Kiszka wrote:
>> On 24.01.19 16:34, Philippe Gerum wrote:
>>> Patch 1 addresses the issue in the posix_clock fix as discussed in
>>
>> And I'm still seeing a 100% identical patch and commit log...
> 
> 
> Please check the short log again:
> 
> https://www.xenomai.org/pipermail/xenomai/2019-January/040215.html
> vs
> https://www.xenomai.org/pipermail/xenomai/2019-January/040238.html
> 

OK, diff tells me that the only difference is the subject line 
("timing-depending test" -> "time-dependent test").

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

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

[Jan Kiszka]

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@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.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 05/12] net/stack: fresh rtskb should have ip_summed set to CHECKSUM_NONE

[Philippe Gerum]

On 1/24/19 7:21 PM, Jan Kiszka wrote:
> Cleanup or fix?
> 
> I agree with the change, but if it's a fix, it should also go into stable.

There seem to be a general issue with checking for ip_summed. I can figure out how this is supposed to work in the regular IP stack, but I'm unsure about the RTnet logic regarding ip_summed. Specifically, the way rtskb_checksum_none_assert() is implemented puzzles me. I would expect some change along these lines for this code to ever make sense:

diff --git a/kernel/drivers/net/stack/include/rtskb.h b/kernel/drivers/net/stack/include/rtskb.h
index a1793e9a8..2d95d6ff8 100644
--- a/kernel/drivers/net/stack/include/rtskb.h
+++ b/kernel/drivers/net/stack/include/rtskb.h
@@ -282,6 +282,9 @@ extern unsigned int rtskb_amount_max;   /* maximum number of allocated rtskbs */
 #ifdef CONFIG_XENO_DRIVERS_NET_CHECKED
 extern void rtskb_over_panic(struct rtskb *skb, int len, void *here);
 extern void rtskb_under_panic(struct rtskb *skb, int len, void *here);
+#define rtskb_checksum_none_assert(skb) (WARN_ON_ONCE(skb->ip_summed != CHECKSUM_NONE))
+#else
+#define rtskb_checksum_none_assert(skb) do { (void)skb; } while (0)
 #endif
 
 extern struct rtskb *rtskb_pool_dequeue(struct rtskb_pool *pool);
@@ -294,8 +297,6 @@ extern void kfree_rtskb(struct rtskb *skb);
 #define dev_kfree_rtskb(a)  kfree_rtskb(a)
 
 
-#define rtskb_checksum_none_assert(skb) (skb->ip_summed = CHECKSUM_NONE)
-
 static inline void rtskb_tx_timestamp(struct rtskb *skb)
 {
 	nanosecs_abs_t *ts = skb->xmit_stamp;

-- 
Philippe.

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

[Philippe Gerum]

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@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.

-- 
Philippe.

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

[Jan Kiszka]

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@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.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

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

[Philippe Gerum]

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@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.

-- 
Philippe.

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

[Jan Kiszka]

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@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.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

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

[Philippe Gerum]

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@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.

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).

-- 
Philippe.

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

[Jan Kiszka]

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@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.

> 
> 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

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

[Philippe Gerum]

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@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.

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

[Jan Kiszka]

On 25.01.19 12:42, Philippe Gerum wrote:
> 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@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.

I just wanted to clarify the author's intention with and the usage of this type. 
It seems you interpreted those differently, and we never talked about 
assumptions regarding this detail. That's all.

> 
>>>
>>> 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.

I thought you were referring to how nanosecs_*_t is implemented. If you concern 
is how it's being used: We can clarify / state explicitly that it is today and 
will remain tomorrow part of the RTDM ABI. But the fact that it is already ABI 
will not change by implementing things differently only in the GPIO driver.

Thanks for following my change request.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 06/12] net/rtdev: ensure per-device skbs get mapped at registration

[Philippe Gerum]

On 1/24/19 7:24 PM, Jan Kiszka wrote:
> On 24.01.19 16:34, Philippe Gerum wrote:
>> This patch works around a regression introduced by #74464ee37d0,
>> causing a new device's skbs not to be passed to its ->map_rtskb()
>> handler when registered, breaking further DMA inits in the driver.
>>
>> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
>> ---
>>   kernel/drivers/net/stack/rtdev.c | 37 +++++++++++++++++++++++---------
>>   1 file changed, 27 insertions(+), 10 deletions(-)
>>
>> diff --git a/kernel/drivers/net/stack/rtdev.c
>> b/kernel/drivers/net/stack/rtdev.c
>> index 631ca1804..286d08cb1 100644
>> --- a/kernel/drivers/net/stack/rtdev.c
>> +++ b/kernel/drivers/net/stack/rtdev.c
>> @@ -45,6 +45,7 @@ struct rtnet_device        
>> *rtnet_devices[MAX_RT_DEVICES];
>>   static struct rtnet_device  *loopback_device;
>>   static DEFINE_RTDM_LOCK(rtnet_devices_rt_lock);
>>   static LIST_HEAD(rtskb_mapped_list);
>> +static LIST_HEAD(rtskb_mapwait_list);
>>     LIST_HEAD(event_hook_list);
>>   DEFINE_MUTEX(rtnet_devices_nrt_lock);
>> @@ -459,8 +460,12 @@ int rtdev_map_rtskb(struct rtskb *skb)
>>       }
>>       }
>>   -    if (!err && skb->buf_dma_addr != RTSKB_UNMAPPED)
>> -    list_add(&skb->entry, &rtskb_mapped_list);
>> +    if (!err) {
>> +        if (skb->buf_dma_addr != RTSKB_UNMAPPED)
>> +            list_add(&skb->entry, &rtskb_mapped_list);
>> +        else
>> +            list_add(&skb->entry, &rtskb_mapwait_list);
>> +    }
>>         mutex_unlock(&rtnet_devices_nrt_lock);
>>   @@ -471,19 +476,31 @@ int rtdev_map_rtskb(struct rtskb *skb)
>>     static int rtdev_map_all_rtskbs(struct rtnet_device *rtdev)
>>   {
>> -    struct rtskb *skb;
>> -    int err = 0;
>> +    struct rtskb *skb, *n;
>> +    int err;
>>         if (!rtdev->map_rtskb)
>> -    return 0;
>> +        return 0;
>>   -    list_for_each_entry(skb, &rtskb_mapped_list, entry) {
>> -    err = rtskb_map(rtdev, skb);
>> -    if (err)
>> -       break;
>> +    if (!list_empty(&rtskb_mapped_list)) {
> 
> Why this extra check? list_for_each_entry should just do nothing if the
> list is empty.
> 
>> +        list_for_each_entry(skb, &rtskb_mapped_list, entry) {
>> +            err = rtskb_map(rtdev, skb);
>> +            if (err)
>> +                return err;
>> +        }
>>       }
>>   -    return err;
>> +    if (!list_empty(&rtskb_mapwait_list)) {
> 
> Same here.
> 
>> +        list_for_each_entry_safe(skb, n, &rtskb_mapwait_list, entry) {
>> +            err = rtskb_map(rtdev, skb);
>> +            if (err)
>> +                return err;
>> +            list_del(&skb->entry);
>> +            list_add(&skb->entry, &rtskb_mapped_list);
>> +        }
>> +    }
>> +
>> +    return 0;
>>   }
>>    
> 
> Style mix: Eventually, we should switch all RTnet code to kernel style.
> For now, we have 4-space-indention, and we should keep it until then.
> Mixing things will only make it more messy.
> 

There is a massive commit [1] pending in my queue fixing RTnet wrt
coding style, which I'm not going to paste here. This is the result of
filtering the code through scripts/Lindent basically.

Would you agree on merging such kind of commit into RTnet? If so, the
vlan and multicast support I'm currently adapting from Gilles' past work
should go on top of such changes.

[1]
https://lab.xenomai.org/xenomai-rpm.git/commit/?h=for-upstream/next&id=d6cab66ddaf4e01cd9166dc530acc60c5b3bd3c2

-- 
Philippe.

Re: [PATCH 06/12] net/rtdev: ensure per-device skbs get mapped at registration

[Jan Kiszka]

On 06.02.19 10:02, Philippe Gerum wrote:
> On 1/24/19 7:24 PM, Jan Kiszka wrote:
>> On 24.01.19 16:34, Philippe Gerum wrote:
>>> This patch works around a regression introduced by #74464ee37d0,
>>> causing a new device's skbs not to be passed to its ->map_rtskb()
>>> handler when registered, breaking further DMA inits in the driver.
>>>
>>> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
>>> ---
>>>    kernel/drivers/net/stack/rtdev.c | 37 +++++++++++++++++++++++---------
>>>    1 file changed, 27 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/kernel/drivers/net/stack/rtdev.c
>>> b/kernel/drivers/net/stack/rtdev.c
>>> index 631ca1804..286d08cb1 100644
>>> --- a/kernel/drivers/net/stack/rtdev.c
>>> +++ b/kernel/drivers/net/stack/rtdev.c
>>> @@ -45,6 +45,7 @@ struct rtnet_device
>>> *rtnet_devices[MAX_RT_DEVICES];
>>>    static struct rtnet_device  *loopback_device;
>>>    static DEFINE_RTDM_LOCK(rtnet_devices_rt_lock);
>>>    static LIST_HEAD(rtskb_mapped_list);
>>> +static LIST_HEAD(rtskb_mapwait_list);
>>>      LIST_HEAD(event_hook_list);
>>>    DEFINE_MUTEX(rtnet_devices_nrt_lock);
>>> @@ -459,8 +460,12 @@ int rtdev_map_rtskb(struct rtskb *skb)
>>>        }
>>>        }
>>>    -    if (!err && skb->buf_dma_addr != RTSKB_UNMAPPED)
>>> -    list_add(&skb->entry, &rtskb_mapped_list);
>>> +    if (!err) {
>>> +        if (skb->buf_dma_addr != RTSKB_UNMAPPED)
>>> +            list_add(&skb->entry, &rtskb_mapped_list);
>>> +        else
>>> +            list_add(&skb->entry, &rtskb_mapwait_list);
>>> +    }
>>>          mutex_unlock(&rtnet_devices_nrt_lock);
>>>    @@ -471,19 +476,31 @@ int rtdev_map_rtskb(struct rtskb *skb)
>>>      static int rtdev_map_all_rtskbs(struct rtnet_device *rtdev)
>>>    {
>>> -    struct rtskb *skb;
>>> -    int err = 0;
>>> +    struct rtskb *skb, *n;
>>> +    int err;
>>>          if (!rtdev->map_rtskb)
>>> -    return 0;
>>> +        return 0;
>>>    -    list_for_each_entry(skb, &rtskb_mapped_list, entry) {
>>> -    err = rtskb_map(rtdev, skb);
>>> -    if (err)
>>> -       break;
>>> +    if (!list_empty(&rtskb_mapped_list)) {
>>
>> Why this extra check? list_for_each_entry should just do nothing if the
>> list is empty.
>>
>>> +        list_for_each_entry(skb, &rtskb_mapped_list, entry) {
>>> +            err = rtskb_map(rtdev, skb);
>>> +            if (err)
>>> +                return err;
>>> +        }
>>>        }
>>>    -    return err;
>>> +    if (!list_empty(&rtskb_mapwait_list)) {
>>
>> Same here.
>>
>>> +        list_for_each_entry_safe(skb, n, &rtskb_mapwait_list, entry) {
>>> +            err = rtskb_map(rtdev, skb);
>>> +            if (err)
>>> +                return err;
>>> +            list_del(&skb->entry);
>>> +            list_add(&skb->entry, &rtskb_mapped_list);
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>>    }
>>>     
>>
>> Style mix: Eventually, we should switch all RTnet code to kernel style.
>> For now, we have 4-space-indention, and we should keep it until then.
>> Mixing things will only make it more messy.
>>
> 
> There is a massive commit [1] pending in my queue fixing RTnet wrt
> coding style, which I'm not going to paste here. This is the result of
> filtering the code through scripts/Lindent basically.
> 
> Would you agree on merging such kind of commit into RTnet? If so, the
> vlan and multicast support I'm currently adapting from Gilles' past work
> should go on top of such changes.

I would take such cleanup. It's a timing issue, though, because any further 
stable fixes that come after that may be harder to backport. New feature are 
fine afterwards, of course.

Jan

> 
> [1]
> https://lab.xenomai.org/xenomai-rpm.git/commit/?h=for-upstream/next&id=d6cab66ddaf4e01cd9166dc530acc60c5b3bd3c2
> 

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 06/12] net/rtdev: ensure per-device skbs get mapped at registration

[Philippe Gerum]

On 2/6/19 10:08 AM, Jan Kiszka wrote:
> On 06.02.19 10:02, Philippe Gerum wrote:
>> On 1/24/19 7:24 PM, Jan Kiszka wrote:
>>> On 24.01.19 16:34, Philippe Gerum wrote:
>>>> This patch works around a regression introduced by #74464ee37d0,
>>>> causing a new device's skbs not to be passed to its ->map_rtskb()
>>>> handler when registered, breaking further DMA inits in the driver.
>>>>
>>>> Signed-off-by: Philippe Gerum <rpm@xenomai.org>
>>>> ---
>>>>    kernel/drivers/net/stack/rtdev.c | 37
>>>> +++++++++++++++++++++++---------
>>>>    1 file changed, 27 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/kernel/drivers/net/stack/rtdev.c
>>>> b/kernel/drivers/net/stack/rtdev.c
>>>> index 631ca1804..286d08cb1 100644
>>>> --- a/kernel/drivers/net/stack/rtdev.c
>>>> +++ b/kernel/drivers/net/stack/rtdev.c
>>>> @@ -45,6 +45,7 @@ struct rtnet_device
>>>> *rtnet_devices[MAX_RT_DEVICES];
>>>>    static struct rtnet_device  *loopback_device;
>>>>    static DEFINE_RTDM_LOCK(rtnet_devices_rt_lock);
>>>>    static LIST_HEAD(rtskb_mapped_list);
>>>> +static LIST_HEAD(rtskb_mapwait_list);
>>>>      LIST_HEAD(event_hook_list);
>>>>    DEFINE_MUTEX(rtnet_devices_nrt_lock);
>>>> @@ -459,8 +460,12 @@ int rtdev_map_rtskb(struct rtskb *skb)
>>>>        }
>>>>        }
>>>>    -    if (!err && skb->buf_dma_addr != RTSKB_UNMAPPED)
>>>> -    list_add(&skb->entry, &rtskb_mapped_list);
>>>> +    if (!err) {
>>>> +        if (skb->buf_dma_addr != RTSKB_UNMAPPED)
>>>> +            list_add(&skb->entry, &rtskb_mapped_list);
>>>> +        else
>>>> +            list_add(&skb->entry, &rtskb_mapwait_list);
>>>> +    }
>>>>          mutex_unlock(&rtnet_devices_nrt_lock);
>>>>    @@ -471,19 +476,31 @@ int rtdev_map_rtskb(struct rtskb *skb)
>>>>      static int rtdev_map_all_rtskbs(struct rtnet_device *rtdev)
>>>>    {
>>>> -    struct rtskb *skb;
>>>> -    int err = 0;
>>>> +    struct rtskb *skb, *n;
>>>> +    int err;
>>>>          if (!rtdev->map_rtskb)
>>>> -    return 0;
>>>> +        return 0;
>>>>    -    list_for_each_entry(skb, &rtskb_mapped_list, entry) {
>>>> -    err = rtskb_map(rtdev, skb);
>>>> -    if (err)
>>>> -       break;
>>>> +    if (!list_empty(&rtskb_mapped_list)) {
>>>
>>> Why this extra check? list_for_each_entry should just do nothing if the
>>> list is empty.
>>>
>>>> +        list_for_each_entry(skb, &rtskb_mapped_list, entry) {
>>>> +            err = rtskb_map(rtdev, skb);
>>>> +            if (err)
>>>> +                return err;
>>>> +        }
>>>>        }
>>>>    -    return err;
>>>> +    if (!list_empty(&rtskb_mapwait_list)) {
>>>
>>> Same here.
>>>
>>>> +        list_for_each_entry_safe(skb, n, &rtskb_mapwait_list, entry) {
>>>> +            err = rtskb_map(rtdev, skb);
>>>> +            if (err)
>>>> +                return err;
>>>> +            list_del(&skb->entry);
>>>> +            list_add(&skb->entry, &rtskb_mapped_list);
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return 0;
>>>>    }
>>>>     
>>>
>>> Style mix: Eventually, we should switch all RTnet code to kernel style.
>>> For now, we have 4-space-indention, and we should keep it until then.
>>> Mixing things will only make it more messy.
>>>
>>
>> There is a massive commit [1] pending in my queue fixing RTnet wrt
>> coding style, which I'm not going to paste here. This is the result of
>> filtering the code through scripts/Lindent basically.
>>
>> Would you agree on merging such kind of commit into RTnet? If so, the
>> vlan and multicast support I'm currently adapting from Gilles' past work
>> should go on top of such changes.
> 
> I would take such cleanup. It's a timing issue, though, because any
> further stable fixes that come after that may be harder to backport. New
> feature are fine afterwards, of course.
> 

Gilles' work I was referring to are unlikely to go to -stable given the
scope of the changes involved, so I'll target -next instead for this,
based on the reformatting patch.

-- 
Philippe.