On Thu, Nov 21, 2013 at 10:22 AM, Stephen Warren <swarren@xxxxxxxxxxxxx> wrote:
> 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:
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.
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 9162ac80c18f..64c163664b9d 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -593,15 +593,20 @@ EXPORT_SYMBOL_GPL(__dma_request_channel);
*/
struct dma_chan *dma_request_slave_channel(struct device *dev, const
char *name)
{
+ struct dma_chan *chan = ERR_PTR(-ENODEV);
+
/* If device-tree is present get slave info from here */
if (dev->of_node)
return of_dma_request_slave_channel(dev->of_node, name);
/* If device was enumerated by ACPI get slave info from here */
- if (ACPI_HANDLE(dev))
- return acpi_dma_request_slave_chan_by_name(dev, name);
+ if (ACPI_HANDLE(dev)) {
+ chan = acpi_dma_request_slave_chan_by_name(dev, name);
+ if (!chan)
+ chan = ERR_PTR(-ENODEV);
+ }
- return NULL;
+ return chan;
}
In the above the assumption is that of_dma_request_slave_channel() is
modified to guarantee it only returns ERR_PTRs or valid pointers never
NULL. acpi_dma_request_slave_chan_by_name() can continue returning
NULL and dma_request_slave_channel will translate it to an ERR_PTR, or
you can convert it as you do in your patch. Not much value in
converting acpi_dma_request_slave_chan_by_name as it does not return
EPROBE_DEFER and nothing currently cares about the other error values.
--
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
