Revert "dmaengine: pl330: add DMA_PAUSE feature"

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

Quoted 2 lines · Frank Mori Hess

This reverts commit 88987d2c7534a0269f567fb101e6d71a08f0f01d.

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

Quoted 6 lines · Vinod Koul

Quoted 2 lines · Frank Mori Hess

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?

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

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 <[email protected]>

---
  drivers/dma/pl330.c | 28 ----------------------------
  1 file changed, 28 deletions(-)

drivers/dma/pl330.c +0 -27

index 6237069001c4..f802bd3b0481 100644
@@ -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);

Hi Frank and Vinod,

Quoted 11 lines · 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).

Quoted 1 line

Signed-off-by Frank Mori Hess <[email protected]>

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.

Quoted 14 lines · Marek Szyprowski

Hi Frank and Vinod,

Quoted 11 lines · 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).

Quoted 5 lines

Quoted 1 line

Signed-off-by Frank Mori Hess <[email protected]>

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.

Quoted 14 lines · Marek Szyprowski

Hi Frank and Vinod,

Quoted 11 lines · 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).

Quoted 5 lines

Quoted 1 line

Signed-off-by Frank Mori Hess <[email protected]>

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.

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.

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

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.

Quoted 11 lines · Marek Szyprowski

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.

Quoted 3 lines

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.

Quoted 9 lines

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.

Hi Frank,

Quoted 13 lines · Frank Mori Hess

Quoted 11 lines · Marek Szyprowski

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.

Quoted 2 lines

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.

Quoted 4 lines

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

Quoted 8 lines

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

Quoted 14 lines · Marek Szyprowski

Quoted 13 lines · Frank Mori Hess

Quoted 11 lines · Marek Szyprowski

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.

Quoted 6 lines

Quoted 2 lines

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

Quoted 12 lines

Quoted 4 lines

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

Quoted 8 lines

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.

Quoted 5 lines

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.

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

Hi Frank,

Quoted 29 lines · Frank Mori Hess

Quoted 14 lines · Marek Szyprowski

Quoted 13 lines · Frank Mori Hess

Quoted 11 lines · Marek Szyprowski

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.

Quoted 6 lines

Quoted 2 lines

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

Quoted 1 line

without data loss.

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

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 17 lines

Quoted 12 lines

Quoted 4 lines

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

Quoted 8 lines

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.

Quoted 5 lines

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

Quoted 13 lines

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.

Quoted 6 lines

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:

Quoted 7 lines

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

Quoted 30 lines · Marek Szyprowski

Quoted 29 lines · Frank Mori Hess

Quoted 14 lines · Marek Szyprowski

Quoted 13 lines · Frank Mori Hess

Quoted 11 lines · Marek Szyprowski

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.

Quoted 6 lines

Quoted 2 lines

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

Quoted 4 lines

Quoted 1 line

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.

Quoted 5 lines

Quoted 5 lines

It checks for the capabilities

cmd_pause
cmd_terminate
residue_granularity != DMA_RESIDUE_GRANULARITY_DESCRIPTOR

Quoted 4 lines

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.

Quoted 7 lines

Quoted 6 lines

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.

Quoted 30 lines

Quoted 30 lines

Quoted 12 lines

Quoted 4 lines

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

Quoted 8 lines

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.

Quoted 5 lines

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?

Quoted 3 lines

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?

Quoted 21 lines

Quoted 6 lines

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:

Quoted 7 lines

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

Quoted 6 lines

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.

Hi Vinod,

Quoted 31 lines · Vinod

Quoted 30 lines · Marek Szyprowski

Quoted 29 lines · Frank Mori Hess

Quoted 14 lines · Marek Szyprowski

Quoted 13 lines · Frank Mori Hess

Quoted 11 lines · Marek Szyprowski

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.

Quoted 6 lines

Quoted 2 lines

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

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

It checks for the capabilities

cmd_pause
cmd_terminate
residue_granularity != DMA_RESIDUE_GRANULARITY_DESCRIPTOR

Quoted 7 lines

Quoted 4 lines

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.

Quoted 8 lines

For Pause/resume data loss is _not_ expected.

Quoted 6 lines

Quoted 6 lines

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

Quoted 2 lines

Quoted 1 line

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.

Quoted 34 lines

Quoted 34 lines

Quoted 18 lines

Quoted 13 lines

Quoted 13 lines

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.

Quoted 5 lines

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

Quoted 13 lines

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.

Quoted 1 line

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.

Quoted 27 lines

Quoted 21 lines

Quoted 6 lines

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:

Quoted 7 lines

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

Quoted 6 lines

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?

Quoted 7 lines

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

Signature · 2 lines

Marek Szyprowski, PhD
Samsung R&D Institute Poland

For Pause/resume data loss is _not_ expected.

Quoted 7 lines

Quoted 6 lines

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

Quoted 1 line

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

Terminate is abort, data loss may happen here.

Quoted 10 lines · Vinod

For Pause/resume data loss is _not_ expected.

Quoted 7 lines

Quoted 6 lines

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

Quoted 3 lines

Quoted 1 line

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.

Quoted 5 lines

Quoted 1 line

Terminate is abort, data loss may happen here.

Wait, are you saying if you do

dma pause

no data loss

Quoted 1 line

read residue

here as well

Quoted 1 line

dma terminate

Oh yes, we aborted...

Quoted 1 line

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

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.

Quoted 6 lines · Vinod Koul

Quoted 5 lines

Quoted 1 line

Terminate is abort, data loss may happen here.

Wait, are you saying if you do

dma pause

Quoted 8 lines

no data loss

Quoted 1 line

read residue

here as well

Quoted 1 line

dma terminate

Oh yes, we aborted...

Quoted 1 line

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.

Quoted 7 lines · 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.

Quoted 10 lines

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.

Quoted 9 lines

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!

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?

Quoted 11 lines · Marek Szyprowski

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.

Quoted 3 lines

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

Hi Vinod,

Quoted 12 lines · Vinod

Quoted 11 lines · Marek Szyprowski

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.

Quoted 4 lines

Quoted 3 lines

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?

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!

Quoted 2 lines · Vinod

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.

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 11 lines

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?

Quoted 12 lines · Vinod Koul

Quoted 11 lines

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.

Quoted 4 lines

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

Quoted 3 lines · Vinod Koul

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.