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

[Vladimir Zapolskiy]

Hi Doug,

On 22.03.2016 05:33, Doug Anderson wrote:
> 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;
> +       }
> 

referencing my answer to v2 for clarity here is my version:

-       rs->dma_tx.ch = dma_request_slave_channel(rs->dev, "tx");
-       if (IS_ERR_OR_NULL(rs->dma_tx.ch)) {
+       rs->dma_tx.ch = dma_request_chan(rs->dev, "tx");
+       if (IS_ERR(rs->dma_tx.ch)) {
                /* Check tx to see if we need defer probing driver */
                if (PTR_ERR(rs->dma_tx.ch) == -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;
        }


You may also add some tweaks like checking for IS_ERR(rs->dma_tx.ch) in the
following code instead of checking for NULL (then you don't need to do
"rs->dma_tx.ch = NULL" on error), then skip "rx" channel request, if "tx"
channel request failed and so on.

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

--
With best wishes,
Vladimir