--- linux-2.4.19-pre4-clean/drivers/char/indydog.c Tue Mar 5 15:19:28 2002
+++ linux/drivers/char/indydog.c Tue Mar 26 09:38:32 2002
@@ -24,7 +24,7 @@
#include <asm/uaccess.h>
#include <asm/sgi/sgimc.h>
-static int indydog_alive;
+static unsigned long indydog_alive;
static struct sgimc_misc_ctrl *mcmisc_regs;
static void indydog_ping()
@@ -41,7 +41,7 @@
{
u32 mc_ctrl0;
- if(indydog_alive)
+ if( test_and_set_bit(0,&indydog_alive) )
return -EBUSY;
#ifdef CONFIG_WATCHDOG_NOWAYOUT
MOD_INC_USE_COUNT;
@@ -55,7 +55,6 @@
mcmisc_regs->cpuctrl0 = mc_ctrl0;
indydog_ping();
- indydog_alive=1;
printk("Started watchdog timer.\n");
return 0;
}
@@ -66,7 +65,6 @@
* Shut off the timer.
* Lock it in if it's a module and we defined ...NOWAYOUT
*/
- lock_kernel();
#ifndef CONFIG_WATCHDOG_NOWAYOUT
{
u32 mc_ctrl0 = mcmisc_regs->cpuctrl0;
@@ -75,8 +73,7 @@
printk("Stopped watchdog timer.\n");
}
#endif
- indydog_alive=0;
- unlock_kernel();
+ clear_bit(0,&indydog_alive);
return 0;
}
On Tue, Mar 26, 2002 at 10:01:40AM -0800, Dave Hansen wrote:
> indydog_open has a race
>
> indydog_open(struct inode *inode, struct file *file)
> {
> if( indydog_alive )
> return -EBUSY;
> ...
> indydog_alive = 1;
> return 0;
> }
>
> If 2 opens happen simultaneously, there can be 2 tasks which both see
> indydog_alive as 0 and both set it to 1, successfully opening the
> device. Block and char devices hold the BKL before calling the driver's
> open function, but misc devices don't do the same.
As Dave and me discussed in private mail this can't actually happen on
non-preemtible kernels since IP22 is non-SMP(which is why I didn't
bother when I wrote it). Anyway, a race is a race, no excuses.
>
> The BKL is not needed in the release function, as far as I can see.
> Alan's softdog.c driver uses atomic ops without BKL to do the same thing
> as your driver. Patch to fix attached.
Tested and works on Mips/IP22. Thanks again Dave,
-- Guido