[09/18] dmaengine/amba-pl08x: Schedule tasklet in case of error interrupt
diff mbox

Message ID 9c81beb9cd84f84b58abbd24ebfae85dfe8d8ee1.1311936524.git.viresh.kumar@st.com
State New, archived
Headers show

Commit Message

Viresh KUMAR July 29, 2011, 10:49 a.m. UTC
Currently, if error interrupt occurs, nothing is done in interrupt handler (just
clearing the interrupts). We must somehow indicate this to the user that DMA is
over, due to ERR interrupt or TC interrupt.

So, this patch just schedules existing tasklet, with a print showing error
interrupt has occured on which channels.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---
 drivers/dma/amba-pl08x.c |   43 ++++++++++++++++++-------------------------
 1 files changed, 18 insertions(+), 25 deletions(-)

Comments

Vinod Koul July 29, 2011, 12:16 p.m. UTC | #1
On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
> Currently, if error interrupt occurs, nothing is done in interrupt handler (just
> clearing the interrupts). We must somehow indicate this to the user that DMA is
> over, due to ERR interrupt or TC interrupt.
> 
> So, this patch just schedules existing tasklet, with a print showing error
> interrupt has occured on which channels.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
>  drivers/dma/amba-pl08x.c |   43 ++++++++++++++++++-------------------------
>  1 files changed, 18 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> index c4ce1a2..b9137bc 100644
> --- a/drivers/dma/amba-pl08x.c
> +++ b/drivers/dma/amba-pl08x.c
> @@ -1630,40 +1630,33 @@ static void pl08x_tasklet(unsigned long data)
>  static irqreturn_t pl08x_irq(int irq, void *dev)
>  {
>  	struct pl08x_driver_data *pl08x = dev;
> -	u32 mask = 0;
> -	u32 val;
> -	int i;
> -
> -	val = readl(pl08x->base + PL080_ERR_STATUS);
> -	if (val) {
> -		/* An error interrupt (on one or more channels) */
> -		dev_err(&pl08x->adev->dev,
> -			"%s error interrupt, register value 0x%08x\n",
> -				__func__, val);
> -		/*
> -		 * Simply clear ALL PL08X error interrupts,
> -		 * regardless of channel and cause
> -		 * FIXME: should be 0x00000003 on PL081 really.
> -		 */
> -		writel(0x000000FF, pl08x->base + PL080_ERR_CLEAR);
> +	u32 err, tc, i;
> +
> +	/* check & clear - ERR & TC interrupts */
> +	err = readl(pl08x->base + PL080_ERR_STATUS);
> +	if (err) {
> +		dev_err(&pl08x->adev->dev, "%s error interrupt, register value"
> +				"0x%08x\n", __func__, err);
again this looks convoluted, and the stuff is quotes can be in single
line :)
> +		writel(err, pl08x->base + PL080_ERR_CLEAR);
>  	}
> -	val = readl(pl08x->base + PL080_INT_STATUS);
> -	for (i = 0; i < pl08x->vd->channels; i++) {
> -		if ((1 << i) & val) {
> +	tc = readl(pl08x->base + PL080_INT_STATUS);
> +	if (tc)
> +		writel(tc, pl08x->base + PL080_TC_CLEAR);
> +
> +	if (!err && !tc)
> +		return IRQ_NONE;
> +
> +	for (i = 0; i < pl08x->vd->channels; i++)
> +		if (((1 << i) & err) || ((1 << i) & tc)) {
>  			/* Locate physical channel */
>  			struct pl08x_phy_chan *phychan = &pl08x->phy_chans[i];
>  			struct pl08x_dma_chan *plchan = phychan->serving;
>  
>  			/* Schedule tasklet on this channel */
>  			tasklet_schedule(&plchan->tasklet);
but in tasklet you will call the client callback, so how does the client
know if this was success or error? Did I miss anything?
> -
> -			mask |= (1 << i);
>  		}
> -	}
> -	/* Clear only the terminal interrupts on channels we processed */
> -	writel(mask, pl08x->base + PL080_TC_CLEAR);
>  
> -	return mask ? IRQ_HANDLED : IRQ_NONE;
> +	return IRQ_HANDLED;
>  }
>  
>  static void pl08x_dma_slave_init(struct pl08x_dma_chan *chan)
Russell King - ARM Linux July 29, 2011, 1:02 p.m. UTC | #2
On Fri, Jul 29, 2011 at 05:46:39PM +0530, Koul, Vinod wrote:
> On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
> > +	/* check & clear - ERR & TC interrupts */
> > +	err = readl(pl08x->base + PL080_ERR_STATUS);
> > +	if (err) {
> > +		dev_err(&pl08x->adev->dev, "%s error interrupt, register value"
> > +				"0x%08x\n", __func__, err);
> again this looks convoluted, and the stuff is quotes can be in single
> line :)

And it shows the danger of wrapping quoted strings.  This will print
the following message:

...: pl08x_irq error interrupt, register value0xNNNNNNNN

So it may want a space between 'value' and '0x%08x' - and it may want a
':' after '%s' too.
viresh kumar July 29, 2011, 3:30 p.m. UTC | #3
On 7/29/11, Koul, Vinod <vinod.koul@intel.com> wrote:
> On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
>> +		dev_err(&pl08x->adev->dev, "%s error interrupt, register value"
>> +				"0x%08x\n", __func__, err);
> again this looks convoluted, and the stuff is quotes can be in single
> line :)

