[11/31] dma: add channel request API that supports deferred probe

Commit Message

Dan Williams Nov. 20, 2013, 8:23 p.m. UTC

On Wed, 2013-11-20 at 12:22 -0700, Stephen Warren wrote:
> On 11/20/2013 12:15 PM, Dan Williams wrote:
> > On Wed, Nov 20, 2013 at 10:24 AM, Stephen Warren <[e-mail]> wrote:
> >> On 11/19/2013 05:38 PM, Dan Williams wrote:
> >>> On Tue, Nov 19, 2013 at 4:09 PM, Stephen Warren <[e-mail]> wrote:
> >>>> Deferred probe certainly isn't the only error that can be returned
> >>>> though,
> >>>
> >>> Of course, but do any of those other ones matter?  Certainly not if
> >>> they've all been hidden behind a NULL return from
> >>> dma_request_slave_channel().
> >>>
> >>>> so I don't think "defer" in the name makes much sense. The
> >>>> function as I wrote it returns a standard "error pointer" value.
> >>>> Typically, callers would simply propagate *any* error code back to the
> >>>> caller of probe() without even looking at it; it's the driver core that
> >>>> checks for -EPROBE_DEFER vs. other error codes. In some cases, drivers
> >>>> do check in order to avoid printing failure messages in the deferred
> >>>> probe case, but again that's pretty standard, and not something specific
> >>>> to this API.
> >>>
> >>> Right, but the only reason to introduce this API is to propagate
> >>> EPROBE_DEFER, right?  It also serves to document drivers that are
> >>> prepared for / depend on deferred probing support.
> >>
> >> Well, that's the reason I'm introducing the API, but it's not really
> >> what the API actually does.
> >>
> > 
> > True, this is quite a bit of back and forth for something that will be
> > temporary.  How bad would it be to short-circuit this discussion and
> > go straight to converting dma_request_slave_channel().  Leave
> > dma_request_channel() as is and just convert the 20 or so users of
> > dma_request_slave_channel() over?
> 
> I had thought about that, but there are drivers that use
> dma_request_slave_channel(), but fall back to dma_request_channel() if
> that fails. I think there were other cases where the two APIs were
> mixed. Drivers would then have a value that sometimes IS_ERR() or is
> valid, and other times ==NULL or is valid. So, the values would have to
> be checked using IS_ERR_OR_NULL() which I believe is now deprecated  -
> certainly Russell will shout at me if I start introducing more usage! 

Ok, I found the discussion about IS_ERR_OR_NULL(), but actually we would
not need to use it, just check for NULL and return an error in
__dma_request_slave_channel.

> So
> that means converting dma_request_channel()'s return value too, and that
> is a /lot/ more to convert. I suppose an alternative might be to have
> the individual affected drivers convert a NULL return from
> dma_request_channel() to an ERR value, but for some reason I forget now,
> even that looked problematic.

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

btw, samsung_dmadev_request() looks like a crash waiting to happen when
that hardware ip shows up on a 64-bit system.

Comments

Stephen Warren Nov. 20, 2013, 9:24 p.m. UTC | #1

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.

The converse is true for functions that return errors encoded as NULL;
callers must check those return results against NULL.

There's no intersection between those two sets of legal tests.

Dan Williams Nov. 21, 2013, 3:22 a.m. UTC | #2

On Wed, Nov 20, 2013 at 1:24 PM, Stephen Warren <[e-mail]> 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.

> The converse is true for functions that return errors encoded as NULL;
> callers must check those return results against NULL.
>
> There's no intersection between those two sets of legal tests.

I don't understand what you are trying to say.  I'm not proposing an
intersection.  I'm proposing that clients explicitly handle an updated
dma_request_slave_channel and we skip this interim
dma_request_slave_channel_no_err step.

Proliferating yet another *request_channel* api is worse than just
having clients understand that dma_request_slave_channel now encodes
errors while dma_request_slave_channel_compat and dma_request_channel
still just return NULL.

Andy Shevchenko Nov. 21, 2013, 9:13 a.m. UTC | #3

