[KJ] [PATCH]

[KJ] [PATCH]

[Patrik Kullman]

This is my first try to submit a patch here, so go gentle on me.. ;)
At least patch-tester@coderock.org thought it seemed ok..

This little patch moves from pci_find_device() in alim1535_wdt.c and alim7101_wdt.c to pci_get_device()/pci_dev_put()
I also found that alim1535.c uses 0x1535/0x7101 instead of the PCI_DEVICE_ID_AL_* defines.

I compared my usage of pci_dev_put() with other drivers and I hope that I've understood it right.

Signed-off-by: Patrik Kullman <patrik@yes.nu>
--- linux-2.6.17/drivers/char/watchdog/alim1535_wdt.c	2006-06-18 03:49:35.000000000 +0200
+++ linux/drivers/char/watchdog/alim1535_wdt.c	2006-07-28 07:15:00.000000000 +0200
@@ -312,7 +312,7 @@
  */
 
 static struct pci_device_id ali_pci_tbl[] = {
-	{ PCI_VENDOR_ID_AL, 0x1535, PCI_ANY_ID, PCI_ANY_ID,},
+	{ PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1535, PCI_ANY_ID, PCI_ANY_ID,},
 	{ 0, },
 };
 MODULE_DEVICE_TABLE(pci, ali_pci_tbl);
@@ -330,17 +330,25 @@
 	u32 wdog;
 
 	/* Check for a 1535 series bridge */
-	pdev = pci_find_device(PCI_VENDOR_ID_AL, 0x1535, NULL);
-	if(pdev = NULL)
+	pdev = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1535, NULL);
+	if(!pdev) {
+		pci_dev_put(pdev);
 		return -ENODEV;
+	} else {
+		pci_dev_put(pdev);
+	}
 
 	/* Check for the a 7101 PMU */
-	pdev = pci_find_device(PCI_VENDOR_ID_AL, 0x7101, NULL);
-	if(pdev = NULL)
+	pdev = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M7101, NULL);
+	if(!pdev) {
+		pci_dev_put(pdev);
 		return -ENODEV;
+	}
 
-	if(pci_enable_device(pdev))
+	if(pci_enable_device(pdev)) {
+		pci_dev_put(pdev);
 		return -EIO;
+	}
 
 	ali_pci = pdev;
 
@@ -447,6 +455,7 @@
 	/* Deregister */
 	unregister_reboot_notifier(&ali_notifier);
 	misc_deregister(&ali_miscdev);
+	pci_dev_put(ali_pci);
 }
 
 module_init(watchdog_init);
--- linux-2.6.17/drivers/char/watchdog/alim7101_wdt.c	2006-06-18 03:49:35.000000000 +0200
+++ linux/drivers/char/watchdog/alim7101_wdt.c	2006-07-28 18:19:52.000000000 +0200
@@ -332,6 +332,7 @@
 	wdt_turnoff();
 	/* Deregister */
 	misc_deregister(&wdt_miscdev);
+	pci_dev_put(alim7101_pmu);
 	unregister_reboot_notifier(&wdt_notifier);
 }
 
@@ -342,17 +343,20 @@
 	char tmp;
 
 	printk(KERN_INFO PFX "Steve Hill <steve@navaho.co.uk>.\n");
-	alim7101_pmu = pci_find_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M7101,NULL);
+	alim7101_pmu = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M7101,NULL);
 	if (!alim7101_pmu) {
 		printk(KERN_INFO PFX "ALi M7101 PMU not present - WDT not set\n");
+		pci_dev_put(alim7101_pmu);
 		return -EBUSY;
 	}
 
 	/* Set the WDT in the PMU to 1 second */
 	pci_write_config_byte(alim7101_pmu, ALI_7101_WDT, 0x02);
 
-	ali1543_south = pci_find_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1533, NULL);
+	ali1543_south = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1533, NULL);
 	if (!ali1543_south) {
+		pci_dev_put(ali1543_south);
+		pci_dev_put(alim7101_pmu);
 		printk(KERN_INFO PFX "ALi 1543 South-Bridge not present - WDT not set\n");
 		return -EBUSY;
 	}
@@ -360,11 +364,15 @@
 	if ((tmp & 0x1e) = 0x00) {
 		if (!use_gpio) {
 			printk(KERN_INFO PFX "Detected old alim7101 revision 'a1d'.  If this is a cobalt board, set the 'use_gpio' module parameter.\n");
+			pci_dev_put(ali1543_south);
+			pci_dev_put(alim7101_pmu);
 			return -EBUSY;
 		} 
 		nowayout = 1;
 	} else if ((tmp & 0x1e) != 0x12 && (tmp & 0x1e) != 0x00) {
 		printk(KERN_INFO PFX "ALi 1543 South-Bridge does not have the correct revision number (???1001?) - WDT not set\n");
+		pci_dev_put(ali1543_south);
+		pci_dev_put(alim7101_pmu);
 		return -EBUSY;
 	}
 
