[Xenomai-core] CONFIG_XENO_OPT_DEBUG_LEVEL

[Xenomai-core] CONFIG_XENO_OPT_DEBUG_LEVEL

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 360 bytes --]

Hi,

as I started to actually use XENO_ASSERT, I noticed that there is no
infrastructure yet to enable it. CONFIG_XENO_OPT_DEBUG_LEVEL is nowhere
defined. Add this as integer to Kconfig? Or better convert
nucleus/assert.h to CONFIG_OPT_DEBUG for now until we really feel like
we need more than on/off for this? I would vote for the latter ATM.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [Xenomai-core] CONFIG_XENO_OPT_DEBUG_LEVEL

[Philippe Gerum]

Jan Kiszka wrote:
> Hi,
> 
> as I started to actually use XENO_ASSERT, I noticed that there is no
> infrastructure yet to enable it. CONFIG_XENO_OPT_DEBUG_LEVEL is nowhere
> defined. Add this as integer to Kconfig? Or better convert
> nucleus/assert.h to CONFIG_OPT_DEBUG for now until we really feel like
> we need more than on/off for this? I would vote for the latter ATM.

Actually, we already need more than a simple switch: I'd really like the 
queue debugging option to become a level on its own (say, 4294967295?). 
I'll add this tomorrow.

> 
> Jan
> 
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Xenomai-core mailing list
> Xenomai-core@domain.hid
> https://mail.gna.org/listinfo/xenomai-core


-- 

Philippe.

Re: [Xenomai-core] CONFIG_XENO_OPT_DEBUG_LEVEL

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1918 bytes --]

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Hi,
>>
>> as I started to actually use XENO_ASSERT, I noticed that there is no
>> infrastructure yet to enable it. CONFIG_XENO_OPT_DEBUG_LEVEL is nowhere
>> defined. Add this as integer to Kconfig? Or better convert
>> nucleus/assert.h to CONFIG_OPT_DEBUG for now until we really feel like
>> we need more than on/off for this? I would vote for the latter ATM.
> 
> Actually, we already need more than a simple switch: I'd really like the
> queue debugging option to become a level on its own (say, 4294967295?).
> I'll add this tomorrow.

Hmm, raises my old concern again: this "vertical" debugging implies to
switch everything on when you only want queue debugging. I still think
we rather need "horizontal" control: switch on queues, asserts, ...

This level "4294967295" indicates for me where we may end with: dozens
of debug levels no one can tell apart, and you have to switch them all
on to gain the relevant pieces or to be sure that you didn't missed
anything. I always have the mess in mind we once had in ndiswrapper: for
serious debugging of the USB layer you had to raise the debug level
which dragged in bulks of (in this case) useless reports from other
subsystems.

A student (Marc Kleine-Budde) once ported some nice debug subsystem into
an internal project that requested a subsystem ID for every debug
statement (I think it came from kaffe). At compilation time or even
later during runtime you could easily select which subsystem should
start babbling and checking and which one is not that interesting for a
specific test. I think this is far more useful than debug levels. Queues
could become such a subsystem, RTDM (with its asserts) another, and so
forth. Of course, this means maintaining those subsystem IDs at a
central place, but it's clearer than deciding which level to pick for a
new debug code.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [Xenomai-core] CONFIG_XENO_OPT_DEBUG_LEVEL

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
> 
>>Jan Kiszka wrote:
>>
>>>Hi,
>>>
>>>as I started to actually use XENO_ASSERT, I noticed that there is no
>>>infrastructure yet to enable it. CONFIG_XENO_OPT_DEBUG_LEVEL is nowhere
>>>defined. Add this as integer to Kconfig? Or better convert
>>>nucleus/assert.h to CONFIG_OPT_DEBUG for now until we really feel like
>>>we need more than on/off for this? I would vote for the latter ATM.
>>
>>Actually, we already need more than a simple switch: I'd really like the
>>queue debugging option to become a level on its own (say, 4294967295?).
>>I'll add this tomorrow.
> 
> 
> Hmm, raises my old concern again: this "vertical" debugging implies to
> switch everything on when you only want queue debugging. I still think
> we rather need "horizontal" control: switch on queues, asserts, ...

It just depends whether we want to make the assertion mechanism a 
formalized exported feature or not. If there is a limited number of 
"clients", the simpler the better. This said, implementing sub-system 
debug switches instead of global debug levels would still be pretty 
trivial, provided we keep the number of sub-systems within reasonable 
limits (e.g. <= 64).

