[PATCH] dmaengine: pl330: use lock of dma channel in pl330_update

[PATCH] dmaengine: pl330: use lock of dma channel in pl330_update

[Hsin-Yu Chao]

The content of pl330_thread could be modified in pl330_update
without protection by the lock from the dma channel who currently
holding this thread. This could cause bug to the calculation of
in pl330_tx_status, if the running request has just been done and
moving to next request while calculating the residual number, an
invalid number from BUSY descriptor could be added up.
---
 drivers/dma/pl330.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 31e9c49..2449cb7 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1564,6 +1564,19 @@ static void pl330_dotask(unsigned long data)
 	return;
 }
 
+static struct dma_pl330_chan *get_dma_channel(struct pl330_dmac *pl330,
+					      struct pl330_thread *thrd)
+{
+	struct dma_pl330_chan *pch;
+	int i;
+	for (i = 0; i < pl330->num_peripherals; i++) {
+		pch = &pl330->peripherals[i];
+		if (pch && pch->thread == thrd)
+			return pch;
+	}
+	return NULL;
+}
+
 /* Returns 1 if state was updated, 0 otherwise */
 static int pl330_update(struct pl330_dmac *pl330)
 {
@@ -1613,6 +1626,7 @@ static int pl330_update(struct pl330_dmac *pl330)
 	for (ev = 0; ev < pl330->pcfg.num_events; ev++) {
 		if (val & (1 << ev)) { /* Event occurred */
 			struct pl330_thread *thrd;
+			struct dma_pl330_chan *pch;
 			u32 inten = readl(regs + INTEN);
 			int active;
 
@@ -1625,6 +1639,9 @@ static int pl330_update(struct pl330_dmac *pl330)
 			id = pl330->events[ev];
 
 			thrd = &pl330->channels[id];
+			pch = get_dma_channel(pl330, thrd);
+			if (pch)
+				spin_lock_irqsave(&pch->lock, flags);
 
 			active = thrd->req_running;
 			if (active == -1) /* Aborted */
@@ -1638,6 +1655,8 @@ static int pl330_update(struct pl330_dmac *pl330)
 
 			/* Get going again ASAP */
 			_start(thrd);
+			if (pch)
+				spin_unlock_irqrestore(&pch->lock, flags);
 
 			/* For now, just make a list of callbacks to be done */
 			list_add_tail(&descdone->rqd, &pl330->req_done);
-- 
2.6.6

Re: [PATCH] dmaengine: pl330: use lock of dma channel in pl330_update

[Vinod Koul]

On Fri, Aug 19, 2016 at 09:03:48PM +0800, Hsin-Yu Chao wrote:
> The content of pl330_thread could be modified in pl330_update
> without protection by the lock from the dma channel who currently
> holding this thread. This could cause bug to the calculation of
> in pl330_tx_status, if the running request has just been done and
> moving to next request while calculating the residual number, an
> invalid number from BUSY descriptor could be added up.
> ---
>  drivers/dma/pl330.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 31e9c49..2449cb7 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -1564,6 +1564,19 @@ static void pl330_dotask(unsigned long data)
>  	return;
>  }
>  
> +static struct dma_pl330_chan *get_dma_channel(struct pl330_dmac *pl330,
> +					      struct pl330_thread *thrd)
> +{
> +	struct dma_pl330_chan *pch;
> +	int i;

empty line here please

-- 
~Vinod

Re: [PATCH] dmaengine: pl330: use lock of dma channel in pl330_update

[Hsin-yu Chao]

It turns out this patch could cause lock dependency issue.
The other patch "Acquire dmac's spinlock in pl330_tx_status" that I've
uploaded is the correct fix to replace this one.

Thanks,
Hsin-yu

On Fri, Sep 9, 2016 at 7:48 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Fri, Aug 19, 2016 at 09:03:48PM +0800, Hsin-Yu Chao wrote:
>> The content of pl330_thread could be modified in pl330_update
>> without protection by the lock from the dma channel who currently
>> holding this thread. This could cause bug to the calculation of
>> in pl330_tx_status, if the running request has just been done and
>> moving to next request while calculating the residual number, an
>> invalid number from BUSY descriptor could be added up.
>> ---
>>  drivers/dma/pl330.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index 31e9c49..2449cb7 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -1564,6 +1564,19 @@ static void pl330_dotask(unsigned long data)
>>       return;
>>  }
>>
>> +static struct dma_pl330_chan *get_dma_channel(struct pl330_dmac *pl330,
>> +                                           struct pl330_thread *thrd)
>> +{
>> +     struct dma_pl330_chan *pch;
>> +     int i;
>
> empty line here please
>
> --
> ~Vinod