[Xenomai-core] [patch, minor] irq proc output in intr.c

[Xenomai-core] [patch, minor] irq proc output in intr.c

[Dmitry Adamushko]

[-- Attachment #1.1: Type: text/plain, Size: 494 bytes --]

On 19/08/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>
> just realised that the output of /proc/xenomai/irq is suboptimal (no
> names) under !CONFIG_XENO_OPT_SHIRQ_LEVEL &&
> !CONFIG_XENO_OPT_SHIRQ_EDGE...



It should be ok but just in case, Jan, pls give it a try first. I was not
able to test a branch with rthal_irq_cookie() (but as I recall it worked
when it was implemented) as I don't have any driver at hand to be loaded.

TIA,


Thanks,
> Jan
>
>

-- 
Best regards,
Dmitry Adamushko

[-- Attachment #1.2: Type: text/html, Size: 1053 bytes --]

[-- Attachment #2: ext-proc.patch --]
[-- Type: text/x-patch, Size: 1857 bytes --]

diff -urp xenomai-SVN/include/nucleus/intr.h xenomai-a/include/nucleus/intr.h
--- xenomai-SVN/include/nucleus/intr.h	2006-07-20 11:09:01.000000000 +0200
+++ xenomai-a/include/nucleus/intr.h	2006-08-22 09:32:24.000000000 +0200
@@ -71,7 +71,9 @@ int xnintr_mount(void);
 
 void xnintr_clock_handler(void);
 
+#if defined(CONFIG_PROC_FS) && defined(__KERNEL__)
 int xnintr_irq_proc(unsigned int irq, char *str);
+#endif /* CONFIG_PROC_FS && __KERNEL__ */
 
     /* Public interface. */
 
diff -urp xenomai-SVN/ksrc/nucleus/intr.c xenomai-a/ksrc/nucleus/intr.c
--- xenomai-SVN/ksrc/nucleus/intr.c	2006-07-20 12:35:40.000000000 +0200
+++ xenomai-a/ksrc/nucleus/intr.c	2006-08-22 09:34:28.000000000 +0200
@@ -691,6 +691,7 @@ int xnintr_mount(void)
 	return 0;
 }
 
+#if defined(CONFIG_PROC_FS) && defined(__KERNEL__)
 int xnintr_irq_proc(unsigned int irq, char *str)
 {
 	xnintr_shirq_t *shirq;
@@ -727,6 +728,7 @@ int xnintr_irq_proc(unsigned int irq, ch
 
 	return p - str;
 }
+#endif /* CONFIG_PROC_FS && __KERNEL__ */
 
 #else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
 
@@ -735,10 +737,31 @@ int xnintr_mount(void)
 	return 0;
 }
 
+#if defined(CONFIG_PROC_FS) && defined(__KERNEL__)
 int xnintr_irq_proc(unsigned int irq, char *str)
 {
-	return 0;
+	xnintr_t *intr;
+	char *p = str;
+
+	if (rthal_virtual_irq_p(irq)) {
+		p += sprintf(p, "         [virtual]");
+		return p - str;
+	} else if (irq == XNARCH_TIMER_IRQ) {
+		p += sprintf(p, "         %s", nkclock.name);
+		return p - str;
+	}
+
+	xnlock_get_irqsave(&nklock, s);
+
+	intr = rthal_irq_cookie(&rthal_domain, irq);
+	if (intr && *(intr->name))
+		p += sprintf(p, "        %s", intr->name);
+
+	xnlock_put_irqrestore(&nklock, s);
+
+	return p - str;
 }
+#endif /* CONFIG_PROC_FS && __KERNEL__ */
 
 #endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
 

[-- Attachment #3: ChangeLog.patch --]
[-- Type: text/x-patch, Size: 439 bytes --]

--- ChangeLog-SVN	2006-08-22 02:14:43.000000000 +0200
+++ ChangeLog	2006-08-22 02:19:21.000000000 +0200
@@ -10,6 +10,9 @@
 
 	* ksrc/nucleus/pipe.c (xnpipe_recv): Re-evaluate the timeout value
 	as we iterate over the message acquisition.
+	
+	* ksrc/nucleus/intr.c (xnintr_irq_proc): Print the names of registered
+	handlers also in case of non-shared interrupts.
 
 2006-08-20  Gilles Chanteperdrix  <gilles.chanteperdrix@laposte.net>
 

[Xenomai-core] Re: [patch, minor] irq proc output in intr.c

[Jan Kiszka]

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

Dmitry Adamushko wrote:
> On 19/08/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>
>> just realised that the output of /proc/xenomai/irq is suboptimal (no
>> names) under !CONFIG_XENO_OPT_SHIRQ_LEVEL &&
>> !CONFIG_XENO_OPT_SHIRQ_EDGE...
> 
> 
> 
> It should be ok but just in case, Jan, pls give it a try first. I was not
> able to test a branch with rthal_irq_cookie() (but as I recall it worked
> when it was implemented) as I don't have any driver at hand to be loaded.

Thanks for the patch. I'm posting some comments below.

> 
> TIA,
> 
> 
> Thanks,
>> Jan
>>
>>
> 
> 
> ------------------------------------------------------------------------
> 
> diff -urp xenomai-SVN/include/nucleus/intr.h xenomai-a/include/nucleus/intr.h
> --- xenomai-SVN/include/nucleus/intr.h	2006-07-20 11:09:01.000000000 +0200
> +++ xenomai-a/include/nucleus/intr.h	2006-08-22 09:32:24.000000000 +0200
> @@ -71,7 +71,9 @@ int xnintr_mount(void);
>  
>  void xnintr_clock_handler(void);
>  
> +#if defined(CONFIG_PROC_FS) && defined(__KERNEL__)
>  int xnintr_irq_proc(unsigned int irq, char *str);
> +#endif /* CONFIG_PROC_FS && __KERNEL__ */
>  
>      /* Public interface. */
>  
> diff -urp xenomai-SVN/ksrc/nucleus/intr.c xenomai-a/ksrc/nucleus/intr.c
> --- xenomai-SVN/ksrc/nucleus/intr.c	2006-07-20 12:35:40.000000000 +0200
> +++ xenomai-a/ksrc/nucleus/intr.c	2006-08-22 09:34:28.000000000 +0200
> @@ -691,6 +691,7 @@ int xnintr_mount(void)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_PROC_FS) && defined(__KERNEL__)
>  int xnintr_irq_proc(unsigned int irq, char *str)
>  {
>  	xnintr_shirq_t *shirq;
> @@ -727,6 +728,7 @@ int xnintr_irq_proc(unsigned int irq, ch
>  
>  	return p - str;
>  }
> +#endif /* CONFIG_PROC_FS && __KERNEL__ */
>  
>  #else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
>  
> @@ -735,10 +737,31 @@ int xnintr_mount(void)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_PROC_FS) && defined(__KERNEL__)
>  int xnintr_irq_proc(unsigned int irq, char *str)
>  {
> -	return 0;
> +	xnintr_t *intr;
> +	char *p = str;

+	spl_t s;

> +
> +	if (rthal_virtual_irq_p(irq)) {
> +		p += sprintf(p, "         [virtual]");
> +		return p - str;
> +	} else if (irq == XNARCH_TIMER_IRQ) {
> +		p += sprintf(p, "         %s", nkclock.name);
> +		return p - str;
> +	}
> +
> +	xnlock_get_irqsave(&nklock, s);

What's the idea of this lock? I'm asking as xnintr_attach/detach should
not run under nklock, right? So, how can it protect us then?

> +
> +	intr = rthal_irq_cookie(&rthal_domain, irq);
> +	if (intr && *(intr->name))
> +		p += sprintf(p, "        %s", intr->name);
> +
> +	xnlock_put_irqrestore(&nklock, s);
> +
> +	return p - str;
>  }
> +#endif /* CONFIG_PROC_FS && __KERNEL__ */
>  
>  #endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
>  
> 
> 
> ------------------------------------------------------------------------
> 
> --- ChangeLog-SVN	2006-08-22 02:14:43.000000000 +0200
> +++ ChangeLog	2006-08-22 02:19:21.000000000 +0200
> @@ -10,6 +10,9 @@
>  
>  	* ksrc/nucleus/pipe.c (xnpipe_recv): Re-evaluate the timeout value
>  	as we iterate over the message acquisition.
> +	
> +	* ksrc/nucleus/intr.c (xnintr_irq_proc): Print the names of registered
> +	handlers also in case of non-shared interrupts.
>  
>  2006-08-20  Gilles Chanteperdrix  <gilles.chanteperdrix@xenomai.org>
>  

Jan


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

[Xenomai-core] Re: [patch, minor] irq proc output in intr.c

[Dmitry Adamushko]

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

On 22/08/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>
> Dmitry Adamushko wrote:
> .....
> >
> > diff -urp xenomai-SVN/include/nucleus/intr.h
> xenomai-a/include/nucleus/intr.h
> > --- xenomai-SVN/include/nucleus/intr.h        2006-07-20 11:09:
> 01.000000000 +0200
> > +++ xenomai-a/include/nucleus/intr.h  2006-08-22 09:32:24.000000000+0200
> > @@ -71,7 +71,9 @@ int xnintr_mount(void);
> >
> >  void xnintr_clock_handler(void);
> >
> > +#if defined(CONFIG_PROC_FS) && defined(__KERNEL__)
> >  int xnintr_irq_proc(unsigned int irq, char *str);
> > +#endif /* CONFIG_PROC_FS && __KERNEL__ */
> >
> >      /* Public interface. */
> >
> > diff -urp xenomai-SVN/ksrc/nucleus/intr.c xenomai-a/ksrc/nucleus/intr.c
> > --- xenomai-SVN/ksrc/nucleus/intr.c   2006-07-20 12:35:40.000000000+0200
> > +++ xenomai-a/ksrc/nucleus/intr.c     2006-08-22 09:34:28.000000000+0200
> > @@ -691,6 +691,7 @@ int xnintr_mount(void)
> >       return 0;
> >  }
> >
> > +#if defined(CONFIG_PROC_FS) && defined(__KERNEL__)
> >  int xnintr_irq_proc(unsigned int irq, char *str)
> >  {
> >       xnintr_shirq_t *shirq;
> > @@ -727,6 +728,7 @@ int xnintr_irq_proc(unsigned int irq, ch
> >
> >       return p - str;
> >  }
> > +#endif /* CONFIG_PROC_FS && __KERNEL__ */
> >
> >  #else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
> >
> > @@ -735,10 +737,31 @@ int xnintr_mount(void)
> >       return 0;
> >  }
> >
> > +#if defined(CONFIG_PROC_FS) && defined(__KERNEL__)
> >  int xnintr_irq_proc(unsigned int irq, char *str)
> >  {
> > -     return 0;
> > +     xnintr_t *intr;
> > +     char *p = str;
>
> +       spl_t s;



My bad. I run diff with an older version, the correct one was in another
directory.


> +
> > +     if (rthal_virtual_irq_p(irq)) {
> > +             p += sprintf(p, "         [virtual]");
> > +             return p - str;
> > +     } else if (irq == XNARCH_TIMER_IRQ) {
> > +             p += sprintf(p, "         %s", nkclock.name);
> > +             return p - str;
> > +     }
> > +
> > +     xnlock_get_irqsave(&nklock, s);
>
> What's the idea of this lock? I'm asking as xnintr_attach/detach should
> not run under nklock, right? So, how can it protect us then?



They can as rthal_critical_enter/exit() is no longer used in
rthal_irq_request().

Now I actually recall why I have not used a cookie field to store a
corresponding xnintr_shirq_t object. It's not safe wrt detach ops. and there
is no way to implement correct synchronization involving only the nucleus
layer.
Ok, it's possible to use something similar to xnintr_shirq_lonk/unlock() but
that would require the per-irq array also for non-irq-shared case and that's
something I wanted to avoid in the first instance. Hmm.. but maybe it's
actually not that bad...

So the problem is that the "xnint_t" object we have got as a cookie can be
deleted at any time while we are using it.

intr = rthal_irq_cookie(&rthal_domain, irq);
if (intr && *(intr->name))
            p += sprintf(p, "        %s", intr->name);


Hm.. if I am not wrong, the same problem is true for xnintr_irq_handler().

Have to think a bit more on this...



Jan
>
>

-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 5135 bytes --]

[Xenomai-core] Re: [patch, minor] irq proc output in intr.c

[Dmitry Adamushko]

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

errr...  xnintr_irq_proc() for shared-irq case is also broken indeed.
xnintr_shirq_lock(shirq) should not be used there as a preemption may take
place and another thread may happilly call xnintr_detach() ->
xnintr_shirq_detach() -> possible [SMP: deadlock, UP: crash].

Can't believe the person who has writen this buggy code was me :o) Well,
it's probably a time for some refactoring.


-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 472 bytes --]