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

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

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

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.

Thanks,
Jan


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