> 
> This level "4294967295" indicates for me where we may end with: dozens
> of debug levels no one can tell apart, and you have to switch them all
> on to gain the relevant pieces or to be sure that you didn't missed
> anything. I always have the mess in mind we once had in ndiswrapper: for
> serious debugging of the USB layer you had to raise the debug level
> which dragged in bulks of (in this case) useless reports from other
> subsystems.
> 
> A student (Marc Kleine-Budde) once ported some nice debug subsystem into
> an internal project that requested a subsystem ID for every debug
> statement (I think it came from kaffe). At compilation time or even
> later during runtime you could easily select which subsystem should
> start babbling and checking and which one is not that interesting for a
> specific test. I think this is far more useful than debug levels. Queues
> could become such a subsystem, RTDM (with its asserts) another, and so
> forth. Of course, this means maintaining those subsystem IDs at a
> central place, but it's clearer than deciding which level to pick for a
> new debug code.
> 
> Jan
> 


-- 

Philippe.

Re: [Xenomai-core] CONFIG_XENO_OPT_DEBUG_LEVEL

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1823 bytes --]

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>
>>> Jan Kiszka wrote:
>>>
>>>> Hi,
>>>>
>>>> as I started to actually use XENO_ASSERT, I noticed that there is no
>>>> infrastructure yet to enable it. CONFIG_XENO_OPT_DEBUG_LEVEL is nowhere
>>>> defined. Add this as integer to Kconfig? Or better convert
>>>> nucleus/assert.h to CONFIG_OPT_DEBUG for now until we really feel like
>>>> we need more than on/off for this? I would vote for the latter ATM.
>>>
>>> Actually, we already need more than a simple switch: I'd really like the
>>> queue debugging option to become a level on its own (say, 4294967295?).
>>> I'll add this tomorrow.
>>
>>
>> Hmm, raises my old concern again: this "vertical" debugging implies to
>> switch everything on when you only want queue debugging. I still think
>> we rather need "horizontal" control: switch on queues, asserts, ...
> 
> It just depends whether we want to make the assertion mechanism a
> formalized exported feature or not. If there is a limited number of
> "clients", the simpler the better. This said, implementing sub-system
> debug switches instead of global debug levels would still be pretty
> trivial, provided we keep the number of sub-systems within reasonable
> limits (e.g. <= 64).
> 

Here is an attempt to base the activation of XENO_ASSERT on a subsystem
debug switch at compile time, applied on RTDM.

The procedure of adding a new system for XENO_ASSERT usage would be to
define the required kbuild switch according to the naming scheme
CONFIG_XENO_OPT_DEBUG_subsystem, include nucleus/assert.h, and add
"#define CONFIG_XENO_OPT_DEBUG_subsystem 0" for the unset case.
XENO_ASSERT is new called with (subsystem, condition, action). If we
invent further debugging macros, they could be controlled in a similar way.

What do you think?

Jan

