Received-SPF: pass (google.com: domain of linux-kernel+bounces-148991-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 17 Apr 2024 20:28:06 +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: <ZiAGpsldQMB-dKkn@smile.fi.intel.com>
References: <20240416162908.24180-1-fancer.lancer@gmail.com>
 <20240416162908.24180-3-fancer.lancer@gmail.com>
 <Zh7Hpuo-TzSmlz69@smile.fi.intel.com>
 <lzipslbrr4fkpqc3plfllltls2sy2mrlentp7clpjoppvgscoi@zlmysqym2kyb>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <lzipslbrr4fkpqc3plfllltls2sy2mrlentp7clpjoppvgscoi@zlmysqym2kyb>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Wed, Apr 17, 2024 at 08:13:59PM +0300, Serge Semin wrote:
> On Tue, Apr 16, 2024 at 09:47:02PM +0300, Andy Shevchenko wrote:
> > 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?
> 
> Sadly the situation isn't symmetrical.
> 
> The main idea of the solution proposed in this patch is to make sure
> that the DMA transactions would fill in the DMA FIFO in a way so in
> case of the suspension all the data would be delivered to the
> destination with nothing left in the DMA FIFO and the CFGx.FIFO_EMPTY
> flag would mean the real FIFO emptiness. It can be reached only if
> (CTLx.SRC_TR_WIDTH * CTLx.SRC_MSIZE) >= CTLx.DST_TR_WIDTH
> (calculated in the real values of course). But CTLx.SRC_MSIZE is only
> relevant for the flow-control/non-memory peripherals. Thus the

Oh, if it involves flow control, shouldn't you check for that as well?
We have a (PPC? IIRC) hardware with this IP that can have peripheral
as flow control.

> conditions under which the problem can be avoided are:
> 
> DMA_MEM_TO_DEV: CTLx.SRC_TR_WIDTH >= CTLx.DST_TR_WIDTH
> DMA_DEV_TO_MEM: CTLx.SRC_TR_WIDTH * CTLx.SRC_MSIZE >= CTLx.DST_TR_WIDTH
> 
> In both cases the non-memory peripheral side parameters (DEV-side)
> can't be changed because they are selected by the client drivers based
> on their specific logic (Device FIFO depth, watermarks, CSR widths,
> etc). But we can vary the memory-side transfer width as long as it's
> within the permitted limits.
> 
> In case of the DMA_MEM_TO_DEV transfers we can change the
> CTLx.SRC_TR_WIDTH because it represents the memory side transfer
> width. But if the maximum memory transfer width is smaller than the
> specified destination register width, there is nothing we can do. Thus
> returning the EINVAL error. Note this is mainly a hypothetical
> situation since normally the max width of the memory master xfers is
> greater than the peripheral master xfer max width (in my case it's 128
> and 32 bits respectively).
> 
> In case of the DMA_DEV_TO_MEM transfers we can change the CTLx.DST_TR_WIDTH
> parameter because it's the memory side. Thus if the maximum
> memory transfer width is smaller than the bursted source transfer,
> then we can stick to the maximum memory transfer width. But if it's
> greater than the bursted source transfer, we can freely reduce it
> so to support the safe suspension+disable DMA-usage pattern.
> 
> > 
> > > +	}
> > > +
> > > +	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.
> 
> No. DMAC ignores the _burst length_ parameters CTLx.SRC_MSIZE and
> CTLx.DEST_MSIZE for the memory side (also see my comment above):
> 
> "The CTLx.SRC_MSIZE and CTLx.DEST_MSIZE are properties valid only for
> peripherals with a handshaking interface; they cannot be used for
> defining the burst length for memory peripherals.
> 
> When the peripherals are memory, the DW_ahb_dmac is always the flow
> controller and uses DMA transfers to move blocks; thus the
> CTLx.SRC_MSIZE and CTLx.DEST_MSIZE values are not used for memory
> peripherals. The SRC_MSIZE/DEST_MSIZE limitations are used to
> accommodate devices that have limited resources, such as a FIFO.
> Memory does not normally have limitations similar to the FIFOs."
> 
> In my case the problem is in the CTLx.SRC_TR_WIDTH and
> CTLx.DST_TR_WIDTH values misconfiguration. Here is the crucial comment
> in the HW-manual about that (cited in the commit messages):
> 
> "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."
> 
> See Chapter 7.7 "Disabling a Channel Prior to Transfer Completion" of
> the DW DMAC HW manual for more details.

Got it. Maybe a little summary in the code to explain all this magic?

..

> > >  	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