[PATCH] Revert "dmaengine: pl330: add DMA_PAUSE feature"

[PATCH] Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Frank Mori Hess]

This reverts commit 88987d2c7534a0269f567fb101e6d71a08f0f01d.

The pl330.c pause implementation violates the dmaengine requirement
for no data loss, since it relies on the DMAKILL
instruction.  However, DMAKILL discards in-flight data from the
dma controller's fifo.  This is documented in the dma-330 manual
and I have observed it with hardware doing device-to-memory burst
transfers.  The discarded data may or may not show up in the
residue count, depending on timing (resulting in data corruption
effectively).

Signed-off-by: Frank Mori Hess <fmh6jj@gmail.com>
---
 drivers/dma/pl330.c | 28 ----------------------------
 1 file changed, 28 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 6237069001c4..f802bd3b0481 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2290,33 +2290,6 @@ static int pl330_terminate_all(struct dma_chan *chan)
 	return 0;
 }
 
-/*
- * We don't support DMA_RESUME command because of hardware
- * limitations, so after pausing the channel we cannot restore
- * it to active state. We have to terminate channel and setup
- * DMA transfer again. This pause feature was implemented to
- * allow safely read residue before channel termination.
- */
-static int pl330_pause(struct dma_chan *chan)
-{
-	struct dma_pl330_chan *pch = to_pchan(chan);
-	struct pl330_dmac *pl330 = pch->dmac;
-	unsigned long flags;
-
-	pm_runtime_get_sync(pl330->ddma.dev);
-	spin_lock_irqsave(&pch->lock, flags);
-
-	spin_lock(&pl330->lock);
-	_stop(pch->thread);
-	spin_unlock(&pl330->lock);
-
-	spin_unlock_irqrestore(&pch->lock, flags);
-	pm_runtime_mark_last_busy(pl330->ddma.dev);
-	pm_runtime_put_autosuspend(pl330->ddma.dev);
-
-	return 0;
-}
-
 static void pl330_free_chan_resources(struct dma_chan *chan)
 {
 	struct dma_pl330_chan *pch = to_pchan(chan);
@@ -3027,7 +3000,6 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 	pd->device_tx_status = pl330_tx_status;
 	pd->device_prep_slave_sg = pl330_prep_slave_sg;
 	pd->device_config = pl330_config;
-	pd->device_pause = pl330_pause;
 	pd->device_terminate_all = pl330_terminate_all;
 	pd->device_issue_pending = pl330_issue_pending;
 	pd->src_addr_widths = PL330_DMA_BUSWIDTHS;
-- 
2.11.0

Re: [PATCH] Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Vinod Koul]

On Sat, Apr 28, 2018 at 05:50:58PM -0400, Frank Mori Hess wrote:
> This reverts commit 88987d2c7534a0269f567fb101e6d71a08f0f01d.
> 
> The pl330.c pause implementation violates the dmaengine requirement
> for no data loss, since it relies on the DMAKILL
> instruction.  However, DMAKILL discards in-flight data from the
> dma controller's fifo.  This is documented in the dma-330 manual
> and I have observed it with hardware doing device-to-memory burst
> transfers.  The discarded data may or may not show up in the
> residue count, depending on timing (resulting in data corruption
> effectively).

I am dropping the orignal patch for queue, so no need for revert patch.

-- 
~Vinod

Re: [PATCH] Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Frank Mori Hess]

On Wed, May 2, 2018 at 12:32 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Sat, Apr 28, 2018 at 05:50:58PM -0400, Frank Mori Hess wrote:
>> This reverts commit 88987d2c7534a0269f567fb101e6d71a08f0f01d.
>>
>
> I am dropping the orignal patch for queue, so no need for revert patch.
>

I don't understand, 88987d2c7534a0269f567fb101e6d71a08f0f01d made it
into the mainline kernel in 2015?


-- 
Frank

Re: [PATCH] Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Vinod Koul]

On Wed, May 02, 2018 at 10:37:09AM -0400, Frank Mori Hess wrote:
> On Wed, May 2, 2018 at 12:32 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> > On Sat, Apr 28, 2018 at 05:50:58PM -0400, Frank Mori Hess wrote:
> >> This reverts commit 88987d2c7534a0269f567fb101e6d71a08f0f01d.
> >>
> >
> > I am dropping the orignal patch for queue, so no need for revert patch.
> >
> 
> I don't understand, 88987d2c7534a0269f567fb101e6d71a08f0f01d made it
> into the mainline kernel in 2015?

Ah looks like i misunderstood the revert, will fix this up

-- 
~Vinod

Re: [PATCH] Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Vinod Koul]

On Sat, Apr 28, 2018 at 05:50:58PM -0400, Frank Mori Hess wrote:
> This reverts commit 88987d2c7534a0269f567fb101e6d71a08f0f01d.
> 
> The pl330.c pause implementation violates the dmaengine requirement
> for no data loss, since it relies on the DMAKILL
> instruction.  However, DMAKILL discards in-flight data from the
> dma controller's fifo.  This is documented in the dma-330 manual
> and I have observed it with hardware doing device-to-memory burst
> transfers.  The discarded data may or may not show up in the
> residue count, depending on timing (resulting in data corruption
> effectively).


Applied, thanks

-- 
~Vinod

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Marek Szyprowski]

Hi Frank and Vinod,

On 2018-04-28 23:50, Frank Mori Hess wrote:
> This reverts commit 88987d2c7534a0269f567fb101e6d71a08f0f01d.
>
> The pl330.c pause implementation violates the dmaengine requirement
> for no data loss, since it relies on the DMAKILL
> instruction.  However, DMAKILL discards in-flight data from the
> dma controller's fifo.  This is documented in the dma-330 manual
> and I have observed it with hardware doing device-to-memory burst
> transfers.  The discarded data may or may not show up in the
> residue count, depending on timing (resulting in data corruption
> effectively).
>
> Signed-off-by: Frank Mori Hess <fmh6jj@gmail.com>

This revert completely breaks serial driver operation on almost all Exynos
SoCs, because serial driver relies on having PAUSE feature and proper
residue reporting from dma engine. Please drop it if possible.

> ---
>   drivers/dma/pl330.c | 28 ----------------------------
>   1 file changed, 28 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 6237069001c4..f802bd3b0481 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2290,33 +2290,6 @@ static int pl330_terminate_all(struct dma_chan *chan)
>   	return 0;
>   }
>   
> -/*
> - * We don't support DMA_RESUME command because of hardware
> - * limitations, so after pausing the channel we cannot restore
> - * it to active state. We have to terminate channel and setup
> - * DMA transfer again. This pause feature was implemented to
> - * allow safely read residue before channel termination.
> - */
> -static int pl330_pause(struct dma_chan *chan)
> -{
> -	struct dma_pl330_chan *pch = to_pchan(chan);
> -	struct pl330_dmac *pl330 = pch->dmac;
> -	unsigned long flags;
> -
> -	pm_runtime_get_sync(pl330->ddma.dev);
> -	spin_lock_irqsave(&pch->lock, flags);
> -
> -	spin_lock(&pl330->lock);
> -	_stop(pch->thread);
> -	spin_unlock(&pl330->lock);
> -
> -	spin_unlock_irqrestore(&pch->lock, flags);
> -	pm_runtime_mark_last_busy(pl330->ddma.dev);
> -	pm_runtime_put_autosuspend(pl330->ddma.dev);
> -
> -	return 0;
> -}
> -
>   static void pl330_free_chan_resources(struct dma_chan *chan)
>   {
>   	struct dma_pl330_chan *pch = to_pchan(chan);
> @@ -3027,7 +3000,6 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>   	pd->device_tx_status = pl330_tx_status;
>   	pd->device_prep_slave_sg = pl330_prep_slave_sg;
>   	pd->device_config = pl330_config;
> -	pd->device_pause = pl330_pause;
>   	pd->device_terminate_all = pl330_terminate_all;
>   	pd->device_issue_pending = pl330_issue_pending;
>   	pd->src_addr_widths = PL330_DMA_BUSWIDTHS;
>
>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Frank Mori Hess]

On Tue, May 8, 2018 at 5:04 AM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Hi Frank and Vinod,
>
> On 2018-04-28 23:50, Frank Mori Hess wrote:
>> This reverts commit 88987d2c7534a0269f567fb101e6d71a08f0f01d.
>>
>> The pl330.c pause implementation violates the dmaengine requirement
>> for no data loss, since it relies on the DMAKILL
>> instruction.  However, DMAKILL discards in-flight data from the
>> dma controller's fifo.  This is documented in the dma-330 manual
>> and I have observed it with hardware doing device-to-memory burst
>> transfers.  The discarded data may or may not show up in the
>> residue count, depending on timing (resulting in data corruption
>> effectively).
>>
>> Signed-off-by: Frank Mori Hess <fmh6jj@gmail.com>
>
> This revert completely breaks serial driver operation on almost all Exynos
> SoCs, because serial driver relies on having PAUSE feature and proper
> residue reporting from dma engine. Please drop it if possible.
>

It will cause the serial driver to not use the pl330.c driver for dma,
the serial driver will fall back on using the cpu.  This is
unfortunate, but the dma hardware simply does not support pause.  The
"nice" stop instruction DMAEND is not allowed to be inserted using the
debug instruction register.  The only possibility for implementing
pause would be to make the dma transfer do a DMAWFE (wait for event)
before every transfer.  Then you would need to devote another dma
thread to doing nothing but DMASEV (send event) to keep the transfer
going.  The pause could then DMAKILL the event-generating thread
rather than the transfer thread.  I don't know exactly what the
performance impact would be, but it couldn't be good.

The serial driver could be modified to still use dma for TX, since it
only needs pause for RX.  Also, if your serial hardware can report
exactly how many bytes it has sitting in its rx fifo, the serial
driver could be modified to use pause-less dma for RX.  This is
actually what I did for the custom serial hardware I'm using with a
dma-330, although our serial hardware has a very large rx fifo which
makes this scheme worthwhile.

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Vinod Koul]

