Re: [PATCH V2] drivers/rtc/rtc-ds1307.c: Enable the mcp794xx alarm after programming time

[Nishanth Menon]

On 04/21/2015 08:09 PM, Alexandre Belloni wrote:
> On 21/04/2015 at 18:58:43 -0500, Nishanth Menon wrote :
>>>
>>> Consider the following use case: a platform is setting the RTC alarm
>>> before going to suspend to ram. Before your patch, it may be woken up
>> ^^ precisely what I am trying to solve.
>>
>>> quite quickly, before expected. After your patch, it may never wake at
>>> all.
>>
>> Why is that so? when set alarm is requested for time X, you want
>> interrupt at time X, not an interrupt for previous configured RTC
>> alarm time!
>>
> 
> You expect at least an interrupt.

And you will get an interrupt if the event occurs before the i2c burst
starts. Once the i2cburst does start, you are committing to the new time.

> 
>> If the time X is > the point when ALM0 is programmed, then you will
>> get an interrupt.
>>
> 
> You are eluding my point. What happens if the alarm expires before ALM0
> is programmed? Your system is probably dead because it will never wake
> up.

if the previous alarm does expire before the new alarm is programmed
(before the burst is started), you will get an event - why does this
patch change this? the case you are concerned seem to be the case where
the time you program is in the past at the point when alarm interrupt is
enabled.

Lets see the current behavior(before this patch): time currently
requested is in the past, you are dead anyways. this can happen since
the decision of programming the time is done in userspace(example with
rtcwake) and there are latencies as part of suspend path (file sync)
that can cause the possible cases - consider "rtcwake -s 1" - and for
the moment, lets assume that rtc event is the only wakeup event possible
for the system (to address your point).

a) If the rtcwake progams time before initiating suspend state and prior
to suspend_no_irq is hit event triggers, irq is handled by rtc driver,
and we proceed on to suspend state - but no further irqs can be expected
"dead state"
b) predicted time occurs before suspend_no_irq level is reached, but
before the system actually enters suspend -> event occurs with irq
disabled and depending on smartness of platform code, it can detect that
the wakeup event occurred prior to attempting suspend (hence "dead state")
c) If rtcwake's set_alarm invocation is slowed due to filesystem or
other load activities, the time requested to wakeup is already in the
past, you are in deadstate as the wakeup event has already occurred
prior to entering suspend path.

Note: even with android alarm driver(at least the last time i looked at
it a few years ago), this situation is probably escaping (a), but (b)
and (c) is still real and has to be properly anticipated.

My patch does nothing different to this behavior - only point changed is
*if the previous event does not take place prior to new request being
initiated, then we have to assume that the new time is what we wanted
wakeup event for*. That I strongly believe is necessary.

Point being - using a time for wake attempted to be predicted ahead of
time of suspend path is a a bunch of empirical data analysis - you can
have a dead system today and this code does not change that behavior - I
doubt you can ever get a clean fix without a constant latency.

>> If you get an interrupt (like my screenshot shows) because the new
>> value has not yet been programmed (just because we enabled interrupt
>> before programming time), it is unexpected event and wrong!
>>
>> Another scenario: Take the following time points A < B < C < D
>> we program at time (A), an interrupt for time (C).
>> but at time B, we intiate a new time request for time (D).
>> if we happen to send the first ALM0EN at time C (before programming
>> D), you will generate an interrupt, but before the irq handler can
>> handle (since we are doing burst i2c), we program D which clears the
>> irq status (as can be seen in waveform).
>>
>> This does not make sense for a predictable behavior! Yeah, it will
>> wakeup quickly, but when we go and read irqstatus (ALM0IF), it will be
>> 0 and nothing will get reported to rtc subsystem. So:
>> a) we woke up at a time not requested - this is wrong
>> b) our irq handler has nothing to handle! - this is wrong as well.
>>
>> in short, the behavior you are asking for is quiet the wrong behavior!
>>
> 
> I agree that an unexpected event is wrong but it is still better than a
> dead system. I'm not asking to keep the current behaviour. I'm just
> wanting to try to not introduce another race condition.
> 
> What about setting ALM0MTH to 0x1F before reading the control registers?

Ummm.... why is that any different? In effect, you are just disabling
the compare logic inside the RTC differently and that said, I am not
entirely sure if I want to test the MCP794xx RTL designer's logic with
invalid dates being programmed for month/date etc.. ;)

> You could also read only the first 3 registers as all the others are
> overwritten. And finally, you only need to write 9 bytes instead of 10

True - had noticed that as well. have'nt had time to optimize the path yet.

> (register 0x10 is reserved). While not eliminating it completely, this
> will definitively reduce the race condition window.

Yep - was planning on fixing 10 to 9 - But that is not related to this
patch anyways. I will do that follow on patch next time I get a break :)

yet another patch that I need to do is to check for OSCRUN before
attempting to program time or alarm -> no point in setting time is
oscillator is non-functional even after waiting for oscillator
stabilization time after ST bit is set.. again.. need to do that
separate from this patch.

-- 
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/