| Message ID | 2484918.HKVQc3yJkt@bear (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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.
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?
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
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
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
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. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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..
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
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.
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
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. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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
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
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. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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!
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
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
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.
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. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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!
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.
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..?
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.
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 -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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.
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;
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(-)