On 08-05-18, 10:36, Frank Mori Hess wrote:
> On Tue, May 8, 2018 at 5:04 AM, Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
> > Hi Frank and Vinod,
> >
> > On 2018-04-28 23:50, Frank Mori Hess wrote:
> >> This reverts commit 88987d2c7534a0269f567fb101e6d71a08f0f01d.
> >>
> >> The pl330.c pause implementation violates the dmaengine requirement
> >> for no data loss, since it relies on the DMAKILL
> >> instruction.  However, DMAKILL discards in-flight data from the
> >> dma controller's fifo.  This is documented in the dma-330 manual
> >> and I have observed it with hardware doing device-to-memory burst
> >> transfers.  The discarded data may or may not show up in the
> >> residue count, depending on timing (resulting in data corruption
> >> effectively).
> >>
> >> Signed-off-by: Frank Mori Hess <fmh6jj@gmail.com>
> >
> > This revert completely breaks serial driver operation on almost all Exynos
> > SoCs, because serial driver relies on having PAUSE feature and proper
> > residue reporting from dma engine. Please drop it if possible.

Hi Marek,

I would appreciate if you can review the pl330 changes as that clearly seems to
impact you. This was in review for quite a bit

> It will cause the serial driver to not use the pl330.c driver for dma,
> the serial driver will fall back on using the cpu.  This is
> unfortunate, but the dma hardware simply does not support pause.  The
> "nice" stop instruction DMAEND is not allowed to be inserted using the
> debug instruction register.  The only possibility for implementing
> pause would be to make the dma transfer do a DMAWFE (wait for event)
> before every transfer.  Then you would need to devote another dma
> thread to doing nothing but DMASEV (send event) to keep the transfer
> going.  The pause could then DMAKILL the event-generating thread
> rather than the transfer thread.  I don't know exactly what the
> performance impact would be, but it couldn't be good.
> 
> The serial driver could be modified to still use dma for TX, since it
> only needs pause for RX.  Also, if your serial hardware can report
> exactly how many bytes it has sitting in its rx fifo, the serial
> driver could be modified to use pause-less dma for RX.  This is
> actually what I did for the custom serial hardware I'm using with a
> dma-330, although our serial hardware has a very large rx fifo which
> makes this scheme worthwhile.

That makes sense to me. If dma doesnt support, then why should SW claim broken
support..

-- 
~Vinod

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Marek Szyprowski]

Hi Frank,

On 2018-05-08 16:36, Frank Mori Hess wrote:
> On Tue, May 8, 2018 at 5:04 AM, Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> Hi Frank and Vinod,
>>
>> On 2018-04-28 23:50, Frank Mori Hess wrote:
>>> This reverts commit 88987d2c7534a0269f567fb101e6d71a08f0f01d.
>>>
>>> The pl330.c pause implementation violates the dmaengine requirement
>>> for no data loss, since it relies on the DMAKILL
>>> instruction.  However, DMAKILL discards in-flight data from the
>>> dma controller's fifo.  This is documented in the dma-330 manual
>>> and I have observed it with hardware doing device-to-memory burst
>>> transfers.  The discarded data may or may not show up in the
>>> residue count, depending on timing (resulting in data corruption
>>> effectively).
>>>
>>> Signed-off-by: Frank Mori Hess <fmh6jj@gmail.com>
>> This revert completely breaks serial driver operation on almost all Exynos
>> SoCs, because serial driver relies on having PAUSE feature and proper
>> residue reporting from dma engine. Please drop it if possible.
>>
> It will cause the serial driver to not use the pl330.c driver for dma,
> the serial driver will fall back on using the cpu.  This is
> unfortunate, but the dma hardware simply does not support pause.

I understand that pl330 doesn't support real PAUSE, but as it has been
implemented since 2015 the limited support is perfectly possible - just
to let serial driver to read the amount of data transferred.

Please note that DMA engine documentation clearly states that the best
residue granularity the driver might expect is BURST granularity. There
is no way to get the information about the data which still sits in the
DMA engine fifo when transfer is paused/terminated. This means that
the serial driver should set maxburst = 1 if it is interested in
getting exact number of bytes received/sent. With maxburst = 1 there
is no such thing as data loose in the DMA engine fifo.

This also means that there is no valid reason to revert the Robert's
commit. This is how this API works, so please don't break things which
worked for years.

I did some further research and indeed there are some other issues with
both pl330 driver and Samsung SoC serial driver:
1. pl330 incorrectly reported that it supports SEGMENT residue granularity
    instead of BURST granularity, but Samsung serial driver didn't check
    it.
2. Samsung driver incorrectly set src_maxburst to 16, what might result
    in lost data if dma engine does real burst transfers
3. Samsung driver doesn't check if DMA engine supports PAUSE feature and
    proper residue reporting granularity, so your revert simply breaks its
    operation.

Please note that till now everything worked fine a bit by luck, because
the pl330 driver didn't implement peripheral burst transfers and ignored
(incorrectly) configured maxburst.

I've checked other device drivers, which use pl330 DMA on Samsung SoCs and
besides serial, none of them configure maxburst > 1. When I forced such
configuration, none worked fine. I'm a bit confused what does it mean.
Either none of the Samsung SoC integrated peripherals support real burst
DMA transfers, or the PL330 in Samsung SoCs are somehow limited or
dysfunctional. There is already a quirk in pl330 for broken FLUSHP, but I
have no idea how to diagnose if this is the case or the problem is in the
SoC peripherals. I can live with maxburst set to 1 in those drivers as the
proper fix.

> The
> "nice" stop instruction DMAEND is not allowed to be inserted using the
> debug instruction register.  The only possibility for implementing
> pause would be to make the dma transfer do a DMAWFE (wait for event)
> before every transfer.  Then you would need to devote another dma
> thread to doing nothing but DMASEV (send event) to keep the transfer
> going.  The pause could then DMAKILL the event-generating thread
> rather than the transfer thread.  I don't know exactly what the
> performance impact would be, but it couldn't be good.

I agree that it doesn't make sense to implement real PAUSE with such high
cost.

> The serial driver could be modified to still use dma for TX, since it
> only needs pause for RX.  Also, if your serial hardware can report
> exactly how many bytes it has sitting in its rx fifo, the serial
> driver could be modified to use pause-less dma for RX.  This is
> actually what I did for the custom serial hardware I'm using with a
> dma-330, although our serial hardware has a very large rx fifo which
> makes this scheme worthwhile.

When the serial driver fifo size is 16 bytes, it doesn't really make any
sense to use DMA with such limited approach. The maxburst = 1 and proper
residue reporting is the only sane solution in such case.

On the other hand, if you have large fifo in serial driver then it still
MIGHT be faster to read all the fifo content with CPU instead of setting
up DMA engine/pl330 structures... One need to benchmark it on the real
hardware to decide.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Frank Mori Hess]

On Wed, May 9, 2018 at 9:19 AM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> I understand that pl330 doesn't support real PAUSE, but as it has been
> implemented since 2015 the limited support is perfectly possible - just
> to let serial driver to read the amount of data transferred.
>
> Please note that DMA engine documentation clearly states that the best
> residue granularity the driver might expect is BURST granularity. There
> is no way to get the information about the data which still sits in the
> DMA engine fifo when transfer is paused/terminated. This means that
> the serial driver should set maxburst = 1 if it is interested in
> getting exact number of bytes received/sent. With maxburst = 1 there
> is no such thing as data loose in the DMA engine fifo.

That is not my understanding of the dmaengine pause requirements, but
of course Vinod can speak authoritatively on this.  I also don't agree
that data loss cannot happen for single transfers.  It only becomes
less likely since there are fewer bytes in the dma controller fifo and
they stay there for a shorter period of time.  But even so, there is
nothing stopping the DMAKILL from killing the transfer thread after it
does a single byte load but before it does the associated single byte
store, as they are performed by separate instructions.  As far as your
serial port goes, the byte has been read by the host, even though it
never made it to memory, so it is gone forever.  The dma-330 does have
a load lock which prevents some operations from interrupting a
load/store combination, but in my observations DMAKILL does not
respect the load lock.


> 3. Samsung driver doesn't check if DMA engine supports PAUSE feature and
>     proper residue reporting granularity, so your revert simply breaks its
>     operation.

Oh, I was wrongly assuming you were talking about an 8250 based serial driver.

> I've checked other device drivers, which use pl330 DMA on Samsung SoCs and
> besides serial, none of them configure maxburst > 1. When I forced such
> configuration, none worked fine. I'm a bit confused what does it mean.
> Either none of the Samsung SoC integrated peripherals support real burst
> DMA transfers, or the PL330 in Samsung SoCs are somehow limited or
> dysfunctional. There is already a quirk in pl330 for broken FLUSHP, but I
> have no idea how to diagnose if this is the case or the problem is in the
> SoC peripherals. I can live with maxburst set to 1 in those drivers as the
> proper fix.

When the DMA-330 is instructed to do a peripheral burst transfer, it
ignores single transfer requests from the peripheral.  When it is
instructed to do a single transfer, it will do a single transfer in
response to either a burst request or a single request.  So unless the
peripheral actually supports burst requests, the transfer will just
wait forever for a burst request which never comes.

-- 
Frank

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Marek Szyprowski]

Hi Frank,

On 2018-05-09 19:48, Frank Mori Hess wrote:
> On Wed, May 9, 2018 at 9:19 AM, Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> I understand that pl330 doesn't support real PAUSE, but as it has been
>> implemented since 2015 the limited support is perfectly possible - just
>> to let serial driver to read the amount of data transferred.
>>
>> Please note that DMA engine documentation clearly states that the best
>> residue granularity the driver might expect is BURST granularity. There
>> is no way to get the information about the data which still sits in the
>> DMA engine fifo when transfer is paused/terminated. This means that
>> the serial driver should set maxburst = 1 if it is interested in
>> getting exact number of bytes received/sent. With maxburst = 1 there
>> is no such thing as data loose in the DMA engine fifo.
> That is not my understanding of the dmaengine pause requirements, but
> of course Vinod can speak authoritatively on this.

