All of lore.kernel.org
 help / color / mirror / Atom feed
From: slash.tmp@free.fr (Mason)
To: linux-arm-kernel@lists.infradead.org
Subject: Tearing down DMA transfer setup after DMA client has finished
Date: Wed, 23 Nov 2016 13:41:29 +0100	[thread overview]
Message-ID: <58358E79.5050404@free.fr> (raw)
In-Reply-To: <yw1xpolmbdih.fsf@unicorn.mansr.com>

On 23/11/2016 13:13, M?ns Rullg?rd wrote:

> Mason wrote:
> 
>> On my platform, setting up a DMA transfer is a two-step process:
>>
>> 1) configure the "switch box" to connect a device to a memory channel
>> 2) configure the transfer details (address, size, command)
>>
>> When the transfer is done, the sbox setup can be torn down,
>> and the DMA driver can start another transfer.
>>
>> The current software architecture for my NFC (NAND Flash controller)
>> driver is as follows (for one DMA transfer).
>>
>>   sg_init_one
>>   dma_map_sg
>>   dmaengine_prep_slave_sg
>>   dmaengine_submit
>>   dma_async_issue_pending
>>   configure_NFC_transfer
>>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>>   wait_for_NFC_idle
>>   dma_unmap_sg
>>
>> The problem is that the DMA driver tears down the sbox setup
>> as soon as it receives the IRQ. However, when writing to the
>> device, the interrupt only means "I have pushed all data from
>> memory to the memory channel". These data have not reached
>> the device yet, and may still be "in flight". Thus the sbox
>> setup can only be torn down after the NFC is idle.
>>
>> How do I call back into the DMA driver after wait_for_NFC_idle,
>> to request sbox tear down?
>>
>> The new architecture would become:
>>
>>   sg_init_one
>>   dma_map_sg
>>   dmaengine_prep_slave_sg
>>   dmaengine_submit
>>   dma_async_issue_pending
>>   configure_NFC_transfer
>>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>>   wait_for_NFC_idle
>>   request_sbox_tear_down /*** HOW TO DO THAT ***/
>>   dma_unmap_sg
>>
>> As far as I can tell, my NFC driver should call dmaengine_synchronize ??
>> (In other words request_sbox_tear_down == dmaengine_synchronize)
>>
>> So the DMA driver should implement the device_synchronize hook,
>> and tear the sbox down in that function.
>>
>> Is that correct? Or am I on the wrong track?
> 
> dmaengine_synchronize() is not meant for this.  See the documentation:
> 
> /**
>  * dmaengine_synchronize() - Synchronize DMA channel termination
>  * @chan: The channel to synchronize
>  *
>  * Synchronizes to the DMA channel termination to the current context. When this
>  * function returns it is guaranteed that all transfers for previously issued
>  * descriptors have stopped and and it is safe to free the memory assoicated
>  * with them. Furthermore it is guaranteed that all complete callback functions
>  * for a previously submitted descriptor have finished running and it is safe to
>  * free resources accessed from within the complete callbacks.
>  *
>  * The behavior of this function is undefined if dma_async_issue_pending() has
>  * been called between dmaengine_terminate_async() and this function.
>  *
>  * This function must only be called from non-atomic context and must not be
>  * called from within a complete callback of a descriptor submitted on the same
>  * channel.
>  */
> 
> This is for use after a dmaengine_terminate_async() call to wait for the
> dma engine to finish whatever it was doing.  This is not the problem
> here.  Your problem is that the dma engine interrupt fires before the
> transfer is actually complete.  Although you get an indication from the
> target device when it has received all the data, there is no way to make
> the dma driver wait for this.

Hello Mans,

I'm confused. Are you saying there is no solution to my problem
within the existing DMA framework?

In its current form, the tangox-dma.c driver will fail randomly
for writes to a device (SATA, NFC).

Maybe an extra hook can be added to the DMA framework?

I'd like to hear from the framework's maintainers. Perhaps they
can provide some guidance.

Regards.

WARNING: multiple messages have this Message-ID (diff)
From: Mason <slash.tmp@free.fr>
To: Mans Rullgard <mans@mansr.com>, dmaengine@vger.kernel.org
Cc: Vinod Koul <vinod.koul@intel.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Dan Williams <dan.j.williams@intel.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Jon Mason <jdmason@kudzu.us>, Mark Brown <broonie@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Lee Jones <lee.jones@linaro.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Maxime Ripard <maxime.ripard@free-electrons.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Peter Ujfalusi <peter.ujfalusi@ti.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Sebastian Frias <sf84@laposte.net>,
	Thibaud Cornic <thibaud_cornic@sigmadesigns.com>
Subject: Re: Tearing down DMA transfer setup after DMA client has finished
Date: Wed, 23 Nov 2016 13:41:29 +0100	[thread overview]
Message-ID: <58358E79.5050404@free.fr> (raw)
In-Reply-To: <yw1xpolmbdih.fsf@unicorn.mansr.com>

On 23/11/2016 13:13, Måns Rullgård wrote:

> Mason wrote:
> 
>> On my platform, setting up a DMA transfer is a two-step process:
>>
>> 1) configure the "switch box" to connect a device to a memory channel
>> 2) configure the transfer details (address, size, command)
>>
>> When the transfer is done, the sbox setup can be torn down,
>> and the DMA driver can start another transfer.
>>
>> The current software architecture for my NFC (NAND Flash controller)
>> driver is as follows (for one DMA transfer).
>>
>>   sg_init_one
>>   dma_map_sg
>>   dmaengine_prep_slave_sg
>>   dmaengine_submit
>>   dma_async_issue_pending
>>   configure_NFC_transfer
>>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>>   wait_for_NFC_idle
>>   dma_unmap_sg
>>
>> The problem is that the DMA driver tears down the sbox setup
>> as soon as it receives the IRQ. However, when writing to the
>> device, the interrupt only means "I have pushed all data from
>> memory to the memory channel". These data have not reached
>> the device yet, and may still be "in flight". Thus the sbox
>> setup can only be torn down after the NFC is idle.
>>
>> How do I call back into the DMA driver after wait_for_NFC_idle,
>> to request sbox tear down?
>>
>> The new architecture would become:
>>
>>   sg_init_one
>>   dma_map_sg
>>   dmaengine_prep_slave_sg
>>   dmaengine_submit
>>   dma_async_issue_pending
>>   configure_NFC_transfer
>>   wait_for_IRQ_from_DMA_engine // via DMA_PREP_INTERRUPT
>>   wait_for_NFC_idle
>>   request_sbox_tear_down /*** HOW TO DO THAT ***/
>>   dma_unmap_sg
>>
>> As far as I can tell, my NFC driver should call dmaengine_synchronize ??
>> (In other words request_sbox_tear_down == dmaengine_synchronize)
>>
>> So the DMA driver should implement the device_synchronize hook,
>> and tear the sbox down in that function.
>>
>> Is that correct? Or am I on the wrong track?
> 
> dmaengine_synchronize() is not meant for this.  See the documentation:
> 
> /**
>  * dmaengine_synchronize() - Synchronize DMA channel termination
>  * @chan: The channel to synchronize
>  *
>  * Synchronizes to the DMA channel termination to the current context. When this
>  * function returns it is guaranteed that all transfers for previously issued
>  * descriptors have stopped and and it is safe to free the memory assoicated
>  * with them. Furthermore it is guaranteed that all complete callback functions
>  * for a previously submitted descriptor have finished running and it is safe to
>  * free resources accessed from within the complete callbacks.
>  *
>  * The behavior of this function is undefined if dma_async_issue_pending() has
>  * been called between dmaengine_terminate_async() and this function.
>  *
>  * This function must only be called from non-atomic context and must not be
>  * called from within a complete callback of a descriptor submitted on the same
>  * channel.
>  */
> 
> This is for use after a dmaengine_terminate_async() call to wait for the
> dma engine to finish whatever it was doing.  This is not the problem
> here.  Your problem is that the dma engine interrupt fires before the
> transfer is actually complete.  Although you get an indication from the
> target device when it has received all the data, there is no way to make
> the dma driver wait for this.

Hello Mans,

I'm confused. Are you saying there is no solution to my problem
within the existing DMA framework?

In its current form, the tangox-dma.c driver will fail randomly
for writes to a device (SATA, NFC).

Maybe an extra hook can be added to the DMA framework?

I'd like to hear from the framework's maintainers. Perhaps they
can provide some guidance.

Regards.

  reply	other threads:[~2016-11-23 12:41 UTC|newest]

Thread overview: 164+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-11-23 10:25 Tearing down DMA transfer setup after DMA client has finished Mason
2016-11-23 10:25 ` Mason
2016-11-23 12:13 ` Måns Rullgård
2016-11-23 12:13   ` Måns Rullgård
2016-11-23 12:41   ` Mason [this message]
2016-11-23 12:41     ` Mason
2016-11-23 17:21     ` Måns Rullgård
2016-11-23 17:21       ` Måns Rullgård
2016-11-24 10:53       ` Mason
2016-11-24 10:53         ` Mason
2016-11-24 14:17         ` Måns Rullgård
2016-11-24 14:17           ` Måns Rullgård
2016-11-24 15:20           ` Mason
2016-11-24 15:20             ` Mason
2016-11-24 16:37             ` Måns Rullgård
2016-11-24 16:37               ` Måns Rullgård
2016-11-25  4:55 ` Vinod Koul
2016-11-25  4:55   ` Vinod Koul
2016-11-25 11:57   ` Måns Rullgård
2016-11-25 11:57     ` Måns Rullgård
2016-11-25 14:05     ` Mason
2016-11-25 14:05       ` Mason
2016-11-25 14:12       ` Måns Rullgård
2016-11-25 14:12         ` Måns Rullgård
2016-11-25 14:28         ` Mason
2016-11-25 14:28           ` Mason
2016-11-25 14:42           ` Måns Rullgård
2016-11-25 14:42             ` Måns Rullgård
2016-11-25 12:45   ` Russell King - ARM Linux
2016-11-25 12:45     ` Russell King - ARM Linux
2016-11-25 13:07     ` Måns Rullgård
2016-11-25 13:07       ` Måns Rullgård
2016-11-25 13:34       ` Russell King - ARM Linux
2016-11-25 13:34         ` Russell King - ARM Linux
2016-11-25 13:50         ` Måns Rullgård
2016-11-25 13:50           ` Måns Rullgård
2016-11-25 13:58           ` Russell King - ARM Linux
2016-11-25 13:58             ` Russell King - ARM Linux
2016-11-25 14:03             ` Måns Rullgård
2016-11-25 14:03               ` Måns Rullgård
2016-11-25 14:17               ` Russell King - ARM Linux
2016-11-25 14:17                 ` Russell King - ARM Linux
2016-11-25 14:40                 ` Måns Rullgård
2016-11-25 14:40                   ` Måns Rullgård
2016-11-25 14:56                   ` Russell King - ARM Linux
2016-11-25 14:56                     ` Russell King - ARM Linux
2016-11-25 15:08                     ` Måns Rullgård
2016-11-25 15:08                       ` Måns Rullgård
2016-11-25 15:02                 ` Mason
2016-11-25 15:02                   ` Mason
2016-11-25 15:12                   ` Måns Rullgård
2016-11-25 15:12                     ` Måns Rullgård
2016-11-25 15:21                     ` Mason
2016-11-25 15:21                       ` Mason
2016-11-25 15:28                       ` Måns Rullgård
2016-11-25 15:28                         ` Måns Rullgård
2016-11-25 12:46   ` Mason
2016-11-25 12:46     ` Mason
2016-11-25 13:11     ` Måns Rullgård
2016-11-25 13:11       ` Måns Rullgård
2016-11-25 14:21       ` Mason
2016-11-25 14:21         ` Mason
2016-11-25 14:37         ` Måns Rullgård
2016-11-25 14:37           ` Måns Rullgård
2016-11-25 15:35           ` Mason
2016-11-25 15:35             ` Mason
2016-11-29 18:25     ` Mason
2016-11-29 18:25       ` Mason
2016-12-06  5:12       ` Vinod Koul
2016-12-06  5:12         ` Vinod Koul
2016-12-06 12:42         ` Mason
2016-12-06 12:42           ` Mason
2016-12-06 13:14           ` Måns Rullgård
2016-12-06 13:14             ` Måns Rullgård
2016-12-06 15:24             ` Mason
2016-12-06 15:24               ` Mason
2016-12-06 15:34               ` Måns Rullgård
2016-12-06 15:34                 ` Måns Rullgård
2016-12-06 22:55                 ` Mason
2016-12-06 22:55                   ` Mason
2016-12-07 16:43             ` Vinod Koul
2016-12-07 16:43               ` Vinod Koul
2016-12-07 16:45               ` Måns Rullgård
2016-12-07 16:45                 ` Måns Rullgård
2016-12-08 10:39                 ` Vinod Koul
2016-12-08 10:39                   ` Vinod Koul
2016-12-08 10:54                   ` Mason
2016-12-08 10:54                     ` Mason
2016-12-08 11:18                     ` Geert Uytterhoeven
2016-12-08 11:18                       ` Geert Uytterhoeven
2016-12-08 11:47                       ` Måns Rullgård
2016-12-08 11:47                         ` Måns Rullgård
2016-12-08 12:03                         ` Geert Uytterhoeven
2016-12-08 12:03                           ` Geert Uytterhoeven
2016-12-08 12:17                           ` Mason
2016-12-08 12:17                             ` Mason
2016-12-08 12:21                           ` Måns Rullgård
2016-12-08 12:21                             ` Måns Rullgård
2016-12-08 16:37                     ` Vinod Koul
2016-12-08 16:37                       ` Vinod Koul
2016-12-08 16:48                       ` Måns Rullgård
2016-12-08 16:48                         ` Måns Rullgård
2016-12-09  6:59                         ` Vinod Koul
2016-12-09  6:59                           ` Vinod Koul
2016-12-09 10:25                           ` Sebastian Frias
2016-12-09 10:25                             ` Sebastian Frias
2016-12-09 11:34                             ` Måns Rullgård
2016-12-09 11:34                               ` Måns Rullgård
2016-12-09 11:35                             ` 1Måns Rullgård
2016-12-09 11:35                               ` 1Måns Rullgård
2016-12-09 17:17                             ` Vinod Koul
2016-12-09 17:17                               ` Vinod Koul
2016-12-09 17:28                               ` Måns Rullgård
2016-12-09 17:28                                 ` Måns Rullgård
2016-12-09 17:53                                 ` Vinod Koul
2016-12-09 17:53                                   ` Vinod Koul
2016-12-09 17:34                               ` Mason
2016-12-09 17:34                                 ` Mason
2016-12-09 17:56                                 ` Vinod Koul
2016-12-09 17:56                                   ` Vinod Koul
2016-12-09 18:17                                   ` Vinod Koul
2016-12-09 18:17                                     ` Vinod Koul
2016-12-09 18:23                                   ` Mason
2016-12-09 18:23                                     ` Mason
2016-12-12  5:01                                     ` Vinod Koul
2016-12-12  5:01                                       ` Vinod Koul
2016-12-15 11:17                                     ` Mark Brown
2016-12-15 11:17                                       ` Mark Brown
2016-12-08 11:44                   ` Måns Rullgård
2016-12-08 11:44                     ` Måns Rullgård
2016-12-08 11:59                     ` Geert Uytterhoeven
2016-12-08 11:59                       ` Geert Uytterhoeven
2016-12-08 12:20                       ` Måns Rullgård
2016-12-08 12:20                         ` Måns Rullgård
2016-12-08 12:31                         ` Geert Uytterhoeven
2016-12-08 12:31                           ` Geert Uytterhoeven
2016-12-08 12:41                         ` Mason
2016-12-08 12:41                           ` Mason
2016-12-08 12:44                           ` Måns Rullgård
2016-12-08 12:44                             ` Måns Rullgård
2016-12-08 13:29                             ` Mason
2016-12-08 13:29                               ` Mason
2016-12-08 13:39                               ` Måns Rullgård
2016-12-08 13:39                                 ` Måns Rullgård
2016-12-08 15:50                         ` Vinod Koul
2016-12-08 15:50                           ` Vinod Koul
2016-12-08 16:36                           ` Måns Rullgård
2016-12-08 16:36                             ` Måns Rullgård
2016-12-08 15:40                     ` Vinod Koul
2016-12-08 15:40                       ` Vinod Koul
2016-12-08 15:43                       ` Mason
2016-12-08 15:43                         ` Mason
2016-12-08 16:21                         ` Vinod Koul
2016-12-08 16:21                           ` Vinod Koul
2016-12-08 16:46                       ` Måns Rullgård
2016-12-08 16:46                         ` Måns Rullgård
2016-12-07 16:41           ` Vinod Koul
2016-12-07 16:41             ` Vinod Koul
2016-12-07 16:44             ` Måns Rullgård
2016-12-07 16:44               ` Måns Rullgård
2016-12-08 10:37               ` Vinod Koul
2016-12-08 10:37                 ` Vinod Koul
2016-12-08 11:44                 ` Måns Rullgård
2016-12-08 11:44                   ` Måns Rullgård

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=58358E79.5050404@free.fr \
    --to=slash.tmp@free.fr \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.