Hi,
On 22.02.2017 11:44, Sascha Hauer wrote:
On Tue, Feb 21, 2017 at 09:48:54AM +0100, Piotr Figiel wrote:
Hi Baruch,
On 21.02.2017 09:18, Baruch Siach wrote:
Hi Piotr,
On Tue, Feb 21, 2017 at 09:00:39AM +0100, Piotr Figiel wrote:
On 20.02.2017 20:20, Baruch Siach wrote:
On Mon, Feb 20, 2017 at 02:54:02PM +0100, Piotr Figiel wrote:
I'm looking to find a correct way to disable RX in the I.MX6 UART during
TX-ing. I stumbled on the issue described below when working on
~SER_RS485_RX_DURING_TX on custom 3.10 tree (branched from Freescale's/NXP's
BSP release) independently to what is now in the main line imx.c, but I see
now that the implementation in mainline also seem to have the same problem
I'm trying to solve now. I would like to request for comments to
confirm/deny whether the issue I raise here is valid and how to best
approach this.
I'm mostly concerned about the fact that the URXD register must not be
accessed (as documented in IMX.6 RM) when RX is disabled (RXEN=0). The issue
is that in the following sequence happening during imx_start_tx with
~SER_RS485_RX_DURING_TX set:
1. Acquire spinlock, disable local interrupts (from serial_core.c),
2. Disable RX receiver (RXEN=0 in UCR2),
3. Release spinlock, reenable interrupts.
The RX fifo may become not empty between #1 and #2. This will raise
interrupt which will be handled after re-enabling interrupts (after #3). ISR
in this case will check the status bit of the interrupt and fetch RX FIFO
contents, which I understand is forbidden by the documentation and may raise
error on the bus [1]. In addition disabling RX IRQ in this procedure, e.g.
after #1 doesn't seem to be enough, as the IRQ still may trigger after #1
and will need to be serviced.
As I understand it, SER_RS485_RX_DURING_TX is for half-duplex RS-485 where
only one device is allowed to transmit at any given time. Higher level code
determines when any given device on the bus is allowed to transmit. If Rx FIFO
becomes non-empty during Tx enable, you have a contention problem to solve.
That's correct, although I don't want bugs from the user-space or
misbehaving devices on RS-485 to cause errors on the AXI/AHB bus resulting
with possible crash/exception on the CPU core. The reason I write about this
is that the RM explicitly forbids accessing those registers in such case.
The trivial solution might be to return early from imx_rxint() when RXEN=0.
Would that be sufficient?
I'm not sure if it's best idea, I don't know if the UART will keep
triggering the interrupt if the source won't be cleared (FIFO drained), but
if it does we'll be irq flooded,
And indeed that's what happens. The RX irq is acked by reading all
characters from the FIFO. I had the same problem as you describe, see
below for the patch I came up with (with the intention to mainline i
t when I find the time). My "usecase" was to send a character while
being flooded with received characters from the other side. That of
course was only for testing purposes, but I could reliably crash
the kernel with this setup.
Thanks for confirming this, this probably requires CVE, since user with
tty access can crash mainline kernel by correctly timing a write
(personally though I wasn't able to trigger this without putting delay
in the code to trigger the race). Some comments regarding the
implementation (thanks for sharing this btw):
+static void imx_receiver_safe_disable(struct imx_port *sport)
+{
+ u32 ucr2, ucr1;
+ int t = 50;
+
+ ucr1 = readl(sport->port.membase + UCR1);
+ ucr1 &= ~UCR1_RRDYEN;
+ writel(ucr1, sport->port.membase + UCR1);
+
+ ucr2 = readl(sport->port.membase + UCR2);
+
+ /*
+ * Disable the receiver and make sure that no characters are left
+ * in the FIFO. Otherwise an interrupt will be triggered even when
+ * UCR1_RRDYEN is cleared. We can't ack this interrupt because with
+ * disabled receiver we are not allowed to read from the FIFO.
+ */
+ do {
+ writel(ucr2 | UCR2_RXEN, sport->port.membase + UCR2);
+
+ while (readl(sport->port.membase + USR2) & USR2_RDR)
+ readl(sport->port.membase + URXD0);
+
+ writel(ucr2 & ~UCR2_RXEN, sport->port.membase + UCR2);
+
+ if (!t--) {
+ dev_err(sport->port.dev, "Failed to disable the receiver\n");
+ return;
In this unlikely event perhaps an error should be returned so that the
RXEN is not disabled in imx_start_tx() (i.e. I think it's better to
ignore ~SER_RS485_RX_DURING_TX for a while than to crash).
+ }
+ } while (readl(sport->port.membase + USR2) & USR2_RDR);
+}
+
/*
* interrupts disabled on entry
*/
@@ -633,8 +669,10 @@ static void imx_start_tx(struct uart_port *port)
imx_port_rts_active(sport, &temp);
else
imx_port_rts_inactive(sport, &temp);
- if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
+ if (!(port->rs485.flags & SER_RS485_RX_DURING_TX)) {
+ imx_receiver_safe_disable(sport);
temp &= ~UCR2_RXEN;
+ }
writel(temp, port->membase + UCR2);
sport->tx_state = WAIT_AFTER_RTS;
sport->tx_state_next_change =
Other than that I think it's OK (it's also consistent with the procedure
from my first post [1]).
Best regards, Piotr.
[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/488957.html
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
