linux-kernel - Re: [PATCH v3 04/10] serial: mps2-uart: add MPS2 UART driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75Vcx_xAY2A4XNgQ1ea_U5mUnD1+6=2FxCM3KzsRSpxgRuw@mail.gmail.com>
Date:	Tue, 16 Feb 2016 12:48:16 +0200
From:	Andy Shevchenko <andy.shevchenko@...il.com>
To:	Vladimir Murzin <vladimir.murzin@....com>
Cc:	Arnd Bergmann <arnd@...db.de>,
	Russell King <linux@....linux.org.uk>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>,
	Mark Rutland <mark.rutland@....com>,
	Pawel Moll <pawel.moll@....com>,
	"ijc+devicetree" <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>, Jiri Slaby <jslaby@...e.cz>,
	Rob Herring <robh+dt@...nel.org>,
	devicetree <devicetree@...r.kernel.org>,
	"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
	"linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
	linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 04/10] serial: mps2-uart: add MPS2 UART driver

On Tue, Feb 16, 2016 at 12:08 PM, Vladimir Murzin
<vladimir.murzin@....com> wrote:
> This driver adds support to the UART controller found on ARM MPS2
> platform.


> +static irqreturn_t mps2_uart_oerrirq(int irq, void *data)
> +{
> +       irqreturn_t handled = IRQ_NONE;
> +       struct uart_port *port = data;
> +       u8 irqflag = mps2_uart_read8(port, UARTn_INT);
> +
> +       spin_lock(&port->lock);
> +
> +       if (irqflag & UARTn_INT_RX_OVERRUN) {
> +               struct tty_port *tport = &port->state->port;
> +
> +               mps2_uart_write8(port, UARTn_INT_RX_OVERRUN, UARTn_INT);
> +               tty_insert_flip_char(tport, 0, TTY_OVERRUN);
> +               port->icount.overrun++;
> +               handled = IRQ_HANDLED;
> +       }
> +
> +       /*
> +        * It's never been seen in practice and it never *should* happen since
> +        * we check if there is enough room in TX buffer before sending data.
> +        * So we keep this check in case something suspicious has happened.
> +        */
> +       if (irqflag & UARTn_INT_TX_OVERRUN) {
> +               mps2_uart_write8(port, UARTn_INT_TX_OVERRUN, UARTn_INT);

> +               dev_warn(port->dev, "unexpected overrun interrupt\n");

I'm not sure there is no dead lock if this happens on the same port
which is used as console.

> +               handled = IRQ_HANDLED;
> +       }
> +
> +       spin_unlock(&port->lock);
> +
> +       return handled;
> +}
> +
> +static int mps2_uart_startup(struct uart_port *port)
> +{
> +       struct mps2_uart_port *mps_port = to_mps2_port(port);
> +       u8 control = mps2_uart_read8(port, UARTn_CTRL);
> +       int ret;
> +
> +       control &= ~(UARTn_CTRL_RX_GRP | UARTn_CTRL_TX_GRP);
> +
> +       mps2_uart_write8(port, control, UARTn_CTRL);
> +
> +       ret = request_irq(mps_port->rx_irq, mps2_uart_rxirq, 0,
> +                         MAKE_NAME(-rx), mps_port);
> +       if (ret) {
> +               dev_err(port->dev, "failed to register rxirq (%d)\n", ret);
> +               goto err_no_rxirq;

It should be below, here just a plain return.

> +       }
> +
> +       ret = request_irq(mps_port->tx_irq, mps2_uart_txirq, 0,
> +                         MAKE_NAME(-tx), mps_port);
> +       if (ret) {
> +               dev_err(port->dev, "failed to register txirq (%d)\n", ret);
> +               goto err_no_txirq;

goto err_free_rxirq;

> +       }
> +
> +       ret = request_irq(port->irq, mps2_uart_oerrirq, IRQF_SHARED,
> +                         MAKE_NAME(-overrun), mps_port);
> +
> +       if (ret) {
> +               dev_err(port->dev, "failed to register oerrirq (%d)\n", ret);

Why not goto pattern here as well?

goto err_free_txirq;

> +       } else {

…and remove this else.

> +               control |= UARTn_CTRL_RX_GRP | UARTn_CTRL_TX_GRP;
> +
> +               mps2_uart_write8(port, control, UARTn_CTRL);
> +
> +               return 0;
> +       }
> +
> +       free_irq(mps_port->tx_irq, mps_port);
> +err_no_txirq:
> +       free_irq(mps_port->rx_irq, mps_port);
> +err_no_rxirq:
> +       return ret;
> +}


> +static struct mps2_uart_port mps2_uart_ports[MPS2_MAX_PORTS];
> +
> +#ifdef CONFIG_SERIAL_MPS2_UART_CONSOLE
> +static void mps2_uart_console_putchar(struct uart_port *port, int ch)
> +{
> +       while (mps2_uart_read8(port, UARTn_STATE) & UARTn_STATE_TX_FULL)
> +               cpu_relax();

Infinite?

> +
> +       mps2_uart_write8(port, ch, UARTn_DATA);
> +}

> +static int mps2_init_port(struct mps2_uart_port *mps_port,
> +                       struct platform_device *pdev)
> +{
> +       int ret;
> +       struct resource *res;

Maybe:
       struct resource *res;
       int ret;
?

-- 
With Best Regards,
Andy Shevchenko