On 11/22/2013 04:13 PM, Dan Williams wrote: > 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? I should have said: s/is documented/would be documented/. Or perhaps s/documented/discussed/. IIRC, in previous discussions of IS_ERR_OR_NULL, this came up as a specific (perhaps hypothetical) thing that APIs could legitimately do that made it important the API clients didn't impose restrictions on return values beyond what APIs documented. -- 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
