It's me, again. This time I:
1) Changed mb() to wmb() (which seems the Right Thing to me)
2) Changed irqsave/irqrestore to irq in all places I'm sure (or at least I
think I am sure) that can't be called with interrupts off.
Now I just have to figure out if rtc_dropped_irq is called with interrupts off,
on, or unknown.
I just tested compiling, and probably missed something somewhere in the code.
--- linux-2.3.99-pre5/drivers/char/rtc.c Fri Apr 14 14:31:08 2000
+++ linux-rtcfix3/drivers/char/rtc.c Sun Apr 23 01:07:10 2000
@@ -40,6 +40,9 @@
* 1.10b Andrew Morton: SMP lock fix
*/
+/* If someone thinks this is spinlocking too much, feel free to use a single
+ * global /dev/rtc driver spinlock plus the rtc_lock for the CMOS_* stuff */
+
#define RTC_VERSION "1.10b"
#define RTC_IRQ 8 /* Can't see this changing soon. */
@@ -124,8 +127,21 @@
#define RTC_IS_OPEN 0x01 /* means /dev/rtc is in use */
#define RTC_TIMER_ON 0x02 /* missed irq timer active */
-static atomic_t rtc_status = ATOMIC_INIT(0); /* bitmapped status byte. */
+/*
+ * rtc_status is never changed by rtc_interrupt, and ioctl/open/close is
+ * protected by the big kernel lock. However, ioctl can still disable the timer
+ * in rtc_status and then with del_timer after the interrupt has read
+ * rtc_status but before mod_timer is called, which would then reenable the
+ * timer (but you would need to have an awful timing before you'd trip on it)
+ */
+static spinlock_t rtc_irq_timer_lock = SPIN_LOCK_UNLOCKED;
+static unsigned long rtc_status = 0; /* bitmapped status byte. */
static unsigned long rtc_freq = 0; /* Current periodic IRQ rate */
+/* Having rtc_lock double as rtc_irq_data_lock is evil */
+static spinlock_t rtc_irq_data_lock = SPIN_LOCK_UNLOCKED;
+/* Making this two variables instead of one might make the interrupt a bit
+ * faster (and exchange a spinlock (2 locked mem accesses) by two variable
+ * atomic updates (2 locked mem accesses)). Or maybe slower. Ideas? */
static unsigned long rtc_irq_data = 0; /* our output to the world */
/*
@@ -151,6 +167,8 @@
static void rtc_interrupt(int irq, void *dev_id, struct pt_regs *regs)
{
+ unsigned long rtc_data;
+
/*
* Can be an alarm interrupt, update complete interrupt,
* or a periodic interrupt. We store the status in the
@@ -158,19 +176,29 @@
* the last read in the remainder of rtc_irq_data.
*/
+ /* First, ack the interrupt */
spin_lock (&rtc_lock);
+ rtc_data = CMOS_READ(RTC_INTR_FLAGS);
+ spin_unlock (&rtc_lock);
+
+ /* Second, avoid a bogus rtc_dropped_irq */
+ spin_lock (&rtc_irq_timer_lock);
+ if (rtc_status & RTC_TIMER_ON)
+ mod_timer(&rtc_irq_timer, jiffies + HZ/rtc_freq + 2*HZ/100);
+ spin_unlock (&rtc_irq_timer_lock);
+
+ /* Third, update rtc_irq_data */
+ spin_lock (&rtc_irq_data_lock);
rtc_irq_data += 0x100;
rtc_irq_data &= ~0xff;
- rtc_irq_data |= (CMOS_READ(RTC_INTR_FLAGS) & 0xF0);
- spin_unlock (&rtc_lock);
+ rtc_irq_data |= rtc_data & 0xF0;
+ spin_unlock (&rtc_irq_data_lock);
+ /* Now do the rest of the actions */
wake_up_interruptible(&rtc_wait);
if (rtc_async_queue)
kill_fasync (rtc_async_queue, SIGIO, POLL_IN);
-
- if (atomic_read(&rtc_status) & RTC_TIMER_ON)
- mod_timer(&rtc_irq_timer, jiffies + HZ/rtc_freq + 2*HZ/100);
}
#endif
@@ -200,7 +228,18 @@
current->state = TASK_INTERRUPTIBLE;
- while ((data = xchg(&rtc_irq_data, 0)) == 0) {
+ do {
+ /* First make it right. Then make it fast. Putting this whole
+ * block within the parentheses of a while would be too
+ * confusing. And no, xchg() is not the answer. */
+ spin_lock_irq (&rtc_irq_data_lock);
+ data = rtc_irq_data;
+ rtc_irq_data = 0;
+ spin_unlock_irq (&rtc_irq_data_lock);
+
+ if (data != 0)
+ break;
+
if (file->f_flags & O_NONBLOCK) {
retval = -EAGAIN;
goto out;
@@ -210,7 +249,7 @@
goto out;
}
schedule();
- }
+ } while (1);
retval = put_user(data, (unsigned long *)buf);
if (!retval)
@@ -227,7 +266,6 @@
unsigned long arg)
{
- unsigned long flags;
struct rtc_time wtime;
switch (cmd) {
@@ -245,11 +283,12 @@
case RTC_PIE_OFF: /* Mask periodic int. enab. bit */
{
mask_rtc_irq_bit(RTC_PIE);
- if (atomic_read(&rtc_status) & RTC_TIMER_ON) {
+ spin_lock_irq (&rtc_irq_timer_lock);
+ if (rtc_status & RTC_TIMER_ON) {
+ rtc_status &= ~RTC_TIMER_ON;
del_timer(&rtc_irq_timer);
- atomic_set(&rtc_status,
- atomic_read(&rtc_status) & ~RTC_TIMER_ON);
}
+ spin_unlock_irq (&rtc_irq_timer_lock);
return 0;
}
case RTC_PIE_ON: /* Allow periodic ints */
@@ -262,11 +301,14 @@
if ((rtc_freq > 64) && (!capable(CAP_SYS_RESOURCE)))
return -EACCES;
- if (!(atomic_read(&rtc_status) & RTC_TIMER_ON)) {
- atomic_set(&rtc_status,
- atomic_read(&rtc_status) | RTC_TIMER_ON);
+ /* No need to spinlock here if RTC_TIMER_ON is only enabled at
+ * the end. The wmb() makes sure the timer was added before we
+ * mark it as enabled in the status flags*/
+ if (!(rtc_status & RTC_TIMER_ON)) {
rtc_irq_timer.expires = jiffies + HZ/rtc_freq + 2*HZ/100;
add_timer(&rtc_irq_timer);
+ wmb ();
+ rtc_status |= RTC_TIMER_ON;
}
set_rtc_irq_bit(RTC_PIE);
return 0;
@@ -320,7 +362,7 @@
if (sec >= 60)
sec = 0xff;
- spin_lock_irqsave(&rtc_lock, flags);
+ spin_lock_irq(&rtc_lock);
if (!(CMOS_READ(RTC_CONTROL) & RTC_DM_BINARY) ||
RTC_ALWAYS_BCD)
{
@@ -331,7 +373,7 @@
CMOS_WRITE(hrs, RTC_HOURS_ALARM);
CMOS_WRITE(min, RTC_MINUTES_ALARM);
CMOS_WRITE(sec, RTC_SECONDS_ALARM);
- spin_unlock_irqrestore(&rtc_lock, flags);
+ spin_unlock_irq(&rtc_lock);
return 0;
}
@@ -346,8 +388,7 @@
unsigned char mon, day, hrs, min, sec, leap_yr;
unsigned char save_control, save_freq_select;
unsigned int yrs;
- unsigned long flags;
-
+
if (!capable(CAP_SYS_TIME))
return -EACCES;
@@ -379,11 +420,11 @@
if ((yrs -= epoch) > 255) /* They are unsigned */
return -EINVAL;
- spin_lock_irqsave(&rtc_lock, flags);
+ spin_lock_irq(&rtc_lock);
if (!(CMOS_READ(RTC_CONTROL) & RTC_DM_BINARY)
|| RTC_ALWAYS_BCD) {
if (yrs > 169) {
- spin_unlock_irqrestore(&rtc_lock, flags);
+ spin_unlock_irq(&rtc_lock);
return -EINVAL;
}
if (yrs >= 100)
@@ -412,7 +453,7 @@
CMOS_WRITE(save_control, RTC_CONTROL);
CMOS_WRITE(save_freq_select, RTC_FREQ_SELECT);
- spin_unlock_irqrestore(&rtc_lock, flags);
+ spin_unlock_irq(&rtc_lock);
return 0;
}
#ifndef __alpha__
@@ -448,11 +489,11 @@
rtc_freq = arg;
- spin_lock_irqsave(&rtc_lock, flags);
+ spin_lock_irq(&rtc_lock);
val = CMOS_READ(RTC_FREQ_SELECT) & 0xf0;
val |= (16 - tmp);
CMOS_WRITE(val, RTC_FREQ_SELECT);
- spin_unlock_irqrestore(&rtc_lock, flags);
+ spin_unlock_irq(&rtc_lock);
return 0;
}
#else
@@ -489,18 +530,18 @@
static int rtc_open(struct inode *inode, struct file *file)
{
- unsigned long flags;
-
- if(atomic_read(&rtc_status) & RTC_IS_OPEN)
+ /* If someday somebody decides to remove the kernel_lock on open and
+ * close and ioctl this is gonna get open to races again */
+ if(rtc_status & RTC_IS_OPEN)
return -EBUSY;
MOD_INC_USE_COUNT;
- atomic_set(&rtc_status, atomic_read(&rtc_status) | RTC_IS_OPEN);
+ rtc_status |= RTC_IS_OPEN;
- spin_lock_irqsave (&rtc_lock, flags);
+ spin_lock_irq (&rtc_irq_data_lock);
rtc_irq_data = 0;
- spin_unlock_irqrestore (&rtc_lock, flags);
+ spin_unlock_irq (&rtc_irq_data_lock);
return 0;
}
@@ -512,7 +553,6 @@
static int rtc_release(struct inode *inode, struct file *file)
{
- unsigned long flags;
#ifndef __alpha__
/*
* Turn off all interrupts once the device is no longer
@@ -521,19 +561,21 @@
unsigned char tmp;
- spin_lock_irqsave(&rtc_lock, flags);
+ spin_lock_irq(&rtc_lock);
tmp = CMOS_READ(RTC_CONTROL);
tmp &= ~RTC_PIE;
tmp &= ~RTC_AIE;
tmp &= ~RTC_UIE;
CMOS_WRITE(tmp, RTC_CONTROL);
CMOS_READ(RTC_INTR_FLAGS);
- spin_unlock_irqrestore(&rtc_lock, flags);
+ spin_unlock_irq(&rtc_lock);
- if (atomic_read(&rtc_status) & RTC_TIMER_ON) {
- atomic_set(&rtc_status, atomic_read(&rtc_status) & ~RTC_TIMER_ON);
+ spin_lock_irq (&rtc_irq_timer_lock);
+ if (rtc_status & RTC_TIMER_ON) {
+ rtc_status &= ~RTC_TIMER_ON;
del_timer(&rtc_irq_timer);
}
+ spin_unlock_irq (&rtc_irq_timer_lock);
if (file->f_flags & FASYNC) {
rtc_fasync (-1, file, 0);
@@ -542,23 +584,24 @@
#endif
MOD_DEC_USE_COUNT;
- spin_lock_irqsave (&rtc_lock, flags);
+ /* FIXME: why is this outside the #ifndef __alpha__? */
+ spin_lock_irq (&rtc_irq_data_lock);
rtc_irq_data = 0;
- spin_unlock_irqrestore (&rtc_lock, flags);
- atomic_set(&rtc_status, atomic_read(&rtc_status) & ~RTC_IS_OPEN);
+ spin_unlock_irq (&rtc_irq_data_lock);
+ rtc_status &= ~RTC_IS_OPEN;
return 0;
}
#ifndef __alpha__
static unsigned int rtc_poll(struct file *file, poll_table *wait)
{
- unsigned long l, flags;
+ unsigned long l;
poll_wait(file, &rtc_wait, wait);
- spin_lock_irqsave (&rtc_lock, flags);
+ spin_lock_irq (&rtc_irq_data_lock);
l = rtc_irq_data;
- spin_unlock_irqrestore (&rtc_lock, flags);
+ spin_unlock_irq (&rtc_irq_data_lock);
if (l != 0)
return POLLIN | POLLRDNORM;
@@ -591,7 +634,6 @@
static int __init rtc_init(void)
{
- unsigned long flags;
#ifdef __alpha__
unsigned int year, ctrl;
unsigned long uip_watchdog;
@@ -662,10 +704,10 @@
while (jiffies - uip_watchdog < 2*HZ/100)
barrier();
- spin_lock_irqsave(&rtc_lock, flags);
+ spin_lock_irq(&rtc_lock);
year = CMOS_READ(RTC_YEAR);
ctrl = CMOS_READ(RTC_CONTROL);
- spin_unlock_irqrestore(&rtc_lock, flags);
+ spin_unlock_irq(&rtc_lock);
if (!(ctrl & RTC_DM_BINARY) || RTC_ALWAYS_BCD)
BCD_TO_BIN(year); /* This should never happen... */
@@ -683,10 +725,10 @@
#ifndef __alpha__
init_timer(&rtc_irq_timer);
rtc_irq_timer.function = rtc_dropped_irq;
- spin_lock_irqsave(&rtc_lock, flags);
+ spin_lock_irq(&rtc_lock);
/* Initialize periodic freq. to CMOS reset default, which is 1024Hz */
CMOS_WRITE(((CMOS_READ(RTC_FREQ_SELECT) & 0xF0) | 0x06), RTC_FREQ_SELECT);
- spin_unlock_irqrestore(&rtc_lock, flags);
+ spin_unlock_irq(&rtc_lock);
#endif
rtc_freq = 1024;
@@ -698,9 +740,14 @@
static void __exit rtc_exit (void)
{
/* interrupts and maybe timer disabled at this point by rtc_release */
+ /* FIXME: Maybe??? */
- if (atomic_read(&rtc_status) & RTC_TIMER_ON)
+#if 0
+ spin_lock_irqsave (&rtc_irq_timer_lock, flags);
+ if (rtc_status & RTC_TIMER_ON)
del_timer(&rtc_irq_timer);
+ spin_unlock_irqrestore (&rtc_irq_timer_lock, flags);
+#endif
remove_proc_entry ("driver/rtc", NULL);
misc_deregister(&rtc_dev);
@@ -734,16 +781,38 @@
static void rtc_dropped_irq(unsigned long data)
{
- unsigned long flags;
+ unsigned long rtc_data;
+ unsigned long flags; /* FIXME are we on cli() or not? */
printk(KERN_INFO "rtc: lost some interrupts at %ldHz.\n", rtc_freq);
- mod_timer(&rtc_irq_timer, jiffies + HZ/rtc_freq + 2*HZ/100);
- spin_lock_irqsave(&rtc_lock, flags);
+ /* First, ack the (missed) interrupt */
+ spin_lock_irqsave (&rtc_lock, flags);
+ rtc_data = CMOS_READ(RTC_INTR_FLAGS);
+ spin_unlock_irqrestore (&rtc_lock, flags);
+
+ /* Second, avoid a bogus rtc_dropped_irq */
+ spin_lock_irqsave (&rtc_irq_timer_lock, flags);
+ /* Just in case someone disabled the timer from behind our back... */
+ if (rtc_status & RTC_TIMER_ON)
+ mod_timer(&rtc_irq_timer, jiffies + HZ/rtc_freq + 2*HZ/100);
+ spin_unlock_irqrestore (&rtc_irq_timer_lock, flags);
+
+ /* Third, update rtc_irq_data */
+ /* FIXME: what happens if the real interrupt comes before we get here?
+ * The order of the status would get a bit missed up... Maybe I should
+ * ack within this block? */
+ spin_lock_irqsave(&rtc_irq_data_lock, flags);
rtc_irq_data += ((rtc_freq/HZ)<<8);
rtc_irq_data &= ~0xff;
- rtc_irq_data |= (CMOS_READ(RTC_INTR_FLAGS) & 0xF0); /* restart */
- spin_unlock_irqrestore(&rtc_lock, flags);
+ rtc_irq_data |= rtc_data & 0xF0;
+ spin_unlock_irqrestore(&rtc_irq_data_lock, flags);
+
+ /* Now we have new data */
+ wake_up_interruptible(&rtc_wait);
+
+ if (rtc_async_queue)
+ kill_fasync (rtc_async_queue, SIGIO, POLL_IN);
}
#endif
@@ -758,12 +827,11 @@
char *p;
struct rtc_time tm;
unsigned char batt, ctrl;
- unsigned long flags;
- spin_lock_irqsave(&rtc_lock, flags);
+ spin_lock_irq(&rtc_lock);
batt = CMOS_READ(RTC_VALID) & RTC_VRT;
ctrl = CMOS_READ(RTC_CONTROL);
- spin_unlock_irqrestore(&rtc_lock, flags);
+ spin_unlock_irq(&rtc_lock);
p = buf;
@@ -843,21 +911,20 @@
/*
* Returns true if a clock update is in progress
*/
+/* FIXME shouldn't this be above rtc_init to inline it? */
static inline unsigned char rtc_is_updating(void)
{
- unsigned long flags;
unsigned char uip;
- spin_lock_irqsave(&rtc_lock, flags);
+ spin_lock_irq(&rtc_lock);
uip = (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP);
- spin_unlock_irqrestore(&rtc_lock, flags);
+ spin_unlock_irq(&rtc_lock);
return uip;
}
static void get_rtc_time(struct rtc_time *rtc_tm)
{
-
- unsigned long flags, uip_watchdog = jiffies;
+ unsigned long uip_watchdog = jiffies;
unsigned char ctrl;
/*
@@ -880,7 +947,7 @@
* RTC has RTC_DAY_OF_WEEK, we ignore it, as it is only updated
* by the RTC when initially set to a non-zero value.
*/
- spin_lock_irqsave(&rtc_lock, flags);
+ spin_lock_irq(&rtc_lock);
rtc_tm->tm_sec = CMOS_READ(RTC_SECONDS);
rtc_tm->tm_min = CMOS_READ(RTC_MINUTES);
rtc_tm->tm_hour = CMOS_READ(RTC_HOURS);
@@ -888,7 +955,7 @@
rtc_tm->tm_mon = CMOS_READ(RTC_MONTH);
rtc_tm->tm_year = CMOS_READ(RTC_YEAR);
ctrl = CMOS_READ(RTC_CONTROL);
- spin_unlock_irqrestore(&rtc_lock, flags);
+ spin_unlock_irq(&rtc_lock);
if (!(ctrl & RTC_DM_BINARY) || RTC_ALWAYS_BCD)
{
@@ -912,19 +979,18 @@
static void get_rtc_alm_time(struct rtc_time *alm_tm)
{
- unsigned long flags;
unsigned char ctrl;
/*
* Only the values that we read from the RTC are set. That
* means only tm_hour, tm_min, and tm_sec.
*/
- spin_lock_irqsave(&rtc_lock, flags);
+ spin_lock_irq(&rtc_lock);
alm_tm->tm_sec = CMOS_READ(RTC_SECONDS_ALARM);
alm_tm->tm_min = CMOS_READ(RTC_MINUTES_ALARM);
alm_tm->tm_hour = CMOS_READ(RTC_HOURS_ALARM);
ctrl = CMOS_READ(RTC_CONTROL);
- spin_unlock_irqrestore(&rtc_lock, flags);
+ spin_unlock_irq(&rtc_lock);
if (!(ctrl & RTC_DM_BINARY) || RTC_ALWAYS_BCD)
{
@@ -948,28 +1014,34 @@
static void mask_rtc_irq_bit(unsigned char bit)
{
unsigned char val;
- unsigned long flags;
- spin_lock_irqsave(&rtc_lock, flags);
+ spin_lock_irq(&rtc_lock);
val = CMOS_READ(RTC_CONTROL);
val &= ~bit;
CMOS_WRITE(val, RTC_CONTROL);
CMOS_READ(RTC_INTR_FLAGS);
+ spin_unlock_irq(&rtc_lock);
+
+ /* FIXME is moving this outside the rtc_lock ok? */
+ spin_lock_irq (&rtc_irq_data_lock);
rtc_irq_data = 0;
- spin_unlock_irqrestore(&rtc_lock, flags);
+ spin_unlock_irq (&rtc_irq_data_lock);
}
static void set_rtc_irq_bit(unsigned char bit)
{
unsigned char val;
- unsigned long flags;
- spin_lock_irqsave(&rtc_lock, flags);
+ spin_lock_irq(&rtc_lock);
val = CMOS_READ(RTC_CONTROL);
val |= bit;
CMOS_WRITE(val, RTC_CONTROL);
CMOS_READ(RTC_INTR_FLAGS);
+ spin_unlock_irq(&rtc_lock);
+
+ /* FIXME is doing this outside the rtc_lock ok? */
+ spin_lock_irq (&rtc_irq_data_lock);
rtc_irq_data = 0;
- spin_unlock_irqrestore(&rtc_lock, flags);
+ spin_unlock_irq (&rtc_irq_data_lock);
}
#endif
-- Cesar Eduardo Barros cesarb@web4u.com.br cesarb@dcc.ufrj.br- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.rutgers.edu Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Sun Apr 23 2000 - 21:00:21 EST