[alsa-devel] [PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic
From: Barry Song Baohua.Song@csr.com
preparing cyclic DMA description can fail either due to DMA desc list is full(-ENOMEM), or due to the coming DMA configuration is illegal or not supported by the acting DMA hardware(other ERR codes).
Signed-off-by: Barry Song Baohua.Song@csr.com --- sound/core/pcm_dmaengine.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c index 6542c40..5cac7e4 100644 --- a/sound/core/pcm_dmaengine.c +++ b/sound/core/pcm_dmaengine.c @@ -163,6 +163,8 @@ static int dmaengine_pcm_prepare_and_submit(struct snd_pcm_substream *substream)
if (!desc) return -ENOMEM; + if (IS_ERR(desc)) + return PTR_ERR(desc);
desc->callback = dmaengine_pcm_dma_complete; desc->callback_param = substream;
On 12/25/2014 05:41 AM, Barry Song wrote:
From: Barry Song Baohua.Song@csr.com
preparing cyclic DMA description can fail either due to DMA desc list is full(-ENOMEM), or due to the coming DMA configuration is illegal or not supported by the acting DMA hardware(other ERR codes).
According to the API definition this returns either NULL or a valid descriptor, so the behavior in pcm_dmaengine is correct. Maybe your particular dmaengine driver as a incorrect implementation.
- Lars
2014-12-25 17:02 GMT+08:00 Lars-Peter Clausen lars@metafoo.de:
On 12/25/2014 05:41 AM, Barry Song wrote:
From: Barry Song Baohua.Song@csr.com
preparing cyclic DMA description can fail either due to DMA desc list is full(-ENOMEM), or due to the coming DMA configuration is illegal or not supported by the acting DMA hardware(other ERR codes).
According to the API definition this returns either NULL or a valid descriptor, so the behavior in pcm_dmaengine is correct. Maybe your particular dmaengine driver as a incorrect implementation.
i think it is something wrong. an functions returns pointer should return one of (1) right pointer (2) NULL (3) a wrong pointer from error CODES
if for 2&3, we both return NULL, it actually means we are taking something wrong.
this should be a generic rule as from clk and other subsystems. why does DMA want to do something special?
- Lars
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
-barry
2014-12-25 17:08 GMT+08:00 Barry Song 21cnbao@gmail.com:
2014-12-25 17:02 GMT+08:00 Lars-Peter Clausen lars@metafoo.de:
On 12/25/2014 05:41 AM, Barry Song wrote:
From: Barry Song Baohua.Song@csr.com
preparing cyclic DMA description can fail either due to DMA desc list is full(-ENOMEM), or due to the coming DMA configuration is illegal or not supported by the acting DMA hardware(other ERR codes).
According to the API definition this returns either NULL or a valid descriptor, so the behavior in pcm_dmaengine is correct. Maybe your particular dmaengine driver as a incorrect implementation.
i think it is something wrong. an functions returns pointer should return one of (1) right pointer (2) NULL (3) a wrong pointer from error CODES
if for 2&3, we both return NULL, it actually means we are taking something wrong.
this should be a generic rule as from clk and other subsystems. why does DMA want to do something special?
+ Vinod, i need your input for this, we rejected an unsupported DMA configuration by returning PTR_ERR, and we returned NULL if there is no free desc. this is clearly two different scenarios and reasons for failed dma_prep_cyclic(). i think this should be visible to the API users. this is a necessary information not a blackbox to the API users.
static struct dma_async_tx_descriptor * sirfsoc_dma_prep_cyclic(struct dma_chan *chan, dma_addr_t addr, size_t buf_len, size_t period_len, enum dma_transfer_direction direction, unsigned long flags) { struct sirfsoc_dma_chan *schan = dma_chan_to_sirfsoc_dma_chan(chan); struct sirfsoc_dma_desc *sdesc = NULL; unsigned long iflags;
/* * we only support cycle transfer with 2 period * If the X-length is set to 0, it would be the loop mode. * The DMA address keeps increasing until reaching the end of a loop * area whose size is defined by (DMA_WIDTH x (Y_LENGTH + 1)). Then * the DMA address goes back to the beginning of this area. * In loop mode, the DMA data region is divided into two parts, BUFA * and BUFB. DMA controller generates interrupts twice in each loop: * when the DMA address reaches the end of BUFA or the end of the * BUFB */ if (buf_len != 2 * period_len) return ERR_PTR(-EINVAL);
/* Get free descriptor */ spin_lock_irqsave(&schan->lock, iflags); if (!list_empty(&schan->free)) { sdesc = list_first_entry(&schan->free, struct sirfsoc_dma_desc, node); list_del(&sdesc->node); } spin_unlock_irqrestore(&schan->lock, iflags);
if (!sdesc) return NULL;
...
return &sdesc->desc; }
do you think we need to refine the API definition or do we need to change the driver?
-barry
On 12/25/2014 10:08 AM, Barry Song wrote:
2014-12-25 17:02 GMT+08:00 Lars-Peter Clausen lars@metafoo.de:
On 12/25/2014 05:41 AM, Barry Song wrote:
From: Barry Song Baohua.Song@csr.com
preparing cyclic DMA description can fail either due to DMA desc list is full(-ENOMEM), or due to the coming DMA configuration is illegal or not supported by the acting DMA hardware(other ERR codes).
According to the API definition this returns either NULL or a valid descriptor, so the behavior in pcm_dmaengine is correct. Maybe your particular dmaengine driver as a incorrect implementation.
i think it is something wrong. an functions returns pointer should return one of (1) right pointer (2) NULL (3) a wrong pointer from error CODES
if for 2&3, we both return NULL, it actually means we are taking something wrong.
this should be a generic rule as from clk and other subsystems. why does DMA want to do something special?
I think the dmaengine API simply predates the widespread use of ERR_PTR.
And it probably makes sense to update it to use ERR_PTR to have more meaningful error codes. But this needs to be done in a way that makes sure that all providers and all consumers agree on the same semantics. E.g. first update all consumers to handle ERR_PTR or NULL, then update all providers to return a ERR_PTR instead of NULL, then update all consumers to just handle ERR_PTR.
- Lars
2014-12-25 17:45 GMT+08:00 Lars-Peter Clausen lars@metafoo.de:
On 12/25/2014 10:08 AM, Barry Song wrote:
2014-12-25 17:02 GMT+08:00 Lars-Peter Clausen lars@metafoo.de:
On 12/25/2014 05:41 AM, Barry Song wrote:
From: Barry Song Baohua.Song@csr.com
preparing cyclic DMA description can fail either due to DMA desc list is full(-ENOMEM), or due to the coming DMA configuration is illegal or not supported by the acting DMA hardware(other ERR codes).
According to the API definition this returns either NULL or a valid descriptor, so the behavior in pcm_dmaengine is correct. Maybe your particular dmaengine driver as a incorrect implementation.
i think it is something wrong. an functions returns pointer should return one of (1) right pointer (2) NULL (3) a wrong pointer from error CODES
if for 2&3, we both return NULL, it actually means we are taking something wrong.
this should be a generic rule as from clk and other subsystems. why does DMA want to do something special?
I think the dmaengine API simply predates the widespread use of ERR_PTR.
And it probably makes sense to update it to use ERR_PTR to have more meaningful error codes. But this needs to be done in a way that makes sure that all providers and all consumers agree on the same semantics. E.g. first update all consumers to handle ERR_PTR or NULL, then update all providers to return a ERR_PTR instead of NULL, then update all consumers to just handle ERR_PTR.
i agree. API should be small but can't be smaller. but for this case, this api has been too small and has not reported enough information to users. this lost information is preparing_dma can fill due to all kinds of hardware or software reasons. users should not be kept in the dark for these .
- Lars
-barry
participants (2)
-
Barry Song -
Lars-Peter Clausen