Re: [Xenomai-core] [PATCH-STACK] Missing bits for -rc1

[Jan Kiszka <jan.kiszka@domain.hid>]

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

Jan Kiszka wrote:
> Dmitry Adamushko wrote:
>> On 02/07/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>> Dmitry Adamushko wrote:
>>>> On 01/07/07, Jan Kiszka <jan.kiszka@domain.hid> 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


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