diff mbox

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

Message ID CAA9_cmfMtp4WjRGVzfTxrwcNWP95+HE7DAYg_GeWfqi=L2K2aw@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dan Williams Nov. 25, 2013, 5:45 p.m. UTC
On Mon, Nov 25, 2013 at 9:26 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 11/22/2013 05:34 PM, Russell King - ARM Linux wrote:
> [... various discussions re: IS_ERR, IS_ERR_OR_NULL, etc.]
>
> Russell, so if we document dma_request_slave_channel() as follows:
>
>> /*
>>  * 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.
>>  */
>
> Are you then OK with clients doing either of e.g.:
>
> chan = dma_request_slave_channel(...);
> if (IS_ERR(chan))
>         // This returns NULL or a valid handle/pointer
>         chan = dma_request_channel()
> if (!chan)
>         Here, chan is invalid;
>         return;
> Here, chan is valid
>

If the driver falls back to dma_request_channel then this one is cleaner.

> or:
>
> if (xxx) {
>         chan = dma_request_slave_channel(...);
>         // Convert to same error value as else{} block generates
>         if (IS_ERR(chan))
>                 chan = NULL
> } else {
>         // This returns NULL or a valid handle/pointer
>         chan = dma_request_channel()
> }
> if (!chan)
>         Here, chan is invalid
>         return;
> Here, chan is valid

Regardless of whether the driver was dma_request_channel as a
fallback, it will currently use NULL to indicate an allocation
failure.  We could go and audit the driver's usage of the result to
add more IS_ERR() guards.  In the absence of a later call to
dma_request_channel() immediately converting to NULL is the least
error prone conversion.  Of course it's up to the driver, for example:

                        goto err;

...but need to go back and see if this can be cleaned up to not invent
the error code here.

Comments

Russell King - ARM Linux Nov. 25, 2013, 6 p.m. UTC | #1
On Mon, Nov 25, 2013 at 09:45:18AM -0800, Dan Williams wrote:
> Regardless of whether the driver was dma_request_channel as a
> fallback, it will currently use NULL to indicate an allocation
> failure.

At the moment, dma_request_slave_channel()'s return values are valid
pointer or NULL.  I'd suggest as that's how it's been established,
that function is left alone - changing the return values in this kind
of invisible way is Really Bad(tm) because you can't check that it's
right in any future users - unless you're prepared to review every
single new user of this function for the next few years or more.

Instead, leaving it as is and introducing a new function name with
the different return error method is the right way to do this.
Stephen Warren Nov. 25, 2013, 6:07 p.m. UTC | #2
On 11/25/2013 11:00 AM, Russell King - ARM Linux wrote:
> On Mon, Nov 25, 2013 at 09:45:18AM -0800, Dan Williams wrote:
>> Regardless of whether the driver was dma_request_channel as a
>> fallback, it will currently use NULL to indicate an allocation
>> failure.
> 
> At the moment, dma_request_slave_channel()'s return values are valid
> pointer or NULL.  I'd suggest as that's how it's been established,
> that function is left alone - changing the return values in this kind
> of invisible way is Really Bad(tm) because you can't check that it's
> right in any future users - unless you're prepared to review every
> single new user of this function for the next few years or more.
> 
> Instead, leaving it as is and introducing a new function name with
> the different return error method is the right way to do this.

Uggh. So here's the history of my patch series:

a) Started modifying dma_request_slave_channel() since there are very
few users right now, and it seemed simple to convert them all.

b) Found that some places mixed dma_request_slave_channel() and
dma_request_channel(). Converting dma_request_channel()'s return value
in the same way seemed prohibitive since it's much more widely used and
has been around for longer.

c) Rewrote patch to introduce a new function instead, since that didn't
require converting any existing drivers.

d) Received review feedback that I really should convert the existing
function's return code.

e) Started work on that using the return-value-conversion idea from Dan,
which allowed the two "return value styles" to co-exist easily, since I
figured that would be accepted.