Basing on the comments in dma_residue_granularity enum documentation (see
include/linux/dmaengine.h) there is no better granularity of residue
reporting than BURST units. If driver needs byte accuracy, then it should
use MAXBURST=1 and DMA_SLAVE_BUSWIDTH_1_BYTE configuration.

> I also don't agree
> that data loss cannot happen for single transfers.  It only becomes
> less likely since there are fewer bytes in the dma controller fifo and
> they stay there for a shorter period of time.  But even so, there is
> nothing stopping the DMAKILL from killing the transfer thread after it
> does a single byte load but before it does the associated single byte
> store, as they are performed by separate instructions.  As far as your
> serial port goes, the byte has been read by the host, even though it
> never made it to memory, so it is gone forever.  The dma-330 does have
> a load lock which prevents some operations from interrupting a
> load/store combination, but in my observations DMAKILL does not
> respect the load lock.

For the last 3 years no one observed any issue with the current design
(single transfers with DMA_SLAVE_BUSWIDTH_1_BYTE). By removing this
feature we will loose ability to use DMA in the serial drivers, what is
mainly useful for low-power bluetooth operation (serial console is really
negligible case).

I agree that PL330 driver should not advertise PAUSE support, because
HW doesn't have such ability, but for now this is the only way to get
the number of bytes transferred after terminating a transfer. One would
need to change DMA engine framework API to fix this.

>> 3. Samsung driver doesn't check if DMA engine supports PAUSE feature and
>>      proper residue reporting granularity, so your revert simply breaks its
>>      operation.
> Oh, I was wrongly assuming you were talking about an 8250 based serial driver.

Nope, Samsung SoCs have a custom UART ip block.

>> I've checked other device drivers, which use pl330 DMA on Samsung SoCs and
>> besides serial, none of them configure maxburst > 1. When I forced such
>> configuration, none worked fine. I'm a bit confused what does it mean.
>> Either none of the Samsung SoC integrated peripherals support real burst
>> DMA transfers, or the PL330 in Samsung SoCs are somehow limited or
>> dysfunctional. There is already a quirk in pl330 for broken FLUSHP, but I
>> have no idea how to diagnose if this is the case or the problem is in the
>> SoC peripherals. I can live with maxburst set to 1 in those drivers as the
>> proper fix.
> When the DMA-330 is instructed to do a peripheral burst transfer, it
> ignores single transfer requests from the peripheral.  When it is
> instructed to do a single transfer, it will do a single transfer in
> response to either a burst request or a single request.  So unless the
> peripheral actually supports burst requests, the transfer will just
> wait forever for a burst request which never comes.

Okay, so if I get broken transfers when I forced BURST mode, then I can
assume that peripherals doesn't really support it.

Best regards

-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Frank Mori Hess]

On Thu, May 10, 2018 at 4:31 AM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Hi Frank,
>
> On 2018-05-09 19:48, Frank Mori Hess wrote:
>> On Wed, May 9, 2018 at 9:19 AM, Marek Szyprowski
>> <m.szyprowski@samsung.com> wrote:
>>> I understand that pl330 doesn't support real PAUSE, but as it has been
>>> implemented since 2015 the limited support is perfectly possible - just
>>> to let serial driver to read the amount of data transferred.
>>>
>>> Please note that DMA engine documentation clearly states that the best
>>> residue granularity the driver might expect is BURST granularity. There
>>> is no way to get the information about the data which still sits in the
>>> DMA engine fifo when transfer is paused/terminated. This means that
>>> the serial driver should set maxburst = 1 if it is interested in
>>> getting exact number of bytes received/sent. With maxburst = 1 there
>>> is no such thing as data loose in the DMA engine fifo.
>> That is not my understanding of the dmaengine pause requirements, but
>> of course Vinod can speak authoritatively on this.
>
> Basing on the comments in dma_residue_granularity enum documentation (see
> include/linux/dmaengine.h) there is no better granularity of residue
> reporting than BURST units. If driver needs byte accuracy, then it should
> use MAXBURST=1 and DMA_SLAVE_BUSWIDTH_1_BYTE configuration.

That's an interesting line of thought.  The 8250 serial driver clearly
assumes it can do the sequence

dma pause
read residue
dma terminate

without data loss.  It checks for the capabilities

cmd_pause
cmd_terminate
residue_granularity != DMA_RESIDUE_GRANULARITY_DESCRIPTOR

and some of the 8250 drivers like 8250_dw.c set a maxburst > 1.  If it
can't count on the pause/residue/terminate working without data loss
then it is just broken.  As is the 8250_omap.c driver.  Is the
description of dmaengine_pause in the documentation of "This pauses
activity on the DMA channel without data loss" to be interpreted as
"as long as you resume afterwards"?

>> I also don't agree
>> that data loss cannot happen for single transfers.  It only becomes
>> less likely since there are fewer bytes in the dma controller fifo and
>> they stay there for a shorter period of time.  But even so, there is
>> nothing stopping the DMAKILL from killing the transfer thread after it
>> does a single byte load but before it does the associated single byte
>> store, as they are performed by separate instructions.  As far as your
>> serial port goes, the byte has been read by the host, even though it
>> never made it to memory, so it is gone forever.  The dma-330 does have
>> a load lock which prevents some operations from interrupting a
>> load/store combination, but in my observations DMAKILL does not
>> respect the load lock.
>
> For the last 3 years no one observed any issue with the current design
> (single transfers with DMA_SLAVE_BUSWIDTH_1_BYTE). By removing this
> feature we will loose ability to use DMA in the serial drivers, what is
> mainly useful for low-power bluetooth operation (serial console is really
> negligible case).

It's not surprising it hasn't been reported.  It is a race that
requires a DMAKILL to be issued just as a byte is in-flight through
the dma controller.  The only reason a driver would pause the
un-resumeable pl330 would be because the driver either knows or
suspects no more data will be arriving and it gives up on the
transfer.  The only reason I noticed is we had a test which sent data
to a serial port, waited just long enough for the serial port rx to
timeout, then sent more data just as the pause was issuing DMAKILL.
And then the test did this a few hundred thousand times until it
noticed bad data.  Also, given the way 8250 rx timeouts work, a person
typing into a serial console wouldn't type fast enough to trigger the
bug unless the baud rate was set extremely low.  And if someone lost a
keystroke every 10^5 bytes, who would blame the dma controller?

I do admit I didn't do this test using single transfers, because our
goal was getting bursts to work.  Unfortunately, the test program
relies on some custom hardware I no longer have access to so I can't
easily re-run the test now.  However, since the DMA-330 manual
documents the DMAKILL instruction to:

"Remove all entries in the MFIFO for the DMA channel."
"Remove all entries in the read buffer queue and write buffer queue
for the DMA channel."

Relying on it to work as a safe pause for single transfers seems like
wishful thinking.  I sure didn't want to believe the DMA-330 couldn't
be safely paused, but I eventually had to resign myself to it.

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Marek Szyprowski]

Hi Frank,

On 2018-05-10 18:04, Frank Mori Hess wrote:
> On Thu, May 10, 2018 at 4:31 AM, Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 2018-05-09 19:48, Frank Mori Hess wrote:
>>> On Wed, May 9, 2018 at 9:19 AM, Marek Szyprowski
>>> <m.szyprowski@samsung.com> wrote:
>>>> I understand that pl330 doesn't support real PAUSE, but as it has been
>>>> implemented since 2015 the limited support is perfectly possible - just
>>>> to let serial driver to read the amount of data transferred.
>>>>
>>>> Please note that DMA engine documentation clearly states that the best
>>>> residue granularity the driver might expect is BURST granularity. There
>>>> is no way to get the information about the data which still sits in the
>>>> DMA engine fifo when transfer is paused/terminated. This means that
>>>> the serial driver should set maxburst = 1 if it is interested in
>>>> getting exact number of bytes received/sent. With maxburst = 1 there
>>>> is no such thing as data loose in the DMA engine fifo.
>>> That is not my understanding of the dmaengine pause requirements, but
>>> of course Vinod can speak authoritatively on this.
>> Basing on the comments in dma_residue_granularity enum documentation (see
>> include/linux/dmaengine.h) there is no better granularity of residue
>> reporting than BURST units. If driver needs byte accuracy, then it should
>> use MAXBURST=1 and DMA_SLAVE_BUSWIDTH_1_BYTE configuration.
> That's an interesting line of thought.  The 8250 serial driver clearly
> assumes it can do the sequence
>
> dma pause
> read residue
> dma terminate
>
> without data loss.

Right. From DMA engine API perspective this is the only valid way to 
read the
residue when you terminate the transfer.

>    It checks for the capabilities
>
> cmd_pause
> cmd_terminate
> residue_granularity != DMA_RESIDUE_GRANULARITY_DESCRIPTOR

Checking for cmd_pause is a bit too strict, because cmd_pause means 'driver
supports both pause and resume', but the serial driver doesn't use resume at
all. A check for residue_granularity != DMA_RESIDUE_GRANULARITY_DESCRIPTOR
is on the other hand a bit too loose.

> and some of the 8250 drivers like 8250_dw.c set a maxburst > 1.  If it
> can't count on the pause/residue/terminate working without data loss
> then it is just broken.  As is the 8250_omap.c driver.  Is the
> description of dmaengine_pause in the documentation of "This pauses
> activity on the DMA channel without data loss" to be interpreted as
> "as long as you resume afterwards"?

I assume that this requirement is for both - resuming and terminating.

