[PATCH] rtc: rtc-at91rm9200: use a variable for storing IMR

[Nicolas Ferre]

On 03/28/2013 07:20 PM, Douglas Gilbert :
> On 13-03-28 05:57 AM, Johan Hovold wrote:
>> On Tue, Mar 26, 2013 at 05:09:59PM -0400, Douglas Gilbert wrote:
>>> On 13-03-26 03:27 PM, Johan Hovold wrote:
>>>> On Fri, Mar 15, 2013 at 06:37:12PM +0100, Nicolas Ferre wrote:
>>>>> On some revisions of AT91 SoCs, the RTC IMR register is not working.
>>>>> Instead of elaborating a workaround for that specific SoC or IP
>>>>> version,
>>>>> we simply use a software variable to store the Interrupt Mask
>>>>> Register and
>>>>> modify it for each enabling/disabling of an interrupt. The overhead
>>>>> of this
>>>>> is negligible anyway.
>>>>
>>>> The patch does not add any memory barriers or register read-backs when
>>>> manipulating the interrupt-mask variable. This could possibly lead to
>>>> spurious interrupts both when enabling and disabling the various
>>>> RTC-interrupts due to write reordering and bus latencies.
>>>>
>>>> Has this been considered? And is this reason enough for a more targeted
>>>> work-around so that the SOCs with functional RTC_IMR are not affected?
>>>
>>> The SoCs in question use a single embedded ARM926EJ-S and
>>> according to the Atmel documentation, that CPU's instruction
>>> set contains no barrier (or related) instructions.
>>
>> The ARM926EJ-S actually does have a Drain Write Buffer instruction but
>> it's not used by the ARM barrier-implementation unless
>> CONFIG_ARM_DMA_MEM_BUFFERABLE or CONFIG_SMP is set.
> 
> The ARM926EJ-S is ARMv5 so CONFIG_ARM_DMA_MEM_BUFFERABLE is not
> available. SMP is not an option for arm/mach-at91.
> 
>> However, wmb() always implies a compiler barrier which is what is needed
>> in this case.
> 
> Even if wmb() did anything, would it make this case "safe"?
> 
>>> In the arch/arm/mach-at91 sub-tree of the kernel source
>>> I can find no use of the wmb() call. Also checked all drivers
>>> in the kernel containing "at91" and none called wmb().
>>
>> I/O-operations are normally not reordered, but this patch is faking a
>> hardware register and thus extra care needs to be taken.
>>
>> To repeat:
>>
>>> @@ -198,9 +203,12 @@ static int at91_rtc_alarm_irq_enable(struct
>>> device *dev, unsigned int enabled)
>>>
>>>        if (enabled) {
>>>                at91_rtc_write(AT91_RTC_SCCR, AT91_RTC_ALARM);
>>> +             at91_rtc_imr |= AT91_RTC_ALARM;
>>
>> Here a barrier is needed to prevent the compiler from reordering the two
>> writes (i.e., mask update and interrupt enable).
> 
> Isn't either order potentially unsafe? So even if the compiler
> did foolishly re-order them, the sequence is still unsafe when
> a SYS interrupt splits those two lines (since the SYS interrupt
> is shared, it can occur at any time).

Absolutely: I think it has to be protected by the proper
spin_lock_(irqsave)() functions, each time we:
- modify an interrupt + updated the shadow register
- read the shadow register

Note, on our current UP, it is the "irqsave" part that makes the
difference tough...

>>>                at91_rtc_write(AT91_RTC_IER, AT91_RTC_ALARM);
>>> -     } else
>>> +     } else {
>>>                at91_rtc_write(AT91_RTC_IDR, AT91_RTC_ALARM);
>>
>> Here a barrier is again needed to prevent the compiler from reordering,
>> but we also need a register read back (of some RTC-register) before
>> updating the mask. Without the register read back, there could be a
>> window where the mask does not match the hardware state due to bus
>> latencies.
>>
>> Note that even with a register read back there is a (theoretical)
>> possibility that the interrupts have not yet been disabled when the fake
>> mask is updated. The only way to know for sure is to poll RTC_IMR but
>> that is the very register you're trying to emulate.
>>
>>> +             at91_rtc_imr &= ~AT91_RTC_ALARM;
>>> +     }
>>>
>>>        return 0;
>>> }
>>
>> In the worst-case scenario ignoring the shared RTC-interrupt could lead
>> to the disabling of the system interrupt and thus also PIT, DBGU, ...
> 
> And how often does the AT91_RTC_ALARM alarm interrupt fire?
> 
>> I think this patch should be reverted and a fix for the broken SoCs be
>> implemented which does not penalise the other SoCs. That is, only
>> fall-back to faking IMR on the SoCs where it is actually broken.
> 
> Even though I sent a patch to fix this problem to Nicolas,
> what was presented is not my version. In mine I added DT
> support:
> 
> #ifdef CONFIG_OF
> static const struct of_device_id at91rm9200_rtc_dt_ids[] = {
>        { .compatible = "atmel,at91rm9200-rtc", .data =
> &at91rm9200_config },
>        { .compatible = "atmel,at91sam9x5-rtc", .data =
> &at91sam9x5_config },
>        { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, at91rm9200_rtc_dt_ids);
> #else
> #define at91rm9200_rtc_dt_ids NULL
> #endif /* CONFIG_OF */
> 
> 
> The shadow IMR variable was only active in the
>      .compatible = "atmel,at91sam9x5-rtc"
> case. That protected all existing users from any problems
> that might be introduced.

Indeed. But I am trying to build a patch that take the "broken/not
broken" information out of the IP revision number.

I try to write it and post it for your reviewing quickly.

Best regards,
-- 
Nicolas Ferre