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

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 11/22/2013 11:04 AM, Dan Williams wrote:
> On Fri, Nov 22, 2013 at 9:34 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 11/21/2013 11:54 PM, Dan Williams wrote:
>>> On Thu, Nov 21, 2013 at 10:22 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>>>> On 11/20/2013 08:22 PM, Dan Williams wrote:
>>>>> On Wed, Nov 20, 2013 at 1:24 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.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?

"Returns pointer to appropriate dma channel on success or an error
pointer." means that callers only have to check for an ERR value. If the
function returns NULL, then other DMA-related functions must treat that
as a valid channel ID. This is case (a) in my previous email.