subject:"\[Xenomai\-core\] Re\: \[PATCH\] Shared interrupts \(ready to merge\)"

Anders Blomdell wrote:
 Jan Kiszka wrote:
 Hi Dmitry,

 Dmitry Adamushko wrote:

 Hi Jan,

 let's make yet another revision of the bits :

 new XN_ISR_HANDLED  == old XN_ISR_HANDLED + old XN_ISR_NO_ENABLE

 ok.

 new XN_ISR_NOENABLE == ~ old XN_ISR_ENABLE

 ok.

 new XN_ISR_PROPAGATE == XN_ISR_CHAINED

 ok.



 Just to make sure that you understand my weird ideas: each of the three
 new XN_ISR_xxx above should be encoded with an individual bit


 new XN_ISR_NOINT == ?

 does it suppose the interrupt line to be .end-ed (enabled) and irq
 not to be
 propagated? Should be so, I guess, if it's different from 5). Then
 nucleus
 ignores implicit IRQ enable for 5) as well as for 3).

 Do we really need that NOINT then, as it seems to be the same as
 ~HANDLED?

 or NOINT == 0 and then it's a scalar value, not a bit.

 So one may consider HANDLED == 1 and NOINT == 0 as really scalar values

 and

 NOENABLE and PROPAGATE as additional bits (used only if needed).



 My idea is to urge the user specifying one of the base return types
 (HANDLED or NOINT) + any of the two additional bits (NOENABLE and
 PROPAGATE).

 For correct drivers NOINT could be 0 indeed, but to check that the user
 picked a new constant we may want to set NOINT != 0. With the old API
 return 0 expressed HANDLED + ~ENABLE for the old API. With the new one
 the user signals no interest and the nucleus may raise a warning that a
 spurious IRQ occurred. So I would add a debug bit for NOINT here to
 optionally (on OPT_XENO_DEBUG) detect old-style usage (return 0).
 Moreover, we gain freedom to move bits in the future when every state is
 encoded via constants. Or am I too paranoid here?
 After reading the above discussion (of which I understand very little),
 and looking at (what I believe to be) the relevant code:
 
 +intr = shirq-handlers;
 +
 +while (intr)
 +{
 +s |= intr-isr(intr);
 +++intr-hits;
 +intr = intr-next;
 +}
 +xnintr_shirq_unlock(shirq);
 +
 +--sched-inesting;
 +
 +if (s  XN_ISR_ENABLE)
 +   xnarch_end_irq(irq);
 +else if (s  XN_ISR_CHAINED)
 +   xnarch_chain_irq(irq);
 
 A number of questions arise:
 
 1. What happens if one of the shared handlers leaves the interrupt
 asserted, returns NOENABLE|HANDLED and another return only HANDLED?
 
 2. What happens if one returns PROPAGATE and another returns HANDLED?
 
 As far as I can tell, after all RT handlers havve run, the following
 scenarios are possible:
 
 1. The interrupt is deasserted (i.e. it was a RT interrupt)
 2. The interrupt is still asserted, it will be deasserted later
by some RT task (i.e. it was a RT interrupt)
 3. The interrupt is still asserted and will be deasserted
by the Linux IRQ handler.
 
 IMHO that leads to the conclusion that the IRQ handlers should return a
 scalar
 
 #define UNHANDLED 0
 #define HANDLED_ENABLE 1
 #define HANDLED_NOENABLE 2
 #define PROPAGATE 3
 
 and the loop should be
 
 s = UNHANDLED
 while (intr) {
   tmp = intr-isr(intr);
   if (tmp  s) { s = tmp; }
   intr = intr-next;
 }
 if (s == PROPAGATE) {
   xnarch_chain_irq(irq);
 } else if (s == HANDLED_ENABLE) {
   xnarch_end_irq(irq);
 }

This is a very useful suggestion, specifically this escalation of the
return value in the shared case. I would just let UNHANLDED start with 1
for the reasons I explained here:

https://mail.gna.org/public/xenomai-core/2006-02/msg00186.html

Ok, I would say let's got for this and finalise the patch!

 
 To be really honest, I think that PROPAGATE should be excluded from the
 RT IRQ-handlers, since with the current scheme all RT-handlers has to
 test if the IRQ was a Linux interrupt (otherwise the system will only
 work when the handler that returns PROPAGATE is installed)
 

Indeed, but I'm with Philippe here: do not exclude the strange corner
case scenarios users may craft with the appropriate care. I could, e.g.,
imagine a scenario where an RT handler takes the arrival time stamp of a
non-RT IRQ and then propagates it.

Jsn



signature.asc
Description: OpenPGP digital signature
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)


On 21/02/06, Jan Kiszka [EMAIL PROTECTED] wrote:
Dmitry Adamushko wrote: N.B. Amongst other things, some thoughts about CHAINED with shared interrupts. On 20/02/06, Anders Blomdell 
[EMAIL PROTECTED] wrote: A number of questions arise: 1. What happens if one of the shared handlers leaves the interrupt asserted,
 returns NOENABLE|HANDLED and another return only HANDLED? 2. What happens if one returns PROPAGATE and another returns HANDLED? Yep, each ISR may return a different value and all of them are
 accumulated in the s variable ( s |= intr-isr(intr); ). So the loop may end up with s which contains all of the possible bits: (e.g. isr1 - HANDLED | ENABLE
 isr2 - HANDLED (don't want the irq to be enabled) isr3 - CHAINED ) s = HANDLED | ENABLE | CHAINED; Then CHAINED will be ignored because of the following code :
 +if (s  XN_ISR_ENABLE) + xnarch_end_irq(irq); +else if (s  XN_ISR_CHAINED)(*) + xnarch_chain_irq(irq);Which may actually be fatal: consider one ISR intentionally not handling
an IRQ but propagating it while another ISR thinks it has to re-enableit - bang! Anders' as well as my original suggestion were to ignore theENABLE in this case. the current code in the CVS doen not contain else in (*), so that ENABLE |
 CHAINED is possible, though it's a wrong combination. This said, we suppose that one knows what he is doing. In the case of a single ISR per line, it's not that difficult to achieve.
 But if there are a few ISRs, then one should analize and take into account all possible return values of all the ISRs, as each of them may affect others (e.g. if one returns CHAINED when another - HANDLED | ENABLE).
 So my feeling is that CHAINED should not be used by drivers which registered their ISRs as SHARED.That's true for standard drivers, but we should not prevent specialcorner-case designs which can be tuned to make even shared RT-IRQs +
propagation possible. Sharing does not necessarily mean that differentRT drivers are involved...
Yep. But in this case the designer of the system should _care_
about all corners of the system, i.e. to make sure that all ISRs return
values which are not contradictory to each other.

e.g.

1)
isr1 : HANDLED (keep disabled)
isr2 : CHAINED

or

2)
isr1 : HANDLED | NOENABLE (will be enabled by rt_task1)
isr2 : HANDLED | NOENABLE (-//- by rt_task2)

1) and 2) are wrong! Both lead to the IRQ line being .end-ed
(xnarch_end_irq()) twice! And on some archs, as we have seen before,
that may lead to lost interrupts. 

Of course, when designing the real-time system, one should take charge
of all corners of the system, so that analyzing the possible return
values of all other ISRs that may share the same IRQ line is not
anything extravagant, but a requirement.

This said, I see no reason then why we should prevent users 
from some cases (like CHAINED | ENABLE) and let him do the ones like 2).
2) is much more common scenario for irq sharing and looks sane
from the point of view of a separate ISR, when the global picture (and
nucleus innards) is not taken into account.

p.s.
my 2) is equal to 2 ISRs returning HANDLED_NOENABLE in terms of Anders.
This case is one of the most common with shared interrupts which I
would imagine. And it nevertheless leads to problems when the whole
picture is not taken into account by the designer of a given ISR.


 Moreover, I actually see the only scenario of CHAINED (I provided it before) : all ISRs in the primary domain have reported UNHANDLED = nucleus propagates the interrupt down the pipeline with xnacrh_chain_irq().
 This call actually returns 1 upon successful propagation (some domain down the pipeline was interested in this irq) and 0 otherwise.But this only means that some handler is registered down there,

 it
cannot predict if that handler will actually care about the event.

Linux (not only vanilla, but ipipe-aware too) always .end-s (calls
desc-handler-end(irq)) on return from __do_IRQ(), no matter
this interrupt was handled by some ISR or not. It also re-enables the
IRQ line if it was not disabled by some ISR explicitly.
We don't care about anything else.


Jan
-- Best regards,Dmitry Adamushko
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

