On Fri, Nov 22, 2013 at 1:50 PM, Stephen Warren <swarren@xxxxxxxxxxxxx> wrote: > On 11/22/2013 01:46 PM, Dan Williams wrote: >> On Fri, Nov 22, 2013 at 11:53 AM, Stephen Warren <swarren@xxxxxxxxxxxxx> wrote: >>> On 11/22/2013 12:49 PM, Dan Williams wrote: >>>> On Fri, Nov 22, 2013 at 10:10 AM, Stephen Warren <swarren@xxxxxxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