[-- Attachment #2: rtdm-asserts.patch --]
[-- Type: text/plain, Size: 9876 bytes --]

Index: include/rtdm/rtdm_driver.h
===================================================================
--- include/rtdm/rtdm_driver.h	(Revision 793)
+++ include/rtdm/rtdm_driver.h	(Arbeitskopie)
@@ -40,6 +40,13 @@
 #include <nucleus/synch.h>
 #include <rtdm/rtdm.h>
 
+/* debug support */
+#include <nucleus/assert.h>
+
+#ifndef CONFIG_XENO_OPT_DEBUG_RTDM
+#define CONFIG_XENO_OPT_DEBUG_RTDM  0
+#endif
+
 
 struct rtdm_dev_context;
 
@@ -891,6 +898,7 @@ static inline rtdm_task_t *rtdm_task_cur
 
 static inline int rtdm_task_wait_period(void)
 {
+    XENO_ASSERT(RTDM, !xnpod_unblockable_p(), return -EPERM;);
     return xnpod_wait_thread_period(NULL);
 }
 
Index: include/nucleus/assert.h
===================================================================
--- include/nucleus/assert.h	(Revision 793)
+++ include/nucleus/assert.h	(Arbeitskopie)
@@ -22,20 +22,12 @@
 
 #include <nucleus/compiler.h>
 
-#ifndef CONFIG_XENO_OPT_DEBUG_LEVEL
-#define CONFIG_XENO_OPT_DEBUG_LEVEL  0
-#endif
-
-#if CONFIG_XENO_OPT_DEBUG_LEVEL > 0
-#define XENO_ASSERT(cond,action)  do { \
-if (unlikely((cond) != 0)) \
+#define XENO_ASSERT(subsystem,cond,action)  do { \
+if (unlikely(CONFIG_XENO_OPT_DEBUG_##subsystem > 0 && !(cond))) \
 	do { action; } while(0); \
 } while(0)
-#else  /* CONFIG_XENO_OPT_DEBUG_LEVEL == 0 */
-#define XENO_ASSERT(cond,action) do { } while(0)
-#endif /* CONFIG_XENO_OPT_DEBUG_LEVEL > 0 */
 
-#define XENO_BUGON(cond)  \
-    XENO_ASSERT(cond,xnpod_fatal("assertion failed at %s:%d",__FILE__,__LINE__))
+#define XENO_BUGON(subsystem,cond)  \
+    XENO_ASSERT(subsystem,cond,xnpod_fatal("assertion failed at %s:%d",__FILE__,__LINE__))
 
 #endif /* !_XENO_NUCLEUS_ASSERT_H */
Index: ChangeLog
===================================================================
--- ChangeLog	(Revision 793)
+++ ChangeLog	(Arbeitskopie)
@@ -1,3 +1,13 @@
+2006-03-26  Jan Kiszka  <jan.kiszka@domain.hid>
+
+	* ksrc/include/nucleus/assert.h: XENO_ASSERT with built-in
+	activation at compile time.
+
+	* ksrc/skins/rtdm/Kconfig, ksrc/skins/rtdm/Config.in,
+	ksrc/skins/rtdm/drvlib.c, include/rtdm/rtdm_driver.h: Use
+	XENO_ASSERT to check for correct RTDM driver API invocation
+	contexts.
+
 2006-03-24  Gilles Chanteperdrix  <gilles.chanteperdrix@xenomai.org>
 
 	* ksrc/nucleus/pod.c (xnpod_init): Return immediately with an
Index: ksrc/skins/rtdm/Kconfig
===================================================================
--- ksrc/skins/rtdm/Kconfig	(Revision 793)
+++ ksrc/skins/rtdm/Kconfig	(Arbeitskopie)
@@ -7,3 +7,12 @@ config XENO_SKIN_RTDM
 	This API skin allows to write real-time drivers against a common
 	light weight interface in kernel mode, but use them across all other
 	skins in both kernel and user mode.
+
+config XENO_OPT_DEBUG_RTDM
+    bool "RTDM Debugging support"
+    depends on XENO_OPT_DEBUG && XENO_SKIN_RTDM
+    help
+
+    This option activates debugging checks for the RTDM subsystem.
+    It is a recommended option for analysing potential issues in RTDM
+    drivers. A minor runtime overhead is added.
Index: ksrc/skins/rtdm/drvlib.c
===================================================================
--- ksrc/skins/rtdm/drvlib.c	(Revision 793)
+++ ksrc/skins/rtdm/drvlib.c	(Arbeitskopie)
@@ -279,6 +279,8 @@ void rtdm_task_join_nrt(rtdm_task_t *tas
     spl_t s;
 
 
+    XENO_ASSERT(RTDM, xnpod_root_p(), return;);
+
     xnlock_get_irqsave(&nklock, s);
 
     while (!xnthread_test_flags(task, XNZOMBIE)) {
@@ -305,6 +307,9 @@ EXPORT_SYMBOL(rtdm_task_join_nrt);
  * - -EINTR is returned if calling task has been unblock by a signal or
  * explicitely via rtdm_task_unblock().
  *
+ * - -EPERM @e may be returned if an illegal invocation environment is
+ * detected.
+ *
  * Environments:
  *
  * This service can be called from:
@@ -319,6 +324,8 @@ int rtdm_task_sleep(uint64_t delay)
     xnthread_t  *thread = xnpod_current_thread();
 
 
+    XENO_ASSERT(RTDM, !xnpod_unblockable_p(), return -EPERM;);
+
     xnpod_suspend_thread(thread, XNDELAY, xnpod_ns2ticks(delay), NULL);
 
     return xnthread_test_flags(thread, XNBREAK) ? -EINTR : 0;
@@ -337,6 +344,9 @@ EXPORT_SYMBOL(rtdm_task_sleep);
  * - -EINTR is returned if calling task has been unblock by a signal or
  * explicitely via rtdm_task_unblock().
  *
+ * - -EPERM @e may be returned if an illegal invocation environment is
+ * detected.
+ *
  * Environments:
  *
  * This service can be called from:
@@ -354,6 +364,8 @@ int rtdm_task_sleep_until(uint64_t wakeu
     int         err = 0;
 
 
+    XENO_ASSERT(RTDM, !xnpod_unblockable_p(), return -EPERM;);
+
     xnlock_get_irqsave(&nklock, s);
 
     delay = xnpod_ns2ticks(wakeup_time) - xnpod_get_time();
@@ -626,6 +638,9 @@ EXPORT_SYMBOL(rtdm_event_signal);
  *
  * - -EIDRM is returned if @a event has been destroyed.
  *
+ * - -EPERM @e may be returned if an illegal invocation environment is
+ * detected.
+ *
  * Environments:
  *
  * This service can be called from:
@@ -641,6 +656,8 @@ int rtdm_event_wait(rtdm_event_t *event)
     int     err = 0;
 
 
+    XENO_ASSERT(RTDM, !xnpod_unblockable_p(), return -EPERM;);
+
     xnlock_get_irqsave(&nklock, s);
 
     if (testbits(event->synch_base.status, SYNCH_DELETED))
@@ -689,6 +706,9 @@ EXPORT_SYMBOL(rtdm_event_wait);
  *
  * - -EIDRM is returned if @a event has been destroyed.
  *
+ * - -EPERM @e may be returned if an illegal invocation environment is
+ * detected.
+ *
  * Environments:
  *
  * This service can be called from:
@@ -706,6 +726,8 @@ int rtdm_event_timedwait(rtdm_event_t *e
     int         err = 0;
 
 
+    XENO_ASSERT(RTDM, !xnpod_unblockable_p(), return -EPERM;);
+
     xnlock_get_irqsave(&nklock, s);
 
     if (unlikely(testbits(event->synch_base.status, SYNCH_DELETED)))
@@ -810,6 +832,9 @@ void rtdm_sem_destroy(rtdm_sem_t *sem);
  *
  * - -EIDRM is returned if @a sem has been destroyed.
  *
+ * - -EPERM @e may be returned if an illegal invocation environment is
+ * detected.
+ *
  * Environments:
  *
  * This service can be called from:
@@ -825,6 +850,8 @@ int rtdm_sem_down(rtdm_sem_t *sem)
     int     err = 0;
 
 
+    XENO_ASSERT(RTDM, !xnpod_unblockable_p(), return -EPERM;);
+
     xnlock_get_irqsave(&nklock, s);
 
     if (testbits(sem->synch_base.status, SYNCH_DELETED))
@@ -878,6 +905,9 @@ EXPORT_SYMBOL(rtdm_sem_down);
  *
  * - -EIDRM is returned if @a sem has been destroyed.
  *
+ * - -EPERM @e may be returned if an illegal invocation environment is
+ * detected.
+ *
  * Environments:
  *
  * This service can be called from:
@@ -895,6 +925,8 @@ int rtdm_sem_timeddown(rtdm_sem_t *sem, 
     int         err = 0;
 
 
+    XENO_ASSERT(RTDM, !xnpod_unblockable_p(), return -EPERM;);
+
     xnlock_get_irqsave(&nklock, s);
 
     if (testbits(sem->synch_base.status, SYNCH_DELETED))
@@ -1035,6 +1067,9 @@ void rtdm_mutex_destroy(rtdm_mutex_t *mu
  *
  * - -EIDRM is returned if @a mutex has been destroyed.
  *
+ * - -EPERM @e may be returned if an illegal invocation environment is
+ * detected.
+ *
  * Environments:
  *
  * This service can be called from:
@@ -1050,6 +1085,8 @@ int rtdm_mutex_lock(rtdm_mutex_t *mutex)
     int     err = 0;
 
 
+    XENO_ASSERT(RTDM, !xnpod_unblockable_p(), return -EPERM;);
+
     xnlock_get_irqsave(&nklock, s);
 
     if (testbits(mutex->synch_base.status, SYNCH_DELETED))
@@ -1097,6 +1134,9 @@ EXPORT_SYMBOL(rtdm_mutex_lock);
  *
  * - -EIDRM is returned if @a mutex has been destroyed.
  *
+ * - -EPERM @e may be returned if an illegal invocation environment is
+ * detected.
+ *
  * Environments:
  *
  * This service can be called from:
@@ -1114,6 +1154,8 @@ int rtdm_mutex_timedlock(rtdm_mutex_t *m
     int         err = 0;
 
 
+    XENO_ASSERT(RTDM, !xnpod_unblockable_p(), return -EPERM;);
+
     xnlock_get_irqsave(&nklock, s);
 
     if (testbits(mutex->synch_base.status, SYNCH_DELETED))
@@ -1179,6 +1221,8 @@ void rtdm_mutex_unlock(rtdm_mutex_t *mut
     spl_t s;
 
 
+    XENO_ASSERT(RTDM, !xnpod_asynch_p(), return;);
+
     xnlock_get_irqsave(&nklock, s);
 
     __clear_bit(0, &mutex->locked);
@@ -1426,6 +1470,9 @@ static struct file_operations rtdm_mmap_
  * - -EAGAIN is returned if too much memory has been already locked by the
  * user process.
  *
+ * - -EPERM @e may be returned if an illegal invocation environment is
+ * detected.
+ *
  * @note RTDM supports two models for unmapping the user memory range again.
  * One is explicite unmapping via rtdm_munmap(), either performed when the
  * user requests it via an IOCTL etc. or when the related device is closed.
@@ -1457,6 +1504,9 @@ int rtdm_mmap_to_user(rtdm_user_info_t *
     void                    *old_priv_data;
     void                    *user_ptr;
 
+
+    XENO_ASSERT(RTDM, xnpod_root_p(), return -EPERM;);
+
     filp = filp_open("/dev/zero", O_RDWR, 0);
     if (IS_ERR(filp))
         return PTR_ERR(filp);
@@ -1499,6 +1549,9 @@ EXPORT_SYMBOL(rtdm_mmap_to_user);
  *
  * - -EINVAL is returned if an invalid address or size was passed.
  *
+ * - -EPERM @e may be returned if an illegal invocation environment is
+ * detected.
+ *
  * Environments:
  *
  * This service can be called from:
@@ -1512,6 +1565,9 @@ int rtdm_munmap(rtdm_user_info_t *user_i
 {
     int err;
 
+
+    XENO_ASSERT(RTDM, xnpod_root_p(), return -EPERM;);
+
     down_write(&user_info->mm->mmap_sem);
     err = do_munmap(user_info->mm, (unsigned long)ptr, len);
     up_write(&user_info->mm->mmap_sem);
Index: ksrc/skins/rtdm/Config.in
===================================================================
--- ksrc/skins/rtdm/Config.in	(Revision 793)
+++ ksrc/skins/rtdm/Config.in	(Arbeitskopie)
@@ -3,3 +3,10 @@
 #
 
 dep_tristate 'Real-Time Driver Model' CONFIG_XENO_SKIN_RTDM $CONFIG_XENO_OPT_NUCLEUS
+
+if [ "$CONFIG_XENO_SKIN_RTDM" != "n" ]; then
+	mainmenu_option next_comment
+	comment 'RTDM interface options'
+	dep_bool 'Debugging support' CONFIG_XENO_OPT_DEBUG_RTDM $CONFIG_XENO_OPT_DEBUG
+	endmenu
+fi

Re: [Xenomai-core] CONFIG_XENO_OPT_DEBUG_LEVEL

[Philippe Gerum]

Jan Kiszka wrote:
> 
> 
> Here is an attempt to base the activation of XENO_ASSERT on a subsystem
> debug switch at compile time, applied on RTDM.
> 
> The procedure of adding a new system for XENO_ASSERT usage would be to
> define the required kbuild switch according to the naming scheme
> CONFIG_XENO_OPT_DEBUG_subsystem, include nucleus/assert.h, and add
> "#define CONFIG_XENO_OPT_DEBUG_subsystem 0" for the unset case.
> XENO_ASSERT is new called with (subsystem, condition, action). If we
> invent further debugging macros, they could be controlled in a similar way.
> 
> What do you think?
> 

I like the flexibity this approach brings (even if I think that the 
context checking in drvlib.c should be done unconditionally, regardless 
of the debug mode -- but that's probably a matter of personal taste). 
I've merged this patch, so that we can further use this debug 
infrastructure in other sub-systems. Thanks.

-- 

Philippe.