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@xxxxxxxxxxx> wrote: > >>>> On 2018-05-09 19:48, Frank Mori Hess wrote: > >>>>> On Wed, May 9, 2018 at 9:19 AM, Marek Szyprowski > >>>>> <m.szyprowski@xxxxxxxxxxx> wrote: > >>>>>> I understand that pl330 doesn't support real PAUSE, but as it has been > >>>>>> implemented since 2015 the limited support is perfectly possible - just > >>>>>> to let serial driver to read the amount of data transferred. > >>>>>> > >>>>>> Please note that DMA engine documentation clearly states that the best > >>>>>> residue granularity the driver might expect is BURST granularity. There > >>>>>> is no way to get the information about the data which still sits in the > >>>>>> DMA engine fifo when transfer is paused/terminated. This means that > >>>>>> the serial driver should set maxburst = 1 if it is interested in > >>>>>> getting exact number of bytes received/sent. With maxburst = 1 there > >>>>>> is no such thing as data loose in the DMA engine fifo. > >>>>> That is not my understanding of the dmaengine pause requirements, but > >>>>> of course Vinod can speak authoritatively on this. > >>>> Basing on the comments in dma_residue_granularity enum documentation (see > >>>> include/linux/dmaengine.h) there is no better granularity of residue > >>>> reporting than BURST units. If driver needs byte accuracy, then it should > >>>> use MAXBURST=1 and DMA_SLAVE_BUSWIDTH_1_BYTE configuration. > >>> That's an interesting line of thought. The 8250 serial driver clearly > >>> assumes it can do the sequence > >>> > >>> dma pause > >>> read residue > >>> dma terminate > >>> > >>> without data loss. > >> Right. From DMA engine API perspective this is the only valid way to > >> read the > >> residue when you terminate the transfer. > > Not really, API allows you to read any point of time, you may support it or not > > is different matter. Granularity of reporting is also a different point. > > I mean to read the residue in stable conditions (the incoming bytes has > been either > read by dma or still sits in the uart fifo). Too bad that it is not > possible to read > residue after terminating the transfer. This would remove the need for > this fake > pause support. you can read the residue before you call terminate. Terminate is basically a cleanup job, so reading after that makes no sense, maybe you need to modify calling sequence.. > > >>> It checks for the capabilities > >>> > >>> cmd_pause > >>> cmd_terminate > >>> residue_granularity != DMA_RESIDUE_GRANULARITY_DESCRIPTOR > >> Checking for cmd_pause is a bit too strict, because cmd_pause means 'driver > >> supports both pause and resume', but the serial driver doesn't use resume at > >> all. A check for residue_granularity != DMA_RESIDUE_GRANULARITY_DESCRIPTOR > >> is on the other hand a bit too loose. > > thats true and it was added for audio which does pause/resume. If you feel it > > helps in serial to get pause & resume capabilities independently we can split > > them up, feel free to send a patch > > Okay, I will prepare a patch for this. > > > For Pause/resume data loss is _not_ expected. > > > >>> and some of the 8250 drivers like 8250_dw.c set a maxburst > 1. If it > >>> can't count on the pause/residue/terminate working without data loss > >>> then it is just broken. As is the 8250_omap.c driver. Is the > >>> description of dmaengine_pause in the documentation of "This pauses > >>> activity on the DMA channel without data loss" to be interpreted as > >>> "as long as you resume afterwards"? > >> I assume that this requirement is for both - resuming and terminating. > > Terminate is abort, data loss may happen here. > > Okay. Then it is up to the drivers to rely on this or not. Yes... > > >>>>> I also don't agree > >>>>> that data loss cannot happen for single transfers. It only becomes > >>>>> less likely since there are fewer bytes in the dma controller fifo and > >>>>> they stay there for a shorter period of time. But even so, there is > >>>>> nothing stopping the DMAKILL from killing the transfer thread after it > >>>>> does a single byte load but before it does the associated single byte > >>>>> store, as they are performed by separate instructions. As far as your > >>>>> serial port goes, the byte has been read by the host, even though it > >>>>> never made it to memory, so it is gone forever. The dma-330 does have > >>>>> a load lock which prevents some operations from interrupting a > >>>>> load/store combination, but in my observations DMAKILL does not > >>>>> respect the load lock. > >>>> For the last 3 years no one observed any issue with the current design > >>>> (single transfers with DMA_SLAVE_BUSWIDTH_1_BYTE). By removing this > >>>> feature we will loose ability to use DMA in the serial drivers, what is > >>>> mainly useful for low-power bluetooth operation (serial console is really > >>>> negligible case). > >>> It's not surprising it hasn't been reported. It is a race that > >>> requires a DMAKILL to be issued just as a byte is in-flight through > >>> the dma controller. The only reason a driver would pause the > >>> un-resumeable pl330 would be because the driver either knows or > >>> suspects no more data will be arriving and it gives up on the > >>> transfer. The only reason I noticed is we had a test which sent data > >>> to a serial port, waited just long enough for the serial port rx to > >>> timeout, then sent more data just as the pause was issuing DMAKILL. > >>> And then the test did this a few hundred thousand times until it > >>> noticed bad data. Also, given the way 8250 rx timeouts work, a person > >>> typing into a serial console wouldn't type fast enough to trigger the > >>> bug unless the baud rate was set extremely low. And if someone lost a > >>> keystroke every 10^5 bytes, who would blame the dma controller? > >> Like I already said, console is not a proper use case for serial dma. > >> The more appropriate is bluetooth and still, I'm not aware of the issue > >> with the current code. > > Why do you say serial is not important? > > I mean that using DMA engine for uart/serial device for implementing only a > console support is a bit pointless, because typically console doesn't > transfer > much data and overhead of using DMA mode (setting up dma engine) is bigger > than doing all the transfers in PIO mode. Well that depends on your system and outweighing dma complexity vs perf gains you get.. > >>> I do admit I didn't do this test using single transfers, because our > >>> goal was getting bursts to work. Unfortunately, the test program > >>> relies on some custom hardware I no longer have access to so I can't > >>> easily re-run the test now. However, since the DMA-330 manual > >>> documents the DMAKILL instruction to: > >>> > >>> "Remove all entries in the MFIFO for the DMA channel." > >>> "Remove all entries in the read buffer queue and write buffer queue > >>> for the DMA channel." > >>> > >>> Relying on it to work as a safe pause for single transfers seems like > >>> wishful thinking. I sure didn't want to believe the DMA-330 couldn't > >>> be safely paused, but I eventually had to resign myself to it. > >> Okay, so you don't have any evidence that DMA transfers done in single > >> reads/writes is broken with the current cmd_pause implementation. > >> > >> This revert in fact at best disables DMA mode in the serial drivers (or > >> in worse case, i.e. Exynos, causes current drivers to do trashed > >> transfers due to lack of checking for the needed dma engine > >> capabilities). > >> > >> Maybe instead of reverting support for it, there should be a check for > >> BURST vs. SINGLE mode and we would keep the current implementation for > >> SINGLE transfers? > >> > >> Vinod, could you comment a bit this discussion from the DMA engine > >> maintainer perspective? > > So I will try to summarise here: > > > > - Pause/resume does not expect data loss > > - Status can be queried any point of time > > - Granularity reporting is upto device > > - To support a use case and device limitations it is okay to support only > > singles. > > What about the revert this thread is about? I would really like to have some > evidence of broken data in single transfer modes before removing > functionality > that was present for years. I do not mind dropping this, do both of you agree to that and agree to fix other issues? > The discussed use case (pause+terminate) is crucial for serial devices at > least on Samsung SoCs and this revert break them. > > Quick grep shows that PL330 is being used by a few other SoC families too, > but I didn't check if any of them use it for serial device. > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland -- ~Vinod -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
