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

[Doug Anderson]

Shawn,

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().
>> ...so 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
dma_request_slave_channel_reason().

* 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
alternative.

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

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

-Doug