Re: [PATCH v7 1/2] Use "request_muxed_region" in it87 watchdog drivers

From: Natarajan Gurumoorthy
Date: Wed Apr 27 2011 - 13:49:11 EST

Next message: Steven Rostedt: "Re: [PATCH] linux/string.h: Introduce streq macro."
Previous message: Anca Emanuel: "Re: [Linaro-mm-sig] [RFC] ARM DMA mapping TODO, v1"
Next in thread: Guenter Roeck: "Re: [PATCH v7 1/2] Use "request_muxed_region" in it87 watchdogdrivers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Guenter,

On Thu, Apr 21, 2011 at 9:44 PM, Guenter Roeck
<guenter.roeck@xxxxxxxxxxxx> wrote:
> > /**
> > @@ -303,8 +308,6 @@ static void wdt_stop(void)
> >
> > static int wdt_set_timeout(int t)
> > {
> > - unsigned long flags;
> > -
> > if (t < 1 || t > max_units * 60)
> > return -EINVAL;
> >
> > @@ -313,14 +316,14 @@ static int wdt_set_timeout(int t)
> > else
> > timeout = t;
> >
> > - spin_lock_irqsave(&spinlock, flags);
> > if (test_bit(WDTS_TIMER_RUN, &wdt_status)) {
> > - superio_enter();
> > + if (superio_enter())
> > + return -EBUSY;
> > +
> I don't think you can do that here. You are moving the lock,
> meaning test_bit is no longer protected by the lock.
>
I am not sure I am in agreement with your here. If you look at other
places in the original driver I see sections of code in "wdt_ioctl"
that look as follows:
case WDIOC_SETOPTIONS:
if (get_user(new_options, uarg.i))
return -EFAULT;

switch (new_options) {
case WDIOS_DISABLECARD:
if (test_bit(WDTS_TIMER_RUN, &wdt_status))
wdt_stop();
clear_bit(WDTS_TIMER_RUN, &wdt_status);
return 0;

case WDIOS_ENABLECARD:
if (!test_and_set_bit(WDTS_TIMER_RUN, &wdt_status))
wdt_start();
return 0;

default:
return -EFAULT;
}

They have run through manipulating the "WDTS_TIMER_RUN" bit in
wdt_status without touching the spinlock. "wdt_stop" does acquire the
spinlock before touching the hardware through superio_enter. It seems
to me that the original writers of this driver were using the spinlock
to isolate access to the hardware and we will be achieving the same by
the use of "request_muxed_region" call from superio_enter.

I am fixing the rest of the issues pointed out by you and am hoping to
ship out a patch by the end of the week.

Regards
Nat

> > superio_select(GPIO);
> > wdt_update_timeout();
> > superio_exit();
> > }
> > - spin_unlock_irqrestore(&spinlock, flags);
> > return 0;
> > }
> >
> > @@ -339,12 +342,10 @@ static int wdt_set_timeout(int t)
> >
> > static int wdt_get_status(int *status)
> > {
> > - unsigned long flags;
> > -
> > *status = 0;
> > if (testmode) {
> > - spin_lock_irqsave(&spinlock, flags);
> > - superio_enter();
> > + if (superio_enter())
> > + return -EBUSY;
> > superio_select(GPIO);
> > if (superio_inb(WDTCTRL) & WDT_ZERO) {
> > superio_outb(0x00, WDTCTRL);
> > @@ -353,7 +354,6 @@ static int wdt_get_status(int *status)
> > }
> >
> > superio_exit();
> > - spin_unlock_irqrestore(&spinlock, flags);
> > }
> > if (test_and_clear_bit(WDTS_KEEPALIVE, &wdt_status))
> > *status |= WDIOF_KEEPALIVEPING;
> > @@ -381,7 +381,12 @@ static int wdt_open(struct inode *inode, struct file *file)
> > if (!test_and_set_bit(WDTS_TIMER_RUN, &wdt_status)) {
> > if (nowayout && !test_and_set_bit(WDTS_LOCKED, &wdt_status))
> > __module_get(THIS_MODULE);
> > - wdt_start();
> > + if (wdt_start()) {
> > + clear_bit(WDTS_LOCKED, &wdt_status);
> > + clear_bit(WDTS_TIMER_RUN, &wdt_status);
> > + clear_bit(WDTS_DEV_OPEN, &wdt_status);
> > + return -EBUSY;
> > + }
> > }
> > return nonseekable_open(inode, file);
> > }
> > @@ -403,7 +408,15 @@ static int wdt_release(struct inode *inode, struct file *file)
> > {
> > if (test_bit(WDTS_TIMER_RUN, &wdt_status)) {
> > if (test_and_clear_bit(WDTS_EXPECTED, &wdt_status)) {
> > - wdt_stop();
> > + if (wdt_stop()) {
> > + /*
> > + * Stop failed. Just keep the watchdog alive
> > + * and hope nothing bad happens.
> > + */
> > + set_bit(WDTS_EXPECTED, &wdt_status);
> > + wdt_keepalive();
> > + return -EBUSY;
> > + }
> > clear_bit(WDTS_TIMER_RUN, &wdt_status);
> > } else {
> > wdt_keepalive();
> > @@ -484,7 +497,8 @@ static long wdt_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > &ident, sizeof(ident)) ? -EFAULT : 0;
> >
> > case WDIOC_GETSTATUS:
> > - wdt_get_status(&status);
> > + if (wdt_get_status(&status))
> > + return -EBUSY;
> > return put_user(status, uarg.i);
> >
> > case WDIOC_GETBOOTSTATUS:
> > @@ -501,13 +515,19 @@ static long wdt_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > switch (new_options) {
> > case WDIOS_DISABLECARD:
> > if (test_bit(WDTS_TIMER_RUN, &wdt_status))
> > - wdt_stop();
> > + if (wdt_stop())
> > + return -EBUSY;
> > +
> > clear_bit(WDTS_TIMER_RUN, &wdt_status);
> > return 0;
> >
> > case WDIOS_ENABLECARD:
> > - if (!test_and_set_bit(WDTS_TIMER_RUN, &wdt_status))
> > - wdt_start();
> > + if (!test_and_set_bit(WDTS_TIMER_RUN, &wdt_status)) {
> > + if (wdt_start()) {
> > + clear_bit(WDTS_TIMER_RUN, &wdt_status);
> > + return -EBUSY;
> > + }
> > + }
> > return 0;
> >
> > default:
> > @@ -532,7 +552,8 @@ static int wdt_notify_sys(struct notifier_block *this, unsigned long code,
> > void *unused)
> > {
> > if (code == SYS_DOWN || code == SYS_HALT)
> > - wdt_stop();
> > + if (wdt_stop())
> > + return -EBUSY;
>
> -EBUSY is not a valid return code here.
>
> > return NOTIFY_DONE;
> > }
> >
> > @@ -560,16 +581,15 @@ static int __init it87_wdt_init(void)
> > int rc = 0;
> > int try_gameport = !nogameport;
> > u8 chip_rev;
> > - unsigned long flags;
> >
> > wdt_status = 0;
> >
> > - spin_lock_irqsave(&spinlock, flags);
> > - superio_enter();
> > + if (superio_enter())
> > + return -EBUSY;
> > +
> > chip_type = superio_inw(CHIPID);
> > chip_rev = superio_inb(CHIPREV) & 0x0f;
> > superio_exit();
> > - spin_unlock_irqrestore(&spinlock, flags);
> >
> > switch (chip_type) {
> > case IT8702_ID:
> > @@ -603,8 +623,8 @@ static int __init it87_wdt_init(void)
> > return -ENODEV;
> > }
> >
> > - spin_lock_irqsave(&spinlock, flags);
> > - superio_enter();
> > + if (superio_enter())
> > + return -EBUSY;
> >
> > superio_select(GPIO);
> > superio_outb(WDT_TOV1, WDTCFG);
> > @@ -621,14 +641,12 @@ static int __init it87_wdt_init(void)
> > gpact = superio_inb(ACTREG);
> > superio_outb(0x01, ACTREG);
> > superio_exit();
> > - spin_unlock_irqrestore(&spinlock, flags);
> > if (request_region(base, 1, WATCHDOG_NAME))
> > set_bit(WDTS_USE_GP, &wdt_status);
> > else
> > rc = -EIO;
> > } else {
> > superio_exit();
> > - spin_unlock_irqrestore(&spinlock, flags);
> > }
> >
> > /* If we haven't Gameport support, try to get CIR support */
> > @@ -646,8 +664,8 @@ static int __init it87_wdt_init(void)
> > goto err_out;
> > }
> > base = CIR_BASE;
> > - spin_lock_irqsave(&spinlock, flags);
> > - superio_enter();
> > + if (superio_enter())
> > + return -EBUSY;
> >
> > superio_select(CIR);
> > superio_outw(base, BASEREG);
> > @@ -660,7 +678,6 @@ static int __init it87_wdt_init(void)
> > }
> >
> > superio_exit();
> > - spin_unlock_irqrestore(&spinlock, flags);
> > }
> >
> > if (timeout < 1 || timeout > max_units * 60) {
> > @@ -711,21 +728,19 @@ err_out_reboot:
> > err_out_region:
> > release_region(base, test_bit(WDTS_USE_GP, &wdt_status) ? 1 : 8);
> > if (!test_bit(WDTS_USE_GP, &wdt_status)) {
> > - spin_lock_irqsave(&spinlock, flags);
> > - superio_enter();
> > + if (superio_enter())
> > + return -EBUSY;
> > superio_select(CIR);
> > superio_outb(ciract, ACTREG);
> > superio_exit();
> > - spin_unlock_irqrestore(&spinlock, flags);
> > }
> > err_out:
> > if (try_gameport) {
> > - spin_lock_irqsave(&spinlock, flags);
> > - superio_enter();
> > + if (superio_enter())
> > + return -EBUSY;
> > superio_select(GAMEPORT);
> > superio_outb(gpact, ACTREG);
> > superio_exit();
> > - spin_unlock_irqrestore(&spinlock, flags);
> > }
> >
> > return rc;
> > @@ -733,11 +748,9 @@ err_out:
> >
> > static void __exit it87_wdt_exit(void)
> > {
> > - unsigned long flags;
> > - int nolock;
> > + if (superio_enter())
> > + return;
>
> Assumption is that you unregister the device. So even if superio_select fails,
> you still have to execute the rest of the code.
>
> >
> > - nolock = !spin_trylock_irqsave(&spinlock, flags);
> > - superio_enter();
> > superio_select(GPIO);
> > superio_outb(0x00, WDTCTRL);
> > superio_outb(0x00, WDTCFG);
> > @@ -752,8 +765,6 @@ static void __exit it87_wdt_exit(void)
> > superio_outb(ciract, ACTREG);
> > }
> > superio_exit();
> > - if (!nolock)
> > - spin_unlock_irqrestore(&spinlock, flags);
> >
> > misc_deregister(&wdt_miscdev);
> > unregister_reboot_notifier(&wdt_notifier);
> > --
> > 1.7.3.1
> >
>
> --
> Guenter Roeck
> Distinguished Engineer
> PDU IP Systems

--
Regards
Nat Gurumoorthy AB6SJ
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Steven Rostedt: "Re: [PATCH] linux/string.h: Introduce streq macro."
Previous message: Anca Emanuel: "Re: [Linaro-mm-sig] [RFC] ARM DMA mapping TODO, v1"
Next in thread: Guenter Roeck: "Re: [PATCH v7 1/2] Use "request_muxed_region" in it87 watchdogdrivers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]