Received-SPF: pass (google.com: domain of linux-kernel+bounces-149085-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Wed, 17 Apr 2024 21:52:42 +0300
From: Serge Semin <fancer.lancer@gmail.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Viresh Kumar <vireshk@kernel.org>, Vinod Koul <vkoul@kernel.org>, 
	Ilpo =?utf-8?B?SsOkcnZpbmVu?= <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: <nroj7c7hvo5ao5gfuububc2zqj7z4rpkoji5flhbrie3xrmgwg@6rhzllxwgj4w>
References: <20240416162908.24180-1-fancer.lancer@gmail.com>
 <20240416162908.24180-3-fancer.lancer@gmail.com>
 <Zh7Hpuo-TzSmlz69@smile.fi.intel.com>
 <lzipslbrr4fkpqc3plfllltls2sy2mrlentp7clpjoppvgscoi@zlmysqym2kyb>
 <ZiAGpsldQMB-dKkn@smile.fi.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZiAGpsldQMB-dKkn@smile.fi.intel.com>

On Wed, Apr 17, 2024 at 08:28:06PM +0300, Andy Shevchenko wrote:
> 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.

Oops. Sorry, I was wrong in saying that the peripheral is a
flow-controller. All my cases imply DMAC as flow-controller. The
correct sentence would be "But CTLx.SRC_MSIZE is only relevant for the
non-memory peripherals."

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

Will it be enough to add something like this:
/*
 * It's possible to have a data portion locked in the DMA FIFO in case
 * of the channel suspension. Subsequent channel disabling will cause
 * that data silent loss. In order to prevent that maintain the src
 * and dst transfer widths coherency by means of the relation:
 * (CTLx.SRC_TR_WIDTH * CTLx.SRC_MSIZE >= CTLx.DST_TR_WIDTH)
 */
?

-Serge(y)

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