On Wed, 2013-11-20 at 19:22 -0800, Dan Williams wrote:
> On Wed, Nov 20, 2013 at 1:24 PM, Stephen Warren <[e-mail]> wrote:

[]

> I'm proposing that clients explicitly handle an updated
> dma_request_slave_channel and we skip this interim
> dma_request_slave_channel_no_err step.

Actually this makes more sense.

Stephen Warren Nov. 21, 2013, 6:22 p.m. UTC | #4

On 11/20/2013 08:22 PM, Dan Williams wrote:
> On Wed, Nov 20, 2013 at 1:24 PM, Stephen Warren <[e-mail]> 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.

Dan Williams Nov. 22, 2013, 11:45 p.m. UTC | #5

On Thu, Nov 21, 2013 at 10:22 AM, Stephen Warren <[e-mail]> wrote:
> On 11/20/2013 08:22 PM, Dan Williams wrote:
> 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;

No it isn't.  IS_ERR_OR_NULL means the api returns 3 states (channel,
null, error-pointer).  The client converting error-pointer to NULL
after the fact is explicit way to say that the client does not care
about the error value.  The client is simply throwing away the error
code and converting the result back into a pass fail.

>         /*
>          * 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;

It's not, because at this point chan will never be an error pointer.
Sure you could do follow on cleanups to allow this driver to propagate
the dma_request_slave_channel error code up and change this to if
(IS_ERR(chan)) return PTR_ERR(chan), but that's incremental to the
initial conversion.

> 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.

It's not solved, you would need to audit the rest of the driver to
make sure that everywhere it checks a channel is NULL it checks for
IS_ERR instead.  That's a deeper / unnecessary rework for driver's
that don't care about the reason they did not get a channel.

> 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

Yes, better to live with this situation and convert existing drivers
vs have a subset of drivers call a new
dma_request_slave_channel_or_err() API and then *still* need to
convert it to NULL.

> 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.

Getting an error from dma_request_slave_channel() and converting that
value to NULL is a bug because dma_request_channel() would also return
NULL if it did not get a channel?  That's normalization, not a bug.

Stephen Warren Nov. 23, 2013, 12:17 a.m. UTC | #6

On 11/22/2013 04:45 PM, Dan Williams wrote:
> On Thu, Nov 21, 2013 at 10:22 AM, Stephen Warren <[e-mail]> wrote:
>> On 11/20/2013 08:22 PM, Dan Williams wrote:
>> 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;
> 
> No it isn't.  IS_ERR_OR_NULL means the api returns 3 states (channel,
> null, error-pointer).  The client converting error-pointer to NULL
> after the fact is explicit way to say that the client does not care
> about the error value.  The client is simply throwing away the error
> code and converting the result back into a pass fail.

The client can only translate an ERR value to NULL if it knows that the
API will never return NULL. If the API could return NULL, then this
translation uses a valid return value to represent the error case.
Functions that return an ERR value or a valid value do NOT in general
say anything either way about a return value of NULL.

While this translation isn't *exactly* the same as an API returning 3
states, it is almost identical; the translation is only possible if the
set of numerically possible return values from the function are
segmented into 3 sets:

a) Valid
b) An ERR value
c) NULL (which is never returned)

I believe the important point in Russell's argument against
IS_ERR_OR_NULL is that the segmentation should be limited to two sets
(a, b) or (a, c) and not 3. I don't /think/ it was relevant whether the
function segmented the return values into 3 sets just so it could
guarantee that it didn't return one of those sets.

If that's not the case, the following would indeed solve the problem easily:

> /**
>  * dma_request_slave_channel - try to allocate an exclusive slave channel
...
>  * Returns pointer to appropriate DMA channel on success or an error pointer.
>  * In order to ease compatibility with other DMA request APIs, this function
>  * guarantees NEVER to return NULL.
>  */

