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

[jhovold@gmail.com (Johan Hovold)]

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.

However, wmb() always implies a compiler barrier which is what is needed
in this case.

> 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).

>               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, ...

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.

Nicolas, should I send a revert patch and follow up with a fix for the
broken SoCs which includes the required barriers and read-backs?

Note that the patch is already being picked up for some stable trees.
The fix I'm proposing would require adding minimal DT-support to the
driver and is not really stable material. Therefore, a revert followed
by a patch for 3.10 seems like the way to go.

Johan