[PATCH 1/2] dmaengine: Fix status handling in imx-dma.

[PATCH 1/2] dmaengine: Fix status handling in imx-dma.

[Javier Martin]

Status must only be changed to DMA_IN_PROGRESS
when the DMA transfer has really begun.

However, since this driver lacks of support for
multiple descriptors a new flag has to be introduced
to avoid the prepare function be called multiple times.

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/dma/imx-dma.c |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
index d99f71c..9a0ac14 100644
--- a/drivers/dma/imx-dma.c
+++ b/drivers/dma/imx-dma.c
@@ -43,6 +43,7 @@ struct imxdma_channel {
 	enum dma_status			status;
 	int				dma_request;
 	struct scatterlist		*sg_list;
+	bool				prepared;
 };
 
 #define MAX_DMA_CHANNELS 8
@@ -72,6 +73,7 @@ static void imxdma_irq_handler(int channel, void *data)
 
 	imxdmac->status = DMA_SUCCESS;
 	imxdma_handle(imxdmac);
+	imxdmac->prepared = false;
 }
 
 static void imxdma_err_handler(int channel, void *data, int error)
@@ -80,6 +82,7 @@ static void imxdma_err_handler(int channel, void *data, int error)
 
 	imxdmac->status = DMA_ERROR;
 	imxdma_handle(imxdmac);
