linux-kernel - Re: [PATCH 1/2] serial: Add driver for the Altera JTAG UART

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: <20100226090456.GA32296@distanz.ch>
Date:	Fri, 26 Feb 2010 10:04:56 +0100
From:	Tobias Klauser <tklauser@...tanz.ch>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
Cc:	linux-serial@...r.kernel.org, nios2-dev@...c.et.ntust.edu.tw,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] serial: Add driver for the Altera JTAG UART

On 2010-02-25 at 16:26:22 +0100, Alan Cox <alan@...rguk.ukuu.org.uk> wrote:
> > +static void altera_jtaguart_set_termios(struct uart_port *port,
> > +					struct ktermios *termios,
> > +					struct ktermios *old)
> > +{
> > +}
> 
> Erm no .. it may be wrong in the code you are copying but please don't
> further that. The requirement is that the termios handed back reflects
> the actual settings not the requested ones.
> 
> Use tty_termios_copy_hw() to copy the old termios hardware settings back
> so that the caller sees it cannot set them.

Would the following be sufficient?

  if (old)
          tty_termios_copy_hw(termios, old);

> > +static void altera_jtaguart_rx_chars(struct altera_jtaguart *pp)
> > +{
> > +	struct uart_port *port = &pp->port;
> > +	unsigned char ch, flag;
> > +	unsigned long status;
> > +
> > +	while ((status = readl(port->membase + ALTERA_JTAGUART_DATA_REG)) &
> > +	       ALTERA_JTAGUART_DATA_RVALID_MSK) {
> > +		ch = status & ALTERA_JTAGUART_DATA_DATA_MSK;
> > +		flag = TTY_NORMAL;
> > +		port->icount.rx++;
> > +
> > +		if (uart_handle_sysrq_char(port, ch))
> > +			continue;
> > +		uart_insert_char(port, 0, 0, ch, flag);
> > +	}
> > +
> > +	tty_flip_buffer_push(port->state->port.tty);
> 
> port.tty needs to be protected here - you might race an unplug/hangup as
> far as I can see - I think you need to take the lock over a bigger area

So should we take port->lock in altera_jtaguart_interrupt already,
something like the following?

  spin_lock(&port->lock);

  if (isr & ALTERA_JTAGUART_CONTROL_RE_MSK)
          altera_jtaguart_rx_chars(pp);
  if (isr & ALTERA_JTAGUART_CONTROL_WE_MSK)
          altera_jtaguart_tx_chars(pp);

  spin_unlock(&port->lock);

And of course remove the lock/unlock from altera_jtaguart_tx_chars.

> [I'm working on switching this to krefs in all the drivers currently so
> that will end up cleaner]
> 
> Basically fine other than that. The proposed UART number clashes with
> some other pending patches but that will get sorted out when they get
> merged

Thanks a lot for your review.

Tobias
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/