On 21/02/06, Anders Blomdell [EMAIL PROTECTED] wrote:
Dmitry Adamushko wrote: N.B. Amongst other things, some thoughts about CHAINED with shared interrupts. On 20/02/06, *Anders Blomdell* 
[EMAIL PROTECTED] mailto:[EMAIL PROTECTED] wrote: A number of questions arise:
 1. What happens if one of the shared handlers leaves the interrupt asserted, returns NOENABLE|HANDLED and another return only HANDLED? 2. What happens if one returns PROPAGATE and another returns HANDLED?
 Yep, each ISR may return a different value and all of them are accumulated in the s variable ( s |= intr-isr(intr); ). So the loop may end up with s which contains all of the possible bits:
 (e.g. isr1 - HANDLED | ENABLE isr2 - HANDLED (don't want the irq to be enabled) isr3 - CHAINED ) s = HANDLED | ENABLE | CHAINED;
 Then CHAINED will be ignored because of the following code : +if (s  XN_ISR_ENABLE) + xnarch_end_irq(irq); +else if (s  XN_ISR_CHAINED)(*) + xnarch_chain_irq(irq);
Which is the worst way possible of prioritizing them, if a Linux interrupt isactive when we get there with ENABLE|CHAINED, interrupts will be enabled withthe Linux interrupt still asserted - the IRQ-handlers will be called once more,
probably returning ENABLE|CHAINED again - infinite loop... the current code in the CVS doen not contain else in (*), so that ENABLE | CHAINED is possible, though it's a wrong combination.
 This said, we suppose that one knows what he is doing. In the case of a single ISR per line, it's not that difficult to achieve. But if there are a few ISRs, then one should analize and take
 into account all possible return values of all the ISRs, as each of them may affect others (e.g. if one returns CHAINED when another - HANDLED | ENABLE).Which is somewhat contrary to the concept of shared interrupts, if we have to
take care of the global picture, why make them shared in the first place?(I like the concept of shared interrupts, but it is important that the frameworkgives a separation of concerns)
Unfortunately, it looks to me that the current picture (even with your
scalar values) requires from the user who develops a given IRQ to take
into account the possible return values of other ISRs.

As I pointed out, the situation when 2 ISRs return HANDLED_NOENABLE may lead to problems on some archs.

---
 ...

I'll take a look at the rest of the message a bit later.

-- Best regards,Dmitry Adamushko
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)


Dmitry Adamushko wrote:


N.B. Amongst other things, some thoughts about CHAINED with shared 
interrupts.



On 20/02/06, *Anders Blomdell* [EMAIL PROTECTED] 
mailto:[EMAIL PROTECTED] wrote:




A number of questions arise:

1. What happens if one of the shared handlers leaves the interrupt
asserted,
returns NOENABLE|HANDLED and another return only HANDLED?

2. What happens if one returns PROPAGATE and another returns HANDLED?


Yep, each ISR may return a different value and all of them are
accumulated in the s variable ( s |= intr-isr(intr); ).

So the loop may end up with s which contains all of the possible bits:

(e.g.

isr1 - HANDLED | ENABLE
isr2 - HANDLED (don't want the irq to be enabled)
isr3 - CHAINED

)

s = HANDLED | ENABLE | CHAINED;

Then CHAINED will be ignored because of the following code :
 
+if (s  XN_ISR_ENABLE)

+   xnarch_end_irq(irq);
+else if (s  XN_ISR_CHAINED)(*)
+   xnarch_chain_irq(irq);

Which is the worst way possible of prioritizing them, if a Linux interrupt is
active when we get there with ENABLE|CHAINED, interrupts will be enabled with
the Linux interrupt still asserted - the IRQ-handlers will be called once more,
probably returning ENABLE|CHAINED again - infinite loop...



the current code in the CVS doen not contain else in (*), so that 
ENABLE | CHAINED is possible, though it's a wrong combination.


This said, we suppose that one knows what he is doing.

In the case of a single ISR per line, it's not that difficult to 
achieve. But if there are a few ISRs, then one should analize and take 
into account all possible return values of all the ISRs, as each of them 
may affect others (e.g. if one returns CHAINED when another - HANDLED | 
ENABLE).

Which is somewhat contrary to the concept of shared interrupts, if we have to
take care of the global picture, why make them shared in the first place?
(I like the concept of shared interrupts, but it is important that the framework
gives a separation of concerns)

So my feeling is that CHAINED should not be used by drivers which 
registered their ISRs as SHARED.

Well, CHAINED should not be used by drivers which return ENABLE (and are of
course hence incompatible with true realtime IRQ's)


Moreover, I actually see the only scenario of CHAINED (I provided it 
before) :


all ISRs in the primary domain have reported UNHANDLED  =  nucleus 
propagates the interrupt down the pipeline with xnacrh_chain_irq().
This call actually returns 1 upon successful propagation (some domain 
down the pipeline was interested in this irq) and 0 otherwise.


Upon 0, this is a spurious irq (none of domains was interested in its 
handling).


ok, let's suppose now :

we have 2 ISRs on the same shared line :

isr1 : HANDLED (will be enabled by rt task. Note, rt task must call 
xnarch_end_irq() and not just xnarch_enable_irq()! )


isr2 : CHAINED

So HANDLED | CHAINED is ok for the single ISR on the line, but it may 
lead to HANDLED | CHAINED | ENABLE in a case of the shared line.


rt task that works jointly with isr1 just calls xnarch_end_irq() at some 
moment of time and some ISR in the linux domain does the same later  =  
the line is .end-ed 2 times.


ISR should never return CHAINED as to indicate _only_ that it is not 
interested in this irq, but ~HANDLED or NOINT (if we'll support it) instead.


If the ISR nevertheless wants to propagate the IRQ to the Linux domain 
_explicitly_, it _must not_ register itself as SHARED, i.e. it _must_ be 
the only ISR on this line, otherwise that may lead to the IRQ line being 
.end-ed twice (lost interrupts in some cases).



#define UNHANDLED 0
#define HANDLED_ENABLE 1
#define HANDLED_NOENABLE 2
#define PROPAGATE 3 



Yep, I'd agree with you. Moreover, PROPAGATE should not be used for 
shared interrupts.

My feeling is that it should be considered an error to attach a RT IRQ handler
to a line that has a Linux IRQ handler (this should be possible to check, since
/proc/interrupts contains the relevant information), unless a Linux IRQ-mask
function is installed. This IRQ-mask function should the be called:

  1. each time domains are switched
  2. each time an interrupt is generated

The IRQ-mask function should look something like:

unsigned int rt_irq_mask(struct ipipe_domain *ipd, unsigned int irq)
{
  int result = 0;
  static int enabled = true;
  int enable = enabled;

  if (irq = 0) {
// Interrupt has occured, we are about to run IRQ handlers
if (disable_early) {
  enable = false;
}
if (for_linux(irq)) {
  result = XN_ISR_CHAINED;
}
  } else if (ipd == ipipe_root_domain) {
// Entering Linux
enable = true;
  } else {
// Other doamin, block linux interrupts
enable = false;
  }
  if (enable != enabled) {
enabled = enable
if (enable) {
  // Enable Linux interrupts by unmasking appropriate
  // device registers (and possibly entire IRQ's)
} else {
  // Disable Linux interrupts by

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)


 Good point, leaves us with 2 possible return values for shared handlers:
 
 HANDLED
 NOT_HANDLED
 
 i.e. shared handlers should never defer the end'ing of the interrupt (which
 makes sense, since this would affect the other [shared] handlers).

HANDLED_NOEBNABLE could be supported too. There would be no need in reenventing
a wheel, just do it the way Linux does it.
But it's about some additional re-designing of the current codebase
(e.g. nested calling for irq_enable/disable())
I'm not sure we do need it for something else rather than irq sharing code but it affects the rest of the code.

And we have a kind of wrong concept :

XN_ISR_ENABLE (or NOENABLE) corresponds to xnarch_end_irq().

But the later one is not only about enabling the line, but
on some archs - about .end-ing it too (sending EOI).

And to support HANDLED_NOENABLE properly, those 2 have to be decoupled, i.e.
EOI should always be sent from xnintr_shirq_handler().

 Yes, should. And this should is best be handled by
 
 a) Documenting the potential conflict in the same place when describing
 the return values
 
 b) Placing some debug warning in the nucleus' IRQ trampoline function to
 bail out (once per line) when running into such situation
 
 But I'm against any further runtime restrictions, especially as most
 drivers will never return anything else than NOT_HANDLED or HANDLED.
 Actually, this was the reason why I tried to separate the NO_ENABLE and
 PROPAGATE features as *additional* bits from HANDLED and
 NOT_HANDLED/UNHANDLED/NOINT. But I acknowledge that having all valid bit
 combination present as constants can be more helpful for the user. We
 just have to draw some line between the standard values and the
 additional gurus return codes 

(documentation: don't use NO_ENABLE or
 PROPAGATE unless you understand their side-effects and pitfalls precisely).

I agree with you on PROPAGATE case, but NO_ENABLE that, as pointed out above,
should (IMHO and at least, in theory) only mean keep the IRQ line disabled
(and have nothing to do with .end-ing the IRQ line) would be better supported.
But this is, again as was pointed out above, about some redesigning of the
current code = some overhead that likely affects non-shared aware code too.


So on one hand,

I'm ready to re-work code with :

HANDLED and UNHANDLED (or NOINT)

+ 2 additional bits : NOENABLE and PROPAGATE.

and document it like you suggested don't use NO_ENABLE or
PROPAGATE with shared interrupts 
unless you understand their side-effects and pitfalls precisely;

on the other hand,

I'd say that I'm almost ready to vote against merging the irq sharing code at all as it looks to be a rather partial solution.
-- Best regards,Dmitry Adamushko
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)


Dmitry Adamushko wrote:


  Good point, leaves us with 2 possible return values for shared handlers:
 
HANDLED
NOT_HANDLED
 
  i.e. shared handlers should never defer the end'ing of the interrupt 
(which

  makes sense, since this would affect the other [shared] handlers).

HANDLED_NOEBNABLE could be supported too. 
Yes, but it breaks decoupling between shared handlers; interrupts will be 
deferred for all [shared] handlers until it is properly ended.


There would be no need in 
reenventing

a wheel, just do it the way Linux does it.
But it's about some additional re-designing of the current codebase
(e.g. nested calling for irq_enable/disable())
I'm not sure we do need it for something else rather than irq sharing 
code but it affects the rest of the code.


And we have a kind of wrong concept :

XN_ISR_ENABLE (or NOENABLE) corresponds to xnarch_end_irq().

Agree



But the later one is not only about enabling the line, but
on some archs - about .end-ing it too (sending EOI).

And to support HANDLED_NOENABLE properly, those 2 have to be decoupled, i.e.
EOI should always be sent from xnintr_shirq_handler().
But the one returning HANDLED_NOENABLE is likely to leave the interrupt 
asserted, hence we can't EOI at this point (unless NO_ENABLE means DISABLE).




  Yes, should. And this should is best be handled by
 
  a) Documenting the potential conflict in the same place when describing
  the return values
 
  b) Placing some debug warning in the nucleus' IRQ trampoline function to
  bail out (once per line) when running into such situation
 
  But I'm against any further runtime restrictions, especially as most
  drivers will never return anything else than NOT_HANDLED or HANDLED.
  Actually, this was the reason why I tried to separate the NO_ENABLE and
  PROPAGATE features as *additional* bits from HANDLED and
  NOT_HANDLED/UNHANDLED/NOINT. But I acknowledge that having all valid bit
  combination present as constants can be more helpful for the user. We
  just have to draw some line between the standard values and the
  additional gurus return codes
 
(documentation: don't use NO_ENABLE or
  PROPAGATE unless you understand their side-effects and pitfalls 
precisely).


I agree with you on PROPAGATE case, but NO_ENABLE that, as pointed out 
above,

should (IMHO and at least, in theory) only mean keep the IRQ line disabled
(and have nothing to do with .end-ing the IRQ line) would be better 
supported.

But this is, again as was pointed out above, about some redesigning of the
current code = some overhead that likely affects non-shared aware code too.


So on one hand,

I'm ready to re-work code with :

HANDLED and UNHANDLED (or NOINT)

+ 2 additional bits : NOENABLE and PROPAGATE.

and document it like you suggested don't use NO_ENABLE or
PROPAGATE with shared interrupts
unless you understand their side-effects and pitfalls precisely;

on the other hand,

I'd say that I'm almost ready to vote against merging the irq sharing 
code at all as it looks to be a rather partial solution.
I vote for (even though I'm the one who complains the most), BUT I think it is 
important to keep the rules for using it simple (that's why I worry about the 
plethora of return-flags).



--

Regards

Anders

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)


Jan Kiszka wrote:

Anders Blomdell wrote:


Dmitry Adamushko wrote:


 Good point, leaves us with 2 possible return values for shared
handlers:

   HANDLED
   NOT_HANDLED

 i.e. shared handlers should never defer the end'ing of the
interrupt (which
 makes sense, since this would affect the other [shared] handlers).

HANDLED_NOEBNABLE could be supported too. 


Yes, but it breaks decoupling between shared handlers; interrupts will
be deferred for all [shared] handlers until it is properly ended.



There would be no need in reenventing
a wheel, just do it the way Linux does it.
But it's about some additional re-designing of the current codebase
(e.g. nested calling for irq_enable/disable())
I'm not sure we do need it for something else rather than irq sharing
code but it affects the rest of the code.

And we have a kind of wrong concept :

XN_ISR_ENABLE (or NOENABLE) corresponds to xnarch_end_irq().


Agree



But the later one is not only about enabling the line, but
on some archs - about .end-ing it too (sending EOI).

And to support HANDLED_NOENABLE properly, those 2 have to be
decoupled, i.e.
EOI should always be sent from xnintr_shirq_handler().


But the one returning HANDLED_NOENABLE is likely to leave the interrupt
asserted, hence we can't EOI at this point (unless NO_ENABLE means
DISABLE).



I guess this is what Dmitry meant: explicitly call disable() if one or
more ISRs returned NOENABLE - at least on archs where end != enable.
Will this work? We could then keep on using the existing IRQ-enable API
from bottom-half IRQ tasks. But what about NOENABLE+PROPAGATE? Will this
special case still mean NOT to end the ISR (as Linux will do)?

Bah, we are running in circles, I'm afraid. I think it's better to call
NOENABLE NOEOI, which will indeed mean to not end this line (as it is
the current situation anyway, isn't it?), and leave the user with what
(s)he can do with such a feature. We found out that there are trillions
of ways to shoot oneself into the foot with NOENABLE and PROPAGATE, and
we cannot prevent most of them. So let's stop trying, at least for this
patch!



 Yes, should. And this should is best be handled by

 a) Documenting the potential conflict in the same place when
