/drivers/char/serial.c cleanup

PATCH In review Andrey Panin 3 participants 4 messages Mar 5, 2001

lore

Share & tools

lore

Andrey Panin

author

2001-03-05 12:37 UTC #0 lore raw

PATCH 1/1 /drivers/char/serial.c cleanup

Hi all,

I already sent this patch some days ago, but didn't get an answer :( So, i'm trying to resubmit it.

Attached patch (2.4.2-ac11) makes some changes in serial driver:
adds ioremap() return code checks, removes panic() calls and adds better error handling in start_pci_pnp_board() function.

Best regards.

Signature · 2 lines

Andrey Panin            | Embedded systems software engineer
[email protected]        | PGP key: http://www.orbita1.ru/~pazke/AndreyPanin.asc

linux/drivers/char/serial.c renamed from linux.vanilla/drivers/char/serial.c +47 ~~-10~~

diff -u /linux.vanilla/drivers/char/serial.c /linux/drivers/char/serial.c
@@ -3876,7 +3876,10 @@
 		return 0;
 	}
 	req->io_type = SERIAL_IO_MEM;
-	req->iomem_base = ioremap(port, board->uart_offset);
+	if ((req->iomem_base = ioremap(port, board->uart_offset))) {
+		printk(KERN_ERR "serial: Couldn's remap IO memory at %#lx\n", port);
+		return 1;
+	}
 	req->iomem_reg_shift = board->reg_shift;
 	req->port = 0;
 	return 0;

@@ -3939,8 +3942,13 @@
 	 * shutdown the board on a module unload.
 	 */
 	if (DEACTIVATE_FUNC(dev) || board->init_fn) {
-		if (serial_pci_board_idx >= NR_PCI_BOARDS)
+		if (serial_pci_board_idx >= NR_PCI_BOARDS) {
+			if (board->init_fn)
+				(board->init_fn)(dev, board, 0);
+			if (board->flags & SPCI_FL_ISPNP)
+				(DEACTIVATE_FUNC(dev))(dev);
 			return;
+		}
 		serial_pci_board[serial_pci_board_idx].board = *board;
 		serial_pci_board[serial_pci_board_idx].dev = dev;
 		serial_pci_board_idx++;

@@ -3954,8 +3962,13 @@
 
 	for (k=0; k < board->num_ports; k++) {
 		serial_req.irq = get_pci_irq(dev, board, k);
-		if (get_pci_port(dev, board, &serial_req, k))
+		if (get_pci_port(dev, board, &serial_req, k)) {
+			if (board->init_fn)
+				(board->init_fn)(dev, board, 0);
+			if ((board->flags & SPCI_FL_ISPNP) && (k == 0))
+				(DEACTIVATE_FUNC(dev))(dev);
 			break;
+		}
 		serial_req.flags = ASYNC_SKIP_TEST | ASYNC_AUTOPROBE;
 #ifdef SERIAL_DEBUG_PCI
 		printk("Setup PCI/PNP port: port %x, irq %d, type %d\n",

@@ -4010,7 +4023,11 @@
 				      data | pci_config);
 	
 	/* enable/disable interrupts */
-	p = ioremap(pci_resource_start(dev, 0), 0x80);
+	if ((p = ioremap(pci_resource_start(dev, 0), 0x80))) {
+		printk( KERN_ERR "serial: Couldn't remap IO memory at %#lx\n",
+			pci_resource_start(dev, 0));
+		return 1;
+	}
 	writel(enable ? irq_config : 0x00, (unsigned long)p + 0x4c);
 	iounmap(p);

@@ -4053,7 +4070,11 @@
 
        if (!enable) return 0;
 
-       p = ioremap(pci_resource_start(dev, 0), 0x80);
+       if ((p = ioremap(pci_resource_start(dev, 0), 0x80))) {
+       		printk( KERN_ERR "serial: Couldn't remap IO memory at %#lx\n",
+			pci_resource_start(dev, 0));
+		return 1;
+       }
 
        switch (dev->device & 0xfff8) {
                case PCI_DEVICE_ID_SIIG_1S_10x:         /* 1S */

@@ -5215,6 +5236,16 @@
 /*
  * The serial driver boot-time initialization code!
  */
+static void __init rs_cleanup_after_failure(struct tty_driver *tty)
+{
+	unsigned long flags;
+	del_timer_sync(&serial_timer);
+	save_flags(flags); cli();
+	remove_bh(SERIAL_BH);
+	if (tty) tty_unregister_driver(tty);
+	restore_flags(flags);
+}
+
 static int __init rs_init(void)
 {
 	int i;

@@ -5315,11 +5346,17 @@
 	callout_driver.proc_entry = 0;
 #endif
 
-	if (tty_register_driver(&serial_driver))
-		panic("Couldn't register serial driver\n");
-	if (tty_register_driver(&callout_driver))
-		panic("Couldn't register callout driver\n");
-	
+	if ((i = tty_register_driver(&serial_driver))) {
+		printk(KERN_ERR "serial: Couldn't register serial driver\n");
+		rs_cleanup_after_failure(NULL);
+		return i;
+	}
+	if ((i = tty_register_driver(&callout_driver))) {
+		printk(KERN_ERR "serial: Couldn't register callout driver\n");
+		rs_cleanup_after_failure(&serial_driver);
+		return i;
+	}
+
 	for (i = 0, state = rs_table; i < NR_PORTS; i++,state++) {
 		state->magic = SSTATE_MAGIC;
 		state->line = i;

Philipp Rumpf

2001-03-05 12:49 UTC #1 lore raw

Quoted 3 lines · Andrey Panin

Attached patch (2.4.2-ac11) makes some changes in serial driver:
adds ioremap() return code checks, removes panic() calls and adds better error handling in start_pci_pnp_board() function.

Did you test it ?

linux/drivers/char/serial.c line 3882

Quoted 12 lines

diff -u /linux.vanilla/drivers/char/serial.c /linux/drivers/char/serial.c
linux/drivers/char/serial.c renamed from linux.vanilla/drivers/char/serial.c +4 -1
@@ -3876,7 +3876,10 @@
 		return 0;
 	}
 	req->io_type = SERIAL_IO_MEM;
-	req->iomem_base = ioremap(port, board->uart_offset);
+	if ((req->iomem_base = ioremap(port, board->uart_offset))) {
+		printk(KERN_ERR "serial: Couldn's remap IO memory at %#lx\n", port);
+		return 1;
+	}

This seems wrong. ioremap returns NULL in case of failure.

Andrey Panin

author

2001-03-05 13:00 UTC #2 lore raw

In reply to Andrey Panin

Quoted 6 lines · Philipp Rumpf

Quoted 3 lines · Andrey Panin
Attached patch (2.4.2-ac11) makes some changes in serial driver:
adds ioremap() return code checks, removes panic() calls and adds better error handling in start_pci_pnp_board() function.
Did you test it ?

Partially (without my ISAPNP modem, lack of ISA slots)

Quoted 15 lines

linux/drivers/char/serial.c line 3882
Quoted 13 lines
diff -u /linux.vanilla/drivers/char/serial.c /linux/drivers/char/serial.c
linux/drivers/char/serial.c renamed from linux.vanilla/drivers/char/serial.c +4 -1
@@ -3876,7 +3876,10 @@
 		return 0;
 	}
 	req->io_type = SERIAL_IO_MEM;
-	req->iomem_base = ioremap(port, board->uart_offset);
+	if ((req->iomem_base = ioremap(port, board->uart_offset))) {
+		printk(KERN_ERR "serial: Couldn's remap IO memory at %#lx\n", port);
+		return 1;
+	}
This seems wrong. ioremap returns NULL in case of failure.

Of course it's a typo, stupid me :((
What about attached patch ?

Signature · 2 lines

Andrey Panin            | Embedded systems software engineer
[email protected]        | PGP key: http://www.orbita1.ru/~pazke/AndreyPanin.asc

linux/drivers/char/serial.c renamed from linux.vanilla/drivers/char/serial.c +47 ~~-10~~

diff -u /linux.vanilla/drivers/char/serial.c /linux/drivers/char/serial.c
@@ -3876,7 +3876,10 @@
 		return 0;
 	}
 	req->io_type = SERIAL_IO_MEM;
-	req->iomem_base = ioremap(port, board->uart_offset);
+	if (!(req->iomem_base = ioremap(port, board->uart_offset))) {
+		printk(KERN_ERR "serial: Couldn's remap IO memory at %#lx\n", port);
+		return 1;
+	}
 	req->iomem_reg_shift = board->reg_shift;
 	req->port = 0;
 	return 0;

@@ -3939,8 +3942,13 @@
 	 * shutdown the board on a module unload.
 	 */
 	if (DEACTIVATE_FUNC(dev) || board->init_fn) {
-		if (serial_pci_board_idx >= NR_PCI_BOARDS)
+		if (serial_pci_board_idx >= NR_PCI_BOARDS) {
+			if (board->init_fn)
+				(board->init_fn)(dev, board, 0);
+			if (board->flags & SPCI_FL_ISPNP)
+				(DEACTIVATE_FUNC(dev))(dev);
 			return;
+		}
 		serial_pci_board[serial_pci_board_idx].board = *board;
 		serial_pci_board[serial_pci_board_idx].dev = dev;
 		serial_pci_board_idx++;

@@ -3954,8 +3962,13 @@
 
 	for (k=0; k < board->num_ports; k++) {
 		serial_req.irq = get_pci_irq(dev, board, k);
-		if (get_pci_port(dev, board, &serial_req, k))
+		if (get_pci_port(dev, board, &serial_req, k)) {
+			if (board->init_fn)
+				(board->init_fn)(dev, board, 0);
+			if ((board->flags & SPCI_FL_ISPNP) && (k == 0))
+				(DEACTIVATE_FUNC(dev))(dev);
 			break;
+		}
 		serial_req.flags = ASYNC_SKIP_TEST | ASYNC_AUTOPROBE;
 #ifdef SERIAL_DEBUG_PCI
 		printk("Setup PCI/PNP port: port %x, irq %d, type %d\n",

@@ -4010,7 +4023,11 @@
 				      data | pci_config);
 	
 	/* enable/disable interrupts */
-	p = ioremap(pci_resource_start(dev, 0), 0x80);
+	if (!(p = ioremap(pci_resource_start(dev, 0), 0x80))) {
+		printk( KERN_ERR "serial: Couldn't remap IO memory at %#lx\n",
+			pci_resource_start(dev, 0));
+		return 1;
+	}
 	writel(enable ? irq_config : 0x00, (unsigned long)p + 0x4c);
 	iounmap(p);

@@ -4053,7 +4070,11 @@
 
        if (!enable) return 0;
 
-       p = ioremap(pci_resource_start(dev, 0), 0x80);
+       if (!(p = ioremap(pci_resource_start(dev, 0), 0x80))) {
+       		printk( KERN_ERR "serial: Couldn't remap IO memory at %#lx\n",
+			pci_resource_start(dev, 0));
+		return 1;
+       }
 
        switch (dev->device & 0xfff8) {
                case PCI_DEVICE_ID_SIIG_1S_10x:         /* 1S */

@@ -5215,6 +5236,16 @@
 /*
  * The serial driver boot-time initialization code!
  */
+static void __init rs_cleanup_after_failure(struct tty_driver *tty)
+{
+	unsigned long flags;
+	del_timer_sync(&serial_timer);
+	save_flags(flags); cli();
+	remove_bh(SERIAL_BH);
+	if (tty) tty_unregister_driver(tty);
+	restore_flags(flags);
+}
+
 static int __init rs_init(void)
 {
 	int i;

@@ -5315,11 +5346,17 @@
 	callout_driver.proc_entry = 0;
 #endif
 
-	if (tty_register_driver(&serial_driver))
-		panic("Couldn't register serial driver\n");
-	if (tty_register_driver(&callout_driver))
-		panic("Couldn't register callout driver\n");
-	
+	if ((i = tty_register_driver(&serial_driver))) {
+		printk(KERN_ERR "serial: Couldn't register serial driver\n");
+		rs_cleanup_after_failure(NULL);
+		return i;
+	}
+	if ((i = tty_register_driver(&callout_driver))) {
+		printk(KERN_ERR "serial: Couldn't register callout driver\n");
+		rs_cleanup_after_failure(&serial_driver);
+		return i;
+	}
+
 	for (i = 0, state = rs_table; i < NR_PORTS; i++,state++) {
 		state->magic = SSTATE_MAGIC;
 		state->line = i;

[email protected]

2001-03-05 15:45 UTC #3 lore raw

In reply to Andrey Panin

Date: Mon, 5 Mar 2001 15:37:04 +0300
From: Andrey Panin <[email protected]>

I already sent this patch some days ago, but didn't get an answer :( So, i'm trying to resubmit it.

Thanks for submitting it; the general idea of the patch looks good, but as prumpf pointed out, the test for ioremap is reversed, so it looks like it hasn't been tested.

I'll fix it up and send it on to Linus.

- Ted