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 ...
[ message continues ]

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 ...
[ message continues ]

definitely true, all apllied without change

Thanks,
Frank


-

[ message continues ]

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 ...
[ message continues ]

all applied without change

Thanks,
Frank
-

[ message continues ]

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 ...
[ message continues ]

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
-

[ message continues ]

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
-

[ message continues ]

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 = ...
[ message continues ]

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

-

[ message continues ]

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 = ...
[ message continues ]

All perfect catches IMHO. Applied without changes.

Thanks,
Frank
-

[ message continues ]

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, ...
[ message continues ]

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

Thanks,
Frank
-

[ message continues ]

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
-

[ message continues ]

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[] = ...
[ message continues ]

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


-

[ message continues ]

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

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

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
-

[ message continues ]

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 ...
[ message continues ]

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
-

[ message continues ]

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, ...
[ message continues ]

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
-

[ message continues ]

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>
 
-

[ message continues ]

defered those (to review next week together with patch 7 and 8)

Thanks,
Frank


-

[ message continues ]

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;
 
-

[ message continues ]

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

Thanks,
Frank
-

[ message continues ]

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 */
 ...
[ message continues ]

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


-

[ message continues ]

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

-

[ message continues ]

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
-

[ message continues ]

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

-

[ message continues ]

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) = ...
[ message continues ]

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 

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

-

[ message continues ]

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	
-

[ message continues ]

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

all applied without a change

Many thanks again,
Frank
-

[ message continues ]

No problem here.

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

[ message continues ]