Message-ID: <20160201211345.3f5c6b72@lxorguk.ukuu.org.uk>
Date:	Mon, 1 Feb 2016 21:13:45 +0000
From:	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>
To:	Mathieu OTHACEHE <m.othacehe@...il.com>
Cc:	gregkh@...uxfoundation.org, jslaby@...e.com,
	akpm@...ux-foundation.org, davem@...emloft.net,
	mchehab@....samsung.com, kvalo@...eaurora.org, joe@...ches.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tty: add Moxa Smartio MUE serial driver

> Here is a new driver for MOXA Smartio MUE cards. It is based
> on the vendor driver available on MOXA website and on the
> mainline mxser driver.
> 
> I was able to test it on a CP-168EL-A card on PC. Some of the
> cards (118E-A, 138E-A, 134EL-A, 116E-A-A et 116E-A-B) have
> a CPLD module programmable via GPIO.
> For now, I dropped all the code related to CPLD/GPIO because I
> can't test it on my card.

Thanks

Some comments (and I realise these are probably not bugs you added!)

> +static int mxupcie_set_baud(struct tty_struct *tty, long newspd)
> +{
> +	int quot = 0;
> +	unsigned char cval;
> +	int custom = 0;
> +	struct mxupcie_port *info = tty->driver_data;
> +
> +	if (newspd > info->max_baud)
> +		return 0;

This doesn't look quite the behavioru you want.

> +
> +	if (newspd == 38400 &&
> +		(info->port.flags & ASYNC_SPD_MASK) == ASYNC_SPD_CUST) {
> +		info->speed = info->custom_baud_rate;
> +		custom = 1;
> +
> +		quot = info->baud_base / info->speed;
> +		if (info->speed <= 0 || info->speed > info->max_baud)
> +			quot = 0;
> +		else
> +			set_linear_baud(info->ioaddr, info->speed);

The custom speed stuff can go really - it's handled by termios directly
these days.

>
> +static int mxupcie_change_speed(struct tty_struct *tty,
> +				struct ktermios *old_termios)
> +{

Be careful with this - if CMSPAR is set the these indicate mark or space
(and if you don't support that then clear the CMSPAR bit in tty->termios)

Likewise you should write the actual baud rate into the tty->termios
using the helper functions (see how 8250.c does it - it's a nice example)


> +static void mxupcie_shutdown(struct tty_port *port)
> +{
> +	struct mxupcie_port *info;
> +	unsigned char reg_flag;
> +	int i;
> +	unsigned long sp_flags;
> +
> +	info = container_of(port, struct mxupcie_port, port);
> +
> +	spin_lock_irqsave(&info->slock, sp_flags);
> +
> +	wake_up_interruptible(&info->port.delta_msr_wait);
> +
> +	if (info->port.xmit_buf) {
> +		free_page((unsigned long)info->port.xmit_buf);
> +		info->port.xmit_buf = NULL;
> +	}
> +
> +	reg_flag = 0;
> +	iowrite8(reg_flag, info->ioaddr + MOXA_PUART_EFR);
> +	iowrite8(reg_flag, info->ioaddr + MOXA_PUART_SFR);
> +
> +	info->ier = 0;
> +	iowrite8(0x00, info->ioaddr + UART_IER);
> +
> +	if (info->speed < 9600) {
> +		int sleep_interval = 0;
> +		int reset_cnt = 0;
> +
> +		if (info->speed <= 600) {
> +			sleep_interval = 10;
> +			reset_cnt = MX_FIFO_RESET_CNT;
> +		} else {
> +			sleep_interval = 1;
> +			reset_cnt = MX_FIFO_RESET_CNT / 10;
> +		}
> +
> +		/* Workaround for clear FIFO in low baudrate */
> +		iowrite8(0x0f, info->ioaddr + MOXA_PUART_ADJ_CLK);
> +		iowrite8(0x03, info->ioaddr + MOXA_PUART_ADJ_ENABLE);
> +
> +		/* clear Rx/Tx FIFO's */
> +		for (i = 0; i < reset_cnt; i++) {
> +			iowrite8((UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT),
> +				 info->ioaddr + UART_FCR);
> +			msleep(sleep_interval);

No can do - you have a spinlock held while you are tring to sleep. I'm
not btw clear that you actually need the lock. The tty_port layer ensures
activate/shutdown don't cross or get duplicated. The only protection you
might need is versus interrupts, and in that case you could free the IRQ
up and claim it in activate/shutdown.

> +static int mxupcie_open(struct tty_struct *tty, struct file *filp)
> +{
> +	struct mxupcie_port *info;
> +	struct mxupcie_board *board;
> +	int line;
> +
> +	line = tty->index;
> +	if (line == MXUPCIE_PORTS)
> +		return 0;

What is this for ?

> +
> +	if ((line < 0) || (line > MXUPCIE_PORTS))
> +		return -ENODEV;

How can this happen ?

> +	/*
> +	 * Before we drop DTR, make sure the UART transmitter
> +	 * has completely drained; this is especially
> +	 * important if there is a transmit FIFO!
> +	 */
> +	timeout = jiffies + HZ;
> +	while (!(ioread8(info->ioaddr + UART_LSR) & UART_LSR_TEMT)) {
> +		schedule_timeout_interruptible(5);

Always 5 - doesn't it depend on HZ ?

> +		if (time_after(jiffies, timeout))
> +			break;
> +	}
> +}
> +
> +static void mxupcie_close(struct tty_struct *tty, struct file *filp)
> +{
> +	struct mxupcie_port *info = tty->driver_data;
> +	struct tty_port *port = &info->port;
> +
> +	if (tty->index == MXUPCIE_PORTS || info == NULL)
> +		return;

Can info be null, given the tty_port code does it matter ?

> +	if (tty_port_close_start(port, tty, filp) == 0)
> +		return;
> +
> +	mutex_lock(&port->mutex);
> +	mxupcie_close_port(port);
> +	mxupcie_flush_buffer(tty);
> +	if (test_bit(ASYNCB_INITIALIZED, &port->flags)) {
> +		if (tty->termios.c_cflag & HUPCL)
> +			tty_port_lower_dtr_rts(port);
> +	}
> +	mxupcie_shutdown(port);
> +	set_bit(TTY_IO_ERROR, &tty->flags);
> +	mutex_unlock(&port->mutex);

This looks confused - the tty_port_close method calls the shutdown method
of the tty port so this should probably just be using tty_port_close and
the rest in the port method ?

>
> +static int mxupcie_ioctl(struct tty_struct *tty, unsigned int cmd,
> +			 unsigned long arg)
> +{
> +	struct mxupcie_port *info = tty->driver_data;
> +	int retval;
> +	struct async_icount cnow;
> +	int ret = 0;
> +	unsigned long sp_flags;
> +
> +	switch (cmd) {
> +	case TCSBRK:
> +		retval = tty_check_change(tty);
> +		if (retval)
> +			return retval;
> +
> +		tty_wait_until_sent(tty, 0);
> +
> +		if (!arg)
> +			mxupcie_send_break(tty, HZ / 4);
> +
> +		break;
> +	case TCSBRKP:
> +		retval = tty_check_change(tty);
> +		if (retval)
> +			return retval;
> +
> +		tty_wait_until_sent(tty, 0);
> +		mxupcie_send_break(tty, arg ? arg * (HZ / 10) : HZ / 4);

These won't work. You need to supply a tty->ops->break_ctl method (which
you seem to have later on)

> +static void mxupcie_set_termios(struct tty_struct *tty,
> +				struct ktermios *old_termios)
> +{
> +	struct mxupcie_port *info = tty->driver_data;
> +	unsigned long sp_flags;
> +
> +	spin_lock_irqsave(&info->slock, sp_flags);
> +	mxupcie_change_speed(tty, old_termios);
> +	spin_unlock_irqrestore(&info->slock, sp_flags);
> +
> +	if (old_termios &&
> +	    !tty_termios_hw_change(&tty->termios, old_termios) &&
> +	    tty->termios.c_iflag == old_termios->c_iflag) {
> +		dev_dbg(tty->dev, "%s - nothing to change\n", __func__);
> +		return;
> +	}

This debug can probably go away

> +static void mxupcie_rx_chars(struct tty_struct *tty,
> +			     int *status)
> +{
> +	struct mxupcie_port *info = tty->driver_data;
> +	unsigned char ch, gdl = 0;
> +	int cnt = 0;
> +	int recv_room;
> +	int max = 256;
> +	unsigned long flags;
> +
> +	if (*status & UART_LSR_SPECIAL)
> +		goto intr_old;
> +
> +	recv_room = tty_buffer_request_room(&info->port, MX_RX_FIFO_SIZE);
> +	if (recv_room) {
> +		gdl = ioread8(info->ioaddr + MOXA_PUART_RCNT);
> +
> +		if (gdl > recv_room)
> +			gdl = recv_room;
> +
> +		if (gdl) {
> +			tty_insert_flip_string(
> +				&info->port,
> +				info->ioaddr + MOXA_PUART_MEMRBR, gdl);
> +
> +			cnt = gdl;
> +		}
> +	} else {
> +		set_bit(TTY_THROTTLED, &tty->flags);
> +	}

The above is almost certainly not worth the effort. If we fill up space
we are very congested already and you'll almost certainly drop bytes
anyway. In fact because you preallocate the full size it's probably more
likely to drop bytes than doign it the simple way and letting the tty
layer manage things.

> +	if (cnt) {
> +		memcpy(info->ioaddr + MOXA_PUART_MEMTHR,
> +		       info->port.xmit_buf + info->xmit_tail, cnt);

This appears to be memcpy into the mmio space. In which case memcpy is
probably not what you want here as memcpy won't work for mmio space on
all platforms and may do very weird things on some !


> +static irqreturn_t mxupcie_interrupt(int irq, void *dev_id)
> +{
> +	int lsr, iir, i;
> +	struct mxupcie_board *board;
> +	struct mxupcie_port *info;
> +	int max, msr;
> +	int pass_counter = 0;
> +	int int_cnt;
> +	int handled = 0;
> +	int vect_flag;
> +	struct tty_struct *tty;
> +
> +	board = NULL;
> +	for (i = 0; i < MXUPCIE_BOARDS; i++) {
> +		if (dev_id == &mxupcie_boards[i]) {
> +			board = dev_id;
> +			break;
> +		}
> +	}

This code makes no sense. Surely it's just

	board = dev_id;

> +
> +	if (i == MXUPCIE_BOARDS)
> +		goto irq_stop;
> +
> +	if (board == NULL)
> +		goto irq_stop;

Not needed


> +
> +	max = board->cinfo->nports;
> +	pass_counter = 0;
> +
> +	do {
> +		vect_flag = 0;
> +
> +		for (i = 0; i < max; i++) {
> +			info = &board->ports[i];
> +			int_cnt = 0;
> +
> +			do {
> +				iir = ioread8(info->ioaddr + UART_IIR);
> +				if (iir == MOXA_IIR_NO_INT) {
> +					vect_flag++;
> +					break;
> +				}
> +
> +				tty = tty_port_tty_get(&info->port);
> +				if (!tty) {
> +					lsr = ioread8(info->ioaddr + UART_LSR);
> +					iowrite8(0x27, info->ioaddr + UART_FCR);
> +					ioread8(info->ioaddr + UART_MSR);

Should this not also set handled ?


> +	err = mxupcie_initbrd(board, pdev);
> +	if (err)
> +		goto err_zero;

This seems to have cases it can leave tty resources partially allocated ?


> +static void mxupcie_pci_remove(struct pci_dev *pdev)
> +{
> +	struct mxupcie_board *board = pci_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < board->cinfo->nports; i++) {
> +		tty_unregister_device(mx_drv, board->index + i);
> +		tty_port_destroy(&board->ports[i].port);
> +	}
> +
> +	free_irq(board->irq, board);

I think you need to free the IRQ first so an IRQ can't crash if it occurs
between the port destroy and the free_irq



Alan

Powered by blists - more mailing lists