>>> I also don't agree
>>> that data loss cannot happen for single transfers.  It only becomes
>>> less likely since there are fewer bytes in the dma controller fifo and
>>> they stay there for a shorter period of time.  But even so, there is
>>> nothing stopping the DMAKILL from killing the transfer thread after it
>>> does a single byte load but before it does the associated single byte
>>> store, as they are performed by separate instructions.  As far as your
>>> serial port goes, the byte has been read by the host, even though it
>>> never made it to memory, so it is gone forever.  The dma-330 does have
>>> a load lock which prevents some operations from interrupting a
>>> load/store combination, but in my observations DMAKILL does not
>>> respect the load lock.
>> For the last 3 years no one observed any issue with the current design
>> (single transfers with DMA_SLAVE_BUSWIDTH_1_BYTE). By removing this
>> feature we will loose ability to use DMA in the serial drivers, what is
>> mainly useful for low-power bluetooth operation (serial console is really
>> negligible case).
> It's not surprising it hasn't been reported.  It is a race that
> requires a DMAKILL to be issued just as a byte is in-flight through
> the dma controller.  The only reason a driver would pause the
> un-resumeable pl330 would be because the driver either knows or
> suspects no more data will be arriving and it gives up on the
> transfer.  The only reason I noticed is we had a test which sent data
> to a serial port, waited just long enough for the serial port rx to
> timeout, then sent more data just as the pause was issuing DMAKILL.
> And then the test did this a few hundred thousand times until it
> noticed bad data.  Also, given the way 8250 rx timeouts work, a person
> typing into a serial console wouldn't type fast enough to trigger the
> bug unless the baud rate was set extremely low.  And if someone lost a
> keystroke every 10^5 bytes, who would blame the dma controller?

Like I already said, console is not a proper use case for serial dma.
The more appropriate is bluetooth and still, I'm not aware of the issue
with the current code.

> I do admit I didn't do this test using single transfers, because our
> goal was getting bursts to work.  Unfortunately, the test program
> relies on some custom hardware I no longer have access to so I can't
> easily re-run the test now.  However, since the DMA-330 manual
> documents the DMAKILL instruction to:
>
> "Remove all entries in the MFIFO for the DMA channel."
> "Remove all entries in the read buffer queue and write buffer queue
> for the DMA channel."
>
> Relying on it to work as a safe pause for single transfers seems like
> wishful thinking.  I sure didn't want to believe the DMA-330 couldn't
> be safely paused, but I eventually had to resign myself to it.

Okay, so you don't have any evidence that DMA transfers done in single
reads/writes is broken with the current cmd_pause implementation.

This revert in fact at best disables DMA mode in the serial drivers (or
in worse case, i.e. Exynos, causes current drivers to do trashed
transfers due to lack of checking for the needed dma engine
capabilities).

Maybe instead of reverting support for it, there should be a check for
BURST vs. SINGLE mode and we would keep the current implementation for
SINGLE transfers?

Vinod, could you comment a bit this discussion from the DMA engine
maintainer perspective?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Frank Mori Hess]

On Fri, May 11, 2018 at 8:57 AM, Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Okay, so you don't have any evidence that DMA transfers done in single
> reads/writes is broken with the current cmd_pause implementation.

I think the easiest way to test this empirically would be to just hack
dmatest to do a bunch of mem-to-mem transfers which it pauses and
checks the copied data is consistent with the reported residue.  Also,
it would need to check the source/destination address registers in the
pl330 for evidence of bytes read but not written.  And the pl330.c
driver would need to be fixed to not ignore the requested maxburst
when doing mem-to-mem transfers.

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Vinod]

On 11-05-18, 14:57, Marek Szyprowski wrote:
> Hi Frank,
> 
> On 2018-05-10 18:04, Frank Mori Hess wrote:
> > On Thu, May 10, 2018 at 4:31 AM, Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> On 2018-05-09 19:48, Frank Mori Hess wrote:
> >>> On Wed, May 9, 2018 at 9:19 AM, Marek Szyprowski
> >>> <m.szyprowski@samsung.com> wrote:
> >>>> I understand that pl330 doesn't support real PAUSE, but as it has been
> >>>> implemented since 2015 the limited support is perfectly possible - just
> >>>> to let serial driver to read the amount of data transferred.
> >>>>
> >>>> Please note that DMA engine documentation clearly states that the best
> >>>> residue granularity the driver might expect is BURST granularity. There
> >>>> is no way to get the information about the data which still sits in the
> >>>> DMA engine fifo when transfer is paused/terminated. This means that
> >>>> the serial driver should set maxburst = 1 if it is interested in
> >>>> getting exact number of bytes received/sent. With maxburst = 1 there
> >>>> is no such thing as data loose in the DMA engine fifo.
> >>> That is not my understanding of the dmaengine pause requirements, but
> >>> of course Vinod can speak authoritatively on this.
> >> Basing on the comments in dma_residue_granularity enum documentation (see
> >> include/linux/dmaengine.h) there is no better granularity of residue
> >> reporting than BURST units. If driver needs byte accuracy, then it should
> >> use MAXBURST=1 and DMA_SLAVE_BUSWIDTH_1_BYTE configuration.
> > That's an interesting line of thought.  The 8250 serial driver clearly
> > assumes it can do the sequence
> >
> > dma pause
> > read residue
> > dma terminate
> >
> > without data loss.
> 
> Right. From DMA engine API perspective this is the only valid way to 
> read the
> residue when you terminate the transfer.

Not really, API allows you to read any point of time, you may support it or not
is different matter. Granularity of reporting is also a different point.
> 
> >    It checks for the capabilities
> >
> > cmd_pause
> > cmd_terminate
> > residue_granularity != DMA_RESIDUE_GRANULARITY_DESCRIPTOR
> 
> Checking for cmd_pause is a bit too strict, because cmd_pause means 'driver
> supports both pause and resume', but the serial driver doesn't use resume at
> all. A check for residue_granularity != DMA_RESIDUE_GRANULARITY_DESCRIPTOR
> is on the other hand a bit too loose.

thats true and it was added for audio which does pause/resume. If you feel it
helps in serial to get pause & resume capabilities independently we can split
them up, feel free to send a patch

For Pause/resume data loss is _not_ expected.

> > and some of the 8250 drivers like 8250_dw.c set a maxburst > 1.  If it
> > can't count on the pause/residue/terminate working without data loss
> > then it is just broken.  As is the 8250_omap.c driver.  Is the
> > description of dmaengine_pause in the documentation of "This pauses
> > activity on the DMA channel without data loss" to be interpreted as
> > "as long as you resume afterwards"?
> 
> I assume that this requirement is for both - resuming and terminating.

Terminate is abort, data loss may happen here.

> >>> I also don't agree
> >>> that data loss cannot happen for single transfers.  It only becomes
> >>> less likely since there are fewer bytes in the dma controller fifo and
> >>> they stay there for a shorter period of time.  But even so, there is
> >>> nothing stopping the DMAKILL from killing the transfer thread after it
> >>> does a single byte load but before it does the associated single byte
> >>> store, as they are performed by separate instructions.  As far as your
> >>> serial port goes, the byte has been read by the host, even though it
> >>> never made it to memory, so it is gone forever.  The dma-330 does have
> >>> a load lock which prevents some operations from interrupting a
> >>> load/store combination, but in my observations DMAKILL does not
> >>> respect the load lock.
> >> For the last 3 years no one observed any issue with the current design
> >> (single transfers with DMA_SLAVE_BUSWIDTH_1_BYTE). By removing this
> >> feature we will loose ability to use DMA in the serial drivers, what is
> >> mainly useful for low-power bluetooth operation (serial console is really
> >> negligible case).
> > It's not surprising it hasn't been reported.  It is a race that
> > requires a DMAKILL to be issued just as a byte is in-flight through
> > the dma controller.  The only reason a driver would pause the
> > un-resumeable pl330 would be because the driver either knows or
> > suspects no more data will be arriving and it gives up on the
> > transfer.  The only reason I noticed is we had a test which sent data
> > to a serial port, waited just long enough for the serial port rx to
> > timeout, then sent more data just as the pause was issuing DMAKILL.
> > And then the test did this a few hundred thousand times until it
> > noticed bad data.  Also, given the way 8250 rx timeouts work, a person
> > typing into a serial console wouldn't type fast enough to trigger the
> > bug unless the baud rate was set extremely low.  And if someone lost a
> > keystroke every 10^5 bytes, who would blame the dma controller?
> 
> Like I already said, console is not a proper use case for serial dma.
> The more appropriate is bluetooth and still, I'm not aware of the issue
> with the current code.

Why do you say serial is not important?

> > I do admit I didn't do this test using single transfers, because our
> > goal was getting bursts to work.  Unfortunately, the test program
> > relies on some custom hardware I no longer have access to so I can't
> > easily re-run the test now.  However, since the DMA-330 manual
> > documents the DMAKILL instruction to:
> >
> > "Remove all entries in the MFIFO for the DMA channel."
> > "Remove all entries in the read buffer queue and write buffer queue
> > for the DMA channel."
> >
> > Relying on it to work as a safe pause for single transfers seems like
> > wishful thinking.  I sure didn't want to believe the DMA-330 couldn't
> > be safely paused, but I eventually had to resign myself to it.
> 
> Okay, so you don't have any evidence that DMA transfers done in single
> reads/writes is broken with the current cmd_pause implementation.
> 
> This revert in fact at best disables DMA mode in the serial drivers (or
> in worse case, i.e. Exynos, causes current drivers to do trashed
> transfers due to lack of checking for the needed dma engine
> capabilities).
> 
> Maybe instead of reverting support for it, there should be a check for
> BURST vs. SINGLE mode and we would keep the current implementation for
> SINGLE transfers?
> 
> Vinod, could you comment a bit this discussion from the DMA engine
> maintainer perspective?

So I will try to summarise here:

- Pause/resume does not expect data loss
- Status can be queried any point of time
- Granularity reporting is upto device
- To support a use case and device limitations it is okay to support only
  singles.

-- 
~Vinod

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Marek Szyprowski]

Hi Vinod,

