[Xenomai-core] Missing IRQ end function on PowerPC

[Xenomai-core] Missing IRQ end function on PowerPC

[Wolfgang Grandegger]

Hello,

with RTnet over Xenomai and Linux 2.4 on PowerPC I realized that Xenomai 
does not distinguish between a normal IRQ enable and an IRQ unmask at 
the end of an ISR (to reenable the IRQ). In the latter case the IRQ 
should be enabled on PowerPC as shown:

    if (rthal_irq_descp(irq)->handler->end != NULL)
  	   rthal_irq_descp(irq)->handler->end(irq);
     else
	   rthal_irq_descp(irq)->handler->enable(irq);

To handle this case properly, we need to different functions, I think.

Wolfgang.

Re: [Xenomai-core] Missing IRQ end function on PowerPC

[Gilles Chanteperdrix]

Wolfgang Grandegger wrote:
 > Hello,
 > 
 > with RTnet over Xenomai and Linux 2.4 on PowerPC I realized that Xenomai 
 > does not distinguish between a normal IRQ enable and an IRQ unmask at 
 > the end of an ISR (to reenable the IRQ). In the latter case the IRQ 
 > should be enabled on PowerPC as shown:
 > 
 >     if (rthal_irq_descp(irq)->handler->end != NULL)
 >   	   rthal_irq_descp(irq)->handler->end(irq);
 >      else
 > 	   rthal_irq_descp(irq)->handler->enable(irq);
 > 
 > To handle this case properly, we need to different functions, I think.

I am not sure I understand what you mean. Would you want functions
rthal_end_irq and xnarch_end_irq added and that this xnarch_end_irq
would be called in xnintr_irq_handler when XN_ISR_ENABLE is set ?

-- 


					    Gilles Chanteperdrix.

Re: [Xenomai-core] Missing IRQ end function on PowerPC

[Wolfgang Grandegger]

Gilles Chanteperdrix wrote:
> Wolfgang Grandegger wrote:
>  > Hello,
>  > 
>  > with RTnet over Xenomai and Linux 2.4 on PowerPC I realized that Xenomai 
>  > does not distinguish between a normal IRQ enable and an IRQ unmask at 
>  > the end of an ISR (to reenable the IRQ). In the latter case the IRQ 
>  > should be enabled on PowerPC as shown:
>  > 
>  >     if (rthal_irq_descp(irq)->handler->end != NULL)
>  >   	   rthal_irq_descp(irq)->handler->end(irq);
>  >      else
>  > 	   rthal_irq_descp(irq)->handler->enable(irq);
>  > 
>  > To handle this case properly, we need to different functions, I think.
> 
> I am not sure I understand what you mean. Would you want functions
> rthal_end_irq and xnarch_end_irq added and that this xnarch_end_irq
> would be called in xnintr_irq_handler when XN_ISR_ENABLE is set ?