describing
 the return values

 b) Placing some debug warning in the nucleus' IRQ trampoline
function to
 bail out (once per line) when running into such situation

 But I'm against any further runtime restrictions, especially as most
 drivers will never return anything else than NOT_HANDLED or HANDLED.
 Actually, this was the reason why I tried to separate the NO_ENABLE
and
 PROPAGATE features as *additional* bits from HANDLED and
 NOT_HANDLED/UNHANDLED/NOINT. But I acknowledge that having all
valid bit
 combination present as constants can be more helpful for the user. We
 just have to draw some line between the standard values and the
 additional gurus return codes

(documentation: don't use NO_ENABLE or
 PROPAGATE unless you understand their side-effects and pitfalls
precisely).

I agree with you on PROPAGATE case, but NO_ENABLE that, as pointed out
above,
should (IMHO and at least, in theory) only mean keep the IRQ line
disabled
(and have nothing to do with .end-ing the IRQ line) would be better
supported.
But this is, again as was pointed out above, about some redesigning of
the
current code = some overhead that likely affects non-shared aware
code too.


So on one hand,

I'm ready to re-work code with :

HANDLED and UNHANDLED (or NOINT)

+ 2 additional bits : NOENABLE and PROPAGATE.

and document it like you suggested don't use NO_ENABLE or
PROPAGATE with shared interrupts
unless you understand their side-effects and pitfalls precisely;

on the other hand,

I'd say that I'm almost ready to vote against merging the irq sharing
code at all as it looks to be a rather partial solution.


I vote for (even though I'm the one who complains the most), BUT I think
it is important to keep the rules for using it simple (that's why I
worry about the plethora of return-flags).




And I'm with you here: My original proposal (2 base-states + 2 bits)
created 8 expressible states while your version only knows 4 states -
those which make sense most (and 2 of them are still the ones recommand
for the masses).

For RTDM I'm now almost determined to rework the API in way that only
HANDLED/UNHANDLED (or what ever their names will be) get exported, any
additional guru features will remain excluded as long as we have no
clean usage policy for them.

You have my vote for this.

--
Anders

___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

Anders Blomdell wrote:
 Jan Kiszka wrote:
 Hi Dmitry,

 Dmitry Adamushko wrote:

 Hi Jan,

 let's make yet another revision of the bits :

 new XN_ISR_HANDLED  == old XN_ISR_HANDLED + old XN_ISR_NO_ENABLE

 ok.

 new XN_ISR_NOENABLE == ~ old XN_ISR_ENABLE

 ok.

 new XN_ISR_PROPAGATE == XN_ISR_CHAINED

 ok.



 Just to make sure that you understand my weird ideas: each of the three
 new XN_ISR_xxx above should be encoded with an individual bit


 new XN_ISR_NOINT == ?

 does it suppose the interrupt line to be .end-ed (enabled) and irq
 not to be
 propagated? Should be so, I guess, if it's different from 5). Then
 nucleus
 ignores implicit IRQ enable for 5) as well as for 3).

 Do we really need that NOINT then, as it seems to be the same as
 ~HANDLED?

 or NOINT == 0 and then it's a scalar value, not a bit.

 So one may consider HANDLED == 1 and NOINT == 0 as really scalar values

 and

 NOENABLE and PROPAGATE as additional bits (used only if needed).



 My idea is to urge the user specifying one of the base return types
 (HANDLED or NOINT) + any of the two additional bits (NOENABLE and
 PROPAGATE).

 For correct drivers NOINT could be 0 indeed, but to check that the user
 picked a new constant we may want to set NOINT != 0. With the old API
 return 0 expressed HANDLED + ~ENABLE for the old API. With the new one
 the user signals no interest and the nucleus may raise a warning that a
 spurious IRQ occurred. So I would add a debug bit for NOINT here to
 optionally (on OPT_XENO_DEBUG) detect old-style usage (return 0).
 Moreover, we gain freedom to move bits in the future when every state is
 encoded via constants. Or am I too paranoid here?
 After reading the above discussion (of which I understand very little),
 and looking at (what I believe to be) the relevant code:
 
 +intr = shirq-handlers;
 +
 +while (intr)
 +{
 +s |= intr-isr(intr);
 +++intr-hits;
 +intr = intr-next;
 +}
 +xnintr_shirq_unlock(shirq);
 +
 +--sched-inesting;
 +
 +if (s  XN_ISR_ENABLE)
 +   xnarch_end_irq(irq);
 +else if (s  XN_ISR_CHAINED)
 +   xnarch_chain_irq(irq);
 
 A number of questions arise:
 
 1. What happens if one of the shared handlers leaves the interrupt
 asserted, returns NOENABLE|HANDLED and another return only HANDLED?
 
 2. What happens if one returns PROPAGATE and another returns HANDLED?
 
 As far as I can tell, after all RT handlers havve run, the following
 scenarios are possible:
 
 1. The interrupt is deasserted (i.e. it was a RT interrupt)
 2. The interrupt is still asserted, it will be deasserted later
by some RT task (i.e. it was a RT interrupt)
 3. The interrupt is still asserted and will be deasserted
by the Linux IRQ handler.
 
 IMHO that leads to the conclusion that the IRQ handlers should return a
 scalar
 
 #define UNHANDLED 0
 #define HANDLED_ENABLE 1
 #define HANDLED_NOENABLE 2
 #define PROPAGATE 3
 
 and the loop should be
 
 s = UNHANDLED
 while (intr) {
   tmp = intr-isr(intr);
   if (tmp  s) { s = tmp; }
   intr = intr-next;
 }
 if (s == PROPAGATE) {
   xnarch_chain_irq(irq);
 } else if (s == HANDLED_ENABLE) {
   xnarch_end_irq(irq);
 }

This is a very useful suggestion, specifically this escalation of the
return value in the shared case. I would just let UNHANLDED start with 1
for the reasons I explained here:

https://mail.gna.org/public/xenomai-core/2006-02/msg00186.html

Ok, I would say let's got for this and finalise the patch!

 
 To be really honest, I think that PROPAGATE should be excluded from the
 RT IRQ-handlers, since with the current scheme all RT-handlers has to
 test if the IRQ was a Linux interrupt (otherwise the system will only
 work when the handler that returns PROPAGATE is installed)
 

Indeed, but I'm with Philippe here: do not exclude the strange corner
case scenarios users may craft with the appropriate care. I could, e.g.,
imagine a scenario where an RT handler takes the arrival time stamp of a
non-RT IRQ and then propagates it.

Jsn



signature.asc
Description: OpenPGP digital signature

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

Dmitry Adamushko wrote:
 N.B. Amongst other things, some thoughts about CHAINED with shared
 interrupts.
 
 
 On 20/02/06, Anders Blomdell [EMAIL PROTECTED] wrote:
 
 
 
 A number of questions arise:

 1. What happens if one of the shared handlers leaves the interrupt
 asserted,
 returns NOENABLE|HANDLED and another return only HANDLED?

 2. What happens if one returns PROPAGATE and another returns HANDLED?
 
 
 Yep, each ISR may return a different value and all of them are
 accumulated in the s variable ( s |= intr-isr(intr); ).
 
 So the loop may end up with s which contains all of the possible bits:
 
 (e.g.
 
 isr1 - HANDLED | ENABLE
 isr2 - HANDLED (don't want the irq to be enabled)
 isr3 - CHAINED
 
 )
 
 s = HANDLED | ENABLE | CHAINED;
 
 Then CHAINED will be ignored because of the following code :
 
 +if (s  XN_ISR_ENABLE)
 +   xnarch_end_irq(irq);
 +else if (s  XN_ISR_CHAINED)(*)
 +   xnarch_chain_irq(irq);

Which may actually be fatal: consider one ISR intentionally not handling
an IRQ but propagating it while another ISR thinks it has to re-enable
it - bang! Anders' as well as my original suggestion were to ignore the
ENABLE in this case.

 
 the current code in the CVS doen not contain else in (*), so that ENABLE |
 CHAINED is possible, though it's a wrong combination.
 
 This said, we suppose that one knows what he is doing.
 
 In the case of a single ISR per line, it's not that difficult to achieve.
 But if there are a few ISRs, then one should analize and take into account
 all possible return values of all the ISRs, as each of them may affect
 others (e.g. if one returns CHAINED when another - HANDLED | ENABLE).
 
 So my feeling is that CHAINED should not be used by drivers which registered
 their ISRs as SHARED.

That's true for standard drivers, but we should not prevent special
corner-case designs which can be tuned to make even shared RT-IRQs +
propagation possible. Sharing does not necessarily mean that different
RT drivers are involved...

 
 Moreover, I actually see the only scenario of CHAINED (I provided it before)
 :
 
 all ISRs in the primary domain have reported UNHANDLED  =  nucleus
 propagates the interrupt down the pipeline with xnacrh_chain_irq().
 This call actually returns 1 upon successful propagation (some domain down
 the pipeline was interested in this irq) and 0 otherwise.

But this only means that some handler is registered down there, it
cannot predict if that handler will actually care about the event.

 
 Upon 0, this is a spurious irq (none of domains was interested in its
 handling).
 
 ok, let's suppose now :
 
 we have 2 ISRs on the same shared line :
 
 isr1 : HANDLED (will be enabled by rt task. Note, rt task must call
 xnarch_end_irq() and not just xnarch_enable_irq()! )
 
 isr2 : CHAINED
 
 So HANDLED | CHAINED is ok for the single ISR on the line, but it may lead
 to HANDLED | CHAINED | ENABLE in a case of the shared line.
 
 rt task that works jointly with isr1 just calls xnarch_end_irq() at some
 moment of time and some ISR in the linux domain does the same later  =  the
 line is .end-ed 2 times.
 
 ISR should never return CHAINED as to indicate _only_ that it is not
 interested in this irq, but ~HANDLED or NOINT (if we'll support it) instead.
 
 If the ISR nevertheless wants to propagate the IRQ to the Linux domain
 _explicitly_, it _must not_ register itself as SHARED, i.e. it _must_ be the
 only ISR on this line, otherwise that may lead to the IRQ line being .end-ed
 twice (lost interrupts in some cases).
 
 
 #define UNHANDLED 0
 #define HANDLED_ENABLE 1
 #define HANDLED_NOENABLE 2
 #define PROPAGATE 3
 
 
 Yep, I'd agree with you. Moreover, PROPAGATE should not be used for shared
 interrupts.
 
 
 --
 Best regards,
 Dmitry Adamushko
 

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)


On 21/02/06, Jan Kiszka [EMAIL PROTECTED] wrote:
Dmitry Adamushko wrote: N.B. Amongst other things, some thoughts about CHAINED with shared interrupts. On 20/02/06, Anders Blomdell 
[EMAIL PROTECTED] wrote: A number of questions arise: 1. What happens if one of the shared handlers leaves the interrupt asserted,
 returns NOENABLE|HANDLED and another return only HANDLED? 2. What happens if one returns PROPAGATE and another returns HANDLED? Yep, each ISR may return a different value and all of them are
 accumulated in the s variable ( s |= intr-isr(intr); ). So the loop may end up with s which contains all of the possible bits: (e.g. isr1 - HANDLED | ENABLE
 isr2 - HANDLED (don't want the irq to be enabled) isr3 - CHAINED ) s = HANDLED | ENABLE | CHAINED; Then CHAINED will be ignored because of the following code :
 +if (s  XN_ISR_ENABLE) + xnarch_end_irq(irq); +else if (s  XN_ISR_CHAINED)(*) + xnarch_chain_irq(irq);Which may actually be fatal: consider one ISR intentionally not handling
an IRQ but propagating it while another ISR thinks it has to re-enableit - bang! Anders' as well as my original suggestion were to ignore theENABLE in this case. the current code in the CVS doen not contain else in (*), so that ENABLE |
 CHAINED is possible, though it's a wrong combination. This said, we suppose that one knows what he is doing. In the case of a single ISR per line, it's not that difficult to achieve.
 But if there are a few ISRs, then one should analize and take into account all possible return values of all the ISRs, as each of them may affect others (e.g. if one returns CHAINED when another - HANDLED | ENABLE).
 So my feeling is that CHAINED should not be used by drivers which registered their ISRs as SHARED.That's true for standard drivers, but we should not prevent specialcorner-case designs which can be tuned to make even shared RT-IRQs +
propagation possible. Sharing does not necessarily mean that differentRT drivers are involved...
Yep. But in this case the designer of the system should _care_
about all corners of the system, i.e. to make sure that all ISRs return
values which are not contradictory to each other.

e.g.

1)
isr1 : HANDLED (keep disabled)
isr2 : CHAINED

or

2)
isr1 : HANDLED | NOENABLE (will be enabled by rt_task1)
isr2 : HANDLED | NOENABLE (-//- by rt_task2)

1) and 2) are wrong! Both lead to the IRQ line being .end-ed
(xnarch_end_irq()) twice! And on some archs, as we have seen before,
that may lead to lost interrupts. 

