Messages in this thread Patch in this message |  | | Date | Wed, 6 Sep 2000 23:25:18 +0100 (BST) | From | Tigran Aivazian <> | Subject | [patch-2.4.0-test8-pre5] take #2 on rtc fixes |
| |
Hi Linus,
Thanks to Jeff Garzik I slightly improved the patch (by returning the proper err codes always). So, to reiterate:
The rtc driver does:
a) not handle failures from misc_register() gracefully
b) not handle failures from create_proc_read_entry() gracefully
c) use check_region() where request_region() would suffice
d) not use KERN_ERR priority for errors consistently
e) initialises some static variables to 0 unnecessarily
The patch below fixes these issues/bugs. Tested under 2.4.0-test8-pre5.
Regards, Tigran
--- 240-test8-p5/drivers/char/rtc.c Fri Jul 28 09:58:59 2000 +++ linux/drivers/char/rtc.c Wed Sep 6 15:16:11 2000 @@ -39,9 +39,10 @@ * 1.10a Andrea Arcangeli: Alpha updates * 1.10b Andrew Morton: SMP lock fix * 1.10c Cesar Barros: SMP locking fixes and cleanup + * 1.10d Tigran Aivazian: Restructured rtc_init and got rid of check_region */ -#define RTC_VERSION "1.10c" +#define RTC_VERSION "1.10d" #define RTC_IO_EXTENT 0x10 /* Only really two ports, but... */ @@ -132,9 +133,9 @@ * 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 unsigned long rtc_status = 0; /* bitmapped status byte. */ -static unsigned long rtc_freq = 0; /* Current periodic IRQ rate */ -static unsigned long rtc_irq_data = 0; /* our output to the world */ +static unsigned long rtc_status; /* bitmapped status byte. */ +static unsigned long rtc_freq; /* Current periodic IRQ rate */ +static unsigned long rtc_irq_data; /* our output to the world */ /* * If this driver ever becomes modularised, it will be really nice @@ -615,6 +616,7 @@ static int __init rtc_init(void) { + int ret; #if defined(__alpha__) || defined(__mips__) unsigned int year, ctrl; unsigned long uip_watchdog; @@ -625,6 +627,17 @@ struct linux_ebus_device *edev; #endif + ret = misc_register(&rtc_dev); + if (ret) { + printk(KERN_ERR "rtc: cannot misc_register on minor=%d\n", RTC_MINOR); + goto out; + } + if (!create_proc_read_entry("driver/rtc", 0, 0, rtc_read_proc, NULL)) { + printk(KERN_ERR "rtc: cannot create file /proc/driver/rtc\n"); + ret = -ENOMEM; + goto outmisc; + } + #ifdef __sparc__ for_each_ebus(ebus) { for_each_ebusdev(edev, ebus) { @@ -633,8 +646,9 @@ } } } - printk("rtc_init: no PC rtc found\n"); - return -EIO; + printk(KERN_ERR "rtc: no PC rtc found\n"); + ret = -ENODEV; + goto outproc; found: rtc_port = edev->resource[0].start; @@ -644,35 +658,34 @@ * PCI Slot 2 INTA# (and some INTx# in Slot 1). SA_INTERRUPT here * is asking for trouble with add-on boards. Change to SA_SHIRQ. */ - if(request_irq(rtc_irq, rtc_interrupt, SA_INTERRUPT, "rtc", (void *)&rtc_port)) { + ret = request_irq(rtc_irq, rtc_interrupt, SA_INTERRUPT, "rtc", (void *)&rtc_port); + if(ret) { /* * Standard way for sparc to print irq's is to use * __irq_itoa(). I think for EBus it's ok to use %d. */ - printk("rtc: cannot register IRQ %d\n", rtc_irq); - return -EIO; + printk(KERN_ERR "rtc: cannot register IRQ %d\n", rtc_irq); + goto outproc; } #else - if (check_region (RTC_PORT (0), RTC_IO_EXTENT)) - { - printk(KERN_ERR "rtc: I/O port %d is not free.\n", RTC_PORT (0)); - return -EIO; + if (!request_region(RTC_PORT (0), RTC_IO_EXTENT, "rtc")) { + printk(KERN_ERR "rtc: I/O port %d is not free\n", RTC_PORT (0)); + ret = -EBUSY; + goto outproc; } #if RTC_IRQ - if(request_irq(RTC_IRQ, rtc_interrupt, SA_INTERRUPT, "rtc", NULL)) - { + ret = request_irq(RTC_IRQ, rtc_interrupt, SA_INTERRUPT, "rtc", NULL); + if(ret) { /* Yeah right, seeing as irq 8 doesn't even hit the bus. */ - printk(KERN_ERR "rtc: IRQ %d is not free.\n", RTC_IRQ); - return -EIO; + printk(KERN_ERR "rtc: IRQ %d is not free\n", RTC_IRQ); + release_region(RTC_PORT (0), RTC_IO_EXTENT); + goto outproc; } #endif - request_region(RTC_PORT(0), RTC_IO_EXTENT, "rtc"); #endif /* __sparc__ vs. others */ - misc_register(&rtc_dev); - create_proc_read_entry ("driver/rtc", 0, 0, rtc_read_proc, NULL); #if defined(__alpha__) || defined(__mips__) rtc_freq = HZ; @@ -716,34 +729,41 @@ rtc_freq = 1024; #endif + ret = 0; printk(KERN_INFO "Real Time Clock Driver v" RTC_VERSION "\n"); +out: + return ret; - return 0; +outproc: + remove_proc_entry("driver/rtc", NULL); +outmisc: + misc_deregister(&rtc_dev); + goto out; } -static void __exit rtc_exit (void) +static void __exit rtc_exit(void) { /* interrupts and maybe timer disabled at this point by rtc_release */ /* FIXME: Maybe??? */ if (rtc_status & RTC_TIMER_ON) { - spin_lock_irq (&rtc_lock); + spin_lock_irq(&rtc_lock); rtc_status &= ~RTC_TIMER_ON; del_timer(&rtc_irq_timer); - spin_unlock_irq (&rtc_lock); + spin_unlock_irq(&rtc_lock); printk(KERN_WARNING "rtc_exit(), and timer still running.\n"); } - remove_proc_entry ("driver/rtc", NULL); + remove_proc_entry("driver/rtc", NULL); misc_deregister(&rtc_dev); #ifdef __sparc__ - free_irq (rtc_irq, &rtc_port); + free_irq(rtc_irq, &rtc_port); #else - release_region (RTC_PORT (0), RTC_IO_EXTENT); + release_region(RTC_PORT (0), RTC_IO_EXTENT); #if RTC_IRQ - free_irq (RTC_IRQ, NULL); + free_irq(RTC_IRQ, NULL); #endif #endif /* __sparc__ */ }
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org Please read the FAQ at http://www.tux.org/lkml/
|  |