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. 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. > 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? -- 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