Of course, when designing the real-time system, one should take charge
of all corners of the system, so that analyzing the possible return
values of all other ISRs that may share the same IRQ line is not
anything extravagant, but a requirement.

This said, I see no reason then why we should prevent users 
from some cases (like CHAINED | ENABLE) and let him do the ones like 2).
2) is much more common scenario for irq sharing and looks sane
from the point of view of a separate ISR, when the global picture (and
nucleus innards) is not taken into account.

p.s.
my 2) is equal to 2 ISRs returning HANDLED_NOENABLE in terms of Anders.
This case is one of the most common with shared interrupts which I
would imagine. And it nevertheless leads to problems when the whole
picture is not taken into account by the designer of a given ISR.


 Moreover, I actually see the only scenario of CHAINED (I provided it before) : all ISRs in the primary domain have reported UNHANDLED = nucleus propagates the interrupt down the pipeline with xnacrh_chain_irq().
 This call actually returns 1 upon successful propagation (some domain down the pipeline was interested in this irq) and 0 otherwise.But this only means that some handler is registered down there,

 it
cannot predict if that handler will actually care about the event.

Linux (not only vanilla, but ipipe-aware too) always .end-s (calls
desc-handler-end(irq)) on return from __do_IRQ(), no matter
this interrupt was handled by some ISR or not. It also re-enables the
IRQ line if it was not disabled by some ISR explicitly.
We don't care about anything else.


Jan
-- Best regards,Dmitry Adamushko

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

On 21/02/06, Anders Blomdell [EMAIL PROTECTED] wrote:
Dmitry Adamushko wrote: N.B. Amongst other things, some thoughts about CHAINED with shared interrupts. On 20/02/06, *Anders Blomdell* 
[EMAIL PROTECTED] mailto:[EMAIL PROTECTED] wrote: A number of questions arise:
 1. What happens if one of the shared handlers leaves the interrupt asserted, returns NOENABLE|HANDLED and another return only HANDLED? 2. What happens if one returns PROPAGATE and another returns HANDLED?
 Yep, each ISR may return a different value and all of them are accumulated in the s variable ( s |= intr-isr(intr); ). So the loop may end up with s which contains all of the possible bits:
 (e.g. isr1 - HANDLED | ENABLE isr2 - HANDLED (don't want the irq to be enabled) isr3 - CHAINED ) s = HANDLED | ENABLE | CHAINED;
 Then CHAINED will be ignored because of the following code : +if (s  XN_ISR_ENABLE) + xnarch_end_irq(irq); +else if (s  XN_ISR_CHAINED)(*) + xnarch_chain_irq(irq);
Which is the worst way possible of prioritizing them, if a Linux interrupt isactive when we get there with ENABLE|CHAINED, interrupts will be enabled withthe Linux interrupt still asserted - the IRQ-handlers will be called once more,
probably returning ENABLE|CHAINED again - infinite loop... the current code in the CVS doen not contain else in (*), so that ENABLE | CHAINED is possible, though it's a wrong combination.
 This said, we suppose that one knows what he is doing. In the case of a single ISR per line, it's not that difficult to achieve. But if there are a few ISRs, then one should analize and take
 into account all possible return values of all the ISRs, as each of them may affect others (e.g. if one returns CHAINED when another - HANDLED | ENABLE).Which is somewhat contrary to the concept of shared interrupts, if we have to
take care of the global picture, why make them shared in the first place?(I like the concept of shared interrupts, but it is important that the frameworkgives a separation of concerns)
Unfortunately, it looks to me that the current picture (even with your
scalar values) requires from the user who develops a given IRQ to take
into account the possible return values of other ISRs.

As I pointed out, the situation when 2 ISRs return HANDLED_NOENABLE may lead to problems on some archs.

---
 ...

I'll take a look at the rest of the message a bit later.

-- Best regards,Dmitry Adamushko

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)


Dmitry Adamushko wrote:


On 21/02/06, *Anders Blomdell* [EMAIL PROTECTED] 
mailto:[EMAIL PROTECTED] wrote:


Dmitry Adamushko wrote:
 
  N.B. Amongst other things, some thoughts about CHAINED with shared
  interrupts.
 
 
  On 20/02/06, *Anders Blomdell*  [EMAIL PROTECTED]
mailto:[EMAIL PROTECTED]
  mailto:[EMAIL PROTECTED]
mailto:[EMAIL PROTECTED] wrote:
 
 
 
  A number of questions arise:
 
  1. What happens if one of the shared handlers leaves the
interrupt
  asserted,
  returns NOENABLE|HANDLED and another return only HANDLED?
 
  2. What happens if one returns PROPAGATE and another returns
HANDLED?
 
 
  Yep, each ISR may return a different value and all of them are
  accumulated in the s variable ( s |= intr-isr(intr); ).
 
  So the loop may end up with s which contains all of the
possible bits:
 
  (e.g.
 
  isr1 - HANDLED | ENABLE
  isr2 - HANDLED (don't want the irq to be enabled)
  isr3 - CHAINED
 
  )
 
  s = HANDLED | ENABLE | CHAINED;
 
  Then CHAINED will be ignored because of the following code :
 
  +if (s  XN_ISR_ENABLE)
  +   xnarch_end_irq(irq);
  +else if (s  XN_ISR_CHAINED)(*)
  +   xnarch_chain_irq(irq);
Which is the worst way possible of prioritizing them, if a Linux
interrupt is
active when we get there with ENABLE|CHAINED, interrupts will be
enabled with
the Linux interrupt still asserted - the IRQ-handlers will be
called once more,
probably returning ENABLE|CHAINED again - infinite loop...

 
  the current code in the CVS doen not contain else in (*), so that
  ENABLE | CHAINED is possible, though it's a wrong combination.
 
  This said, we suppose that one knows what he is doing.
 
  In the case of a single ISR per line, it's not that difficult to
  achieve. But if there are a few ISRs, then one should analize and
take
  into account all possible return values of all the ISRs, as each
of them
  may affect others (e.g. if one returns CHAINED when another -
HANDLED |
  ENABLE).
Which is somewhat contrary to the concept of shared interrupts, if
we have to
take care of the global picture, why make them shared in the first
place?
(I like the concept of shared interrupts, but it is important that
the framework
gives a separation of concerns)


Unfortunately, it looks to me that the current picture (even with your 
scalar values) requires from the user who develops a given IRQ to take 
into account the possible return values of other ISRs.


As I pointed out, the situation when 2 ISRs return HANDLED_NOENABLE may 
lead to problems on some archs.

Good point, leaves us with 2 possible return values for shared handlers:

  HANDLED
  NOT_HANDLED

i.e. shared handlers should never defer the end'ing of the interrupt (which 
makes sense, since this would affect the other [shared] handlers).


--
Anders

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)


Dmitry Adamushko wrote:


N.B. Amongst other things, some thoughts about CHAINED with shared 
interrupts.



On 20/02/06, *Anders Blomdell* [EMAIL PROTECTED] 
mailto:[EMAIL PROTECTED] wrote:




A number of questions arise:

1. What happens if one of the shared handlers leaves the interrupt
asserted,
returns NOENABLE|HANDLED and another return only HANDLED?

2. What happens if one returns PROPAGATE and another returns HANDLED?


Yep, each ISR may return a different value and all of them are
accumulated in the s variable ( s |= intr-isr(intr); ).

So the loop may end up with s which contains all of the possible bits:

(e.g.

isr1 - HANDLED | ENABLE
isr2 - HANDLED (don't want the irq to be enabled)
isr3 - CHAINED

)

s = HANDLED | ENABLE | CHAINED;

Then CHAINED will be ignored because of the following code :
 
+if (s  XN_ISR_ENABLE)

+   xnarch_end_irq(irq);
+else if (s  XN_ISR_CHAINED)(*)
+   xnarch_chain_irq(irq);

Which is the worst way possible of prioritizing them, if a Linux interrupt is
active when we get there with ENABLE|CHAINED, interrupts will be enabled with
the Linux interrupt still asserted - the IRQ-handlers will be called once more,
probably returning ENABLE|CHAINED again - infinite loop...



the current code in the CVS doen not contain else in (*), so that 
ENABLE | CHAINED is possible, though it's a wrong combination.


This said, we suppose that one knows what he is doing.

In the case of a single ISR per line, it's not that difficult to 
achieve. But if there are a few ISRs, then one should analize and take 
into account all possible return values of all the ISRs, as each of them 
may affect others (e.g. if one returns CHAINED when another - HANDLED | 
ENABLE).

Which is somewhat contrary to the concept of shared interrupts, if we have to
take care of the global picture, why make them shared in the first place?
(I like the concept of shared interrupts, but it is important that the framework
gives a separation of concerns)

So my feeling is that CHAINED should not be used by drivers which 
registered their ISRs as SHARED.

Well, CHAINED should not be used by drivers which return ENABLE (and are of
course hence incompatible with true realtime IRQ's)


Moreover, I actually see the only scenario of CHAINED (I provided it 
before) :


all ISRs in the primary domain have reported UNHANDLED  =  nucleus 
propagates the interrupt down the pipeline with xnacrh_chain_irq().
This call actually returns 1 upon successful propagation (some domain 
down the pipeline was interested in this irq) and 0 otherwise.


Upon 0, this is a spurious irq (none of domains was interested in its 
handling).


ok, let's suppose now :

we have 2 ISRs on the same shared line :

isr1 : HANDLED (will be enabled by rt task. Note, rt task must call 
xnarch_end_irq() and not just xnarch_enable_irq()! )


isr2 : CHAINED

So HANDLED | CHAINED is ok for the single ISR on the line, but it may 
lead to HANDLED | CHAINED | ENABLE in a case of the shared line.


rt task that works jointly with isr1 just calls xnarch_end_irq() at some 
moment of time and some ISR in the linux domain does the same later  =  
the line is .end-ed 2 times.


ISR should never return CHAINED as to indicate _only_ that it is not 
interested in this irq, but ~HANDLED or NOINT (if we'll support it) instead.


If the ISR nevertheless wants to propagate the IRQ to the Linux domain 
_explicitly_, it _must not_ register itself as SHARED, i.e. it _must_ be 
the only ISR on this line, otherwise that may lead to the IRQ line being 
.end-ed twice (lost interrupts in some cases).



#define UNHANDLED 0
#define HANDLED_ENABLE 1
#define HANDLED_NOENABLE 2
#define PROPAGATE 3 



Yep, I'd agree with you. Moreover, PROPAGATE should not be used for 
shared interrupts.

My feeling is that it should be considered an error to attach a RT IRQ handler
to a line that has a Linux IRQ handler (this should be possible to check, since
/proc/interrupts contains the relevant information), unless a Linux IRQ-mask
function is installed. This IRQ-mask function should the be called:

  1. each time domains are switched
  2. each time an interrupt is generated

The IRQ-mask function should look something like:

unsigned int rt_irq_mask(struct ipipe_domain *ipd, unsigned int irq)
{
  int result = 0;
  static int enabled = true;
  int enable = enabled;

  if (irq = 0) {
// Interrupt has occured, we are about to run IRQ handlers
if (disable_early) {
  enable = false;
}
if (for_linux(irq)) {
  result = XN_ISR_CHAINED;
}
  } else if (ipd == ipipe_root_domain) {
// Entering Linux
enable = true;
  } else {
// Other doamin, block linux interrupts
enable = false;
  }
  if (enable != enabled) {
enabled = enable
if (enable) {
  // Enable Linux interrupts by unmasking appropriate
  // device registers (and possibly entire IRQ's)
} else {
  // Disable Linux interrupts by

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

Anders Blomdell wrote:
 Dmitry Adamushko wrote:

 On 21/02/06, *Anders Blomdell* [EMAIL PROTECTED]
 mailto:[EMAIL PROTECTED] wrote:

 Dmitry Adamushko wrote:
  
   N.B. Amongst other things, some thoughts about CHAINED with shared
   interrupts.
  
  
   On 20/02/06, *Anders Blomdell*  [EMAIL PROTECTED]
 mailto:[EMAIL PROTECTED]
   mailto:[EMAIL PROTECTED]
 mailto:[EMAIL PROTECTED] wrote:
  
  
  
   A number of questions arise:
  
   1. What happens if one of the shared handlers leaves the
 interrupt
   asserted,
   returns NOENABLE|HANDLED and another return only HANDLED?
  
   2. What happens if one returns PROPAGATE and another returns
 HANDLED?
  
  
   Yep, each ISR may return a different value and all of them are
   accumulated in the s variable ( s |= intr-isr(intr); ).
  
   So the loop may end up with s which contains all of the
 possible bits:
  
   (e.g.
  
   isr1 - HANDLED | ENABLE
   isr2 - HANDLED (don't want the irq to be enabled)
   isr3 - CHAINED
  
   )
  
   s = HANDLED | ENABLE | CHAINED;
  
   Then CHAINED will be ignored because of the following code :
  
   +if (s  XN_ISR_ENABLE)
   +   xnarch_end_irq(irq);
   +else if (s  XN_ISR_CHAINED)(*)
   +   xnarch_chain_irq(irq);
 Which is the worst way possible of prioritizing them, if a Linux
 interrupt is
 active when we get there with ENABLE|CHAINED, interrupts will be
 enabled with
 the Linux interrupt still asserted - the IRQ-handlers will be
 called once more,
 probably returning ENABLE|CHAINED again - infinite loop...

  
   the current code in the CVS doen not contain else in (*), so
 that
   ENABLE | CHAINED is possible, though it's a wrong combination.
  
   This said, we suppose that one knows what he is doing.
  
   In the case of a single ISR per line, it's not that difficult to
   achieve. But if there are a few ISRs, then one should analize and
 take
   into account all possible return values of all the ISRs, as each
 of them
   may affect others (e.g. if one returns CHAINED when another -
 HANDLED |
   ENABLE).
 Which is somewhat contrary to the concept of shared interrupts, if
 we have to
 take care of the global picture, why make them shared in the first
 place?
 (I like the concept of shared interrupts, but it is important that
 the framework
 gives a separation of concerns)


 Unfortunately, it looks to me that the current picture (even with your
 scalar values) requires from the user who develops a given IRQ to take
 into account the possible return values of other ISRs.

 As I pointed out, the situation when 2 ISRs return HANDLED_NOENABLE
 may lead to problems on some archs.
 Good point, leaves us with 2 possible return values for shared handlers:
 
   HANDLED
   NOT_HANDLED
 
 i.e. shared handlers should never defer the end'ing of the interrupt
 (which makes sense, since this would affect the other [shared] handlers).
 

Yes, should. And this should is best be handled by

a) Documenting the potential conflict in the same place when describing
the return values

b) Placing some debug warning in the nucleus' IRQ trampoline function to
bail out (once per line) when running into such situation

But I'm against any further runtime restrictions, especially as most
drivers will never return anything else than NOT_HANDLED or HANDLED.
Actually, this was the reason why I tried to separate the NO_ENABLE and
PROPAGATE features as *additional* bits from HANDLED and
NOT_HANDLED/UNHANDLED/NOINT. But I acknowledge that having all valid bit
combination present as constants can be more helpful for the user. We
just have to draw some line between the standard values and the
additional gurus return codes (documentation: don't use NO_ENABLE or
PROPAGATE unless you understand their side-effects and pitfalls precisely).

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)


 Good point, leaves us with 2 possible return values for shared handlers:
 
 HANDLED
 NOT_HANDLED
 
 i.e. shared handlers should never defer the end'ing of the interrupt (which
 makes sense, since this would affect the other [shared] handlers).

HANDLED_NOEBNABLE could be supported too. There would be no need in reenventing
a wheel, just do it the way Linux does it.
But it's about some additional re-designing of the current codebase
(e.g. nested calling for irq_enable/disable())
I'm not sure we do need it for something else rather than irq sharing code but it affects the rest of the code.

And we have a kind of wrong concept :

XN_ISR_ENABLE (or NOENABLE) corresponds to xnarch_end_irq().

But the later one is not only about enabling the line, but
on some archs - about .end-ing it too (sending EOI).

And to support HANDLED_NOENABLE properly, those 2 have to be decoupled, i.e.
EOI should always be sent from xnintr_shirq_handler().

 Yes, should. And this should is best be handled by
 
 a) Documenting the potential conflict in the same place when describing
 the return values
 
 b) Placing some debug warning in the nucleus' IRQ trampoline function to
 bail out (once per line) when running into such situation
 
 But I'm against any further runtime restrictions, especially as most
 drivers will never return anything else than NOT_HANDLED or HANDLED.
 Actually, this was the reason why I tried to separate the NO_ENABLE and
 PROPAGATE features as *additional* bits from HANDLED and
 NOT_HANDLED/UNHANDLED/NOINT. But I acknowledge that having all valid bit
 combination present as constants can be more helpful for the user. We
 just have to draw some line between the standard values and the
 additional gurus return codes 

(documentation: don't use NO_ENABLE or
 PROPAGATE unless you understand their side-effects and pitfalls precisely).

I agree with you on PROPAGATE case, but NO_ENABLE that, as pointed out above,
should (IMHO and at least, in theory) only mean keep the IRQ line disabled
(and have nothing to do with .end-ing the IRQ line) would be better supported.
But this is, again as was pointed out above, about some redesigning of the
current code = some overhead that likely affects non-shared aware code too.


So on one hand,

I'm ready to re-work code with :

HANDLED and UNHANDLED (or NOINT)

+ 2 additional bits : NOENABLE and PROPAGATE.

and document it like you suggested don't use NO_ENABLE or
PROPAGATE with shared interrupts 
unless you understand their side-effects and pitfalls precisely;

on the other hand,

I'd say that I'm almost ready to vote against merging the irq sharing code at all as it looks to be a rather partial solution.
-- Best regards,Dmitry Adamushko

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)


Dmitry Adamushko wrote:


  Good point, leaves us with 2 possible return values for shared handlers:
 
HANDLED
NOT_HANDLED
 
  i.e. shared handlers should never defer the end'ing of the interrupt 
(which

  makes sense, since this would affect the other [shared] handlers).

HANDLED_NOEBNABLE could be supported too. 
Yes, but it breaks decoupling between shared handlers; interrupts will be 
deferred for all [shared] handlers until it is properly ended.


There would be no need in 
reenventing

a wheel, just do it the way Linux does it.
But it's about some additional re-designing of the current codebase
(e.g. nested calling for irq_enable/disable())
I'm not sure we do need it for something else rather than irq sharing 
code but it affects the rest of the code.


And we have a kind of wrong concept :

XN_ISR_ENABLE (or NOENABLE) corresponds to xnarch_end_irq().

Agree



But the later one is not only about enabling the line, but
on some archs - about .end-ing it too (sending EOI).

And to support HANDLED_NOENABLE properly, those 2 have to be decoupled, i.e.
EOI should always be sent from xnintr_shirq_handler().
But the one returning HANDLED_NOENABLE is likely to leave the interrupt 
asserted, hence we can't EOI at this point (unless NO_ENABLE means DISABLE).




  Yes, should. And this should is best be handled by
 
  a) Documenting the potential conflict in the same place when describing
  the return values
 
  b) Placing some debug warning in the nucleus' IRQ trampoline function to
  bail out (once per line) when running into such situation
 
  But I'm against any further runtime restrictions, especially as most
  drivers will never return anything else than NOT_HANDLED or HANDLED.
  Actually, this was the reason why I tried to separate the NO_ENABLE and
  PROPAGATE features as *additional* bits from HANDLED and
  NOT_HANDLED/UNHANDLED/NOINT. But I acknowledge that having all valid bit
  combination present as constants can be more helpful for the user. We
  just have to draw some line between the standard values and the
  additional gurus return codes
 
(documentation: don't use NO_ENABLE or
  PROPAGATE unless you understand their side-effects and pitfalls 
precisely).


I agree with you on PROPAGATE case, but NO_ENABLE that, as pointed out 
above,

should (IMHO and at least, in theory) only mean keep the IRQ line disabled
(and have nothing to do with .end-ing the IRQ line) would be better 
supported.

But this is, again as was pointed out above, about some redesigning of the
current code = some overhead that likely affects non-shared aware code too.


So on one hand,

I'm ready to re-work code with :

HANDLED and UNHANDLED (or NOINT)

+ 2 additional bits : NOENABLE and PROPAGATE.

and document it like you suggested don't use NO_ENABLE or
PROPAGATE with shared interrupts
unless you understand their side-effects and pitfalls precisely;

on the other hand,

I'd say that I'm almost ready to vote against merging the irq sharing 
code at all as it looks to be a rather partial solution.
I vote for (even though I'm the one who complains the most), BUT I think it is 
important to keep the rules for using it simple (that's why I worry about the 
plethora of return-flags).



--

Regards

Anders

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

Anders Blomdell wrote:
 Dmitry Adamushko wrote:

   Good point, leaves us with 2 possible return values for shared
 handlers:
  
 HANDLED
 NOT_HANDLED
  
   i.e. shared handlers should never defer the end'ing of the
 interrupt (which
   makes sense, since this would affect the other [shared] handlers).

 HANDLED_NOEBNABLE could be supported too. 
 Yes, but it breaks decoupling between shared handlers; interrupts will
 be deferred for all [shared] handlers until it is properly ended.
 
 There would be no need in reenventing
 a wheel, just do it the way Linux does it.
 But it's about some additional re-designing of the current codebase
 (e.g. nested calling for irq_enable/disable())
 I'm not sure we do need it for something else rather than irq sharing
 code but it affects the rest of the code.

 And we have a kind of wrong concept :

 XN_ISR_ENABLE (or NOENABLE) corresponds to xnarch_end_irq().
 Agree
 

 But the later one is not only about enabling the line, but
 on some archs - about .end-ing it too (sending EOI).

 And to support HANDLED_NOENABLE properly, those 2 have to be
 decoupled, i.e.
 EOI should always be sent from xnintr_shirq_handler().
 But the one returning HANDLED_NOENABLE is likely to leave the interrupt
 asserted, hence we can't EOI at this point (unless NO_ENABLE means
 DISABLE).

