Received-SPF: pass (google.com: domain of linux-kernel+bounces-147424-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 16 Apr 2024 21:47:02 +0300
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Serge Semin <fancer.lancer@gmail.com>
Cc: Viresh Kumar <vireshk@kernel.org>, Vinod Koul <vkoul@kernel.org>,
	Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>, dmaengine@vger.kernel.org,
	linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
	Viresh Kumar <viresh.kumar@linaro.org>
Subject: Re: [PATCH 2/4] dmaengine: dw: Add memory bus width verification
Message-ID: <Zh7Hpuo-TzSmlz69@smile.fi.intel.com>
References: <20240416162908.24180-1-fancer.lancer@gmail.com>
 <20240416162908.24180-3-fancer.lancer@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240416162908.24180-3-fancer.lancer@gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Tue, Apr 16, 2024 at 07:28:56PM +0300, Serge Semin wrote:
> Currently in case of the DEV_TO_MEM or MEM_TO_DEV DMA transfers the memory
> data width (single transfer width) is determined based on the buffer
> length, buffer base address or DMA master-channel max address width
> capability. It isn't enough in case of the channel disabling prior the
> block transfer is finished. Here is what DW AHB DMA IP-core databook says
> regarding the port suspension (DMA-transfer pause) implementation in the
> controller:
> 
> "When CTLx.SRC_TR_WIDTH < CTLx.DST_TR_WIDTH and the CFGx.CH_SUSP bit is
> high, the CFGx.FIFO_EMPTY is asserted once the contents of the FIFO do not
> permit a single word of CTLx.DST_TR_WIDTH to be formed. However, there may
> still be data in the channel FIFO, but not enough to form a single
> transfer of CTLx.DST_TR_WIDTH. In this scenario, once the channel is
> disabled, the remaining data in the channel FIFO is not transferred to the
> destination peripheral."
> 
> So in case if the port gets to be suspended and then disabled it's
> possible to have the data silently discarded even though the controller
> reported that FIFO is empty and the CTLx.BLOCK_TS indicated the dropped
> data already received from the source device. This looks as if the data
> somehow got lost on a way from the peripheral device to memory and causes
> problems for instance in the DW APB UART driver, which pauses and disables
> the DMA-transfer as soon as the recv data timeout happens. Here is the way
> it looks:
> 
>  Memory <------- DMA FIFO <------ UART FIFO <---------------- UART
>   DST_TR_WIDTH -+--------|       |         |
>                 |        |       |         |                No more data
>    Current lvl -+--------|       |---------+- DMA-burst lvl
>                 |        |       |---------+- Leftover data
>                 |        |       |---------+- SRC_TR_WIDTH
>                -+--------+-------+---------+
> 
> In the example above: no more data is getting received over the UART port
> and BLOCK_TS is not even close to be fully received; some data is left in
> the UART FIFO, but not enough to perform a bursted DMA-xfer to the DMA
> FIFO; some data is left in the DMA FIFO, but not enough to be passed
> further to the system memory in a single transfer. In this situation the
> 8250 UART driver catches the recv timeout interrupt, pauses the
> DMA-transfer and terminates it completely, after which the IRQ handler
> manually fetches the leftover data from the UART FIFO into the
> recv-buffer. But since the DMA-channel has been disabled with the data
> left in the DMA FIFO, that data will be just discarded and the recv-buffer
> will have a gap of the "current lvl" size in the recv-buffer at the tail
> of the lately received data portion. So the data will be lost just due to
> the misconfigured DMA transfer.
> 
> Note this is only relevant for the case of the transfer suspension and
> _disabling_. No problem will happen if the transfer will be re-enabled
> afterwards or the block transfer is fully completed. In the later case the
> "FIFO flush mode" will be executed at the transfer final stage in order to
> push out the data left in the DMA FIFO.
> 
> In order to fix the denoted problem the DW AHB DMA-engine driver needs to
> make sure that the _bursted_ source transfer width is greater or equal to
> the single destination transfer (note the HW databook describes more
> strict constraint than actually required). Since the peripheral-device
> side is prescribed by the client driver logic, the memory-side can be only
> used for that. The solution can be easily implemented for the DEV_TO_MEM
> transfers just by adjusting the memory-channel address width. Sadly it's
> not that easy for the MEM_TO_DEV transfers since the mem-to-dma burst size
> is normally dynamically determined by the controller. So the only thing
> that can be done is to make sure that memory-side address width can be
> greater than the peripheral device address width.

..

> +static int dwc_verify_m_buswidth(struct dma_chan *chan)
> +{
> +	struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
> +	struct dw_dma *dw = to_dw_dma(chan->device);
> +	u32 reg_width, reg_burst, mem_width;
> +
> +	mem_width = dw->pdata->data_width[dwc->dws.m_master];
> +
> +	/* Make sure src and dst word widths are coherent */
> +	if (dwc->dma_sconfig.direction == DMA_MEM_TO_DEV) {
> +		reg_width = dwc->dma_sconfig.dst_addr_width;
> +		if (mem_width < reg_width)
> +			return -EINVAL;
> +
> +		dwc->dma_sconfig.src_addr_width = mem_width;
> +	} else if (dwc->dma_sconfig.direction == DMA_DEV_TO_MEM) {
> +		reg_width = dwc->dma_sconfig.src_addr_width;
> +		reg_burst = rounddown_pow_of_two(dwc->dma_sconfig.src_maxburst);
> +
> +		dwc->dma_sconfig.dst_addr_width = min(mem_width, reg_width * reg_burst);

I understand the desire to go this way, but wouldn't be better to have
a symmetrical check and return an error?

> +	}
> +
> +	return 0;
> +}

IIRC MEM side of the DMA channel will ignore those in HW, so basically you are
(re-)using them purely for the __ffs() corrections.

..

>  	dwc->dma_sconfig.src_maxburst =
> -		clamp(dwc->dma_sconfig.src_maxburst, 0U, dwc->max_burst);
> +		clamp(dwc->dma_sconfig.src_maxburst, 1U, dwc->max_burst);
>  	dwc->dma_sconfig.dst_maxburst =
> -		clamp(dwc->dma_sconfig.dst_maxburst, 0U, dwc->max_burst);
> +		clamp(dwc->dma_sconfig.dst_maxburst, 1U, dwc->max_burst);

-- 
With Best Regards,
Andy Shevchenko