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 10:04:32 -0800 Message-ID: References: <1384548866-13141-1-git-send-email-swarren@wwwdotorg.org> <1384548866-13141-12-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> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Return-path: In-Reply-To: <528F95A9.2050305-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 9:34 AM, Stephen Warren wrote: > On 11/21/2013 11:54 PM, Dan Williams wrote: >> On Thu, Nov 21, 2013 at 10:22 AM, Stephen Warren wrote: >>> On 11/20/2013 08:22 PM, Dan Williams wrote: >>>> On Wed, Nov 20, 2013 at 1:24 PM, Stephen Warren wrote: >>>>> On 11/20/2013 01:23 PM, Williams, Dan J wrote: >>>>> ... >>>>>> Why do the drivers that call dma_request_channel need to convert it to >>>>>> an ERR value? i.e. what's problematic about the below (not compile >>>>>> tested)? >>>>> ... >>>>>> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c >>>>> ... >>>>>> @@ -22,16 +22,20 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch, >>>>>> struct samsung_dma_req *param, >>>>>> struct device *dev, char *ch_name) >>>>> ... >>>>>> + if (dev->of_node) { >>>>>> + chan = dma_request_slave_channel(dev, ch_name); >>>>>> + return IS_ERR(chan) ? (unsigned) NULL : (unsigned) chan; >>>>>> + } else { >>>>>> return (unsigned)dma_request_channel(mask, pl330_filter, >>>>>> (void *)dma_ch); >>>>>> + } >>>>> >>>>> The argument is that if a function returns errors encoded as an ERR >>>>> pointer, then callers must assume that any non-IS_ERR value that the >>>>> function returns is valid. NULL is one of those values. As such, callers >>>>> can no longer check the value against NULL, but must use IS_ERR(). >>>>> Converting any IS_ERR() returns to NULL theoretically is the act of >>>>> converting one valid return value to some other completely random return >>>>> value. >>>> >>>> You describe how IS_ERR() works, but you didn't address the patch. >>>> There's nothing random about the changes to samsung_dmadev_request(). >>>> It still returns NULL or a valid channel just as before. >>> >>> I was addressing the patch. I guess I should have explained as follows. >>> >>> First, the following code is technically buggy: >> >> No, it's not, but I think we have different implementations in mind. >> >>> >>> + chan = dma_request_slave_channel(dev, ch_name); >>> + return IS_ERR(chan) ? (unsigned) NULL : (unsigned) chan; >>> >>> ... since it assumes that dma_request_slave_channel() never returns NULL >>> as a valid non-error value. This is specifically prohibited by the fact >>> that dma_request_slave_channel() returns either an ERR value or a valid >>> value; in that case, NULL is not an ERR value, and hence must be >>> considered valid. >> >> Let's stop there and be clear we are talking about the same proposal. >> >> 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? From mboxrd@z Thu Jan 1 00:00:00 1970 From: dan.j.williams@intel.com (Dan Williams) Date: Fri, 22 Nov 2013 10:04:32 -0800 Subject: [PATCH 11/31] dma: add channel request API that supports deferred probe In-Reply-To: <528F95A9.2050305@wwwdotorg.org> References: <1384548866-13141-1-git-send-email-swarren@wwwdotorg.org> <1384548866-13141-12-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> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Fri, Nov 22, 2013 at 9:34 AM, Stephen Warren wrote: > On 11/21/2013 11:54 PM, Dan Williams wrote: >> On Thu, Nov 21, 2013 at 10:22 AM, Stephen Warren wrote: >>> On 11/20/2013 08:22 PM, Dan Williams wrote: >>>> On Wed, Nov 20, 2013 at 1:24 PM, Stephen Warren wrote: >>>>> On 11/20/2013 01:23 PM, Williams, Dan J wrote: >>>>> ... >>>>>> Why do the drivers that call dma_request_channel need to convert it to >>>>>> an ERR value? i.e. what's problematic about the below (not compile >>>>>> tested)? >>>>> ... >>>>>> diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c >>>>> ... >>>>>> @@ -22,16 +22,20 @@ static unsigned samsung_dmadev_request(enum dma_ch dma_ch, >>>>>> struct samsung_dma_req *param, >>>>>> struct device *dev, char *ch_name) >>>>> ... >>>>>> + if (dev->of_node) { >>>>>> + chan = dma_request_slave_channel(dev, ch_name); >>>>>> + return IS_ERR(chan) ? (unsigned) NULL : (unsigned) chan; >>>>>> + } else { >>>>>> return (unsigned)dma_request_channel(mask, pl330_filter, >>>>>> (void *)dma_ch); >>>>>> + } >>>>> >>>>> The argument is that if a function returns errors encoded as an ERR >>>>> pointer, then callers must assume that any non-IS_ERR value that the >>>>> function returns is valid. NULL is one of those values. As such, callers >>>>> can no longer check the value against NULL, but must use IS_ERR(). >>>>> Converting any IS_ERR() returns to NULL theoretically is the act of >>>>> converting one valid return value to some other completely random return >>>>> value. >>>> >>>> You describe how IS_ERR() works, but you didn't address the patch. >>>> There's nothing random about the changes to samsung_dmadev_request(). >>>> It still returns NULL or a valid channel just as before. >>> >>> I was addressing the patch. I guess I should have explained as follows. >>> >>> First, the following code is technically buggy: >> >> No, it's not, but I think we have different implementations in mind. >> >>> >>> + chan = dma_request_slave_channel(dev, ch_name); >>> + return IS_ERR(chan) ? (unsigned) NULL : (unsigned) chan; >>> >>> ... since it assumes that dma_request_slave_channel() never returns NULL >>> as a valid non-error value. This is specifically prohibited by the fact >>> that dma_request_slave_channel() returns either an ERR value or a valid >>> value; in that case, NULL is not an ERR value, and hence must be >>> considered valid. >> >> Let's stop there and be clear we are talking about the same proposal. >> >> 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?