| Message ID | CAA9_cmfMtp4WjRGVzfTxrwcNWP95+HE7DAYg_GeWfqi=L2K2aw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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?
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?
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.
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.
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.
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.
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.
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 --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;