[PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic

[PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic

[Barry Song]

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;
-- 
2.2.0

Re: [PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic

[Lars-Peter Clausen]

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

Re: [PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic

[Barry Song]

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

Re: [PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic

[Barry Song]

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

Re: [PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic

[Lars-Peter Clausen]

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

Re: [PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic

[Barry Song]

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