On 07/12/2016 01:45 AM, Dave Jiang wrote:
> Adding error reporting infrastructure though the DMA cookie.
> An error status will be set in the cookie if the descriptor is
> returned from driver as failed in order to notify upper layer of
> the status.
>
> Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
Hi,
Thanks for working on this. But I'm afraid that this particular approach
will not work. The idea behind the cookies is that they are stateless except
for the cookie value itself and do not require any lifetime management. The
cookie values are monotonically incrementing which means you can easily
check whether a transfer is completed by comparing its value to the last
completed transfer. No additional state is required.
What you are doing in this patch is conceptionally the same as you did in
the first version of your patch. You assign additional state as well as
lifetime management to the cookie embedded in the dma_async_tx_descriptor
struct. The lifetime of this new cookie struct is the same as the
tx_descriptor it belong to though and all your interfaces work on a struct
dma_async_tx_descriptor as well. So in essence this is not any different
compared to embedding the error code in the tx_descriptor itself. In
addition this patch series and this patch in particular introduce a fair
amount of memory leaks since not everybody is calling dma_cookie_free().
So the same review points of version 1 of the patch still apply.
My personal preference for error reporting is to add status field to the
complete callback. This state struct should contain error contentions but
also other status information like the number of bytes transferred for the
descriptor.
- Lars
> ---
> drivers/dma/dmaengine.c | 4 ++--
> drivers/dma/dmaengine.h | 38 +++++++++++++++++++++++++++++---------
> drivers/dma/hsu/hsu.c | 2 +-
> drivers/dma/ioat/dma.h | 2 +-
> drivers/dma/virt-dma.c | 4 ++--
> drivers/dma/virt-dma.h | 4 ++--
> include/linux/dmaengine.h | 31 ++++++++++++++++++++++++++++++-
> 7 files changed, 67 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index 8c9f45f..21fd715 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -1224,7 +1224,7 @@ dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx)
> if (!tx)
> return DMA_COMPLETE;
>
> - while (tx->cookie == -EBUSY) {
> + while (tx->cookie->cookie == -EBUSY) {
> if (time_after_eq(jiffies, dma_sync_wait_timeout)) {
> dev_err(tx->chan->device->dev,
> "%s timeout waiting for descriptor submission\n",
> @@ -1233,7 +1233,7 @@ dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx)
> }
> cpu_relax();
> }
> - return dma_sync_wait(tx->chan, tx->cookie);
> + return dma_sync_wait(tx->chan, tx->cookie->cookie);
> }
> EXPORT_SYMBOL_GPL(dma_wait_for_async_tx);
>
> diff --git a/drivers/dma/dmaengine.h b/drivers/dma/dmaengine.h
> index 17f983a..048aa81 100644
> --- a/drivers/dma/dmaengine.h
> +++ b/drivers/dma/dmaengine.h
> @@ -6,6 +6,7 @@
> #define DMAENGINE_H
>
> #include <linux/bug.h>
> +#include <linux/slab.h>
> #include <linux/dmaengine.h>
>
> /**
> @@ -28,14 +29,19 @@ static inline void dma_cookie_init(struct dma_chan *chan)
> static inline dma_cookie_t dma_cookie_assign(struct dma_async_tx_descriptor *tx)
> {
> struct dma_chan *chan = tx->chan;
> - dma_cookie_t cookie;
> + struct dma_cookie *cookie;
>
> - cookie = chan->cookie + 1;
> - if (cookie < DMA_MIN_COOKIE)
> - cookie = DMA_MIN_COOKIE;
> - tx->cookie = chan->cookie = cookie;
> + cookie = kzalloc(sizeof(struct dma_cookie), GFP_KERNEL);
> + if (!cookie)
> + return -ENOMEM;
>
> - return cookie;
> + cookie->cookie = chan->cookie + 1;
> + if (cookie->cookie < DMA_MIN_COOKIE)
> + cookie->cookie = DMA_MIN_COOKIE;
> + tx->cookie->cookie = chan->cookie;
> + tx->cookie = cookie;
> +
> + return cookie->cookie;
> }
>
> /**
> @@ -50,9 +56,23 @@ static inline dma_cookie_t dma_cookie_assign(struct dma_async_tx_descriptor *tx)
> */
> static inline void dma_cookie_complete(struct dma_async_tx_descriptor *tx)
> {
> - BUG_ON(tx->cookie < DMA_MIN_COOKIE);
> - tx->chan->completed_cookie = tx->cookie;
> - tx->cookie = 0;
> + BUG_ON(tx->cookie->cookie < DMA_MIN_COOKIE);
> + tx->chan->completed_cookie = tx->cookie->cookie;
> + tx->cookie->cookie = 0;
> +}
> +
> +/**
> + * dma_cookie_free - free the dma cookie
> + * @tx: descriptor that points to the cookie
> + *
> + * Free the allocated dma_cookie
> + */
> +static inline void dma_cookie_free(struct dma_async_tx_descriptor *tx)
> +{
> + if (tx->cookie) {
> + kfree(tx->cookie);
> + tx->cookie = NULL;
> + }
> }
>
> /**
> diff --git a/drivers/dma/hsu/hsu.c b/drivers/dma/hsu/hsu.c
> index f8c5cd5..50e714a 100644
> --- a/drivers/dma/hsu/hsu.c
> +++ b/drivers/dma/hsu/hsu.c
> @@ -283,7 +283,7 @@ static enum dma_status hsu_dma_tx_status(struct dma_chan *chan,
>
> spin_lock_irqsave(&hsuc->vchan.lock, flags);
> vdesc = vchan_find_desc(&hsuc->vchan, cookie);
> - if (hsuc->desc && cookie == hsuc->desc->vdesc.tx.cookie) {
> + if (hsuc->desc && cookie == hsuc->desc->vdesc.tx.cookie->cookie) {
> bytes = hsu_dma_active_desc_size(hsuc);
> dma_set_residue(state, bytes);
> status = hsuc->desc->status;
> diff --git a/drivers/dma/ioat/dma.h b/drivers/dma/ioat/dma.h
> index a9bc1a1..d73acec 100644
> --- a/drivers/dma/ioat/dma.h
> +++ b/drivers/dma/ioat/dma.h
> @@ -231,7 +231,7 @@ __dump_desc_dbg(struct ioatdma_chan *ioat_chan, struct ioat_dma_descriptor *hw,
> dev_dbg(dev, "desc[%d]: (%#llx->%#llx) cookie: %d flags: %#x"
> " ctl: %#10.8x (op: %#x int_en: %d compl: %d)\n", id,
> (unsigned long long) tx->phys,
> - (unsigned long long) hw->next, tx->cookie, tx->flags,
> + (unsigned long long) hw->next, tx->cookie->cookie, tx->flags,
> hw->ctl, hw->ctl_f.op, hw->ctl_f.int_en, hw->ctl_f.compl_write);
> }
>
> diff --git a/drivers/dma/virt-dma.c b/drivers/dma/virt-dma.c
> index a35c211..9487873 100644
> --- a/drivers/dma/virt-dma.c
> +++ b/drivers/dma/virt-dma.c
> @@ -60,7 +60,7 @@ int vchan_tx_desc_free(struct dma_async_tx_descriptor *tx)
> spin_unlock_irqrestore(&vc->lock, flags);
>
> dev_dbg(vc->chan.device->dev, "vchan %p: txd %p[%x]: freeing\n",
> - vc, vd, vd->tx.cookie);
> + vc, vd, vd->tx.cookie->cookie);
> vc->desc_free(vd);
> return 0;
> }
> @@ -72,7 +72,7 @@ struct virt_dma_desc *vchan_find_desc(struct virt_dma_chan *vc,
> struct virt_dma_desc *vd;
>
> list_for_each_entry(vd, &vc->desc_issued, node)
> - if (vd->tx.cookie == cookie)
> + if (vd->tx.cookie->cookie == cookie)
> return vd;
>
> return NULL;
> diff --git a/drivers/dma/virt-dma.h b/drivers/dma/virt-dma.h
> index d9731ca..c82e90a 100644
> --- a/drivers/dma/virt-dma.h
> +++ b/drivers/dma/virt-dma.h
> @@ -92,12 +92,12 @@ static inline bool vchan_issue_pending(struct virt_dma_chan *vc)
> static inline void vchan_cookie_complete(struct virt_dma_desc *vd)
> {
> struct virt_dma_chan *vc = to_virt_chan(vd->tx.chan);
> - dma_cookie_t cookie;
> + struct dma_cookie *cookie;
>
> cookie = vd->tx.cookie;
> dma_cookie_complete(&vd->tx);
> dev_vdbg(vc->chan.device->dev, "txd %p[%x]: marked complete\n",
> - vd, cookie);
> + vd, cookie->cookie);
> list_add_tail(&vd->node, &vc->desc_completed);
>
> tasklet_schedule(&vc->task);
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 30de019..9fa47d4 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -34,6 +34,18 @@
> typedef s32 dma_cookie_t;
> #define DMA_MIN_COOKIE 1
>
> +enum dma_trans_error {
> + DMA_TRANS_NOERROR = 0,
> + DMA_TRANS_READ_FAILED,
> + DMA_TRANS_WRITE_FAILED,
> + DMA_TRANS_ABORTED,
> +};
> +
> +struct dma_cookie {
> + dma_cookie_t cookie;
> + enum dma_trans_error error;
> +};
> +
> static inline int dma_submit_error(dma_cookie_t cookie)
> {
> return cookie < 0 ? cookie : 0;
> @@ -471,7 +483,7 @@ struct dmaengine_unmap_data {
> * @lock: protect the parent and next pointers
> */
> struct dma_async_tx_descriptor {
> - dma_cookie_t cookie;
> + struct dma_cookie *cookie;
> enum dma_ctrl_flags flags; /* not a 'long' to pack with cookie */
> dma_addr_t phys;
> struct dma_chan *chan;
> @@ -781,6 +793,23 @@ struct dma_device {
> void (*device_issue_pending)(struct dma_chan *chan);
> };
>
> +static inline bool dma_trans_good(struct dma_async_tx_descriptor *tx)
> +{
> + return (tx->cookie->cookie == DMA_TRANS_NOERROR) ? true : false;
> +}
> +
> +static inline enum dma_trans_error
> +dma_trans_status(struct dma_async_tx_descriptor *tx)
> +{
> + return tx->cookie->error;
> +}
> +
> +static inline void dma_trans_set_status(struct dma_async_tx_descriptor *tx,
> + enum dma_trans_error status)
> +{
> + tx->cookie->error = status;
> +}
> +
> static inline int dmaengine_slave_config(struct dma_chan *chan,
> struct dma_slave_config *config)
> {
>
> --
> 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
>
--
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
