linux-kernel - RE: [PATCH v5 2/2] serial:sunplus-uart:Add Sunplus SoC 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]
Date:   Mon, 13 Dec 2021 11:37:21 +0000
From:   Hammer Hsieh 謝宏孟 
        <hammer.hsieh@...plus.com>
To:     Jiri Slaby <jirislaby@...nel.org>,
        Hammer Hsieh <hammerh0314@...il.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>
CC:     Wells Lu 呂芳騰 <wells.lu@...plus.com>
Subject: RE: [PATCH v5 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver

Hi, Jiri Slaby:

Thanks for your review.
My response is below in mail.

Best Regards,
Hammer Hsieh

> -----Original Message-----
> From: Jiri Slaby <jirislaby@...nel.org>
> Sent: Monday, December 13, 2021 3:47 PM
> To: Hammer Hsieh <hammerh0314@...il.com>; gregkh@...uxfoundation.org;
> robh+dt@...nel.org; linux-serial@...r.kernel.org; devicetree@...r.kernel.org;
> linux-kernel@...r.kernel.org; p.zabel@...gutronix.de
> Cc: Wells Lu 呂芳騰 <wells.lu@...plus.com>; Hammer Hsieh 謝宏孟
> <hammer.hsieh@...plus.com>
> Subject: Re: [PATCH v5 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver
> 
> On 13. 12. 21, 8:10, Hammer Hsieh wrote:
> > Add Sunplus SoC UART Driver
> >
> > Signed-off-by: Hammer Hsieh <hammer.hsieh@...plus.com>
> 
> ...
> 
> > --- /dev/null
> > +++ b/drivers/tty/serial/sunplus-uart.c
> 
> ...
> 
> > +static void receive_chars(struct uart_port *port) {
> > +	unsigned int lsr = readl(port->membase + SUP_UART_LSR);
> > +	unsigned int ch, flag;
> > +
> > +	do {
> > +		ch = readl(port->membase + SUP_UART_DATA);
> > +		flag = TTY_NORMAL;
> > +		port->icount.rx++;
> > +
> > +		if (unlikely(lsr & SUP_UART_LSR_BRK_ERROR_BITS)) {
> > +			if (lsr & SUP_UART_LSR_BC) {
> > +				lsr &= ~(SUP_UART_LSR_FE | SUP_UART_LSR_PE);
> > +				port->icount.brk++;
> > +				if (uart_handle_break(port))
> > +					goto ignore_char;
> > +			} else if (lsr & SUP_UART_LSR_PE) {
> > +				port->icount.parity++;
> > +			} else if (lsr & SUP_UART_LSR_FE) {
> > +				port->icount.frame++;
> > +			}
> > +
> > +			if (lsr & SUP_UART_LSR_OE)
> > +				port->icount.overrun++;
> > +
> > +			if (lsr & SUP_UART_LSR_BC)
> > +				flag = TTY_BREAK;
> > +			else if (lsr & SUP_UART_LSR_PE)
> > +				flag = TTY_PARITY;
> > +			else if (lsr & SUP_UART_LSR_FE)
> > +				flag = TTY_FRAME;
> 
> Why do you handle these separately and not above?
> 

Indeed, I will modify as below.
		if (lsr & SUP_UART_LSR_BC) {
			lsr &= ~(SUP_UART_LSR_FE | SUP_UART_LSR_PE);
			port->icount.brk++;
			flag = TTY_BREAK;
			if (uart_handle_break(port))
				goto ignore_char;
			} else if (lsr & SUP_UART_LSR_PE) {
			port->icount.parity++;
			flag = TTY_PARITY;
		} else if (lsr & SUP_UART_LSR_FE) {
			port->icount.frame++;
			flag = TTY_FRAME;
		}

> > +		}
> > +
> > +		if (port->ignore_status_mask & SUP_DUMMY_READ)
> > +			goto ignore_char;
> > +
> > +		if (uart_handle_sysrq_char(port, ch))
> > +			goto ignore_char;
> > +
> > +		uart_insert_char(port, lsr, SUP_UART_LSR_OE, ch, flag);
> > +
> > +ignore_char:
> > +		lsr = readl(port->membase + SUP_UART_LSR);
> > +	} while (lsr & SUP_UART_LSR_RX);
> > +
> > +	tty_flip_buffer_push(&port->state->port);
> > +}
> > +
> > +static irqreturn_t sunplus_uart_irq(int irq, void *args) {
> > +	struct uart_port *port = (struct uart_port *)args;
> 
> No need to cast here.
> 

Ok, will modify it.

> > +	unsigned int isc = readl(port->membase + SUP_UART_ISC);
> 
> Shouldn't this be under the spinlock?
> 
> And "if (!isc) return IRQ_NONE"?
> 

Will modify it as below.
Spin_lock()
isc = readl(port->membase + SUP_UART_ISC);
if (!isc) return IRQ_NONE;
if(isc&RX) receive_chars();
if(isc&TX) transmit_chars();
Spin_unlock()

> > +	spin_lock(&port->lock);
> > +
> > +	if (isc & SUP_UART_ISC_RX)
> > +		receive_chars(port);
> > +
> > +	if (isc & SUP_UART_ISC_TX)
> > +		transmit_chars(port);
> > +
> > +	spin_unlock(&port->lock);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int sunplus_startup(struct uart_port *port) {
> > +	unsigned long flags;
> > +	unsigned int isc;
> > +	int ret;
> > +
> > +	ret = request_irq(port->irq, sunplus_uart_irq, 0, "sunplus_uart",
> > +port);
> 
> Cannot the interrupt be shared?
> 

Each UART have its own hardware interrupt in Sunplus SP7021 SoC.
No need to set IRQF_SHARED for serial driver.

> > +	if (ret)
> > +		return ret;
> > +
> > +	spin_lock_irqsave(&port->lock, flags);
> > +
> > +	isc |= SUP_UART_ISC_RXM;
> > +	writel(isc, port->membase + SUP_UART_ISC);
> > +
> > +	spin_unlock_irqrestore(&port->lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static void sunplus_shutdown(struct uart_port *port) {
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&port->lock, flags);
> > +	writel(0, port->membase + SUP_UART_ISC);
> 
> What bus is this -- posting?
> 

Here just clear interrupt.
Not really understand your comment?

> > +	spin_unlock_irqrestore(&port->lock, flags);
> > +
> > +	free_irq(port->irq, port);
> > +}
> 
> ...
> 
> > +static void sunplus_release_port(struct uart_port *port) { }
> > +
> > +static int sunplus_request_port(struct uart_port *port) {
> > +	return 0;
> > +}
> 
> These two are optional -- no need to provide them.
> 

Ok, thanks.
I will remove these two functions.

> regards,
> --
> js
> suse labs