I guess this is what Dmitry meant: explicitly call disable() if one or
more ISRs returned NOENABLE - at least on archs where end != enable.
Will this work? We could then keep on using the existing IRQ-enable API
from bottom-half IRQ tasks. But what about NOENABLE+PROPAGATE? Will this
special case still mean NOT to end the ISR (as Linux will do)?

Bah, we are running in circles, I'm afraid. I think it's better to call
NOENABLE NOEOI, which will indeed mean to not end this line (as it is
the current situation anyway, isn't it?), and leave the user with what
(s)he can do with such a feature. We found out that there are trillions
of ways to shoot oneself into the foot with NOENABLE and PROPAGATE, and
we cannot prevent most of them. So let's stop trying, at least for this
patch!

 

   Yes, should. And this should is best be handled by
  
   a) Documenting the potential conflict in the same place when
 describing
   the return values
  
   b) Placing some debug warning in the nucleus' IRQ trampoline
 function to
   bail out (once per line) when running into such situation
  
   But I'm against any further runtime restrictions, especially as most
   drivers will never return anything else than NOT_HANDLED or HANDLED.
   Actually, this was the reason why I tried to separate the NO_ENABLE
 and
   PROPAGATE features as *additional* bits from HANDLED and
   NOT_HANDLED/UNHANDLED/NOINT. But I acknowledge that having all
 valid bit
   combination present as constants can be more helpful for the user. We
   just have to draw some line between the standard values and the
   additional gurus return codes
  
 (documentation: don't use NO_ENABLE or
   PROPAGATE unless you understand their side-effects and pitfalls
 precisely).

 I agree with you on PROPAGATE case, but NO_ENABLE that, as pointed out
 above,
 should (IMHO and at least, in theory) only mean keep the IRQ line
 disabled
 (and have nothing to do with .end-ing the IRQ line) would be better
 supported.
 But this is, again as was pointed out above, about some redesigning of
 the
 current code = some overhead that likely affects non-shared aware
 code too.


 So on one hand,

 I'm ready to re-work code with :

 HANDLED and UNHANDLED (or NOINT)

 + 2 additional bits : NOENABLE and PROPAGATE.

 and document it like you suggested don't use NO_ENABLE or
 PROPAGATE with shared interrupts
 unless you understand their side-effects and pitfalls precisely;

 on the other hand,

 I'd say that I'm almost ready to vote against merging the irq sharing
 code at all as it looks to be a rather partial solution.
 I vote for (even though I'm the one who complains the most), BUT I think
 it is important to keep the rules for using it simple (that's why I
 worry about the plethora of return-flags).
 

And I'm with you here: My original proposal (2 base-states + 2 bits)
created 8 expressible states while your version only knows 4 states -
those which make sense most (and 2 of them are still the ones recommand
for the masses).

For RTDM I'm now almost determined to rework the API in way that only
HANDLED/UNHANDLED (or what ever their names will be) get exported, any
additional guru features will remain excluded as long as we have no
clean usage policy for them.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

2006-02-20 Thread Anders Blomdell


Jan Kiszka wrote:

Hi Dmitry,

Dmitry Adamushko wrote:


Hi Jan,

let's make yet another revision of the bits :

new XN_ISR_HANDLED  == old XN_ISR_HANDLED + old XN_ISR_NO_ENABLE

ok.

new XN_ISR_NOENABLE == ~ old XN_ISR_ENABLE

ok.

new XN_ISR_PROPAGATE == XN_ISR_CHAINED

ok.




Just to make sure that you understand my weird ideas: each of the three
new XN_ISR_xxx above should be encoded with an individual bit



new XN_ISR_NOINT == ?

does it suppose the interrupt line to be .end-ed (enabled) and irq not to be
propagated? Should be so, I guess, if it's different from 5). Then nucleus
ignores implicit IRQ enable for 5) as well as for 3).

Do we really need that NOINT then, as it seems to be the same as ~HANDLED?

or NOINT == 0 and then it's a scalar value, not a bit.

So one may consider HANDLED == 1 and NOINT == 0 as really scalar values

and

NOENABLE and PROPAGATE as additional bits (used only if needed).




My idea is to urge the user specifying one of the base return types
(HANDLED or NOINT) + any of the two additional bits (NOENABLE and
PROPAGATE).

For correct drivers NOINT could be 0 indeed, but to check that the user
picked a new constant we may want to set NOINT != 0. With the old API
return 0 expressed HANDLED + ~ENABLE for the old API. With the new one
the user signals no interest and the nucleus may raise a warning that a
spurious IRQ occurred. So I would add a debug bit for NOINT here to
optionally (on OPT_XENO_DEBUG) detect old-style usage (return 0).
Moreover, we gain freedom to move bits in the future when every state is
encoded via constants. Or am I too paranoid here?
After reading the above discussion (of which I understand very little), and 
looking at (what I believe to be) the relevant code:


+intr = shirq-handlers;
+
+while (intr)
+{
+s |= intr-isr(intr);
+++intr-hits;
+intr = intr-next;
+}
+xnintr_shirq_unlock(shirq);
+
+--sched-inesting;
+
+if (s  XN_ISR_ENABLE)
+   xnarch_end_irq(irq);
+else if (s  XN_ISR_CHAINED)
+   xnarch_chain_irq(irq);

A number of questions arise:

1. What happens if one of the shared handlers leaves the interrupt asserted, 
returns NOENABLE|HANDLED and another return only HANDLED?


2. What happens if one returns PROPAGATE and another returns HANDLED?

As far as I can tell, after all RT handlers havve run, the following scenarios 
are possible:


1. The interrupt is deasserted (i.e. it was a RT interrupt)
2. The interrupt is still asserted, it will be deasserted later
   by some RT task (i.e. it was a RT interrupt)
3. The interrupt is still asserted and will be deasserted
   by the Linux IRQ handler.

IMHO that leads to the conclusion that the IRQ handlers should return a scalar

#define UNHANDLED 0
#define HANDLED_ENABLE 1
#define HANDLED_NOENABLE 2
#define PROPAGATE 3

and the loop should be

s = UNHANDLED
while (intr) {
  tmp = intr-isr(intr);
  if (tmp  s) { s = tmp; }
  intr = intr-next;
}
if (s == PROPAGATE) {
  xnarch_chain_irq(irq);
} else if (s == HANDLED_ENABLE) {
  xnarch_end_irq(irq);
}

To be really honest, I think that PROPAGATE should be excluded from the RT 
IRQ-handlers, since with the current scheme all RT-handlers has to test if the 
IRQ was a Linux interrupt (otherwise the system will only work when the handler 
that returns PROPAGATE is installed)


--

Anders

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

2006-02-20 Thread Dmitry Adamushko


N.B. Amongst other things, some thoughts about CHAINED with shared interrupts.

On 20/02/06, Anders Blomdell [EMAIL PROTECTED] wrote:


A number of questions arise:1. What happens if one of the shared handlers leaves the interrupt asserted,
returns NOENABLE|HANDLED and another return only HANDLED?2. What happens if one returns PROPAGATE and another returns HANDLED?
Yep, each ISR may return a different value and all of them are
accumulated in the s variable ( s |= intr-isr(intr); ).

So the loop may end up with s which contains all of the possible bits:

(e.g.

isr1 - HANDLED | ENABLE
isr2 - HANDLED (don't want the irq to be enabled)
isr3 - CHAINED

)

s = HANDLED | ENABLE | CHAINED;

Then CHAINED will be ignored because of the following code :

+ if (s  XN_ISR_ENABLE)
+ xnarch_end_irq(irq);
+ else if (s  XN_ISR_CHAINED) (*)
+ xnarch_chain_irq(irq);

the current code in the CVS doen not contain else in (*), so that
ENABLE | CHAINED is possible, though it's a wrong combination.

This said, we suppose that one knows what he is doing.

In the case of a single ISR per line, it's not that difficult to
achieve. But if there are a few ISRs, then one should analize and take
into account all possible return values of all the ISRs, as each of
them may affect others (e.g. if one returns CHAINED when another -
HANDLED | ENABLE).

So my feeling is that CHAINED should not be used by drivers which registered their ISRs as SHARED.

Moreover, I actually see the only scenario of CHAINED (I provided it before) :

all ISRs in the primary domain have reported UNHANDLED
= nucleus propagates the interrupt down the pipeline with
xnacrh_chain_irq().
This call actually returns 1 upon successful propagation (some domain
down the pipeline was interested in this irq) and 0 otherwise.

Upon 0, this is a spurious irq (none of domains was interested in its handling).

ok, let's suppose now :

we have 2 ISRs on the same shared line :

isr1 : HANDLED (will be enabled by rt task. Note, rt task must call xnarch_end_irq() and not just xnarch_enable_irq()! )

isr2 : CHAINED 

So HANDLED | CHAINED is ok for the single ISR on the line, but it may
lead to HANDLED | CHAINED | ENABLE in a case of the shared line.

rt task that works jointly with isr1 just calls xnarch_end_irq() at
some moment of time and some ISR in the linux domain does the same
later = the line is .end-ed 2 times.

ISR should never return CHAINED as to indicate _only_ that it is not
interested in this irq, but ~HANDLED or NOINT (if we'll support it)
instead.

If the ISR nevertheless wants to propagate the IRQ to the Linux domain
_explicitly_, it _must not_ register itself as SHARED, i.e. it _must_
be the only ISR on this line, otherwise that may lead to the IRQ line
being .end-ed twice (lost interrupts in some cases).

#define UNHANDLED 0#define HANDLED_ENABLE 1#define HANDLED_NOENABLE 2#define PROPAGATE 3

Yep, I'd agree with you. Moreover, PROPAGATE should not be used for shared interrupts.
-- Best regards,Dmitry Adamushko

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

2006-02-18 Thread Dmitry Adamushko


Hi Jan,

let's make yet another revision of the bits :

new XN_ISR_HANDLED == old XN_ISR_HANDLED + old XN_ISR_NO_ENABLE

ok.

new XN_ISR_NOENABLE == ~ old XN_ISR_ENABLE

ok.

new XN_ISR_PROPAGATE == XN_ISR_CHAINED

ok.

new XN_ISR_NOINT == ?

does it suppose the interrupt line to be .end-ed (enabled) and irq not
to be propagated? Should be so, I guess, if it's different from 5).
Then nucleus ignores implicit IRQ enable for 5) as well as for 3).

Do we really need that NOINT then, as it seems to be the same as ~HANDLED?

or NOINT == 0 and then it's a scalar value, not a bit.

So one may consider HANDLED == 1 and NOINT == 0 as really scalar values 

and 

NOENABLE and PROPAGATE as additional bits (used only if needed).


-- Best regards,Dmitry Adamushko

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

2006-02-18 Thread Jan Kiszka

Hi Dmitry,

