From bef29ec508e58bf8b9ec0915de5b0739fb800c91 Mon Sep 17 00:00:00 2001 From: Markus Pargmann Date: Sun, 24 Feb 2013 16:36:09 +0100 Subject: [PATCH 1/5] DMA: of: Constant names No DMA of-function alters the name, so this patch changes the name arguments to be constant. Most drivers will probably request DMA channels using a constant name. Signed-off-by: Markus Pargmann Signed-off-by: Vinod Koul --- drivers/dma/dmaengine.c | 2 +- drivers/dma/of-dma.c | 6 +++--- include/linux/dmaengine.h | 4 ++-- include/linux/of_dma.h | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c index b2728d6ba2fd..2cbfefea93ee 100644 --- a/drivers/dma/dmaengine.c +++ b/drivers/dma/dmaengine.c @@ -555,7 +555,7 @@ EXPORT_SYMBOL_GPL(__dma_request_channel); * @dev: pointer to client device structure * @name: slave channel name */ -struct dma_chan *dma_request_slave_channel(struct device *dev, char *name) +struct dma_chan *dma_request_slave_channel(struct device *dev, const char *name) { /* If device-tree is present get slave info from here */ if (dev->of_node) diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c index 69d04d28b1ef..6036cd08e222 100644 --- a/drivers/dma/of-dma.c +++ b/drivers/dma/of-dma.c @@ -172,8 +172,8 @@ EXPORT_SYMBOL_GPL(of_dma_controller_free); * specifiers, matches the name provided. Returns 0 if the name matches and * a valid pointer to the DMA specifier is found. Otherwise returns -ENODEV. */ -static int of_dma_match_channel(struct device_node *np, char *name, int index, - struct of_phandle_args *dma_spec) +static int of_dma_match_channel(struct device_node *np, const char *name, + int index, struct of_phandle_args *dma_spec) { const char *s; @@ -198,7 +198,7 @@ static int of_dma_match_channel(struct device_node *np, char *name, int index, * Returns pointer to appropriate dma channel on success or NULL on error. */ struct dma_chan *of_dma_request_slave_channel(struct device_node *np, - char *name) + const char *name) { struct of_phandle_args dma_spec; struct of_dma *ofdma; diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 91ac8da25020..274071ca6f04 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -968,7 +968,7 @@ enum dma_status dma_sync_wait(struct dma_chan *chan, dma_cookie_t cookie); enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx); void dma_issue_pending_all(void); struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param); -struct dma_chan *dma_request_slave_channel(struct device *dev, char *name); +struct dma_chan *dma_request_slave_channel(struct device *dev, const char *name); void dma_release_channel(struct dma_chan *chan); #else static inline enum dma_status dma_wait_for_async_tx(struct dma_async_tx_descriptor *tx) @@ -984,7 +984,7 @@ static inline struct dma_chan *__dma_request_channel(dma_cap_mask_t *mask, return NULL; } static inline struct dma_chan *dma_request_slave_channel(struct device *dev, - char *name) + const char *name) { return NULL; } diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h index d15073e080dd..ce6a8ab3d2bb 100644 --- a/include/linux/of_dma.h +++ b/include/linux/of_dma.h @@ -40,7 +40,7 @@ extern int of_dma_controller_register(struct device_node *np, void *data); extern int of_dma_controller_free(struct device_node *np); extern struct dma_chan *of_dma_request_slave_channel(struct device_node *np, - char *name); + const char *name); extern struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec, struct of_dma *ofdma); #else @@ -58,7 +58,7 @@ static inline int of_dma_controller_free(struct device_node *np) } static inline struct dma_chan *of_dma_request_slave_channel(struct device_node *np, - char *name) + const char *name) { return NULL; } From 9a188eb126aa7bf27077ee46fcb914898d6fc281 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Fri, 15 Mar 2013 14:18:20 +0530 Subject: [PATCH 2/5] DMA: OF: Check properties value before running be32_to_cpup() on it In of_dma_controller_register() routine we are calling of_get_property() as an parameter to be32_to_cpup(). In case the property doesn't exist we will get a crash. This patch changes this code to check if we got a valid property first and then runs be32_to_cpup() on it. Signed-off-by: Viresh Kumar Signed-off-by: Vinod Koul --- drivers/dma/of-dma.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c index 6036cd08e222..00db454f70d3 100644 --- a/drivers/dma/of-dma.c +++ b/drivers/dma/of-dma.c @@ -93,6 +93,7 @@ int of_dma_controller_register(struct device_node *np, { struct of_dma *ofdma; int nbcells; + const __be32 *prop; if (!np || !of_dma_xlate) { pr_err("%s: not enough information provided\n", __func__); @@ -103,8 +104,11 @@ int of_dma_controller_register(struct device_node *np, if (!ofdma) return -ENOMEM; - nbcells = be32_to_cpup(of_get_property(np, "#dma-cells", NULL)); - if (!nbcells) { + prop = of_get_property(np, "#dma-cells", NULL); + if (prop) + nbcells = be32_to_cpup(prop); + + if (!prop || !nbcells) { pr_err("%s: #dma-cells property is missing or invalid\n", __func__); kfree(ofdma); From af31826d9b1ce432e033f0e91529aa1013076482 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Mon, 25 Mar 2013 14:24:22 +0100 Subject: [PATCH 3/5] dma: of: Remove unnecessary list_empty check list_for_each_entry is able to handle empty lists just fine, there is no need to make sure that the list is non empty. Signed-off-by: Lars-Peter Clausen Signed-off-by: Vinod Koul --- drivers/dma/of-dma.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c index 00db454f70d3..8266893fef45 100644 --- a/drivers/dma/of-dma.c +++ b/drivers/dma/of-dma.c @@ -36,11 +36,6 @@ static struct of_dma *of_dma_get_controller(struct of_phandle_args *dma_spec) spin_lock(&of_dma_lock); - if (list_empty(&of_dma_list)) { - spin_unlock(&of_dma_lock); - return NULL; - } - list_for_each_entry(ofdma, &of_dma_list, of_dma_controllers) if ((ofdma->of_node == dma_spec->np) && (ofdma->of_dma_nbcells == dma_spec->args_count)) { From f22eb1402244885126c4263eb36b857e4182dd6f Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Fri, 19 Apr 2013 11:42:13 +0200 Subject: [PATCH 4/5] dma: of: Fix of_node reference leak of_dma_request_slave_channel() currently does not drop the reference to the dma_spec of_node if no DMA controller matching the of_node could be found. This patch fixes it by always calling of_node_put(). Signed-off-by: Lars-Peter Clausen Acked-by: Arnd Bergmann Reviewed-by: Jon Hunter Signed-off-by: Vinod Koul --- drivers/dma/of-dma.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c index 8266893fef45..2882403a39cf 100644 --- a/drivers/dma/of-dma.c +++ b/drivers/dma/of-dma.c @@ -221,12 +221,13 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np, ofdma = of_dma_get_controller(&dma_spec); - if (!ofdma) - continue; + if (ofdma) { + chan = ofdma->of_dma_xlate(&dma_spec, ofdma); - chan = ofdma->of_dma_xlate(&dma_spec, ofdma); - - of_dma_put_controller(ofdma); + of_dma_put_controller(ofdma); + } else { + chan = NULL; + } of_node_put(dma_spec.np); From de61608acf89779c8831aaa1428b6975d49d98c0 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Fri, 19 Apr 2013 11:42:14 +0200 Subject: [PATCH 5/5] dma:of: Use a mutex to protect the of_dma_list Currently the OF DMA code uses a spin lock to protect the of_dma_list from concurrent access and a per controller reference count to protect the controller from being freed while a request operation is in progress. If of_dma_controller_free() is called for a controller who's reference count is not zero it will return -EBUSY and not remove the controller. This is fine up until here, but leaves the question what the caller of of_dma_controller_free() is supposed to do if the controller couldn't be freed. The only viable solution for the caller is to spin on of_dma_controller_free() until it returns success. E.g. do { ret = of_dma_controller_free(dev->of_node) } while (ret != -EBUSY); This is rather ugly and unnecessary and none of the current users of of_dma_controller_free() check it's return value anyway. Instead protect the list by a mutex. The mutex will be held as long as a request operation is in progress. So if of_dma_controller_free() is called while a request operation is in progress it will be put to sleep and only wake up once the request operation has finished. This means that it is no longer possible to register or unregister OF DMA controllers from a context where it's not possible to sleep. But I doubt that we'll ever need this. Also rename of_dma_get_controller back to of_dma_find_controller. Signed-off-by: Lars-Peter Clausen Acked-by: Arnd Bergmann Signed-off-by: Vinod Koul --- drivers/dma/of-dma.c | 76 +++++++++++------------------------------- include/linux/of_dma.h | 6 ++-- 2 files changed, 22 insertions(+), 60 deletions(-) diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c index 2882403a39cf..7aa0864cd487 100644 --- a/drivers/dma/of-dma.c +++ b/drivers/dma/of-dma.c @@ -13,38 +13,31 @@ #include #include #include -#include +#include #include #include #include static LIST_HEAD(of_dma_list); -static DEFINE_SPINLOCK(of_dma_lock); +static DEFINE_MUTEX(of_dma_lock); /** - * of_dma_get_controller - Get a DMA controller in DT DMA helpers list + * of_dma_find_controller - Get a DMA controller in DT DMA helpers list * @dma_spec: pointer to DMA specifier as found in the device tree * * Finds a DMA controller with matching device node and number for dma cells - * in a list of registered DMA controllers. If a match is found the use_count - * variable is increased and a valid pointer to the DMA data stored is retuned. - * A NULL pointer is returned if no match is found. + * in a list of registered DMA controllers. If a match is found a valid pointer + * to the DMA data stored is retuned. A NULL pointer is returned if no match is + * found. */ -static struct of_dma *of_dma_get_controller(struct of_phandle_args *dma_spec) +static struct of_dma *of_dma_find_controller(struct of_phandle_args *dma_spec) { struct of_dma *ofdma; - spin_lock(&of_dma_lock); - list_for_each_entry(ofdma, &of_dma_list, of_dma_controllers) if ((ofdma->of_node == dma_spec->np) && - (ofdma->of_dma_nbcells == dma_spec->args_count)) { - ofdma->use_count++; - spin_unlock(&of_dma_lock); + (ofdma->of_dma_nbcells == dma_spec->args_count)) return ofdma; - } - - spin_unlock(&of_dma_lock); pr_debug("%s: can't find DMA controller %s\n", __func__, dma_spec->np->full_name); @@ -52,22 +45,6 @@ static struct of_dma *of_dma_get_controller(struct of_phandle_args *dma_spec) return NULL; } -/** - * of_dma_put_controller - Decrement use count for a registered DMA controller - * @of_dma: pointer to DMA controller data - * - * Decrements the use_count variable in the DMA data structure. This function - * should be called only when a valid pointer is returned from - * of_dma_get_controller() and no further accesses to data referenced by that - * pointer are needed. - */ -static void of_dma_put_controller(struct of_dma *ofdma) -{ - spin_lock(&of_dma_lock); - ofdma->use_count--; - spin_unlock(&of_dma_lock); -} - /** * of_dma_controller_register - Register a DMA controller to DT DMA helpers * @np: device node of DMA controller @@ -114,12 +91,11 @@ int of_dma_controller_register(struct device_node *np, ofdma->of_dma_nbcells = nbcells; ofdma->of_dma_xlate = of_dma_xlate; ofdma->of_dma_data = data; - ofdma->use_count = 0; /* Now queue of_dma controller structure in list */ - spin_lock(&of_dma_lock); + mutex_lock(&of_dma_lock); list_add_tail(&ofdma->of_dma_controllers, &of_dma_list); - spin_unlock(&of_dma_lock); + mutex_unlock(&of_dma_lock); return 0; } @@ -131,32 +107,20 @@ EXPORT_SYMBOL_GPL(of_dma_controller_register); * * Memory allocated by of_dma_controller_register() is freed here. */ -int of_dma_controller_free(struct device_node *np) +void of_dma_controller_free(struct device_node *np) { struct of_dma *ofdma; - spin_lock(&of_dma_lock); - - if (list_empty(&of_dma_list)) { - spin_unlock(&of_dma_lock); - return -ENODEV; - } + mutex_lock(&of_dma_lock); list_for_each_entry(ofdma, &of_dma_list, of_dma_controllers) if (ofdma->of_node == np) { - if (ofdma->use_count) { - spin_unlock(&of_dma_lock); - return -EBUSY; - } - list_del(&ofdma->of_dma_controllers); - spin_unlock(&of_dma_lock); kfree(ofdma); - return 0; + break; } - spin_unlock(&of_dma_lock); - return -ENODEV; + mutex_unlock(&of_dma_lock); } EXPORT_SYMBOL_GPL(of_dma_controller_free); @@ -219,15 +183,15 @@ struct dma_chan *of_dma_request_slave_channel(struct device_node *np, if (of_dma_match_channel(np, name, i, &dma_spec)) continue; - ofdma = of_dma_get_controller(&dma_spec); + mutex_lock(&of_dma_lock); + ofdma = of_dma_find_controller(&dma_spec); - if (ofdma) { + if (ofdma) chan = ofdma->of_dma_xlate(&dma_spec, ofdma); - - of_dma_put_controller(ofdma); - } else { + else chan = NULL; - } + + mutex_unlock(&of_dma_lock); of_node_put(dma_spec.np); diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h index ce6a8ab3d2bb..364dda734877 100644 --- a/include/linux/of_dma.h +++ b/include/linux/of_dma.h @@ -25,7 +25,6 @@ struct of_dma { struct dma_chan *(*of_dma_xlate) (struct of_phandle_args *, struct of_dma *); void *of_dma_data; - int use_count; }; struct of_dma_filter_info { @@ -38,7 +37,7 @@ extern int of_dma_controller_register(struct device_node *np, struct dma_chan *(*of_dma_xlate) (struct of_phandle_args *, struct of_dma *), void *data); -extern int of_dma_controller_free(struct device_node *np); +extern void of_dma_controller_free(struct device_node *np); extern struct dma_chan *of_dma_request_slave_channel(struct device_node *np, const char *name); extern struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec, @@ -52,9 +51,8 @@ static inline int of_dma_controller_register(struct device_node *np, return -ENODEV; } -static inline int of_dma_controller_free(struct device_node *np) +static inline void of_dma_controller_free(struct device_node *np) { - return -ENODEV; } static inline struct dma_chan *of_dma_request_slave_channel(struct device_node *np,