Re: [PATCH] RTC: Fix for issues in the kernel RTC API.

[John Stultz <john.stultz@linaro.org>]

On Wed, 2011-02-02 at 15:58 -0200, Marcelo Roberto Jimenez wrote:
> The reason I added it before was that the user space behavior of the
> rtc-test kernel RTC driver had changed, but maybe its previous
> behavior is is no longer acceptable. Alarm interrupts were hand
> generated by the user by echoing to the sys interface, but it does not
> make sense to even have a timer in that case. That way,
> alarm_irq_enable() would bypass the kernel implementation.
> 
> Also, the way it is today, the programmer implementing the driver
> _must_ define alarm_irq_enable(), even if this function does nothing,
> otherwise you get EINVAL.
> 
> Maybe I got alarm_irq_enable() completely wrong and it should just be removed.

No, I think you are just bringing up some subtleties of the interface,
and clearly if the rtc-test driver is broken, that's my fault.

I've started playing with the rtc-test driver, and I am seeing some
issues there, so I'll try to see if I can't fix those quickly here.


> >> 2 - In the current configuration, an update interrupt, for example,
> >> would be a trigger for an alarm interrupt as well, even though the mode
> >> variable stated it was an update interrupt. To avoid that, mode is
> >> beeing checked inside rtc_handle_legacy_irq(). That used to be the
> >> previous user space behaviour.
> >
> > Right, we internally use recurring alarm interrupts to emulate the
> > update interrupt mode. Although I believe I provided the right behavior
> > to userland, so I'm not sure I see the issue or how your changes correct
> > things.
> 
> Again, what the rtc-test kernel RTC and the strongarm RTC user space
> behavior have changed. Alarm interrupts and update interrupts were
> generated by a different interrupts in the strongarm driver, and the
> rtc-test driver also behaved similarly, i.e., an update interrupt did
> not trigger an alarm interrupt. Currently, rtc_handle_legacy_irq()
> centralizes the irq processing, and by not checking the generated
> interrupt, it allows the new behavior, which seemed broken to me.

So 


> >> 3 - The irq_set_freq() and rtc_irq_set_state() functions are entry
> >> points to the RTC API, and were not calling the irq_set_state() and
> >> irq_set_freq() callbacks. These are also necessary overrides to provide
> >> a proper independent RTC implementation.
> >
> > Again, the freq is handled via a hrtimer now, so we shouldn't be calling
> > out to the hardware specific rtc driver anymore.
> 
> My reasoning here goes along the same lines as in the
> alarm_irq_enable() case, so I believe those callbacks should also be
> removed, right?
> 
> For rtc_irq_set_freq(), I believe that the current code allows a user
> space program to call the RTC_IRQP_SET ioclt() with freq == 0 and have
> a division by zero in the kernel code. I'll send a separate patch for
> that.

Yes. Very good catch here. Thanks for sending that in! I'll be sending
it upstream through Thomas shortly here.


> >> 4 - The rtc-test kernel RTC driver has been updated to work properly
> >> with the new kernel RTC timer infrastructure. This driver has been used
> >> together with the rtctest.c source code present in the file
> >> Documentation/rtc.txt to test the user space behaviour of all the
> >> modifications in this patch.
> >
> > So forgive me, as I might just be missing what you're describing. It
> > might be easier if you break fixes up into multiple patches?
> 
> The original purpose of the patch was to bring back the previous
> rtc-test driver behaviour, I think that anything less would be to
> commit broken work in progress code.

Very much agreed. 

So thanks for your more detailed explanation. Taking a look at the
rtc-test driver, I can see there's some issues in my emulation code
surrounding the split between the rtc driver ioctl and the emulated
processing (basically if the driver registers an ioctl handler, it
short-circuits the emulation code causing problems).

There's some ugliness here, as basically either the RTC driver can try
to handle all or most of the functionality through the ioctl or
implement operations and let the generic code handle the rest. 

So for instance, the driver can handle RTC_AIE_ON directly in the ioctl
operation, and then not implement the alarm_irq_enable operation.

So there's a number of ways you could implement the same functionality
in the driver.  I missed this inconsistency here. So I'll have to clean
that up somewhat. :(

Thanks again for pointing this issue out! I'll be sending you patches
for testing soon.

thanks
-john