On Mon, Nov 25, 2013 at 9:26 AM, Stephen Warren <swarren@xxxxxxxxxxxxx> 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:
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;
goto err;
...but need to go back and see if this can be cleaned up to not invent
the error code here.
--
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