Dmitry Adamushko wrote:
 Hi Jan,
 
 let's make yet another revision of the bits :
 
 new XN_ISR_HANDLED  == old XN_ISR_HANDLED + old XN_ISR_NO_ENABLE
 
 ok.
 
 new XN_ISR_NOENABLE == ~ old XN_ISR_ENABLE
 
 ok.
 
 new XN_ISR_PROPAGATE == XN_ISR_CHAINED
 
 ok.
 

Just to make sure that you understand my weird ideas: each of the three
new XN_ISR_xxx above should be encoded with an individual bit

 new XN_ISR_NOINT == ?
 
 does it suppose the interrupt line to be .end-ed (enabled) and irq not to be
 propagated? Should be so, I guess, if it's different from 5). Then nucleus
 ignores implicit IRQ enable for 5) as well as for 3).
 
 Do we really need that NOINT then, as it seems to be the same as ~HANDLED?
 
 or NOINT == 0 and then it's a scalar value, not a bit.
 
 So one may consider HANDLED == 1 and NOINT == 0 as really scalar values
 
 and
 
 NOENABLE and PROPAGATE as additional bits (used only if needed).
 

My idea is to urge the user specifying one of the base return types
(HANDLED or NOINT) + any of the two additional bits (NOENABLE and
PROPAGATE).

For correct drivers NOINT could be 0 indeed, but to check that the user
picked a new constant we may want to set NOINT != 0. With the old API
return 0 expressed HANDLED + ~ENABLE for the old API. With the new one
the user signals no interest and the nucleus may raise a warning that a
spurious IRQ occurred. So I would add a debug bit for NOINT here to
optionally (on OPT_XENO_DEBUG) detect old-style usage (return 0).
Moreover, we gain freedom to move bits in the future when every state is
encoded via constants. Or am I too paranoid here?

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

2006-02-16 Thread Dmitry Adamushko

On 16/02/06, Jan Kiszka [EMAIL PROTECTED] wrote:
Dmitry Adamushko wrote: On 16/02/06, Jan Kiszka [EMAIL PROTECTED] wrote: Dmitry Adamushko wrote: On 16/02/06, Jan Kiszka 
[EMAIL PROTECTED] wrote: Hmm, I still find XN_ISR_NOINT in the patch. Shouldn't we solve this before merging (i.e. make XN_ISR_HANDLED non-zero)?
 Ok, XN_ISR_NOINT is removed in favor of XN_ISR_HANDLED which is now non-zero. Actually, at first I wanted to make it just the other way about. Hmm, thinking about this again, I would like to revert my suggestion in
 favour of a third approach (trying to keep you busy ;) ). Ok, you are wellcome :) I didn't get it, at least while looking briefly. To make it a bit easier, at least for me, let's go another way.
 At the left is the list of allowed values as Philippe pointed out. At the right is another list which corresponds to the left one but when NOINT is used instead of HANDLED. Moreover, I have added another case - pure
 NOINT. The ISR says it's not mine and, well, it doesn't know whether it should be propagated or no (ok, so far CHAINED standed for NOINT).
1.)
HANDLED
-0 2.) HANDLED | ENABLE -ENABLE 3.) HANDLED | CHAINED-CHAINED
4.)
CHAINED-CHAINED
| NOINT
5.)-
NOINT Could you provide the 3-d corresponding table using your new flags?Still not 3D, but hopefully clarifying: :)
1.) XN_ISR_HANDLED |XN_ISR_NO_ENABLE
2.) XN_ISR_HANDLED3.) XN_ISR_HANDLED | XN_ISR_PROPAGATE(nucleus ignores implicit IRQ enable)4.) XN_ISR_NOINT | XN_ISR_PROPAGATE5.) XN_ISR_NOINT
Then why do we need XN_ISR_NOINT? is it not just the same as ~HANDLED?

Moreover, I tend to think now that PROPAGATE (CHAINED) should not be
normally used by ISRs at all. The only exception would be irq sharing
between RT and non-RT domains, but that's of no concern for most part
of rt drivers and, as last events revealed, it's very partially
supported at the moment and possible implementations are quite arguable.

So HANDLED means the irq line is .end-ed by xnintr_irq_handler() (xnarch_end_irq(irq) is called). 

If one wants to do it on his own later, NOENABLE can be returned additionally.
But then, one must use xnintr_end_irq() and not just xnintr_irq_enable().

If xnintr_end_irq() would be always the same as xnintr_irq_enable() or
we would decouple ending and enabling operations on the line (don't
have sources at hand); we could go Linux way and make use of a nested
counter in xnintr_t, so that xnintr_irq_disable() could be called in a
nested way.
This said, calling xnintr_irq_disable() in ISR leads to the interrupt
line being still disabled upon return from xnintr_irq_handler() and
re-enabled again by the user later.

~HANDLED means the ISR was not concerned about this interrupt. i.e. :

- there is ISR in another domain (down the pipeline) which is interested in this irq ;

 well, the designer of the system should be
aware of such cases and probably avoid that on hw layer or, at least,
to know what she is doing.

- this is a spurious irq.

So upon getting ~HANDLED, xnintr_irq_handler() does :

success = xnintr_irq_chained(irq);

if (success)
{
/* i.e. another domain is really interested and got this irq as pending*/

return without .end-ing
}
else
{
/* this is a spurious irq */

make use of some accounting of spurious interrupts (if we need it)),
i.e. keep the irq line disabled if the % of unhandled requests 
some limit.

otherwise

xnintr_end_irq() and return
}

too long :) ok, it's quite similar indeed to what you have suggested,
with the exception that

- no need for NOINT (if it's really == ~HANDLED);
- nucleus provides some additional logic while handling ~HANDLED case.
- user don't need to return ENABLE explicitly - in most cases, that's
what he actually needs but sometimes just forgets (I recall a few
questions on the mailing list about only one interrupt.


Jan-- Best regards,Dmitry Adamushko
___
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

[Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

Dmitry Adamushko wrote:
 Hello everybody,
 
 being inspired by successful results of tests conducted recently by Jan 
 team,
 I'm presenting the final (yep, yet another final :) combo-patch.
 
 The shirq support now is optional, so that
 
 CONFIG_XENO_OPT_SHIRQ_LEVEL -enables shirq for level-triggered
 interrupts;
 
 CONFIG_XENO_OPT_SHIRQ_EDGE  --//- for edge-triggered ones.
 
 I addressed all the remarks and now, IMHO, it's (hopefully) ready for merge.
 

Hmm, I still find XN_ISR_NOINT in the patch. Shouldn't we solve this
before merging (i.e. make XN_ISR_HANDLED non-zero)?

Moreover, I attached an add-on patch to overcome the name buffer in
xnintr_t. Note that this patch is only compile-tested, I have no native
interrupt test-case at hand.

Jan
diff -u include/nucleus/intr.h include/nucleus/intr.h
--- include/nucleus/intr.h	(Arbeitskopie)
+++ include/nucleus/intr.h	(Arbeitskopie)
@@ -54,7 +54,7 @@
 
 xniack_t iack;	/* ! Interrupt acknowledge routine. */
 
-char name[XNOBJECT_NAME_LEN]; /* ! Symbolic name. */
+const char *name; /* ! Symbolic name. */
 
 } xnintr_t;
 
diff -u ksrc/skins/native/syscall.c ksrc/skins/native/syscall.c
--- ksrc/skins/native/syscall.c	(Arbeitskopie)
+++ ksrc/skins/native/syscall.c	(Arbeitskopie)
@@ -2883,23 +2883,15 @@
 char name[XNOBJECT_NAME_LEN];
 RT_INTR_PLACEHOLDER ph;
 int err, mode;
-RT_INTR *intr;
+struct {
+	RT_INTR intr;
+	char name[XNOBJECT_NAME_LEN];
+} *intr_buf;
 unsigned irq;
 
 if (!__xn_access_ok(curr,VERIFY_WRITE,__xn_reg_arg1(regs),sizeof(ph)))
 	return -EFAULT;
 
-if (__xn_reg_arg2(regs))
-	{
-	if (!__xn_access_ok(curr,VERIFY_READ,__xn_reg_arg2(regs),sizeof(name)))
-	return -EFAULT;
-
-	__xn_strncpy_from_user(curr,name,(const char __user *)__xn_reg_arg2(regs),sizeof(name) - 1);
-	name[sizeof(name) - 1] = '\0';
-	}
-else
-	*name = '\0';
-
 /* Interrupt line number. */
 irq = (unsigned)__xn_reg_arg3(regs);
 
@@ -2909,23 +2901,41 @@
 if (mode  ~(I_AUTOENA|I_PROPAGATE))
 	return -EINVAL;
 
-intr = (RT_INTR *)xnmalloc(sizeof(*intr));
+intr_buf = (typeof(intr_buf))xnmalloc(sizeof(*intr_buf));
 
-if (!intr)
+if (!intr_buf)
 	return -ENOMEM;
 
-err = rt_intr_create(intr,name,irq,rt_intr_handler,NULL,0);
+if (__xn_reg_arg2(regs))
+	{
+	if (!__xn_access_ok(curr,VERIFY_READ,__xn_reg_arg2(regs),sizeof(name)))
+	{
+	xnfree(intr_buf);
+	return -EFAULT;
+	}
+
+	__xn_strncpy_from_user(curr,
+	   intr_buf-name,
+	   (const char __user *)__xn_reg_arg2(regs),
+	   sizeof(intr_buf-name) - 1);
+	intr_buf-name[sizeof(intr_buf-name) - 1] = '\0';
+	}
+else
+	intr_buf-name[0] = '\0';
+
+
+err = rt_intr_create(intr_buf-intr,intr_buf-name,irq,rt_intr_handler,NULL,0);
 
 if (err == 0)
 	{
-	intr-mode = mode;
-	intr-cpid = curr-pid;
+	intr_buf-intr.mode = mode;
+	intr_buf-intr.cpid = curr-pid;
 	/* Copy back the registry handle to the ph struct. */
-	ph.opaque = intr-handle;
+	ph.opaque = intr_buf-intr.handle;
 	__xn_copy_to_user(curr,(void __user *)__xn_reg_arg1(regs),ph,sizeof(ph));
 	}
 else
-	xnfree(intr);
+	xnfree(intr_buf);
 
 return err;
 }
diff -u ksrc/nucleus/intr.c ksrc/nucleus/intr.c
--- ksrc/nucleus/intr.c	(Arbeitskopie)
+++ ksrc/nucleus/intr.c	(Arbeitskopie)
@@ -148,7 +148,7 @@
 intr-cookie = NULL;
 intr-hits = 0;
 intr-flags = flags;
-xnobject_copy_name(intr-name,name);
+intr-name = name;
 #if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
 intr-next = NULL;
 #endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */


signature.asc
Description: OpenPGP digital signature

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

Dmitry Adamushko wrote:
 On 16/02/06, Jan Kiszka [EMAIL PROTECTED] wrote:
 
 Hmm, I still find XN_ISR_NOINT in the patch. Shouldn't we solve this
 before merging (i.e. make XN_ISR_HANDLED non-zero)?
 
 
 Ok, XN_ISR_NOINT is removed in favor of XN_ISR_HANDLED which is now
 non-zero.
 Actually, at first I wanted to make it just the other way about.
 

Hmm, thinking about this again, I would like to revert my suggestion in
favour of a third approach (trying to keep you busy ;) ).

Recommended default values:
newXN_ISR_HANDLED = old non-zeroXN_ISR_HANDLED | oldXN_ISR_ENABLE

newXN_ISR_NOINT = newXN_ISR_NO_ENABLE


Additional bits:
newXN_ISR_NO_ENABLE
 = indicate that oldXN_ISR_ENABLE is not set
