Re: [PATCH v3 2/5] serial: mvebu-uart: implement UART clock driver for configuring UART base clock

From: Andrew Lunn <andrew@lunn.ch>
To: "Pali Rohár" <pali@kernel.org>
Cc: "Michael Turquette" <mturquette@baylibre.com>,
	"Stephen Boyd" <sboyd@kernel.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Gregory Clement" <gregory.clement@bootlin.com>,
	"Sebastian Hesselbarth" <sebastian.hesselbarth@gmail.com>,
	"Vladimir Vid" <vladimir.vid@sartura.hr>,
	"Marek Behún" <kabel@kernel.org>,
	"Geert Uytterhoeven" <geert@linux-m68k.org>,
	linux-clk@vger.kernel.org, linux-serial@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 2/5] serial: mvebu-uart: implement UART clock driver for configuring UART base clock
Date: Sat, 17 Jul 2021 19:26:51 +0200	[thread overview]
Message-ID: <YPMS24faTg9tqreR@lunn.ch> (raw)
In-Reply-To: <20210717123829.5201-3-pali@kernel.org>

On Sat, Jul 17, 2021 at 02:38:26PM +0200, Pali Rohár wrote:
> @@ -445,6 +472,7 @@ static void mvebu_uart_shutdown(struct uart_port *port)
>  static int mvebu_uart_baud_rate_set(struct uart_port *port, unsigned int baud)
>  {
>  	unsigned int d_divisor, m_divisor;
> +	unsigned long flags;
>  	u32 brdv, osamp;
>  
>  	if (!port->uartclk)
> @@ -463,10 +491,12 @@ static int mvebu_uart_baud_rate_set(struct uart_port *port, unsigned int baud)
>  	m_divisor = OSAMP_DEFAULT_DIVISOR;
>  	d_divisor = DIV_ROUND_CLOSEST(port->uartclk, baud * m_divisor);
>  
> +	spin_lock_irqsave(&mvebu_uart_lock, flags);

Hi Pali

You only need spin_lock_irqsave() if you plan on taking the spinlock
in an interrupt handler. It seems unlikely the baud rate will be
changed in interrupt context? Please check, and then swap to plain
spin_lock().

>  	brdv = readl(port->membase + UART_BRDV);
>  	brdv &= ~BRDV_BAUD_MASK;
>  	brdv |= d_divisor;
>  	writel(brdv, port->membase + UART_BRDV);
> +	spin_unlock_irqrestore(&mvebu_uart_lock, flags);
>  
>  	osamp = readl(port->membase + UART_OSAMP);
>  	osamp &= ~OSAMP_DIVISORS_MASK;

> +	/* Recalculate UART1 divisor so UART1 baudrate does not change */
> +	if (prev_clock_rate) {
> +		divisor = DIV_U64_ROUND_CLOSEST((u64)(val & BRDV_BAUD_MASK) *
> +						parent_clock_rate * prev_d1d2,
> +						prev_clock_rate * d1 * d2);
> +		if (divisor < 1)
> +			divisor = 1;
> +		else if (divisor > BRDV_BAUD_MAX)
> +			divisor = BRDV_BAUD_MAX;
> +		val = (val & ~BRDV_BAUD_MASK) | divisor;
> +	}

I don't see any range checks in the patch which verifies the requested
baud rate is actually possible. With code like this, it seems like the
baud rate change will be successful, but the actual baud rate will not
be what is requested.

> +	/* Recalculate UART2 divisor so UART2 baudrate does not change */
> +	if (prev_clock_rate) {
> +		val = readl(uart_clock_base->reg2);
> +		divisor = DIV_U64_ROUND_CLOSEST((u64)(val & BRDV_BAUD_MASK) *
> +						parent_clock_rate * prev_d1d2,
> +						prev_clock_rate * d1 * d2);
> +		if (divisor < 1)
> +			divisor = 1;
> +		else if (divisor > BRDV_BAUD_MAX)
> +			divisor = BRDV_BAUD_MAX;
> +		val = (val & ~BRDV_BAUD_MASK) | divisor;
> +		writel(val, uart_clock_base->reg2);

Here it looks like UART1 could request a baud rate change, which ends
up setting the clocks so that UART2 is out of range? Could the change
for UART1 be successful, but you end up breaking UART2? I'm thinking
when you are at opposite ends of the scale. UART2 is running at
110baud and UART1 at 230400baud.

	Andrew