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 <[email protected]>
---
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 8266893..2882403 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;
-
- chan = ofdma->of_dma_xlate(&dma_spec, ofdma);
+ if (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);
--
1.8.0
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 non 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 <[email protected]>
---
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 2882403..7aa0864 100644
--- a/drivers/dma/of-dma.c
+++ b/drivers/dma/of-dma.c
@@ -13,38 +13,31 @@
#include <linux/device.h>
#include <linux/err.h>
#include <linux/module.h>
-#include <linux/rculist.h>
+#include <linux/mutex.h>
#include <linux/slab.h>
#include <linux/of.h>
#include <linux/of_dma.h>
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);
@@ -53,22 +46,6 @@ static struct of_dma *of_dma_get_controller(struct of_phandle_args *dma_spec)
}
/**
- * 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
* @of_dma_xlate: translation function which converts a phandle
@@ -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 ce6a8ab..364dda7 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,
--
1.8.0
On Friday 19 April 2013, Lars-Peter Clausen wrote:
> 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 <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
On Friday 19 April 2013, Lars-Peter Clausen wrote:
> This is rather ugly and unnecessary and non 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 <[email protected]>
I guess we could also fix it by turning the reference count into a proper
kref with kref_put calling the destructor, but your solution seems to solve
the problem just as well.
Acked-by: Arnd Bergmann <[email protected]>
On 04/19/2013 12:13 PM, Arnd Bergmann wrote:
> On Friday 19 April 2013, Lars-Peter Clausen wrote:
>
>> This is rather ugly and unnecessary and non 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 <[email protected]>
>
> I guess we could also fix it by turning the reference count into a proper
> kref with kref_put calling the destructor, but your solution seems to solve
> the problem just as well.
Well, It's not only about the of_dma struct, e.g. the of_dma's of_dma_data
field usually points to some driver allocated data. If we do not block in
of_dma_controller_free() this data might be freed while the controller is
still in use.
On Friday 19 April 2013, Lars-Peter Clausen wrote:
> Well, It's not only about the of_dma struct, e.g. the of_dma's of_dma_data
> field usually points to some driver allocated data. If we do not block in
> of_dma_controller_free() this data might be freed while the controller is
> still in use.
Right, makes sense. This could of course be worked around using a destructor
in the of_dma struct, but your approach is much simpler.
Arnd
On 04/19/2013 05:10 AM, Arnd Bergmann wrote:
> On Friday 19 April 2013, Lars-Peter Clausen wrote:
>> 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 <[email protected]>
>
> Acked-by: Arnd Bergmann <[email protected]>
Thanks! FWIW ...
Reviewed-by: Jon Hunter <[email protected]>
Cheers
Jon
On 04/19/2013 04:42 AM, Lars-Peter Clausen wrote:
> 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 non 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.
Change also means that of_dma_request_slave_channel() cannot be called
from a context where it is not possible to sleep too, right? May be
worth mentioning this in the changelog as well.
> Also rename of_dma_get_controller back to of_dma_find_controller.
>
> Signed-off-by: Lars-Peter Clausen <[email protected]>
> ---
> 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 2882403..7aa0864 100644
> --- a/drivers/dma/of-dma.c
> +++ b/drivers/dma/of-dma.c
> @@ -13,38 +13,31 @@
> #include <linux/device.h>
> #include <linux/err.h>
> #include <linux/module.h>
> -#include <linux/rculist.h>
> +#include <linux/mutex.h>
> #include <linux/slab.h>
> #include <linux/of.h>
> #include <linux/of_dma.h>
>
> 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);
> @@ -53,22 +46,6 @@ static struct of_dma *of_dma_get_controller(struct of_phandle_args *dma_spec)
> }
>
> /**
> - * 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
> * @of_dma_xlate: translation function which converts a phandle
> @@ -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);
I think that there is a problem here. For controllers using the
of_dma_simple_xlate(), this will call dma_request_channel() which also
uses a mutex.
Cheers
Jon
On Saturday 20 April 2013, Jon Hunter wrote:
> Change also means that of_dma_request_slave_channel() cannot be called
> from a context where it is not possible to sleep too, right? May be
> worth mentioning this in the changelog as well.
You already cannot do that, because it requires dma_list_mutex.
Arnd
On 04/20/2013 12:45 AM, Jon Hunter wrote:
[...]
>> @@ -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);
>
> I think that there is a problem here. For controllers using the
> of_dma_simple_xlate(), this will call dma_request_channel() which also
> uses a mutex.
That would only be a problem if it'd use the same mutex. Holding two mutexes at
the same time is not a problem per se.
- Lars
On Saturday 20 April 2013, Lars-Peter Clausen wrote:
> On 04/20/2013 12:45 AM, Jon Hunter wrote:
> > I think that there is a problem here. For controllers using the
> > of_dma_simple_xlate(), this will call dma_request_channel() which also
> > uses a mutex.
>
> That would only be a problem if it'd use the same mutex. Holding two mutexes at
> the same time is not a problem per se.
>
I guess Jon originlly tried it with a spinlock as the outer lock, which indeed
does not work, but the new version does not have this problem.
Arnd
On 04/20/2013 05:38 AM, Arnd Bergmann wrote:
> On Saturday 20 April 2013, Lars-Peter Clausen wrote:
>> On 04/20/2013 12:45 AM, Jon Hunter wrote:
>>> I think that there is a problem here. For controllers using the
>>> of_dma_simple_xlate(), this will call dma_request_channel() which also
>>> uses a mutex.
>>
>> That would only be a problem if it'd use the same mutex. Holding two mutexes at
>> the same time is not a problem per se.
>>
>
> I guess Jon originlly tried it with a spinlock as the outer lock, which indeed
> does not work, but the new version does not have this problem.
Yes the spinlock was a problem and hence for the put/get functions. Ok
so that's fine then.
Cheers
Jon
On 04/19/2013 06:13 PM, Arnd Bergmann wrote:
> On Saturday 20 April 2013, Jon Hunter wrote:
>> Change also means that of_dma_request_slave_channel() cannot be called
>> from a context where it is not possible to sleep too, right? May be
>> worth mentioning this in the changelog as well.
>
> You already cannot do that, because it requires dma_list_mutex.
Right in the case where you use of_dma_simple_xlate(). However, someone
could implement whatever they wanted for the xlate. Probably not likely
as it needs to return struct dma_chan, but could be possible. That's
all. Not a big deal.
Cheers
Jon
On Fri, Apr 19, 2013 at 11:42:13AM +0200, Lars-Peter Clausen wrote:
> 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 <[email protected]>
Applied both with minor typo fix in changelog for second.
Thanks
~Vinod
> ---
> 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 8266893..2882403 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;
> -
> - chan = ofdma->of_dma_xlate(&dma_spec, ofdma);
> + if (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);
>
> --
> 1.8.0
>