>>         /*
>>          * 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;
> 
> It's not, because at this point chan will never be an error pointer.

It's the combination of the two. IS_ERR_OR_NULL is two separate tests in
one macro. Simply separating the two tests into separate lines of code
doesn't change what the code is doing.

> Sure you could do follow on cleanups to allow this driver to propagate
> the dma_request_slave_channel error code up and change this to if
> (IS_ERR(chan)) return PTR_ERR(chan), but that's incremental to the
> initial conversion.
> 
>> 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.
> 
> It's not solved, you would need to audit the rest of the driver to
> make sure that everywhere it checks a channel is NULL it checks for
> IS_ERR instead.  That's a deeper / unnecessary rework for driver's
> that don't care about the reason they did not get a channel.

I was of course assuming that the auditing would be done, and indeed
when I first started work on this conversion, it's exactly what I was
doing. That's why I said *all* checks, not just "the first check".

>> 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
> 
> Yes, better to live with this situation and convert existing drivers
> vs have a subset of drivers call a new
> dma_request_slave_channel_or_err() API and then *still* need to
> convert it to NULL.
> 
>> 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.
> 
> Getting an error from dma_request_slave_channel() and converting that
> value to NULL is a bug because dma_request_channel() would also return
> NULL if it did not get a channel?  That's normalization, not a bug.

No, it's a bug because if dma_request_slave_channel() is documented to
return valid-or-ERR, then assuming that it can never return NULL is
inconsistent with that. The translation is only possible if it's
documented to return valid-or-ERR-but-never-NULL.

Dan Williams Nov. 23, 2013, 12:37 a.m. UTC | #7

On Fri, Nov 22, 2013 at 4:17 PM, Stephen Warren <[e-mail]> wrote:
> No, it's a bug because if dma_request_slave_channel() is documented to
> return valid-or-ERR, then assuming that it can never return NULL is
> inconsistent with that. The translation is only possible if it's
> documented to return valid-or-ERR-but-never-NULL.

Yes!  We are in violent agreement about this point.  Make
dma_request_slave_channel return valid-or-ERR-but-never-NULL, and show
me the compat user that would care if it got an EPROBE_DEFER instead
of ENODEV.

Patch

diff --git a/arch/arm/plat-samsung/dma-ops.c b/arch/arm/plat-samsung/dma-ops.c
index ec0d731b0e7b..abee452bcf6e 100644
--- 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)
 {
+	struct dma_chan *chan;
+
 	dma_cap_mask_t mask;
 
 	dma_cap_zero(mask);
 	dma_cap_set(param->cap, mask);
 
-	if (dev->of_node)
-		return (unsigned)dma_request_slave_channel(dev, ch_name);
-	else
+	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);
+	}
 }
 
 static int samsung_dmadev_release(unsigned ch, void *param)
diff --git a/drivers/ata/pata_arasan_cf.c b/drivers/ata/pata_arasan_cf.c
index 853f610af28f..c483b095e157 100644
--- a/drivers/ata/pata_arasan_cf.c
+++ b/drivers/ata/pata_arasan_cf.c
@@ -528,7 +528,8 @@  static void data_xfer(struct work_struct *work)
 	/* request dma channels */
 	/* dma_request_channel may sleep, so calling from process context */
 	acdev->dma_chan = dma_request_slave_channel(acdev->host->dev, "data");
-	if (!acdev->dma_chan) {
+	if (IS_ERR(acdev->dma_chan)) {
+		acdev->dma_chan = NULL;
 		dev_err(acdev->host->dev, "Unable to get dma_chan\n");
 		goto chan_request_fail;
 	}
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;
 }
 EXPORT_SYMBOL_GPL(dma_request_slave_channel);
 
diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index b7c857774708..777e18db654a 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -723,7 +723,8 @@  static int mxs_i2c_probe(struct platform_device *pdev)
 
 	/* Setup the DMA */
 	i2c->dmach = dma_request_slave_channel(dev, "rx-tx");
