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

[Dan Williams]

On Fri, Nov 22, 2013 at 11:53 AM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 11/22/2013 12:49 PM, Dan Williams wrote:
>> On Fri, Nov 22, 2013 at 10:10 AM, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>>>> 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.
>>
>> How can a channel be "valid" and NULL at the same time?  Without the
>> guarantee that dma_request_channel always returns a non-null-channel
>> pointer or an error pointer you're forcing clients to use or open-code
>> IS_ERR_OR_NULL.
>
> No, callers should just follow the documentation. If all error cases are
> indicated by an ERR pointer, then there is no need to check for NULL. In
> fact, client must not check anything beyond whether the value is an ERR
> value or not. So, there's no need to use IS_ERR_OR_NULL.
>
> It's up to the API to make sure that it returns values that are valid
> for other calls to related APIs. If that doesn't include NULL, it won't
> return NULL. If it does, it might. But, that's an internal
> implementation detail of the API (and associated APIs), not something
> that clients should know about.
>
> One situation where a NULL might be valid is where the return value
> isn't really a pointer, but an integer index or ID cast to a pointer.

Ok that's the piece I am missing, and maybe explains why
samsung_dmadev_request() looks so broken.  Are there really
implementations out there that somehow know that the return value from
dma_request_slave channel is not a (struct dma_chan *)??

At that point just change the prototype of dma_request_slave_channel to:

MAGIC_t dma_request_slave_channel(struct device *dev, const char *name)

Those clients need to be killed or fixed, otherwise how do you
guarantee that the 'integer index or ID' does not collide with the
ERR_PTR() number space?