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

[Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Thu, Nov 21, 2013 at 10:22 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> 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.