@@ -397,6 +405,7 @@
 		__module_get(THIS_MODULE);
 	}
 
+	pci_dev_put(ali1543_south);
 	printk(KERN_INFO PFX "WDT driver for ALi M7101 initialised. timeout=%d sec (nowayout=%d)\n",
 		timeout, nowayout);
 	return 0;
@@ -404,6 +413,8 @@
 err_out_miscdev:
 	misc_deregister(&wdt_miscdev);
 err_out:
+	pci_dev_put(alim7101_pmu);
+	pci_dev_put(ali1543_south);
 	return rc;
 }
 


_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH]

[Domen Puncer]

On 28/07/06 18:31 +0200, Patrik Kullman wrote:
> This is my first try to submit a patch here, so go gentle on me.. ;)
> At least patch-tester@coderock.org thought it seemed ok..
> 
> This little patch moves from pci_find_device() in alim1535_wdt.c and alim7101_wdt.c to pci_get_device()/pci_dev_put()
> I also found that alim1535.c uses 0x1535/0x7101 instead of the PCI_DEVICE_ID_AL_* defines.
> 
> I compared my usage of pci_dev_put() with other drivers and I hope that I've understood it right.
> 
> Signed-off-by: Patrik Kullman <patrik@yes.nu>
> --- linux-2.6.17/drivers/char/watchdog/alim1535_wdt.c	2006-06-18 03:49:35.000000000 +0200
> +++ linux/drivers/char/watchdog/alim1535_wdt.c	2006-07-28 07:15:00.000000000 +0200
> @@ -312,7 +312,7 @@
>   */
>  
>  static struct pci_device_id ali_pci_tbl[] = {
> -	{ PCI_VENDOR_ID_AL, 0x1535, PCI_ANY_ID, PCI_ANY_ID,},
> +	{ PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1535, PCI_ANY_ID, PCI_ANY_ID,},
>  	{ 0, },
>  };
>  MODULE_DEVICE_TABLE(pci, ali_pci_tbl);
> @@ -330,17 +330,25 @@
>  	u32 wdog;
>  
>  	/* Check for a 1535 series bridge */
> -	pdev = pci_find_device(PCI_VENDOR_ID_AL, 0x1535, NULL);
> -	if(pdev = NULL)
> +	pdev = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1535, NULL);
> +	if(!pdev) {
> +		pci_dev_put(pdev);

pci_dev_put(NULL)?

>  		return -ENODEV;
> +	} else {
> +		pci_dev_put(pdev);
> +	}
>  
>  	/* Check for the a 7101 PMU */
> -	pdev = pci_find_device(PCI_VENDOR_ID_AL, 0x7101, NULL);
> -	if(pdev = NULL)
> +	pdev = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M7101, NULL);

What if the previous _and_ this one succeed?

