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:
<new>XN_ISR_HANDLED => <old non-zero>XN_ISR_HANDLED | <old>XN_ISR_ENABLE

<new>XN_ISR_NOINT => <new>XN_ISR_NO_ENABLE


Additional bits:
<new>XN_ISR_NO_ENABLE
 => indicate that <old>XN_ISR_ENABLE is not set
    (use case: handle but delay re-enable)

<new>XN_ISR_PROPAGATE
 => <old>XN_ISR_CHAINED (the new name is closer to the related function
                         calls - at least for me).


Not expressible combination:
<old>XN_ISR_ENABLE | <new>XN_ISR_NOINT
 => Doesn't make sense, does it?


Still expressible invalid combination:
<new>XN_ISR_HANDLED | <new>XN_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

Attachment: signature.asc
Description: OpenPGP digital signature

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

Reply via email to