+	imxdmac->prepared = false;
 }
 
 static void imxdma_progression(int channel, void *data,
@@ -89,6 +92,7 @@ static void imxdma_progression(int channel, void *data,
 
 	imxdmac->status = DMA_SUCCESS;
 	imxdma_handle(imxdmac);
+	imxdmac->prepared = false;
 }
 
 static int imxdma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
@@ -103,6 +107,7 @@ static int imxdma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
 	case DMA_TERMINATE_ALL:
 		imxdmac->status = DMA_ERROR;
 		imx_dma_disable(imxdmac->imxdma_channel);
+		imxdmac->prepared = false;
 		return 0;
 	case DMA_SLAVE_CONFIG:
 		if (dmaengine_cfg->direction == DMA_FROM_DEVICE) {
@@ -204,7 +209,7 @@ static int imxdma_alloc_chan_resources(struct dma_chan *chan)
 	imxdmac->desc.flags = DMA_CTRL_ACK;
 
 	imxdmac->status = DMA_SUCCESS;
-
+	imxdmac->prepared = false;
 	return 0;
 }
 
@@ -230,11 +235,9 @@ static struct dma_async_tx_descriptor *imxdma_prep_slave_sg(
 	int i, ret, dma_length = 0;
 	unsigned int dmamode;
 
-	if (imxdmac->status == DMA_IN_PROGRESS)
+	if (imxdmac->prepared)
 		return NULL;
 
-	imxdmac->status = DMA_IN_PROGRESS;
-
 	for_each_sg(sgl, sg, sg_len, i) {
 		dma_length += sg->length;
 	}
@@ -264,6 +267,8 @@ static struct dma_async_tx_descriptor *imxdma_prep_slave_sg(
 	if (ret)
 		return NULL;
 
+	imxdmac->prepared = true;
+
 	return &imxdmac->desc;
 }
 
@@ -280,9 +285,8 @@ static struct dma_async_tx_descriptor *imxdma_prep_dma_cyclic(
 	dev_dbg(imxdma->dev, "%s channel: %d buf_len=%d period_len=%d\n",
 			__func__, imxdmac->channel, buf_len, period_len);
 
-	if (imxdmac->status == DMA_IN_PROGRESS)
+	if (imxdmac->prepared)
 		return NULL;
-	imxdmac->status = DMA_IN_PROGRESS;
 
 	ret = imx_dma_setup_progression_handler(imxdmac->imxdma_channel,
 			imxdma_progression);
@@ -325,14 +329,18 @@ static struct dma_async_tx_descriptor *imxdma_prep_dma_cyclic(
 	if (ret)
 		return NULL;
 
+	imxdmac->prepared = true;
+
 	return &imxdmac->desc;
 }
 
 static void imxdma_issue_pending(struct dma_chan *chan)
 {
 	/*
-	 * Nothing to do. We only have a single descriptor
+	 * Only change status since we have a single descriptor
 	 */
+	struct imxdma_channel *imxdmac = to_imxdma_chan(chan);
+	imxdmac->status = DMA_IN_PROGRESS;
 }
 
 static int __init imxdma_probe(struct platform_device *pdev)
-- 
1.7.0.4

[PATCH 2/2] [v2] dmaengine: Add support for MEMCPY for imx-dma.

[Javier Martin]

MEMCPY transfers allow DMA copies from memory to
memory. This patch has been tested with dmatest
device driver.

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/dma/imx-dma.c |   37 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 36 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
index 9a0ac14..8e6d680 100644
--- a/drivers/dma/imx-dma.c
+++ b/drivers/dma/imx-dma.c
@@ -201,7 +201,8 @@ static int imxdma_alloc_chan_resources(struct dma_chan *chan)
 	struct imxdma_channel *imxdmac = to_imxdma_chan(chan);
 	struct imx_dma_data *data = chan->private;
 
-	imxdmac->dma_request = data->dma_request;
+	if (data != NULL)
+		imxdmac->dma_request = data->dma_request;
 
 	dma_async_tx_descriptor_init(&imxdmac->desc, chan);
 	imxdmac->desc.tx_submit = imxdma_tx_submit;
@@ -334,6 +335,37 @@ static struct dma_async_tx_descriptor *imxdma_prep_dma_cyclic(
 	return &imxdmac->desc;
 }
 
+static struct dma_async_tx_descriptor *imxdma_prep_dma_memcpy(
+	struct dma_chan *chan, dma_addr_t dest,
+	dma_addr_t src, size_t len, unsigned long flags)
+{
+	struct imxdma_channel *imxdmac = to_imxdma_chan(chan);
+	struct imxdma_engine *imxdma = imxdmac->imxdma;
+	int ret;
+
+	dev_dbg(imxdma->dev, "%s channel: %d src=0x%x dst=0x%x len=%d\n",
+			__func__, imxdmac->channel, src, dest, len);
+
+	if (imxdmac->prepared)
+		return NULL;
+
+	ret = imx_dma_config_channel(imxdmac->imxdma_channel,
+			       IMX_DMA_MEMSIZE_32 | IMX_DMA_TYPE_LINEAR,
+			       IMX_DMA_MEMSIZE_32 | IMX_DMA_TYPE_LINEAR,
+			       0, 0);
+	if (ret)
+		return NULL;
+
+	ret = imx_dma_setup_single(imxdmac->imxdma_channel, src, len,
+				   dest, DMA_MODE_WRITE);
+	if (ret)
+		return NULL;
+
+	imxdmac->prepared = true;
+
+	return &imxdmac->desc;
+}
+
 static void imxdma_issue_pending(struct dma_chan *chan)
 {
 	/*
@@ -356,6 +388,7 @@ static int __init imxdma_probe(struct platform_device *pdev)
 
 	dma_cap_set(DMA_SLAVE, imxdma->dma_device.cap_mask);
 	dma_cap_set(DMA_CYCLIC, imxdma->dma_device.cap_mask);
+	dma_cap_set(DMA_MEMCPY, imxdma->dma_device.cap_mask);
 
 	/* Initialize channel parameters */
 	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
@@ -389,11 +422,13 @@ static int __init imxdma_probe(struct platform_device *pdev)
 	imxdma->dma_device.device_tx_status = imxdma_tx_status;
 	imxdma->dma_device.device_prep_slave_sg = imxdma_prep_slave_sg;
 	imxdma->dma_device.device_prep_dma_cyclic = imxdma_prep_dma_cyclic;
+	imxdma->dma_device.device_prep_dma_memcpy = imxdma_prep_dma_memcpy;
 	imxdma->dma_device.device_control = imxdma_control;
 	imxdma->dma_device.device_issue_pending = imxdma_issue_pending;
 
 	platform_set_drvdata(pdev, imxdma);
 
+	imxdma->dma_device.copy_align = 2; /* 2^2 = 4 bytes alignment */
 	imxdma->dma_device.dev->dma_parms = &imxdma->dma_parms;
 	dma_set_max_seg_size(imxdma->dma_device.dev, 0xffffff);
 
-- 
1.7.0.4

[PATCH 1/2] dmaengine: Fix status handling in imx-dma.

[Vinod Koul]

On Mon, 2012-01-02 at 13:18 +0100, Javier Martin wrote:
> Status must only be changed to DMA_IN_PROGRESS
> when the DMA transfer has really begun.
> 
> However, since this driver lacks of support for
> multiple descriptors a new flag has to be introduced
> to avoid the prepare function be called multiple times.
Thanks this is the right approach to fix this driver

But this will obviously break any users of this driver as they need to
call the right APIs now :D

Sacha: can you check this patch and see which users of this driver will
break. we need those fixes to go along this patch as well
> 
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> ---
>  drivers/dma/imx-dma.c |   22 +++++++++++++++-------
>  1 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
> index d99f71c..9a0ac14 100644
> --- a/drivers/dma/imx-dma.c
> +++ b/drivers/dma/imx-dma.c
> @@ -43,6 +43,7 @@ struct imxdma_channel {
>  	enum dma_status			status;
>  	int				dma_request;
>  	struct scatterlist		*sg_list;
> +	bool				prepared;
>  };
>  
>  #define MAX_DMA_CHANNELS 8
> @@ -72,6 +73,7 @@ static void imxdma_irq_handler(int channel, void *data)
>  
>  	imxdmac->status = DMA_SUCCESS;
>  	imxdma_handle(imxdmac);
> +	imxdmac->prepared = false;
>  }
>  
>  static void imxdma_err_handler(int channel, void *data, int error)
> @@ -80,6 +82,7 @@ static void imxdma_err_handler(int channel, void *data, int error)
>  
>  	imxdmac->status = DMA_ERROR;
>  	imxdma_handle(imxdmac);
> +	imxdmac->prepared = false;
>  }
>  
>  static void imxdma_progression(int channel, void *data,
> @@ -89,6 +92,7 @@ static void imxdma_progression(int channel, void *data,
>  
>  	imxdmac->status = DMA_SUCCESS;
>  	imxdma_handle(imxdmac);
> +	imxdmac->prepared = false;
>  }
>  
>  static int imxdma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> @@ -103,6 +107,7 @@ static int imxdma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
>  	case DMA_TERMINATE_ALL:
>  		imxdmac->status = DMA_ERROR;
>  		imx_dma_disable(imxdmac->imxdma_channel);
> +		imxdmac->prepared = false;
>  		return 0;
>  	case DMA_SLAVE_CONFIG:
>  		if (dmaengine_cfg->direction == DMA_FROM_DEVICE) {
> @@ -204,7 +209,7 @@ static int imxdma_alloc_chan_resources(struct dma_chan *chan)
>  	imxdmac->desc.flags = DMA_CTRL_ACK;
>  
>  	imxdmac->status = DMA_SUCCESS;
> -
> +	imxdmac->prepared = false;
>  	return 0;
>  }
>  
> @@ -230,11 +235,9 @@ static struct dma_async_tx_descriptor *imxdma_prep_slave_sg(
>  	int i, ret, dma_length = 0;
>  	unsigned int dmamode;
>  
> -	if (imxdmac->status == DMA_IN_PROGRESS)
> +	if (imxdmac->prepared)
>  		return NULL;
>  
> -	imxdmac->status = DMA_IN_PROGRESS;
> -
>  	for_each_sg(sgl, sg, sg_len, i) {
>  		dma_length += sg->length;
>  	}
> @@ -264,6 +267,8 @@ static struct dma_async_tx_descriptor *imxdma_prep_slave_sg(
>  	if (ret)
>  		return NULL;
>  
> +	imxdmac->prepared = true;
> +
>  	return &imxdmac->desc;
>  }
>  
> @@ -280,9 +285,8 @@ static struct dma_async_tx_descriptor *imxdma_prep_dma_cyclic(
>  	dev_dbg(imxdma->dev, "%s channel: %d buf_len=%d period_len=%d\n",
>  			__func__, imxdmac->channel, buf_len, period_len);
>  
> -	if (imxdmac->status == DMA_IN_PROGRESS)
> +	if (imxdmac->prepared)
>  		return NULL;
> -	imxdmac->status = DMA_IN_PROGRESS;
>  
>  	ret = imx_dma_setup_progression_handler(imxdmac->imxdma_channel,
>  			imxdma_progression);
> @@ -325,14 +329,18 @@ static struct dma_async_tx_descriptor *imxdma_prep_dma_cyclic(
>  	if (ret)
>  		return NULL;
>  
> +	imxdmac->prepared = true;
> +
>  	return &imxdmac->desc;
>  }
>  
>  static void imxdma_issue_pending(struct dma_chan *chan)
>  {
>  	/*
> -	 * Nothing to do. We only have a single descriptor
> +	 * Only change status since we have a single descriptor
>  	 */
> +	struct imxdma_channel *imxdmac = to_imxdma_chan(chan);
> +	imxdmac->status = DMA_IN_PROGRESS;
>  }
>  
>  static int __init imxdma_probe(struct platform_device *pdev)


-- 
~Vinod

[PATCH 1/2] dmaengine: Fix status handling in imx-dma.

[Sascha Hauer]

On Fri, Jan 06, 2012 at 11:26:29AM +0530, Vinod Koul wrote:
> On Mon, 2012-01-02 at 13:18 +0100, Javier Martin wrote:
> > Status must only be changed to DMA_IN_PROGRESS
> > when the DMA transfer has really begun.
> > 
> > However, since this driver lacks of support for
> > multiple descriptors a new flag has to be introduced
> > to avoid the prepare function be called multiple times.
> Thanks this is the right approach to fix this driver
> 
> But this will obviously break any users of this driver as they need to
> call the right APIs now :D
> 
> Sacha: can you check this patch and see which users of this driver will
> break. we need those fixes to go along this patch as well

Which users should break? I just tried with the mxcmmc driver and this
one does not break.

I do not really understand this patch anyway. It changes imxdmac->status
to a write-only variable and introduces a imxdmac->prepared variable
with the same meaning. This patch is a complicated no-op.

What was the original problem? The fact that we used a enum dma_status
with the wrong semantics? In that case I suggest to simply replace this
variable. All we need to track is that we do not enter
imxdma_prep_slave_sg with an already running transfer.

Sascha

> > 
> > Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> > ---
> >  drivers/dma/imx-dma.c |   22 +++++++++++++++-------
> >  1 files changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
> > index d99f71c..9a0ac14 100644
> > --- a/drivers/dma/imx-dma.c
> > +++ b/drivers/dma/imx-dma.c
> > @@ -43,6 +43,7 @@ struct imxdma_channel {
> >  	enum dma_status			status;
> >  	int				dma_request;
> >  	struct scatterlist		*sg_list;
> > +	bool				prepared;
> >  };
> >  
> >  #define MAX_DMA_CHANNELS 8
> > @@ -72,6 +73,7 @@ static void imxdma_irq_handler(int channel, void *data)
> >  
> >  	imxdmac->status = DMA_SUCCESS;
> >  	imxdma_handle(imxdmac);
> > +	imxdmac->prepared = false;
> >  }
> >  
> >  static void imxdma_err_handler(int channel, void *data, int error)
> > @@ -80,6 +82,7 @@ static void imxdma_err_handler(int channel, void *data, int error)
> >  
> >  	imxdmac->status = DMA_ERROR;
> >  	imxdma_handle(imxdmac);
> > +	imxdmac->prepared = false;
> >  }
> >  
> >  static void imxdma_progression(int channel, void *data,
> > @@ -89,6 +92,7 @@ static void imxdma_progression(int channel, void *data,
> >  
> >  	imxdmac->status = DMA_SUCCESS;
> >  	imxdma_handle(imxdmac);
> > +	imxdmac->prepared = false;
> >  }
> >  
> >  static int imxdma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> > @@ -103,6 +107,7 @@ static int imxdma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd,
> >  	case DMA_TERMINATE_ALL:
> >  		imxdmac->status = DMA_ERROR;
> >  		imx_dma_disable(imxdmac->imxdma_channel);
> > +		imxdmac->prepared = false;
> >  		return 0;
> >  	case DMA_SLAVE_CONFIG:
> >  		if (dmaengine_cfg->direction == DMA_FROM_DEVICE) {
> > @@ -204,7 +209,7 @@ static int imxdma_alloc_chan_resources(struct dma_chan *chan)
> >  	imxdmac->desc.flags = DMA_CTRL_ACK;
> >  
> >  	imxdmac->status = DMA_SUCCESS;
> > -
> > +	imxdmac->prepared = false;
> >  	return 0;
> >  }
> >  
> > @@ -230,11 +235,9 @@ static struct dma_async_tx_descriptor *imxdma_prep_slave_sg(
> >  	int i, ret, dma_length = 0;
> >  	unsigned int dmamode;
> >  
> > -	if (imxdmac->status == DMA_IN_PROGRESS)
> > +	if (imxdmac->prepared)
> >  		return NULL;
> >  
> > -	imxdmac->status = DMA_IN_PROGRESS;
> > -
> >  	for_each_sg(sgl, sg, sg_len, i) {
> >  		dma_length += sg->length;
> >  	}
> > @@ -264,6 +267,8 @@ static struct dma_async_tx_descriptor *imxdma_prep_slave_sg(
> >  	if (ret)
> >  		return NULL;
> >  
> > +	imxdmac->prepared = true;
> > +
> >  	return &imxdmac->desc;
> >  }
> >  
> > @@ -280,9 +285,8 @@ static struct dma_async_tx_descriptor *imxdma_prep_dma_cyclic(
> >  	dev_dbg(imxdma->dev, "%s channel: %d buf_len=%d period_len=%d\n",
> >  			__func__, imxdmac->channel, buf_len, period_len);
> >  
> > -	if (imxdmac->status == DMA_IN_PROGRESS)
> > +	if (imxdmac->prepared)
> >  		return NULL;
> > -	imxdmac->status = DMA_IN_PROGRESS;
> >  
> >  	ret = imx_dma_setup_progression_handler(imxdmac->imxdma_channel,
> >  			imxdma_progression);
> > @@ -325,14 +329,18 @@ static struct dma_async_tx_descriptor *imxdma_prep_dma_cyclic(
> >  	if (ret)
> >  		return NULL;
> >  
> > +	imxdmac->prepared = true;
> > +
> >  	return &imxdmac->desc;
> >  }
> >  
> >  static void imxdma_issue_pending(struct dma_chan *chan)
> >  {
> >  	/*
> > -	 * Nothing to do. We only have a single descriptor
> > +	 * Only change status since we have a single descriptor
> >  	 */
> > +	struct imxdma_channel *imxdmac = to_imxdma_chan(chan);
> > +	imxdmac->status = DMA_IN_PROGRESS;
> >  }
> >  
> >  static int __init imxdma_probe(struct platform_device *pdev)
> 
> 
> -- 
> ~Vinod
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/2] dmaengine: Fix status handling in imx-dma.

[Vinod Koul]

On Fri, 2012-01-06 at 10:37 +0100, Sascha Hauer wrote:
> On Fri, Jan 06, 2012 at 11:26:29AM +0530, Vinod Koul wrote:
> > On Mon, 2012-01-02 at 13:18 +0100, Javier Martin wrote:
> > > Status must only be changed to DMA_IN_PROGRESS
> > > when the DMA transfer has really begun.
> > > 
> > > However, since this driver lacks of support for
> > > multiple descriptors a new flag has to be introduced
> > > to avoid the prepare function be called multiple times.
> > Thanks this is the right approach to fix this driver
> > 
> > But this will obviously break any users of this driver as they need to
> > call the right APIs now :D
> > 
> > Sacha: can you check this patch and see which users of this driver will
> > break. we need those fixes to go along this patch as well
> 
> Which users should break? I just tried with the mxcmmc driver and this
> one does not break.
> 
> I do not really understand this patch anyway. It changes imxdmac->status
> to a write-only variable and introduces a imxdmac->prepared variable
> with the same meaning. This patch is a complicated no-op.
> 
> What was the original problem? The fact that we used a enum dma_status
> with the wrong semantics? In that case I suggest to simply replace this
> variable. All we need to track is that we do not enter
> imxdma_prep_slave_sg with an already running transfer.
the memcpy patch submitted earlier didn't use correct dmaengine API. a
descriptor is not supposed to be started in prepare. The reason given
was the driver already does so for all other prepares, so i suggested to
fix that. Descriptors should be started in issue_pending only.

I thought that change is done in this patch (looks like i didn't look
too carefully), so if prepare is used per API definition, clients which
dont call issue_pending would break...


-- 
~Vinod

[PATCH 1/2] dmaengine: Fix status handling in imx-dma.

[Sascha Hauer]

On Fri, Jan 06, 2012 at 04:40:24PM +0530, Vinod Koul wrote:
> On Fri, 2012-01-06 at 10:37 +0100, Sascha Hauer wrote:
> > On Fri, Jan 06, 2012 at 11:26:29AM +0530, Vinod Koul wrote:
> > > On Mon, 2012-01-02 at 13:18 +0100, Javier Martin wrote:
> > > > Status must only be changed to DMA_IN_PROGRESS
> > > > when the DMA transfer has really begun.
> > > > 
> > > > However, since this driver lacks of support for
> > > > multiple descriptors a new flag has to be introduced
> > > > to avoid the prepare function be called multiple times.
> > > Thanks this is the right approach to fix this driver
> > > 
> > > But this will obviously break any users of this driver as they need to
> > > call the right APIs now :D
> > > 
> > > Sacha: can you check this patch and see which users of this driver will
> > > break. we need those fixes to go along this patch as well
> > 
> > Which users should break? I just tried with the mxcmmc driver and this
> > one does not break.
> > 
> > I do not really understand this patch anyway. It changes imxdmac->status
> > to a write-only variable and introduces a imxdmac->prepared variable
> > with the same meaning. This patch is a complicated no-op.
> > 
> > What was the original problem? The fact that we used a enum dma_status
> > with the wrong semantics? In that case I suggest to simply replace this
> > variable. All we need to track is that we do not enter
> > imxdma_prep_slave_sg with an already running transfer.
> the memcpy patch submitted earlier didn't use correct dmaengine API. a
> descriptor is not supposed to be started in prepare. The reason given
> was the driver already does so for all other prepares, so i suggested to
> fix that. Descriptors should be started in issue_pending only.

The transfer is not started in prepare but in imxdmac->desc.tx_submit

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 1/2] dmaengine: Fix status handling in imx-dma.

[Vinod Koul]

On Fri, 2012-01-06 at 13:43 +0100, Sascha Hauer wrote:
> On Fri, Jan 06, 2012 at 04:40:24PM +0530, Vinod Koul wrote:
> The transfer is not started in prepare but in imxdmac->desc.tx_submit
> 
Well that's incorrect as well. It should be done in issue_pending

Care to fix this up?

-- 
~Vinod