> +	if(!pdev) {
> +		pci_dev_put(pdev);
>  		return -ENODEV;
> +	}
>  
> -	if(pci_enable_device(pdev))
> +	if(pci_enable_device(pdev)) {
> +		pci_dev_put(pdev);
>  		return -EIO;
> +	}
>  
>  	ali_pci = pdev;
>  
> @@ -447,6 +455,7 @@
>  	/* Deregister */
>  	unregister_reboot_notifier(&ali_notifier);
>  	misc_deregister(&ali_miscdev);
> +	pci_dev_put(ali_pci);
>  }
>  
>  module_init(watchdog_init);
> --- linux-2.6.17/drivers/char/watchdog/alim7101_wdt.c	2006-06-18 03:49:35.000000000 +0200
> +++ linux/drivers/char/watchdog/alim7101_wdt.c	2006-07-28 18:19:52.000000000 +0200
> @@ -332,6 +332,7 @@
>  	wdt_turnoff();
>  	/* Deregister */
>  	misc_deregister(&wdt_miscdev);
> +	pci_dev_put(alim7101_pmu);
>  	unregister_reboot_notifier(&wdt_notifier);
>  }
>  
> @@ -342,17 +343,20 @@
>  	char tmp;
>  
>  	printk(KERN_INFO PFX "Steve Hill <steve@navaho.co.uk>.\n");
> -	alim7101_pmu = pci_find_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M7101,NULL);
> +	alim7101_pmu = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M7101,NULL);
>  	if (!alim7101_pmu) {
>  		printk(KERN_INFO PFX "ALi M7101 PMU not present - WDT not set\n");
> +		pci_dev_put(alim7101_pmu);
>  		return -EBUSY;
>  	}
>  
>  	/* Set the WDT in the PMU to 1 second */
>  	pci_write_config_byte(alim7101_pmu, ALI_7101_WDT, 0x02);
>  
> -	ali1543_south = pci_find_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1533, NULL);
> +	ali1543_south = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1533, NULL);
>  	if (!ali1543_south) {
> +		pci_dev_put(ali1543_south);
pci_dev_put(NULL)

> +		pci_dev_put(alim7101_pmu);

Goto err_out (or something appropriate).

>  		printk(KERN_INFO PFX "ALi 1543 South-Bridge not present - WDT not set\n");
>  		return -EBUSY;
>  	}
> @@ -360,11 +364,15 @@
>  	if ((tmp & 0x1e) = 0x00) {
>  		if (!use_gpio) {
>  			printk(KERN_INFO PFX "Detected old alim7101 revision 'a1d'.  If this is a cobalt board, set the 'use_gpio' module parameter.\n");
> +			pci_dev_put(ali1543_south);
> +			pci_dev_put(alim7101_pmu);

ditto

>  			return -EBUSY;
>  		} 
>  		nowayout = 1;
>  	} else if ((tmp & 0x1e) != 0x12 && (tmp & 0x1e) != 0x00) {
>  		printk(KERN_INFO PFX "ALi 1543 South-Bridge does not have the correct revision number (???1001?) - WDT not set\n");
> +		pci_dev_put(ali1543_south);
> +		pci_dev_put(alim7101_pmu);

ditto

>  		return -EBUSY;
>  	}
>  
> @@ -397,6 +405,7 @@
>  		__module_get(THIS_MODULE);
>  	}
>  
> +	pci_dev_put(ali1543_south);
>  	printk(KERN_INFO PFX "WDT driver for ALi M7101 initialised. timeout=%d sec (nowayout=%d)\n",
>  		timeout, nowayout);
>  	return 0;
> @@ -404,6 +413,8 @@
>  err_out_miscdev:
>  	misc_deregister(&wdt_miscdev);
>  err_out:
> +	pci_dev_put(alim7101_pmu);
> +	pci_dev_put(ali1543_south);
>  	return rc;
>  }
>  
> 
> 
> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/kernel-janitors
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH]

[Domen Puncer]

On 31/07/06 21:51 +0200, Patrik Kullman wrote:
> > >  		return -ENODEV;
> > > +	} else {
> > > +		pci_dev_put(pdev);
> > > +	}
> > >  
> > >  	/* Check for the a 7101 PMU */
> > > -	pdev = pci_find_device(PCI_VENDOR_ID_AL, 0x7101, NULL);
> > > -	if(pdev = NULL)
> > > +	pdev = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M7101, NULL);
> > 
> > What if the previous _and_ this one succeed?
> 
> Since the original developer chose to just search for the 1535 and then
> overwrite the variable with the 7101 (that will be used), I thought I'd
> clear the memory of the first device before assigning the new device to
> it.

Guess I misunderstood the meaning.
Still... you only pci_dev_put one device on exit.

btw. nice to know patch-tester still works :-)


	Domen
> 
> I guess I shouldn't have.
> 
> Will fix.
> 
> 
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] [PATCH] drivers/char/riscom8.c: save_flags()/cli()/sti()

[Amol Lad]

Removed save_flags()/cli()/sti() and used (light weight) spin locks

Signed-off-by: Amol Lad <amol@verismonetworks.com>
---
--- linux-2.6.19-rc1-orig/drivers/char/riscom8.c	2006-10-05 14:00:43.000000000 +0530
+++ linux-2.6.19-rc1/drivers/char/riscom8.c	2006-10-13 17:06:44.000000000 +0530
@@ -81,6 +81,7 @@
 
 static struct riscom_board * IRQ_to_board[16];
 static struct tty_driver *riscom_driver;