Will take care of this in all patches.

> but in tasklet you will call the client callback, so how does the client
> know if this was success or error? Did I miss anything?

No you didn't. I couldn't find a way in framework to report back to
client success or failure.
Is there any way? For now i have only kept error prints.

--
viresh
Linus Walleij July 31, 2011, 12:19 a.m. UTC | #4
2011/7/29 Viresh Kumar <viresh.kumar@st.com>:

(...)
> -       u32 mask = 0;
(...)
> +       if (!err && !tc)
> +               return IRQ_NONE;
(...)
> -
> -                       mask |= (1 << i);
> -       /* Clear only the terminal interrupts on channels we processed */
> -       writel(mask, pl08x->base + PL080_TC_CLEAR);
>
> -       return mask ? IRQ_HANDLED : IRQ_NONE;
> +       return IRQ_HANDLED;

NAK.

These snippets illustrate what is not good about this patch, you're making
the driver fragile by removing checks for spurious IRQ.

So for example if there is an error or TC IRQ on a channel that is not
in use, what happens now?

It used to result in IRQ_NONE, now all of a sudden we start handling
spurious IRQs and claim IRQ_HANDLED with totally unpredictable
results, whereas they would previously gather error metrics for
spurious IRQs.

Yours,
Linus Walleij
viresh kumar July 31, 2011, 5:33 a.m. UTC | #5
On 7/30/11, Linus Walleij <linus.walleij@linaro.org> wrote:
> 2011/7/29 Viresh Kumar <viresh.kumar@st.com>:
> NAK.
>
> These snippets illustrate what is not good about this patch, you're making
> the driver fragile by removing checks for spurious IRQ.
>
> So for example if there is an error or TC IRQ on a channel that is not
> in use, what happens now?
>
> It used to result in IRQ_NONE, now all of a sudden we start handling
> spurious IRQs and claim IRQ_HANDLED with totally unpredictable
> results, whereas they would previously gather error metrics for
> spurious IRQs.
>

I got your point. Will resend this patch with better handling for
spurious interrupts.

--
viresh
Vinod Koul Aug. 4, 2011, 4:25 a.m. UTC | #6
On Fri, 2011-07-29 at 08:30 -0700, viresh kumar wrote:
> On 7/29/11, Koul, Vinod <vinod.koul@intel.com> wrote:
> > On Fri, 2011-07-29 at 16:19 +0530, Viresh Kumar wrote:
> >> +		dev_err(&pl08x->adev->dev, "%s error interrupt, register value"
> >> +				"0x%08x\n", __func__, err);
> > again this looks convoluted, and the stuff is quotes can be in single
> > line :)
> 
> Will take care of this in all patches.
> 
> > but in tasklet you will call the client callback, so how does the client
> > know if this was success or error? Did I miss anything?
> 
> No you didn't. I couldn't find a way in framework to report back to
> client success or failure.
> Is there any way? For now i have only kept error prints.
Sorry for not responding earlier.

Yes that's the whole point, today callback mechanism doesn't tell the
_status_ of the transfer (which if we need change can be discussed as
well), but to counter argue I have never been able to generate the error
interrupt, are you able to do on your controller?

One more point wrt this patch, what do we gain here from calling
tasklet, are you cleaning up your queue for the error descriptor or
something, I think not. so just calling to print doesn't make sense to
me, comments?
Vinod Koul Aug. 4, 2011, 5:32 a.m. UTC | #7
On Thu, 2011-08-04 at 11:24 +0530, viresh kumar wrote:
> On 08/04/2011 09:55 AM, Koul, Vinod wrote:
> > Yes that's the whole point, today callback mechanism doesn't tell the
> > _status_ of the transfer (which if we need change can be discussed as
> > well), but to counter argue I have never been able to generate the error
> > interrupt, are you able to do on your controller?
> 
> Yes, if we supply unaligned address, with access width as word, then it gives
> error interrupt.
> 
> Regarding your point of updating callback for reporting errors,
> I think it is required and should be a common issue.
Okay till now no one has asked for it. We can add to callback arguments
either a bool to indicate what is the transfer status or usual error
code to also indicate what type of failure.
I would prefer latter, if there is consensus on this, we can go thru
with this approach, Dan your comments?
Viresh KUMAR Aug. 4, 2011, 5:54 a.m. UTC | #8
On 08/04/2011 09:55 AM, Koul, Vinod wrote:
> Yes that's the whole point, today callback mechanism doesn't tell the
> _status_ of the transfer (which if we need change can be discussed as
> well), but to counter argue I have never been able to generate the error
> interrupt, are you able to do on your controller?

Yes, if we supply unaligned address, with access width as word, then it gives
error interrupt.

Regarding your point of updating callback for reporting errors,
I think it is required and should be a common issue.