Yes, that's what I have basically done to get the RTnet SCC enet driver 
working on my MPC8xx module. In arch/ppc/kernel/irq.c you can see how 
Linux re-enables interrupts at the end of it's ISR (see
http://cvs.gna.org/cvsweb/linux/v2.4/2.4.25-ppc/arch/ppc/kernel/irq.c?rev=1.1;cvsroot=adeos):

   507: 	 * The ->end() handler has to deal with interrupts which got
   508: 	 * disabled while the handler was running.
   509: 	 */
   510: 	if (desc->handler) {
   511: 		if (desc->handler->end)
   512: 			desc->handler->end(irq);
   513: 		else if (desc->handler->enable)
   514: 			desc->handler->enable(irq);

In PPC Linux 2.6 things are a bit different. The "end" functions seems 
to be mandatory now (I have to check more carefully).

Therefore we need a dedicated function to re-enable interrupts in the 
ISR. We could name it *_end_irq, but maybe *_enable_isr_irq is more 
obvious. On non-PPC archs it would translate to *_irq_enable. I 
realized, that *_irq_enable is used in various place/skins and therefore 
I have not yet provided a patch.

Wolfgang.

Re: [Xenomai-core] Missing IRQ end function on PowerPC

[Gilles Chanteperdrix]

Wolfgang Grandegger wrote:
 > Therefore we need a dedicated function to re-enable interrupts in the 
 > ISR. We could name it *_end_irq, but maybe *_enable_isr_irq is more 
 > obvious. On non-PPC archs it would translate to *_irq_enable. I 
 > realized, that *_irq_enable is used in various place/skins and therefore 
 > I have not yet provided a patch.

The function xnarch_irq_enable seems to be called in only two functions,
xintr_enable and xnintr_irq_handler when the flag XN_ISR_ENABLE is set.

In any case, since I am not sure if this has to be done at the Adeos
level or in Xenomai, we will wait for Philippe to come back and decide.

-- 


					    Gilles Chanteperdrix.

Re: [Xenomai-core] Missing IRQ end function on PowerPC

[Wolfgang Grandegger]

Gilles Chanteperdrix wrote:
> Wolfgang Grandegger wrote:
>  > Therefore we need a dedicated function to re-enable interrupts in the 
>  > ISR. We could name it *_end_irq, but maybe *_enable_isr_irq is more 
>  > obvious. On non-PPC archs it would translate to *_irq_enable. I 
>  > realized, that *_irq_enable is used in various place/skins and therefore 
>  > I have not yet provided a patch.
> 
> The function xnarch_irq_enable seems to be called in only two functions,
> xintr_enable and xnintr_irq_handler when the flag XN_ISR_ENABLE is set.

But the user may want to re-enable the interrupt without using 
XN_ISR_ENABLE as well.

> In any case, since I am not sure if this has to be done at the Adeos
> level or in Xenomai, we will wait for Philippe to come back and decide.

In both, I guess. OK and thanks.

Wolfgang.

Re: [Xenomai-core] Missing IRQ end function on PowerPC

[Wolfgang Grandegger]

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

Gilles Chanteperdrix wrote:
> Wolfgang Grandegger wrote:
>  > Therefore we need a dedicated function to re-enable interrupts in the 
>  > ISR. We could name it *_end_irq, but maybe *_enable_isr_irq is more 
>  > obvious. On non-PPC archs it would translate to *_irq_enable. I 
>  > realized, that *_irq_enable is used in various place/skins and therefore 
>  > I have not yet provided a patch.
> 
> The function xnarch_irq_enable seems to be called in only two functions,
> xintr_enable and xnintr_irq_handler when the flag XN_ISR_ENABLE is set.
> 
> In any case, since I am not sure if this has to be done at the Adeos
> level or in Xenomai, we will wait for Philippe to come back and decide.

Attached is a temporary Xenomai patch fixing the IRQ end problem for the 
PowerPC arch. I had a closer look to the various IRQ end functions on 
PowerPC:

   ic_end(unsigned int irq)
   {
         ic_ack(irq);
         if (!(irq_desc[irq].status & (IRQ_DISABLED | IRQ_INPROGRESS))) {
                 ic_enable(irq);
         }
   }

In most cases the end functions do the same than the begin functions but 
there are exceptions where the end functions do an additional ic_ack()
as shown above.

Wolfgang.



[-- Attachment #2: xenomai-2.1-end-irq.patch --]
[-- Type: text/x-patch, Size: 3194 bytes --]

+ diff -u xenomai/include/asm-generic/hal.h.IRQEND xenomai/include/asm-generic/hal.h
--- xenomai/include/asm-generic/hal.h.IRQEND	2006-01-11 18:03:34.000000000 +0100
+++ xenomai/include/asm-generic/hal.h	2006-01-19 20:52:40.000000000 +0100
@@ -357,6 +357,8 @@
 
 int rthal_irq_disable(unsigned irq);
 
+int rthal_irq_end(unsigned irq);
+
 int rthal_irq_host_request(unsigned irq,
 			   irqreturn_t (*handler)(int irq,
 						  void *dev_id,
+ diff -u xenomai/include/asm-generic/system.h.IRQEND xenomai/include/asm-generic/system.h
--- xenomai/include/asm-generic/system.h.IRQEND	2006-01-11 18:03:34.000000000 +0100
+++ xenomai/include/asm-generic/system.h	2006-01-19 20:50:17.000000000 +0100
@@ -496,6 +496,12 @@
     return rthal_irq_disable(irq);
 }
 
+static inline int xnarch_end_irq (unsigned irq)
+
+{
+     return rthal_irq_end(irq);
+}
+                                                                                
 static inline void xnarch_chain_irq (unsigned irq)
 
 {
+ diff -u xenomai/include/asm-uvm/system.h.IRQEND xenomai/include/asm-uvm/system.h
--- xenomai/include/asm-uvm/system.h.IRQEND	2006-01-11 18:03:34.000000000 +0100
+++ xenomai/include/asm-uvm/system.h	2006-01-19 20:51:36.000000000 +0100
@@ -296,6 +296,13 @@
     return -ENOSYS;
 }
 
+static inline int xnarch_end_irq (unsigned irq)
+
+{
+    return -ENOSYS;
+}
+                                                                                
+
 static inline void xnarch_chain_irq (unsigned irq)
 
 { /* Nop */ }
+ diff -u xenomai/ksrc/arch/generic/hal.c.IRQEND xenomai/ksrc/arch/generic/hal.c
--- xenomai/ksrc/arch/generic/hal.c.IRQEND	2006-01-11 18:03:42.000000000 +0100
+++ xenomai/ksrc/arch/generic/hal.c	2006-01-19 20:54:06.000000000 +0100
@@ -1156,6 +1156,7 @@
 EXPORT_SYMBOL(rthal_irq_release);
 EXPORT_SYMBOL(rthal_irq_enable);
 EXPORT_SYMBOL(rthal_irq_disable);
+EXPORT_SYMBOL(rthal_irq_end);
 EXPORT_SYMBOL(rthal_irq_host_request);
 EXPORT_SYMBOL(rthal_irq_host_release);
 EXPORT_SYMBOL(rthal_irq_host_pend);
+ diff -u xenomai/ksrc/arch/powerpc/hal.c.IRQEND xenomai/ksrc/arch/powerpc/hal.c
--- xenomai/ksrc/arch/powerpc/hal.c.IRQEND	2006-01-11 18:03:41.000000000 +0100
+++ xenomai/ksrc/arch/powerpc/hal.c	2006-01-19 21:56:19.000000000 +0100
@@ -356,6 +356,27 @@
     return 0;
 }
 
+int rthal_irq_end (unsigned irq)
+
+{
+    if (irq >= IPIPE_NR_XIRQS)
+	return -EINVAL;
+
+    if (rthal_irq_descp(irq)->handler != NULL)
+        {
+	if (rthal_irq_descp(irq)->handler->end != NULL)
+	    rthal_irq_descp(irq)->handler->end(irq);
+	else if (rthal_irq_descp(irq)->handler->enable != NULL)
+	    rthal_irq_descp(irq)->handler->enable(irq);
+	else
+	    return -ENODEV;
+	}
+    else
+	return -ENODEV;
+
+    return 0;
+}
+
 static inline int do_exception_event (unsigned event, unsigned domid, void *data)
 
 {
+ diff -u xenomai/ksrc/nucleus/intr.c.IRQEND xenomai/ksrc/nucleus/intr.c
--- xenomai/ksrc/nucleus/intr.c.IRQEND	2006-01-11 18:03:42.000000000 +0100
+++ xenomai/ksrc/nucleus/intr.c	2006-01-19 20:42:53.000000000 +0100
@@ -363,7 +363,7 @@
     ++intr->hits;
 
     if (s & XN_ISR_ENABLE)
-	xnarch_enable_irq(irq);
+	xnarch_end_irq(irq);
 
     if (s & XN_ISR_CHAINED)
 	xnarch_chain_irq(irq);

Re: [Xenomai-core] Missing IRQ end function on PowerPC

[Philippe Gerum]

Gilles Chanteperdrix wrote:
> Wolfgang Grandegger wrote:
>  > Therefore we need a dedicated function to re-enable interrupts in the 
>  > ISR. We could name it *_end_irq, but maybe *_enable_isr_irq is more 
>  > obvious. On non-PPC archs it would translate to *_irq_enable. I 
>  > realized, that *_irq_enable is used in various place/skins and therefore 
>  > I have not yet provided a patch.
> 
> The function xnarch_irq_enable seems to be called in only two functions,
> xintr_enable and xnintr_irq_handler when the flag XN_ISR_ENABLE is set.
> 
> In any case, since I am not sure if this has to be done at the Adeos
> level or in Xenomai, we will wait for Philippe to come back and decide.
> 

->enable() and ->end() all mixed up illustrates a silly x86 bias I once had. We do 
need to differentiate the mere enabling from the IRQ epilogue at PIC level since 
Linux does it - i.e. we don't want to change the semantics here.

I would go for adding xnarch_end_irq -> rthal_irq_end to stick with the Linux 
naming scheme, and have the proper epilogue done from there on a per-arch basis.

Current uses of xnarch_enable_irq() should be reserved to the non-epilogue case, 
like xnintr_enable() i.e. forcibly unmasking the IRQ source at PIC level outside 
of any ISR context for such interrupt (*). XN_ISR_ENABLE would trigger a call to 
xnarch_end_irq, instead of xnarch_enable_irq. I see no reason for this fix to leak 
to the Adeos layer, since the HAL already controls the way interrupts are ended 
actually; it just does it improperly on some platforms.

(*) Jan, does rtdm_irq_enable() have the same meaning, or is it intended to be 
used from the ISR too in order to revalidate the source at PIC level?

-- 

Philippe.

Re: [Xenomai-core] Missing IRQ end function on PowerPC

[Jan Kiszka]

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

Philippe Gerum wrote:
> Gilles Chanteperdrix wrote:
>> Wolfgang Grandegger wrote:
>>  > Therefore we need a dedicated function to re-enable interrupts in
>> the  > ISR. We could name it *_end_irq, but maybe *_enable_isr_irq is
>> more  > obvious. On non-PPC archs it would translate to *_irq_enable.
>> I  > realized, that *_irq_enable is used in various place/skins and
>> therefore  > I have not yet provided a patch.
>>
>> The function xnarch_irq_enable seems to be called in only two functions,
>> xintr_enable and xnintr_irq_handler when the flag XN_ISR_ENABLE is set.
>>
>> In any case, since I am not sure if this has to be done at the Adeos
>> level or in Xenomai, we will wait for Philippe to come back and decide.
>>
> 
> ->enable() and ->end() all mixed up illustrates a silly x86 bias I once
> had. We do need to differentiate the mere enabling from the IRQ epilogue
> at PIC level since Linux does it - i.e. we don't want to change the
> semantics here.
> 
> I would go for adding xnarch_end_irq -> rthal_irq_end to stick with the
> Linux naming scheme, and have the proper epilogue done from there on a
> per-arch basis.
> 
> Current uses of xnarch_enable_irq() should be reserved to the
> non-epilogue case, like xnintr_enable() i.e. forcibly unmasking the IRQ
> source at PIC level outside of any ISR context for such interrupt (*).
> XN_ISR_ENABLE would trigger a call to xnarch_end_irq, instead of
> xnarch_enable_irq. I see no reason for this fix to leak to the Adeos
> layer, since the HAL already controls the way interrupts are ended
> actually; it just does it improperly on some platforms.
> 
> (*) Jan, does rtdm_irq_enable() have the same meaning, or is it intended
> to be used from the ISR too in order to revalidate the source at PIC level?
> 

Nope, rtdm_irq_enable() was never intended to re-enable an IRQ line
after an interrupt, and the documentation does not suggest this either.
I see no problem here.

Jan


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

Re: [Xenomai-core] Missing IRQ end function on PowerPC

[Wolfgang Grandegger]

> This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
> 
> Philippe Gerum wrote:
> > Gilles Chanteperdrix wrote:
> >> Wolfgang Grandegger wrote:
> >>  > Therefore we need a dedicated function to re-enable interrupts in
> >> the  > ISR. We could name it *_end_irq, but maybe *_enable_isr_irq is
> >> more  > obvious. On non-PPC archs it would translate to *_irq_enable.
> >> I  > realized, that *_irq_enable is used in various place/skins and
> >> therefore  > I have not yet provided a patch.
> >>
> >> The function xnarch_irq_enable seems to be called in only two
functions,
> >> xintr_enable and xnintr_irq_handler when the flag XN_ISR_ENABLE is set.
> >>
> >> In any case, since I am not sure if this has to be done at the Adeos
> >> level or in Xenomai, we will wait for Philippe to come back and decide.
> >>
> > 
> > ->enable() and ->end() all mixed up illustrates a silly x86 bias I once
> > had. We do need to differentiate the mere enabling from the IRQ epilogue
> > at PIC level since Linux does it - i.e. we don't want to change the
> > semantics here.
> > 
> > I would go for adding xnarch_end_irq -> rthal_irq_end to stick with the
> > Linux naming scheme, and have the proper epilogue done from there on a
> > per-arch basis.
> > 
> > Current uses of xnarch_enable_irq() should be reserved to the
> > non-epilogue case, like xnintr_enable() i.e. forcibly unmasking the IRQ
> > source at PIC level outside of any ISR context for such interrupt (*).
> > XN_ISR_ENABLE would trigger a call to xnarch_end_irq, instead of
> > xnarch_enable_irq. I see no reason for this fix to leak to the Adeos
> > layer, since the HAL already controls the way interrupts are ended
> > actually; it just does it improperly on some platforms.
> > 
> > (*) Jan, does rtdm_irq_enable() have the same meaning, or is it intended
> > to be used from the ISR too in order to revalidate the source at PIC
level?
> > 
> 
> Nope, rtdm_irq_enable() was never intended to re-enable an IRQ line
> after an interrupt, and the documentation does not suggest this either.
> I see no problem here.

But RTDM needs a rtdm_irq_end() functions as well in case the
user wants to reenable the interrupt outside the ISR, I think.

Wolfgang.

Re: [Xenomai-core] Missing IRQ end function on PowerPC

[Jan Kiszka]

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

Wolfgang Grandegger wrote:
> 
>> This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
>>
>> Philippe Gerum wrote:
>>> Gilles Chanteperdrix wrote:
>>>> Wolfgang Grandegger wrote:
>>>>  > Therefore we need a dedicated function to re-enable interrupts in
>>>> the  > ISR. We could name it *_end_irq, but maybe *_enable_isr_irq is
>>>> more  > obvious. On non-PPC archs it would translate to *_irq_enable.
>>>> I  > realized, that *_irq_enable is used in various place/skins and
>>>> therefore  > I have not yet provided a patch.
>>>>
>>>> The function xnarch_irq_enable seems to be called in only two
> functions,
>>>> xintr_enable and xnintr_irq_handler when the flag XN_ISR_ENABLE is set.
>>>>
>>>> In any case, since I am not sure if this has to be done at the Adeos
>>>> level or in Xenomai, we will wait for Philippe to come back and decide.
>>>>
>>> ->enable() and ->end() all mixed up illustrates a silly x86 bias I once
>>> had. We do need to differentiate the mere enabling from the IRQ epilogue
>>> at PIC level since Linux does it - i.e. we don't want to change the
>>> semantics here.
>>>
>>> I would go for adding xnarch_end_irq -> rthal_irq_end to stick with the
>>> Linux naming scheme, and have the proper epilogue done from there on a
>>> per-arch basis.
>>>
>>> Current uses of xnarch_enable_irq() should be reserved to the
>>> non-epilogue case, like xnintr_enable() i.e. forcibly unmasking the IRQ
>>> source at PIC level outside of any ISR context for such interrupt (*).
>>> XN_ISR_ENABLE would trigger a call to xnarch_end_irq, instead of
>>> xnarch_enable_irq. I see no reason for this fix to leak to the Adeos
>>> layer, since the HAL already controls the way interrupts are ended
>>> actually; it just does it improperly on some platforms.
>>>
>>> (*) Jan, does rtdm_irq_enable() have the same meaning, or is it intended
>>> to be used from the ISR too in order to revalidate the source at PIC
> level?
>> Nope, rtdm_irq_enable() was never intended to re-enable an IRQ line
>> after an interrupt, and the documentation does not suggest this either.
>> I see no problem here.
> 
> But RTDM needs a rtdm_irq_end() functions as well in case the
> user wants to reenable the interrupt outside the ISR, I think.

If this is a valid use-case, it should be really straightforward to add
this abstraction to RTDM. We should just document that rtdm_irq_end()
shall not be invoked from IRQ context - to avoid breaking the chain in
the shared-IRQ scenario. RTDM_IRQ_ENABLE must remain the way to
re-enable the line from the handler.

Jan



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

Re: [Xenomai-core] Missing IRQ end function on PowerPC

[Philippe Gerum]

Jan Kiszka wrote:
> Wolfgang Grandegger wrote:
> 
>>>This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
>>>
>>>Philippe Gerum wrote:
>>>
>>>>Gilles Chanteperdrix wrote:
>>>>
>>>>>Wolfgang Grandegger wrote:
>>>>> > Therefore we need a dedicated function to re-enable interrupts in
>>>>>the  > ISR. We could name it *_end_irq, but maybe *_enable_isr_irq is
>>>>>more  > obvious. On non-PPC archs it would translate to *_irq_enable.
>>>>>I  > realized, that *_irq_enable is used in various place/skins and
>>>>>therefore  > I have not yet provided a patch.
>>>>>
>>>>>The function xnarch_irq_enable seems to be called in only two
>>
>>functions,
>>
>>>>>xintr_enable and xnintr_irq_handler when the flag XN_ISR_ENABLE is set.
>>>>>
>>>>>In any case, since I am not sure if this has to be done at the Adeos
>>>>>level or in Xenomai, we will wait for Philippe to come back and decide.
>>>>>
>>>>
>>>>->enable() and ->end() all mixed up illustrates a silly x86 bias I once
>>>>had. We do need to differentiate the mere enabling from the IRQ epilogue
>>>>at PIC level since Linux does it - i.e. we don't want to change the
>>>>semantics here.
>>>>
>>>>I would go for adding xnarch_end_irq -> rthal_irq_end to stick with the
>>>>Linux naming scheme, and have the proper epilogue done from there on a
>>>>per-arch basis.
>>>>
>>>>Current uses of xnarch_enable_irq() should be reserved to the
>>>>non-epilogue case, like xnintr_enable() i.e. forcibly unmasking the IRQ
>>>>source at PIC level outside of any ISR context for such interrupt (*).
>>>>XN_ISR_ENABLE would trigger a call to xnarch_end_irq, instead of
>>>>xnarch_enable_irq. I see no reason for this fix to leak to the Adeos
>>>>layer, since the HAL already controls the way interrupts are ended
>>>>actually; it just does it improperly on some platforms.
>>>>
>>>>(*) Jan, does rtdm_irq_enable() have the same meaning, or is it intended
>>>>to be used from the ISR too in order to revalidate the source at PIC
>>
>>level?
>>
>>>Nope, rtdm_irq_enable() was never intended to re-enable an IRQ line
>>>after an interrupt, and the documentation does not suggest this either.
>>>I see no problem here.
>>
>>But RTDM needs a rtdm_irq_end() functions as well in case the
>>user wants to reenable the interrupt outside the ISR, I think.
> 
> 
> If this is a valid use-case, it should be really straightforward to add
> this abstraction to RTDM. We should just document that rtdm_irq_end()
> shall not be invoked from IRQ context -

It's the other way around: ->end() would indeed be called from the ISR epilogue, 
and ->enable() would not.

  to avoid breaking the chain in
> the shared-IRQ scenario. RTDM_IRQ_ENABLE must remain the way to
> re-enable the line from the handler.
> 
> Jan
> 
> 


-- 

Philippe.

Re: [Xenomai-core] Missing IRQ end function on PowerPC

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
> 
>>Gilles Chanteperdrix wrote:
>>
>>>Wolfgang Grandegger wrote:
>>> > Therefore we need a dedicated function to re-enable interrupts in
>>>the  > ISR. We could name it *_end_irq, but maybe *_enable_isr_irq is
>>>more  > obvious. On non-PPC archs it would translate to *_irq_enable.
>>>I  > realized, that *_irq_enable is used in various place/skins and
>>>therefore  > I have not yet provided a patch.
>>>
>>>The function xnarch_irq_enable seems to be called in only two functions,
>>>xintr_enable and xnintr_irq_handler when the flag XN_ISR_ENABLE is set.
>>>
>>>In any case, since I am not sure if this has to be done at the Adeos
>>>level or in Xenomai, we will wait for Philippe to come back and decide.
>>>
>>
>>->enable() and ->end() all mixed up illustrates a silly x86 bias I once
>>had. We do need to differentiate the mere enabling from the IRQ epilogue
>>at PIC level since Linux does it - i.e. we don't want to change the
>>semantics here.
>>
>>I would go for adding xnarch_end_irq -> rthal_irq_end to stick with the
>>Linux naming scheme, and have the proper epilogue done from there on a
>>per-arch basis.
>>
>>Current uses of xnarch_enable_irq() should be reserved to the
>>non-epilogue case, like xnintr_enable() i.e. forcibly unmasking the IRQ
>>source at PIC level outside of any ISR context for such interrupt (*).
>>XN_ISR_ENABLE would trigger a call to xnarch_end_irq, instead of
>>xnarch_enable_irq. I see no reason for this fix to leak to the Adeos
>>layer, since the HAL already controls the way interrupts are ended
>>actually; it just does it improperly on some platforms.
>>
>>(*) Jan, does rtdm_irq_enable() have the same meaning, or is it intended
>>to be used from the ISR too in order to revalidate the source at PIC level?
>>
> 
> 
> Nope, rtdm_irq_enable() was never intended to re-enable an IRQ line
> after an interrupt, and the documentation does not suggest this either.
> I see no problem here.
>

Ok, so no change would be needed here to implement what's described above.

> Jan
> 


-- 

Philippe.

Re: [Xenomai-core] Missing IRQ end function on PowerPC

[Jan Kiszka]

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

Philippe Gerum wrote:
> Jan Kiszka wrote:
>> Wolfgang Grandegger wrote:
>>
>>>> This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
>>>>
>>>> Philippe Gerum wrote:
>>>>
>>>>> Gilles Chanteperdrix wrote:
>>>>>
>>>>>> Wolfgang Grandegger wrote:
>>>>>> > Therefore we need a dedicated function to re-enable interrupts in
>>>>>> the  > ISR. We could name it *_end_irq, but maybe *_enable_isr_irq is
>>>>>> more  > obvious. On non-PPC archs it would translate to *_irq_enable.
>>>>>> I  > realized, that *_irq_enable is used in various place/skins and
>>>>>> therefore  > I have not yet provided a patch.
>>>>>>
>>>>>> The function xnarch_irq_enable seems to be called in only two
>>>
>>> functions,
>>>
>>>>>> xintr_enable and xnintr_irq_handler when the flag XN_ISR_ENABLE is
>>>>>> set.
>>>>>>
>>>>>> In any case, since I am not sure if this has to be done at the Adeos
>>>>>> level or in Xenomai, we will wait for Philippe to come back and
>>>>>> decide.
>>>>>>
>>>>>
>>>>> ->enable() and ->end() all mixed up illustrates a silly x86 bias I
>>>>> once
>>>>> had. We do need to differentiate the mere enabling from the IRQ
>>>>> epilogue
>>>>> at PIC level since Linux does it - i.e. we don't want to change the
>>>>> semantics here.
>>>>>
>>>>> I would go for adding xnarch_end_irq -> rthal_irq_end to stick with
>>>>> the
>>>>> Linux naming scheme, and have the proper epilogue done from there on a
>>>>> per-arch basis.
>>>>>
>>>>> Current uses of xnarch_enable_irq() should be reserved to the
>>>>> non-epilogue case, like xnintr_enable() i.e. forcibly unmasking the
>>>>> IRQ
>>>>> source at PIC level outside of any ISR context for such interrupt (*).
>>>>> XN_ISR_ENABLE would trigger a call to xnarch_end_irq, instead of
>>>>> xnarch_enable_irq. I see no reason for this fix to leak to the Adeos
>>>>> layer, since the HAL already controls the way interrupts are ended
>>>>> actually; it just does it improperly on some platforms.
>>>>>
>>>>> (*) Jan, does rtdm_irq_enable() have the same meaning, or is it
>>>>> intended
>>>>> to be used from the ISR too in order to revalidate the source at PIC
>>>
>>> level?
>>>
>>>> Nope, rtdm_irq_enable() was never intended to re-enable an IRQ line
>>>> after an interrupt, and the documentation does not suggest this either.
>>>> I see no problem here.
>>>
>>> But RTDM needs a rtdm_irq_end() functions as well in case the
>>> user wants to reenable the interrupt outside the ISR, I think.
>>
>>
>> If this is a valid use-case, it should be really straightforward to add
>> this abstraction to RTDM. We should just document that rtdm_irq_end()
>> shall not be invoked from IRQ context -
> 
> It's the other way around: ->end() would indeed be called from the ISR
> epilogue, and ->enable() would not.

I think we are talking about different things: Wolfgang was asking for
an equivalent of RTDM_IRQ_ENABLE outside the IRQ handler. That's the
user API. You are now referring to the back-end which had to provide
some end-mechanism to be used both by the nucleus' ISR epilogue and that
rtdm_irq_end(), and that mechanism shall be told apart from the IRQ
enable/disable API. Well, that's at least how I think to have got it...

> 
>  to avoid breaking the chain in
>> the shared-IRQ scenario. RTDM_IRQ_ENABLE must remain the way to
>> re-enable the line from the handler.
>>
>> Jan
>>
>>
> 
> 

Jan


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

Re: [Xenomai-core] Missing IRQ end function on PowerPC

[Wolfgang Grandegger]

> This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
> 
> Philippe Gerum wrote:
> > Jan Kiszka wrote:
> >> Wolfgang Grandegger wrote:
> >>
> >>>> This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
> >>>>
> >>>> Philippe Gerum wrote:
> >>>>
> >>>>> Gilles Chanteperdrix wrote:
> >>>>>
> >>>>>> Wolfgang Grandegger wrote:
> >>>>>> > Therefore we need a dedicated function to re-enable interrupts in
> >>>>>> the  > ISR. We could name it *_end_irq, but maybe
*_enable_isr_irq is
> >>>>>> more  > obvious. On non-PPC archs it would translate to
*_irq_enable.
> >>>>>> I  > realized, that *_irq_enable is used in various place/skins and
> >>>>>> therefore  > I have not yet provided a patch.
> >>>>>>
> >>>>>> The function xnarch_irq_enable seems to be called in only two
> >>>
> >>> functions,
> >>>
> >>>>>> xintr_enable and xnintr_irq_handler when the flag XN_ISR_ENABLE is
> >>>>>> set.
> >>>>>>
> >>>>>> In any case, since I am not sure if this has to be done at the
Adeos
> >>>>>> level or in Xenomai, we will wait for Philippe to come back and
> >>>>>> decide.
> >>>>>>
> >>>>>
> >>>>> ->enable() and ->end() all mixed up illustrates a silly x86 bias I
> >>>>> once
> >>>>> had. We do need to differentiate the mere enabling from the IRQ
> >>>>> epilogue
> >>>>> at PIC level since Linux does it - i.e. we don't want to change the
> >>>>> semantics here.
> >>>>>
> >>>>> I would go for adding xnarch_end_irq -> rthal_irq_end to stick with
> >>>>> the
> >>>>> Linux naming scheme, and have the proper epilogue done from
there on a
> >>>>> per-arch basis.
> >>>>>
> >>>>> Current uses of xnarch_enable_irq() should be reserved to the
> >>>>> non-epilogue case, like xnintr_enable() i.e. forcibly unmasking the
> >>>>> IRQ
> >>>>> source at PIC level outside of any ISR context for such
interrupt (*).
> >>>>> XN_ISR_ENABLE would trigger a call to xnarch_end_irq, instead of
> >>>>> xnarch_enable_irq. I see no reason for this fix to leak to the Adeos
> >>>>> layer, since the HAL already controls the way interrupts are ended
> >>>>> actually; it just does it improperly on some platforms.
> >>>>>
> >>>>> (*) Jan, does rtdm_irq_enable() have the same meaning, or is it
> >>>>> intended
> >>>>> to be used from the ISR too in order to revalidate the source at PIC
> >>>
> >>> level?
> >>>
> >>>> Nope, rtdm_irq_enable() was never intended to re-enable an IRQ line
> >>>> after an interrupt, and the documentation does not suggest this
either.
> >>>> I see no problem here.
> >>>
> >>> But RTDM needs a rtdm_irq_end() functions as well in case the
> >>> user wants to reenable the interrupt outside the ISR, I think.
> >>
> >>
> >> If this is a valid use-case, it should be really straightforward to add
> >> this abstraction to RTDM. We should just document that rtdm_irq_end()
> >> shall not be invoked from IRQ context -
> > 
> > It's the other way around: ->end() would indeed be called from the ISR
> > epilogue, and ->enable() would not.
> 
> I think we are talking about different things: Wolfgang was asking for
> an equivalent of RTDM_IRQ_ENABLE outside the IRQ handler. That's the
> user API. You are now referring to the back-end which had to provide
> some end-mechanism to be used both by the nucleus' ISR epilogue and that
> rtdm_irq_end(), and that mechanism shall be told apart from the IRQ
> enable/disable API. Well, that's at least how I think to have got it...

Yep, I was thinking of deferred interrupt handling in a real-time task.
Then rtdm_irq_end() would be required.
 
> > 
> >  to avoid breaking the chain in
> >> the shared-IRQ scenario. RTDM_IRQ_ENABLE must remain the way to
> >> re-enable the line from the handler.
> >>
> >> Jan
> >>
> >>
> > 
> > 
> 
> Jan
> 
> 
> 

Wolfgang.

Re: [Xenomai-core] Missing IRQ end function on PowerPC

[Philippe Gerum]

Jan Kiszka wrote:
> Philippe Gerum wrote:
> 
>>Jan Kiszka wrote:
>>
>>>Wolfgang Grandegger wrote:
>>>
>>>
>>>>>This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
>>>>>
>>>>>Philippe Gerum wrote:
>>>>>
>>>>>
>>>>>>Gilles Chanteperdrix wrote:
>>>>>>
>>>>>>
>>>>>>>Wolfgang Grandegger wrote:
>>>>>>>
>>>>>>>>Therefore we need a dedicated function to re-enable interrupts in
>>>>>>>
>>>>>>>the  > ISR. We could name it *_end_irq, but maybe *_enable_isr_irq is
>>>>>>>more  > obvious. On non-PPC archs it would translate to *_irq_enable.
>>>>>>>I  > realized, that *_irq_enable is used in various place/skins and
>>>>>>>therefore  > I have not yet provided a patch.
>>>>>>>
>>>>>>>The function xnarch_irq_enable seems to be called in only two
>>>>
>>>>functions,
>>>>
>>>>
>>>>>>>xintr_enable and xnintr_irq_handler when the flag XN_ISR_ENABLE is
>>>>>>>set.
>>>>>>>
>>>>>>>In any case, since I am not sure if this has to be done at the Adeos
>>>>>>>level or in Xenomai, we will wait for Philippe to come back and
>>>>>>>decide.
>>>>>>>
>>>>>>
>>>>>>->enable() and ->end() all mixed up illustrates a silly x86 bias I
>>>>>>once
>>>>>>had. We do need to differentiate the mere enabling from the IRQ
>>>>>>epilogue
>>>>>>at PIC level since Linux does it - i.e. we don't want to change the
>>>>>>semantics here.
>>>>>>
>>>>>>I would go for adding xnarch_end_irq -> rthal_irq_end to stick with
>>>>>>the
>>>>>>Linux naming scheme, and have the proper epilogue done from there on a
>>>>>>per-arch basis.
>>>>>>
>>>>>>Current uses of xnarch_enable_irq() should be reserved to the
>>>>>>non-epilogue case, like xnintr_enable() i.e. forcibly unmasking the
>>>>>>IRQ
>>>>>>source at PIC level outside of any ISR context for such interrupt (*).
>>>>>>XN_ISR_ENABLE would trigger a call to xnarch_end_irq, instead of
>>>>>>xnarch_enable_irq. I see no reason for this fix to leak to the Adeos
>>>>>>layer, since the HAL already controls the way interrupts are ended
>>>>>>actually; it just does it improperly on some platforms.
>>>>>>
>>>>>>(*) Jan, does rtdm_irq_enable() have the same meaning, or is it
>>>>>>intended
>>>>>>to be used from the ISR too in order to revalidate the source at PIC
>>>>
>>>>level?
>>>>
>>>>
>>>>>Nope, rtdm_irq_enable() was never intended to re-enable an IRQ line
>>>>>after an interrupt, and the documentation does not suggest this either.
>>>>>I see no problem here.
>>>>
>>>>But RTDM needs a rtdm_irq_end() functions as well in case the
>>>>user wants to reenable the interrupt outside the ISR, I think.
>>>
>>>
>>>If this is a valid use-case, it should be really straightforward to add
>>>this abstraction to RTDM. We should just document that rtdm_irq_end()
>>>shall not be invoked from IRQ context -
>>
>>It's the other way around: ->end() would indeed be called from the ISR
>>epilogue, and ->enable() would not.
> 
> 
> I think we are talking about different things: Wolfgang was asking for
> an equivalent of RTDM_IRQ_ENABLE outside the IRQ handler. That's the
> user API. You are now referring to the back-end which had to provide
> some end-mechanism to be used both by the nucleus' ISR epilogue and that
> rtdm_irq_end(), and that mechanism shall be told apart from the IRQ
> enable/disable API. Well, that's at least how I think to have got it...
> 

My point was only about naming here: *_end() should be reserved for the epilogue 
stuff, hence be callable from ISR context. Aside of that, I'm ok with the 
abstraction you described though.

> 
>> to avoid breaking the chain in
>>
>>>the shared-IRQ scenario. RTDM_IRQ_ENABLE must remain the way to
>>>re-enable the line from the handler.
>>>
>>>Jan
>>>
>>>
>>
>>
> 
> Jan
> 


-- 

Philippe.

Re: [Xenomai-core] Missing IRQ end function on PowerPC

[Philippe Gerum]

Wolfgang Grandegger wrote:
> Gilles Chanteperdrix wrote:
> 
>> Wolfgang Grandegger wrote:
>>  > Therefore we need a dedicated function to re-enable interrupts in 
>> the  > ISR. We could name it *_end_irq, but maybe *_enable_isr_irq is 
>> more  > obvious. On non-PPC archs it would translate to *_irq_enable. 
>> I  > realized, that *_irq_enable is used in various place/skins and 
>> therefore  > I have not yet provided a patch.
>>
>> The function xnarch_irq_enable seems to be called in only two functions,
>> xintr_enable and xnintr_irq_handler when the flag XN_ISR_ENABLE is set.
>>
>> In any case, since I am not sure if this has to be done at the Adeos
>> level or in Xenomai, we will wait for Philippe to come back and decide.
> 
> 
> Attached is a temporary Xenomai patch fixing the IRQ end problem for the 
> PowerPC arch. I had a closer look to the various IRQ end functions on 
> PowerPC:

Applied and generalized for all archs, thanks.

> 
>   ic_end(unsigned int irq)
>   {
>         ic_ack(irq);
>         if (!(irq_desc[irq].status & (IRQ_DISABLED | IRQ_INPROGRESS))) {
>                 ic_enable(irq);
>         }
>   }
> 
> In most cases the end functions do the same than the begin functions but 
> there are exceptions where the end functions do an additional ic_ack()
> as shown above.
> 
> Wolfgang.
> 
> 
> 
> ------------------------------------------------------------------------
> 
> + diff -u xenomai/include/asm-generic/hal.h.IRQEND xenomai/include/asm-generic/hal.h
> --- xenomai/include/asm-generic/hal.h.IRQEND	2006-01-11 18:03:34.000000000 +0100
> +++ xenomai/include/asm-generic/hal.h	2006-01-19 20:52:40.000000000 +0100
> @@ -357,6 +357,8 @@
>  
>  int rthal_irq_disable(unsigned irq);
>  
> +int rthal_irq_end(unsigned irq);
> +
>  int rthal_irq_host_request(unsigned irq,
>  			   irqreturn_t (*handler)(int irq,
>  						  void *dev_id,
> + diff -u xenomai/include/asm-generic/system.h.IRQEND xenomai/include/asm-generic/system.h
> --- xenomai/include/asm-generic/system.h.IRQEND	2006-01-11 18:03:34.000000000 +0100
> +++ xenomai/include/asm-generic/system.h	2006-01-19 20:50:17.000000000 +0100
> @@ -496,6 +496,12 @@
>      return rthal_irq_disable(irq);
>  }
>  
> +static inline int xnarch_end_irq (unsigned irq)
> +
> +{
> +     return rthal_irq_end(irq);
> +}
> +                                                                                
>  static inline void xnarch_chain_irq (unsigned irq)
>  
>  {
> + diff -u xenomai/include/asm-uvm/system.h.IRQEND xenomai/include/asm-uvm/system.h
> --- xenomai/include/asm-uvm/system.h.IRQEND	2006-01-11 18:03:34.000000000 +0100
> +++ xenomai/include/asm-uvm/system.h	2006-01-19 20:51:36.000000000 +0100
> @@ -296,6 +296,13 @@
>      return -ENOSYS;
>  }
>  
> +static inline int xnarch_end_irq (unsigned irq)
> +
> +{
> +    return -ENOSYS;
> +}
> +                                                                                
> +
>  static inline void xnarch_chain_irq (unsigned irq)
>  
>  { /* Nop */ }
> + diff -u xenomai/ksrc/arch/generic/hal.c.IRQEND xenomai/ksrc/arch/generic/hal.c
> --- xenomai/ksrc/arch/generic/hal.c.IRQEND	2006-01-11 18:03:42.000000000 +0100
> +++ xenomai/ksrc/arch/generic/hal.c	2006-01-19 20:54:06.000000000 +0100
> @@ -1156,6 +1156,7 @@
>  EXPORT_SYMBOL(rthal_irq_release);
>  EXPORT_SYMBOL(rthal_irq_enable);
>  EXPORT_SYMBOL(rthal_irq_disable);
> +EXPORT_SYMBOL(rthal_irq_end);
>  EXPORT_SYMBOL(rthal_irq_host_request);
>  EXPORT_SYMBOL(rthal_irq_host_release);
>  EXPORT_SYMBOL(rthal_irq_host_pend);
> + diff -u xenomai/ksrc/arch/powerpc/hal.c.IRQEND xenomai/ksrc/arch/powerpc/hal.c
> --- xenomai/ksrc/arch/powerpc/hal.c.IRQEND	2006-01-11 18:03:41.000000000 +0100
> +++ xenomai/ksrc/arch/powerpc/hal.c	2006-01-19 21:56:19.000000000 +0100
> @@ -356,6 +356,27 @@
>      return 0;
>  }
>  
> +int rthal_irq_end (unsigned irq)
> +
> +{
> +    if (irq >= IPIPE_NR_XIRQS)
> +	return -EINVAL;
> +
> +    if (rthal_irq_descp(irq)->handler != NULL)
> +        {
> +	if (rthal_irq_descp(irq)->handler->end != NULL)
> +	    rthal_irq_descp(irq)->handler->end(irq);
> +	else if (rthal_irq_descp(irq)->handler->enable != NULL)
> +	    rthal_irq_descp(irq)->handler->enable(irq);
> +	else
> +	    return -ENODEV;
> +	}
> +    else
> +	return -ENODEV;
> +
> +    return 0;
> +}
> +
>  static inline int do_exception_event (unsigned event, unsigned domid, void *data)
>  
>  {
> + diff -u xenomai/ksrc/nucleus/intr.c.IRQEND xenomai/ksrc/nucleus/intr.c
> --- xenomai/ksrc/nucleus/intr.c.IRQEND	2006-01-11 18:03:42.000000000 +0100
> +++ xenomai/ksrc/nucleus/intr.c	2006-01-19 20:42:53.000000000 +0100
> @@ -363,7 +363,7 @@
>      ++intr->hits;
>  
>      if (s & XN_ISR_ENABLE)
> -	xnarch_enable_irq(irq);
> +	xnarch_end_irq(irq);
>  
>      if (s & XN_ISR_CHAINED)
>  	xnarch_chain_irq(irq);
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Xenomai-core mailing list
> Xenomai-core@domain.hid
> https://mail.gna.org/listinfo/xenomai-core


-- 

Philippe.