Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754818Ab1BBR6o (ORCPT ); Wed, 2 Feb 2011 12:58:44 -0500 Received: from mail-ww0-f42.google.com ([74.125.82.42]:37592 "EHLO mail-ww0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754491Ab1BBR6n convert rfc822-to-8bit (ORCPT ); Wed, 2 Feb 2011 12:58:43 -0500 MIME-Version: 1.0 In-Reply-To: <1296615605.3336.239.camel@work-vm> References: <1296577820-27456-1-git-send-email-mroberto@cpti.cetuc.puc-rio.br> <1296615605.3336.239.camel@work-vm> From: Marcelo Roberto Jimenez Date: Wed, 2 Feb 2011 15:58:21 -0200 Message-ID: Subject: Re: [PATCH] RTC: Fix for issues in the kernel RTC API. To: John Stultz Cc: a.zummo@towertech.it, rtc-linux@googlegroups.com, linux-kernel@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9348 Lines: 230 Hi John, Thank you for the quick answer. My original motivation was to rewrite the strongarm rtc, which recently got broken due to the last changes. In the process, I tested with the rtc-test kernel RTC driver to make it work as it did before, but I may have got some semantics wrong. On Wed, Feb 2, 2011 at 01:00, John Stultz wrote: > On Tue, 2011-02-01 at 14:30 -0200, Marcelo Roberto Jimenez wrote: >> This patch fixes the following issues with the RTC API: >> >> 1 - The alarm_irq_enable() and the update_irq_enable() callbacks were >> not beeing called anywhere in the code. They are necessary if you want >> to write a driver that does not use the kernel pre-existent timer >> implemented alarm and update interrupts. > > I'm not sure I totally follow this one. alarm_irq_enable is still being > called in that function. > > There are some callbacks that are no longer being made because the > functionality is now virtualized internally to the kernel. Could you > expand on the rational for this a bit more? Possibly showing a userland > example why the virtualized implementation isn't sufficient? > > I will admit that I probably should clean out the callback hooks and > remove them from the drivers, as that probably adds to some of the > confusion here. You are right, alarm_irq_enable() is already being called, but after the timer enqueue/remove code. My original intention was to change the order but I missed the removal of the old code, sorry for that. I understood that this callback was there in case one wants to implement an alternative to the current kernel RTC implementation, but maybe I got that wrong. 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. >> 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. >> 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. >> 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. Anyway, this rtc-test driver has been handy before for debugging purposes, I guess we should decide if it still makes sense to have it around. Currently, it is broken. > More comments in the code below. > > ... > > So first, you're not checking if rtc->ops is valid before dereferencing. > Second, I'm not sure I see why you would call it and then avoid the new > virtualized rtc layer? Esp since we do end up calling the same > alarm_irq_enable function later (see the top of your second chunk). > > ... > > Same issue above here. Ok, I have answered that on my first paragraph. >> @@ -263,19 +274,23 @@ static void rtc_handle_legacy_irq(struct rtc_device *rtc, int num, int mode) >> ?{ >> ? ? ? unsigned long flags; >> >> - ? ? /* mark one irq of the appropriate mode */ >> - ? ? spin_lock_irqsave(&rtc->irq_lock, flags); >> - ? ? rtc->irq_data = (rtc->irq_data + (num << 8)) | (RTC_IRQF|mode); >> - ? ? spin_unlock_irqrestore(&rtc->irq_lock, flags); >> - >> - ? ? /* call the task func */ >> - ? ? spin_lock_irqsave(&rtc->irq_task_lock, flags); >> - ? ? if (rtc->irq_task) >> - ? ? ? ? ? ? rtc->irq_task->func(rtc->irq_task->private_data); >> - ? ? spin_unlock_irqrestore(&rtc->irq_task_lock, flags); >> - >> - ? ? wake_up_interruptible(&rtc->irq_queue); >> - ? ? kill_fasync(&rtc->async_queue, SIGIO, POLL_IN); >> + ? ? if (((mode & RTC_UF) && rtc->uie_rtctimer.enabled) || >> + ? ? ? ? ((mode & RTC_AF) && rtc->aie_timer.enabled) || >> + ? ? ? ? ((mode & RTC_PF) && rtc->pie_enabled)) { >> + ? ? ? ? ? ? /* mark one irq of the appropriate mode */ >> + ? ? ? ? ? ? spin_lock_irqsave(&rtc->irq_lock, flags); >> + ? ? ? ? ? ? rtc->irq_data = (rtc->irq_data + (num << 8)) | (RTC_IRQF|mode); >> + ? ? ? ? ? ? spin_unlock_irqrestore(&rtc->irq_lock, flags); >> + >> + ? ? ? ? ? ? /* call the task func */ >> + ? ? ? ? ? ? spin_lock_irqsave(&rtc->irq_task_lock, flags); >> + ? ? ? ? ? ? if (rtc->irq_task) >> + ? ? ? ? ? ? ? ? ? ? rtc->irq_task->func(rtc->irq_task->private_data); >> + ? ? ? ? ? ? spin_unlock_irqrestore(&rtc->irq_task_lock, flags); >> + >> + ? ? ? ? ? ? wake_up_interruptible(&rtc->irq_queue); >> + ? ? ? ? ? ? kill_fasync(&rtc->async_queue, SIGIO, POLL_IN); >> + ? ? } >> ?} > > I don't quite understand what this is doing. This is preventing interpreting an alarm interrupt as an update interrupt. > ... > > Again, not checking rtc->ops and irq_set_state shouldn't be called > anymore, as the hrtimer handles periodic mode irq emulation now. > > ... > > Same as the last comment here. irq_set_freq is handled by generic > emulation code now. Ok, addressed before too. > > >> diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c >> index 212b16e..d901160 100644 >> --- a/drivers/rtc/rtc-dev.c >> +++ b/drivers/rtc/rtc-dev.c >> @@ -261,12 +261,12 @@ static long rtc_dev_ioctl(struct file *file, >> ? ? ? ? ? ? ? return rtc_set_time(rtc, &tm); >> >> ? ? ? case RTC_PIE_ON: >> - ? ? ? ? ? ? err = rtc_irq_set_state(rtc, NULL, 1); >> - ? ? ? ? ? ? break; >> + ? ? ? ? ? ? mutex_unlock(&rtc->ops_lock); >> + ? ? ? ? ? ? return rtc_irq_set_state(rtc, NULL, 1); >> >> ? ? ? case RTC_PIE_OFF: >> - ? ? ? ? ? ? err = rtc_irq_set_state(rtc, NULL, 0); >> - ? ? ? ? ? ? break; >> + ? ? ? ? ? ? mutex_unlock(&rtc->ops_lock); >> + ? ? ? ? ? ? return rtc_irq_set_state(rtc, NULL, 0); >> >> ? ? ? case RTC_AIE_ON: >> ? ? ? ? ? ? ? mutex_unlock(&rtc->ops_lock); >> @@ -285,8 +285,8 @@ static long rtc_dev_ioctl(struct file *file, >> ? ? ? ? ? ? ? return rtc_update_irq_enable(rtc, 0); >> >> ? ? ? case RTC_IRQP_SET: >> - ? ? ? ? ? ? err = rtc_irq_set_freq(rtc, NULL, arg); >> - ? ? ? ? ? ? break; >> + ? ? ? ? ? ? mutex_unlock(&rtc->ops_lock); >> + ? ? ? ? ? ? return rtc_irq_set_freq(rtc, NULL, arg); >> >> ? ? ? case RTC_IRQP_READ: > > Why the unlock/return change here? Because the mutex had to be taken inside rtc_irq_set_state() and rtc_irq_set_freq() in order to perform the callbacks. And being entry points, the mutex must no be taken. Breaking instead of returning would release the mutex twice. > If I'm being totally daft here, my apologies and please let me know. The > rework I did was fairly large, and its quite possible there are issues > lurking. But I'm not connecting your patch to specific breakage yet. > > thanks > -john Thanks for your comments, I will leave the rtc-driver aside, as its old behaviour probably no longer makes sense and concentrate on the sa1100 driver. Regards, Marcelo. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/