-	if (!i2c->dmach) {
+	if (IS_ERR(i2c->dmach)) {
+		i2c->dmach = NULL;
 		dev_err(dev, "Failed to request dma\n");
 		return -ENODEV;
 	}
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index c3785edc0e92..342408961590 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -350,6 +350,11 @@  static void mmci_dma_setup(struct mmci_host *host)
 	host->dma_rx_channel = dma_request_slave_channel(mmc_dev(host->mmc), "rx");
 	host->dma_tx_channel = dma_request_slave_channel(mmc_dev(host->mmc), "tx");
 
+	if (IS_ERR(host->dma_rx_channel))
+		host->dma_rx_channel = NULL;
+	if (IS_ERR(host->dma_tx_channel))
+		host->dma_tx_channel = NULL;
+
 	/* initialize pre request cookie */
 	host->next_data.cookie = 1;
 
diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index c174c6a0d224..527c1a427b13 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -1170,11 +1170,13 @@  static int mxcmci_probe(struct platform_device *pdev)
 			host->dma = dma_request_channel(mask, filter, host);
 		}
 	}
-	if (host->dma)
+	if (!IS_ERR(host->dma))
 		mmc->max_seg_size = dma_get_max_seg_size(
 				host->dma->device->dev);
-	else
+	else {
+		host->dma = NULL;
 		dev_info(mmc_dev(host->mmc), "dma not available. Using PIO\n");
+	}
 
 	INIT_WORK(&host->datawork, mxcmci_datawork);
 
diff --git a/drivers/mmc/host/mxs-mmc.c b/drivers/mmc/host/mxs-mmc.c
index e1fa3ef735e0..f5411a6001fa 100644
--- a/drivers/mmc/host/mxs-mmc.c
+++ b/drivers/mmc/host/mxs-mmc.c
@@ -636,7 +636,8 @@  static int mxs_mmc_probe(struct platform_device *pdev)
 	}
 
 	ssp->dmach = dma_request_slave_channel(&pdev->dev, "rx-tx");
