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

[Dan Williams]

On Fri, Nov 22, 2013 at 1:50 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 11/22/2013 01:46 PM, Dan Williams wrote:
>> 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 *)??
>
> No client of the API should know that; it'd be more like an agreement
> between multiple functions in the subsystem:
>
> handle = subsystemx_allocate_something();
> ...
> subsystemx_use_handle(handle);
>
> Where subsystemx_allocate_something() casts from ID to "pointer", and
> subsystemx_use_handle() casts back from "pointer" to ID. The callers
> would have no idea this was happening.

That's a bug not a feature.  That's someone abusing an api and
breaking type safety to pass arbitrary data.  But since we're talking
in abstract 'buggy_subsytemx' terms why worry?

> I'm not actually aware of any specific cases where that actually happens
> right now, it's just that given the way subsystemx_allocate_something()
> is documented (valid handle/cookie return or ERR value) it's legal for
> "subsystemx" to work that way if it wants, and it should be able to
> change between this cast-a-handle style and actual pointer returns
> without clients being affected.

Wait, this busted way of doing things is documented?

>> 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?
>
> subsystemx_allocate_something() would have to ensure that. Probably just
> by imposing a maximum limit on the handle/ID values.
>
> Anyway, your proposal can certainly /work/. I simply wanted to point out
> that it was different to the two currently accepted styles of return
> value. If you're sure e.g. Russell isn't going to shout at me or you for
> introducing an API that works as you describe, we certainly could go
> ahead with it. Should we explicitly ping him to confirm that?

He already has in other threads IS_ERR_OR_NULL() must die, this
proposal is not that.  Let me go back to your note about "case 2" and
clarify.