Jan Kiszka wrote: > Dmitry Adamushko wrote: >> On 02/07/07, Jan Kiszka <[EMAIL PROTECTED]> wrote: >>> Dmitry Adamushko wrote: >>>> On 01/07/07, Jan Kiszka <[EMAIL PROTECTED]> wrote: >>>>> Hi all, >>>>> >>>>> I've just uploaded my rebased patch stack, including specifically the >>>>> last bits that should go in before -rc1, >> a few comments: >> >> (1) in xnintr_irq_handler() >> >> + xnlock_get(&xnirqs[irq].lock); >> + >> +#ifdef CONFIG_SMP >> + /* In SMP case, we have to reload the cookie under the per-IRQ lock >> + to avoid racing with xnintr_detach. */ >> + intr = rthal_irq_cookie(&rthal_domain, irq); >> + if (unlikely(!intr)) { >> + s = 0; >> + goto unlock_and_exit; >> + } >> +#else >> + intr = cookie; >> +#endif >> >> I think, it would be better to check 'intr' for NULL at this point so >> to cover 'cookie == NULL' (who knows.. and it's better not to rely on >> the lower layer here), I guess. > > The lower layer just passes what we hand over - and when we do so. Keep > in mind: this is the non-SMP case, thus we can rely on the atomicity of > this handler like the one of registering a handler + its cookie (like we > always did -- unfortunately also for SMP...). For me, checking for NULL > on UP is redundant code in a hotpath. Should I spread some comment > regarding this assumption? > >> (2) use cases of xnintr_sync_stat_references() in xnintr_irq_detach() >> >> + >> + xnlock_get(&xnirqs[irq].lock); >> + >> + err = xnarch_release_irq(intr->irq); >> + xnintr_sync_stat_references(intr); >> + xnlock_put(&xnirqs[irq].lock); >> >> Why do you call xnintr_sync_stat_references() inside the locked section >> here? >> Does xnintr_sync_stat_references() really need to be protected by >> 'intr->lock' (it's been already detached) ? > > That's now my own overcautiousness :). I think we can move it out. > Someone must sync on some xnintr object while someone else tries to > re-register an object with very same address -- classical user error, > nothing we need to catch here.
Modified version just went online, see http://www.rts.uni-hannover.de/rtaddon/patches/xenomai/fix-intr-locking-part-ii-v3.patch whitespace-damaged interdiff: --- xenomai/ksrc/nucleus/intr.c +++ xenomai/ksrc/nucleus/intr.c @@ -124,6 +124,7 @@ goto unlock_and_exit; } #else + /* cookie always valid, attach/detach happens with IRQs disabled */ intr = cookie; #endif s = intr->isr(intr); @@ -438,9 +439,10 @@ /* Remove the given interrupt object from the list. */ xnlock_get(&shirq->lock); *p = e->next; - xnintr_sync_stat_references(intr); xnlock_put(&shirq->lock); + xnintr_sync_stat_references(intr); + /* Release the IRQ line if this was the last user */ if (shirq->handlers == NULL) err = xnarch_release_irq(intr->irq); @@ -468,12 +470,11 @@ int err; xnlock_get(&xnirqs[irq].lock); - err = xnarch_release_irq(intr->irq); - xnintr_sync_stat_references(intr); - xnlock_put(&xnirqs[irq].lock); + xnintr_sync_stat_references(intr); + return err; } Jan
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Xenomai-core mailing list Xenomai-core@gna.org https://mail.gna.org/listinfo/xenomai-core