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 *)?? 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? -- 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