On 2018-05-15 08:21, Vinod wrote:
> On 11-05-18, 14:57, Marek Szyprowski wrote:
>> On 2018-05-10 18:04, Frank Mori Hess wrote:
>>> On Thu, May 10, 2018 at 4:31 AM, Marek Szyprowski
>>> <m.szyprowski@samsung.com> wrote:
>>>> On 2018-05-09 19:48, Frank Mori Hess wrote:
>>>>> On Wed, May 9, 2018 at 9:19 AM, Marek Szyprowski
>>>>> <m.szyprowski@samsung.com> wrote:
>>>>>> I understand that pl330 doesn't support real PAUSE, but as it has been
>>>>>> implemented since 2015 the limited support is perfectly possible - just
>>>>>> to let serial driver to read the amount of data transferred.
>>>>>>
>>>>>> Please note that DMA engine documentation clearly states that the best
>>>>>> residue granularity the driver might expect is BURST granularity. There
>>>>>> is no way to get the information about the data which still sits in the
>>>>>> DMA engine fifo when transfer is paused/terminated. This means that
>>>>>> the serial driver should set maxburst = 1 if it is interested in
>>>>>> getting exact number of bytes received/sent. With maxburst = 1 there
>>>>>> is no such thing as data loose in the DMA engine fifo.
>>>>> That is not my understanding of the dmaengine pause requirements, but
>>>>> of course Vinod can speak authoritatively on this.
>>>> Basing on the comments in dma_residue_granularity enum documentation (see
>>>> include/linux/dmaengine.h) there is no better granularity of residue
>>>> reporting than BURST units. If driver needs byte accuracy, then it should
>>>> use MAXBURST=1 and DMA_SLAVE_BUSWIDTH_1_BYTE configuration.
>>> That's an interesting line of thought.  The 8250 serial driver clearly
>>> assumes it can do the sequence
>>>
>>> dma pause
>>> read residue
>>> dma terminate
>>>
>>> without data loss.
>> Right. From DMA engine API perspective this is the only valid way to
>> read the
>> residue when you terminate the transfer.
> Not really, API allows you to read any point of time, you may support it or not
> is different matter. Granularity of reporting is also a different point.

I mean to read the residue in stable conditions (the incoming bytes has 
been either
read by dma or still sits in the uart fifo). Too bad that it is not 
possible to read
residue after terminating the transfer. This would remove the need for 
this fake
pause support.

>>>     It checks for the capabilities
>>>
>>> cmd_pause
>>> cmd_terminate
>>> residue_granularity != DMA_RESIDUE_GRANULARITY_DESCRIPTOR
>> Checking for cmd_pause is a bit too strict, because cmd_pause means 'driver
>> supports both pause and resume', but the serial driver doesn't use resume at
>> all. A check for residue_granularity != DMA_RESIDUE_GRANULARITY_DESCRIPTOR
>> is on the other hand a bit too loose.
> thats true and it was added for audio which does pause/resume. If you feel it
> helps in serial to get pause & resume capabilities independently we can split
> them up, feel free to send a patch

Okay, I will prepare a patch for this.

> For Pause/resume data loss is _not_ expected.
>
>>> and some of the 8250 drivers like 8250_dw.c set a maxburst > 1.  If it
>>> can't count on the pause/residue/terminate working without data loss
>>> then it is just broken.  As is the 8250_omap.c driver.  Is the
>>> description of dmaengine_pause in the documentation of "This pauses
>>> activity on the DMA channel without data loss" to be interpreted as
>>> "as long as you resume afterwards"?
>> I assume that this requirement is for both - resuming and terminating.
> Terminate is abort, data loss may happen here.

Okay. Then it is up to the drivers to rely on this or not.

>>>>> I also don't agree
>>>>> that data loss cannot happen for single transfers.  It only becomes
>>>>> less likely since there are fewer bytes in the dma controller fifo and
>>>>> they stay there for a shorter period of time.  But even so, there is
>>>>> nothing stopping the DMAKILL from killing the transfer thread after it
>>>>> does a single byte load but before it does the associated single byte
>>>>> store, as they are performed by separate instructions.  As far as your
>>>>> serial port goes, the byte has been read by the host, even though it
>>>>> never made it to memory, so it is gone forever.  The dma-330 does have
>>>>> a load lock which prevents some operations from interrupting a
>>>>> load/store combination, but in my observations DMAKILL does not
>>>>> respect the load lock.
>>>> For the last 3 years no one observed any issue with the current design
>>>> (single transfers with DMA_SLAVE_BUSWIDTH_1_BYTE). By removing this
>>>> feature we will loose ability to use DMA in the serial drivers, what is
>>>> mainly useful for low-power bluetooth operation (serial console is really
>>>> negligible case).
>>> It's not surprising it hasn't been reported.  It is a race that
>>> requires a DMAKILL to be issued just as a byte is in-flight through
>>> the dma controller.  The only reason a driver would pause the
>>> un-resumeable pl330 would be because the driver either knows or
>>> suspects no more data will be arriving and it gives up on the
>>> transfer.  The only reason I noticed is we had a test which sent data
>>> to a serial port, waited just long enough for the serial port rx to
>>> timeout, then sent more data just as the pause was issuing DMAKILL.
>>> And then the test did this a few hundred thousand times until it
>>> noticed bad data.  Also, given the way 8250 rx timeouts work, a person
>>> typing into a serial console wouldn't type fast enough to trigger the
>>> bug unless the baud rate was set extremely low.  And if someone lost a
>>> keystroke every 10^5 bytes, who would blame the dma controller?
>> Like I already said, console is not a proper use case for serial dma.
>> The more appropriate is bluetooth and still, I'm not aware of the issue
>> with the current code.
> Why do you say serial is not important?

I mean that using DMA engine for uart/serial device for implementing only a
console support is a bit pointless, because typically console doesn't 
transfer
much data and overhead of using DMA mode (setting up dma engine) is bigger
than doing all the transfers in PIO mode.

>>> I do admit I didn't do this test using single transfers, because our
>>> goal was getting bursts to work.  Unfortunately, the test program
>>> relies on some custom hardware I no longer have access to so I can't
>>> easily re-run the test now.  However, since the DMA-330 manual
>>> documents the DMAKILL instruction to:
>>>
>>> "Remove all entries in the MFIFO for the DMA channel."
>>> "Remove all entries in the read buffer queue and write buffer queue
>>> for the DMA channel."
>>>
>>> Relying on it to work as a safe pause for single transfers seems like
>>> wishful thinking.  I sure didn't want to believe the DMA-330 couldn't
>>> be safely paused, but I eventually had to resign myself to it.
>> Okay, so you don't have any evidence that DMA transfers done in single
>> reads/writes is broken with the current cmd_pause implementation.
>>
>> This revert in fact at best disables DMA mode in the serial drivers (or
>> in worse case, i.e. Exynos, causes current drivers to do trashed
>> transfers due to lack of checking for the needed dma engine
>> capabilities).
>>
>> Maybe instead of reverting support for it, there should be a check for
>> BURST vs. SINGLE mode and we would keep the current implementation for
>> SINGLE transfers?
>>
>> Vinod, could you comment a bit this discussion from the DMA engine
>> maintainer perspective?
> So I will try to summarise here:
>
> - Pause/resume does not expect data loss
> - Status can be queried any point of time
> - Granularity reporting is upto device
> - To support a use case and device limitations it is okay to support only
>    singles.

What about the revert this thread is about? I would really like to have some
evidence of broken data in single transfer modes before removing 
functionality
that was present for years.

The discussed use case (pause+terminate) is crucial for serial devices at
least on Samsung SoCs and this revert break them.

Quick grep shows that PL330 is being used by a few other SoC families too,
but I didn't check if any of them use it for serial device.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Vinod]

On 15-05-18, 14:24, Marek Szyprowski wrote:
> Hi Vinod,
> 
> On 2018-05-15 08:21, Vinod wrote:
> > On 11-05-18, 14:57, Marek Szyprowski wrote:
> >> On 2018-05-10 18:04, Frank Mori Hess wrote:
> >>> On Thu, May 10, 2018 at 4:31 AM, Marek Szyprowski
> >>> <m.szyprowski@samsung.com> wrote:
> >>>> On 2018-05-09 19:48, Frank Mori Hess wrote:
> >>>>> On Wed, May 9, 2018 at 9:19 AM, Marek Szyprowski
> >>>>> <m.szyprowski@samsung.com> wrote:
> >>>>>> I understand that pl330 doesn't support real PAUSE, but as it has been
> >>>>>> implemented since 2015 the limited support is perfectly possible - just
> >>>>>> to let serial driver to read the amount of data transferred.
> >>>>>>
> >>>>>> Please note that DMA engine documentation clearly states that the best
> >>>>>> residue granularity the driver might expect is BURST granularity. There
> >>>>>> is no way to get the information about the data which still sits in the
> >>>>>> DMA engine fifo when transfer is paused/terminated. This means that
> >>>>>> the serial driver should set maxburst = 1 if it is interested in
> >>>>>> getting exact number of bytes received/sent. With maxburst = 1 there
> >>>>>> is no such thing as data loose in the DMA engine fifo.
> >>>>> That is not my understanding of the dmaengine pause requirements, but
> >>>>> of course Vinod can speak authoritatively on this.
> >>>> Basing on the comments in dma_residue_granularity enum documentation (see
> >>>> include/linux/dmaengine.h) there is no better granularity of residue
> >>>> reporting than BURST units. If driver needs byte accuracy, then it should
> >>>> use MAXBURST=1 and DMA_SLAVE_BUSWIDTH_1_BYTE configuration.
> >>> That's an interesting line of thought.  The 8250 serial driver clearly
> >>> assumes it can do the sequence
> >>>
> >>> dma pause
> >>> read residue
> >>> dma terminate
> >>>
> >>> without data loss.
> >> Right. From DMA engine API perspective this is the only valid way to
> >> read the
> >> residue when you terminate the transfer.
> > Not really, API allows you to read any point of time, you may support it or not
> > is different matter. Granularity of reporting is also a different point.
> 
> I mean to read the residue in stable conditions (the incoming bytes has 
> been either
> read by dma or still sits in the uart fifo). Too bad that it is not 
> possible to read
> residue after terminating the transfer. This would remove the need for 
> this fake
> pause support.

you can read the residue before you call terminate. Terminate is basically a
cleanup job, so reading after that makes no sense, maybe you need to modify
calling sequence..

