dmaengine: pl330: Fix some race conditions in residue calculation

The residue calculation in pl330_tx_status doesn't handle transitional states that occur at the time one descriptor (A) is completed and the next (B) is started. Specifically, both A and B can simultaneously be in the BUSY state and at this time the thread's 'req_running' may (or may not) be -1.

To cope with this situation we change the code to ensure A is treated as complete and B as having not yet started. Prior to the change, the code would calculate a transferred byte count as if both A and B had completed.

Fixes aee4d1fac887 ("dmaengine: pl330: improve pl330_tx_status() function")

Signed-off-by Jon Medhurst <[email protected]>

---

I discovered this issue when trying to work out why audio stopped working on ARM's Juno platform and bisected it to commit aee4d1fac887. Whilst this patch seems to fix the problems I was seeing, I can't help but think there are more race conditions with this code. E.g. if the running descriptor changes under us, pl330_get_current_xferred_count can end up reading values from hardware that relate to a different descriptor. And if we're really unlucky, the reading of the 'val' and 'addr' values in pl330_get_current_xferred_count can come from different descriptors. I don't know if there is any locks we can use to prevent such races or if we need to try and detect when things have changed and redo/abort the residue calculation...

 drivers/dma/pl330.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

drivers/dma/pl330.c +20 -4

index 17ee758..55e3c5f 100644
@@ -2240,6 +2240,7 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 	struct dma_pl330_desc *desc, *running = NULL;
 	struct dma_pl330_chan *pch = to_pchan(chan);
 	unsigned int transferred, residual = 0;
+	bool first_busy;
 
 	ret = dma_cookie_status(chan, cookie, txstate);
 

@@ -2253,16 +2254,31 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 
 	if (pch->thread->req_running != -1)
 		running = pch->thread->req[pch->thread->req_running].desc;