+spinlock_t riscom_lock = SPIN_LOCK_UNLOCKED;
 
 static unsigned long baud_table[] =  {
 	0, 50, 75, 110, 134, 150, 200, 300, 600, 1200, 1800, 2400, 4800,
@@ -231,13 +232,13 @@ static void __init rc_init_CD180(struct 
 {
 	unsigned long flags;
 	
-	save_flags(flags); cli();
+	spin_lock_irqsave(&riscom_lock, flags);
 	rc_out(bp, RC_CTOUT, 0);     	           /* Clear timeout             */
 	rc_wait_CCR(bp);			   /* Wait for CCR ready        */
 	rc_out(bp, CD180_CCR, CCR_HARDRESET);      /* Reset CD180 chip          */
-	sti();
+	spin_unlock_irq(&riscom_lock);
 	rc_long_delay(HZ/20);                      /* Delay 0.05 sec            */
-	cli();
+	spin_lock_irq(&riscom_lock);
 	rc_out(bp, CD180_GIVR, RC_ID);             /* Set ID for this chip      */
 	rc_out(bp, CD180_GICR, 0);                 /* Clear all bits            */
 	rc_out(bp, CD180_PILR1, RC_ACK_MINT);      /* Prio for modem intr       */
@@ -248,7 +249,7 @@ static void __init rc_init_CD180(struct 
 	rc_out(bp, CD180_PPRH, (RC_OSCFREQ/(1000000/RISCOM_TPS)) >> 8);
 	rc_out(bp, CD180_PPRL, (RC_OSCFREQ/(1000000/RISCOM_TPS)) & 0xff);
 	
-	restore_flags(flags);
+	spin_unlock_irqrestore(&riscom_lock, flags);
 }
 
 /* Main probing routine, also sets irq. */
@@ -832,7 +833,7 @@ static int rc_setup_port(struct riscom_b
 		port->xmit_buf = (unsigned char *) tmp;
 	}
 		
-	save_flags(flags); cli();
+	spin_lock_irqsave(&riscom_lock, flags);
 		
 	if (port->tty) 
 		clear_bit(TTY_IO_ERROR, &port->tty->flags);
@@ -844,7 +845,7 @@ static int rc_setup_port(struct riscom_b
 	rc_change_speed(bp, port);
 	port->flags |= ASYNC_INITIALIZED;
 		
-	restore_flags(flags);
+	spin_unlock_irqrestore(&riscom_lock, flags);
 	return 0;
 }
 
@@ -955,19 +956,19 @@ static int block_til_ready(struct tty_st
 	 */
 	retval = 0;
 	add_wait_queue(&port->open_wait, &wait);
-	cli();
+	spin_lock_irq(&riscom_lock);
 	if (!tty_hung_up_p(filp))
 		port->count--;
-	sti();
+	spin_unlock_irq(&riscom_lock);
 	port->blocked_open++;
 	while (1) {
-		cli();
+		spin_lock_irq(&riscom_lock);
 		rc_out(bp, CD180_CAR, port_No(port));
 		CD = rc_in(bp, CD180_MSVR) & MSVR_CD;
 		rc_out(bp, CD180_MSVR, MSVR_RTS);
 		bp->DTR &= ~(1u << port_No(port));
 		rc_out(bp, RC_DTR, bp->DTR);
-		sti();
+		spin_unlock_irq(&riscom_lock);
 		set_current_state(TASK_INTERRUPTIBLE);
 		if (tty_hung_up_p(filp) ||
 		    !(port->flags & ASYNC_INITIALIZED)) {
@@ -1040,7 +1041,7 @@ static void rc_close(struct tty_struct *
 	if (!port || rc_paranoia_check(port, tty->name, "close"))
 		return;
 	
-	save_flags(flags); cli();
+	spin_lock_irqsave(&riscom_lock, flags);
 	if (tty_hung_up_p(filp))
 		goto out;
 	
@@ -1107,7 +1108,8 @@ static void rc_close(struct tty_struct *
 	}
 	port->flags &= ~(ASYNC_NORMAL_ACTIVE|ASYNC_CLOSING);
 	wake_up_interruptible(&port->close_wait);
-out:	restore_flags(flags);
+out:	
+	spin_unlock_irqrestore(&riscom_lock, flags);
 }
 
 static int rc_write(struct tty_struct * tty, 
@@ -1126,34 +1128,34 @@ static int rc_write(struct tty_struct * 
 	if (!tty || !port->xmit_buf)
 		return 0;
 
-	save_flags(flags);
+	local_save_flags(flags);
 	while (1) {
-		cli();		
+		spin_lock_irq(&riscom_lock);
 		c = min_t(int, count, min(SERIAL_XMIT_SIZE - port->xmit_cnt - 1,
 					  SERIAL_XMIT_SIZE - port->xmit_head));
 		if (c <= 0) {
-			restore_flags(flags);
+			spin_unlock_irqrestore(&riscom_lock, flags);
 			break;
 		}
 
 		memcpy(port->xmit_buf + port->xmit_head, buf, c);
 		port->xmit_head = (port->xmit_head + c) & (SERIAL_XMIT_SIZE-1);
 		port->xmit_cnt += c;
-		restore_flags(flags);
+		spin_unlock_irqrestore(&riscom_lock, flags);
 
 		buf += c;
 		count -= c;
 		total += c;
 	}
 
-	cli();
+	spin_lock_irq(&riscom_lock);
 	if (port->xmit_cnt && !tty->stopped && !tty->hw_stopped &&
 	    !(port->IER & IER_TXRDY)) {
 		port->IER |= IER_TXRDY;
 		rc_out(bp, CD180_CAR, port_No(port));
 		rc_out(bp, CD180_IER, port->IER);
 	}
-	restore_flags(flags);
+	spin_unlock_irqrestore(&riscom_lock, flags);
 
 	return total;
 }
@@ -1169,7 +1171,7 @@ static void rc_put_char(struct tty_struc
 	if (!tty || !port->xmit_buf)
 		return;
 
-	save_flags(flags); cli();
+	spin_lock_irqsave(&riscom_lock, flags);
 	
 	if (port->xmit_cnt >= SERIAL_XMIT_SIZE - 1)
 		goto out;
@@ -1177,7 +1179,8 @@ static void rc_put_char(struct tty_struc
 	port->xmit_buf[port->xmit_head++] = ch;
 	port->xmit_head &= SERIAL_XMIT_SIZE - 1;
 	port->xmit_cnt++;
-out:	restore_flags(flags);
+out:	
+	spin_unlock_irqrestore(&riscom_lock, flags);
 }
 
 static void rc_flush_chars(struct tty_struct * tty)
@@ -1192,11 +1195,11 @@ static void rc_flush_chars(struct tty_st
 	    !port->xmit_buf)
 		return;
 
-	save_flags(flags); cli();
+	spin_lock_irqsave(&riscom_lock, flags);
 	port->IER |= IER_TXRDY;
 	rc_out(port_Board(port), CD180_CAR, port_No(port));
 	rc_out(port_Board(port), CD180_IER, port->IER);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&riscom_lock, flags);
 }
 
 static int rc_write_room(struct tty_struct * tty)
@@ -1231,9 +1234,9 @@ static void rc_flush_buffer(struct tty_s
 	if (rc_paranoia_check(port, tty->name, "rc_flush_buffer"))
 		return;
 
-	save_flags(flags); cli();
+	spin_lock_irqsave(&riscom_lock, flags);
 	port->xmit_cnt = port->xmit_head = port->xmit_tail = 0;
-	restore_flags(flags);
+	spin_unlock_irqrestore(&riscom_lock, flags);
 	
 	wake_up_interruptible(&tty->write_wait);
 	tty_wakeup(tty);
@@ -1251,11 +1254,11 @@ static int rc_tiocmget(struct tty_struct
 		return -ENODEV;
 
 	bp = port_Board(port);
-	save_flags(flags); cli();
+	spin_lock_irqsave(&riscom_lock, flags);
 	rc_out(bp, CD180_CAR, port_No(port));
 	status = rc_in(bp, CD180_MSVR);
 	result = rc_in(bp, RC_RI) & (1u << port_No(port)) ? 0 : TIOCM_RNG;
-	restore_flags(flags);
+	spin_unlock_irqrestore(&riscom_lock, flags);
 	result |= ((status & MSVR_RTS) ? TIOCM_RTS : 0)
 		| ((status & MSVR_DTR) ? TIOCM_DTR : 0)
 		| ((status & MSVR_CD)  ? TIOCM_CAR : 0)
@@ -1276,7 +1279,7 @@ static int rc_tiocmset(struct tty_struct
 
 	bp = port_Board(port);
 
-	save_flags(flags); cli();
+	spin_lock_irqsave(&riscom_lock, flags);
 	if (set & TIOCM_RTS)
 		port->MSVR |= MSVR_RTS;
 	if (set & TIOCM_DTR)
@@ -1290,7 +1293,7 @@ static int rc_tiocmset(struct tty_struct
 	rc_out(bp, CD180_CAR, port_No(port));
 	rc_out(bp, CD180_MSVR, port->MSVR);
 	rc_out(bp, RC_DTR, bp->DTR);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&riscom_lock, flags);
 	return 0;
 }
 
@@ -1299,7 +1302,7 @@ static inline void rc_send_break(struct 
 	struct riscom_board *bp = port_Board(port);
 	unsigned long flags;
 	
-	save_flags(flags); cli();
+	spin_lock_irqsave(&riscom_lock, flags);
 	port->break_length = RISCOM_TPS / HZ * length;
 	port->COR2 |= COR2_ETC;
 	port->IER  |= IER_TXRDY;
@@ -1309,7 +1312,7 @@ static inline void rc_send_break(struct 
 	rc_wait_CCR(bp);
 	rc_out(bp, CD180_CCR, CCR_CORCHG2);
 	rc_wait_CCR(bp);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&riscom_lock, flags);
 }
 
 static inline int rc_set_serial_info(struct riscom_port * port,
@@ -1352,9 +1355,9 @@ static inline int rc_set_serial_info(str
 		port->closing_wait = tmp.closing_wait;
 	}
 	if (change_speed)  {
-		save_flags(flags); cli();
+		spin_lock_irqsave(&riscom_lock, flags);
 		rc_change_speed(bp, port);
-		restore_flags(flags);
+		spin_unlock_irqrestore(&riscom_lock, flags);
 	}
 	return 0;
 }
@@ -1435,7 +1438,7 @@ static void rc_throttle(struct tty_struc
 	
 	bp = port_Board(port);
 	
-	save_flags(flags); cli();
+	spin_lock_irqsave(&riscom_lock, flags);
 	port->MSVR &= ~MSVR_RTS;
 	rc_out(bp, CD180_CAR, port_No(port));
 	if (I_IXOFF(tty))  {
@@ -1444,7 +1447,7 @@ static void rc_throttle(struct tty_struc
 		rc_wait_CCR(bp);
 	}
 	rc_out(bp, CD180_MSVR, port->MSVR);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&riscom_lock, flags);
 }
 
 static void rc_unthrottle(struct tty_struct * tty)
@@ -1458,7 +1461,7 @@ static void rc_unthrottle(struct tty_str
 	
 	bp = port_Board(port);
 	
-	save_flags(flags); cli();
+	spin_lock_irqsave(&riscom_lock, flags);
 	port->MSVR |= MSVR_RTS;
 	rc_out(bp, CD180_CAR, port_No(port));
 	if (I_IXOFF(tty))  {
@@ -1467,7 +1470,7 @@ static void rc_unthrottle(struct tty_str
 		rc_wait_CCR(bp);
 	}
 	rc_out(bp, CD180_MSVR, port->MSVR);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&riscom_lock, flags);
 }
 
 static void rc_stop(struct tty_struct * tty)
@@ -1481,11 +1484,11 @@ static void rc_stop(struct tty_struct * 
 	
 	bp = port_Board(port);
 	
-	save_flags(flags); cli();
+	spin_lock_irqsave(&riscom_lock, flags);
 	port->IER &= ~IER_TXRDY;
 	rc_out(bp, CD180_CAR, port_No(port));
 	rc_out(bp, CD180_IER, port->IER);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&riscom_lock, flags);
 }
 
 static void rc_start(struct tty_struct * tty)
@@ -1499,13 +1502,13 @@ static void rc_start(struct tty_struct *
 	
 	bp = port_Board(port);
 	
-	save_flags(flags); cli();
+	spin_lock_irqsave(&riscom_lock, flags);
 	if (port->xmit_cnt && port->xmit_buf && !(port->IER & IER_TXRDY))  {
 		port->IER |= IER_TXRDY;
 		rc_out(bp, CD180_CAR, port_No(port));
 		rc_out(bp, CD180_IER, port->IER);
 	}
-	restore_flags(flags);
+	spin_unlock_irqrestore(&riscom_lock, flags);
 }
 
 /*
@@ -1557,9 +1560,9 @@ static void rc_set_termios(struct tty_st
 	    tty->termios->c_iflag = old_termios->c_iflag)
 		return;
 
-	save_flags(flags); cli();
+	spin_lock_irqsave(&riscom_lock, flags);
 	rc_change_speed(port_Board(port), port);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&riscom_lock, flags);
 
 	if ((old_termios->c_cflag & CRTSCTS) &&
 	    !(tty->termios->c_cflag & CRTSCTS)) {
@@ -1648,11 +1651,10 @@ static void rc_release_drivers(void)
 {
 	unsigned long flags;
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&riscom_lock, flags);
 	tty_unregister_driver(riscom_driver);
 	put_tty_driver(riscom_driver);
-	restore_flags(flags);
+	spin_unlock_irqrestore(&riscom_lock, flags);
 }
 
 #ifndef MODULE


_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] drivers/char/riscom8.c:

[Alan Cox]

Ar Gwe, 2006-10-13 am 17:10 +0530, ysgrifennodd Amol Lad:
> Removed save_flags()/cli()/sti() and used (light weight) spin locks
> 

Three things I see that look problematic

1. The irq locking isn't doing anything as the IRQ handler itself is not
taking the lock
2. If the irq handler itself dumbly locks to fix this then we get
tty_flip_buffer_push() re-entering the other code paths and deadlocking
if low latency is enabled
3. Some of the use of local_save/spin_lock_irq seems over-clever and
unneeded

Fixable but how about we just delete the file since it has been broken
for ages and nobody can be using it ?

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] drivers/char/riscom8.c:

[Matthew Wilcox]

On Sat, Oct 14, 2006 at 03:20:02PM +0100, Alan Cox wrote:
> 1. The irq locking isn't doing anything as the IRQ handler itself is not
> taking the lock

Looks like you reviewed the first version instead of the version Amol
sent after Arjan critiqued that.

> 2. If the irq handler itself dumbly locks to fix this then we get
> tty_flip_buffer_push() re-entering the other code paths and deadlocking
> if low latency is enabled

Yep, still a problem with the revised patch.

> 3. Some of the use of local_save/spin_lock_irq seems over-clever and
> unneeded
> 
> Fixable but how about we just delete the file since it has been broken
> for ages and nobody can be using it ?

Only broken on SMP ...

I wouldn't mind writing a new driver (using the serial core) if someone
wants to send me one.  I need a multiport serial card anyway ...
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH]

[Alan Cox]

Ar Sad, 2006-10-14 am 11:49 -0600, ysgrifennodd Matthew Wilcox:
> Only broken on SMP ...
> 
> I wouldn't mind writing a new driver (using the serial core) if someone
> wants to send me one.  I need a multiport serial card anyway ...

You still have ISA bus ?

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

[KJ] [PATCH] drivers/serial/dz.c:

[Amol Lad]

Replaced save_flags()/cli()/restore_flags() pair with spin_lock
alternatives.

For this case, I believe spin lock plays no role but I also do not have
a better way.

Tested compile only.

Signed-off-by: Amol Lad <amol@verismonetworks.com>
---
--- linux-2.6.19-rc1-orig/drivers/serial/dz.c	2006-09-21 10:15:41.000000000 +0530
+++ linux-2.6.19-rc1/drivers/serial/dz.c	2006-10-16 12:48:31.000000000 +0530
@@ -778,13 +778,13 @@ int __init dz_init(void)
 {
 	unsigned long flags;
 	int ret, i;
+	spinlock_t dz_lock = SPIN_LOCK_UNLOCKED;
 
 	printk("%s%s\n", dz_name, dz_version);
 
 	dz_init_ports();
 
-	save_flags(flags);
-	cli();
+	spin_lock_irqsave(&dz_lock,flags);
 
 #ifndef CONFIG_SERIAL_DZ_CONSOLE
 	/* reset the chip */
@@ -794,7 +794,7 @@ int __init dz_init(void)
 	/* order matters here... the trick is that flags
 	   is updated... in request_irq - to immediatedly obliterate
 	   it is unwise. */
-	restore_flags(flags);
+	spin_unlock_irqrestore(&dz_lock,flags);
 
 	if (request_irq(dz_ports[0].port.irq, dz_interrupt,
 			IRQF_DISABLED, "DZ", &dz_ports[0]))


_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] drivers/serial/dz.c:

[Matthew Wilcox]

On Mon, Oct 16, 2006 at 12:58:52PM +0530, Amol Lad wrote:
> Replaced save_flags()/cli()/restore_flags() pair with spin_lock
> alternatives.
> 
> For this case, I believe spin lock plays no role but I also do not have
> a better way.

I think there's a better way.  Here's the full stretch covered by that:

        save_flags(flags);
        cli();

#ifndef CONFIG_SERIAL_DZ_CONSOLE
        /* reset the chip */
        dz_reset(&dz_ports[0]);
#endif

        /* order matters here... the trick is that flags
           is updated... in request_irq - to immediatedly obliterate
           it is unwise. */
        restore_flags(flags);

Now, we can obviously move the junk inside the ifdef:

#ifndef CONFIG_SERIAL_DZ_CONSOLE
        save_flags(flags);
        cli();

        /* reset the chip */
        dz_reset(&dz_ports[0]);

        restore_flags(flags);
#endif

Now, there's only one other place that dz_reset is called from, and to
be honest, it looks like it's missing some locking too.  Looking at the
other uses of spin_lock within this file, we can see that it's used to
protect the DZ_ ports.

So I think a better patch would look like this:

diff --git a/drivers/serial/dz.c b/drivers/serial/dz.c
index 8a98aae..de7a0b1 100644
--- a/drivers/serial/dz.c
+++ b/drivers/serial/dz.c
@@ -661,6 +661,8 @@ static void __init dz_init_ports(void)
 
 static void dz_reset(struct dz_port *dport)
 {
+       unsigned long flags;
+       spin_lock_irqsave((&dport->port.lock, flags);
        dz_out(dport, DZ_CSR, DZ_CLR);
 
        while (dz_in(dport, DZ_CSR) & DZ_CLR);
@@ -670,6 +672,7 @@ static void dz_reset(struct dz_port *dpo
 
        /* enable scanning */
        dz_out(dport, DZ_CSR, DZ_MSE);
+       spin_unlock_irqrestore((&dport->port.lock, flags);
 }
 
 #ifdef CONFIG_SERIAL_DZ_CONSOLE
@@ -783,19 +786,11 @@ int __init dz_init(void)
 
        dz_init_ports();
 
-       save_flags(flags);
-       cli();
-
 #ifndef CONFIG_SERIAL_DZ_CONSOLE
        /* reset the chip */
        dz_reset(&dz_ports[0]);
 #endif
 
-       /* order matters here... the trick is that flags
-          is updated... in request_irq - to immediatedly obliterate
-          it is unwise. */
-       restore_flags(flags);
-
        if (request_irq(dz_ports[0].port.irq, dz_interrupt,
                        IRQF_DISABLED, "DZ", &dz_ports[0]))
                panic("Unable to register DZ interrupt");

But looking at the driver, there's some places we're missing locking.

In dz_set_mctrl(), we read-modify-write DZ_TCR without holding a lock.
We also do that in dz_console_setup(), but I suspect we're guaranteed
by higher levels not to race with anything.

I suspect it can't hit us in practice (due to the dz driver being for
hardware that's UP only, but maybe with preemption, it could bite
us ...), but there's no locking in the interrupt handler.  I think
dz_transmit_chars() needs locking against dz_console_putchar(), for
example.

Anyway, that's enough to be going on with.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH]

[Amol Lad]

On Mon, 2006-10-16 at 07:23 -0600, Matthew Wilcox wrote:
> On Mon, Oct 16, 2006 at 12:58:52PM +0530, Amol Lad wrote:
> > Replaced save_flags()/cli()/restore_flags() pair with spin_lock
> > alternatives.
> > 
> > For this case, I believe spin lock plays no role but I also do not have
> > a better way.
> 
> I think there's a better way.  Here's the full stretch covered by that:
> 
>         save_flags(flags);
>         cli();
> 
> #ifndef CONFIG_SERIAL_DZ_CONSOLE
>         /* reset the chip */
>         dz_reset(&dz_ports[0]);
> #endif
> 
>         /* order matters here... the trick is that flags
>            is updated... in request_irq - to immediatedly obliterate
>            it is unwise. */
>         restore_flags(flags);
> 
> Now, we can obviously move the junk inside the ifdef:
> 
> #ifndef CONFIG_SERIAL_DZ_CONSOLE
>         save_flags(flags);
>         cli();
> 
>         /* reset the chip */
>         dz_reset(&dz_ports[0]);
> 
>         restore_flags(flags);
> #endif
> 
> Now, there's only one other place that dz_reset is called from, and to
> be honest, it looks like it's missing some locking too.  Looking at the
> other uses of spin_lock within this file, we can see that it's used to
> protect the DZ_ ports.
> 
> So I think a better patch would look like this:
> 
> diff --git a/drivers/serial/dz.c b/drivers/serial/dz.c
> index 8a98aae..de7a0b1 100644
> --- a/drivers/serial/dz.c
> +++ b/drivers/serial/dz.c
> @@ -661,6 +661,8 @@ static void __init dz_init_ports(void)
>  
>  static void dz_reset(struct dz_port *dport)
>  {
> +       unsigned long flags;
> +       spin_lock_irqsave((&dport->port.lock, flags);
>         dz_out(dport, DZ_CSR, DZ_CLR);
>  
>         while (dz_in(dport, DZ_CSR) & DZ_CLR);
> @@ -670,6 +672,7 @@ static void dz_reset(struct dz_port *dpo
>  
>         /* enable scanning */
>         dz_out(dport, DZ_CSR, DZ_MSE);
> +       spin_unlock_irqrestore((&dport->port.lock, flags);
>  }
>  
>  #ifdef CONFIG_SERIAL_DZ_CONSOLE
> @@ -783,19 +786,11 @@ int __init dz_init(void)
>  
>         dz_init_ports();
>  
> -       save_flags(flags);
> -       cli();
> -
>  #ifndef CONFIG_SERIAL_DZ_CONSOLE
>         /* reset the chip */
>         dz_reset(&dz_ports[0]);
>  #endif
>  
> -       /* order matters here... the trick is that flags
> -          is updated... in request_irq - to immediatedly obliterate
> -          it is unwise. */
> -       restore_flags(flags);
> -
>         if (request_irq(dz_ports[0].port.irq, dz_interrupt,
>                         IRQF_DISABLED, "DZ", &dz_ports[0]))
>                 panic("Unable to register DZ interrupt");
> 
> But looking at the driver, there's some places we're missing locking.
> 
> In dz_set_mctrl(), we read-modify-write DZ_TCR without holding a lock.
> We also do that in dz_console_setup(), but I suspect we're guaranteed
> by higher levels not to race with anything.
> 
> I suspect it can't hit us in practice (due to the dz driver being for
> hardware that's UP only, but maybe with preemption, it could bite
> us ...), but there's no locking in the interrupt handler.  I think
> dz_transmit_chars() needs locking against dz_console_putchar(), for
> example.
> 
> Anyway, that's enough to be going on with.

Thanks for that insight. Why not just include your this patch initally
and we add more locking later (as it will surely need run time testing)

If you think it's fine then please sign-off the patch.

Thanks again !

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors