On 11/20/2013 08:22 PM, Dan Williams wrote:
> On Wed, Nov 20, 2013 at 1:24 PM, Stephen Warren <swarren@xxxxxxxxxxxxx> 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:
+ 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.
Also, observe that the following are semantically equivalent:
1)
/*
* This is a demonstration of using IS_ERR_OR_NULL for error
* checking. We want to remove use of IS_ERR_OR_NULL though.
*/
chan = dma_request_slave_channel(dev, ch_name);
if (IS_ERR_OR_NULL(chan))
return -ESOMETHING;
2)
/* These first 3 lines are part of your patch */
chan = dma_request_slave_channel(dev, ch_name);
if (IS_ERR(chan)
chan = NULL;
if (!chan) // This test and the above are IS_ERR_OR_NULL
attempt allocation some other way;
/*
* This is code elsewhere in a driver where DMA is optional;
* that code must act differently based on whether a DMA
* channel was acquired or not. So, it tests chan against
* NULL.
*/
if (!chan) // This test and the above IS_ERR are IS_ERR_OR_NULL
return -ESOMETHING;
In case (2) above, if the driver /only/ calls a modified
dma_request_slave_channel(), all the checks could just be if
(IS_ERR(chan)) instead - then problem solved. However, if the driver
mixes the new dma_request_slave_channel() and the unconverted
dma_request_channel(), it has to either (a) convert an ERR return from
dma_request_slave_channel() to match dma_request_channel()'s NULL error
return, or (b) convert a NULL return from dma_request_channel() to match
dma_request_slave_channel()'s ERR return. Without the conversion, all
tests would have to use the deprecated IS_ERR_OR_NULL. Either of those
conversion options converts an error value from 1 API into a
theoretically valid return value from the other API, which is a bug.
Perhaps one can argue that an API that returns either a valid value or
NULL can never return a value that matches an ERR value? If so, perhaps
the following would work in practice:
if (dev->of_node) {
chan = dma_request_slave_channel(dev, ch_name);
} else {
chan = dma_request_channel(mask, pl330_filter,
(void *)dma_ch);
if (!chan)
chan = ERR_PTR(-ENODEV);
}
...
if (IS_ERR(chan))
...
... but I'm not sure whether that assumption is acceptable.
--
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
