Hello, > On Thu, Oct 5, 2023, at 02:08, kernel test robot wrote: >> Hi Gregory, >> >> kernel test robot noticed the following build errors: >> >> [auto build test ERROR on robh/for-next] >> [also build test ERROR on lee-mfd/for-mfd-next linus/master v6.6-rc4 >> next-20231004] >> [cannot apply to lee-mfd/for-mfd-fixes] >> [If your patch is applied to the wrong git tree, kindly drop us a note. >> And when submitting patch, we suggest to use '--base' as documented in >> https://git-scm.com/docs/git-format-patch#_base_tree_information] > > >> If you fix the issue in a separate patch/commit (i.e. not just a new >> version of >> the same patch/commit), kindly add following tags >> | Reported-by: kernel test robot <lkp@xxxxxxxxx> >> | Closes: >> https://lore.kernel.org/oe-kbuild-all/202310050726.GDpZbMDO-lkp@xxxxxxxxx/ >> >> All error/warnings (new ones prefixed by >>): >> >> drivers/tty/serial/amba-pl011.c: In function 'pl011_sgbuf_init': >>>> drivers/tty/serial/amba-pl011.c:380:30: error: implicit declaration of function 'phys_to_page'; did you mean 'pfn_to_page'? [-Werror=implicit-function-declaration] >> 380 | sg_set_page(&sg->sg, phys_to_page(dma_addr), >> | ^~~~~~~~~~~~ >> | pfn_to_page > > I discussed this with Gregory on IRC, and prototyped a > possible fix. The issue was caused by the use of coherent memory > for the buffer and passing that into a scatterlist structure. > > Since there is no guarantee that the memory returned by > dma_alloc_coherent() is associated with a 'struct page', using > the architecture specific phys_to_page() is wrong, but using > virt_to_page() would be as well. > > An easy workaround is to stop using sg lists altogether and > just use the *_single() functions instead. This also simplifies > the code a bit since the scatterlists in this driver always have > only one entry anyway. > > Fixes: cb06ff102e2d7 ("ARM: PL011: Add support for Rx DMA buffer polling.") > Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx> I tested the following patch and it didn't introduce any regression and when using the same defconfig than the bot there is no more any error. So we can add, Tested-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxx> However, we don't use DMA on our platform for UART so the tests are limited. Linus; I know that you have a couple of boards that used the same UART controller. By any chance do you have some of them with DMA support that you could test ? Gregory PS: we are going to send series of clean-up and improvement for the pl011, but there are not mandatory for using the EyeQ5 platform. We hope being able to send them soon. > > diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c > index 0667e045ccb31..a3d92a91ff17d 100644 > --- a/drivers/tty/serial/amba-pl011.c > +++ b/drivers/tty/serial/amba-pl011.c > @@ -219,8 +219,9 @@ static struct vendor_data vendor_st = { > /* Deals with DMA transactions */ > > struct pl011_sgbuf { > - struct scatterlist sg; > - char *buf; > + dma_addr_t dma; > + size_t len; > + char *buf; > }; > > struct pl011_dmarx_data { > @@ -241,7 +242,8 @@ struct pl011_dmarx_data { > > struct pl011_dmatx_data { > struct dma_chan *chan; > - struct scatterlist sg; > + dma_addr_t dma; > + size_t len; > char *buf; > bool queued; > }; > @@ -369,18 +371,11 @@ static int pl011_fifo_to_tty(struct uart_amba_port *uap) > static int pl011_sgbuf_init(struct dma_chan *chan, struct pl011_sgbuf *sg, > enum dma_data_direction dir) > { > - dma_addr_t dma_addr; > - > - sg->buf = dma_alloc_coherent(chan->device->dev, > - PL011_DMA_BUFFER_SIZE, &dma_addr, GFP_KERNEL); > + sg->buf = dma_alloc_coherent(chan->device->dev, PL011_DMA_BUFFER_SIZE, > + &sg->dma, GFP_KERNEL); > if (!sg->buf) > return -ENOMEM; > - > - sg_init_table(&sg->sg, 1); > - sg_set_page(&sg->sg, phys_to_page(dma_addr), > - PL011_DMA_BUFFER_SIZE, offset_in_page(dma_addr)); > - sg_dma_address(&sg->sg) = dma_addr; > - sg_dma_len(&sg->sg) = PL011_DMA_BUFFER_SIZE; > + sg->len = PL011_DMA_BUFFER_SIZE; > > return 0; > } > @@ -390,8 +385,7 @@ static void pl011_sgbuf_free(struct dma_chan *chan, struct pl011_sgbuf *sg, > { > if (sg->buf) { > dma_free_coherent(chan->device->dev, > - PL011_DMA_BUFFER_SIZE, sg->buf, > - sg_dma_address(&sg->sg)); > + PL011_DMA_BUFFER_SIZE, sg->buf, sg->dma); > } > } > > @@ -552,8 +546,8 @@ static void pl011_dma_tx_callback(void *data) > > uart_port_lock_irqsave(&uap->port, &flags); > if (uap->dmatx.queued) > - dma_unmap_sg(dmatx->chan->device->dev, &dmatx->sg, 1, > - DMA_TO_DEVICE); > + dma_unmap_single(dmatx->chan->device->dev, dmatx->dma, > + dmatx->len, DMA_TO_DEVICE); > > dmacr = uap->dmacr; > uap->dmacr = dmacr & ~UART011_TXDMAE; > @@ -639,18 +633,19 @@ static int pl011_dma_tx_refill(struct uart_amba_port *uap) > memcpy(&dmatx->buf[first], &xmit->buf[0], second); > } > > - dmatx->sg.length = count; > - > - if (dma_map_sg(dma_dev->dev, &dmatx->sg, 1, DMA_TO_DEVICE) != 1) { > + dmatx->len = count; > + dmatx->dma = dma_map_single(dma_dev->dev, dmatx->buf, count, > + DMA_TO_DEVICE); > + if (dmatx->dma == DMA_MAPPING_ERROR) { > uap->dmatx.queued = false; > dev_dbg(uap->port.dev, "unable to map TX DMA\n"); > return -EBUSY; > } > > - desc = dmaengine_prep_slave_sg(chan, &dmatx->sg, 1, DMA_MEM_TO_DEV, > + desc = dmaengine_prep_slave_single(chan, dmatx->dma, dmatx->len, DMA_MEM_TO_DEV, > DMA_PREP_INTERRUPT | DMA_CTRL_ACK); > if (!desc) { > - dma_unmap_sg(dma_dev->dev, &dmatx->sg, 1, DMA_TO_DEVICE); > + dma_unmap_single(dma_dev->dev, dmatx->dma, dmatx->len, DMA_TO_DEVICE); > uap->dmatx.queued = false; > /* > * If DMA cannot be used right now, we complete this > @@ -813,8 +808,8 @@ __acquires(&uap->port.lock) > dmaengine_terminate_async(uap->dmatx.chan); > > if (uap->dmatx.queued) { > - dma_unmap_sg(uap->dmatx.chan->device->dev, &uap->dmatx.sg, 1, > - DMA_TO_DEVICE); > + dma_unmap_single(uap->dmatx.chan->device->dev, uap->dmatx.dma, > + uap->dmatx.len, DMA_TO_DEVICE); > uap->dmatx.queued = false; > uap->dmacr &= ~UART011_TXDMAE; > pl011_write(uap->dmacr, uap, REG_DMACR); > @@ -836,7 +831,7 @@ static int pl011_dma_rx_trigger_dma(struct uart_amba_port *uap) > /* Start the RX DMA job */ > sgbuf = uap->dmarx.use_buf_b ? > &uap->dmarx.sgbuf_b : &uap->dmarx.sgbuf_a; > - desc = dmaengine_prep_slave_sg(rxchan, &sgbuf->sg, 1, > + desc = dmaengine_prep_slave_single(rxchan, sgbuf->dma, sgbuf->len, > DMA_DEV_TO_MEM, > DMA_PREP_INTERRUPT | DMA_CTRL_ACK); > /* > @@ -886,7 +881,7 @@ static void pl011_dma_rx_chars(struct uart_amba_port *uap, > > if (uap->dmarx.poll_rate) { > /* The data can be taken by polling */ > - dmataken = sgbuf->sg.length - dmarx->last_residue; > + dmataken = sgbuf->len - dmarx->last_residue; > /* Recalculate the pending size */ > if (pending >= dmataken) > pending -= dmataken; > @@ -911,7 +906,7 @@ static void pl011_dma_rx_chars(struct uart_amba_port *uap, > > /* Reset the last_residue for Rx DMA poll */ > if (uap->dmarx.poll_rate) > - dmarx->last_residue = sgbuf->sg.length; > + dmarx->last_residue = sgbuf->len; > > /* > * Only continue with trying to read the FIFO if all DMA chars have > @@ -969,7 +964,7 @@ static void pl011_dma_rx_irq(struct uart_amba_port *uap) > pl011_write(uap->dmacr, uap, REG_DMACR); > uap->dmarx.running = false; > > - pending = sgbuf->sg.length - state.residue; > + pending = sgbuf->len - state.residue; > BUG_ON(pending > PL011_DMA_BUFFER_SIZE); > /* Then we terminate the transfer - we now know our residue */ > dmaengine_terminate_all(rxchan); > @@ -1015,7 +1010,7 @@ static void pl011_dma_rx_callback(void *data) > * the DMA irq handler. So we check the residue here. > */ > rxchan->device->device_tx_status(rxchan, dmarx->cookie, &state); > - pending = sgbuf->sg.length - state.residue; > + pending = sgbuf->len - state.residue; > BUG_ON(pending > PL011_DMA_BUFFER_SIZE); > /* Then we terminate the transfer - we now know our residue */ > dmaengine_terminate_all(rxchan); > @@ -1074,7 +1069,7 @@ static void pl011_dma_rx_poll(struct timer_list *t) > sgbuf = dmarx->use_buf_b ? &uap->dmarx.sgbuf_b : &uap->dmarx.sgbuf_a; > rxchan->device->device_tx_status(rxchan, dmarx->cookie, &state); > if (likely(state.residue < dmarx->last_residue)) { > - dmataken = sgbuf->sg.length - dmarx->last_residue; > + dmataken = sgbuf->len - dmarx->last_residue; > size = dmarx->last_residue - state.residue; > dma_count = tty_insert_flip_string(port, sgbuf->buf + dmataken, > size); > @@ -1123,7 +1118,7 @@ static void pl011_dma_startup(struct uart_amba_port *uap) > return; > } > > - sg_init_one(&uap->dmatx.sg, uap->dmatx.buf, PL011_DMA_BUFFER_SIZE); > + uap->dmatx.len = PL011_DMA_BUFFER_SIZE; > > /* The DMA buffer is now the FIFO the TTY subsystem can use */ > uap->port.fifosize = PL011_DMA_BUFFER_SIZE; > @@ -1200,8 +1195,9 @@ static void pl011_dma_shutdown(struct uart_amba_port *uap) > /* In theory, this should already be done by pl011_dma_flush_buffer */ > dmaengine_terminate_all(uap->dmatx.chan); > if (uap->dmatx.queued) { > - dma_unmap_sg(uap->dmatx.chan->device->dev, &uap->dmatx.sg, 1, > - DMA_TO_DEVICE); > + dma_unmap_single(uap->dmatx.chan->device->dev, > + uap->dmatx.dma, uap->dmatx.len, > + DMA_TO_DEVICE); > uap->dmatx.queued = false; > } > -- Gregory Clement, Bootlin Embedded Linux and Kernel engineering http://bootlin.com
