From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Williams Subject: Re: [PATCH 11/31] dma: add channel request API that supports deferred probe Date: Fri, 22 Nov 2013 12:46:11 -0800 Message-ID: References: <1384548866-13141-1-git-send-email-swarren@wwwdotorg.org> <1384766276.14845.155.camel@smile> <528A5170.3090809@wwwdotorg.org> <1384862421.14845.222.camel@smile> <528B9CAE.3040600@wwwdotorg.org> <528BFDD0.3090503@wwwdotorg.org> <528CFE68.6060908@wwwdotorg.org> <528D0C06.9080006@wwwdotorg.org> <1384979000.2067.5.camel@dwillia2-mobl1.amr.corp.intel.com> <528D28A1.2050307@wwwdotorg.org> <528E4F55.9070204@wwwdotorg.org> <528F95A9.2050305@wwwdotorg.org> <528F9DF9.6080706@wwwdotorg.org> <528FB62C.2060607@wwwdotorg.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Return-path: In-Reply-To: <528FB62C.2060607-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Stephen Warren Cc: Andy Shevchenko , Stephen Warren , "Koul, Vinod" , "pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org" , "dmaengine-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org" , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" List-Id: linux-tegra@vger.kernel.org On Fri, Nov 22, 2013 at 11:53 AM, Stephen Warren wrote: > On 11/22/2013 12:49 PM, Dan Williams wrote: >> On Fri, Nov 22, 2013 at 10:10 AM, Stephen Warren 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? From mboxrd@z Thu Jan 1 00:00:00 1970 From: dan.j.williams@intel.com (Dan Williams) Date: Fri, 22 Nov 2013 12:46:11 -0800 Subject: [PATCH 11/31] dma: add channel request API that supports deferred probe In-Reply-To: <528FB62C.2060607@wwwdotorg.org> References: <1384548866-13141-1-git-send-email-swarren@wwwdotorg.org> <1384766276.14845.155.camel@smile> <528A5170.3090809@wwwdotorg.org> <1384862421.14845.222.camel@smile> <528B9CAE.3040600@wwwdotorg.org> <528BFDD0.3090503@wwwdotorg.org> <528CFE68.6060908@wwwdotorg.org> <528D0C06.9080006@wwwdotorg.org> <1384979000.2067.5.camel@dwillia2-mobl1.amr.corp.intel.com> <528D28A1.2050307@wwwdotorg.org> <528E4F55.9070204@wwwdotorg.org> <528F95A9.2050305@wwwdotorg.org> <528F9DF9.6080706@wwwdotorg.org> <528FB62C.2060607@wwwdotorg.org> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Fri, Nov 22, 2013 at 11:53 AM, Stephen Warren wrote: > On 11/22/2013 12:49 PM, Dan Williams wrote: >> On Fri, Nov 22, 2013 at 10:10 AM, Stephen Warren 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?