From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <4A26909F.4020504@domain.hid> Date: Wed, 03 Jun 2009 17:02:55 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <20090603104015.13268.34073.stgit@domain.hid> <20090603104015.13268.59718.stgit@domain.hid> <4A267036.8010507@domain.hid> <4A267591.2060301@domain.hid> <4A267669.9030304@domain.hid> <4A267BBE.8010407@domain.hid> <4A268B8D.7060404@domain.hid> In-Reply-To: <4A268B8D.7060404@domain.hid> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Xenomai-core] [PATCH 2/3] RTDM: Instrument rtdm_lock_get for proper use List-Id: Xenomai life and development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gilles Chanteperdrix Cc: xenomai@xenomai.org Gilles Chanteperdrix wrote: > Jan Kiszka wrote: >> Gilles Chanteperdrix wrote: >>> Jan Kiszka wrote: >>>> Gilles Chanteperdrix wrote: >>>>> Jan Kiszka wrote: >>>>>> In case the user thinks rtdm_lock_get could be used like spin_lock or >>>>>> messes up the IRQ protection for other reasons, catch this with a >>>>>> XENO_BUGON. >>>>>> >>>>>> Signed-off-by: Jan Kiszka >>>>>> --- >>>>>> >>>>>> include/rtdm/rtdm_driver.h | 8 ++++++++ >>>>>> 1 files changed, 8 insertions(+), 0 deletions(-) >>>>>> >>>>>> diff --git a/include/rtdm/rtdm_driver.h b/include/rtdm/rtdm_driver.h >>>>>> index 058a9f8..fe42eea 100644 >>>>>> --- a/include/rtdm/rtdm_driver.h >>>>>> +++ b/include/rtdm/rtdm_driver.h >>>>>> @@ -655,7 +655,15 @@ typedef unsigned long rtdm_lockctx_t; >>>>>> * >>>>>> * Rescheduling: never. >>>>>> */ >>>>>> +#ifdef DOXYGEN_CPP /* Beautify doxygen output */ >>>>>> #define rtdm_lock_get(lock) rthal_spin_lock(lock) >>>>>> +#else /* This is how it really works */ >>>>>> +#define rtdm_lock_get(lock) \ >>>>>> + do { \ >>>>>> + XENO_BUGON(RTDM, rthal_local_irq_test()); \ >>>>>> + rthal_spin_lock(lock); \ >>>>>> + } while (0) >>>>>> +#endif >>>>> Why is it a problem to call rthal_spin_lock with irqs off? >>>> Did I messed it up again? I meant it is a problem to call it with irqs >>>> *on*. Checking what rthal_local_irq_test() actually means... Hmm, I >>>> still think it's correct. Maybe we should rename rthal_local_irq_test to >>>> rthal_local_irq_enabled to clarify the usage. >>> Ok. Got it. So, maybe, what you want is: >>> >>> if (rthal_local_irq_test()) >>> xnpod_lock_sched(); >>> rthal_spin_lock >>> >>> ? >> That would be a semantical enhancement of rtdm_spin_lock/unlock, but I'm >> not sure we want it. Because then you can run into bugs if the user >> forgets to pick the irqsave version for task context when there is also >> an IRQ context use of the same lock. >> >> So far the semantic of rtdm_lock was very simple: cross-CPU protection >> via spin lock, local preemption protection via IRQ lock. And that >> pattern could easily be validated with the instrumentation I posted. And >> so far no one asked for more. And finally: xnpod_lock/unlock_sched won't >> be be cheaper than irqsave/restore as it involves a full nklock >> acquisition - with irqsave/restore... > > On the other hand, I already had to port some plain Linux drivers to > RTDM, and from this perspective, having a one to one mapping of the > services is a real win. I also think that equivalents to > preempt_enable() and preempt_disable() are missing in the RTDM > interface. I found myself calling > xnpod_lock_sched()/xnpod_unlock_sched() in some RTDM drivers, which is > bad, I know... Yes, but as long as we have no comparably cheap preempt_enable/disable in Xenomai, I think it is counterproductive to motivate its (potentially heavy) use in drivers. Jan -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux