[RFC 1/13] Char: nozomi, remove unneded stuff

[RFC 1/13] Char: nozomi, remove unneded stuff

[Jiri Slaby]

Frank, could you comment (and test) following changes to the nozomi driver?
I did them before you post the updated version and rediffed them a while
ago, I hope I haven't done any mistake while doing it.

Thanks.

--

nozomi, remove unneded stuff

- tty_termios* are set to tty struct, but tty_register_driver overwrites it
  with its own values (or NULLs), no need to have these pointers stored
- [rt]x_data are updated but never used

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit 79aeb51662902086507927cb7fcd682ae319630a
tree 157e1fcc2b57542d831bcb80a6328edbfe6ed231
parent 9d50b0aba24ca0b601820663939987990db134c6
author Jiri Slaby <jirislaby@gmail.com> Mon, 29 Oct 2007 11:01:49 +0100
committer Jiri Slaby <jirislaby@gmail.com> Fri, 09 Nov 2007 22:52:10 +0100

 drivers/char/nozomi.c |   18 ------------------
 1 files changed, 0 insertions(+), 18 deletions(-)

diff --git a/drivers/char/nozomi.c b/drivers/char/nozomi.c
index 93b48b4..458e70b 100644
--- a/drivers/char/nozomi.c
+++ b/drivers/char/nozomi.c
@@ -412,8 +412,6 @@ struct port {
 	wait_queue_head_t tty_wait;
 	struct async_icount tty_icount;
 	int tty_index;
-	u32 rx_data, tx_data;
-
 };
 
 /* Private data one for each card in the system */
@@ -435,8 +433,6 @@ struct nozomi {
 
 	struct tty_driver *tty_driver;
 
-	struct ktermios *tty_termios[NTTY_TTY_MINORS];
-	struct ktermios *tty_termios_locked[NTTY_TTY_MINORS];
 	spinlock_t spin_mutex;	/* secures access to registers and tty */
 
 	u32 open_ttys;
@@ -1012,8 +1008,6 @@ static int send_data(enum port_type index, struct nozomi *dc)
 		return 0;
 	}
 
-	port->tx_data += size;
-
 	/* DUMP(buf, size); */
 
 	/* Write length + data */
@@ -1059,8 +1053,6 @@ static int receive_data(enum port_type index, struct nozomi *dc)
 		return 1;
 	}
 
-	port->rx_data += size;
-
 	tty_buffer_request_room(tty, size);
 
 	while (size > 0) {
@@ -1517,7 +1509,6 @@ static void nozomi_get_card_type(struct nozomi *dc)
 static void nozomi_setup_private_data(struct nozomi *dc)
 {
 	void __iomem *offset = dc->base_addr + dc->card_type / 2;
-	int i;
 
 	dc->reg_fcr = (void __iomem *)(offset + R_FCR);
 	dc->reg_iir = (void __iomem *)(offset + R_IIR);
@@ -1529,11 +1520,6 @@ static void nozomi_setup_private_data(struct nozomi *dc)
 	dc->port[PORT_DIAG].token_dl = DIAG_DL;
 	dc->port[PORT_APP1].token_dl = APP1_DL;
 	dc->port[PORT_APP2].token_dl = APP2_DL;
-
-	for (i = PORT_MDM; i < MAX_PORT; ++i) {
-		dc->port[i].rx_data = 0;
-		dc->port[i].tx_data = 0;
-	}
 }
 
 static ssize_t card_type_show(struct device *dev, struct device_attribute *attr,
@@ -1871,8 +1857,6 @@ static int ntty_open(struct tty_struct *tty, struct file *file)
 		tty->driver_data = port;
 		port->tty = tty;
 		port->tty_index = tty->index;
-		port->rx_data = 0;
-		port->tx_data = 0;
 		DBG1("open: %d", port->token_dl);
 		spin_lock_irqsave(&dc->spin_mutex, flags);
 		dc->last_ier =
@@ -2239,8 +2223,6 @@ static int ntty_tty_init(struct nozomi *dc)
 	td->init_termios.c_cflag = B115200 | CS8 | CREAD | HUPCL | CLOCAL;
 	td->init_termios.c_ispeed = 115200;
 	td->init_termios.c_ospeed = 115200;
-	td->termios = dc->tty_termios;
-	td->termios_locked = dc->tty_termios_locked;
 	tty_set_operations(dc->tty_driver, &tty_ops);
 
 	rval = tty_register_driver(td);

[RFC 2/13] Char: nozomi, expand some functions

[Jiri Slaby]

nozomi, expand some functions

nozomi_setup_interrupt and tty_do_close are used only in one place and has
no pros of being in separate functions. move tty_do_close contents into
ntty_close (it contained only tty_do_close call before) and
nozomi_setup_interrupt (only request_irq) into nozomi_card_init.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit 29335b619cc5ee27417d50982f029d440570f67c
tree 9cef37ef05e53453cd4d245a34c038ca12433253
parent 79aeb51662902086507927cb7fcd682ae319630a
author Jiri Slaby <jirislaby@gmail.com> Sun, 28 Oct 2007 11:06:05 +0100
committer Jiri Slaby <jirislaby@gmail.com> Fri, 09 Nov 2007 22:54:16 +0100

 drivers/char/nozomi.c |   75 +++++++++++++++++++------------------------------
 1 files changed, 29 insertions(+), 46 deletions(-)

diff --git a/drivers/char/nozomi.c b/drivers/char/nozomi.c
index 458e70b..2e2cbc5 100644
--- a/drivers/char/nozomi.c
+++ b/drivers/char/nozomi.c
@@ -1478,20 +1478,6 @@ none:
 	return IRQ_NONE;
 }
 
-/* Request a shared IRQ from system */
-static int nozomi_setup_interrupt(struct nozomi_devices *ndev)
-{
-	int rval;
-
-	rval = request_irq(ndev->my_dev->pdev->irq, &interrupt_handler,
-				IRQF_SHARED, NOZOMI_NAME, ndev);
-	if (unlikely(rval))
-		dev_err(&ndev->my_dev->pdev->dev, "Cannot open because IRQ %d "
-			"is already in use.\n", ndev->my_dev->pdev->irq);
-
-	return rval;
-}
-
 static void nozomi_get_card_type(struct nozomi *dc)
 {
 	int i;
@@ -1635,9 +1621,10 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 	dc->last_ier = 0;
 	writew(dc->last_ier, dc->reg_ier);
 
-	/* Setup interrupt handler */
-	if (nozomi_setup_interrupt(newdev)) {
-		ret = -EIO;
+	ret = request_irq(pdev->irq, &interrupt_handler, IRQF_SHARED,
+			NOZOMI_NAME, newdev);
+	if (unlikely(ret)) {
+		dev_err(&pdev->dev, "can't request irq\n");
 		goto err_disable_regions;
 	}
 
@@ -1694,34 +1681,6 @@ err_disable_device:
 	return ret;
 }
 
-static void tty_do_close(struct nozomi *dc, struct port *port)
-{
-	unsigned long flags;
-
-	if (!dc || !port)
-		return;
-
-	if (down_interruptible(&port->tty_sem))
-		return;
-
-	if (!port->tty_open_count)
-		goto exit;
-
-	dc->open_ttys--;
-	port->tty_open_count--;
-
-	if (port->tty_open_count == 0) {
-		DBG1("close: %d", port->token_dl);
-		spin_lock_irqsave(&dc->spin_mutex, flags);
-		dc->last_ier &= ~(port->token_dl);
-		writew(dc->last_ier, dc->reg_ier);
-		spin_unlock_irqrestore(&dc->spin_mutex, flags);
-	}
-
-exit:
-	up(&port->tty_sem);
-}
-
 static void __devexit tty_exit(struct nozomi_devices *ndev)
 {
 	struct nozomi *dc = ndev->my_dev;
@@ -1874,7 +1833,31 @@ static int ntty_open(struct tty_struct *tty, struct file *file)
 static void ntty_close(struct tty_struct *tty, struct file *file)
 {
 	struct nozomi *dc = get_dc_by_tty(tty);
-	tty_do_close(dc, (struct port *)tty->driver_data);
+	struct port *port = tty->driver_data;
+	unsigned long flags;
+
+	if (!dc || !port)
+		return;
+
+	if (down_interruptible(&port->tty_sem))
+		return;
+
+	if (!port->tty_open_count)
+		goto exit;
+
+	dc->open_ttys--;
+	port->tty_open_count--;
+
+	if (port->tty_open_count == 0) {
+		DBG1("close: %d", port->token_dl);
+		spin_lock_irqsave(&dc->spin_mutex, flags);
+		dc->last_ier &= ~(port->token_dl);
+		writew(dc->last_ier, dc->reg_ier);
+		spin_unlock_irqrestore(&dc->spin_mutex, flags);
+	}
+
+exit:
+	up(&port->tty_sem);
 }
 
 /*

Re: [RFC 2/13] Char: nozomi, expand some functions

[Frank Seidel]

On Samstag 10 November 2007 00:44:10, you (Jiri Slaby) wrote:
> nozomi, expand some functions
> 
> nozomi_setup_interrupt and tty_do_close are used only in one place and has
> no pros of being in separate functions. 

definitely true, all apllied without change

Thanks,
Frank

[RFC 3/13] Char: nozomi, fix fail paths

[Jiri Slaby]

nozomi, fix fail paths

Free resources on fail path in probe function properly (free_irq,
remove_sysfs_files, kfifo_free, kfree(dc->send_buf) and atomic_dec). Also
use kfifo_free instead of kfree on release function, because it leaked
fifo->buffer.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit 14e887763c076e45f4f768348361a37c10709902
tree 67854b6ec0a6460d857b37379a2f0772b7f5a9f8
parent 29335b619cc5ee27417d50982f029d440570f67c
author Jiri Slaby <jirislaby@gmail.com> Fri, 09 Nov 2007 23:08:52 +0100
committer Jiri Slaby <jirislaby@gmail.com> Fri, 09 Nov 2007 23:08:52 +0100

 drivers/char/nozomi.c |   47 ++++++++++++++++++++++++++++++-----------------
 1 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/drivers/char/nozomi.c b/drivers/char/nozomi.c
index 2e2cbc5..e33f21e 100644
--- a/drivers/char/nozomi.c
+++ b/drivers/char/nozomi.c
@@ -1549,7 +1549,7 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 				      const struct pci_device_id *ent)
 {
 	resource_size_t start;
-	int ret = -EIO;
+	int ret = -ENOMEM;
 	struct nozomi *dc = NULL;
 	struct nozomi_devices *newdev = NULL;
 	struct nozomi_devices *first = NULL;
@@ -1563,13 +1563,12 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 	dc = kzalloc(sizeof(struct nozomi), GFP_KERNEL);
 	if (unlikely(!dc)) {
 		dev_err(&pdev->dev, "Could not allocate memory\n");
-		return -ENOMEM;
+		goto err_free;
 	}
 	newdev = kzalloc(sizeof(struct nozomi_devices), GFP_KERNEL);
 	if (unlikely(!newdev)) {
 		dev_err(&pdev->dev, "Could not allocate memory\n");
-		kfree(dc);
-		return -ENOMEM;
+		goto err_free;
 	}
 
 	dc->pdev = pdev;
@@ -1581,9 +1580,7 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 
 	if (pci_enable_device(dc->pdev)) {
 		dev_err(&pdev->dev, "Failed to enable PCI Device\n");
-		kfree(dc);
-		kfree(newdev);
-		return -ENODEV;
+		goto err_free;
 	}
 
 	start = pci_resource_start(dc->pdev, 0);
@@ -1604,17 +1601,18 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 
 	nozomi_setup_private_data(dc);
 
-	if (pci_request_regions(dc->pdev, NOZOMI_NAME)) {
+	ret = pci_request_regions(dc->pdev, NOZOMI_NAME);
+	if (ret) {
 		dev_err(&pdev->dev, "I/O address 0x%04x already in use\n",
 			(int) /* nozomi_private.io_addr */ 0);
-		ret = -EIO;
-		goto err_disable_regions;
+		goto err_unmap;
 	}
 
 	dc->send_buf = kmalloc(SEND_BUF_MAX, GFP_KERNEL);
 	if (!dc->send_buf) {
 		dev_err(&pdev->dev, "Could not allocate send buffer?\n");
-		goto err_disable_regions;
+		ret = -ENOMEM;
+		goto err_free_sbuf;
 	}
 
 	/* Disable all interrupts */
@@ -1625,7 +1623,7 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 			NOZOMI_NAME, newdev);
 	if (unlikely(ret)) {
 		dev_err(&pdev->dev, "can't request irq\n");
-		goto err_disable_regions;
+		goto err_free_sbuf;
 	}
 
 	DBG1("base_addr: %p", dc->base_addr);
@@ -1636,13 +1634,17 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 	if (new_index < 0) {
 		dev_err(&pdev->dev, "already reached maximum card count.\n");
 		ret = -EIO;
-		goto err_disable_regions;
+		goto err_free_irq;
 	}
 
 	make_sysfs_files(dc);
 
 	if (atomic_read(&cards_found) == 1) {
-		ntty_tty_init(dc);
+		ret = ntty_tty_init(dc);
+		if (ret) {
+			dev_err(&pdev->dev, "can't alloc ntty\n");
+			goto err_sysfs;
+		}
 	} else {
 		first = list_first_entry(&my_devices, struct nozomi_devices,
 									list);
@@ -1669,15 +1671,25 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 
 	return 0;
 
-err_disable_regions:
+err_sysfs:
+	remove_sysfs_files(dc);
+err_free_irq:
+	free_irq(pdev->irq, newdev);
+	for (i = PORT_MDM; i < MAX_PORT; i++)	/* allocated in isr, might be */
+		if (dc->port[i].fifo_ul)	/* filled yet */
+			kfifo_free(dc->port[i].fifo_ul);
+err_free_sbuf:
+	kfree(dc->send_buf);
 	pci_release_regions(pdev);
+err_unmap:
 	iounmap(dc->base_addr);
 	dc->base_addr = NULL;
-
 err_disable_device:
 	pci_disable_device(pdev);
+err_free:
 	kfree(dc);
 	kfree(newdev);
+	atomic_dec(&cards_found);
 	return ret;
 }
 
@@ -1751,7 +1763,8 @@ static void __devexit nozomi_card_exit(struct pci_dev *pdev)
 	free_irq(pdev->irq, deventry);
 
 	for (i = PORT_MDM; i < MAX_PORT; i++)
-		kfree(dc->port[i].fifo_ul);
+		if (dc->port[i].fifo_ul)
+			kfifo_free(dc->port[i].fifo_ul);
 
 	kfree(dc->send_buf);
 

Re: [RFC 3/13] Char: nozomi, fix fail paths

[Frank Seidel]

On Samstag 10 November 2007 00:44:50, you (Jiri Slaby) wrote:
> nozomi, fix fail paths
> 
> Free resources on fail path in probe function properly (free_irq,
> remove_sysfs_files, kfifo_free, kfree(dc->send_buf) and atomic_dec). Also
> use kfifo_free instead of kfree on release function, because it leaked
> fifo->buffer.
> 
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

all applied without change

Thanks,
Frank

[RFC 4/13] Char: nozomi, tty index cleanup

[Jiri Slaby]

nozomi, tty index cleanup

- don't store unneeded copy of tty->index into port structure, tty->index
  is available everywhere
- mod tty->index by MAX_PORT where expected (otherwise array index out of
  bounds)

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit 3afb47133874a0979f57928d7450612fa982a6c5
tree 8d9ea282ddbffed0d75c946b3ae046e3bdcc7456
parent 14e887763c076e45f4f768348361a37c10709902
author Jiri Slaby <jirislaby@gmail.com> Sun, 28 Oct 2007 22:15:22 +0100
committer Jiri Slaby <jirislaby@gmail.com> Fri, 09 Nov 2007 23:09:04 +0100

 drivers/char/nozomi.c |   47 +++++++++++++++++++----------------------------
 1 files changed, 19 insertions(+), 28 deletions(-)

diff --git a/drivers/char/nozomi.c b/drivers/char/nozomi.c
index e33f21e..d19b5c5 100644
--- a/drivers/char/nozomi.c
+++ b/drivers/char/nozomi.c
@@ -411,7 +411,6 @@ struct port {
 	struct semaphore tty_sem;
 	wait_queue_head_t tty_wait;
 	struct async_icount tty_icount;
-	int tty_index;
 };
 
 /* Private data one for each card in the system */
@@ -1782,22 +1781,22 @@ static void __devexit nozomi_card_exit(struct pci_dev *pdev)
 static void set_rts(int index, int rts)
 {
 	struct nozomi *dc = get_dc_by_index(index);
-	int devindex = index % NTTY_TTY_MINORS;
+	struct port *port = &dc->port[index % MAX_PORT];
 
-	dc->port[devindex].ctrl_ul.RTS = rts;
-	dc->port[devindex].update_flow_control = 1;
+	port->ctrl_ul.RTS = rts;
+	port->update_flow_control = 1;
 	enable_transmit_ul(PORT_CTRL, dc);
 }
 
 static void set_dtr(int index, int dtr)
 {
 	struct nozomi *dc = get_dc_by_index(index);
-	int devindex = index % NTTY_TTY_MINORS;
+	struct port *port = &dc->port[index % MAX_PORT];
 
 	DBG1("SETTING DTR index: %d, dtr: %d", index, dtr);
 
-	dc->port[devindex].ctrl_ul.DTR = dtr;
-	dc->port[devindex].update_flow_control = 1;
+	port->ctrl_ul.DTR = dtr;
+	port->update_flow_control = 1;
 	enable_transmit_ul(PORT_CTRL, dc);
 }
 
@@ -1828,7 +1827,6 @@ static int ntty_open(struct tty_struct *tty, struct file *file)
 		tty->low_latency = 1;
 		tty->driver_data = port;
 		port->tty = tty;
-		port->tty_index = tty->index;
 		DBG1("open: %d", port->token_dl);
 		spin_lock_irqsave(&dc->spin_mutex, flags);
 		dc->last_ier =
