[PATCH 11/31] dma: add channel request API that supports deferred probe

[Dan Williams]

On Fri, Nov 22, 2013 at 9:34 AM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 11/21/2013 11:54 PM, Dan Williams wrote:
>> On Thu, Nov 21, 2013 at 10:22 AM, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>> On 11/20/2013 08:22 PM, Dan Williams wrote:
>>>> On Wed, Nov 20, 2013 at 1:24 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>>> On 11/20/2013 01:23 PM, Williams, Dan J wrote:
>>>>> ...
>>>>>> Why do the drivers that call dma_request_channel need to convert it to
>>>>>> an ERR value?  i.e. what's problematic about the below (not compile
>>>>>> tested)?
>>>>> ...
>>>>>> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
>>>>> ...
>>>>>> @@ -22,16 +22,20 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch,
>>>>>>                               struct samsung_dma_req *param,
>>>>>>                               struct device *dev, char *ch_name)
>>>>> ...
>>>>>> +     if (dev->of_node) {
>>>>>> +             chan = dma_request_slave_channel(dev, ch_name);
>>>>>> +             return IS_ERR(chan) ? (unsigned) NULL : (unsigned) chan;
>>>>>> +     } else {
>>>>>>               return (unsigned)dma_request_channel(mask, pl330_filter,
>>>>>>                                                       (void *)dma_ch);
>>>>>> +     }
>>>>>
>>>>> The argument is that if a function returns errors encoded as an ERR
>>>>> pointer, then callers must assume that any non-IS_ERR value that the
>>>>> function returns is valid. NULL is one of those values. As such, callers
>>>>> can no longer check the value against NULL, but must use IS_ERR().
>>>>> Converting any IS_ERR() returns to NULL theoretically is the act of
>>>>> converting one valid return value to some other completely random return
>>>>> value.
>>>>
>>>> You describe how IS_ERR() works, but you didn't address the patch.
>>>> There's nothing random about the changes to samsung_dmadev_request().
>>>> It still returns NULL or a valid channel just as before.
>>>
>>> I was addressing the patch. I guess I should have explained as follows.
>>>
>>> First, the following code is technically buggy:
>>
>> No, it's not, but I think we have different implementations in mind.
>>
>>>
>>> +             chan = dma_request_slave_channel(dev, ch_name);
>>> +             return IS_ERR(chan) ? (unsigned) NULL : (unsigned) chan;
>>>
>>> ... since it assumes that dma_request_slave_channel() never returns NULL
>>> as a valid non-error value. This is specifically prohibited by the fact
>>> that dma_request_slave_channel() returns either an ERR value or a valid
>>> value; in that case, NULL is not an ERR value, and hence must be
>>> considered valid.
>>
>> Let's stop there and be clear we are talking about the same proposal.
>>
>> The proposal is dma_request_slave_channel only returns errors or valid
>> pointers, never NULL.
>
> OK, so if you make that assumption, I guess it's safe.

I made that assumption because that is what your original patch proposed:

+/**
+ * dma_request_slave_channel_or_err - try to allocate an exclusive
slave channel
+ * @dev:       pointer to client device structure
+ * @name:      slave channel name
+ *
+ * Returns pointer to appropriate dma channel on success or an error pointer.
+ */

What's the benefit of leaking NULL values to callers?  If they already
need to check for err, why force them to check for NULL too?