> 
> One more point wrt this patch, what do we gain here from calling
> tasklet, are you cleaning up your queue for the error descriptor or
> something, I think not. so just calling to print doesn't make sense to
> me, comments?

Tasklet does following:
- If any other transfer request is queued up, then it is started
- else, it releases physical channel, by clearing it completely.

Finally it unmap buffers, frees txd, and call callback.

This probably is enough to end transfer for which user was waiting.
If user hasn't used any timeout mechanisms then he will never come
to know that an error occurred. So its better to call its callback,
to tell him, transfer has completed, successfully or unsuccessfully.
Russell King - ARM Linux Aug. 14, 2011, 8:29 a.m. UTC | #9
On Thu, Aug 04, 2011 at 11:02:42AM +0530, Koul, Vinod wrote:
> On Thu, 2011-08-04 at 11:24 +0530, viresh kumar wrote:
> > On 08/04/2011 09:55 AM, Koul, Vinod wrote:
> > > Yes that's the whole point, today callback mechanism doesn't tell the
> > > _status_ of the transfer (which if we need change can be discussed as
> > > well), but to counter argue I have never been able to generate the error
> > > interrupt, are you able to do on your controller?
> > 
> > Yes, if we supply unaligned address, with access width as word, then it gives
> > error interrupt.
> > 
> > Regarding your point of updating callback for reporting errors,
> > I think it is required and should be a common issue.
> Okay till now no one has asked for it. We can add to callback arguments
> either a bool to indicate what is the transfer status or usual error
> code to also indicate what type of failure.
> I would prefer latter, if there is consensus on this, we can go thru
> with this approach, Dan your comments?

If we are concerned about unaligned addresses, then that's something which
should be checked for at prepare time, to avoid failing DMA transfers.  A
failed DMA transfer is potentially a data loss situation, one that we want
to avoid.

Consider for instance a UART driver using DMA for TX/RX.  We want to know
when we prepare the RX DMA whether the DMA engine is going to be able to
perform the requested transfer.  We don't want to know when characters
start coming in, because not only would we have to undo the DMA setup,
but we'd also need to ensure that we start reading the characters via PIO
as quickly as possible to avoid overruns.

So, what I'd say is if you receive an error interrupt, either the prep
code isn't robust enough - it's like a BUG() telling us that something
needs fixing.  So, I think printing a warning and stalling makes sense -
and hopefully we get a bug report to investigate the causes.

Patch
diff mbox

diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
index c4ce1a2..b9137bc 100644
--- a/drivers/dma/amba-pl08x.c
+++ b/drivers/dma/amba-pl08x.c
@@ -1630,40 +1630,33 @@  static void pl08x_tasklet(unsigned long data)
 static irqreturn_t pl08x_irq(int irq, void *dev)
 {
 	struct pl08x_driver_data *pl08x = dev;
-	u32 mask = 0;
-	u32 val;
-	int i;
-
-	val = readl(pl08x->base + PL080_ERR_STATUS);
-	if (val) {
-		/* An error interrupt (on one or more channels) */
-		dev_err(&pl08x->adev->dev,
-			"%s error interrupt, register value 0x%08x\n",
-				__func__, val);
-		/*
-		 * Simply clear ALL PL08X error interrupts,
-		 * regardless of channel and cause
-		 * FIXME: should be 0x00000003 on PL081 really.
-		 */
-		writel(0x000000FF, pl08x->base + PL080_ERR_CLEAR);
+	u32 err, tc, i;
+
+	/* check & clear - ERR & TC interrupts */
+	err = readl(pl08x->base + PL080_ERR_STATUS);
+	if (err) {
+		dev_err(&pl08x->adev->dev, "%s error interrupt, register value"
+				"0x%08x\n", __func__, err);
+		writel(err, pl08x->base + PL080_ERR_CLEAR);
 	}
-	val = readl(pl08x->base + PL080_INT_STATUS);
-	for (i = 0; i < pl08x->vd->channels; i++) {
-		if ((1 << i) & val) {
+	tc = readl(pl08x->base + PL080_INT_STATUS);
+	if (tc)
+		writel(tc, pl08x->base + PL080_TC_CLEAR);
+
+	if (!err && !tc)
+		return IRQ_NONE;
+
+	for (i = 0; i < pl08x->vd->channels; i++)
+		if (((1 << i) & err) || ((1 << i) & tc)) {
 			/* Locate physical channel */
 			struct pl08x_phy_chan *phychan = &pl08x->phy_chans[i];
 			struct pl08x_dma_chan *plchan = phychan->serving;
 
 			/* Schedule tasklet on this channel */
 			tasklet_schedule(&plchan->tasklet);
-
-			mask |= (1 << i);
 		}
-	}
-	/* Clear only the terminal interrupts on channels we processed */
-	writel(mask, pl08x->base + PL080_TC_CLEAR);
 
-	return mask ? IRQ_HANDLED : IRQ_NONE;
+	return IRQ_HANDLED;
 }
 
 static void pl08x_dma_slave_init(struct pl08x_dma_chan *chan)