[PATCH/RFC,v2,00/29] serial: sh-sci: Miscellaneous and DMA Improvements

Commit Message

Yoshihiro Shimoda July 28, 2015, 7:56 a.m. UTC

Hi Geert-san,

Thank you for the patches!

(2015/07/17 3:21), Geert Uytterhoeven wrote:
> 	Hi all,
> 
> This patch series contains various updates for the Renesas
> (H)SCI(F{,A,B}) driver, incl. DMA support for SCIF on R-Car Gen2.
> Some of these patches have been sent before. (Few) Changes are indicated
> in the individual patches.
> 
> This is definitely not a final series, that's why it's marked as RFC and
> sent to a limited audience:
>   - There are still some race conditions between e.g. RX DMA
>     completion(s) and the worker function,
>   - Under high load RX DMA breaks,
>   - The patch to add DT DMA support should be last, after all DMA fixes,
>     to avoid regressions,
>   - Unlike the old shdmac DMA engine driver, the new rcar-dmac DMA
>     engine driver does not support resubmitting a DMA descriptor, so I
>     added a workaround to the sh-sci driver,
>   - This won't work with the old shdmac DMA engine driver anymore, due
>     to the lack of residue handling in shdmac (preliminary untested
>     patch sent, but it will need more work),
>   - There are issues with residual handling in general,
>   - ...
> 
> However, DMA is now usable for the serial console on r8a7791/koelsch.
> This also received light testing with scif3 and scifa5 on r8a7791/koelsch.
> 
> For your convenience, I've also pushed this to
> git://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git#scif-dma-v2
> 
> Thanks for your comments!

I tested this scif-dma-v2 branch using r8a7790/lager.
If I enabled CONFIG_HIGHMEM, a WARNING and kernel panic happened.
(I copyed the log at end of this email.)

I investigated this issue and I could fix this issue using the following patch.
The patch can be applied on the top of your scif-dma-v2.
If possible, would you check the patch and merge your patch set to v3?

---

Subject: [PATCH] serial: sh-sci: Fix NULL pointer dereference if HIGHMEM is enabled

From: Yoshihiro Shimoda <[e-mail]>

This patch fixes an issue that this driver causes a NULL pointer
dereference if the following conditions:
 - CONFIG_HIGHMEM and CONFIG_SERIAL_SH_SCI_DMA are enabled
 - This driver runs on the sci_dma_rx_push()

This issue was caused by virt_to_page(buf) in the sci_request_dma()
because this driver didn't check if the "buf" was valid or not.
So, this patch uses the "buf" from dma_alloc_coherent() as is, not page.

This patch also relves a WARNING issue that this driver runs on the
sci_rx_dma_release().

Signed-off-by: Yoshihiro Shimoda <[e-mail]>
---
 drivers/tty/serial/sh-sci.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

Patch

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 7e8fa37..e1ac4c3b 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -113,6 +113,8 @@  struct sci_port {
 	struct scatterlist		sg_tx;
 	unsigned int			sg_len_tx;
 	struct scatterlist		sg_rx[MAX_BUF_RX];
+	void				*rx_chunk;
+	u8				*rx_buf[MAX_BUF_RX];
 	unsigned int			total_buf_len_rx;
 	unsigned int			nr_buf_rx;
 	struct work_struct		work_tx;
@@ -1307,8 +1309,7 @@  static void sci_dma_tx_complete(void *arg)
 }

 /* Locking: called with port lock held */
-static int sci_dma_rx_push(struct sci_port *s, struct scatterlist *sg,
-			   unsigned int count)
+static int sci_dma_rx_push(struct sci_port *s, u8 *buf, unsigned int count)
 {
 	struct uart_port *port = &s->port;
 	struct tty_port *tport = &port->state->port;
@@ -1323,7 +1324,7 @@  static int sci_dma_rx_push(struct sci_port *s, struct scatterlist *sg,
 		return room;

 	for (i = 0; i < room; i++)
-		tty_insert_flip_char(tport, ((u8 *)sg_virt(sg))[i], TTY_NORMAL);
+		tty_insert_flip_char(tport, buf[i], TTY_NORMAL);

 	port->icount.rx += room;

@@ -1357,7 +1358,7 @@  static void sci_dma_rx_complete(void *arg)

 	active = sci_dma_rx_find_active(s);
 	if (active >= 0)
-		count = sci_dma_rx_push(s, &s->sg_rx[active],
+		count = sci_dma_rx_push(s, s->rx_buf[active],
 					sg_dma_len(&s->sg_rx[active]));

 	mod_timer(&s->rx_timer, jiffies + s->rx_timeout);
@@ -1385,8 +1386,7 @@  static void sci_rx_dma_release(struct sci_port *s, bool enable_pio)
 		s->cookie_rx[i] = -EINVAL;
 	if (sg_dma_address(&s->sg_rx[0])) {
 		dma_free_coherent(chan->device->dev, s->total_buf_len_rx,
-				  sg_virt(&s->sg_rx[0]),
-				  sg_dma_address(&s->sg_rx[0]));
+				  s->rx_chunk, sg_dma_address(&s->sg_rx[0]));
 		sg_dma_address(&s->sg_rx[0]) = 0;
 	}
 	dma_release_channel(chan);
@@ -1491,7 +1491,7 @@  static void work_fn_rx(struct work_struct *work)
 		dev_dbg(port->dev, "Read %zu bytes with cookie %d\n", read,
 			s->active_rx);

-		count = sci_dma_rx_push(s, &s->sg_rx[new], read);
+		count = sci_dma_rx_push(s, s->rx_buf[new], read);

 		if (count)
 			tty_flip_buffer_push(&port->state->port);
@@ -1823,12 +1823,13 @@  static void sci_request_dma(struct uart_port *port)
 			return;
 		}

+		s->rx_chunk = buf;
 		for (i = 0; i < s->nr_buf_rx; i++) {
 			struct scatterlist *sg = &s->sg_rx[i];

 			sg_init_table(sg, 1);
-			sg_set_page(sg, virt_to_page(buf), buflen[i],
-				    offset_in_page(buf));
+			s->rx_buf[i] = buf;
+			sg->length = buflen[i];
 			sg_dma_address(sg) = dma;

 			buf += buflen[i];