Re: [Xenomai-core] arm: Unprotected access to irq_desc field?

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

Am 29.10.2010 14:58, Philippe Gerum wrote:
> On Fri, 2010-10-29 at 14:46 +0200, Philippe Gerum wrote:
>> On Fri, 2010-10-29 at 14:09 +0200, Jan Kiszka wrote:
>>> Am 29.10.2010 14:00, Philippe Gerum wrote:
>>>> On Fri, 2010-10-29 at 11:05 +0200, Jan Kiszka wrote:
>>>>> Am 29.10.2010 10:27, Philippe Gerum wrote:
>>>>>> On Fri, 2010-10-29 at 09:00 +0200, Jan Kiszka wrote:
>>>>>>> Am 28.10.2010 21:34, Philippe Gerum wrote:
>>>>>>>> On Thu, 2010-10-28 at 21:15 +0200, Gilles Chanteperdrix wrote:
>>>>>>>>> Jan Kiszka wrote:
>>>>>>>>>> Gilles,
>>>>>>>>>>
>>>>>>>>>> I happened to come across rthal_mark_irq_disabled/enabled on arm. On
>>>>>>>>>> first glance, it looks like these helpers manipulate irq_desc::status
>>>>>>>>>> non-atomically, i.e. without holding irq_desc::lock. Isn't this fragile?
>>>>>>>>>
>>>>>>>>> I have no idea. How do the other architectures do? As far as I know,
>>>>>>>>> this code has been copied from there.
>>>>>>>>
>>>>>>>> Other archs do the same, simply because once an irq is managed by the
>>>>>>>> hal, it may not be shared in any way with the regular kernel. So locking
>>>>>>>> is pointless.
>>>>>>>
>>>>>>> Indeed, I missed that all the other archs have this uninlined in hal.c.
>>>>>>>
>>>>>>> However, this leaves at least a race between xnintr_disable/enable and
>>>>>>> XN_ISR_PROPAGATE (ie. the related Linux path) behind.
>>>>>>
>>>>>> I can't see why XN_ISR_PROPAGATE would be involved here. This service
>>>>>> pends an interrupt in the pipeline log.
>>>>>
>>>>> And this finally lets Linux code run that fiddles with irq_desc::status
>>>>> as well - potentially in parallel to an unsyncrhonized
>>>>> xnintr_irq_disable in a different context. That's the problem.
>>>>
>>>> Propagation happens in primary domain. When is this supposed to conflict
>>>> on the same CPU with linux?
>>>
>>> The propagation triggers the delivery of this IRQ to the Linux domain,
>>> thus at some point there will be Linux accessing the descriptor while
>>> there might be xnintr_irq_enable/disable running on some other CPU (or
>>> it was preempted at the wrong point on the very same CPU).
>>
>> The point is that XN_ISR_PROPAGATE, as a mean to force sharing of an IRQ
>> between both domains is plain wrong. Remove this, and no conflict
>> remains; this is what needs to be addressed. The potential issue between
>> xnintr_enable/disable and the hal routines does not exist, if those
>> callers handle locking properly.
>>
> 
> In any case, I don't think we could accept that sharing, so flipping the
> bits in the hal is in fact pointless. To match the linux locking, we
> should iron the irq_desc::lock, which we won't since this would cause
> massive jittery. We should stick to the basic logic: no sharing,
> therefore no tracking need for the irqflags. I'll kill XN_ISR_PROPAGATE
> in forge at some point, for sure.
> 

Sounds good.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux