Hi Wolfram,
> > + retval = request_irq(rtc->irq, lpc32xx_rtc_alarm_interrupt,> > + IRQF_DISABLED, "rtcalarm", rtc);> > + if (retval < 0) {> > + dev_err(&pdev->dev, "Can't request interrupt\n");> > + goto err_free_irq;> > + }> > I saw that a number of rtc-drivers register their irq after they> register the device. I wonder if this is OK here? Couldn't it happen> that after rtc_device_register() there is a preemption and another> process could set the alarm? Then there is a race between interrupts> already enabled and no handler available, no?
If you do it the other way around the irq might trigger and the handler
reports an irq for a device that doesn't exist yet.
Best regards
Uwe

On Tue, Aug 10, 2010 at 02:00:55PM +0200, Uwe Kleine-König wrote:
> Hi Wolfram,> > > > + retval = request_irq(rtc->irq, lpc32xx_rtc_alarm_interrupt,> > > + IRQF_DISABLED, "rtcalarm", rtc);> > > + if (retval < 0) {> > > + dev_err(&pdev->dev, "Can't request interrupt\n");> > > + goto err_free_irq;> > > + }> > > > I saw that a number of rtc-drivers register their irq after they> > register the device. I wonder if this is OK here? Couldn't it happen> > that after rtc_device_register() there is a preemption and another> > process could set the alarm? Then there is a race between interrupts> > already enabled and no handler available, no?> If you do it the other way around the irq might trigger and the handler> reports an irq for a device that doesn't exist yet.
Well, I was assuming that you initially have all interrupts disabled...

Hi Wolfram,
Thanks for helping review this.
I'll get your suggestions installed and an update posted in a few days.
> > +> > + device_init_wakeup(&pdev->dev, 1);> > + rtc->rtc = rtc_device_register(RTC_NAME, &pdev->dev, &lpc32xx_rtc_ops,> > + THIS_MODULE);> > + if (IS_ERR(rtc->rtc)) {> > + dev_err(&pdev->dev, "Can't get RTC\n");> > + retval = PTR_ERR(rtc->rtc);> > + goto err_noreg;> > + }> > +> > + retval = request_irq(rtc->irq, lpc32xx_rtc_alarm_interrupt,> > + IRQF_DISABLED, "rtcalarm", rtc);> > + if (retval < 0) {> > + dev_err(&pdev->dev, "Can't request interrupt\n");> > + goto err_free_irq;> > + }> > I saw that a number of rtc-drivers register their irq after they register the> device. I wonder if this is OK here? Couldn't it happen that after> rtc_device_register() there is a preemption and another process could set the> alarm? Then there is a race between interrupts already enabled and no handler> available, no?>
The 32xx RTC registers can save states across power or reset
cycles and are only initialized if they have never been previously
initialized. When waking up from suspend, power on, or reset - the
alarm IRQ may have fired and may be enabled from a previous
initialization, so the RTC device needs to be registered first.
You bring up a good point though, I wonder if going through probe
requires that the alarm be disabled (ie, suspend won't go via probe,
but a hard system reset will). I'll review this a bit more.