Message-ID: <87pm1nc05u.fsf@BL-laptop>
Date:   Mon, 09 Oct 2023 16:58:53 +0200
From:   Gregory CLEMENT <gregory.clement@...tlin.com>
To:     Arnd Bergmann <arnd@...db.de>, kernel test robot <lkp@...el.com>,
        Paul Burton <paulburton@...nel.org>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        linux-mips@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>
Cc:     oe-kbuild-all@...ts.linux.dev,
        Vladimir Kondratiev <vladimir.kondratiev@...el.com>,
        Tawfik Bayouk <tawfik.bayouk@...ileye.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Théo Lebrun <theo.lebrun@...tlin.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Chanho Min <chanho.min@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Russell King <linux@...linux.org.uk>
Subject: Re: [PATCH 10/11] MIPS: generic: Add support for Mobileye EyeQ5

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@...el.com>
>> | Closes: 
>> https://lore.kernel.org/oe-kbuild-all/202310050726.GDpZbMDO-lkp@intel.com/
>>
>> 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@...db.de>

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@...tlin.com>

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

Powered by blists - more mailing lists