(use case: handle but delay re-enable)

newXN_ISR_PROPAGATE
 = oldXN_ISR_CHAINED (the new name is closer to the related function
 calls - at least for me).


Not expressible combination:
oldXN_ISR_ENABLE | newXN_ISR_NOINT
 = Doesn't make sense, does it?


Still expressible invalid combination:
newXN_ISR_HANDLED | newXN_ISR_PROPAGATE
 = The implicit enable request should be easy to ignore at nucleus
level.


Rational:

Most users will either indicate that an IRQ was handled and should be
re-enabled as soon as possible or that it was unhandled and should
better remain masked if no one else cares.

XN_ISR_PROPAGATE (or related skin defines) are only useful for very very
special corner-case drivers and applications. Normal generic drivers
should not touch this (including my own xeno_16550A... bah!). BTW, if
there is some unintentional IRQ sharing so that spurious RT interrupts
occur, I think we should detect this at nucleus level and bail out.
Otherwise, systems quickly lock up (I just faced this once again two
days ago on an old RTAI box).


This will break some stuff (RTDM e.g., but we have some version flags
for such cases), but it takes us closer to the standard Linux API
without loosing control over the details when required.

Comments?

 
 Moreover, I attached an add-on patch to overcome the name buffer in
 xnintr_t. Note that this patch is only compile-tested, I have no native
 interrupt test-case at hand.
 
 
 
 Heh.. someone once suggested the -p key with diff to me? :o)

Mmpf. Must have been too late, and I was too impressed how interdiff
worked out this patch. :)

 
 I bet your patch would even work but it's, well, slightly broken indeed ;-)
 
 Look at the native/syscall.c : __rt_intr_delete() :
 
 RT_INTR *intr = (RT_INTR *)rt_registry_fetch(ph.opaque);
 ...
 xnfree(intr);
 
 intr actually points to intr_buf-intr and it works as expected only
 because RT_INTR intr is the first member of the struct, so that it's
 address == the address of the whole object of this struct.

But this is not an unusual trick, and I see no practical problems.

 
 To do it properly you would need to make your unnamed struct visible
 for both __rt_intr_create() and __rt_intr_delete(). Something like :
 
 struct RT_INTR_ENV {
 
 #define link2intr_env(laddr) \
 ((RT_INTR_ENV *)(((char* )laddr) - (int)(((RT_INTR_ENV *)0)-intr)))
 
 RT_INTR intr;
 char name[XNOBJECT_NAME_LEN];
 };
 
 and then make use of link2intr_env() in __rt_intr_delet().
 
 
 Ok, with you approach we would avoid allocating memory
 for name[XNOBJECT_NAME_LEN] when an object is created in kernel-mode
 and do it via RT_INTR_ENV only when it's created in user-mode.
 
 We could even use that approach for all objects in native skin, but we can't
 because of anonymous objects supported in kernel-mode too and, well, I
 personally would not like those RT_OBJECT_ENV structures all over the map in
 syscall.c :)

Me too.

 
 So I don't want to break the whole picture only for the RT_INTR object (it
 doesn't support anonymous objects now).
 
 Nevertheless, if you still want to save a hundred bytes of data or so (how
 many interrupt objects would a normal system have? and I guess the lenght of
 name is about 10 symbols on average :)

RT_INTR is now disabled by default in the kernel config. Thus most users
should not see its code in their kernels, only the additional data and
code related to xnintr_t.name.

 
 for the users that don't use native skin, then I would suggest the following
 way :
 
 xnintr_t contains just the link char *name as you suggested but RT_INTR
 contains the name[XNOBJECT_NAME_LEN] member (as it was before actually).
 
 If it's ok,  then I'll send um... yet another final patch that addresses
 both issues :)

I'm fine with this as well if you prefer it, no problem.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

2006-02-16 Thread Dmitry Adamushko

On 16/02/06, Jan Kiszka [EMAIL PROTECTED] wrote:
Dmitry Adamushko wrote: On 16/02/06, Jan Kiszka [EMAIL PROTECTED] wrote: Hmm, I still find XN_ISR_NOINT in the patch. Shouldn't we solve this before merging (
i.e. make XN_ISR_HANDLED non-zero)? Ok, XN_ISR_NOINT is removed in favor of XN_ISR_HANDLED which is now non-zero. Actually, at first I wanted to make it just the other way about.
Hmm, thinking about this again, I would like to revert my suggestion infavour of a third approach (trying to keep you busy ;) ).
Ok, you are wellcome :)

I didn't get it, at least while looking briefly. To make it a bit easier, at least for me, let's go another way.

At the left is the list of allowed values as Philippe pointed out.
At the right is another list which corresponds to the left one but when
NOINT is used instead of HANDLED. Moreover, I have added another case -
pure NOINT. The ISR says it's not mine and, well, it doesn't know
whether it should be propagated or no
(ok, so far CHAINED standed for NOINT).

HANDLED
- 0

HANDLED | ENABLE - ENABLE

HANDLED | CHAINED - CHAINED
CHAINED
- CHAINED | NOINT

- NOINT

Could you provide the 3-d corresponding table using your new flags?

xnintr_t contains just the link char *name as you suggested but RT_INTR
contains the name[XNOBJECT_NAME_LEN] member (as it was before actually). If it's ok,then I'll send um... yet another final patch that addresses both issues :)I'm fine with this as well if you prefer it, no problem.

Yep, let's go this way.

Jan
-- Best regards,Dmitry Adamushko

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

Dmitry Adamushko wrote:
 On 16/02/06, Jan Kiszka [EMAIL PROTECTED] wrote:
 Dmitry Adamushko wrote:
 On 16/02/06, Jan Kiszka [EMAIL PROTECTED] wrote:

 Hmm, I still find XN_ISR_NOINT in the patch. Shouldn't we solve this
 before merging (i.e. make XN_ISR_HANDLED non-zero)?

 Ok, XN_ISR_NOINT is removed in favor of XN_ISR_HANDLED which is now
 non-zero.
 Actually, at first I wanted to make it just the other way about.

 Hmm, thinking about this again, I would like to revert my suggestion in
 favour of a third approach (trying to keep you busy ;) ).
 
 
 Ok, you are wellcome :)
 
 I didn't get it, at least while looking briefly. To make it a bit easier, at
 least for me, let's go another way.
 
 At the left is the list of allowed values as Philippe pointed out.
 At the right is another list which corresponds to the left one but when
 NOINT is used instead of HANDLED. Moreover, I have added another case - pure
 NOINT. The ISR says it's not mine and, well, it doesn't know whether it
 should be propagated or no
 (ok, so far CHAINED standed for NOINT).
 
 1.) HANDLED -  0
 2.) HANDLED | ENABLE -  ENABLE
 3.) HANDLED | CHAINED-  CHAINED
 4.) CHAINED  -  CHAINED | NOINT
 5.)-   NOINT
 
 Could you provide the 3-d corresponding table using your new flags?
 

Still not 3D, but hopefully clarifying: :)

1.) XN_ISR_HANDLED |XN_ISR_NO_ENABLE
2.) XN_ISR_HANDLED
3.) XN_ISR_HANDLED | XN_ISR_PROPAGATE
(nucleus ignores implicit IRQ enable)
4.) XN_ISR_NOINT | XN_ISR_PROPAGATE
5.) XN_ISR_NOINT

2.) and 5.) are most common, the others for special scenarios.
Especially, as long as we have no API for ending IRQs outside the
handler, 1.) is of limited usage I think.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)

2006-02-16 Thread Dmitry Adamushko

On 16/02/06, Jan Kiszka [EMAIL PROTECTED] wrote:
Dmitry Adamushko wrote: On 16/02/06, Jan Kiszka [EMAIL PROTECTED] wrote: Dmitry Adamushko wrote: On 16/02/06, Jan Kiszka 
[EMAIL PROTECTED] wrote: Hmm, I still find XN_ISR_NOINT in the patch. Shouldn't we solve this before merging (i.e. make XN_ISR_HANDLED non-zero)?
 Ok, XN_ISR_NOINT is removed in favor of XN_ISR_HANDLED which is now non-zero. Actually, at first I wanted to make it just the other way about. Hmm, thinking about this again, I would like to revert my suggestion in
 favour of a third approach (trying to keep you busy ;) ). Ok, you are wellcome :) I didn't get it, at least while looking briefly. To make it a bit easier, at least for me, let's go another way.
 At the left is the list of allowed values as Philippe pointed out. At the right is another list which corresponds to the left one but when NOINT is used instead of HANDLED. Moreover, I have added another case - pure
 NOINT. The ISR says it's not mine and, well, it doesn't know whether it should be propagated or no (ok, so far CHAINED standed for NOINT).
1.)
HANDLED
-0 2.) HANDLED | ENABLE -ENABLE 3.) HANDLED | CHAINED-CHAINED
4.)
CHAINED-CHAINED
| NOINT
5.)-
NOINT Could you provide the 3-d corresponding table using your new flags?Still not 3D, but hopefully clarifying: :)
1.) XN_ISR_HANDLED |XN_ISR_NO_ENABLE
2.) XN_ISR_HANDLED3.) XN_ISR_HANDLED | XN_ISR_PROPAGATE(nucleus ignores implicit IRQ enable)4.) XN_ISR_NOINT | XN_ISR_PROPAGATE5.) XN_ISR_NOINT
Then why do we need XN_ISR_NOINT? is it not just the same as ~HANDLED?

Moreover, I tend to think now that PROPAGATE (CHAINED) should not be
normally used by ISRs at all. The only exception would be irq sharing
between RT and non-RT domains, but that's of no concern for most part
of rt drivers and, as last events revealed, it's very partially
supported at the moment and possible implementations are quite arguable.

So HANDLED means the irq line is .end-ed by xnintr_irq_handler() (xnarch_end_irq(irq) is called). 

If one wants to do it on his own later, NOENABLE can be returned additionally.
But then, one must use xnintr_end_irq() and not just xnintr_irq_enable().

If xnintr_end_irq() would be always the same as xnintr_irq_enable() or
we would decouple ending and enabling operations on the line (don't
have sources at hand); we could go Linux way and make use of a nested
counter in xnintr_t, so that xnintr_irq_disable() could be called in a
nested way.
This said, calling xnintr_irq_disable() in ISR leads to the interrupt
line being still disabled upon return from xnintr_irq_handler() and
re-enabled again by the user later.

~HANDLED means the ISR was not concerned about this interrupt. i.e. :

- there is ISR in another domain (down the pipeline) which is interested in this irq ;

 well, the designer of the system should be
aware of such cases and probably avoid that on hw layer or, at least,
to know what she is doing.

- this is a spurious irq.

So upon getting ~HANDLED, xnintr_irq_handler() does :

success = xnintr_irq_chained(irq);

if (success)
{
/* i.e. another domain is really interested and got this irq as pending*/

return without .end-ing
}
else
{
/* this is a spurious irq */

make use of some accounting of spurious interrupts (if we need it)),
i.e. keep the irq line disabled if the % of unhandled requests 
some limit.

otherwise

xnintr_end_irq() and return
}

too long :) ok, it's quite similar indeed to what you have suggested,
with the exception that

- no need for NOINT (if it's really == ~HANDLED);
- nucleus provides some additional logic while handling ~HANDLED case.
- user don't need to return ENABLE explicitly - in most cases, that's
what he actually needs but sometimes just forgets (I recall a few
questions on the mailing list about only one interrupt.


Jan-- Best regards,Dmitry Adamushko

Re: [Xenomai-core] Re: [PATCH] Shared interrupts (ready to merge)