> 
> >>>     It checks for the capabilities
> >>>
> >>> cmd_pause
> >>> cmd_terminate
> >>> residue_granularity != DMA_RESIDUE_GRANULARITY_DESCRIPTOR
> >> Checking for cmd_pause is a bit too strict, because cmd_pause means 'driver
> >> supports both pause and resume', but the serial driver doesn't use resume at
> >> all. A check for residue_granularity != DMA_RESIDUE_GRANULARITY_DESCRIPTOR
> >> is on the other hand a bit too loose.
> > thats true and it was added for audio which does pause/resume. If you feel it
> > helps in serial to get pause & resume capabilities independently we can split
> > them up, feel free to send a patch
> 
> Okay, I will prepare a patch for this.
> 
> > For Pause/resume data loss is _not_ expected.
> >
> >>> and some of the 8250 drivers like 8250_dw.c set a maxburst > 1.  If it
> >>> can't count on the pause/residue/terminate working without data loss
> >>> then it is just broken.  As is the 8250_omap.c driver.  Is the
> >>> description of dmaengine_pause in the documentation of "This pauses
> >>> activity on the DMA channel without data loss" to be interpreted as
> >>> "as long as you resume afterwards"?
> >> I assume that this requirement is for both - resuming and terminating.
> > Terminate is abort, data loss may happen here.
> 
> Okay. Then it is up to the drivers to rely on this or not.

Yes...

> 
> >>>>> I also don't agree
> >>>>> that data loss cannot happen for single transfers.  It only becomes
> >>>>> less likely since there are fewer bytes in the dma controller fifo and
> >>>>> they stay there for a shorter period of time.  But even so, there is
> >>>>> nothing stopping the DMAKILL from killing the transfer thread after it
> >>>>> does a single byte load but before it does the associated single byte
> >>>>> store, as they are performed by separate instructions.  As far as your
> >>>>> serial port goes, the byte has been read by the host, even though it
> >>>>> never made it to memory, so it is gone forever.  The dma-330 does have
> >>>>> a load lock which prevents some operations from interrupting a
> >>>>> load/store combination, but in my observations DMAKILL does not
> >>>>> respect the load lock.
> >>>> For the last 3 years no one observed any issue with the current design
> >>>> (single transfers with DMA_SLAVE_BUSWIDTH_1_BYTE). By removing this
> >>>> feature we will loose ability to use DMA in the serial drivers, what is
> >>>> mainly useful for low-power bluetooth operation (serial console is really
> >>>> negligible case).
> >>> It's not surprising it hasn't been reported.  It is a race that
> >>> requires a DMAKILL to be issued just as a byte is in-flight through
> >>> the dma controller.  The only reason a driver would pause the
> >>> un-resumeable pl330 would be because the driver either knows or
> >>> suspects no more data will be arriving and it gives up on the
> >>> transfer.  The only reason I noticed is we had a test which sent data
> >>> to a serial port, waited just long enough for the serial port rx to
> >>> timeout, then sent more data just as the pause was issuing DMAKILL.
> >>> And then the test did this a few hundred thousand times until it
> >>> noticed bad data.  Also, given the way 8250 rx timeouts work, a person
> >>> typing into a serial console wouldn't type fast enough to trigger the
> >>> bug unless the baud rate was set extremely low.  And if someone lost a
> >>> keystroke every 10^5 bytes, who would blame the dma controller?
> >> Like I already said, console is not a proper use case for serial dma.
> >> The more appropriate is bluetooth and still, I'm not aware of the issue
> >> with the current code.
> > Why do you say serial is not important?
> 
> I mean that using DMA engine for uart/serial device for implementing only a
> console support is a bit pointless, because typically console doesn't 
> transfer
> much data and overhead of using DMA mode (setting up dma engine) is bigger
> than doing all the transfers in PIO mode.

Well that depends on your system and outweighing dma complexity vs perf gains
you get..

> >>> I do admit I didn't do this test using single transfers, because our
> >>> goal was getting bursts to work.  Unfortunately, the test program
> >>> relies on some custom hardware I no longer have access to so I can't
> >>> easily re-run the test now.  However, since the DMA-330 manual
> >>> documents the DMAKILL instruction to:
> >>>
> >>> "Remove all entries in the MFIFO for the DMA channel."
> >>> "Remove all entries in the read buffer queue and write buffer queue
> >>> for the DMA channel."
> >>>
> >>> Relying on it to work as a safe pause for single transfers seems like
> >>> wishful thinking.  I sure didn't want to believe the DMA-330 couldn't
> >>> be safely paused, but I eventually had to resign myself to it.
> >> Okay, so you don't have any evidence that DMA transfers done in single
> >> reads/writes is broken with the current cmd_pause implementation.
> >>
> >> This revert in fact at best disables DMA mode in the serial drivers (or
> >> in worse case, i.e. Exynos, causes current drivers to do trashed
> >> transfers due to lack of checking for the needed dma engine
> >> capabilities).
> >>
> >> Maybe instead of reverting support for it, there should be a check for
> >> BURST vs. SINGLE mode and we would keep the current implementation for
> >> SINGLE transfers?
> >>
> >> Vinod, could you comment a bit this discussion from the DMA engine
> >> maintainer perspective?
> > So I will try to summarise here:
> >
> > - Pause/resume does not expect data loss
> > - Status can be queried any point of time
> > - Granularity reporting is upto device
> > - To support a use case and device limitations it is okay to support only
> >    singles.
> 
> What about the revert this thread is about? I would really like to have some
> evidence of broken data in single transfer modes before removing 
> functionality
> that was present for years.

I do not mind dropping this, do both of you agree to that and agree to fix other
issues?

> The discussed use case (pause+terminate) is crucial for serial devices at
> least on Samsung SoCs and this revert break them.
> 
> Quick grep shows that PL330 is being used by a few other SoC families too,
> but I didn't check if any of them use it for serial device.
> 
> Best regards
> -- 
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland

-- 
~Vinod

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Frank Mori Hess]

On Tue, May 15, 2018 at 2:21 AM, Vinod <vkoul@kernel.org> wrote:
>
> For Pause/resume data loss is _not_ expected.
>
>> > and some of the 8250 drivers like 8250_dw.c set a maxburst > 1.  If it
>> > can't count on the pause/residue/terminate working without data loss
>> > then it is just broken.  As is the 8250_omap.c driver.  Is the
>> > description of dmaengine_pause in the documentation of "This pauses
>> > activity on the DMA channel without data loss" to be interpreted as
>> > "as long as you resume afterwards"?
>>
>> I assume that this requirement is for both - resuming and terminating.
>
> Terminate is abort, data loss may happen here.

Wait, are you saying if you do

dma pause
read residue
dma terminate

then it is acceptable for there to be data loss, because it can be
blamed on the terminate?  In that case the usage of dmaengine in all
the 8250 serial drivers is broken.  Every time there is a break in the
incoming rx data, the 8250 driver does pause/residue/terminate which
could potentially cause data from the incoming data stream to be
dropped.

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Vinod Koul]

On 15-05-18, 11:50, Frank Mori Hess wrote:
> On Tue, May 15, 2018 at 2:21 AM, Vinod <vkoul@kernel.org> wrote:
> >
> > For Pause/resume data loss is _not_ expected.
> >
> >> > and some of the 8250 drivers like 8250_dw.c set a maxburst > 1.  If it
> >> > can't count on the pause/residue/terminate working without data loss
> >> > then it is just broken.  As is the 8250_omap.c driver.  Is the
> >> > description of dmaengine_pause in the documentation of "This pauses
> >> > activity on the DMA channel without data loss" to be interpreted as
> >> > "as long as you resume afterwards"?
> >>
> >> I assume that this requirement is for both - resuming and terminating.
> >
> > Terminate is abort, data loss may happen here.
> 
> Wait, are you saying if you do
> 
> dma pause

no data loss
> read residue

here as well
> dma terminate

Oh yes, we aborted...
> 
> then it is acceptable for there to be data loss, because it can be
> blamed on the terminate?  In that case the usage of dmaengine in all
> the 8250 serial drivers is broken.  Every time there is a break in the
> incoming rx data, the 8250 driver does pause/residue/terminate which
> could potentially cause data from the incoming data stream to be
> dropped.

As I said terminate is abort. It cleans up the channel and is supposed to
used for cleanup not for stopping. See Documentation:

- device_terminate_all

  - Aborts all the pending and ongoing transfers on the channel

  - For aborted transfers the complete callback should not be called

  - Can be called from atomic context or from within a complete
    callback of a descriptor. Must not sleep. Drivers must be able
    to handle this correctly.

  - Termination may be asynchronous. The driver does not have to
    wait until the currently active transfer has completely stopped.
    See device_synchronize.

Thanks
-- 
~Vinod

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Frank Mori Hess]

Sorry to keep coming back to this, but I'm experiencing a bit of
incredulity that you are saying what you seem to be saying.  You seem
to be saying dmaengine provides no way to permanently stop a transfer
safely other than transferring the full number of bytes initially
requested.  So the proper resolution is the 8250 serial driver needs
to remove rx dma support, because they are just trying to do something
that is not supported.

On Thu, May 17, 2018 at 12:19 AM, Vinod Koul <vinod.koul@linaro.org> wrote:
>> > Terminate is abort, data loss may happen here.
>>
>> Wait, are you saying if you do
>>
>> dma pause
>
> no data loss
>> read residue
>
> here as well
>> dma terminate
>
> Oh yes, we aborted...
>>

I see two ways of interpreting what you are saying. First, from the
point of view of the user of the dmaengine api.  From this point of
view it is impossible for data loss to occur during pause or reading
the residue, so saying they cause no data loss during
pause/residue/terminate is meaningless.  This is because the user
can't confirm any data loss until after they have read the residue and
the transfer is terminated, since optimistically the data may still be
available if only the user would resume and allow the transfer to
continue.

Second there is the interpretation I want to believe.  This is "no
data loss on pause" means that after the pause, no data has been
discarded by the dma controller hardware, in fact all the data it has
read before being paused has been fully transferred to its
destination.  Reading the residue while paused gives you an accurate,
up-to-date state of the paused transfer.  Then finally, although in
general dma terminate causes data loss, it does not in this case since
we terminated while we were paused and read the up-to-date residue.
This is the interpretation implicit in the 8250 serial driver.

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Lars-Peter Clausen]