Now you're telling me I should do what I already did in the patch, but
Dan is telling me to do the opposite:-(

How do we resolve this? Is there an option both you and Dan can accept?
Dan Williams Nov. 25, 2013, 6:42 p.m. UTC | #3
On Mon, Nov 25, 2013 at 10:00 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Nov 25, 2013 at 09:45:18AM -0800, Dan Williams wrote:
>> Regardless of whether the driver was dma_request_channel as a
>> fallback, it will currently use NULL to indicate an allocation
>> failure.
>
> At the moment, dma_request_slave_channel()'s return values are valid
> pointer or NULL.  I'd suggest as that's how it's been established,
> that function is left alone - changing the return values in this kind
> of invisible way is Really Bad(tm) because you can't check that it's
> right in any future users - unless you're prepared to review every
> single new user of this function for the next few years or more.
>
> Instead, leaving it as is and introducing a new function name with
> the different return error method is the right way to do this.

I was attempting to tap the brakes on the number of request_channel
apis in circulation.
dma_request_channel
dma_request_slave_channel
dma_request_slave_channel_compat
... and now dma_request_slave_channel_reason

...but given we already have things like samsung_dmadev_request() in
the wild, tracking correct usage going forward will be an uphill
battle.  We can do this the other way round.  Introduce the new api
and delete the old one (after some time).

Stephen,

Can we make the new api not pass the 'defer' flag.  You mention it is
for compat reasons, but since there are no users I'm not seeing how
there can be compatibility concerns.  Can you elaborate?
Stephen Warren Nov. 25, 2013, 7 p.m. UTC | #4
On 11/25/2013 11:42 AM, Dan Williams wrote:
> On Mon, Nov 25, 2013 at 10:00 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Mon, Nov 25, 2013 at 09:45:18AM -0800, Dan Williams wrote:
>>> Regardless of whether the driver was dma_request_channel as a
>>> fallback, it will currently use NULL to indicate an allocation
>>> failure.
>>
>> At the moment, dma_request_slave_channel()'s return values are valid
>> pointer or NULL.  I'd suggest as that's how it's been established,
>> that function is left alone - changing the return values in this kind
>> of invisible way is Really Bad(tm) because you can't check that it's
>> right in any future users - unless you're prepared to review every
>> single new user of this function for the next few years or more.
>>
>> Instead, leaving it as is and introducing a new function name with
>> the different return error method is the right way to do this.
> 
> I was attempting to tap the brakes on the number of request_channel
> apis in circulation.
> dma_request_channel
> dma_request_slave_channel
> dma_request_slave_channel_compat
> ... and now dma_request_slave_channel_reason
> 
> ...but given we already have things like samsung_dmadev_request() in
> the wild, tracking correct usage going forward will be an uphill
> battle.  We can do this the other way round.  Introduce the new api
> and delete the old one (after some time).
> 
> Stephen,
> 
> Can we make the new api not pass the 'defer' flag.  You mention it is
> for compat reasons, but since there are no users I'm not seeing how
> there can be compatibility concerns.  Can you elaborate?

dma_request_slave_channel_or_err() doesn't have a defer flag. Rather,
__dma_request_slave_channel_or_err() did, which is the shared
implementation between the existing existing dma_request_slave_channel()
and the new dma_request_slave_channel_or_err().

That flag exists so it can be passed to of_dma_request_slave_channel().
That function needs it because I didn't want to change the existing
behaviour of dma_request_slave_channel() at all, yet I wanted slightly
different behaviour from dma_request_slave_channel_or_err().

Specifically, if of_dma_request_slave_channel() finds an entry in the
dmas DT property for which there is no registered DMA controller, it
simply ignores that entry and carries on, hoping to find an alternative
entry that can satisfy the request (since some controllers can be
supported by multiple DMA controllers). However, to support deferred
probe, we really need to bail out from the loop early with -EPROBE_DEFER
if any DMA controller isn't found, in order to wait for it to be registered.

I suppose an alternative would be to remove that flag, and have the loop
in of_dma_request_slave_channel() initially ignore any unregistered DMA
controllers, and still continue to look through the property for any
alternative controller, and return a channel from one if it is found.
Then, at the very end of the function, we could always return
-EPROBE_DEFER if any unregistered DMA controllers were found, otherwise
return -ENODEV. That would keep compatible behaviour, but it would mean
that device probe order would/could influence which dmas entry provided
the channel, since some entries might be ignored based simply on
timing/ordering of DMA controller registration. Is that acceptable?

Either way, just let me know and I'll adjust the code accordingly; the
code changes for either option are simple.
Russell King - ARM Linux Nov. 25, 2013, 7:09 p.m. UTC | #5
On Mon, Nov 25, 2013 at 10:42:52AM -0800, Dan Williams wrote:
> On Mon, Nov 25, 2013 at 10:00 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Nov 25, 2013 at 09:45:18AM -0800, Dan Williams wrote:
> >> Regardless of whether the driver was dma_request_channel as a
> >> fallback, it will currently use NULL to indicate an allocation
> >> failure.
> >
> > At the moment, dma_request_slave_channel()'s return values are valid
> > pointer or NULL.  I'd suggest as that's how it's been established,
> > that function is left alone - changing the return values in this kind
> > of invisible way is Really Bad(tm) because you can't check that it's
> > right in any future users - unless you're prepared to review every
> > single new user of this function for the next few years or more.
> >
> > Instead, leaving it as is and introducing a new function name with
> > the different return error method is the right way to do this.
> 
> I was attempting to tap the brakes on the number of request_channel
> apis in circulation.
> dma_request_channel
> dma_request_slave_channel
> dma_request_slave_channel_compat
> ... and now dma_request_slave_channel_reason
> 
> ...but given we already have things like samsung_dmadev_request() in
> the wild, tracking correct usage going forward will be an uphill
> battle.  We can do this the other way round.  Introduce the new api
> and delete the old one (after some time).

Yes, new and deprecate the old is the right way to do this.

> Can we make the new api not pass the 'defer' flag.  You mention it is
> for compat reasons, but since there are no users I'm not seeing how
> there can be compatibility concerns.  Can you elaborate?

Yes, I agree.  I don't think there's a compatibility concern here - with
the old interface, a non-present resource would be indicated as "does not
exist".

If we convert any error pointer including the one representing
-EPROBE_DEFER to the "does not exist" representation, we're not changing
the behaviour.
Dan Williams Nov. 25, 2013, 7:28 p.m. UTC | #6
On Mon, Nov 25, 2013 at 11:00 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> I suppose an alternative would be to remove that flag, and have the loop
> in of_dma_request_slave_channel() initially ignore any unregistered DMA
> controllers, and still continue to look through the property for any
> alternative controller, and return a channel from one if it is found.
> Then, at the very end of the function, we could always return
> -EPROBE_DEFER if any unregistered DMA controllers were found, otherwise
> return -ENODEV. That would keep compatible behaviour, but it would mean
> that device probe order would/could influence which dmas entry provided
> the channel, since some entries might be ignored based simply on
> timing/ordering of DMA controller registration. Is that acceptable?
>

Yes, I think this option makes the most sense, and is just as
susceptible to probe order as the current implementation.
Stephen Warren Nov. 25, 2013, 7:30 p.m. UTC | #7
On 11/25/2013 12:28 PM, Dan Williams wrote:
> On Mon, Nov 25, 2013 at 11:00 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> I suppose an alternative would be to remove that flag, and have the loop
>> in of_dma_request_slave_channel() initially ignore any unregistered DMA
>> controllers, and still continue to look through the property for any
>> alternative controller, and return a channel from one if it is found.
>> Then, at the very end of the function, we could always return
>> -EPROBE_DEFER if any unregistered DMA controllers were found, otherwise
>> return -ENODEV. That would keep compatible behaviour, but it would mean
>> that device probe order would/could influence which dmas entry provided
>> the channel, since some entries might be ignored based simply on
>> timing/ordering of DMA controller registration. Is that acceptable?
>>
> 
> Yes, I think this option makes the most sense, and is just as
> susceptible to probe order as the current implementation.

OK great. Last two questions then:

1) Do you want me to revert the changes to acpi-dma.c, and simply handle
the return value conversion inside __dma_request_slave_channel().