-	if (!ssp->dmach) {
+	if (IS_ERR(ssp->dmach)) {
+		ssp->dmach = NULL;
 		dev_err(mmc_dev(host->mmc),
 			"%s: failed to request dma\n", __func__);
 		ret = -ENODEV;
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 59ab0692f0b9..fbe1f372faca 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -564,7 +564,7 @@  static int acquire_dma_channels(struct gpmi_nand_data *this)
 
 	/* request dma channel */
 	dma_chan = dma_request_slave_channel(&pdev->dev, "rx-tx");
-	if (!dma_chan) {
+	if (IS_ERR(dma_chan)) {
 		pr_err("Failed to request DMA channel.\n");
 		goto acquire_err;
 	}
diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
index de7b1141b90f..7a3ebe87b106 100644
--- a/drivers/spi/spi-mxs.c
+++ b/drivers/spi/spi-mxs.c
@@ -552,7 +552,8 @@  static int mxs_spi_probe(struct platform_device *pdev)
 		goto out_master_free;
 
 	ssp->dmach = dma_request_slave_channel(&pdev->dev, "rx-tx");
-	if (!ssp->dmach) {
+	if (IS_ERR(ssp->dmach)) {
+		ssp->dmach = NULL;
 		dev_err(ssp->dev, "Failed to request DMA\n");
 		ret = -ENODEV;
 		goto out_master_free;
diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 9c511a954d21..38d4121f7073 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -1140,11 +1140,11 @@  static int pl022_dma_autoprobe(struct pl022 *pl022)
 
 	/* automatically configure DMA channels from platform, normally using DT */
 	pl022->dma_rx_channel = dma_request_slave_channel(dev, "rx");
-	if (!pl022->dma_rx_channel)
+	if (IS_ERR(pl022->dma_rx_channel))
 		goto err_no_rxchan;
 
 	pl022->dma_tx_channel = dma_request_slave_channel(dev, "tx");
-	if (!pl022->dma_tx_channel)
+	if (IS_ERR(pl022->dma_tx_channel))
 		goto err_no_txchan;
 
 	pl022->dummypage = kmalloc(PAGE_SIZE, GFP_KERNEL);
@@ -1155,11 +1155,11 @@  static int pl022_dma_autoprobe(struct pl022 *pl022)
 
 err_no_dummypage:
 	dma_release_channel(pl022->dma_tx_channel);
-	pl022->dma_tx_channel = NULL;
 err_no_txchan:
+	pl022->dma_tx_channel = NULL;
 	dma_release_channel(pl022->dma_rx_channel);
-	pl022->dma_rx_channel = NULL;
 err_no_rxchan:
+	pl022->dma_rx_channel = NULL;
 	return -ENODEV;
 }
 		
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index aaa22867e656..c0f400c3c954 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -278,7 +278,7 @@  static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
 
 	chan = dma_request_slave_channel(dev, "tx");
 
-	if (!chan) {
+	if (IS_ERR(chan)) {
 		/* We need platform data */
 		if (!plat || !plat->dma_filter) {
 			dev_info(uap->port.dev, "no DMA platform data\n");
@@ -305,8 +305,10 @@  static void pl011_dma_probe_initcall(struct device *dev, struct uart_amba_port *
 
 	/* Optionally make use of an RX channel as well */
 	chan = dma_request_slave_channel(dev, "rx");
+	if (IS_ERR(chan))
+		chan = NULL;
 	
-	if (!chan && plat->dma_rx_param) {
+	if (chan && plat->dma_rx_param) {
 		chan = dma_request_channel(mask, plat->dma_filter, plat->dma_rx_param);
 
 		if (!chan) {
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index d067285a2d20..c0ae083e061c 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -723,8 +723,10 @@  static int atmel_prepare_tx_dma(struct uart_port *port)
 	dma_cap_set(DMA_SLAVE, mask);
 
 	atmel_port->chan_tx = dma_request_slave_channel(port->dev, "tx");
-	if (atmel_port->chan_tx == NULL)
+	if (IS_ERR(atmel_port->chan_tx)) {
+		atmel_port->chan_tx = NULL;
 		goto chan_err;
+	}
 	dev_info(port->dev, "using %s for tx DMA transfers\n",
 		dma_chan_name(atmel_port->chan_tx));
 
@@ -890,8 +892,10 @@  static int atmel_prepare_rx_dma(struct uart_port *port)
 	dma_cap_set(DMA_CYCLIC, mask);
 
 	atmel_port->chan_rx = dma_request_slave_channel(port->dev, "rx");
-	if (atmel_port->chan_rx == NULL)
+	if (IS_ERR(atmel_port->chan_rx)) {
+		atmel_port->chan_rx = NULL;
 		goto chan_err;
+	}
 	dev_info(port->dev, "using %s for rx DMA transfers\n",
 		dma_chan_name(atmel_port->chan_rx));
 
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 042aa077b5b3..1e4cb60ce0af 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -985,7 +985,8 @@  static int imx_uart_dma_init(struct imx_port *sport)
 
 	/* Prepare for RX : */
 	sport->dma_chan_rx = dma_request_slave_channel(dev, "rx");
-	if (!sport->dma_chan_rx) {
+	if (IS_ERR(sport->dma_chan_rx)) {
+		sport->dma_chan_rx = NULL;
 		dev_dbg(dev, "cannot get the DMA channel.\n");
 		ret = -EINVAL;
 		goto err;
@@ -1011,7 +1012,8 @@  static int imx_uart_dma_init(struct imx_port *sport)
 
 	/* Prepare for TX : */
 	sport->dma_chan_tx = dma_request_slave_channel(dev, "tx");
-	if (!sport->dma_chan_tx) {
+	if (IS_ERR(sport->dma_chan_tx)) {
+		sport->dma_chan_tx = NULL;
 		dev_err(dev, "cannot get the TX DMA channel!\n");
 		ret = -EINVAL;
 		goto err;
diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 10e9d70b5c40..cc9747ab71cd 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -530,16 +530,20 @@  static int mxs_auart_dma_init(struct mxs_auart_port *s)
 
 	/* init for RX */
 	s->rx_dma_chan = dma_request_slave_channel(s->dev, "rx");
-	if (!s->rx_dma_chan)
+	if (IS_ERR(s->rx_dma_chan)) {
+		s->rx_dma_chan = NULL;
 		goto err_out;
+	}
 	s->rx_dma_buf = kzalloc(UART_XMIT_SIZE, GFP_KERNEL | GFP_DMA);
 	if (!s->rx_dma_buf)
 		goto err_out;
 
 	/* init for TX */
 	s->tx_dma_chan = dma_request_slave_channel(s->dev, "tx");
-	if (!s->tx_dma_chan)
+	if (IS_ERR(s->tx_dma_chan)) {
+		s->tx_dma_chan = NULL;
 		goto err_out;
+	}
 	s->tx_dma_buf = kzalloc(UART_XMIT_SIZE, GFP_KERNEL | GFP_DMA);
 	if (!s->tx_dma_buf)
 		goto err_out;
diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
index ff9d6de2b746..b71c5f138968 100644
--- a/drivers/usb/musb/musb_cppi41.c
+++ b/drivers/usb/musb/musb_cppi41.c
@@ -502,7 +502,7 @@  static int cppi41_dma_controller_start(struct cppi41_dma_controller *controller)
 		musb_dma->max_len = SZ_4M;
 
 		dc = dma_request_slave_channel(dev, str);
-		if (!dc) {
+		if (IS_ERR(dc)) {
 			dev_err(dev, "Falied to request %s.\n", str);
 			ret = -EPROBE_DEFER;
 			goto err;
diff --git a/drivers/usb/musb/ux500_dma.c b/drivers/usb/musb/ux500_dma.c
index 3700e9713258..6c863b90ad66 100644
--- a/drivers/usb/musb/ux500_dma.c
+++ b/drivers/usb/musb/ux500_dma.c
@@ -330,13 +330,14 @@  static int ux500_dma_controller_start(struct ux500_dma_controller *controller)
 			ux500_channel->dma_chan =
 				dma_request_slave_channel(dev, chan_names[ch_num]);
 
-			if (!ux500_channel->dma_chan)
+			if (IS_ERR(ux500_channel->dma_chan)) {
 				ux500_channel->dma_chan =
 					dma_request_channel(mask,
 							    data ?
 							    data->dma_filter :
 							    NULL,
 							    param_array[ch_num]);
+			}
 
 			if (!ux500_channel->dma_chan) {
 				ERR("Dma pipe allocation error dir=%d ch=%d\n",
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 0bc727534108..0da9425d64e7 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -1056,7 +1056,7 @@  static inline struct dma_chan
 	struct dma_chan *chan;
 
 	chan = dma_request_slave_channel(dev, name);
-	if (chan)
+	if (!IS_ERR(chan))
 		return chan;
 
 	return __dma_request_channel(mask, fn, fn_param);
diff --git a/sound/soc/omap/omap-pcm.c b/sound/soc/omap/omap-pcm.c
index a11405de86e8..56c3056cdac7 100644
--- a/sound/soc/omap/omap-pcm.c
+++ b/sound/soc/omap/omap-pcm.c
@@ -125,7 +125,7 @@  static int omap_pcm_open(struct snd_pcm_substream *substream)
 
 		chan = dma_request_slave_channel(rtd->cpu_dai->dev,
 						 dma_data->filter_data);
-		ret = snd_dmaengine_pcm_open(substream, chan);
+		ret = snd_dmaengine_pcm_open(substream, IS_ERR(chan) ? NULL : chan);
 	} else {
 		ret = snd_dmaengine_pcm_open_request_chan(substream,
 							  omap_dma_filter_fn,
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
index e29ec3cd84b1..4182b203fad5 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -227,11 +227,15 @@  static void dmaengine_pcm_request_chan_of(struct dmaengine_pcm *pcm,
 
 	if (pcm->flags & SND_DMAENGINE_PCM_FLAG_HALF_DUPLEX) {
 		pcm->chan[0] = dma_request_slave_channel(dev, "rx-tx");
+		if (IS_ERR(pcm->chan[0]))
+			pcm->chan[0] = NULL;
 		pcm->chan[1] = pcm->chan[0];
 	} else {
 		for (i = SNDRV_PCM_STREAM_PLAYBACK; i <= SNDRV_PCM_STREAM_CAPTURE; i++) {
 			pcm->chan[i] = dma_request_slave_channel(dev,
 					dmaengine_pcm_dma_channel_names[i]);
+			if (IS_ERR(pcm->chan[i]))
+				pcm->chan[i] = NULL;
 		}
 	}
 }