| Message ID | 20131022210828.31348.41879.stgit@viggo.jf.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 8326047bf61dcd52de8f28872fae2ec8f6402a32 |
| Delegated to: | Dan Williams |
| Headers | show |
On Fri, Oct 18, 2013 at 6:06 PM, Jon Mason <jon.mason@intel.com> wrote: > On Fri, Oct 18, 2013 at 07:35:31PM +0200, Bartlomiej Zolnierkiewicz wrote: >> Use the generic unmap object to unmap dma buffers. >> >> As NTB can be compiled without DMA_ENGINE support add > > Seems like the stubs should be added outside of this patch. I think they are ok here as this is the only driver that uses them. The alternative is a new api patch without a user. > Also, the > comment implies that NTB could not be compiled without DMA_ENGINE > support before, which it could be. Hmm, I read it as "since NTB *can* be compiled without dmaengine here are some stubs". > >> stub functions to <linux/dmaengine.h> for dma_set_unmap(), >> dmaengine_get_unmap_data() and dmaengine_unmap_put(). >> >> Cc: Dan Williams <djbw@fb.com> >> Cc: Vinod Koul <vinod.koul@intel.com> >> Cc: Tomasz Figa <t.figa@samsung.com> >> Cc: Dave Jiang <dave.jiang@intel.com> >> Cc: Jon Mason <jon.mason@intel.com> >> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >> --- >> drivers/ntb/ntb_transport.c | 63 +++++++++++++++++++++++++++++++++------------ >> include/linux/dmaengine.h | 15 +++++++++++ >> 2 files changed, 61 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c >> index 12a9e83..fc6bbf1 100644 >> --- a/drivers/ntb/ntb_transport.c >> +++ b/drivers/ntb/ntb_transport.c >> @@ -1034,7 +1034,7 @@ static void ntb_async_rx(struct ntb_queue_entry *entry, void *offset, >> struct dma_chan *chan = qp->dma_chan; >> struct dma_device *device; >> size_t pay_off, buff_off; >> - dma_addr_t src, dest; >> + struct dmaengine_unmap_data *unmap; >> dma_cookie_t cookie; >> void *buf = entry->buf; >> unsigned long flags; >> @@ -1054,27 +1054,41 @@ static void ntb_async_rx(struct ntb_queue_entry *entry, void *offset, >> if (!is_dma_copy_aligned(device, pay_off, buff_off, len)) >> goto err1; >> >> - dest = dma_map_single(device->dev, buf, len, DMA_FROM_DEVICE); >> - if (dma_mapping_error(device->dev, dest)) >> + unmap = dmaengine_get_unmap_data(device->dev, 2, GFP_NOIO); >> + if (!unmap) >> goto err1; >> >> - src = dma_map_single(device->dev, offset, len, DMA_TO_DEVICE); >> - if (dma_mapping_error(device->dev, src)) >> + unmap->addr[0] = dma_map_page(device->dev, virt_to_page(offset), >> + pay_off, len, DMA_TO_DEVICE); >> + if (dma_mapping_error(device->dev, unmap->addr[0])) >> goto err2; >> >> - flags = DMA_COMPL_DEST_UNMAP_SINGLE | DMA_COMPL_SRC_UNMAP_SINGLE | >> + unmap->to_cnt = 1; >> + >> + unmap->addr[1] = dma_map_page(device->dev, virt_to_page(buf), >> + buff_off, len, DMA_FROM_DEVICE); >> + if (dma_mapping_error(device->dev, unmap->addr[1])) >> + goto err2; >> + >> + unmap->from_cnt = 1; >> + >> + flags = DMA_COMPL_SKIP_SRC_UNMAP | DMA_COMPL_SKIP_DEST_UNMAP | >> DMA_PREP_INTERRUPT; >> - txd = device->device_prep_dma_memcpy(chan, dest, src, len, flags); >> + txd = device->device_prep_dma_memcpy(chan, unmap->addr[1], >> + unmap->addr[0], len, flags); > > I must say, as a user I dislike this interface much less than the > previous. Can we abstract all of this away in a helper function > that simply takes the src, dest, and len, then maps and calls > device_prep_dma_memcpy? The driver does not keep track of the > dmaengine_unmap_data, so the helper function could simply return an > error if something isn't happy. Thoughts? Well, that's almost dma_async_memcpy_pg_to_pg, except NTB wants it's own callback. We could add a new helper that does dma_async_memcpy_pg_to_pg() with callback, but I want to do a larger reorganization of dmaengine to try to kill off the need to specify the callback after ->prep(). Can we go with as is for now and cleanup later? The same is happening to the other clients in this patchset (async_tx, dmatest, net_dma) in terms of picking up unmap responsibility from the drivers. It looks "worse" for each client at this stage, but the overall effect is: 34 files changed, 538 insertions(+), 1157 deletions(-) -- Dan -- 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 Tue, Oct 22, 2013 at 02:29:36PM -0700, Dan Williams wrote: > On Fri, Oct 18, 2013 at 6:06 PM, Jon Mason <jon.mason@intel.com> wrote: > > On Fri, Oct 18, 2013 at 07:35:31PM +0200, Bartlomiej Zolnierkiewicz wrote: > >> Use the generic unmap object to unmap dma buffers. > >> > >> As NTB can be compiled without DMA_ENGINE support add > > > > Seems like the stubs should be added outside of this patch. > > I think they are ok here as this is the only driver that uses them. > The alternative is a new api patch without a user. > > > Also, the > > comment implies that NTB could not be compiled without DMA_ENGINE > > support before, which it could be. > > Hmm, I read it as "since NTB *can* be compiled without dmaengine here > are some stubs". This poses an overall question of whether it would simply be better to abstract all of the with/without DMA_ENGINE part and simply remap it to memcpy if DMA_ENGINE is not set (or if the DMA engine is hotplugged). Of course, this is outside the scope of this patch. > > > >> stub functions to <linux/dmaengine.h> for dma_set_unmap(), > >> dmaengine_get_unmap_data() and dmaengine_unmap_put(). > >> > >> Cc: Dan Williams <djbw@fb.com> > >> Cc: Vinod Koul <vinod.koul@intel.com> > >> Cc: Tomasz Figa <t.figa@samsung.com> > >> Cc: Dave Jiang <dave.jiang@intel.com> > >> Cc: Jon Mason <jon.mason@intel.com> > >> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > >> --- > >> drivers/ntb/ntb_transport.c | 63 +++++++++++++++++++++++++++++++++------------ > >> include/linux/dmaengine.h | 15 +++++++++++ > >> 2 files changed, 61 insertions(+), 17 deletions(-) > >> > >> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c > >> index 12a9e83..fc6bbf1 100644 > >> --- a/drivers/ntb/ntb_transport.c > >> +++ b/drivers/ntb/ntb_transport.c > >> @@ -1034,7 +1034,7 @@ static void ntb_async_rx(struct ntb_queue_entry *entry, void *offset, > >> struct dma_chan *chan = qp->dma_chan; > >> struct dma_device *device; > >> size_t pay_off, buff_off; > >> - dma_addr_t src, dest; > >> + struct dmaengine_unmap_data *unmap; > >> dma_cookie_t cookie; > >> void *buf = entry->buf; > >> unsigned long flags; > >> @@ -1054,27 +1054,41 @@ static void ntb_async_rx(struct ntb_queue_entry *entry, void *offset, > >> if (!is_dma_copy_aligned(device, pay_off, buff_off, len)) > >> goto err1; > >> > >> - dest = dma_map_single(device->dev, buf, len, DMA_FROM_DEVICE); > >> - if (dma_mapping_error(device->dev, dest)) > >> + unmap = dmaengine_get_unmap_data(device->dev, 2, GFP_NOIO); > >> + if (!unmap) > >> goto err1; > >> > >> - src = dma_map_single(device->dev, offset, len, DMA_TO_DEVICE); > >> - if (dma_mapping_error(device->dev, src)) > >> + unmap->addr[0] = dma_map_page(device->dev, virt_to_page(offset), > >> + pay_off, len, DMA_TO_DEVICE); > >> + if (dma_mapping_error(device->dev, unmap->addr[0])) > >> goto err2; > >> > >> - flags = DMA_COMPL_DEST_UNMAP_SINGLE | DMA_COMPL_SRC_UNMAP_SINGLE | > >> + unmap->to_cnt = 1; > >> + > >> + unmap->addr[1] = dma_map_page(device->dev, virt_to_page(buf), > >> + buff_off, len, DMA_FROM_DEVICE); > >> + if (dma_mapping_error(device->dev, unmap->addr[1])) > >> + goto err2; > >> + > >> + unmap->from_cnt = 1; > >> + > >> + flags = DMA_COMPL_SKIP_SRC_UNMAP | DMA_COMPL_SKIP_DEST_UNMAP | > >> DMA_PREP_INTERRUPT; > >> - txd = device->device_prep_dma_memcpy(chan, dest, src, len, flags); > >> + txd = device->device_prep_dma_memcpy(chan, unmap->addr[1], > >> + unmap->addr[0], len, flags); > > > > I must say, as a user I dislike this interface much less than the > > previous. Can we abstract all of this away in a helper function > > that simply takes the src, dest, and len, then maps and calls > > device_prep_dma_memcpy? The driver does not keep track of the > > dmaengine_unmap_data, so the helper function could simply return an > > error if something isn't happy. Thoughts? > > Well, that's almost dma_async_memcpy_pg_to_pg, except NTB wants it's > own callback. We could add a new helper that does > dma_async_memcpy_pg_to_pg() with callback, but I want to do a larger > reorganization of dmaengine to try to kill off the need to specify the > callback after ->prep(). Can we go with as is for now and cleanup > later? The same is happening to the other clients in this patchset > (async_tx, dmatest, net_dma) in terms of picking up unmap > responsibility from the drivers. It looks "worse" for each client at > this stage, but the overall effect is: > > 34 files changed, 538 insertions(+), 1157 deletions(-) That is fine. It can be like this in the short term. Thanks, Jon > > -- > Dan -- 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 Tue, Oct 22, 2013 at 4:12 PM, Jon Mason <jon.mason@intel.com> wrote: > On Tue, Oct 22, 2013 at 02:29:36PM -0700, Dan Williams wrote: >> On Fri, Oct 18, 2013 at 6:06 PM, Jon Mason <jon.mason@intel.com> wrote: >> > On Fri, Oct 18, 2013 at 07:35:31PM +0200, Bartlomiej Zolnierkiewicz wrote: >> >> Use the generic unmap object to unmap dma buffers. >> >> >> >> As NTB can be compiled without DMA_ENGINE support add >> > >> > Seems like the stubs should be added outside of this patch. >> >> I think they are ok here as this is the only driver that uses them. >> The alternative is a new api patch without a user. >> >> > Also, the >> > comment implies that NTB could not be compiled without DMA_ENGINE >> > support before, which it could be. >> >> Hmm, I read it as "since NTB *can* be compiled without dmaengine here >> are some stubs". > > This poses an overall question of whether it would simply be better to > abstract all of the with/without DMA_ENGINE part and simply remap it > to memcpy if DMA_ENGINE is not set (or if the DMA engine is > hotplugged). Of course, this is outside the scope of this patch. That's at least the promise of async_memcpy() it does not care if a channel is there or not, but I think it is better if the client has a strict dma and non-dma path. Hiding the dma details from the client seems to have been the wrong choice at least for raid. > That is fine. It can be like this in the short term. > > Thanks, > Jon I'll take that as: Acked-by: Jon Mason <jon.mason@intel.com> ...but holler if not. -- 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 Tue, Oct 22, 2013 at 06:05:31PM -0700, Dan Williams wrote: > On Tue, Oct 22, 2013 at 4:12 PM, Jon Mason <jon.mason@intel.com> wrote: > > On Tue, Oct 22, 2013 at 02:29:36PM -0700, Dan Williams wrote: > >> On Fri, Oct 18, 2013 at 6:06 PM, Jon Mason <jon.mason@intel.com> wrote: > >> > On Fri, Oct 18, 2013 at 07:35:31PM +0200, Bartlomiej Zolnierkiewicz wrote: > >> >> Use the generic unmap object to unmap dma buffers. > >> >> > >> >> As NTB can be compiled without DMA_ENGINE support add > >> > > >> > Seems like the stubs should be added outside of this patch. > >> > >> I think they are ok here as this is the only driver that uses them. > >> The alternative is a new api patch without a user. > >> > >> > Also, the > >> > comment implies that NTB could not be compiled without DMA_ENGINE > >> > support before, which it could be. > >> > >> Hmm, I read it as "since NTB *can* be compiled without dmaengine here > >> are some stubs". > > > > This poses an overall question of whether it would simply be better to > > abstract all of the with/without DMA_ENGINE part and simply remap it > > to memcpy if DMA_ENGINE is not set (or if the DMA engine is > > hotplugged). Of course, this is outside the scope of this patch. > > That's at least the promise of async_memcpy() it does not care if a > channel is there or not, but I think it is better if the client has a > strict dma and non-dma path. Hiding the dma details from the client > seems to have been the wrong choice at least for raid. > > > That is fine. It can be like this in the short term. > > > > Thanks, > > Jon > > I'll take that as: > > Acked-by: Jon Mason <jon.mason@intel.com> Begrudgingly-Acked-by: Jon Mason <jon.mason@intel.com> > > ...but holler if not. -- 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
diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c index 12a9e83c008b..fc6bbf1e16d9 100644 --- a/drivers/ntb/ntb_transport.c +++ b/drivers/ntb/ntb_transport.c @@ -1034,7 +1034,7 @@ static void ntb_async_rx(struct ntb_queue_entry *entry, void *offset, struct dma_chan *chan = qp->dma_chan; struct dma_device *device; size_t pay_off, buff_off; - dma_addr_t src, dest; + struct dmaengine_unmap_data *unmap; dma_cookie_t cookie; void *buf = entry->buf; unsigned long flags; @@ -1054,27 +1054,41 @@ static void ntb_async_rx(struct ntb_queue_entry *entry, void *offset, if (!is_dma_copy_aligned(device, pay_off, buff_off, len)) goto err1; - dest = dma_map_single(device->dev, buf, len, DMA_FROM_DEVICE); - if (dma_mapping_error(device->dev, dest)) + unmap = dmaengine_get_unmap_data(device->dev, 2, GFP_NOIO); + if (!unmap) goto err1; - src = dma_map_single(device->dev, offset, len, DMA_TO_DEVICE); - if (dma_mapping_error(device->dev, src)) + unmap->addr[0] = dma_map_page(device->dev, virt_to_page(offset), + pay_off, len, DMA_TO_DEVICE); + if (dma_mapping_error(device->dev, unmap->addr[0])) goto err2; - flags = DMA_COMPL_DEST_UNMAP_SINGLE | DMA_COMPL_SRC_UNMAP_SINGLE | + unmap->to_cnt = 1; + + unmap->addr[1] = dma_map_page(device->dev, virt_to_page(buf), + buff_off, len, DMA_FROM_DEVICE); + if (dma_mapping_error(device->dev, unmap->addr[1])) + goto err2; + + unmap->from_cnt = 1; + + flags = DMA_COMPL_SKIP_SRC_UNMAP | DMA_COMPL_SKIP_DEST_UNMAP | DMA_PREP_INTERRUPT; - txd = device->device_prep_dma_memcpy(chan, dest, src, len, flags); + txd = device->device_prep_dma_memcpy(chan, unmap->addr[1], + unmap->addr[0], len, flags); if (!txd) - goto err3; + goto err2; txd->callback = ntb_rx_copy_callback; txd->callback_param = entry; + dma_set_unmap(txd, unmap); cookie = dmaengine_submit(txd); if (dma_submit_error(cookie)) goto err3; + dmaengine_unmap_put(unmap); + qp->last_cookie = cookie; qp->rx_async++; @@ -1082,9 +1096,9 @@ static void ntb_async_rx(struct ntb_queue_entry *entry, void *offset, return; err3: - dma_unmap_single(device->dev, src, len, DMA_TO_DEVICE); + dmaengine_unmap_put(unmap); err2: - dma_unmap_single(device->dev, dest, len, DMA_FROM_DEVICE); + dmaengine_unmap_put(unmap); err1: /* If the callbacks come out of order, the writing of the index to the * last completed will be out of order. This may result in the @@ -1245,7 +1259,8 @@ static void ntb_async_tx(struct ntb_transport_qp *qp, struct dma_chan *chan = qp->dma_chan; struct dma_device *device; size_t dest_off, buff_off; - dma_addr_t src, dest; + struct dmaengine_unmap_data *unmap; + dma_addr_t dest; dma_cookie_t cookie; void __iomem *offset; size_t len = entry->len; @@ -1273,28 +1288,42 @@ static void ntb_async_tx(struct ntb_transport_qp *qp, if (!is_dma_copy_aligned(device, buff_off, dest_off, len)) goto err; - src = dma_map_single(device->dev, buf, len, DMA_TO_DEVICE); - if (dma_mapping_error(device->dev, src)) + unmap = dmaengine_get_unmap_data(device->dev, 1, GFP_NOIO); + if (!unmap) goto err; - flags = DMA_COMPL_SRC_UNMAP_SINGLE | DMA_PREP_INTERRUPT; - txd = device->device_prep_dma_memcpy(chan, dest, src, len, flags); + unmap->addr[0] = dma_map_page(device->dev, virt_to_page(buf), + buff_off, len, DMA_TO_DEVICE); + if (dma_mapping_error(device->dev, unmap->addr[0])) + goto err1; + + unmap->to_cnt = 1; + + flags = DMA_COMPL_SKIP_SRC_UNMAP | DMA_COMPL_SKIP_DEST_UNMAP | + DMA_PREP_INTERRUPT; + txd = device->device_prep_dma_memcpy(chan, dest, unmap->addr[0], len, + flags); if (!txd) goto err1; txd->callback = ntb_tx_copy_callback; txd->callback_param = entry; + dma_set_unmap(txd, unmap); cookie = dmaengine_submit(txd); if (dma_submit_error(cookie)) - goto err1; + goto err2; + + dmaengine_unmap_put(unmap); dma_async_issue_pending(chan); qp->tx_async++; return; +err2: + dmaengine_unmap_put(unmap); err1: - dma_unmap_single(device->dev, src, len, DMA_TO_DEVICE); + dmaengine_unmap_put(unmap); err: ntb_memcpy_tx(entry, offset); qp->tx_memcpy++; diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index dc98bc5d4929..3782cdb782a8 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -457,6 +457,7 @@ struct dma_async_tx_descriptor { #endif }; +#ifdef CONFIG_DMA_ENGINE static inline void dma_set_unmap(struct dma_async_tx_descriptor *tx, struct dmaengine_unmap_data *unmap) { @@ -467,6 +468,20 @@ static inline void dma_set_unmap(struct dma_async_tx_descriptor *tx, struct dmaengine_unmap_data * dmaengine_get_unmap_data(struct device *dev, int nr, gfp_t flags); void dmaengine_unmap_put(struct dmaengine_unmap_data *unmap); +#else +static inline void dma_set_unmap(struct dma_async_tx_descriptor *tx, + struct dmaengine_unmap_data *unmap) +{ +} +static inline struct dmaengine_unmap_data * +dmaengine_get_unmap_data(struct device *dev, int nr, gfp_t flags) +{ + return NULL; +} +static inline void dmaengine_unmap_put(struct dmaengine_unmap_data *unmap) +{ +} +#endif static inline void dma_descriptor_unmap(struct dma_async_tx_descriptor *tx) {