On 05/17/2018 06:20 PM, Frank Mori Hess wrote:
> Sorry to keep coming back to this, but I'm experiencing a bit of
> incredulity that you are saying what you seem to be saying.  You seem
> to be saying dmaengine provides no way to permanently stop a transfer
> safely other than transferring the full number of bytes initially
> requested.  So the proper resolution is the 8250 serial driver needs
> to remove rx dma support, because they are just trying to do something
> that is not supported.

The problem is not so much on the software side but even more so on the
hardware side. Not all hardware even supports aborting a transfer with no
data loss because there is no precise measurement of how much data has been
transferred. The residue that is reported is always the lower bound, this
much has been transferred but it might actually have been more.

And even if hardware supports precise residue reporting there often is no
synchronization mechanism to ensure that there is no pending data anywhere
in the pipeline. This doesn't even have to be inside the DMA's buffers, the
DMA might have send the data to the peripheral but it has not arrived at the
peripheral side.

If the peripheral has a memory mapped FIFO there will typically be some
confirmation that the data was received, but with a dedicated DMA bus
between the DMA controller and the peripheral that is often not the case.
And for the first case the DMA controller also needs to actually expose the
information how many acknowledgements it has received.

Because there are basically two different resides, residue one is how much
data has been read from/written to memory and the other is how much data has
been read from/written to the peripheral. Any differences between the two
will account for in flight data.

What I'd recommend is to add a flush_sync() operation to the DMA interface.
This function would wait until all in flight data transfers have been
completed. That makes it safe to call terminate() without the risk of
loosing data and also allow accurate residue reporting.

Although I'd expect that there is fair risk that implementations will not
get flush_sync() 100% correct because it requires in-depth knowledge about
the internal behavior of the DMA which is often not documented.

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Frank Mori Hess]

On Thu, May 17, 2018 at 1:22 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 05/17/2018 06:20 PM, Frank Mori Hess wrote:
> The problem is not so much on the software side but even more so on the
> hardware side. Not all hardware even supports aborting a transfer with no
> data loss because there is no precise measurement of how much data has been
> transferred. The residue that is reported is always the lower bound, this
> much has been transferred but it might actually have been more.

I'd just like to point out, if the pl330 driver actually did report a
upper bound on the residue (lower bound on bytes transferred) that
would also blow up Marek's samsung serial driver use case.  Instead of
being in a situation where data loss might occur rarely due to a race
condition, they would be in a situation where data loss occurs every
time they stop a transfer.  Not that such a residue reporting would be
incorrect though, the pl330 driver doesn't even advertise burst level
granularity with its residue reporting.

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Vinod]

On 17-05-18, 12:20, Frank Mori Hess wrote:
> Sorry to keep coming back to this, but I'm experiencing a bit of
> incredulity that you are saying what you seem to be saying.  You seem
> to be saying dmaengine provides no way to permanently stop a transfer
> safely other than transferring the full number of bytes initially
> requested.  So the proper resolution is the 8250 serial driver needs
> to remove rx dma support, because they are just trying to do something
> that is not supported.
> 
> On Thu, May 17, 2018 at 12:19 AM, Vinod Koul <vinod.koul@linaro.org> wrote:
> >> > Terminate is abort, data loss may happen here.
> >>
> >> Wait, are you saying if you do
> >>
> >> dma pause
> >
> > no data loss
> >> read residue
> >
> > here as well
> >> dma terminate
> >
> > Oh yes, we aborted...
> >>
> 
> I see two ways of interpreting what you are saying. First, from the
> point of view of the user of the dmaengine api.  From this point of
> view it is impossible for data loss to occur during pause or reading
> the residue, so saying they cause no data loss during
> pause/residue/terminate is meaningless.  This is because the user
> can't confirm any data loss until after they have read the residue and
> the transfer is terminated, since optimistically the data may still be
> available if only the user would resume and allow the transfer to
> continue.
> 
> Second there is the interpretation I want to believe.  This is "no
> data loss on pause" means that after the pause, no data has been
> discarded by the dma controller hardware, in fact all the data it has
> read before being paused has been fully transferred to its
> destination.  Reading the residue while paused gives you an accurate,
> up-to-date state of the paused transfer.  Then finally, although in
> general dma terminate causes data loss, it does not in this case since
> we terminated while we were paused and read the up-to-date residue.
> This is the interpretation implicit in the 8250 serial driver.

You are simply mixing things up! On Pause we don't expect data loss, as user can
resume the transfer. This means as you rightly guessed, the DMA HW should not drop
any data, nor should SW.

Now if you want to read residue at this point it is perfectly valid. But if you
decide to terminate the channel (yes it is terminate_all API), we abort and don't
have context to report back!

As Lars rightly pointed out, residue calculation are very tricky, DMA fifo may
have data, some data may be in device FIFO, so residue is always from DMA point
of view and may differ from device view (more or less depending upon direction)

Now if you require to add more features for your usecase, please do feel free to
send a patch. The framework can always be improved, we haven't solved world
hunger yet!

-- 
~Vinod

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Marek Szyprowski]

Hi Vinod,

On 2018-05-18 06:03, Vinod wrote:
> On 17-05-18, 12:20, Frank Mori Hess wrote:
>> Sorry to keep coming back to this, but I'm experiencing a bit of
>> incredulity that you are saying what you seem to be saying.  You seem
>> to be saying dmaengine provides no way to permanently stop a transfer
>> safely other than transferring the full number of bytes initially
>> requested.  So the proper resolution is the 8250 serial driver needs
>> to remove rx dma support, because they are just trying to do something
>> that is not supported.

...

>> I see two ways of interpreting what you are saying. First, from the
>> point of view of the user of the dmaengine api.  From this point of
>> view it is impossible for data loss to occur during pause or reading
>> the residue, so saying they cause no data loss during
>> pause/residue/terminate is meaningless.  This is because the user
>> can't confirm any data loss until after they have read the residue and
>> the transfer is terminated, since optimistically the data may still be
>> available if only the user would resume and allow the transfer to
>> continue.
>>
>> Second there is the interpretation I want to believe.  This is "no
>> data loss on pause" means that after the pause, no data has been
>> discarded by the dma controller hardware, in fact all the data it has
>> read before being paused has been fully transferred to its
>> destination.  Reading the residue while paused gives you an accurate,
>> up-to-date state of the paused transfer.  Then finally, although in
>> general dma terminate causes data loss, it does not in this case since
>> we terminated while we were paused and read the up-to-date residue.
>> This is the interpretation implicit in the 8250 serial driver.
> You are simply mixing things up! On Pause we don't expect data loss, as user can
> resume the transfer. This means as you rightly guessed, the DMA HW should not drop
> any data, nor should SW.
>
> Now if you want to read residue at this point it is perfectly valid. But if you
> decide to terminate the channel (yes it is terminate_all API), we abort and don't
> have context to report back!
>
> As Lars rightly pointed out, residue calculation are very tricky, DMA fifo may
> have data, some data may be in device FIFO, so residue is always from DMA point
> of view and may differ from device view (more or less depending upon direction)
>
> Now if you require to add more features for your usecase, please do feel free to
> send a patch. The framework can always be improved, we haven't solved world
> hunger yet!

Okay, I see that in theory, there are some tricky bits in implementing DMA
support in UART drivers. On the other hand there are already drivers 
with seems
to be working fine. This discussion is about revert of the feature 
present in
pl330 driver, which is required by other in-kernel driver without proper 
fix to
them.

Can we drop it for now and discuss what and how should be implemented to 
make
everyone happy, without any regressions?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Vinod]

On 18-05-18, 08:28, Marek Szyprowski wrote:
> Hi Vinod,
> 
> Okay, I see that in theory, there are some tricky bits in implementing DMA
> support in UART drivers. On the other hand there are already drivers 
> with seems
> to be working fine. This discussion is about revert of the feature 
> present in
> pl330 driver, which is required by other in-kernel driver without proper 
> fix to
> them.
> 
> Can we drop it for now and discuss what and how should be implemented to 
> make
> everyone happy, without any regressions?

Sure am dropping the revert, we can always bring it back if it is required

-- 
~Vinod

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Marek Szyprowski]

Hi Vinod,

On 2018-05-18 09:21, Vinod wrote:
> On 18-05-18, 08:28, Marek Szyprowski wrote:
>> Hi Vinod,
>>
>> Okay, I see that in theory, there are some tricky bits in implementing DMA
>> support in UART drivers. On the other hand there are already drivers
>> with seems
>> to be working fine. This discussion is about revert of the feature
>> present in
>> pl330 driver, which is required by other in-kernel driver without proper
>> fix to
>> them.
>>
>> Can we drop it for now and discuss what and how should be implemented to
>> make
>> everyone happy, without any regressions?
> Sure am dropping the revert, we can always bring it back if it is required

This revert is still in your -next branch. Do you plan to update it as well?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Vinod]

On 29-05-18, 07:17, Marek Szyprowski wrote:
> Hi Vinod,
> 
> On 2018-05-18 09:21, Vinod wrote:
> > On 18-05-18, 08:28, Marek Szyprowski wrote:
> >> Hi Vinod,
> >>
> >> Okay, I see that in theory, there are some tricky bits in implementing DMA
> >> support in UART drivers. On the other hand there are already drivers
> >> with seems
> >> to be working fine. This discussion is about revert of the feature
> >> present in
> >> pl330 driver, which is required by other in-kernel driver without proper
> >> fix to
> >> them.
> >>
> >> Can we drop it for now and discuss what and how should be implemented to
> >> make
> >> everyone happy, without any regressions?
> > Sure am dropping the revert, we can always bring it back if it is required
> 
> This revert is still in your -next branch. Do you plan to update it as well?

