(Optional?) DMA vs. PIO
Hi!
During internal review of one patch I have been puzzled with the following code and Pierre suggested to ask mailing list for help.
My main concern is what was the idea behind? Does it mean we support optional DMA in such case? If now, why not to return an error code directly?
---8<---8<---8<---
Why ASoC core has the following code in the first place:
387 chan = dma_request_chan(dev, name); 388 if (IS_ERR(chan)) { 389 if (PTR_ERR(chan) == -EPROBE_DEFER) 390 return -EPROBE_DEFER; 391 pcm->chan[i] = NULL; 392 } else { 393 pcm->chan[i] = chan; 394 }
(note lines 389-391). If PIO fallback is not okay, why not to return an error there?
no idea, the code has been this way since 2013 (5eda87b890f867b098e5566b5543642851e8b9c3)
It's worth asking the question on the mailing list, I don't know if this is a bug or a feature.
Andy Shevchenko wrote at Thursday, October 8, 2020 9:06 AM:
During internal review of one patch I have been puzzled with the following code and Pierre suggested to ask mailing list for help.
My main concern is what was the idea behind? Does it mean we support optional DMA in such case? If now, why not to return an error code directly?
---8<---8<---8<---
Why ASoC core has the following code in the first place:
387 chan = dma_request_chan(dev, name); 388 if (IS_ERR(chan)) { 389 if (PTR_ERR(chan) == -EPROBE_DEFER) 390 return -EPROBE_DEFER; 391 pcm->chan[i] = NULL; 392 } else { 393 pcm->chan[i] = chan; 394 }
(note lines 389-391). If PIO fallback is not okay, why not to return an error there?
no idea, the code has been this way since 2013 (5eda87b890f867b098e5566b5543642851e8b9c3)
It's been there a bit longer, in fact since the file was created:
commit 28c4468b00a1e55e08cc20117de968f7c6275441 Author: Lars-Peter Clausen lars@metafoo.de Date: Mon Apr 15 19:19:50 2013 +0200
ASoC: Add a generic dmaengine_pcm driver
+ pcm->chan[i] = of_dma_request_slave_channel(dev->of_node, + dmaengine_pcm_dma_channel_names[i]);
The commit you mentioned above simply prevents the code from taking the "no DMA available" path if deferred probe occurs.
It's worth asking the question on the mailing list, I don't know if this is a bug or a feature.
This does seem like a bug, but there are a few places in the code which explicitly check for a NULL chan or dma_dev (derived from chan) object, so it seems deliberate.
On 10/8/20 5:05 PM, Andy Shevchenko wrote:
Hi!
During internal review of one patch I have been puzzled with the following code and Pierre suggested to ask mailing list for help.
My main concern is what was the idea behind? Does it mean we support optional DMA in such case? If now, why not to return an error code directly?
---8<---8<---8<---
Why ASoC core has the following code in the first place:
387 chan = dma_request_chan(dev, name); 388 if (IS_ERR(chan)) { 389 if (PTR_ERR(chan) == -EPROBE_DEFER) 390 return -EPROBE_DEFER; 391 pcm->chan[i] = NULL; 392 } else { 393 pcm->chan[i] = chan; 394 }
(note lines 389-391). If PIO fallback is not okay, why not to return an error there?
no idea, the code has been this way since 2013 (5eda87b890f867b098e5566b5543642851e8b9c3)
It's worth asking the question on the mailing list, I don't know if this is a bug or a feature.
This is to handle the case where a only TX or only RX is supported. In that case it is not an error if only one DMA channel is specified.
On Thu, Oct 08, 2020 at 06:05:39PM +0300, Andy Shevchenko wrote:
My main concern is what was the idea behind? Does it mean we support optional DMA in such case? If now, why not to return an error code directly?
...
no idea, the code has been this way since 2013 (5eda87b890f867b098e5566b5543642851e8b9c3)
That's "ASoC: dmaengine: support deferred probe for DMA channels" for those playing at home. It's been that way since before then since previously we ignored errors entirely.
It's worth asking the question on the mailing list, I don't know if this is a bug or a feature.
I'm fairly sure it's intentional for systems with limited DMA channels available but ICBW, it's obviously been quite some time. In retrospect a comment explaining the decision would have been a good idea.
participants (4)
-
Andy Shevchenko
-
Lars-Peter Clausen
-
Mark Brown
-
Stephen Warren