2) What's the final call on the new API name?

Just let me know on both - the changes are simple. Thanks.
Dan Williams Nov. 25, 2013, 7:45 p.m. UTC | #8
On Mon, Nov 25, 2013 at 11:30 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 11/25/2013 12:28 PM, Dan Williams wrote:
>> On Mon, Nov 25, 2013 at 11:00 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> I suppose an alternative would be to remove that flag, and have the loop
>>> in of_dma_request_slave_channel() initially ignore any unregistered DMA
>>> controllers, and still continue to look through the property for any
>>> alternative controller, and return a channel from one if it is found.
>>> Then, at the very end of the function, we could always return
>>> -EPROBE_DEFER if any unregistered DMA controllers were found, otherwise
>>> return -ENODEV. That would keep compatible behaviour, but it would mean
>>> that device probe order would/could influence which dmas entry provided
>>> the channel, since some entries might be ignored based simply on
>>> timing/ordering of DMA controller registration. Is that acceptable?
>>>
>>
>> Yes, I think this option makes the most sense, and is just as
>> susceptible to probe order as the current implementation.
>
> OK great. Last two questions then:
>
> 1) Do you want me to revert the changes to acpi-dma.c, and simply handle
> the return value conversion inside __dma_request_slave_channel().

Yeah, no need to spread the complexity further than necessary.

> 2) What's the final call on the new API name?

What do you think of _reason?  I just read the existence of
dma_request_slave_channel_or_err() as an implication that
dma_request_slave_channel() may not fail.

> Just let me know on both - the changes are simple. Thanks.

Well thanks for hashing this back and forth, we can laugh about it
over a drink at the next conference.
Stephen Warren Nov. 25, 2013, 7:47 p.m. UTC | #9
On 11/25/2013 12:45 PM, Dan Williams wrote:
> On Mon, Nov 25, 2013 at 11:30 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:

>> 2) What's the final call on the new API name?
> 
> What do you think of _reason?  I just read the existence of
> dma_request_slave_channel_or_err() as an implication that
> dma_request_slave_channel() may not fail.

OK, _reason makes good sense. Thanks.

>> Just let me know on both - the changes are simple. Thanks.
> 
> Well thanks for hashing this back and forth, we can laugh about it
> over a drink at the next conference.

:-)
diff mbox

Patch

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");

...does not need to convert it to NULL.

Nor this one...
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;