Re: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER

From: Shawn Lin
Date: Mon Mar 21 2016 - 22:53:43 EST

+ Vinod

On 2016/3/22 10:33, Doug Anderson wrote:

On Mon, Mar 21, 2016 at 7:03 PM, Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> wrote:
...but, looking at this, presumably before landing any patch that made
dma_request_slave_channel() return -EPROBE_DEFER you'd need to modify
_all_ users of dma_request_slave_channel to handle error pointers
being returned. Right now dma_request_slave_channel() says it returns
a pointer to a channel or NULL and the function explicitly avoids
returning any errors. That might be possible, but it's a big

At first glance, it's a big change, but maybe not really.
Almost all of them use the templet like:
ch = dma_request_slave_channel
if (!ch)

It's same for all the non-null return pointer/non-zero value ?

So from my view, we can safely change dma_request_slave_channel,
and leave the caller here. I presumably the respective
drivers will graduately migrate to check the return value with
EPROBE_DEFER if they do care this issue. Otherwise, we believe
they don't suffer the changes we make, just as what they did in the
past. Does that make sense?

...but if you return ERR_PTR(-EPROBE_DEFER) and don't change existing
callers, then existing callers will think you've returned a valid
pointer when you really returned an error pointer. They'll pass this
error pointer around like it's a valid "struct dma_chan", won't then?

possibly, it depends on how caller deal with it. Should check it case by
case for each caller.

Actually, could your code just call
dma_request_slave_channel_reason(). Oh, looks like that's exactly
what you want. See commit 0ad7c00057dc ("dma: add channel request API
that supports deferred probe"). Oh, but I'm looking at 4.4. Looking
at linuxnext, it looks like this got renamed to dma_request_chan(). you need to use that, no?

Strange, but on 4.4 there was some extra code in
dma_request_slave_channel() that wasn't in
dma_request_slave_channel_reason(). ...but looks like that all got
cleaned up in the same CL that added the new name.

dma_request_chan already return ERR_PTR(-EPROBE_DEFER), but
dma_request_slave_channel ignore this and rewrite it to be NULL.
Strange behaviour looks to me. commit 0ad7c00057dc ("dma: add channel
request API that supports deferred probe") did the right thing, but
what happened then? It was drop for some reasons?

Hello Vinod,

Could you please elaborate some more infomation to commit 0ad7c00057dc
("dma: add channel request API that supports deferred probe") :) ?


Best Regards
Shawn Lin