@@ -1918,14 +1916,13 @@ static int ntty_write(struct tty_struct *tty, const unsigned char *buffer,
 	if (port == &(dc->port[PORT_MDM])) {
 		if (port->ctrl_dl.CTS) {
 			DBG4("Enable interrupt");
-			enable_transmit_ul(port->tty_index % NTTY_TTY_MINORS,
-									dc);
+			enable_transmit_ul(tty->index % MAX_PORT, dc);
 		} else {
 			dev_err(&dc->pdev->dev,
 				"CTS not active on modem port?\n");
 		}
 	} else {
-		enable_transmit_ul(port->tty_index % NTTY_TTY_MINORS, dc);
+		enable_transmit_ul(tty->index % MAX_PORT, dc);
 	}
 	spin_unlock_irqrestore(&dc->spin_mutex, flags);
 
@@ -1980,17 +1977,15 @@ static int ntty_tiocmget(struct tty_struct *tty, struct file *file)
 static int ntty_tiocmset(struct tty_struct *tty, struct file *file,
 	unsigned int set, unsigned int clear)
 {
-	struct port *port = (struct port *)tty->driver_data;
-
 	if (set & TIOCM_RTS)
-		set_rts(port->tty_index, 1);
+		set_rts(tty->index, 1);
 	else if (clear & TIOCM_RTS)
-		set_rts(port->tty_index, 0);
+		set_rts(tty->index, 0);
 
 	if (set & TIOCM_DTR)
-		set_dtr(port->tty_index, 1);
+		set_dtr(tty->index, 1);
 	else if (clear & TIOCM_DTR)
-		set_dtr(port->tty_index, 0);
+		set_dtr(tty->index, 0);
 
 	return 0;
 }
@@ -2064,7 +2059,6 @@ static int ntty_ioctl_tiocgicount(struct tty_struct *tty, struct file *file,
 static int ntty_ioctl(struct tty_struct *tty, struct file *file,
 		      unsigned int cmd, unsigned long arg)
 {
-	struct port *port = tty->driver_data;
 	struct nozomi *dc = get_dc_by_tty(tty);
 	unsigned long flags;
 	int mask;
@@ -2094,9 +2088,9 @@ static int ntty_ioctl(struct tty_struct *tty, struct file *file,
 
 		spin_lock_irqsave(&dc->spin_mutex, flags);
 		if (mask & TIOCM_RTS)
-			set_rts(port->tty_index, 0);
+			set_rts(tty->index, 0);
 		if (mask & TIOCM_DTR)
-			set_dtr(port->tty_index, 0);
+			set_dtr(tty->index, 0);
 		spin_unlock_irqrestore(&dc->spin_mutex, flags);
 		rval = 0;
 		break;
@@ -2106,9 +2100,9 @@ static int ntty_ioctl(struct tty_struct *tty, struct file *file,
 
 		spin_lock_irqsave(&dc->spin_mutex, flags);
 		if (mask & TIOCM_RTS)
-			set_rts(port->tty_index, 1);
+			set_rts(tty->index, 1);
 		if (mask & TIOCM_DTR)
-			set_dtr(port->tty_index, 1);
+			set_dtr(tty->index, 1);
 		spin_unlock_irqrestore(&dc->spin_mutex, flags);
 		rval = 0;
 		break;
@@ -2126,15 +2120,13 @@ static int ntty_ioctl(struct tty_struct *tty, struct file *file,
  */
 static void ntty_unthrottle(struct tty_struct *tty)
 {
-	struct port *port = (struct port *)tty->driver_data;
 	struct nozomi *dc = get_dc_by_tty(tty);
 	unsigned long flags;
 
 	DBG1("UNTHROTTLE");
 	spin_lock_irqsave(&dc->spin_mutex, flags);
-	enable_transmit_dl(port->tty_index % NTTY_TTY_MINORS, dc);
-	set_rts(port->tty_index, 1);
-
+	enable_transmit_dl(tty->index % MAX_PORT, dc);
+	set_rts(tty->index, 1);
 	spin_unlock_irqrestore(&dc->spin_mutex, flags);
 }
 
@@ -2144,13 +2136,12 @@ static void ntty_unthrottle(struct tty_struct *tty)
  */
 static void ntty_throttle(struct tty_struct *tty)
 {
-	struct port *port = (struct port *)tty->driver_data;
 	struct nozomi *dc = get_dc_by_tty(tty);
 	unsigned long flags;
 
 	DBG1("THROTTLE");
 	spin_lock_irqsave(&dc->spin_mutex, flags);
-	set_rts(port->tty_index, 0);
+	set_rts(tty->index, 0);
 	spin_unlock_irqrestore(&dc->spin_mutex, flags);
 }
 

Re: [RFC 4/13] Char: nozomi, tty index cleanup

[Frank Seidel]

On Samstag 10 November 2007 00:45:30, you (Jiri Slaby) wrote:
> nozomi, tty index cleanup
> 
> - don't store unneeded copy of tty->index into port structure, tty->index
>   is available everywhere
> - mod tty->index by MAX_PORT where expected (otherwise array index out of
>   bounds)
The last point i can't retrace, as all those parts were already IMHO.
NTTY_TTY_MINORS is just an alias for MAX_PORT, but of course its much 
better readable and also shorter. But i doubt this prevents to an array
index to get out of bounds ;-)

but all applied without a change

Thanks,
Frank

Re: [RFC 4/13] Char: nozomi, tty index cleanup

[Jiri Slaby]

On 11/10/2007 04:41 PM, Frank Seidel wrote:
> On Samstag 10 November 2007 00:45:30, you (Jiri Slaby) wrote:
>> nozomi, tty index cleanup
>>
>> - don't store unneeded copy of tty->index into port structure, tty->index
>>   is available everywhere
>> - mod tty->index by MAX_PORT where expected (otherwise array index out of
>>   bounds)
> The last point i can't retrace, as all those parts were already IMHO.
> NTTY_TTY_MINORS is just an alias for MAX_PORT, but of course its much 
> better readable and also shorter. But i doubt this prevents to an array
> index to get out of bounds ;-)

Aha, this is comment from old patch (before you made the same fixes in c version
of the driver), where I added % in some places, e.g. index of port in dc
structure...

regards,
-- 
Jiri Slaby (jirislaby@gmail.com)
Faculty of Informatics, Masaryk University

[RFC 5/13] Char: nozomi, ioctls cleanup

[Jiri Slaby]

nozomi, ioctls cleanup

- init tty_wait
- don't forget to wake up tiocmiwait waiters
- convert the whole tiocmiwait into wait_event_interruptible
- don't check for cmd == TIOCGICOUNT in ntty_ioctl_tiocgicount, as it's
  expected to be it when we call the function, also pass internal
  structures (port, argp) directly instead of ioctl params
- remove TIOCMGET, TIOCMSET, TIOCMBIC, TIOCMBIS as it is never invoked by
  tty layer this way, add ntty_tiocmget into tty ops instead

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit 2470e477561f2577137d557aa4dcf92b0229a745
tree 3486f8adf936d56143d46b5b9e6ad7f55c7a88db
parent 3afb47133874a0979f57928d7450612fa982a6c5
author Jiri Slaby <jirislaby@gmail.com> Mon, 29 Oct 2007 12:28:57 +0100
committer Jiri Slaby <jirislaby@gmail.com> Fri, 09 Nov 2007 23:12:06 +0100

 drivers/char/nozomi.c |  153 ++++++++++++++++---------------------------------
 1 files changed, 50 insertions(+), 103 deletions(-)

diff --git a/drivers/char/nozomi.c b/drivers/char/nozomi.c
index d19b5c5..3f4ae3d 100644
--- a/drivers/char/nozomi.c
+++ b/drivers/char/nozomi.c
@@ -1199,6 +1199,9 @@ static int receive_flow_control(struct nozomi *dc, struct irq *m)
 		dc->port[port].tty_icount.rng++;
 	if (old_ctrl.DCD != ctrl_dl.DCD)
 		dc->port[port].tty_icount.dcd++;
+
+	wake_up_interruptible(&dc->port[port].tty_wait);
+
 	DBG1("port: %d DCD(%d), CTS(%d), RI(%d), DSR(%d)",
 	   port,
 	   dc->port[port].tty_icount.dcd, dc->port[port].tty_icount.cts,
@@ -1494,6 +1497,7 @@ static void nozomi_get_card_type(struct nozomi *dc)
 static void nozomi_setup_private_data(struct nozomi *dc)
 {
 	void __iomem *offset = dc->base_addr + dc->card_type / 2;
+	unsigned int i;
 
 	dc->reg_fcr = (void __iomem *)(offset + R_FCR);
 	dc->reg_iir = (void __iomem *)(offset + R_IIR);
@@ -1505,6 +1509,9 @@ static void nozomi_setup_private_data(struct nozomi *dc)
 	dc->port[PORT_DIAG].token_dl = DIAG_DL;
 	dc->port[PORT_APP1].token_dl = APP1_DL;
 	dc->port[PORT_APP2].token_dl = APP2_DL;
+
+	for (i = 0; i < MAX_PORT; i++)
+		init_waitqueue_head(&dc->port[i].tty_wait);
 }
 
 static ssize_t card_type_show(struct device *dev, struct device_attribute *attr,
@@ -1965,12 +1972,12 @@ static int ntty_tiocmget(struct tty_struct *tty, struct file *file)
 	struct ctrl_dl *ctrl_dl = &port->ctrl_dl;
 	struct ctrl_ul *ctrl_ul = &port->ctrl_ul;
 
-	return 0 | (ctrl_ul->RTS ? TIOCM_RTS : 0)
-	    | (ctrl_ul->DTR ? TIOCM_DTR : 0)
-	    | (ctrl_dl->DCD ? TIOCM_CAR : 0)
-	    | (ctrl_dl->RI ? TIOCM_RNG : 0)
-	    | (ctrl_dl->DSR ? TIOCM_DSR : 0)
-	    | (ctrl_dl->CTS ? TIOCM_CTS : 0);
+	return	(ctrl_ul->RTS ? TIOCM_RTS : 0) |
+		(ctrl_ul->DTR ? TIOCM_DTR : 0) |
+		(ctrl_dl->DCD ? TIOCM_CAR : 0) |
+		(ctrl_dl->RI  ? TIOCM_RNG : 0) |
+		(ctrl_dl->DSR ? TIOCM_DSR : 0) |
+		(ctrl_dl->CTS ? TIOCM_CTS : 0);
 }
 
 /* Sets io controls parameters */
@@ -1990,121 +1997,60 @@ static int ntty_tiocmset(struct tty_struct *tty, struct file *file,
 	return 0;
 }
 
-static int ntty_ioctl_tiocmiwait(struct tty_struct *tty, struct file *file,
-				 unsigned int cmd, unsigned long arg)
+static int ntty_cflags_changed(struct port *port, unsigned long flags,
+		struct async_icount *cprev)
 {
-	struct port *port = (struct port *)tty->driver_data;
+	struct async_icount cnow = port->tty_icount;
+	int ret;
 
-	if (cmd == TIOCMIWAIT) {
-		DECLARE_WAITQUEUE(wait, current);
-		struct async_icount cnow;
-		struct async_icount cprev;
-
-		cprev = port->tty_icount;
-		while (1) {
-			add_wait_queue(&port->tty_wait, &wait);
-			set_current_state(TASK_INTERRUPTIBLE);
-			schedule();
-			remove_wait_queue(&port->tty_wait, &wait);
-
-			/* see if a signal woke us up */
-			if (signal_pending(current))
-				return -ERESTARTSYS;
-
-			cnow = port->tty_icount;
-			if (cnow.rng == cprev.rng && cnow.dsr == cprev.dsr &&
-			    cnow.dcd == cprev.dcd && cnow.cts == cprev.cts)
-				return -EIO;	/* no change => error */
-			if (((arg & TIOCM_RNG) && (cnow.rng != cprev.rng)) ||
-			    ((arg & TIOCM_DSR) && (cnow.dsr != cprev.dsr)) ||
-			    ((arg & TIOCM_CD) && (cnow.dcd != cprev.dcd)) ||
-			    ((arg & TIOCM_CTS) && (cnow.cts != cprev.cts)))
-				return 0;
-
-			cprev = cnow;
-		}
+	ret =	((flags & TIOCM_RNG) && (cnow.rng != cprev->rng)) ||
+		((flags & TIOCM_DSR) && (cnow.dsr != cprev->dsr)) ||
+		((flags & TIOCM_CD)  && (cnow.dcd != cprev->dcd)) ||
+		((flags & TIOCM_CTS) && (cnow.cts != cprev->cts));
 
-	}
-	return -ENOIOCTLCMD;
+	*cprev = cnow;
+
+	return ret;
 }
 
-static int ntty_ioctl_tiocgicount(struct tty_struct *tty, struct file *file,
-				  unsigned int cmd, void __user *arg)
+static int ntty_ioctl_tiocgicount(struct port *port, void __user *argp)
 {
-	struct port *port = (struct port *)tty->driver_data;
-
-	if (cmd == TIOCGICOUNT) {
-		struct async_icount cnow = port->tty_icount;
-		struct serial_icounter_struct icount;
-
-		icount.cts = cnow.cts;
-		icount.dsr = cnow.dsr;
-		icount.rng = cnow.rng;
-		icount.dcd = cnow.dcd;
-		icount.rx = cnow.rx;
-		icount.tx = cnow.tx;
-		icount.frame = cnow.frame;
-		icount.overrun = cnow.overrun;
-		icount.parity = cnow.parity;
-		icount.brk = cnow.brk;
-		icount.buf_overrun = cnow.buf_overrun;
-
-		if (copy_to_user(arg, &icount, sizeof(icount)))
-			return -EFAULT;
-		return 0;
-	}
-	return -ENOIOCTLCMD;
+	struct async_icount cnow = port->tty_icount;
+	struct serial_icounter_struct icount;
+
+	icount.cts = cnow.cts;
+	icount.dsr = cnow.dsr;
+	icount.rng = cnow.rng;
+	icount.dcd = cnow.dcd;
+	icount.rx = cnow.rx;
+	icount.tx = cnow.tx;
+	icount.frame = cnow.frame;
+	icount.overrun = cnow.overrun;
+	icount.parity = cnow.parity;
+	icount.brk = cnow.brk;
+	icount.buf_overrun = cnow.buf_overrun;
+
+	return copy_to_user(argp, &icount, sizeof(icount));
 }
 
 static int ntty_ioctl(struct tty_struct *tty, struct file *file,
 		      unsigned int cmd, unsigned long arg)
 {
-	struct nozomi *dc = get_dc_by_tty(tty);
-	unsigned long flags;
-	int mask;
+	struct port *port = tty->driver_data;
+	void __user *argp = (void __user *)arg;
 	int rval = -ENOIOCTLCMD;
 
 	DBG1("******** IOCTL, cmd: %d", cmd);
 
 	switch (cmd) {
-	case TIOCMIWAIT:
-		rval = ntty_ioctl_tiocmiwait(tty, file, cmd, arg);
-		break;
-	case TIOCGICOUNT:
-		rval =
-		    ntty_ioctl_tiocgicount(tty, file, cmd, (void __user *)arg);
-		break;
-	case TIOCMGET:
-		spin_lock_irqsave(&dc->spin_mutex, flags);
-		rval = ntty_tiocmget(tty, file);
-		spin_unlock_irqrestore(&dc->spin_mutex, flags);
-		break;
-	case TIOCMSET:
-		rval = ntty_tiocmset(tty, file, arg, ~arg);
-		break;
-	case TIOCMBIC:
-		if (get_user(mask, (unsigned long __user *)arg))
-			return -EFAULT;
+	case TIOCMIWAIT: {
+		struct async_icount cprev = port->tty_icount;
 
-		spin_lock_irqsave(&dc->spin_mutex, flags);
-		if (mask & TIOCM_RTS)
-			set_rts(tty->index, 0);
-		if (mask & TIOCM_DTR)
-			set_dtr(tty->index, 0);
-		spin_unlock_irqrestore(&dc->spin_mutex, flags);
-		rval = 0;
+		rval = wait_event_interruptible(port->tty_wait,
+				ntty_cflags_changed(port, arg, &cprev));
 		break;
-	case TIOCMBIS:
-		if (get_user(mask, (unsigned long __user *)arg))
-			return -EFAULT;
-
-		spin_lock_irqsave(&dc->spin_mutex, flags);
-		if (mask & TIOCM_RTS)
-			set_rts(tty->index, 1);
-		if (mask & TIOCM_DTR)
-			set_dtr(tty->index, 1);
-		spin_unlock_irqrestore(&dc->spin_mutex, flags);
-		rval = 0;
+	} case TIOCGICOUNT:
+		rval = ntty_ioctl_tiocgicount(port, argp);
 		break;
 	default:
 		DBG1("ERR: 0x%08X, %d", cmd, cmd);
@@ -2186,6 +2132,7 @@ static struct tty_operations tty_ops = {
 	.throttle = ntty_throttle,
 	.chars_in_buffer = ntty_chars_in_buffer,
 	.put_char = ntty_put_char,
+	.tiocmget = ntty_tiocmget,
 	.tiocmset = ntty_tiocmset,
 };
 

Re: [RFC 5/13] Char: nozomi, ioctls cleanup

[Frank Seidel]

On Samstag 10 November 2007 00:46:11, you (Jiri Slaby) wrote:
> nozomi, ioctls cleanup
> 
> - init tty_wait
> - don't forget to wake up tiocmiwait waiters
> - convert the whole tiocmiwait into wait_event_interruptible
> - don't check for cmd == TIOCGICOUNT in ntty_ioctl_tiocgicount, as it's
>   expected to be it when we call the function, also pass internal
>   structures (port, argp) directly instead of ioctl params
> - remove TIOCMGET, TIOCMSET, TIOCMBIC, TIOCMBIS as it is never invoked by
>   tty layer this way, add ntty_tiocmget into tty ops instead

Sadfully my knowledge of the tty layer is still too low to comment anything
usefull, but all sound and looks reasonable to me.
And i did a special testrun after this patch and couldn't see any problems.

So, applied without change.

Thanks,
Frank

[RFC 6/13] Char: nozomi, reorder and cleanup probe, remove

[Jiri Slaby]

nozomi, reorder and cleanup probe, remove

- remap space after requesting region, to not map something we cannot use
  anyway
- init spin lock before request_irq, because due to shared irq debug, isr
  might be called immediately, where the lock is being held
- remove INIT_LIST_HEAD before list_add, it's useless, because list_add
  re-sets prev and next
- reorder cleanup to be the same as fail path of probe (or in other words,
  the reverse of probe)
- no need to call nozomi_setup_private_data in remove, pointers are set in
  probe

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit 662a9d19654d8d79fbaeab13d7afe7fe021e6b42
tree 3c73690a8715ce52b8125413b94ea24b8f0013d3
parent 2470e477561f2577137d557aa4dcf92b0229a745
author Jiri Slaby <jirislaby@gmail.com> Fri, 02 Nov 2007 09:29:38 +0100
committer Jiri Slaby <jirislaby@gmail.com> Fri, 09 Nov 2007 23:20:25 +0100

 drivers/char/nozomi.c |   52 ++++++++++++++++++++-----------------------------
 1 files changed, 21 insertions(+), 31 deletions(-)

diff --git a/drivers/char/nozomi.c b/drivers/char/nozomi.c
index 3f4ae3d..eaa65fc 100644
--- a/drivers/char/nozomi.c
+++ b/drivers/char/nozomi.c
@@ -1579,7 +1579,6 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 
 	dc->pdev = pdev;
 	newdev->my_dev = dc;
-	pci_set_drvdata(pdev, newdev);
 
 	/* Find out what card type it is */
 	nozomi_get_card_type(dc);
@@ -1596,22 +1595,18 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 		goto err_disable_device;
 	}
 
-	dc->base_addr = ioremap(start, dc->card_type);
-	if (!dc->base_addr) {
-		dev_err(&pdev->dev, "Unable to map card MMIO\n");
-		ret = -ENODEV;
-		goto err_disable_device;
-	}
-
-	dc->open_ttys = 0;
-
-	nozomi_setup_private_data(dc);
-
 	ret = pci_request_regions(dc->pdev, NOZOMI_NAME);
 	if (ret) {
 		dev_err(&pdev->dev, "I/O address 0x%04x already in use\n",
 			(int) /* nozomi_private.io_addr */ 0);
-		goto err_unmap;
+		goto err_disable_device;
+	}
+
+	dc->base_addr = ioremap(start, dc->card_type);
+	if (!dc->base_addr) {
+		dev_err(&pdev->dev, "Unable to map card MMIO\n");
+		ret = -ENODEV;
+		goto err_rel_regs;
 	}
 
 	dc->send_buf = kmalloc(SEND_BUF_MAX, GFP_KERNEL);
@@ -1621,6 +1616,10 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 		goto err_free_sbuf;
 	}
 
+	spin_lock_init(&dc->spin_mutex);
+
+	nozomi_setup_private_data(dc);
+
 	/* Disable all interrupts */
 	dc->last_ier = 0;
 	writew(dc->last_ier, dc->reg_ier);
@@ -1634,8 +1633,6 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 
 	DBG1("base_addr: %p", dc->base_addr);
 
-	spin_lock_init(&dc->spin_mutex);
-
 	new_index = get_free_index();
 	if (new_index < 0) {
 		dev_err(&pdev->dev, "already reached maximum card count.\n");
@@ -1670,7 +1667,6 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 	dc->last_ier = RESET;
 	writew(dc->last_ier, dc->reg_ier);
 
-	INIT_LIST_HEAD(&newdev->list);
 	list_add_tail(&newdev->list, &my_devices);
 
 	pci_set_drvdata(pdev, newdev);
@@ -1686,10 +1682,9 @@ err_free_irq:
 			kfifo_free(dc->port[i].fifo_ul);
 err_free_sbuf:
 	kfree(dc->send_buf);
-	pci_release_regions(pdev);
-err_unmap:
 	iounmap(dc->base_addr);
-	dc->base_addr = NULL;
+err_rel_regs:
+	pci_release_regions(pdev);
 err_disable_device:
 	pci_disable_device(pdev);
 err_free:
@@ -1753,18 +1748,12 @@ static void __devexit nozomi_card_exit(struct pci_dev *pdev)
 	DBG1("sending flow control 0x%04X", *((u16 *)&ctrl));
 
 	/* Setup dc->reg addresses to we can use defines here */
-	nozomi_setup_private_data(dc);
 	write_mem32(dc->port[PORT_CTRL].ul_addr[0], (u32 *)&ctrl, 2);
 	writew(CTRL_UL, dc->reg_fcr);	/* push the token to the card. */
 
-	DBG1("pci_release_regions");
-	pci_release_regions(pdev);
-
-	if (dc->base_addr)
-		iounmap(dc->base_addr);
+	list_del(&deventry->list);
 
-	DBG1("pci_disable_device");
-	pci_disable_device(pdev);
+	remove_sysfs_files(dc);
 
 	free_irq(pdev->irq, deventry);
 
@@ -1774,12 +1763,13 @@ static void __devexit nozomi_card_exit(struct pci_dev *pdev)
 
 	kfree(dc->send_buf);
 
-	remove_sysfs_files(dc);
+	iounmap(dc->base_addr);
 
-	kfree(dc);
-	deventry->my_dev = 0;
+	pci_release_regions(pdev);
 
-	list_del(&deventry->list);
+	pci_disable_device(pdev);
+
+	kfree(dc);
 	kfree(deventry);
 
 	atomic_dec(&cards_found);

Re: [RFC 6/13] Char: nozomi, reorder and cleanup probe, remove

[Frank Seidel]

On Samstag 10 November 2007 00:46:51, you (Jiri Slaby) wrote:
> nozomi, reorder and cleanup probe, remove
> - remap space after requesting region, to not map something we cannot use
>   anyway
> - init spin lock before request_irq, because due to shared irq debug, isr
>   might be called immediately, where the lock is being held
> - remove INIT_LIST_HEAD before list_add, it's useless, because list_add
>   re-sets prev and next
> - reorder cleanup to be the same as fail path of probe (or in other words,
>   the reverse of probe)
> - no need to call nozomi_setup_private_data in remove, pointers are set in
>   probe

All perfect catches IMHO. Applied without changes.

Thanks,
Frank

[RFC 7/13] Char: nozomi, remove struct irq

[Jiri Slaby]

nozomi, remove struct irq

struct irq (named as my_irq) is used only in ISR and its called functions.
We might silently use u16 variable on stack and remove all references to
this structure. This is the first step of struct nozomi_devices removal.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit 9f9d7197e901ea00771812b7e903b14b95f54e40
tree fd36411ffccd536721bc42464489ffe2d3517dc2
parent 662a9d19654d8d79fbaeab13d7afe7fe021e6b42
author Jiri Slaby <jirislaby@gmail.com> Fri, 02 Nov 2007 10:57:49 +0100
committer Jiri Slaby <jirislaby@gmail.com> Fri, 09 Nov 2007 23:22:01 +0100

 drivers/char/nozomi.c |   85 ++++++++++++++++++++++---------------------------
 1 files changed, 39 insertions(+), 46 deletions(-)

diff --git a/drivers/char/nozomi.c b/drivers/char/nozomi.c
index eaa65fc..49e16c7 100644
--- a/drivers/char/nozomi.c
+++ b/drivers/char/nozomi.c
@@ -454,16 +454,10 @@ static struct pci_device_id nozomi_pci_tbl[] = {
 
 MODULE_DEVICE_TABLE(pci, nozomi_pci_tbl);
 
-/* Used to store interrupt variables */
-struct irq {
-	u16 read_iir;		/* Holds current interrupt tokens */
-};
-
 /* Representing the pci device of interest */
 struct nozomi_devices {
 	struct list_head list;
 	struct nozomi *my_dev;
-	struct irq my_irq;
 	int index_start;
 };
 static atomic_t cards_found = ATOMIC_INIT(0);
@@ -1125,7 +1119,7 @@ static char *interrupt2str(u16 interrupt)
  * Receive flow control
  * Return 1 - If ok, else 0
  */
-static int receive_flow_control(struct nozomi *dc, struct irq *m)
+static int receive_flow_control(struct nozomi *dc)
 {
 	enum port_type port = PORT_MDM;
 	struct ctrl_dl ctrl_dl;
@@ -1262,28 +1256,28 @@ static int send_flow_control(struct nozomi *dc)
  * Return 1 - ok
  * Return 0 - toggle fields are out of sync
  */
-static int handle_data_dl(struct nozomi *dc, struct irq *m, enum port_type port,
-			  u8 *toggle, u16 mask1, u16 mask2)
+static int handle_data_dl(struct nozomi *dc, enum port_type port, u8 *toggle,
+		u16 read_iir, u16 mask1, u16 mask2)
 {
-	if (*toggle == 0 && m->read_iir & mask1) {
+	if (*toggle == 0 && read_iir & mask1) {
 		if (receive_data(port, dc)) {
 			writew(mask1, dc->reg_fcr);
 			*toggle = !(*toggle);
 		}
 
-		if (m->read_iir & mask2) {
+		if (read_iir & mask2) {
 			if (receive_data(port, dc)) {
 				writew(mask2, dc->reg_fcr);
 				*toggle = !(*toggle);
 			}
 		}
-	} else if (*toggle == 1 && m->read_iir & mask2) {
+	} else if (*toggle == 1 && read_iir & mask2) {
 		if (receive_data(port, dc)) {
 			writew(mask2, dc->reg_fcr);
 			*toggle = !(*toggle);
 		}
 
-		if (m->read_iir & mask1) {
+		if (read_iir & mask1) {
 			if (receive_data(port, dc)) {
 				writew(mask1, dc->reg_fcr);
 				*toggle = !(*toggle);
@@ -1302,11 +1296,11 @@ static int handle_data_dl(struct nozomi *dc, struct irq *m, enum port_type port,
  * Return 1 - ok
  * Return 0 - toggle field are out of sync
  */
-static int handle_data_ul(struct nozomi *dc, struct irq *m, enum port_type port)
+static int handle_data_ul(struct nozomi *dc, enum port_type port, u16 read_iir)
 {
 	u8 *toggle = &(dc->port[port].toggle_ul);
 
-	if (*toggle == 0 && m->read_iir & MDM_UL1) {
+	if (*toggle == 0 && read_iir & MDM_UL1) {
 		dc->last_ier &= ~MDM_UL;
 		writew(dc->last_ier, dc->reg_ier);
 		if (send_data(port, dc)) {
@@ -1316,7 +1310,7 @@ static int handle_data_ul(struct nozomi *dc, struct irq *m, enum port_type port)
 			*toggle = !*toggle;
 		}
 
-		if (m->read_iir & MDM_UL2) {
+		if (read_iir & MDM_UL2) {
 			dc->last_ier &= ~MDM_UL;
 			writew(dc->last_ier, dc->reg_ier);
 			if (send_data(port, dc)) {
@@ -1328,7 +1322,7 @@ static int handle_data_ul(struct nozomi *dc, struct irq *m, enum port_type port)
 			}
 		}
 
-	} else if (*toggle == 1 && m->read_iir & MDM_UL2) {
+	} else if (*toggle == 1 && read_iir & MDM_UL2) {
 		dc->last_ier &= ~MDM_UL;
 		writew(dc->last_ier, dc->reg_ier);
 		if (send_data(port, dc)) {
@@ -1338,7 +1332,7 @@ static int handle_data_ul(struct nozomi *dc, struct irq *m, enum port_type port)
 			*toggle = !*toggle;
 		}
 
-		if (m->read_iir & MDM_UL1) {
+		if (read_iir & MDM_UL1) {
 			dc->last_ier &= ~MDM_UL;
 			writew(dc->last_ier, dc->reg_ier);
 			if (send_data(port, dc)) {
@@ -1350,7 +1344,7 @@ static int handle_data_ul(struct nozomi *dc, struct irq *m, enum port_type port)
 			}
 		}
 	} else {
-		writew(m->read_iir & MDM_UL, dc->reg_fcr);
+		writew(read_iir & MDM_UL, dc->reg_fcr);
 		dev_err(&dc->pdev->dev, "port out of sync!\n");
 		return 0;
 	}
@@ -1361,34 +1355,33 @@ static irqreturn_t interrupt_handler(int irq, void *dev_id)
 {
 	struct nozomi_devices *ndev = dev_id;
 	struct nozomi *dc;
-	struct irq *m;
+	u16 read_iir;
 
 	if (!ndev)
 		return IRQ_NONE;
 
 	dc = ndev->my_dev;
-	m = &ndev->my_irq;
 
 	spin_lock(&dc->spin_mutex);
-	m->read_iir = readw(dc->reg_iir);
+	read_iir = readw(dc->reg_iir);
 
 	/* Card removed */
-	if (m->read_iir == (u16)-1)
+	if (read_iir == (u16)-1)
 		goto none;
 	/*
 	 * Just handle interrupt enabled in IER
 	 * (by masking with dc->last_ier)
 	 */
-	m->read_iir &= dc->last_ier;
+	read_iir &= dc->last_ier;
 
-	if (m->read_iir == 0)
+	if (read_iir == 0)
 		goto none;
 
 
-	DBG4("%s irq:0x%04X, prev:0x%04X", interrupt2str(m->read_iir),
-	   m->read_iir, dc->last_ier);
+	DBG4("%s irq:0x%04X, prev:0x%04X", interrupt2str(read_iir), read_iir,
+			dc->last_ier);
 
-	if (m->read_iir & RESET) {
+	if (read_iir & RESET) {
 		if (unlikely(!nozomi_read_config_table(dc))) {
 			dc->last_ier = 0x0;
 			writew(dc->last_ier, dc->reg_ier);
@@ -1400,7 +1393,7 @@ static irqreturn_t interrupt_handler(int irq, void *dev_id)
 		/* No more useful info if this was the reset interrupt. */
 		goto exit_handler;
 	}
-	if (m->read_iir & CTRL_UL) {
+	if (read_iir & CTRL_UL) {
 		DBG1("CTRL_UL");
 		dc->last_ier &= ~CTRL_UL;
 		writew(dc->last_ier, dc->reg_ier);
@@ -1410,33 +1403,33 @@ static irqreturn_t interrupt_handler(int irq, void *dev_id)
 			writew(dc->last_ier, dc->reg_ier);
 		}
 	}
-	if (m->read_iir & CTRL_DL) {
-		receive_flow_control(dc, m);
+	if (read_iir & CTRL_DL) {
+		receive_flow_control(dc);
 		writew(CTRL_DL, dc->reg_fcr);
 	}
-	if (m->read_iir & MDM_DL) {
-		if (!handle_data_dl(dc, m, PORT_MDM,
-				&(dc->port[PORT_MDM].toggle_dl), MDM_DL1,
-				MDM_DL2)) {
+	if (read_iir & MDM_DL) {
+		if (!handle_data_dl(dc, PORT_MDM,
+				&(dc->port[PORT_MDM].toggle_dl), read_iir,
+				MDM_DL1, MDM_DL2)) {
 			dev_err(&dc->pdev->dev, "MDM_DL out of sync!\n");
 			goto exit_handler;
 		}
 	}
-	if (m->read_iir & MDM_UL) {
-		if (!handle_data_ul(dc, m, PORT_MDM)) {
+	if (read_iir & MDM_UL) {
+		if (!handle_data_ul(dc, PORT_MDM, read_iir)) {
 			dev_err(&dc->pdev->dev, "MDM_UL out of sync!\n");
 			goto exit_handler;
 		}
 	}
-	if (m->read_iir & DIAG_DL) {
-		if (!handle_data_dl(dc, m, PORT_DIAG,
-				&(dc->port[PORT_DIAG].toggle_dl), DIAG_DL1,
-				DIAG_DL2)) {
+	if (read_iir & DIAG_DL) {
+		if (!handle_data_dl(dc, PORT_DIAG,
+				&(dc->port[PORT_DIAG].toggle_dl), read_iir,
+				DIAG_DL1, DIAG_DL2)) {
 			dev_err(&dc->pdev->dev, "DIAG_DL out of sync!\n");
 			goto exit_handler;
 		}
 	}
-	if (m->read_iir & DIAG_UL) {
+	if (read_iir & DIAG_UL) {
 		dc->last_ier &= ~DIAG_UL;
 		writew(dc->last_ier, dc->reg_ier);
 		if (send_data(PORT_DIAG, dc)) {
@@ -1445,11 +1438,11 @@ static irqreturn_t interrupt_handler(int irq, void *dev_id)
 			writew(dc->last_ier, dc->reg_ier);
 		}
 	}
-	if (m->read_iir & APP1_DL) {
+	if (read_iir & APP1_DL) {
 		if (receive_data(PORT_APP1, dc))
 			writew(APP1_DL, dc->reg_fcr);
 	}
-	if (m->read_iir & APP1_UL) {
+	if (read_iir & APP1_UL) {
 		dc->last_ier &= ~APP1_UL;
 		writew(dc->last_ier, dc->reg_ier);
 		if (send_data(PORT_APP1, dc)) {
@@ -1458,11 +1451,11 @@ static irqreturn_t interrupt_handler(int irq, void *dev_id)
 			writew(dc->last_ier, dc->reg_ier);
 		}
 	}
-	if (m->read_iir & APP2_DL) {
+	if (read_iir & APP2_DL) {
 		if (receive_data(PORT_APP2, dc))
 			writew(APP2_DL, dc->reg_fcr);
 	}
-	if (m->read_iir & APP2_UL) {
+	if (read_iir & APP2_UL) {
 		dc->last_ier &= ~APP2_UL;
 		writew(dc->last_ier, dc->reg_ier);
 		if (send_data(PORT_APP2, dc)) {

Re: [RFC 7/13] Char: nozomi, remove struct irq

[Frank Seidel]

On Samstag 10 November 2007 00:47:31, you (Jiri Slaby) wrote:
> nozomi, remove struct irq
> 
> struct irq (named as my_irq) is used only in ISR and its called functions.
> We might silently use u16 variable on stack and remove all references to
> this structure. This is the first step of struct nozomi_devices removal.

i need a closer look at this (together with RFC 8/13).

Thanks,
Frank

Re: [RFC 7/13] Char: nozomi, remove struct irq

[Frank Seidel]

On Samstag 10 November 2007 00:47:31, you (Jiri Slaby) wrote:
> nozomi, remove struct irq
> struct irq (named as my_irq) is used only in ISR and its called functions.
> We might silently use u16 variable on stack and remove all references to
> this structure. This is the first step of struct nozomi_devices removal.

As this u16 is kind of local (just handed around from interrupt_handler) and
the structs value was anyway overwritten always/on every interrupt call
this seems to be totally ok.
Just to be sure i ran a whole testseries and it really didn't show anything
unusual => So, applied without changes.

Thanks,
Frank

[RFC 8/13] Char: nozomi, tty cleanup

[Jiri Slaby]

nozomi, tty cleanup

- init and deinit tty driver at module load/unload. When the OS (user)
  loads the driver the hardware usually is ready to driver.
- merge (unify) MAX_PORT, NTTY_TTY_MINORS into NOZOMI_MAX_PORTS
- remove struct nozomi_devices, it was used only as list entries

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit d0b01ce89a7b18ba37ea6192eea6a98cdc01d62e
tree a61f0a99633772b863130ec1e8a2be25891b3578
parent 9f9d7197e901ea00771812b7e903b14b95f54e40
author Jiri Slaby <jirislaby@gmail.com> Sat, 10 Nov 2007 00:12:53 +0100
committer Jiri Slaby <jirislaby@gmail.com> Sat, 10 Nov 2007 00:12:53 +0100

 drivers/char/nozomi.c |  336 ++++++++++++++++---------------------------------
 1 files changed, 112 insertions(+), 224 deletions(-)

diff --git a/drivers/char/nozomi.c b/drivers/char/nozomi.c
index 49e16c7..4a3ab38 100644
--- a/drivers/char/nozomi.c
+++ b/drivers/char/nozomi.c
@@ -172,9 +172,6 @@ static int debug;
 #define NOZOMI_NAME_TTY		"nozomi_tty"
 #define DRIVER_DESC		"Nozomi driver"
 
-#define NTTY_TTY_MAJOR		241
-#define NTTY_TTY_MINORS		MAX_PORT
-#define NTTY_TTY_MAXMINORS	256
 #define NTTY_FIFO_BUFFER_SIZE	8192
 
 /* Must be power of 2 */
@@ -225,8 +222,9 @@ static int debug;
 #define CTRL_DTR	0x0001
 #define CTRL_RTS	0x0002
 
-#define MAX_PORT		4
-#define NOZOMI_MAX_PORTS	5
+#define NOZOMI_MAX_PORTS	4
+#define NOZOMI_MAX_CARDS	(256 / NOZOMI_MAX_PORTS)
+#define NOZOMI_MAX_MINORS	(NOZOMI_MAX_PORTS * NOZOMI_MAX_CARDS)
 
 /*    Type definitions */
 
@@ -430,10 +428,9 @@ struct nozomi {
 	struct port port[NOZOMI_MAX_PORTS];
 	u8 *send_buf;
 
-	struct tty_driver *tty_driver;
-
 	spinlock_t spin_mutex;	/* secures access to registers and tty */
 
+	unsigned int index_start;
 	u32 open_ttys;
 };
 
@@ -443,9 +440,6 @@ struct buffer {
 	u8 *data;
 } __attribute__ ((packed));
 
-/*    Function declarations */
-static int ntty_tty_init(struct nozomi *dc);
-
 /*    Global variables */
 static struct pci_device_id nozomi_pci_tbl[] = {
 	{PCI_DEVICE(VENDOR1, DEVICE1)},
@@ -454,80 +448,22 @@ static struct pci_device_id nozomi_pci_tbl[] = {
 
 MODULE_DEVICE_TABLE(pci, nozomi_pci_tbl);
 
-/* Representing the pci device of interest */
-struct nozomi_devices {
-	struct list_head list;
-	struct nozomi *my_dev;
-	int index_start;
-};
-static atomic_t cards_found = ATOMIC_INIT(0);
-static LIST_HEAD(my_devices);
+static struct nozomi *ndevs[NOZOMI_MAX_CARDS];
+static struct tty_driver *ntty_driver;
 
 /*
  * find card by tty_index
  */
-static struct nozomi *get_dc_by_index(s32 index)
-{
-	struct list_head *p;
-	struct nozomi_devices *curdev;
-	int devidx;
-
-	if (likely(atomic_read(&cards_found) == 1)) {
-		curdev = list_first_entry(&my_devices,
-					struct nozomi_devices, list);
-		return curdev->my_dev;
-	} else {
-		devidx = index - (index % NTTY_TTY_MINORS);
 
-		list_for_each(p, &my_devices) {
-			curdev = list_entry(p, struct nozomi_devices, list);
-			if (curdev->index_start == devidx)
-				return curdev->my_dev;
-		}
-	}
-
-	printk(KERN_ALERT "Fatal error: could not find device" \
-		" for tty-index %d\n", index);
-
-	return NULL;
-}
-
-static struct port *get_port_by_tty(const struct tty_struct *tty)
+static inline struct nozomi *get_dc_by_tty(const struct tty_struct *tty)
 {
-	struct nozomi *ndev = get_dc_by_index(tty->index);
-	return ndev ? &ndev->port[tty->index % NTTY_TTY_MINORS] : NULL;
+	return ndevs[tty->index / NOZOMI_MAX_PORTS];
 }
 
-static struct nozomi *get_dc_by_tty(const struct tty_struct *tty)
+static inline struct port *get_port_by_tty(const struct tty_struct *tty)
 {
-	return get_dc_by_index(tty->index);
-}
-
-static int get_free_index(void)
-{
-	struct list_head *p;
-	struct nozomi_devices *curdev;
-	u8 busy;
-	int new_index;
-
-	for (new_index = 0; new_index < NTTY_TTY_MAXMINORS; new_index += 4) {
-		busy = 0;
-		list_for_each(p, &my_devices) {
-			curdev = list_entry(p, struct nozomi_devices, list);
-			if (curdev->index_start == new_index) {
-				++busy;
-				break;
-			}
-		}
-
-		if (!busy)
-			break;
-	}
-
-	if (new_index >= NTTY_TTY_MAXMINORS)
-		return -ENODEV;
-
-	return new_index;
+	struct nozomi *ndev = get_dc_by_tty(tty);
+	return ndev ? &ndev->port[tty->index % NOZOMI_MAX_PORTS] : NULL;
 }
 
 /*
@@ -822,7 +758,7 @@ static int nozomi_read_config_table(struct nozomi *dc)
 
 		dump_table(dc);
 
-		for (i = PORT_MDM; i < MAX_PORT; i++) {
+		for (i = PORT_MDM; i < NOZOMI_MAX_PORTS; i++) {
 			dc->port[i].fifo_ul =
 			    kfifo_alloc(FIFO_BUFFER_SIZE_UL, GFP_ATOMIC, NULL);
 			memset(&dc->port[i].ctrl_dl, 0, sizeof(struct ctrl_dl));
@@ -1234,7 +1170,7 @@ static int send_flow_control(struct nozomi *dc)
 	u32 i, more_flow_control_to_be_updated = 0;
 	u16 *ctrl;
 
-	for (i = PORT_MDM; i < MAX_PORT; i++) {
+	for (i = PORT_MDM; i < NOZOMI_MAX_PORTS; i++) {
 		if (dc->port[i].update_flow_control) {
 			if (more_flow_control_to_be_updated) {
 				/* We have more flow control to be updated */
@@ -1353,15 +1289,9 @@ static int handle_data_ul(struct nozomi *dc, enum port_type port, u16 read_iir)
 
 static irqreturn_t interrupt_handler(int irq, void *dev_id)
 {
-	struct nozomi_devices *ndev = dev_id;
-	struct nozomi *dc;
+	struct nozomi *dc = dev_id;
 	u16 read_iir;
 
-	if (!ndev)
-		return IRQ_NONE;
-
-	dc = ndev->my_dev;
-
 	spin_lock(&dc->spin_mutex);
 	read_iir = readw(dc->reg_iir);
 
@@ -1503,27 +1433,25 @@ static void nozomi_setup_private_data(struct nozomi *dc)
 	dc->port[PORT_APP1].token_dl = APP1_DL;
 	dc->port[PORT_APP2].token_dl = APP2_DL;
 
-	for (i = 0; i < MAX_PORT; i++)
+	for (i = 0; i < NOZOMI_MAX_PORTS; i++)
 		init_waitqueue_head(&dc->port[i].tty_wait);
 }
 
 static ssize_t card_type_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct nozomi_devices *deventry = pci_get_drvdata(pdev);
+	struct nozomi *dc = pci_get_drvdata(to_pci_dev(dev));
 
-	return sprintf(buf, "%d\n", deventry->my_dev->card_type);
+	return sprintf(buf, "%d\n", dc->card_type);
 }
 static DEVICE_ATTR(card_type, 0444, card_type_show, NULL);
 
 static ssize_t open_ttys_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct nozomi_devices *deventry = pci_get_drvdata(pdev);
+	struct nozomi *dc = pci_get_drvdata(to_pci_dev(dev));
 
-	return sprintf(buf, "%u\n", deventry->my_dev->open_ttys);
+	return sprintf(buf, "%u\n", dc->open_ttys);
 }
 static DEVICE_ATTR(open_ttys, 0444, open_ttys_show, NULL);
 
@@ -1548,35 +1476,36 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 				      const struct pci_device_id *ent)
 {
 	resource_size_t start;
-	int ret = -ENOMEM;
 	struct nozomi *dc = NULL;
-	struct nozomi_devices *newdev = NULL;
-	struct nozomi_devices *first = NULL;
-	int new_index;
-	int i;
+	unsigned int i, ndev_idx;
+	int ret;
 
-	atomic_inc(&cards_found);
-	dev_dbg(&pdev->dev, "Init, cards_found: %d\n",
-		atomic_read(&cards_found));
+	dev_dbg(&pdev->dev, "Init\n");
+
+	for (ndev_idx = 0; ndev_idx < ARRAY_SIZE(ndevs); ndev_idx++)
+		if (!ndevs[ndev_idx])
+			break;
+
+	if (ndev_idx >= ARRAY_SIZE(ndevs)) {
+		dev_err(&pdev->dev, "no free tty range for this card\n");
+		ret = -EIO;
+		goto err;
+	}
 
 	dc = kzalloc(sizeof(struct nozomi), GFP_KERNEL);
 	if (unlikely(!dc)) {
 		dev_err(&pdev->dev, "Could not allocate memory\n");
-		goto err_free;
-	}
-	newdev = kzalloc(sizeof(struct nozomi_devices), GFP_KERNEL);
-	if (unlikely(!newdev)) {
-		dev_err(&pdev->dev, "Could not allocate memory\n");
+		ret = -ENOMEM;
 		goto err_free;
 	}
 
 	dc->pdev = pdev;
-	newdev->my_dev = dc;
 
 	/* Find out what card type it is */
 	nozomi_get_card_type(dc);
 
-	if (pci_enable_device(dc->pdev)) {
+	ret = pci_enable_device(dc->pdev);
+	if (ret) {
 		dev_err(&pdev->dev, "Failed to enable PCI Device\n");
 		goto err_free;
 	}
@@ -1618,7 +1547,7 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 	writew(dc->last_ier, dc->reg_ier);
 
 	ret = request_irq(pdev->irq, &interrupt_handler, IRQF_SHARED,
-			NOZOMI_NAME, newdev);
+			NOZOMI_NAME, dc);
 	if (unlikely(ret)) {
 		dev_err(&pdev->dev, "can't request irq\n");
 		goto err_free_sbuf;
@@ -1626,53 +1555,26 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 
 	DBG1("base_addr: %p", dc->base_addr);
 
-	new_index = get_free_index();
-	if (new_index < 0) {
-		dev_err(&pdev->dev, "already reached maximum card count.\n");
-		ret = -EIO;
-		goto err_free_irq;
-	}
-
 	make_sysfs_files(dc);
 
-	if (atomic_read(&cards_found) == 1) {
-		ret = ntty_tty_init(dc);
-		if (ret) {
-			dev_err(&pdev->dev, "can't alloc ntty\n");
-			goto err_sysfs;
-		}
-	} else {
-		first = list_first_entry(&my_devices, struct nozomi_devices,
-									list);
-		dc->tty_driver = first->my_dev->tty_driver;
-	}
-
-	for (i = 0; i < NTTY_TTY_MINORS; i++) {
+	dc->index_start = ndev_idx * NOZOMI_MAX_PORTS;
+	ndevs[ndev_idx] = dc;
+	for (i = 0; i < NOZOMI_MAX_PORTS; i++) {
 		init_MUTEX(&dc->port[i].tty_sem);
 		dc->port[i].tty_open_count = 0;
 		dc->port[i].tty = NULL;
-		tty_register_device(dc->tty_driver, new_index + i,
-							&dc->pdev->dev);
+		tty_register_device(ntty_driver, dc->index_start + i,
+							&pdev->dev);
 	}
-	newdev->index_start = new_index;
 
 	/* Enable  RESET interrupt. */
 	dc->last_ier = RESET;
 	writew(dc->last_ier, dc->reg_ier);
 
-	list_add_tail(&newdev->list, &my_devices);
-
-	pci_set_drvdata(pdev, newdev);
+	pci_set_drvdata(pdev, dc);
 
 	return 0;
 
-err_sysfs:
-	remove_sysfs_files(dc);
-err_free_irq:
-	free_irq(pdev->irq, newdev);
-	for (i = PORT_MDM; i < MAX_PORT; i++)	/* allocated in isr, might be */
-		if (dc->port[i].fifo_ul)	/* filled yet */
-			kfifo_free(dc->port[i].fifo_ul);
 err_free_sbuf:
 	kfree(dc->send_buf);
 	iounmap(dc->base_addr);
@@ -1682,40 +1584,25 @@ err_disable_device:
 	pci_disable_device(pdev);
 err_free:
 	kfree(dc);
-	kfree(newdev);
-	atomic_dec(&cards_found);
+err:
 	return ret;
 }
 
-static void __devexit tty_exit(struct nozomi_devices *ndev)
+static void __devexit tty_exit(struct nozomi *dc)
 {
-	struct nozomi *dc = ndev->my_dev;
-	int i, ret;
+	unsigned int i;
 
 	DBG1(" ");
 
-	flush_scheduled_work();
-
-	for (i = 0; i < NTTY_TTY_MINORS; ++i)
-		if (dc->port[i].tty && \
-				list_empty(&dc->port[i].tty->hangup_work.entry))
+	for (i = 0; i < NOZOMI_MAX_PORTS; i++)
+		if (dc->port[i].tty)
 			tty_hangup(dc->port[i].tty);
 
 	while (dc->open_ttys)
 		msleep(1);
 
-	for (i = ndev->index_start; i < ndev->index_start + NTTY_TTY_MINORS; \
-									++i)
-		tty_unregister_device(dc->tty_driver, i);
-
-	/* only unregister ttydriver if its the last card available */
-	if (atomic_read(&cards_found) == 1) {
-		ret = tty_unregister_driver(dc->tty_driver);
-		if (ret)
-			printk(KERN_ERR "Unable to unregister the tty driver !"
-							" (%d)\n", ret);
-		put_tty_driver(dc->tty_driver);
-	}
+	for (i = dc->index_start; i < dc->index_start + NOZOMI_MAX_PORTS; ++i)
+ 		tty_unregister_device(ntty_driver, i);
 }
 
 /* Deallocate memory for one device */
@@ -1723,14 +1610,13 @@ static void __devexit nozomi_card_exit(struct pci_dev *pdev)
 {
 	int i;
 	struct ctrl_ul ctrl;
-	struct nozomi_devices *deventry = pci_get_drvdata(pdev);
-	struct nozomi *dc = deventry->my_dev;
+	struct nozomi *dc = pci_get_drvdata(pdev);
 
 	/* Disable all interrupts */
 	dc->last_ier = 0;
 	writew(dc->last_ier, dc->reg_ier);
 
-	tty_exit(deventry);
+	tty_exit(dc);
 
 	/* Send 0x0001, command card to resend the reset token.  */
 	/* This is to get the reset when the module is reloaded. */
@@ -1744,13 +1630,11 @@ static void __devexit nozomi_card_exit(struct pci_dev *pdev)
 	write_mem32(dc->port[PORT_CTRL].ul_addr[0], (u32 *)&ctrl, 2);
 	writew(CTRL_UL, dc->reg_fcr);	/* push the token to the card. */
 
-	list_del(&deventry->list);
-
 	remove_sysfs_files(dc);
 
-	free_irq(pdev->irq, deventry);
+	free_irq(pdev->irq, dc);
 
-	for (i = PORT_MDM; i < MAX_PORT; i++)
+	for (i = 0; i < NOZOMI_MAX_PORTS; i++)
 		if (dc->port[i].fifo_ul)
 			kfifo_free(dc->port[i].fifo_ul);
 
@@ -1762,32 +1646,29 @@ static void __devexit nozomi_card_exit(struct pci_dev *pdev)
 
 	pci_disable_device(pdev);
 
-	kfree(dc);
-	kfree(deventry);
+	ndevs[dc->index_start / NOZOMI_MAX_PORTS] = NULL;
 
-	atomic_dec(&cards_found);
+	kfree(dc);
 }
 
-static void set_rts(int index, int rts)
+static void set_rts(struct tty_struct *tty, int rts)
 {
-	struct nozomi *dc = get_dc_by_index(index);
-	struct port *port = &dc->port[index % MAX_PORT];
+	struct port *port = get_port_by_tty(tty);
 
 	port->ctrl_ul.RTS = rts;
 	port->update_flow_control = 1;
-	enable_transmit_ul(PORT_CTRL, dc);
+	enable_transmit_ul(PORT_CTRL, get_dc_by_tty(tty));
 }
 
-static void set_dtr(int index, int dtr)
+static void set_dtr(struct tty_struct *tty, int dtr)
 {
-	struct nozomi *dc = get_dc_by_index(index);
-	struct port *port = &dc->port[index % MAX_PORT];
+	struct port *port = get_port_by_tty(tty);
 
-	DBG1("SETTING DTR index: %d, dtr: %d", index, dtr);
+	DBG1("SETTING DTR index: %d, dtr: %d", tty->index, dtr);
 
 	port->ctrl_ul.DTR = dtr;
 	port->update_flow_control = 1;
-	enable_transmit_ul(PORT_CTRL, dc);
+	enable_transmit_ul(PORT_CTRL, get_dc_by_tty(tty));
 }
 
 /*
@@ -1906,13 +1787,13 @@ static int ntty_write(struct tty_struct *tty, const unsigned char *buffer,
 	if (port == &(dc->port[PORT_MDM])) {
 		if (port->ctrl_dl.CTS) {
 			DBG4("Enable interrupt");
-			enable_transmit_ul(tty->index % MAX_PORT, dc);
+			enable_transmit_ul(tty->index % NOZOMI_MAX_PORTS, dc);
 		} else {
 			dev_err(&dc->pdev->dev,
 				"CTS not active on modem port?\n");
 		}
 	} else {
-		enable_transmit_ul(tty->index % MAX_PORT, dc);
+		enable_transmit_ul(tty->index % NOZOMI_MAX_PORTS, dc);
 	}
 	spin_unlock_irqrestore(&dc->spin_mutex, flags);
 
@@ -1968,14 +1849,14 @@ static int ntty_tiocmset(struct tty_struct *tty, struct file *file,
 	unsigned int set, unsigned int clear)
 {
 	if (set & TIOCM_RTS)
-		set_rts(tty->index, 1);
+		set_rts(tty, 1);
 	else if (clear & TIOCM_RTS)
-		set_rts(tty->index, 0);
+		set_rts(tty, 0);
 
 	if (set & TIOCM_DTR)
-		set_dtr(tty->index, 1);
+		set_dtr(tty, 1);
 	else if (clear & TIOCM_DTR)
-		set_dtr(tty->index, 0);
+		set_dtr(tty, 0);
 
 	return 0;
 }
@@ -2054,8 +1935,8 @@ static void ntty_unthrottle(struct tty_struct *tty)
 
 	DBG1("UNTHROTTLE");
 	spin_lock_irqsave(&dc->spin_mutex, flags);
-	enable_transmit_dl(tty->index % MAX_PORT, dc);
-	set_rts(tty->index, 1);
+	enable_transmit_dl(tty->index % NOZOMI_MAX_PORTS, dc);
+	set_rts(tty, 1);
 	spin_unlock_irqrestore(&dc->spin_mutex, flags);
 }
 
@@ -2070,7 +1951,7 @@ static void ntty_throttle(struct tty_struct *tty)
 
 	DBG1("THROTTLE");
 	spin_lock_irqsave(&dc->spin_mutex, flags);
-	set_rts(tty->index, 0);
+	set_rts(tty, 0);
 	spin_unlock_irqrestore(&dc->spin_mutex, flags);
 }
 
@@ -2119,39 +2000,6 @@ static struct tty_operations tty_ops = {
 	.tiocmset = ntty_tiocmset,
 };
 
-/* Initializes the tty */
-static int ntty_tty_init(struct nozomi *dc)
-{
-	struct tty_driver *td;
-	int rval;
-
-	dc->tty_driver = alloc_tty_driver(NTTY_TTY_MAXMINORS);
-	if (!dc->tty_driver)
-		return -ENOMEM;
-	td = dc->tty_driver;
-	td->owner = THIS_MODULE;
-	td->driver_name = NOZOMI_NAME_TTY;
-	td->name = "noz";
-	td->major = NTTY_TTY_MAJOR;
-	td->type = TTY_DRIVER_TYPE_SERIAL;
-	td->subtype = SERIAL_TYPE_NORMAL;
-	td->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
-	td->init_termios = tty_std_termios;
-	td->init_termios.c_cflag = B115200 | CS8 | CREAD | HUPCL | CLOCAL;
-	td->init_termios.c_ispeed = 115200;
-	td->init_termios.c_ospeed = 115200;
-	tty_set_operations(dc->tty_driver, &tty_ops);
-
-	rval = tty_register_driver(td);
-	if (rval) {
-		dev_err(&dc->pdev->dev, "failed to register ntty tty driver\n");
-		return rval;
-	}
-
-	dev_info(&dc->pdev->dev, DRIVER_DESC " " NOZOMI_NAME_TTY "\n");
-	return rval;
-}
-
 /* Module initialization */
 static struct pci_driver nozomi_driver = {
 	.name = NOZOMI_NAME,
@@ -2162,14 +2010,54 @@ static struct pci_driver nozomi_driver = {
 
 static __init int nozomi_init(void)
 {
+	int ret;
+
 	printk(KERN_INFO "Initializing %s\n", VERSION_STRING);
-	return pci_register_driver(&nozomi_driver);
+
+	ntty_driver = alloc_tty_driver(NOZOMI_MAX_MINORS);
+	if (!ntty_driver)
+		return -ENOMEM;
+
+	ntty_driver->owner = THIS_MODULE;
+	ntty_driver->driver_name = NOZOMI_NAME_TTY;
+	ntty_driver->name = "noz";
+	ntty_driver->major = 0;
+	ntty_driver->type = TTY_DRIVER_TYPE_SERIAL;
+	ntty_driver->subtype = SERIAL_TYPE_NORMAL;
+	ntty_driver->flags = TTY_DRIVER_REAL_RAW | TTY_DRIVER_DYNAMIC_DEV;
+	ntty_driver->init_termios = tty_std_termios;
+	ntty_driver->init_termios.c_cflag = B115200 | CS8 | CREAD | HUPCL |
+		CLOCAL;
+	ntty_driver->init_termios.c_ispeed = 115200;
+	ntty_driver->init_termios.c_ospeed = 115200;
+	tty_set_operations(ntty_driver, &tty_ops);
+
+	ret = tty_register_driver(ntty_driver);
+	if (ret) {
+		printk(KERN_ERR "Nozomi: failed to register ntty driver\n");
+		goto free_tty;
+	}
+
+	ret = pci_register_driver(&nozomi_driver);
+	if (ret) {
+		printk(KERN_ERR "Nozomi: can't register pci driver\n");
+		goto unr_tty;
+	}
+
+	return 0;
+unr_tty:
+	tty_unregister_driver(ntty_driver);
+free_tty:
+	put_tty_driver(ntty_driver);
+	return ret;
 }
 
 static __exit void nozomi_exit(void)
 {
 	printk(KERN_INFO "Unloading %s\n", DRIVER_DESC);
 	pci_unregister_driver(&nozomi_driver);
+	tty_unregister_driver(ntty_driver);
+	put_tty_driver(ntty_driver);
 }
 
 module_init(nozomi_init);

Re: [RFC 8/13] Char: nozomi, tty cleanup

[Frank Seidel]

On Samstag 10 November 2007 00:48:11, you (Jiri Slaby) wrote:
> nozomi, tty cleanup
> 
> - init and deinit tty driver at module load/unload. When the OS (user)
>   loads the driver the hardware usually is ready to driver.
> - merge (unify) MAX_PORT, NTTY_TTY_MINORS into NOZOMI_MAX_PORTS
> - remove struct nozomi_devices, it was used only as list entries

Oh, i fear this patch makes some issues (e.g. changing NOZOMI_MAX_PORTS
without adapting all its usages).. as this one (together with [RFC 7/13])
is quite big and i really don't wanna loose the multicard support (which
i also cannot test yet here at home), 
i'd defer those two until next week when i get a chance to test and have
more time.

Thanks,
Frank

Re: [RFC 8/13] Char: nozomi, tty cleanup

[Frank Seidel]

On Samstag 10 November 2007 00:48:11, you (Jiri Slaby) wrote:
> nozomi, tty cleanup
> - init and deinit tty driver at module load/unload. When the OS (user)
>   loads the driver the hardware usually is ready to driver.
> - merge (unify) MAX_PORT, NTTY_TTY_MINORS into NOZOMI_MAX_PORTS
> - remove struct nozomi_devices, it was used only as list entries

This one took a little bit longer to review and test in depth, but
i finally made it.
I really find this a really good rework, i did only a few little
very minor changes (see comments below).
Please take my comments with a big pinch of salt (as i really am very
passionate to become a kernel hacker but still lack too much knowledge
and experience in many areas).

> ...
> @@ -172,9 +172,6 @@ static int debug;
>  ...
> -#define NTTY_TTY_MINORS		MAX_PORT
> -#define NTTY_TTY_MAXMINORS	256
>  ...
> -#define MAX_PORT		4
> -#define NOZOMI_MAX_PORTS	5
> +#define NOZOMI_MAX_PORTS	4
> +#define NOZOMI_MAX_CARDS	(256 / NOZOMI_MAX_PORTS)
> +#define NOZOMI_MAX_MINORS	(NOZOMI_MAX_PORTS * NOZOMI_MAX_CARDS)

I don't see the benefit in indirectly defining the maxminors in maxcards
and count back then. Perhaps this is cleaner/better but i just don't
understand it (now). So, i just did a additional
#define NOZOMI_MAX_CARDS (NTTY_TTY_MAXMINORS / NTTY_TTY_MINORS)
which seamed more straightforward to me and didn't break anything (as the
change to NOZOMI_MAX_PORTS currently would).
But to at least merge it a bit i removed the NTTY_TTY_MINORS alias for
MAX_PORT (and of course replaced usages with MAX_PORT).

>  ...
> -		for (i = PORT_MDM; i < MAX_PORT; i++) {
> +		for (i = PORT_MDM; i < NOZOMI_MAX_PORTS; i++) {

Those changes e.g. aren't needed when just adding the one define as i
suggested above.
All other comments vanished after i studied them better :-)

I'll repost the complete patch in a few moments (in CC to Greg again).

Thanks so much for this,
Frank

[RFC 9/13] Char: nozomi, lock cleanup

[Jiri Slaby]

nozomi, lock cleanup

- semaphore is deprecated, use mutex instead
- don't return -ERESTARTSYS when signal might not be pending since it's not
  permitted (unknown retval mioght reach userspace)
- don't lock interruptible in close or the card might not be stopped on last
  close

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit 19a0196a97ed70efdc421b359e28684e058d7121
tree 1c65691920ac6efddc7d9e221ee302a0b32b2831
parent d0b01ce89a7b18ba37ea6192eea6a98cdc01d62e
author Jiri Slaby <jirislaby@gmail.com> Mon, 05 Nov 2007 13:53:39 +0100
committer Jiri Slaby <jirislaby@gmail.com> Sat, 10 Nov 2007 00:13:46 +0100

 drivers/char/nozomi.c |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/char/nozomi.c b/drivers/char/nozomi.c
index 4a3ab38..2c4d388 100644
--- a/drivers/char/nozomi.c
+++ b/drivers/char/nozomi.c
@@ -406,7 +406,7 @@ struct port {
 
 	struct tty_struct *tty;
 	int tty_open_count;
-	struct semaphore tty_sem;
+	struct mutex tty_sem;
 	wait_queue_head_t tty_wait;
 	struct async_icount tty_icount;
 };
@@ -1560,7 +1560,7 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 	dc->index_start = ndev_idx * NOZOMI_MAX_PORTS;
 	ndevs[ndev_idx] = dc;
 	for (i = 0; i < NOZOMI_MAX_PORTS; i++) {
-		init_MUTEX(&dc->port[i].tty_sem);
+		mutex_init(&dc->port[i].tty_sem);
 		dc->port[i].tty_open_count = 0;
 		dc->port[i].tty = NULL;
 		tty_register_device(ntty_driver, dc->index_start + i,
@@ -1687,7 +1687,7 @@ static int ntty_open(struct tty_struct *tty, struct file *file)
 	if (!port || !dc)
 		return -ENODEV;
 
-	if (down_interruptible(&port->tty_sem))
+	if (mutex_lock_interruptible(&port->tty_sem))
 		return -ERESTARTSYS;
 
 	port->tty_open_count++;
@@ -1706,7 +1706,7 @@ static int ntty_open(struct tty_struct *tty, struct file *file)
 		spin_unlock_irqrestore(&dc->spin_mutex, flags);
 	}
 
-	up(&port->tty_sem);
+	mutex_unlock(&port->tty_sem);
 
 	return 0;
 }
@@ -1721,8 +1721,7 @@ static void ntty_close(struct tty_struct *tty, struct file *file)
 	if (!dc || !port)
 		return;
 
-	if (down_interruptible(&port->tty_sem))
-		return;
+	mutex_lock(&port->tty_sem);
 
 	if (!port->tty_open_count)
 		goto exit;
@@ -1739,7 +1738,7 @@ static void ntty_close(struct tty_struct *tty, struct file *file)
 	}
 
 exit:
-	up(&port->tty_sem);
+	mutex_unlock(&port->tty_sem);
 }
 
 /*
@@ -1759,14 +1758,14 @@ static int ntty_write(struct tty_struct *tty, const unsigned char *buffer,
 	if (!dc || !port)
 		return -ENODEV;
 
-	if (unlikely(down_trylock(&port->tty_sem))) {
+	if (unlikely(mutex_trylock(&port->tty_sem))) {
 		/*
 		 * must test lock as tty layer wraps calls
 		 * to this function with BKL
 		 */
 		dev_err(&dc->pdev->dev, "Would have deadlocked - "
-			"return ERESTARTSYS\n");
-		return -ERESTARTSYS;
+			"return EAGAIN\n");
+		return -EAGAIN;
 	}
 
 	if (unlikely(!port->tty_open_count)) {
@@ -1798,7 +1797,7 @@ static int ntty_write(struct tty_struct *tty, const unsigned char *buffer,
 	spin_unlock_irqrestore(&dc->spin_mutex, flags);
 
 exit:
-	up(&port->tty_sem);
+	mutex_unlock(&port->tty_sem);
 	return rval;
 }
 
@@ -1816,7 +1815,7 @@ static int ntty_write_room(struct tty_struct *tty)
 
 	if (!dc || !port)
 		return 0;
-	if (down_trylock(&port->tty_sem))
+	if (mutex_trylock(&port->tty_sem))
 		return 0;
 
 	if (!port->tty_open_count)
@@ -1825,7 +1824,7 @@ static int ntty_write_room(struct tty_struct *tty)
 	room = port->fifo_ul->size - __kfifo_len(port->fifo_ul);
 
 exit:
-	up(&port->tty_sem);
+	mutex_unlock(&port->tty_sem);
 	return room;
 }
 

Re: [RFC 9/13] Char: nozomi, lock cleanup

[Frank Seidel]

On Samstag 10 November 2007 00:48:52, you (Jiri Slaby) wrote:
> nozomi, lock cleanup
> 
> - semaphore is deprecated, use mutex instead
> - don't return -ERESTARTSYS when signal might not be pending since it's not
>   permitted (unknown retval mioght reach userspace)
> - don't lock interruptible in close or the card might not be stopped on last
>   close

Good catches. But i had to change it a bit, especially the mutex_trylock.
>From mutex.c: "..it is negated to the down_trylock() return values!
Be careful about this when converting semaphores to mutexes." ;-)
Now it works again (doesn't deadlock my card anymore).

Thanks,
Frank

[RFC! 10/13] Char: nozomi, fix tty_flip_buffer_push

[Jiri Slaby]

Especially on this I would like to see feedback. Unlock and lock the
spinlock just around the tty_flip_buffer_push would be much more easier, but
won't it break anything?

--

nozomi, fix tty_flip_buffer_push

tty_flip_buffer_push call may deadlock when invoking it while holding
spinlock used in e.g. throttle too. Nozomi sets low_latency tty and that
means, that ldisc flush will be called immediately and it can call some
nozomi functions back. Solve it by invoking tty_flip_buffer_push after
releasing the spinlock.

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit 2bd359aea85cf712d4d161a83e940fe5e1202ee8
tree 0de7cbb0d78d1ff24d6be957b85ee30d4fc01a63
parent 19a0196a97ed70efdc421b359e28684e058d7121
author Jiri Slaby <jirislaby@gmail.com> Mon, 05 Nov 2007 15:19:55 +0100
committer Jiri Slaby <jirislaby@gmail.com> Sat, 10 Nov 2007 00:14:41 +0100

 drivers/char/nozomi.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/char/nozomi.c b/drivers/char/nozomi.c
index 2c4d388..3e0b18e 100644
--- a/drivers/char/nozomi.c
+++ b/drivers/char/nozomi.c
@@ -414,7 +414,7 @@ struct port {
 /* Private data one for each card in the system */
 struct nozomi {
 	void __iomem *base_addr;
-	u8 closing;
+	u32 flip;
 
 	/* Pointers to registers */
 	void __iomem *reg_iir;
@@ -1000,7 +1000,7 @@ static int receive_data(enum port_type index, struct nozomi *dc)
 		}
 	}
 
-	tty_flip_buffer_push(tty);
+	set_bit(index, &dc->flip);
 
 	return 1;
 }
@@ -1290,6 +1290,7 @@ static int handle_data_ul(struct nozomi *dc, enum port_type port, u16 read_iir)
 static irqreturn_t interrupt_handler(int irq, void *dev_id)
 {
 	struct nozomi *dc = dev_id;
+	unsigned int a;
 	u16 read_iir;
 
 	spin_lock(&dc->spin_mutex);
@@ -1397,6 +1398,9 @@ static irqreturn_t interrupt_handler(int irq, void *dev_id)
 
 exit_handler:
 	spin_unlock(&dc->spin_mutex);
+	for (a = 0; a < NOZOMI_MAX_PORTS; a++)
+		if (test_and_clear_bit(a, &dc->flip))
+			tty_flip_buffer_push(dc->port[a].tty);
 	return IRQ_HANDLED;
 none:
 	spin_unlock(&dc->spin_mutex);
@@ -1426,7 +1430,7 @@ static void nozomi_setup_private_data(struct nozomi *dc)
 	dc->reg_iir = (void __iomem *)(offset + R_IIR);
 	dc->reg_ier = (void __iomem *)(offset + R_IER);
 	dc->last_ier = 0;
-	dc->closing = 0;
+	dc->flip = 0;
 
 	dc->port[PORT_MDM].token_dl = MDM_DL;
 	dc->port[PORT_DIAG].token_dl = DIAG_DL;

Re: [RFC! 10/13] Char: nozomi, fix tty_flip_buffer_push

[Frank Seidel]

On Samstag 10 November 2007 00:49:32, you (Jiri Slaby) wrote:
> Especially on this I would like to see feedback. Unlock and lock the
> spinlock just around the tty_flip_buffer_push would be much more easier, but
> won't it break anything?
> --
> nozomi, fix tty_flip_buffer_push
> 
> tty_flip_buffer_push call may deadlock when invoking it while holding
> spinlock used in e.g. throttle too. Nozomi sets low_latency tty and that
> means, that ldisc flush will be called immediately and it can call some
> nozomi functions back. Solve it by invoking tty_flip_buffer_push after
> releasing the spinlock.

IMHO its a brilliant idea to defer it this way to a safe place. I just
had to adapt the type of flip to "usigned long" (otherwise gcc spits
warnings around).
Eventhough i never ran in such a deadlock of course it looks much safer
now this way. I tested this patch and it seems to not break anything.

Applied without changes.

Thanks a lot,
Frank

[RFC 11/13] Char: nozomi, remove unused includes

[Jiri Slaby]

nozomi, remove unused includes

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit 1ab5d64fc4a2dd7a50b8971c67b9793511b4c47a
tree d169718adab1aaf82ee655dc2048535eae3742a7
parent 2bd359aea85cf712d4d161a83e940fe5e1202ee8
author Jiri Slaby <jirislaby@gmail.com> Mon, 05 Nov 2007 15:25:18 +0100
committer Jiri Slaby <jirislaby@gmail.com> Sat, 10 Nov 2007 00:14:51 +0100

 drivers/char/nozomi.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/char/nozomi.c b/drivers/char/nozomi.c
index 3e0b18e..e87ded2 100644
--- a/drivers/char/nozomi.c
+++ b/drivers/char/nozomi.c
@@ -101,9 +101,7 @@
 #include <linux/kmod.h>
 #include <linux/init.h>
 #include <linux/kfifo.h>
-#include <linux/list.h>
 #include <linux/uaccess.h>
-#include <asm/atomic.h>
 
 #include <linux/delay.h>
 

Re: [RFC 11/13] Char: nozomi, remove unused includes

[Frank Seidel]

On Samstag 10 November 2007 00:50:12, you (Jiri Slaby) wrote:
> nozomi, remove unused includes
> 
defered those (to review next week together with patch 7 and 8)

Thanks,
Frank

[RFC 12/13] Char: nozomi, remove void acc char2 char3 more mp mp.c mp.yy m1 nozomi2 proto rej slock1 casts

[Jiri Slaby]

nozomi, remove void * casts

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit 92ca82bcbd5dde95096dac24d0f6a9beab68e003
tree 748ac937f772410b364245897492b693491743f6
parent 1ab5d64fc4a2dd7a50b8971c67b9793511b4c47a
author Jiri Slaby <jirislaby@gmail.com> Mon, 05 Nov 2007 15:28:46 +0100
committer Jiri Slaby <jirislaby@gmail.com> Sat, 10 Nov 2007 00:15:05 +0100

 drivers/char/nozomi.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/char/nozomi.c b/drivers/char/nozomi.c
index e87ded2..0735df0 100644
--- a/drivers/char/nozomi.c
+++ b/drivers/char/nozomi.c
@@ -1752,7 +1752,7 @@ static int ntty_write(struct tty_struct *tty, const unsigned char *buffer,
 {
 	int rval = -EINVAL;
 	struct nozomi *dc = get_dc_by_tty(tty);
-	struct port *port = (struct port *)tty->driver_data;
+	struct port *port = tty->driver_data;
 	unsigned long flags;
 
 	/* DBG1( "WRITEx: %d, index = %d", count, index); */
@@ -1811,7 +1811,7 @@ exit:
  */
 static int ntty_write_room(struct tty_struct *tty)
 {
-	struct port *port = (struct port *)tty->driver_data;
+	struct port *port = tty->driver_data;
 	int room = 0;
 	struct nozomi *dc = get_dc_by_tty(tty);
 
@@ -1966,7 +1966,7 @@ static void ntty_put_char(struct tty_struct *tty, unsigned char c)
 /* Returns number of chars in buffer, called by tty layer */
 static s32 ntty_chars_in_buffer(struct tty_struct *tty)
 {
-	struct port *port = (struct port *)tty->driver_data;
+	struct port *port = tty->driver_data;
 	struct nozomi *dc = get_dc_by_tty(tty);
 	s32 rval;
 

Re: [RFC 12/13] Char: nozomi, remove void acc char2 char3 more mp mp.c mp.yy m1 nozomi2 proto rej slock1 casts

[Frank Seidel]

On Samstag 10 November 2007 00:50:53, you (Jiri Slaby) wrote:
> nozomi, remove void * casts

Yes, they are of course pointless. Applied your patch
without changes.

Thanks,
Frank

[RFC 13/13] Char: nozomi, cleanup read and write

[Jiri Slaby]

nozomi, cleanup read and write

- remove macros testing for endianness, le*_to_cpu and complements will do
  it
- pointers cleanup (no need to have different pointers for be and le)
- put unlikely into reading/writing while loop, because it will proceed
  through this case only on last two bytes
- also fix typos in write, le16_to_cpu instead of cpu_to_le16 for 2-byte
  writes

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit fe27da2c4a35d2f91fd1ed542d91353f7a3c1dd2
tree a9f8d35b2047a24277d92758d3d579eb59427323
parent 92ca82bcbd5dde95096dac24d0f6a9beab68e003
author Jiri Slaby <jirislaby@gmail.com> Sat, 10 Nov 2007 00:19:04 +0100
committer Jiri Slaby <jirislaby@gmail.com> Sat, 10 Nov 2007 00:19:04 +0100

 drivers/char/nozomi.c |   80 ++++++++++---------------------------------------
 1 files changed, 16 insertions(+), 64 deletions(-)

diff --git a/drivers/char/nozomi.c b/drivers/char/nozomi.c
index 0735df0..7379cd4 100644
--- a/drivers/char/nozomi.c
+++ b/drivers/char/nozomi.c
@@ -105,6 +105,7 @@
 
 #include <linux/delay.h>
 
+#include <asm/byteorder.h>
 
 #define VERSION_STRING DRIVER_DESC " 2.1c (build date: " \
 					__DATE__ " " __TIME__ ")"
@@ -261,8 +262,7 @@ enum port_type {
 	PORT_ERROR	= -1,
 };
 
-#ifdef __ARMEB__
-/* Big endian */
+#ifdef __BIG_ENDIAN
 
 struct toggles {
 	unsigned enabled:5;	/*
@@ -470,16 +470,10 @@ static inline struct port *get_port_by_tty(const struct tty_struct *tty)
  * -Rewrite cleaner
  */
 
-static void read_mem32(u32 *buf, const void __iomem *mem_addr_start,
-			u32 size_bytes)
+static void read_mem32(u32 *buf, void __iomem *ptr, u32 size_bytes)
 {
-	u32 i = 0;
-#ifdef __ARMEB__
-	const u32 *ptr = (u32 *) mem_addr_start;
-#else
-	const u32 *ptr = (__force u32 *) mem_addr_start;
-#endif
 	u16 *buf16;
+	u32 i = 0;
 
 	if (unlikely(!ptr || !buf))
 		goto out;
@@ -488,40 +482,22 @@ static void read_mem32(u32 *buf, const void __iomem *mem_addr_start,
 	switch (size_bytes) {
 	case 2:	/* 2 bytes */
 		buf16 = (u16 *) buf;
-#ifdef __ARMEB__
-		*buf16 = __le16_to_cpu(readw(ptr));
-#else
-		*buf16 = readw((void __iomem *)ptr);
-#endif
+		*buf16 = le16_to_cpu(readw(ptr));
 		goto out;
-		break;
 	case 4:	/* 4 bytes */
-#ifdef __ARMEB__
-		*(buf) = __le32_to_cpu(readl(ptr));
-#else
-		*(buf) = readl((void __iomem *)ptr);
-#endif
+		*buf = le32_to_cpu(readl(ptr));
 		goto out;
-		break;
 	}
 
 	while (i < size_bytes) {
-		if (size_bytes - i == 2) {
+		if (unlikely(size_bytes - i == 2)) {
 			/* Handle 2 bytes in the end */
 			buf16 = (u16 *) buf;
-#ifdef __ARMEB__
-			*(buf16) = __le16_to_cpu(readw(ptr));
-#else
-			*(buf16) = readw((void __iomem *)ptr);
-#endif
+			*buf16 = le16_to_cpu(readw(ptr));
 			i += 2;
 		} else {
 			/* Read 4 bytes */
-#ifdef __ARMEB__
-			*(buf) = __le32_to_cpu(readl(ptr));
-#else
-			*(buf) = readl((void __iomem *)ptr);
-#endif
+			*buf = le32_to_cpu(readl(ptr));
 			i += 4;
 		}
 		buf++;
@@ -536,15 +512,9 @@ out:
  * -Optimize
  * -Rewrite cleaner
  */
-static u32 write_mem32(void __iomem *mem_addr_start, u32 *buf,
-			u32 size_bytes)
+static u32 write_mem32(void __iomem *ptr, u32 *buf, u32 size_bytes)
 {
 	u32 i = 0;
-#ifdef __ARMEB__
-	u32 *ptr = (u32 *) mem_addr_start;
-#else
-	u32 *ptr = (__force u32 *) mem_addr_start;
-#endif
 	u16 *buf16;
 
 	if (unlikely(!ptr || !buf))
@@ -553,41 +523,23 @@ static u32 write_mem32(void __iomem *mem_addr_start, u32 *buf,
 	/* shortcut for extremely often used cases */
 	switch (size_bytes) {
 	case 2:	/* 2 bytes */
-		buf16 = (u16 *) buf;
-#ifdef __ARMEB__
-		writew(__le16_to_cpu(*buf16), ptr);
-#else
-		writew(*buf16, (void __iomem *)ptr);
-#endif
+		buf16 = (u16 *)buf;
+		writew(cpu_to_le16(*buf16), ptr);
 		return 2;
-		break;
 	case 4: /* 4 bytes */
-#ifdef __ARMEB__
-		writel(__cpu_to_le32(*buf), ptr);
-#else
-		writel(*buf, (void __iomem *)ptr);
-#endif
+		writel(cpu_to_le32(*buf), ptr);
 		return 4;
-		break;
 	}
 
 	while (i < size_bytes) {
-		if (size_bytes - i == 2) {
+		if (unlikely(size_bytes - i == 2)) {
 			/* 2 bytes */
 			buf16 = (u16 *) buf;
-#ifdef __ARMEB__
-			writew(__le16_to_cpu(*buf16), ptr);
-#else
-			writew(*buf16, (void __iomem *)ptr);
-#endif
+			writew(cpu_to_le16(*buf16), ptr);
 			i += 2;
 		} else {
 			/* 4 bytes */
-#ifdef __ARMEB__
-			writel(__cpu_to_le32(*buf), ptr);
-#else
-			writel(*buf, (void __iomem *)ptr);
-#endif
+			writel(cpu_to_le32(*buf), ptr);
 			i += 4;
 		}
 		buf++;

Re: [RFC 13/13] Char: nozomi, cleanup read and write

[Frank Seidel]

On Samstag 10 November 2007 00:51:33, you (Jiri Slaby) wrote:
> nozomi, cleanup read and write
> 
> - remove macros testing for endianness, le*_to_cpu and complements will do
>   it
> - pointers cleanup (no need to have different pointers for be and le)
> - put unlikely into reading/writing while loop, because it will proceed
>   through this case only on last two bytes
> - also fix typos in write, le16_to_cpu instead of cpu_to_le16 for 2-byte
>   writes

Even though this looks much better it does not work :-(
With this patch applied the card goes "crazy". *sigh* I wish
i had more time today to look into this, but it probably
has to wait until next week on my side, sorry.
But i'll definitly have a close look into this.

Thanks so much for all this,
Frank

Re: [RFC 13/13] Char: nozomi, cleanup read and write

[Adrian Bunk]

On Fri, Nov 09, 2007 at 06:51:35PM -0500, Jiri Slaby wrote:
>...
> --- a/drivers/char/nozomi.c
> +++ b/drivers/char/nozomi.c
>...
> -		if (size_bytes - i == 2) {
> +		if (unlikely(size_bytes - i == 2)) {
>...

Please don't add likely/unlikely in drivers unless it brings a 
measurable improvement.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [RFC 13/13] Char: nozomi, cleanup read and write

[Jiri Slaby]

On 11/10/2007 05:15 PM, Adrian Bunk wrote:
> On Fri, Nov 09, 2007 at 06:51:35PM -0500, Jiri Slaby wrote:
>> ...
>> --- a/drivers/char/nozomi.c
>> +++ b/drivers/char/nozomi.c
>> ...
>> -		if (size_bytes - i == 2) {
>> +		if (unlikely(size_bytes - i == 2)) {
>> ...
> 
> Please don't add likely/unlikely in drivers unless it brings a 
> measurable improvement.

Why? Anyway I think this is the case. The body of the then branch is executed at
most once, while the else branch each time but last. If you write/read 1002
bytes, it means 250:1. ...and it's invoked from interrupt too...

regards,
-- 
Jiri Slaby (jirislaby@gmail.com)
Faculty of Informatics, Masaryk University

Re: [RFC 13/13] Char: nozomi, cleanup read and write

[Frank Seidel]

On Samstag 10 November 2007 23:04:41, you (Jiri Slaby) wrote:
> On 11/10/2007 05:15 PM, Adrian Bunk wrote:
> > On Fri, Nov 09, 2007 at 06:51:35PM -0500, Jiri Slaby wrote:
> >> ...
> >> -		if (size_bytes - i == 2) {
> >> +		if (unlikely(size_bytes - i == 2)) {
> >> ...
> > 
> > Please don't add likely/unlikely in drivers unless it brings a 
> > measurable improvement.
> Why? Anyway I think this is the case. The body of the then branch is executed at
> most once, while the else branch each time but last. If you write/read 1002
> bytes, it means 250:1. ...and it's invoked from interrupt too...

I just did some measurements of how often (under real life scenarios like
downloading big files, websurfing, chat and ssh sessions) those pathes are
used.

While in the read_mem32 the unlikekly really seems to be of no use at all (the
switch-case ahead seems to be hit nearly always), the unlikely in the 
write_mem32 seems to be fine.
I compared after each 30 seconds and got median ratio of 1381:1 (for the
likely path) after about 20 minutes, i see a range between 1046:1 and
3511:1. So i wouldn't call it a bad guess from my beginners point of view.

Thanks,
Frank

Re: [RFC 13/13] Char: nozomi, cleanup read and write

[Frank Seidel]

On Sonntag 11 November 2007 03:37:28, you (Frank Seidel) wrote:
> While in the read_mem32 the unlikekly really seems to be of no use at all (the
> switch-case ahead seems to be hit nearly always), the unlikely in the 
> write_mem32 seems to be fine.
> I compared after each 30 seconds and got median ratio of 1381:1 (for the
> likely path) after about 20 minutes, i see a range between 1046:1 and
> 3511:1. So i wouldn't call it a bad guess from my beginners point of view.

I even did some more tracking which pathes get used in there with what
size_bytes values for write_mem32. To what i could see i get about
50% of the calls with size_bytes == 4 (what gets handled in the
switch-case shortcut), about 30% of the calls with size_bytes == 1 (
so i also added a shortcut for this which was just one line) and the
remaining calls (not handled in the switch-case ahead) still reach
a ratio of about 800:1 for the 4-byte-case to the (unlikely) 2-byte-
case.

So i did a rework of that patch which is nearly as nice as Jiris,
but works here without problems and has the size_bytes 1 shortcut,
plus the "unlikely" for the remaining 2-bytes path.

I know the format of the patch isn't fully correct, but i'll integrate
it into the complete patch und polish it there before posting that
again.

Thanks a lot,
Frank
---
Index: linux/drivers/char/nozomi.c
===================================================================
--- linux.orig/drivers/char/nozomi.c
+++ linux/drivers/char/nozomi.c
@@ -104,6 +104,7 @@
 #include <linux/list.h>
 #include <linux/uaccess.h>
 #include <asm/atomic.h>
+#include <asm/byteorder.h>
 
 #include <linux/delay.h>
 
@@ -265,7 +266,7 @@ enum port_type {
 	PORT_ERROR	= -1,
 };
 
-#ifdef __ARMEB__
+#ifdef __BIG_ENDIAN
 /* Big endian */
 
 struct toggles {
@@ -547,11 +548,7 @@ static void read_mem32(u32 *buf, const v
 			u32 size_bytes)
 {
 	u32 i = 0;
-#ifdef __ARMEB__
-	const u32 *ptr = (u32 *) mem_addr_start;
-#else
 	const u32 *ptr = (__force u32 *) mem_addr_start;
-#endif
 	u16 *buf16;
 
 	if (unlikely(!ptr || !buf))
@@ -561,19 +558,11 @@ static void read_mem32(u32 *buf, const v
 	switch (size_bytes) {
 	case 2:	/* 2 bytes */
 		buf16 = (u16 *) buf;
-#ifdef __ARMEB__
-		*buf16 = __le16_to_cpu(readw(ptr));
-#else
-		*buf16 = readw((void __iomem *)ptr);
-#endif
+		*buf16 = __le16_to_cpu(readw((void __iomem *)ptr));
 		goto out;
 		break;
 	case 4:	/* 4 bytes */
-#ifdef __ARMEB__
-		*(buf) = __le32_to_cpu(readl(ptr));
-#else
-		*(buf) = readl((void __iomem *)ptr);
-#endif
+		*(buf) = __le32_to_cpu(readl((void __iomem *)ptr));
 		goto out;
 		break;
 	}
@@ -582,19 +571,11 @@ static void read_mem32(u32 *buf, const v
 		if (size_bytes - i == 2) {
 			/* Handle 2 bytes in the end */
 			buf16 = (u16 *) buf;
-#ifdef __ARMEB__
-			*(buf16) = __le16_to_cpu(readw(ptr));
-#else
-			*(buf16) = readw((void __iomem *)ptr);
-#endif
+			*(buf16) = __le16_to_cpu(readw((void __iomem *)ptr));
 			i += 2;
 		} else {
 			/* Read 4 bytes */
-#ifdef __ARMEB__
-			*(buf) = __le32_to_cpu(readl(ptr));
-#else
-			*(buf) = readl((void __iomem *)ptr);
-#endif
+			*(buf) = __le32_to_cpu(readl((void __iomem *)ptr));
 			i += 4;
 		}
 		buf++;
@@ -613,11 +594,7 @@ static u32 write_mem32(void __iomem *mem
 			u32 size_bytes)
 {
 	u32 i = 0;
-#ifdef __ARMEB__
-	u32 *ptr = (u32 *) mem_addr_start;
-#else
 	u32 *ptr = (__force u32 *) mem_addr_start;
-#endif
 	u16 *buf16;
 
 	if (unlikely(!ptr || !buf))
@@ -627,40 +604,28 @@ static u32 write_mem32(void __iomem *mem
 	switch (size_bytes) {
 	case 2:	/* 2 bytes */
 		buf16 = (u16 *) buf;
-#ifdef __ARMEB__
-		writew(__le16_to_cpu(*buf16), ptr);
-#else
-		writew(*buf16, (void __iomem *)ptr);
-#endif
+		writew(__cpu_to_le16(*buf16), (void __iomem *)ptr);
 		return 2;
 		break;
+	case 1:
+		/* also need to write 4 bytes in this case
+		 * so falling through..
+		 */
 	case 4: /* 4 bytes */
-#ifdef __ARMEB__
-		writel(__cpu_to_le32(*buf), ptr);
-#else
-		writel(*buf, (void __iomem *)ptr);
-#endif
+		writel(__cpu_to_le32(*buf), (void __iomem *)ptr);
 		return 4;
 		break;
 	}
 
 	while (i < size_bytes) {
-		if (size_bytes - i == 2) {
+		if (unlikely(size_bytes - i == 2)) {
 			/* 2 bytes */
 			buf16 = (u16 *) buf;
-#ifdef __ARMEB__
-			writew(__le16_to_cpu(*buf16), ptr);
-#else
-			writew(*buf16, (void __iomem *)ptr);
-#endif
+			writew(__cpu_to_le16(*buf16), (void __iomem *)ptr);
 			i += 2;
 		} else {
 			/* 4 bytes */
-#ifdef __ARMEB__
-			writel(__cpu_to_le32(*buf), ptr);
-#else
-			writel(*buf, (void __iomem *)ptr);
-#endif
+			writel(__cpu_to_le32(*buf), (void __iomem *)ptr);
 			i += 4;
 		}
 		buf++;

Re: [RFC 13/13] Char: nozomi, cleanup read and write

[Adrian Bunk]

On Sat, Nov 10, 2007 at 11:04:41PM +0100, Jiri Slaby wrote:
> On 11/10/2007 05:15 PM, Adrian Bunk wrote:
> > On Fri, Nov 09, 2007 at 06:51:35PM -0500, Jiri Slaby wrote:
> >> ...
> >> --- a/drivers/char/nozomi.c
> >> +++ b/drivers/char/nozomi.c
> >> ...
> >> -		if (size_bytes - i == 2) {
> >> +		if (unlikely(size_bytes - i == 2)) {
> >> ...
> > 
> > Please don't add likely/unlikely in drivers unless it brings a 
> > measurable improvement.
> 
> Why? Anyway I think this is the case. The body of the then branch is executed at
> most once, while the else branch each time but last. If you write/read 1002
> bytes, it means 250:1. ...and it's invoked from interrupt too...

AFAIK there is no well defined semantics whether likely/unlikely means 
10:1 or 10000:1 that is guaranteed to not change during the next
10 years.

Unless there's a measurable difference, it's best to write readable 
C code and simply leave all optimizations to the compiler.

> regards,

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [RFC 13/13] Char: nozomi, cleanup read and write

[Frank Seidel]

On Montag 12 November 2007 08:54:52, you (Adrian Bunk) wrote:
> On Sat, Nov 10, 2007 at 11:04:41PM +0100, Jiri Slaby wrote:
> > Why? Anyway I think this is the case. The body of the then branch is executed at
> > most once, while the else branch each time but last. If you write/read 1002
> > bytes, it means 250:1. ...and it's invoked from interrupt too...
> 
> AFAIK there is no well defined semantics whether likely/unlikely means 
> 10:1 or 10000:1 that is guaranteed to not change during the next
> 10 years.
> 
> Unless there's a measurable difference, it's best to write readable 
> C code and simply leave all optimizations to the compiler.

>From my (totally) beginners point of view i would have guessed a chance of
very well below one percent that this condition is true could be called
"unlikely",
but i have to admit this is most probably a much too naive way of thinking,
especially in regards of compiler optimizations.
And in this special case now there are already most calls (about 80 percent)
caught by the switch-case-shortcuts anyway.

So, i'll revert it.

Thanks a lot for your feedback,
Frank	

Re: [RFC 1/13] Char: nozomi, remove unneded stuff

[Frank Seidel]

On Samstag 10 November 2007 00:43:29, you (Jiri Slaby) wrote:
> Frank, could you comment (and test) following changes to the nozomi driver?
> I did them before you post the updated version and rediffed them a while
> ago, I hope I haven't done any mistake while doing it.

Great thanks for all this! I went through all of them,
put some comments in, ran tests and incorporated them
(nearly completely and unchanged).
They really catched a great number of issues.

I also added you to the header of the complete patch
(also with your Signed-off-by), is that ok?
After some final tests are through i'll repost the complete patch in
the review thread and in CC to Greg again.

> --
> 
> nozomi, remove unneded stuff
> 
> - tty_termios* are set to tty struct, but tty_register_driver overwrites it
>   with its own values (or NULLs), no need to have these pointers stored
> - [rt]x_data are updated but never used

completely agree

> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> 
> ---
all applied without a change

Many thanks again,
Frank

Re: [RFC 1/13] Char: nozomi, remove unneded stuff

[Jiri Slaby]

On 11/10/2007 04:41 PM, Frank Seidel wrote:
> I also added you to the header of the complete patch
> (also with your Signed-off-by), is that ok?

No problem here.

thanks,
-- 
Jiri Slaby (jirislaby@gmail.com)
Faculty of Informatics, Masaryk University