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

From: Doug Anderson
Date: Mon Mar 21 2016 - 23:33:31 EST


On Mon, Mar 21, 2016 at 7:53 PM, Shawn Lin <shawn.lin@xxxxxxxxxxxxxx> wrote:
> + Vinod
> On 2016/3/22 10:33, Doug Anderson wrote:
>> Shawn,
>> 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
>>>> change...
>>> 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)
>>> balabala....
>>> 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") :) ?

I think it's relatively straightforward.

The scheme they came up with allows them to more easily update one
client at a time. AKA:

* If your code has been updated to handle ERR_PTR() returns, you call

* If your code hasn't been updated, it will still call
dma_request_slave_channel(). In this case EPROBE_DEFER is treated
like any other failure. That's not ideal but better than the

* In recent kernels dma_request_slave_channel() was renamed to
dma_request_chan(). Old code can still use
dma_request_slave_channel_reason() but presumably they want you to use
dma_request_chan() for new code. They are equivalent:

> #define dma_request_slave_channel_reason(dev, name) dma_request_chan(dev, name)

So your patch should be:

- rs-> = dma_request_slave_channel(rs->dev, "tx");
- if (!rs->
+ rs-> = dma_request_slave_chan(rs->dev, "tx");
+ if (IS_ERR_OR_NULL(rs-> {
+ /* Check tx to see if we need defer probing driver */
+ if (rs-> == ERR_PTR(-EPROBE_DEFER)) {
+ ret = -EPROBE_DEFER;
+ goto err_get_fifo_len;
+ }
dev_warn(rs->dev, "Failed to request TX DMA channel\n");
+ rs-> = NULL;
+ }

...and then a similar patch for the "rx" side of things.