+	first_busy = true;
 
 	/* Check in pending list */
 	list_for_each_entry(desc, &pch->work_list, node) {
 		if (desc->status == DONE)
 			transferred = desc->bytes_requested;
-		else if (running && desc == running)
-			transferred =
-				pl330_get_current_xferred_count(pch, desc);
-		else
+		else if (desc->status == BUSY && first_busy) {
+			first_busy = false;
+			if (running && desc == running) {
+				transferred =
+					pl330_get_current_xferred_count(pch, desc);
+			} else {
+				/* BUSY but not running means it's just completed */
+				transferred = desc->bytes_requested;
+			}
+		} else {
+			/*
+			 * Descriptor is either in PREP state queued for future
+			 * transfer or it is the second BUSY descriptor we have
+			 * seen. The latter case means it has just, or is about
+			 * to be, started, so treat it as having not yet
+			 * transferred any bytes, the same as PREP.
+			 */
 			transferred = 0;
+		}
 		residual += desc->bytes_requested - transferred;
 		if (desc->txd.cookie == cookie) {
 			switch (desc->status) {

Signature · 1 line

2.1.4

Quoted 5 lines · Jon Medhurst (Tixy)

The residue calculation in pl330_tx_status doesn't handle transitional states that occur at the time one descriptor (A) is completed and the next (B) is started. Specifically, both A and B can simultaneously be in the BUSY state and at this time the thread's 'req_running' may (or may not) be -1.

you are under lock so descriptor state wont be update while we are it.

Also the query for residue is for "a descriptor" not whatever is the current running descriptor...

Quoted 1 line

Quoted 4 lines

To cope with this situation we change the code to ensure A is treated as complete and B as having not yet started. Prior to the change, the code would calculate a transferred byte count as if both A and B had completed.

You query for either A or B not both!

Quoted 74 lines

Fixes aee4d1fac887 ("dmaengine: pl330: improve pl330_tx_status() function")

Signed-off-by Jon Medhurst <[email protected]>

---

I discovered this issue when trying to work out why audio stopped working on ARM's Juno platform and bisected it to commit aee4d1fac887. Whilst this patch seems to fix the problems I was seeing, I can't help but think there are more race conditions with this code. E.g. if the running descriptor changes under us, pl330_get_current_xferred_count can end up reading values from hardware that relate to a different descriptor. And if we're really unlucky, the reading of the 'val' and 'addr' values in pl330_get_current_xferred_count can come from different descriptors. I don't know if there is any locks we can use to prevent such races or if we need to try and detect when things have changed and redo/abort the residue calculation...

 drivers/dma/pl330.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

drivers/dma/pl330.c +20 -4

index 17ee758..55e3c5f 100644
@@ -2240,6 +2240,7 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 	struct dma_pl330_desc *desc, *running = NULL;
 	struct dma_pl330_chan *pch = to_pchan(chan);
 	unsigned int transferred, residual = 0;
+	bool first_busy;
 
 	ret = dma_cookie_status(chan, cookie, txstate);
 

@@ -2253,16 +2254,31 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 
 	if (pch->thread->req_running != -1)
 		running = pch->thread->req[pch->thread->req_running].desc;
+	first_busy = true;
 
 	/* Check in pending list */
 	list_for_each_entry(desc, &pch->work_list, node) {
 		if (desc->status == DONE)
 			transferred = desc->bytes_requested;
-		else if (running && desc == running)
-			transferred =
-				pl330_get_current_xferred_count(pch, desc);
-		else
+		else if (desc->status == BUSY && first_busy) {
+			first_busy = false;
+			if (running && desc == running) {
+				transferred =
+					pl330_get_current_xferred_count(pch, desc);
+			} else {
+				/* BUSY but not running means it's just completed */
+				transferred = desc->bytes_requested;
+			}
+		} else {
+			/*
+			 * Descriptor is either in PREP state queued for future
+			 * transfer or it is the second BUSY descriptor we have
+			 * seen. The latter case means it has just, or is about
+			 * to be, started, so treat it as having not yet
+			 * transferred any bytes, the same as PREP.
+			 */
 			transferred = 0;
+		}
 		residual += desc->bytes_requested - transferred;
 		if (desc->txd.cookie == cookie) {
 			switch (desc->status) {

Signature · 1 line

2.1.4

Quoted 18 lines · Vinod Koul

Quoted 5 lines · Jon Medhurst (Tixy)

The residue calculation in pl330_tx_status doesn't handle transitional states that occur at the time one descriptor (A) is completed and the next (B) is started. Specifically, both A and B can simultaneously be in the BUSY state and at this time the thread's 'req_running' may (or may not) be -1.

you are under lock so descriptor state wont be update while we are it.

Also the query for residue is for "a descriptor" not whatever is the current running descriptor...

Quoted 5 lines

To cope with this situation we change the code to ensure A is treated as complete and B as having not yet started. Prior to the change, the code would calculate a transferred byte count as if both A and B had completed.

Quoted 1 line

You query for either A or B not both!

I've probably been using wrong/ambiguous terminology...

In my description I'm using 'descriptor' to refer to a 'struct dma_pl330_desc', I guess other people assume 'struct dma_async_tx_descriptor'?

The situation I was debugging was audio playback, where ASoC ends up calling pl330_prep_dma_cyclic() with a period one quarter the length of the buffer it is using, so that results in four dma_pl330_desc 'descriptors' being created to cover that buffer. These later get submitted to a DMA channel (struct dma_pl330_chan) which has a list of these that it is processing (the 'work_list').

The residual calculation that currently exists in pl08x_dma_tx_status() is iterating this work_list and summing the length of currently transferring 'descriptor' with those later pending ones. I believe that is correct behaviour because these 'descriptors' (dma_pl330_desc) are all internal implementation details of the driver, and the dmaengine API's are dealing in units of 'dma_async_tx_descriptor' ?

If the current code is OK in this regard, it is definitely buggy because it doesn't cope with the situation when two dma_pl330_desc's are in the state 'BUSY' a, which I have seen occur when debugging this issue, had worked out can happen by analysing the code, and is acknowledged by the in-source comments for enum desc_status...

/*
* Sitting on the work_list and already submitted
* to the PL330 core. Not more than two descriptors
* of a channel can be BUSY at any time. */
BUSY,

In my problematic usecase I have userside code calling ALSA ioctls to poll the current audio playback position which results in pl08x_dma_tx_status() being called multiple times a second. After only a second or two the buggy situation gets hit, resulting in a miscalculation that ASoC interprets as a buffer underflow and so it stops the stream.

I spent several days debugging this, with enough ad hoc tests and printk's littered everywhere to be very confident as to how things are going wrong - what I'm not not totally confident of is how things should be properly fixed.

This patch appears to fix the situation that I was hitting, but it really looks like there isn't any locking that prevent this polling use of pl08x_dma_tx_status() from happening concurrently with the irq handler reprogramming the hardware for the next dma_pl330_desc. I didn't attempt any fix for that for fear of introducing bugs in what looks like complex code, and because it's not a problem I saw happen in practice.

-- Tixy

Quoted 75 lines

Quoted 74 lines

Fixes aee4d1fac887 ("dmaengine: pl330: improve pl330_tx_status() function")

Signed-off-by Jon Medhurst <[email protected]>

---

I discovered this issue when trying to work out why audio stopped working on ARM's Juno platform and bisected it to commit aee4d1fac887. Whilst this patch seems to fix the problems I was seeing, I can't help but think there are more race conditions with this code. E.g. if the running descriptor changes under us, pl330_get_current_xferred_count can end up reading values from hardware that relate to a different descriptor. And if we're really unlucky, the reading of the 'val' and 'addr' values in pl330_get_current_xferred_count can come from different descriptors. I don't know if there is any locks we can use to prevent such races or if we need to try and detect when things have changed and redo/abort the residue calculation...

 drivers/dma/pl330.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

drivers/dma/pl330.c +20 -4

index 17ee758..55e3c5f 100644
@@ -2240,6 +2240,7 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 	struct dma_pl330_desc *desc, *running = NULL;
 	struct dma_pl330_chan *pch = to_pchan(chan);
 	unsigned int transferred, residual = 0;
+	bool first_busy;
 
 	ret = dma_cookie_status(chan, cookie, txstate);
 

@@ -2253,16 +2254,31 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 
 	if (pch->thread->req_running != -1)
 		running = pch->thread->req[pch->thread->req_running].desc;
+	first_busy = true;
 
 	/* Check in pending list */
 	list_for_each_entry(desc, &pch->work_list, node) {
 		if (desc->status == DONE)
 			transferred = desc->bytes_requested;
-		else if (running && desc == running)
-			transferred =
-				pl330_get_current_xferred_count(pch, desc);
-		else
+		else if (desc->status == BUSY && first_busy) {
+			first_busy = false;
+			if (running && desc == running) {
+				transferred =
+					pl330_get_current_xferred_count(pch, desc);
+			} else {
+				/* BUSY but not running means it's just completed */
+				transferred = desc->bytes_requested;
+			}
+		} else {
+			/*
+			 * Descriptor is either in PREP state queued for future
+			 * transfer or it is the second BUSY descriptor we have
+			 * seen. The latter case means it has just, or is about
+			 * to be, started, so treat it as having not yet
+			 * transferred any bytes, the same as PREP.
+			 */
 			transferred = 0;
+		}
 		residual += desc->bytes_requested - transferred;
 		if (desc->txd.cookie == cookie) {
 			switch (desc->status) {

Signature · 1 line

2.1.4

Quoted 19 lines · Jon Medhurst (Tixy)

Quoted 18 lines · Vinod Koul

Quoted 5 lines · Jon Medhurst (Tixy)

The residue calculation in pl330_tx_status doesn't handle transitional states that occur at the time one descriptor (A) is completed and the next (B) is started. Specifically, both A and B can simultaneously be in the BUSY state and at this time the thread's 'req_running' may (or may not) be -1.

you are under lock so descriptor state wont be update while we are it.

Also the query for residue is for "a descriptor" not whatever is the current running descriptor...

Quoted 5 lines

To cope with this situation we change the code to ensure A is treated as complete and B as having not yet started. Prior to the change, the code would calculate a transferred byte count as if both A and B had completed.

Quoted 7 lines

Quoted 1 line

You query for either A or B not both!

I've probably been using wrong/ambiguous terminology...

In my description I'm using 'descriptor' to refer to a 'struct dma_pl330_desc', I guess other people assume 'struct dma_async_tx_descriptor'?

Nope

Quoted 7 lines

The situation I was debugging was audio playback, where ASoC ends up calling pl330_prep_dma_cyclic() with a period one quarter the length of the buffer it is using, so that results in four dma_pl330_desc 'descriptors' being created to cover that buffer. These later get submitted to a DMA channel (struct dma_pl330_chan) which has a list of these that it is processing (the 'work_list').

Quoted 6 lines

The residual calculation that currently exists in pl08x_dma_tx_status() is iterating this work_list and summing the length of currently transferring 'descriptor' with those later pending ones. I believe that is correct behaviour because these 'descriptors' (dma_pl330_desc) are all internal implementation details of the driver, and the dmaengine API's are dealing in units of 'dma_async_tx_descriptor' ?

Not really. If you look closely dma_pl330_desc contains dma_async_tx_descriptor. A descriptor represents a tranasaction and is certainly not internal detail of your driver

The reside is requested for "a descriptor". For example if you have prepared two descriptors A and B and submitted them, then you can request status and reside for A and you need to calculate that for A only and not take into account status of B

Quoted 22 lines

If the current code is OK in this regard, it is definitely buggy because it doesn't cope with the situation when two dma_pl330_desc's are in the state 'BUSY' a, which I have seen occur when debugging this issue, had worked out can happen by analysing the code, and is acknowledged by the in-source comments for enum desc_status...

/*
* Sitting on the work_list and already submitted
* to the PL330 core. Not more than two descriptors
* of a channel can be BUSY at any time. */
BUSY,

In my problematic usecase I have userside code calling ALSA ioctls to poll the current audio playback position which results in pl08x_dma_tx_status() being called multiple times a second. After only a second or two the buggy situation gets hit, resulting in a miscalculation that ASoC interprets as a buffer underflow and so it stops the stream.

Quoted 4 lines

I spent several days debugging this, with enough ad hoc tests and printk's littered everywhere to be very confident as to how things are going wrong - what I'm not not totally confident of is how things should be properly fixed.

There maybe a problem but this patch and the fix dont explain that

Quoted 87 lines

This patch appears to fix the situation that I was hitting, but it really looks like there isn't any locking that prevent this polling use of pl08x_dma_tx_status() from happening concurrently with the irq handler reprogramming the hardware for the next dma_pl330_desc. I didn't attempt any fix for that for fear of introducing bugs in what looks like complex code, and because it's not a problem I saw happen in practice.

-- Tixy

Quoted 75 lines

Quoted 74 lines

Fixes aee4d1fac887 ("dmaengine: pl330: improve pl330_tx_status() function")

Signed-off-by Jon Medhurst <[email protected]>

---

I discovered this issue when trying to work out why audio stopped working on ARM's Juno platform and bisected it to commit aee4d1fac887. Whilst this patch seems to fix the problems I was seeing, I can't help but think there are more race conditions with this code. E.g. if the running descriptor changes under us, pl330_get_current_xferred_count can end up reading values from hardware that relate to a different descriptor. And if we're really unlucky, the reading of the 'val' and 'addr' values in pl330_get_current_xferred_count can come from different descriptors. I don't know if there is any locks we can use to prevent such races or if we need to try and detect when things have changed and redo/abort the residue calculation...

 drivers/dma/pl330.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

drivers/dma/pl330.c +20 -4

index 17ee758..55e3c5f 100644
@@ -2240,6 +2240,7 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 	struct dma_pl330_desc *desc, *running = NULL;
 	struct dma_pl330_chan *pch = to_pchan(chan);
 	unsigned int transferred, residual = 0;
+	bool first_busy;
 
 	ret = dma_cookie_status(chan, cookie, txstate);
 

@@ -2253,16 +2254,31 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 
 	if (pch->thread->req_running != -1)
 		running = pch->thread->req[pch->thread->req_running].desc;
+	first_busy = true;
 
 	/* Check in pending list */
 	list_for_each_entry(desc, &pch->work_list, node) {
 		if (desc->status == DONE)
 			transferred = desc->bytes_requested;
-		else if (running && desc == running)
-			transferred =
-				pl330_get_current_xferred_count(pch, desc);
-		else
+		else if (desc->status == BUSY && first_busy) {
+			first_busy = false;
+			if (running && desc == running) {
+				transferred =
+					pl330_get_current_xferred_count(pch, desc);
+			} else {
+				/* BUSY but not running means it's just completed */
+				transferred = desc->bytes_requested;
+			}
+		} else {
+			/*
+			 * Descriptor is either in PREP state queued for future
+			 * transfer or it is the second BUSY descriptor we have
+			 * seen. The latter case means it has just, or is about
+			 * to be, started, so treat it as having not yet
+			 * transferred any bytes, the same as PREP.
+			 */
 			transferred = 0;
+		}
 		residual += desc->bytes_requested - transferred;
 		if (desc->txd.cookie == cookie) {
 			switch (desc->status) {

Signature · 1 line

2.1.4

Quoted 4 lines

The reside is requested for "a descriptor". For example if you have prepared two descriptors A and B and submitted them, then you can request status and reside for A and you need to calculate that for A only and not take into account status of B

But, in the case of the pl330 driver, A and B may each consist of multiple internal/hidden descriptors. So the residue calculation has to sum up the residue of all these internal/hidden descriptors as well. This is what the current pl330_tx_status() function does, but has bugs.

I've only just managed to clearly understand all the above details whilst writing this email, and this confusion obviously means the code and any commit messages need to explain things better.

Quoted 10 lines · Jon Medhurst (Tixy)

Quoted 4 lines

The reside is requested for "a descriptor". For example if you have prepared two descriptors A and B and submitted them, then you can request status and reside for A and you need to calculate that for A only and not take into account status of B

But, in the case of the pl330 driver, A and B may each consist of multiple internal/hidden descriptors. So the residue calculation has to sum up the residue of all these internal/hidden descriptors as well. This is what the current pl330_tx_status() function does, but has bugs.

Quoted 3 lines

I've only just managed to clearly understand all the above details whilst writing this email, and this confusion obviously means the code and any commit messages need to explain things better.

Okay I relooked at the code, the driver is creating descriptor per period, so yes while calculating that should be taken into account but only for cyclic case and not for rest, as it will break reside values for for them.