I dont send it to linus and merge topic/* into for-linus and send. It gets
updated on rebase on -rc1.

-- 
~Vinod

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Frank Mori Hess]

On Fri, May 18, 2018 at 12:03 AM, Vinod <vkoul@kernel.org> wrote:
>
> You are simply mixing things up!

It certainly feels like I'm mixed up.  If I have to resolve this, I'd
like to be a little less mixed up before I submit more patches which
are going to inevitably result in subtly broken code suddenly becoming
prominently and unignorably broken code.  Unfortunately I get the
impression I'm exhausting your patience to answer my questions, and
I've failed to fully communicate what the question is.


> On Pause we don't expect data loss, as user can
> resume the transfer. This means as you rightly guessed, the DMA HW should not drop
> any data, nor should SW.
>
> Now if you want to read residue at this point it is perfectly valid. But if you
> decide to terminate the channel (yes it is terminate_all API), we abort and don't
> have context to report back!

I understand the residue can't be read after terminate, that's why
reading the residue is step 2 in pause/residue/terminate.  My question
was whether the entire sequence pause/residue/terminate taken as a
whole can or cannot lose data.  Saying that individual steps can or
can't lose data is not enough, context is required.  The key point is
whether pause flushes in-flight data to its destination or not.  If it
does, and our residue is accurate, the terminate cannot cause data
loss.  If pause doesn't flush, an additional step of flush_sync as
Lars suggested is required.  So pause/flush_sync/residue/terminate
would be the safe sequence that cannot lose data.


> As Lars rightly pointed out, residue calculation are very tricky, DMA fifo may
> have data, some data may be in device FIFO, so residue is always from DMA point
> of view and may differ from device view (more or less depending upon direction)
>
> Now if you require to add more features for your usecase, please do feel free to
> send a patch. The framework can always be improved, we haven't solved world
> hunger yet!

World hunger?  I don't see how asking questions about a dma engine's
data integrity guarantees is either unreasonable or out of scope.

-- 
Frank

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Vinod Koul]

On 18-05-18, 14:56, Frank Mori Hess wrote:
> On Fri, May 18, 2018 at 12:03 AM, Vinod <vkoul@kernel.org> wrote:
> >
> > You are simply mixing things up!
> 
> It certainly feels like I'm mixed up.  If I have to resolve this, I'd
> like to be a little less mixed up before I submit more patches which
> are going to inevitably result in subtly broken code suddenly becoming
> prominently and unignorably broken code.  Unfortunately I get the
> impression I'm exhausting your patience to answer my questions, and
> I've failed to fully communicate what the question is.
> 
> 
> > On Pause we don't expect data loss, as user can
> > resume the transfer. This means as you rightly guessed, the DMA HW should not drop
> > any data, nor should SW.
> >
> > Now if you want to read residue at this point it is perfectly valid. But if you
> > decide to terminate the channel (yes it is terminate_all API), we abort and don't
> > have context to report back!
> 
> I understand the residue can't be read after terminate, that's why
> reading the residue is step 2 in pause/residue/terminate.  My question
> was whether the entire sequence pause/residue/terminate taken as a
> whole can or cannot lose data.  Saying that individual steps can or
> can't lose data is not enough, context is required.  The key point is
> whether pause flushes in-flight data to its destination or not.  If it
> does, and our residue is accurate, the terminate cannot cause data
> loss.  If pause doesn't flush, an additional step of flush_sync as
> Lars suggested is required.  So pause/flush_sync/residue/terminate
> would be the safe sequence that cannot lose data.

I wouldn't use cannot, shouldn't would be better here as it depends on HW and
where all data has been buffered and if it can be flushed or not.

Have you checked if pl330 supports such flushing?

> > As Lars rightly pointed out, residue calculation are very tricky, DMA fifo may
> > have data, some data may be in device FIFO, so residue is always from DMA point
> > of view and may differ from device view (more or less depending upon direction)
> >
> > Now if you require to add more features for your usecase, please do feel free to
> > send a patch. The framework can always be improved, we haven't solved world
> > hunger yet!
> 
> World hunger?  I don't see how asking questions about a dma engine's
> data integrity guarantees is either unreasonable or out of scope.

No they are not out of scope, and dmaengine APIs support requested usages.  Have
we solved all cases, hell no. Can we add more to support different scenarios,
absolutely!

-- 
~Vinod

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Frank Mori Hess]

On Mon, May 21, 2018 at 5:16 AM, Vinod Koul <vinod.koul@linaro.org> wrote:
>>
>> I understand the residue can't be read after terminate, that's why
>> reading the residue is step 2 in pause/residue/terminate.  My question
>> was whether the entire sequence pause/residue/terminate taken as a
>> whole can or cannot lose data.  Saying that individual steps can or
>> can't lose data is not enough, context is required.  The key point is
>> whether pause flushes in-flight data to its destination or not.  If it
>> does, and our residue is accurate, the terminate cannot cause data
>> loss.  If pause doesn't flush, an additional step of flush_sync as
>> Lars suggested is required.  So pause/flush_sync/residue/terminate
>> would be the safe sequence that cannot lose data.
>
> I wouldn't use cannot, shouldn't would be better here as it depends on HW and
> where all data has been buffered and if it can be flushed or not.
>
> Have you checked if pl330 supports such flushing?

It does not, at least in the context of pausing.  The dma-330 DMAEND
instruction flushes in-flight data to its destination, and there are
read/write barrier instructions also, but none of them can be injected
on the fly into a running dma thread.  DMAKILL can be, but it discards
in-flight data.  Currently, if an 8250 serial driver uses the pl330
for rx dma, the result is possible data loss/corruption.  If there was
a stronger pause capability, call it "cmd_sync_pause" which guaranteed
flushing of in-flight data to its destination and accurate residue
reading when paused, then the 8250 serial driver could check for
"cmd_sync_pause" and reject dma drivers that do not have that
capability.  pl330.c would not advertise cmd_sync_pause.  I don't know
if other dmaengine hardware would be able to support cmd_sync_pause or
not, I'm mostly just familiar with the pl330.  The ep93xx_dma.c driver
for example has a m2p_hw_synchronize function which seems to do a
flush.

-- 
Frank

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Vinod Koul]

On 21-05-18, 20:56, Frank Mori Hess wrote:
> On Mon, May 21, 2018 at 5:16 AM, Vinod Koul <vinod.koul@linaro.org> wrote:
> >>
> >> I understand the residue can't be read after terminate, that's why
> >> reading the residue is step 2 in pause/residue/terminate.  My question
> >> was whether the entire sequence pause/residue/terminate taken as a
> >> whole can or cannot lose data.  Saying that individual steps can or
> >> can't lose data is not enough, context is required.  The key point is
> >> whether pause flushes in-flight data to its destination or not.  If it
> >> does, and our residue is accurate, the terminate cannot cause data
> >> loss.  If pause doesn't flush, an additional step of flush_sync as
> >> Lars suggested is required.  So pause/flush_sync/residue/terminate
> >> would be the safe sequence that cannot lose data.
> >
> > I wouldn't use cannot, shouldn't would be better here as it depends on HW and
> > where all data has been buffered and if it can be flushed or not.
> >
> > Have you checked if pl330 supports such flushing?
> 
> It does not, at least in the context of pausing.  The dma-330 DMAEND
> instruction flushes in-flight data to its destination, and there are
> read/write barrier instructions also, but none of them can be injected
> on the fly into a running dma thread.  DMAKILL can be, but it discards
> in-flight data.  Currently, if an 8250 serial driver uses the pl330
> for rx dma, the result is possible data loss/corruption.  If there was
> a stronger pause capability, call it "cmd_sync_pause" which guaranteed
> flushing of in-flight data to its destination and accurate residue
> reading when paused, then the 8250 serial driver could check for
> "cmd_sync_pause" and reject dma drivers that do not have that
> capability.  pl330.c would not advertise cmd_sync_pause.  I don't know
> if other dmaengine hardware would be able to support cmd_sync_pause or
> not, I'm mostly just familiar with the pl330.  The ep93xx_dma.c driver
> for example has a m2p_hw_synchronize function which seems to do a
> flush.

Well looks like even adding support for sync_pause doesn't solve your issue on
pl330. Do you want to move this to PIO mode then..?

-- 
~Vinod

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Frank Mori Hess]

On Mon, May 21, 2018 at 11:37 PM, Vinod Koul <vinod.koul@linaro.org> wrote:
>
> Well looks like even adding support for sync_pause doesn't solve your issue on
> pl330. Do you want to move this to PIO mode then..?
>

I'm not sure what you think my issue is with the pl330, are you
confusing me with Marek?  My position is that pause is unsupported by
the pl330 at the hardware level.  Nothing the pl330.c driver or
dmaengine api or 8250 serial driver does will change that.  That is
why I sent a patch to remove pause support from pl330.c.  The rest is
just trying to fix some bugs in mainline Linux I hit when I was a
naive youngster who thought Linux support for the computer world's
canonical serial uart could be counted on to move data from point A to
point B.  In truth though, I've already concluded it's not worth it to
slog though the linux kernel development process just to clean up
other people's messes.  I've already fixed my issues out-of-tree.  I
don't mind answering questions if anyone else cares about the issue
though.  Other than that, I think I'll just be ghosting out now.

-- 
Frank

Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"

[Vinod]

On 22-05-18, 10:27, Frank Mori Hess wrote:
> On Mon, May 21, 2018 at 11:37 PM, Vinod Koul <vinod.koul@linaro.org> wrote:
> >
> > Well looks like even adding support for sync_pause doesn't solve your issue on
> > pl330. Do you want to move this to PIO mode then..?

The issue for which you requested the revert of pl330 pause

> I'm not sure what you think my issue is with the pl330, are you
> confusing me with Marek?  My position is that pause is unsupported by
> the pl330 at the hardware level.  Nothing the pl330.c driver or
> dmaengine api or 8250 serial driver does will change that.  That is
> why I sent a patch to remove pause support from pl330.c.  The rest is
> just trying to fix some bugs in mainline Linux I hit when I was a
> naive youngster who thought Linux support for the computer world's
> canonical serial uart could be counted on to move data from point A to
> point B.  In truth though, I've already concluded it's not worth it to
> slog though the linux kernel development process just to clean up
> other people's messes.  I've already fixed my issues out-of-tree.  I
> don't mind answering questions if anyone else cares about the issue
> though.  Other than that, I think I'll just be ghosting out now.

~Vinod