lkml.org 
[lkml]   [2016]   [Mar]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 3/3] spi: rockchip: check requesting dma channel with EPROBE_DEFER
From
Date
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@rock-chips.com> 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@rock-chips.com>
>>> 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

\
 
 \ /
  Last update: 2016-03-22 15:01    [W:0.062 / U:0.600 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site