From: Vinod Koul <vinod.koul@intel.com> To: Sinan Kaya <okaya@codeaurora.org> Cc: Russell King - ARM Linux <linux@armlinux.org.uk>, linux-arm-msm@vger.kernel.org, timur@codeaurora.org, linux-kernel@vger.kernel.org, Christopher Covington <cov@codeaurora.org>, dmaengine@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback Date: Mon, 8 Aug 2016 14:32:03 +0530 [thread overview] Message-ID: <20160808090203.GY9681@localhost> (raw) In-Reply-To: <cff588a1-d85f-3d2c-ef61-1674d0d3ca92@codeaurora.org> On Thu, Aug 04, 2016 at 11:27:46AM -0400, Sinan Kaya wrote: > On 8/4/2016 10:40 AM, Russell King - ARM Linux wrote: > > On Thu, Aug 04, 2016 at 10:17:24AM -0400, Sinan Kaya wrote: > >> On 8/4/2016 8:55 AM, Vinod Koul wrote: > >>> Dmaengine tells transaction is complete. It does not say if the txn is > >>> success or failure. It can transfer data and not say if data was > >>> correct. A successful transaction implies data integrity as well, which > >>> dmaengine can't provide. > >> > >> Thanks for describing this. I was confused about DMA_SUCCESS and DMA_COMPLETE. > >> I now understand that tx_success API just returns information that the request > >> was executed whether the result is error or not. This makes sense now. > >> > >> However, if the txn is failure; then we should never call the client callback > >> since DMA engine cannot provide such feedback to the client without Dave's patch. > >> You are saying that the calling the callback is optional. > >> > >> Then, the callback cannot be optional in the error case for old behavior. > >> > >> How does the client know if memcpy executed or not? The client got its callback > >> and tx_status is also DMA_COMPLETE. > > > > If an error occurred, then dma_async_is_tx_complete() is supposed to > > return DMA_ERROR. It's up to the DMA engine driver to ensure that > > this happens if it has error detection abilities. > > > > Well, that's not what I'm getting from Vinod and also from the current usage > in most of the drivers that I looked. Sorry but, you are not interpreting it correctly. Me and Russell are saying the same thing! > > The current drivers implement tx_status as follows. > > static enum dma_status xyz_tx_status(struct dma_chan *chan, > dma_cookie_t cookie, struct dma_tx_state *state) > { > ... > ret = dma_cookie_status(&c->vc.chan, cookie, state); > if (ret == DMA_COMPLETE) > return ret; > ... > } > > What Vinod is telling me that I need to set the cookie to complete > whether the transaction is successful or not if the request was accepted > by HW. xyz_tx_status is just an indication that the transaction was accepted > by HW. An error can happen as a result of transaction execution. Nope, if the txn is completed you mark it complete. If you can detect error (can you??) then you can report DMA_ERROR. In that latter case do not use dma_async_is_complete() to check. You would need to store and report that cookie 'x' failed which you report status in .tx_statis() > > If I call dma_cookie_complete for all transactions regardless of transaction > success or not, then the xyz_tx_status returns DMA_COMPLETE. Again that is based on your implementation. > This also matches what Vinod is saying. The transaction is complete but > it may not be success. I'm saying that if we follow this scheme, then > we should not call the callback. That is not in driver's control. If the callback is set, you have to call it. Client may choose to not set it. > I also made the argument that the driver should not call dma_cookie_complete > for failure cases and somehow return an error for transactions that failed. > This is your suggestion. > > I don't think it matches Vinod's expectation. It does! > > Most of the helpers in drivers/dma/dmaengine.h are there to _assist_ > > the driver writer - they can't do magic. dma_cookie_status() will > > return from the point of view of the generic DMA code what the status > > of a particular cookie is, and the cookie state. It doesn't take > > care of whether a particular transaction associated with a cookie > > failed or not - that's up to the driver. > > > > So, if dma_cookie_status() says that a cookie has DMA_COMPLETED > > status, and the DMA engine is able to detect errors on individual > > transfers, then the driver needs to do further status lookup to > > determine whether the particular transaction referred to by the > > cookie did fail, and modify the returned status appropriately. > > > > If dma_cookie_status() says that the cookie is DMA_IN_PROGRESS, > > then the driver is expected to calculate and report the residue > > (the remaining number of bytes) of the referred to transaction. > > > > This part is fine. I'm worried about transactions that are failing. And you issue is complete orthogonal to this debate. I am not saying we should not discuss this, but you fix would be entirely different here (going by data you have provided till now) -- ~Vinod
WARNING: multiple messages have this Message-ID (diff)
From: vinod.koul@intel.com (Vinod Koul) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback Date: Mon, 8 Aug 2016 14:32:03 +0530 [thread overview] Message-ID: <20160808090203.GY9681@localhost> (raw) In-Reply-To: <cff588a1-d85f-3d2c-ef61-1674d0d3ca92@codeaurora.org> On Thu, Aug 04, 2016 at 11:27:46AM -0400, Sinan Kaya wrote: > On 8/4/2016 10:40 AM, Russell King - ARM Linux wrote: > > On Thu, Aug 04, 2016 at 10:17:24AM -0400, Sinan Kaya wrote: > >> On 8/4/2016 8:55 AM, Vinod Koul wrote: > >>> Dmaengine tells transaction is complete. It does not say if the txn is > >>> success or failure. It can transfer data and not say if data was > >>> correct. A successful transaction implies data integrity as well, which > >>> dmaengine can't provide. > >> > >> Thanks for describing this. I was confused about DMA_SUCCESS and DMA_COMPLETE. > >> I now understand that tx_success API just returns information that the request > >> was executed whether the result is error or not. This makes sense now. > >> > >> However, if the txn is failure; then we should never call the client callback > >> since DMA engine cannot provide such feedback to the client without Dave's patch. > >> You are saying that the calling the callback is optional. > >> > >> Then, the callback cannot be optional in the error case for old behavior. > >> > >> How does the client know if memcpy executed or not? The client got its callback > >> and tx_status is also DMA_COMPLETE. > > > > If an error occurred, then dma_async_is_tx_complete() is supposed to > > return DMA_ERROR. It's up to the DMA engine driver to ensure that > > this happens if it has error detection abilities. > > > > Well, that's not what I'm getting from Vinod and also from the current usage > in most of the drivers that I looked. Sorry but, you are not interpreting it correctly. Me and Russell are saying the same thing! > > The current drivers implement tx_status as follows. > > static enum dma_status xyz_tx_status(struct dma_chan *chan, > dma_cookie_t cookie, struct dma_tx_state *state) > { > ... > ret = dma_cookie_status(&c->vc.chan, cookie, state); > if (ret == DMA_COMPLETE) > return ret; > ... > } > > What Vinod is telling me that I need to set the cookie to complete > whether the transaction is successful or not if the request was accepted > by HW. xyz_tx_status is just an indication that the transaction was accepted > by HW. An error can happen as a result of transaction execution. Nope, if the txn is completed you mark it complete. If you can detect error (can you??) then you can report DMA_ERROR. In that latter case do not use dma_async_is_complete() to check. You would need to store and report that cookie 'x' failed which you report status in .tx_statis() > > If I call dma_cookie_complete for all transactions regardless of transaction > success or not, then the xyz_tx_status returns DMA_COMPLETE. Again that is based on your implementation. > This also matches what Vinod is saying. The transaction is complete but > it may not be success. I'm saying that if we follow this scheme, then > we should not call the callback. That is not in driver's control. If the callback is set, you have to call it. Client may choose to not set it. > I also made the argument that the driver should not call dma_cookie_complete > for failure cases and somehow return an error for transactions that failed. > This is your suggestion. > > I don't think it matches Vinod's expectation. It does! > > Most of the helpers in drivers/dma/dmaengine.h are there to _assist_ > > the driver writer - they can't do magic. dma_cookie_status() will > > return from the point of view of the generic DMA code what the status > > of a particular cookie is, and the cookie state. It doesn't take > > care of whether a particular transaction associated with a cookie > > failed or not - that's up to the driver. > > > > So, if dma_cookie_status() says that a cookie has DMA_COMPLETED > > status, and the DMA engine is able to detect errors on individual > > transfers, then the driver needs to do further status lookup to > > determine whether the particular transaction referred to by the > > cookie did fail, and modify the returned status appropriately. > > > > If dma_cookie_status() says that the cookie is DMA_IN_PROGRESS, > > then the driver is expected to calculate and report the residue > > (the remaining number of bytes) of the referred to transaction. > > > > This part is fine. I'm worried about transactions that are failing. And you issue is complete orthogonal to this debate. I am not saying we should not discuss this, but you fix would be entirely different here (going by data you have provided till now) -- ~Vinod
next prev parent reply other threads:[~2016-08-08 9:02 UTC|newest] Thread overview: 80+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-07-14 2:57 [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback Sinan Kaya 2016-07-14 2:57 ` Sinan Kaya 2016-07-16 1:00 ` Sinan Kaya 2016-07-16 1:00 ` Sinan Kaya 2016-07-24 6:24 ` Vinod Koul 2016-07-24 6:24 ` Vinod Koul 2016-07-25 14:19 ` Sinan Kaya 2016-07-25 14:19 ` Sinan Kaya 2016-08-04 12:55 ` Vinod Koul 2016-08-04 12:55 ` Vinod Koul 2016-08-04 12:55 ` Vinod Koul 2016-08-04 14:17 ` Sinan Kaya 2016-08-04 14:17 ` Sinan Kaya 2016-08-04 14:40 ` Russell King - ARM Linux 2016-08-04 14:40 ` Russell King - ARM Linux 2016-08-04 15:27 ` Sinan Kaya 2016-08-04 15:27 ` Sinan Kaya 2016-08-04 15:38 ` Russell King - ARM Linux 2016-08-04 15:38 ` Russell King - ARM Linux 2016-08-04 15:59 ` Lars-Peter Clausen 2016-08-04 15:59 ` Lars-Peter Clausen 2016-08-08 9:08 ` Vinod Koul 2016-08-08 9:08 ` Vinod Koul 2016-08-08 12:25 ` Lars-Peter Clausen 2016-08-08 12:25 ` Lars-Peter Clausen 2016-08-10 17:23 ` Vinod Koul 2016-08-10 17:23 ` Vinod Koul 2016-08-10 17:23 ` Vinod Koul 2016-08-04 16:08 ` Sinan Kaya 2016-08-04 16:08 ` Sinan Kaya 2016-08-04 16:15 ` Lars-Peter Clausen 2016-08-04 16:15 ` Lars-Peter Clausen 2016-08-05 6:32 ` Robert Jarzmik 2016-08-05 6:32 ` Robert Jarzmik 2016-08-05 8:34 ` Lars-Peter Clausen 2016-08-05 8:34 ` Lars-Peter Clausen 2016-08-05 8:34 ` Lars-Peter Clausen 2016-08-05 15:17 ` Sinan Kaya 2016-08-05 15:17 ` Sinan Kaya 2016-08-08 9:02 ` Vinod Koul [this message] 2016-08-08 9:02 ` Vinod Koul 2016-08-08 14:45 ` Sinan Kaya 2016-08-08 14:45 ` Sinan Kaya 2016-08-10 17:28 ` Vinod Koul 2016-08-10 17:28 ` Vinod Koul 2016-08-10 17:28 ` Vinod Koul 2016-08-10 17:31 ` Sinan Kaya 2016-08-10 17:31 ` Sinan Kaya 2016-08-10 17:31 ` Sinan Kaya 2016-08-19 2:48 ` Vinod Koul 2016-08-19 2:48 ` Vinod Koul 2016-08-19 3:26 ` Sinan Kaya 2016-08-19 3:26 ` Sinan Kaya 2016-08-19 3:26 ` Sinan Kaya 2016-08-19 3:42 ` Vinod Koul 2016-08-19 3:42 ` Vinod Koul 2016-08-19 3:48 ` Sinan Kaya 2016-08-19 3:48 ` Sinan Kaya 2016-08-19 5:52 ` Vinod Koul 2016-08-19 5:52 ` Vinod Koul 2016-08-19 11:13 ` okaya 2016-08-19 11:13 ` okaya at codeaurora.org 2016-08-19 11:13 ` okaya 2016-08-19 17:02 ` Vinod Koul 2016-08-19 17:02 ` Vinod Koul 2016-08-19 17:02 ` Vinod Koul 2016-08-19 17:21 ` Sinan Kaya 2016-08-19 17:21 ` Sinan Kaya 2016-08-19 17:21 ` Sinan Kaya 2016-08-22 6:08 ` Vinod Koul 2016-08-22 6:08 ` Vinod Koul 2016-08-22 6:08 ` Vinod Koul 2016-08-22 13:27 ` Sinan Kaya 2016-08-22 13:27 ` Sinan Kaya 2016-08-22 17:00 ` Vinod Koul 2016-08-22 17:00 ` Vinod Koul 2016-08-08 8:51 ` Vinod Koul 2016-08-08 8:51 ` Vinod Koul 2016-08-08 12:10 ` okaya 2016-08-08 12:10 ` okaya at codeaurora.org
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=20160808090203.GY9681@localhost \ --to=vinod.koul@intel.com \ --cc=cov@codeaurora.org \ --cc=dmaengine@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=okaya@codeaurora.org \ --cc=timur@codeaurora.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: linkBe 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.