* [PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic @ 2014-12-25 4:41 Barry Song 2014-12-25 9:02 ` Lars-Peter Clausen 0 siblings, 1 reply; 6+ messages in thread From: Barry Song @ 2014-12-25 4:41 UTC (permalink / raw) To: peter.ujfalusi, lars, tiwai, broonie Cc: alsa-devel, workgroup.linux, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic 2014-12-25 4:41 [PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic Barry Song @ 2014-12-25 9:02 ` Lars-Peter Clausen 2014-12-25 9:08 ` Barry Song 0 siblings, 1 reply; 6+ messages in thread From: Lars-Peter Clausen @ 2014-12-25 9:02 UTC (permalink / raw) To: Barry Song, peter.ujfalusi, tiwai, broonie Cc: alsa-devel, workgroup.linux, Barry Song 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic 2014-12-25 9:02 ` Lars-Peter Clausen @ 2014-12-25 9:08 ` Barry Song 2014-12-25 9:35 ` Barry Song 2014-12-25 9:45 ` Lars-Peter Clausen 0 siblings, 2 replies; 6+ messages in thread From: Barry Song @ 2014-12-25 9:08 UTC (permalink / raw) To: Lars-Peter Clausen Cc: alsa-devel@alsa-project.org, Mark Brown, Takashi Iwai, DL-SHA-WorkGroupLinux, peter.ujfalusi, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic 2014-12-25 9:08 ` Barry Song @ 2014-12-25 9:35 ` Barry Song 2014-12-25 9:45 ` Lars-Peter Clausen 1 sibling, 0 replies; 6+ messages in thread From: Barry Song @ 2014-12-25 9:35 UTC (permalink / raw) To: Lars-Peter Clausen, Koul, Vinod Cc: alsa-devel@alsa-project.org, Mark Brown, Takashi Iwai, DL-SHA-WorkGroupLinux, peter.ujfalusi, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic 2014-12-25 9:08 ` Barry Song 2014-12-25 9:35 ` Barry Song @ 2014-12-25 9:45 ` Lars-Peter Clausen 2014-12-25 9:53 ` Barry Song 1 sibling, 1 reply; 6+ messages in thread From: Lars-Peter Clausen @ 2014-12-25 9:45 UTC (permalink / raw) To: Barry Song Cc: alsa-devel@alsa-project.org, Mark Brown, Takashi Iwai, DL-SHA-WorkGroupLinux, peter.ujfalusi, Barry Song 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic 2014-12-25 9:45 ` Lars-Peter Clausen @ 2014-12-25 9:53 ` Barry Song 0 siblings, 0 replies; 6+ messages in thread From: Barry Song @ 2014-12-25 9:53 UTC (permalink / raw) To: Lars-Peter Clausen Cc: alsa-devel@alsa-project.org, Mark Brown, Takashi Iwai, DL-SHA-WorkGroupLinux, peter.ujfalusi, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-12-25 9:53 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-12-25 4:41 [PATCH] ALSA: pcm_dmaengine: correct the error handler of dmaengine_prep_dma_cyclic Barry Song 2014-12-25 9:02 ` Lars-Peter Clausen 2014-12-25 9:08 ` Barry Song 2014-12-25 9:35 ` Barry Song 2014-12-25 9:45 ` Lars-Peter Clausen 2014-12-25 9:53 ` Barry Song
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.