Re: [PATCH V2 4/7] cpufreq: arm_big_little: don't initialize opp table

[Inderpal Singh]

On Wed, May 21, 2014 at 4:40 PM, Viresh Kumar  wrote:
> OPP tables are already initialized for all CPUs by cpufreq core and so we 
> don't
> need to reinitialize them from arm_big_little_dt driver.
>

I guess you wanted to say "cpu core code" instead of "cpufreq core".
Correction may be needed here and in the subsequent patches as well.


> Also, as the arm_big_little_dt driver doesn't have a .init_opp_table() 
> callback
> anymore, make it optional in arm_big_little driver.
>
> Cc: Sudeep Holla 
> Signed-off-by: Viresh Kumar 
> ---
>  drivers/cpufreq/arm_big_little.c| 12 +++-
>  drivers/cpufreq/arm_big_little_dt.c | 18 --
>  2 files changed, 7 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/cpufreq/arm_big_little.c 
> b/drivers/cpufreq/arm_big_little.c
> index 1f4d4e3..561261e 100644
> --- a/drivers/cpufreq/arm_big_little.c
> +++ b/drivers/cpufreq/arm_big_little.c
> @@ -325,11 +325,13 @@ static int _get_cluster_clk_and_freq_table(struct 
> device *cpu_dev)
> if (freq_table[cluster])
> return 0;
>
> -   ret = arm_bL_ops->init_opp_table(cpu_dev);
> -   if (ret) {
> -   dev_err(cpu_dev, "%s: init_opp_table failed, cpu: %d, err: 
> %d\n",
> +   if (arm_bL_ops->init_opp_table) {
> +   ret = arm_bL_ops->init_opp_table(cpu_dev);
> +   if (ret) {
> +   dev_err(cpu_dev, "%s: init_opp_table failed, cpu: %d, 
> err: %d\n",
> __func__, cpu_dev->id, ret);
> -   goto out;
> +   goto out;
> +   }
> }
>
> ret = dev_pm_opp_init_cpufreq_table(cpu_dev, _table[cluster]);
> @@ -542,7 +544,7 @@ int bL_cpufreq_register(struct cpufreq_arm_bL_ops *ops)
> return -EBUSY;
> }
>
> -   if (!ops || !strlen(ops->name) || !ops->init_opp_table) {
> +   if (!ops || !strlen(ops->name)) {
> pr_err("%s: Invalid arm_bL_ops, exiting\n", __func__);
> return -ENODEV;
> }
> diff --git a/drivers/cpufreq/arm_big_little_dt.c 
> b/drivers/cpufreq/arm_big_little_dt.c
> index 8d9d591..502182d 100644
> --- a/drivers/cpufreq/arm_big_little_dt.c
> +++ b/drivers/cpufreq/arm_big_little_dt.c
> @@ -43,23 +43,6 @@ static struct device_node *get_cpu_node_with_valid_op(int 
> cpu)
> return np;
>  }
>
> -static int dt_init_opp_table(struct device *cpu_dev)
> -{
> -   struct device_node *np;
> -   int ret;
> -
> -   np = of_node_get(cpu_dev->of_node);
> -   if (!np) {
> -   pr_err("failed to find cpu%d node\n", cpu_dev->id);
> -   return -ENOENT;
> -   }
> -
> -   ret = of_init_opp_table(cpu_dev);
> -   of_node_put(np);
> -
> -   return ret;
> -}
> -
>  static int dt_get_transition_latency(struct device *cpu_dev)
>  {
> struct device_node *np;
> @@ -81,7 +64,6 @@ static int dt_get_transition_latency(struct device *cpu_dev)
>  static struct cpufreq_arm_bL_ops dt_bL_ops = {
> .name   = "dt-bl",
> .get_transition_latency = dt_get_transition_latency,
> -   .init_opp_table = dt_init_opp_table,
>  };
>
>  static int generic_bL_probe(struct platform_device *pdev)
> --
> 2.0.0.rc2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] PM / OPP: Implement opp_remove and free_opp_table functions

[Inderpal Singh]

On Wed, May 21, 2014 at 1:25 PM, Viresh Kumar  wrote:
> On 21 May 2014 11:21, Inderpal Singh  wrote:
>> At the driver unloading time the associated opps and its table may need
>> to be deleted. Otherwise it amounts to memory leak. The existing
>> OPP library does not have provision to do so.
>>
>> Hence this patch implements the required functions to free the same.
>>
>> Signed-off-by: Inderpal Singh 
>> ---
>> Changes in v2:
>> - As suggested by Nishanth, added support to remove a single OPP
>>   for non-DT users
>> - Added an internal helper function to avoid the code duplication
>>   between opp_remove and free_opp_table functions
>>
>>  drivers/base/power/opp.c | 122 
>> ++-
>>  include/linux/pm_opp.h   |  14 +-
>>  2 files changed, 134 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>> index faae9cf..22adef3 100644
>> --- a/drivers/base/power/opp.c
>> +++ b/drivers/base/power/opp.c
>> @@ -89,6 +89,7 @@ struct device_opp {
>> struct device *dev;
>> struct srcu_notifier_head head;
>> struct list_head opp_list;
>> +   struct rcu_head head_rcu;
>
> This isn't used ...

Its being used in opp_remove_and_free function to free dev_opp with kfree_rcu.

>
>>  };
>>
>>  /*
>> @@ -427,7 +428,6 @@ int dev_pm_opp_add(struct device *dev, unsigned long 
>> freq, unsigned long u_volt)
>> __func__);
>> return -ENOMEM;
>> }
>> -
>
> Unrelated change.

Ahhh, missed it. Thanks for pointing.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2] PM / OPP: Implement opp_remove and free_opp_table functions

[Inderpal Singh]

On Wed, May 21, 2014 at 1:25 PM, Viresh Kumar viresh.ku...@linaro.org wrote:
 On 21 May 2014 11:21, Inderpal Singh inderpa...@samsung.com wrote:
 At the driver unloading time the associated opps and its table may need
 to be deleted. Otherwise it amounts to memory leak. The existing
 OPP library does not have provision to do so.

 Hence this patch implements the required functions to free the same.

 Signed-off-by: Inderpal Singh inderpa...@samsung.com
 ---
 Changes in v2:
 - As suggested by Nishanth, added support to remove a single OPP
   for non-DT users
 - Added an internal helper function to avoid the code duplication
   between opp_remove and free_opp_table functions

  drivers/base/power/opp.c | 122 
 ++-
  include/linux/pm_opp.h   |  14 +-
  2 files changed, 134 insertions(+), 2 deletions(-)

 diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
 index faae9cf..22adef3 100644
 --- a/drivers/base/power/opp.c
 +++ b/drivers/base/power/opp.c
 @@ -89,6 +89,7 @@ struct device_opp {
 struct device *dev;
 struct srcu_notifier_head head;
 struct list_head opp_list;
 +   struct rcu_head head_rcu;

 This isn't used ...

Its being used in opp_remove_and_free function to free dev_opp with kfree_rcu.


  };

  /*
 @@ -427,7 +428,6 @@ int dev_pm_opp_add(struct device *dev, unsigned long 
 freq, unsigned long u_volt)
 __func__);
 return -ENOMEM;
 }
 -

 Unrelated change.

Ahhh, missed it. Thanks for pointing.

 --
 To unsubscribe from this list: send the line unsubscribe linux-pm in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2 4/7] cpufreq: arm_big_little: don't initialize opp table

[Inderpal Singh]

On Wed, May 21, 2014 at 4:40 PM, Viresh Kumar viresh.ku...@linaro.org wrote:
 OPP tables are already initialized for all CPUs by cpufreq core and so we 
 don't
 need to reinitialize them from arm_big_little_dt driver.


I guess you wanted to say cpu core code instead of cpufreq core.
Correction may be needed here and in the subsequent patches as well.


 Also, as the arm_big_little_dt driver doesn't have a .init_opp_table() 
 callback
 anymore, make it optional in arm_big_little driver.

 Cc: Sudeep Holla sudeep.ho...@arm.com
 Signed-off-by: Viresh Kumar viresh.ku...@linaro.org
 ---
  drivers/cpufreq/arm_big_little.c| 12 +++-
  drivers/cpufreq/arm_big_little_dt.c | 18 --
  2 files changed, 7 insertions(+), 23 deletions(-)

 diff --git a/drivers/cpufreq/arm_big_little.c 
 b/drivers/cpufreq/arm_big_little.c
 index 1f4d4e3..561261e 100644
 --- a/drivers/cpufreq/arm_big_little.c
 +++ b/drivers/cpufreq/arm_big_little.c
 @@ -325,11 +325,13 @@ static int _get_cluster_clk_and_freq_table(struct 
 device *cpu_dev)
 if (freq_table[cluster])
 return 0;

 -   ret = arm_bL_ops-init_opp_table(cpu_dev);
 -   if (ret) {
 -   dev_err(cpu_dev, %s: init_opp_table failed, cpu: %d, err: 
 %d\n,
 +   if (arm_bL_ops-init_opp_table) {
 +   ret = arm_bL_ops-init_opp_table(cpu_dev);
 +   if (ret) {
 +   dev_err(cpu_dev, %s: init_opp_table failed, cpu: %d, 
 err: %d\n,
 __func__, cpu_dev-id, ret);
 -   goto out;
 +   goto out;
 +   }
 }

 ret = dev_pm_opp_init_cpufreq_table(cpu_dev, freq_table[cluster]);
 @@ -542,7 +544,7 @@ int bL_cpufreq_register(struct cpufreq_arm_bL_ops *ops)
 return -EBUSY;
 }

 -   if (!ops || !strlen(ops-name) || !ops-init_opp_table) {
 +   if (!ops || !strlen(ops-name)) {
 pr_err(%s: Invalid arm_bL_ops, exiting\n, __func__);
 return -ENODEV;
 }
 diff --git a/drivers/cpufreq/arm_big_little_dt.c 
 b/drivers/cpufreq/arm_big_little_dt.c
 index 8d9d591..502182d 100644
 --- a/drivers/cpufreq/arm_big_little_dt.c
 +++ b/drivers/cpufreq/arm_big_little_dt.c
 @@ -43,23 +43,6 @@ static struct device_node *get_cpu_node_with_valid_op(int 
 cpu)
 return np;
  }

 -static int dt_init_opp_table(struct device *cpu_dev)
 -{
 -   struct device_node *np;
 -   int ret;
 -
 -   np = of_node_get(cpu_dev-of_node);
 -   if (!np) {
 -   pr_err(failed to find cpu%d node\n, cpu_dev-id);
 -   return -ENOENT;
 -   }
 -
 -   ret = of_init_opp_table(cpu_dev);
 -   of_node_put(np);
 -
 -   return ret;
 -}
 -
  static int dt_get_transition_latency(struct device *cpu_dev)
  {
 struct device_node *np;
 @@ -81,7 +64,6 @@ static int dt_get_transition_latency(struct device *cpu_dev)
  static struct cpufreq_arm_bL_ops dt_bL_ops = {
 .name   = dt-bl,
 .get_transition_latency = dt_get_transition_latency,
 -   .init_opp_table = dt_init_opp_table,
  };

  static int generic_bL_probe(struct platform_device *pdev)
 --
 2.0.0.rc2

 --
 To unsubscribe from this list: send the line unsubscribe linux-pm in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2] PM / OPP: Implement opp_remove and free_opp_table functions

[Inderpal Singh]

At the driver unloading time the associated opps and its table may need
to be deleted. Otherwise it amounts to memory leak. The existing
OPP library does not have provision to do so.

Hence this patch implements the required functions to free the same.

Signed-off-by: Inderpal Singh 
---
Changes in v2:
- As suggested by Nishanth, added support to remove a single OPP
  for non-DT users
- Added an internal helper function to avoid the code duplication
  between opp_remove and free_opp_table functions
 
 drivers/base/power/opp.c | 122 ++-
 include/linux/pm_opp.h   |  14 +-
 2 files changed, 134 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index faae9cf..22adef3 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -89,6 +89,7 @@ struct device_opp {
struct device *dev;
struct srcu_notifier_head head;
struct list_head opp_list;
+   struct rcu_head head_rcu;
 };
 
 /*
@@ -427,7 +428,6 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, 
unsigned long u_volt)
__func__);
return -ENOMEM;
}
-
dev_opp->dev = dev;
srcu_init_notifier_head(_opp->head);
INIT_LIST_HEAD(_opp->opp_list);
@@ -464,6 +464,82 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, 
unsigned long u_volt)
 EXPORT_SYMBOL_GPL(dev_pm_opp_add);
 
 /**
+ * opp_remove_and_free()  - Internal helper to remove and free an OPP
+ * @dev_opp:   device_opp for which we do this operation
+ * @opp:   OPP to be removed
+ *
+ * This function removes an opp from the opp list and frees it. And, if the
+ * list is empty, it also removes the device_opp from the root list and
+ * frees it.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * The callers need to use RCU updater strategy with mutex locks to keep the
+ * integrity of the internal data structures.
+ */
+static void opp_remove_and_free(struct device_opp *dev_opp,
+   struct dev_pm_opp *opp)
+{
+   list_del_rcu(>node);
+   /*
+* Notify the removal of the opp
+*/
+   srcu_notifier_call_chain(_opp->head, OPP_EVENT_REMOVE, opp);
+   kfree_rcu(opp, head);
+
+   if (list_empty(_opp->opp_list)) {
+   list_del_rcu(_opp->node);
+   kfree_rcu(dev_opp, head_rcu);
+   }
+}
+
+/**
+ * dev_pm_opp_remove() - remove a specific OPP
+ * @dev:   device for which we do this operation
+ * @freq:  OPP frequency to remove
+ *
+ * Removes a provided opp.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks to
+ * keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex locking cannot be used.
+ */
+void dev_pm_opp_remove(struct device *dev, unsigned long freq)
+{
+   struct device_opp *dev_opp = ERR_PTR(-ENODEV);
+   struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
+
+   if (IS_ERR_OR_NULL(dev))
+   return;
+
+   /* Hold our list modification lock here */
+   mutex_lock(_opp_list_lock);
+
+   /* Check for existing list for 'dev' */
+   dev_opp = find_device_opp(dev);
+   if (IS_ERR(dev_opp))
+   goto unlock;
+
+   /* Do we have the frequency? */
+   list_for_each_entry(temp_opp, _opp->opp_list, node)
+   if (temp_opp->rate == freq) {
+   opp = temp_opp;
+   break;
+   }
+
+   if (IS_ERR(opp)) {
+   dev_warn(dev, "%s: OPP freq not found\n", __func__);
+   goto unlock;
+   }
+
+   opp_remove_and_free(dev_opp, opp);
+unlock:
+   mutex_unlock(_opp_list_lock);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
+
+/**
  * opp_set_availability() - helper to set the availability of an opp
  * @dev:   device for which we do this operation
  * @freq:  OPP frequency to modify availability
@@ -653,3 +729,47 @@ int of_init_opp_table(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
 #endif
+
+/**
+ * dev_pm_opp_free_opp_table() - free the opp table
+ * @dev:   device for which we do this operation
+ *
+ * Free up the allocated opp table
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks to
+ * keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex locking cannot be used.
+ */
+void dev_pm_opp_free_opp_table(struct device *dev)

[PATCH v2] PM / OPP: Implement opp_remove and free_opp_table functions

[Inderpal Singh]

At the driver unloading time the associated opps and its table may need
to be deleted. Otherwise it amounts to memory leak. The existing
OPP library does not have provision to do so.

Hence this patch implements the required functions to free the same.

Signed-off-by: Inderpal Singh inderpa...@samsung.com
---
Changes in v2:
- As suggested by Nishanth, added support to remove a single OPP
  for non-DT users
- Added an internal helper function to avoid the code duplication
  between opp_remove and free_opp_table functions
 
 drivers/base/power/opp.c | 122 ++-
 include/linux/pm_opp.h   |  14 +-
 2 files changed, 134 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index faae9cf..22adef3 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -89,6 +89,7 @@ struct device_opp {
struct device *dev;
struct srcu_notifier_head head;
struct list_head opp_list;
+   struct rcu_head head_rcu;
 };
 
 /*
@@ -427,7 +428,6 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, 
unsigned long u_volt)
__func__);
return -ENOMEM;
}
-
dev_opp-dev = dev;
srcu_init_notifier_head(dev_opp-head);
INIT_LIST_HEAD(dev_opp-opp_list);
@@ -464,6 +464,82 @@ int dev_pm_opp_add(struct device *dev, unsigned long freq, 
unsigned long u_volt)
 EXPORT_SYMBOL_GPL(dev_pm_opp_add);
 
 /**
+ * opp_remove_and_free()  - Internal helper to remove and free an OPP
+ * @dev_opp:   device_opp for which we do this operation
+ * @opp:   OPP to be removed
+ *
+ * This function removes an opp from the opp list and frees it. And, if the
+ * list is empty, it also removes the device_opp from the root list and
+ * frees it.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * The callers need to use RCU updater strategy with mutex locks to keep the
+ * integrity of the internal data structures.
+ */
+static void opp_remove_and_free(struct device_opp *dev_opp,
+   struct dev_pm_opp *opp)
+{
+   list_del_rcu(opp-node);
+   /*
+* Notify the removal of the opp
+*/
+   srcu_notifier_call_chain(dev_opp-head, OPP_EVENT_REMOVE, opp);
+   kfree_rcu(opp, head);
+
+   if (list_empty(dev_opp-opp_list)) {
+   list_del_rcu(dev_opp-node);
+   kfree_rcu(dev_opp, head_rcu);
+   }
+}
+
+/**
+ * dev_pm_opp_remove() - remove a specific OPP
+ * @dev:   device for which we do this operation
+ * @freq:  OPP frequency to remove
+ *
+ * Removes a provided opp.
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks to
+ * keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex locking cannot be used.
+ */
+void dev_pm_opp_remove(struct device *dev, unsigned long freq)
+{
+   struct device_opp *dev_opp = ERR_PTR(-ENODEV);
+   struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
+
+   if (IS_ERR_OR_NULL(dev))
+   return;
+
+   /* Hold our list modification lock here */
+   mutex_lock(dev_opp_list_lock);
+
+   /* Check for existing list for 'dev' */
+   dev_opp = find_device_opp(dev);
+   if (IS_ERR(dev_opp))
+   goto unlock;
+
+   /* Do we have the frequency? */
+   list_for_each_entry(temp_opp, dev_opp-opp_list, node)
+   if (temp_opp-rate == freq) {
+   opp = temp_opp;
+   break;
+   }
+
+   if (IS_ERR(opp)) {
+   dev_warn(dev, %s: OPP freq not found\n, __func__);
+   goto unlock;
+   }
+
+   opp_remove_and_free(dev_opp, opp);
+unlock:
+   mutex_unlock(dev_opp_list_lock);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_remove);
+
+/**
  * opp_set_availability() - helper to set the availability of an opp
  * @dev:   device for which we do this operation
  * @freq:  OPP frequency to modify availability
@@ -653,3 +729,47 @@ int of_init_opp_table(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
 #endif
+
+/**
+ * dev_pm_opp_free_opp_table() - free the opp table
+ * @dev:   device for which we do this operation
+ *
+ * Free up the allocated opp table
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks to
+ * keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex locking cannot be used.
+ */
+void dev_pm_opp_free_opp_table(struct device *dev

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Inderpal Singh]

On Tue, May 20, 2014 at 12:04 AM, Nishanth Menon  wrote:
> On Mon, May 19, 2014 at 1:08 PM, Inderpal Singh  
> wrote:
>> Hi Nishanth,
>>
>> Thanks for the review comments.
>>
>> On Mon, May 19, 2014 at 6:43 PM, Nishanth Menon  wrote:
>>> On 05/16/2014 04:09 AM, Inderpal Singh wrote:
>>>> At the driver unloading time the associated opp table may need
>>>> to be deleted. Otherwise it amounts to memory leak. The existing
>>>> OPP library does not have provison to do so.
>>>>
>>>> Hence this patch implements the function to free the opp table.
>>>>
>>>> Signed-off-by: Inderpal Singh 
>>>> ---
>>>>  drivers/base/power/opp.c | 41 +
>>>>  include/linux/pm_opp.h   |  6 ++
>>>>  2 files changed, 47 insertions(+)
>>>>
>>>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>>>> index d9e376a..d45ffd5 100644
>>>> --- a/drivers/base/power/opp.c
>>>> +++ b/drivers/base/power/opp.c
>>>> @@ -654,4 +654,45 @@ int of_init_opp_table(struct device *dev)
>>>>   return 0;
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(of_init_opp_table);
>>>> +
>>>> +/**
>>>> + * dev_pm_opp_free_opp_table() - free the opp table
>>>> + * @dev: device for which we do this operation
>>>> + *
>>>> + * Free up the allocated opp table
>>>> + *
>>>> + * Locking: The internal device_opp and opp structures are RCU protected.
>>>> + * Hence this function internally uses RCU updater strategy with mutex 
>>>> locks to
>>>> + * keep the integrity of the internal data structures. Callers should 
>>>> ensure
>>>> + * that this function is *NOT* called under RCU protection or in contexts 
>>>> where
>>>> + * mutex locking or synchronize_rcu() blocking calls cannot be used.
>>>> + */
>>>> +void dev_pm_opp_free_opp_table(struct device *dev)
>>>> +{
>>>> + struct device_opp *dev_opp = NULL;
>>>> + struct dev_pm_opp *opp;
>>>> +
>>> if (!dev)
>>> return;
>>>
>>
>> missed it. Will take care in the next version.
>>
>>>> + /* Hold our list modification lock here */
>>>> + mutex_lock(_opp_list_lock);
>>>> +
>>>> + /* Check for existing list for 'dev' */
>>>> + dev_opp = find_device_opp(dev);
>>>> + if (IS_ERR(dev_opp)) {
>>>> + mutex_unlock(_opp_list_lock);
>>>> + return;
>>>> + }
>>>> +
>>>> + while (!list_empty(_opp->opp_list)) {
>>>> + opp = list_entry_rcu(dev_opp->opp_list.next,
>>>> + struct dev_pm_opp, node);
>>>> + list_del_rcu(>node);
>>>> + kfree_rcu(opp, head);
>>>> + }
>>>
>>> How about the OPP notifiers? should'nt we add a new event
>>> OPP_EVENT_REMOVE?
>>>
>>
>> As this function is to free the whole opp table. Hence, I think,
>> notifier may not be needed. It may be required for per opp removal as
>> is the case with opp addition and enable/disable. But at present there
>> are no users of these notifiers at all. Let me know your view.
>
> umm.. we do have devfreq which depends on OPPs :).

Yes, devfreq does depend on OPPs, but no devfreq driver is registering
its notifier_block to handle OPP notifications.

>
>>> To maintain non-dt behavior coherency, should'nt we rather add a
>>> opp_remove or an opp_del function?
>>
>> Yes we should have opp_remove as well, but what's the use case ?
>> Should we go ahead and implement it Or, wait for the use-case?
>
> IMHO, if we are doing it properly, we should add the requisite
> function as well. we dont want to have differing behavior device tree
> Vs non-DT.

So we will have 2 functions then. One to remove the whole opp table
and the the other to remove the individual OPPs.
I will cover this in v2. Will also take care of the OPP_EVENT_REMOVE
notification part.


Regards,
Inder

>
> Regards,
> Nishanth Menon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Inderpal Singh]

Hi Nishanth,

Thanks for the review comments.

On Mon, May 19, 2014 at 6:43 PM, Nishanth Menon  wrote:
> On 05/16/2014 04:09 AM, Inderpal Singh wrote:
>> At the driver unloading time the associated opp table may need
>> to be deleted. Otherwise it amounts to memory leak. The existing
>> OPP library does not have provison to do so.
>>
>> Hence this patch implements the function to free the opp table.
>>
>> Signed-off-by: Inderpal Singh 
>> ---
>>  drivers/base/power/opp.c | 41 +
>>  include/linux/pm_opp.h   |  6 ++
>>  2 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>> index d9e376a..d45ffd5 100644
>> --- a/drivers/base/power/opp.c
>> +++ b/drivers/base/power/opp.c
>> @@ -654,4 +654,45 @@ int of_init_opp_table(struct device *dev)
>>   return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(of_init_opp_table);
>> +
>> +/**
>> + * dev_pm_opp_free_opp_table() - free the opp table
>> + * @dev: device for which we do this operation
>> + *
>> + * Free up the allocated opp table
>> + *
>> + * Locking: The internal device_opp and opp structures are RCU protected.
>> + * Hence this function internally uses RCU updater strategy with mutex 
>> locks to
>> + * keep the integrity of the internal data structures. Callers should ensure
>> + * that this function is *NOT* called under RCU protection or in contexts 
>> where
>> + * mutex locking or synchronize_rcu() blocking calls cannot be used.
>> + */
>> +void dev_pm_opp_free_opp_table(struct device *dev)
>> +{
>> + struct device_opp *dev_opp = NULL;
>> + struct dev_pm_opp *opp;
>> +
> if (!dev)
> return;
>

missed it. Will take care in the next version.

>> + /* Hold our list modification lock here */
>> + mutex_lock(_opp_list_lock);
>> +
>> + /* Check for existing list for 'dev' */
>> + dev_opp = find_device_opp(dev);
>> + if (IS_ERR(dev_opp)) {
>> + mutex_unlock(_opp_list_lock);
>> + return;
>> + }
>> +
>> + while (!list_empty(_opp->opp_list)) {
>> + opp = list_entry_rcu(dev_opp->opp_list.next,
>> + struct dev_pm_opp, node);
>> + list_del_rcu(>node);
>> + kfree_rcu(opp, head);
>> + }
>
> How about the OPP notifiers? should'nt we add a new event
> OPP_EVENT_REMOVE?
>

As this function is to free the whole opp table. Hence, I think,
notifier may not be needed. It may be required for per opp removal as
is the case with opp addition and enable/disable. But at present there
are no users of these notifiers at all. Let me know your view.

> To maintain non-dt behavior coherency, should'nt we rather add a
> opp_remove or an opp_del function?

Yes we should have opp_remove as well, but what's the use case ?
Should we go ahead and implement it Or, wait for the use-case?

Thanks,
Inder

>
>> +
>> + list_del_rcu(_opp->node);
>> + mutex_unlock(_opp_list_lock);
>> + synchronize_rcu();
>> + kfree(dev_opp);
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_opp_free_opp_table);
>>  #endif
>> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
>> index 0330217..3c29620 100644
>> --- a/include/linux/pm_opp.h
>> +++ b/include/linux/pm_opp.h
>> @@ -50,6 +50,8 @@ int dev_pm_opp_enable(struct device *dev, unsigned long 
>> freq);
>>  int dev_pm_opp_disable(struct device *dev, unsigned long freq);
>>
>>  struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
>> +
>> +void dev_pm_opp_free_opp_table(struct device *dev);
>>  #else
>>  static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
>>  {
>> @@ -105,6 +107,10 @@ static inline struct srcu_notifier_head 
>> *dev_pm_opp_get_notifier(
>>  {
>>   return ERR_PTR(-EINVAL);
>>  }
>> +
>> +void dev_pm_opp_free_opp_table(struct device *dev)
>> +{
>> +}
>>  #endif   /* CONFIG_PM_OPP */
>>
>>  #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
>>
>
>
> --
> Regards,
> Nishanth Menon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Inderpal Singh]

On Tue, May 20, 2014 at 12:04 AM, Nishanth Menon n...@ti.com wrote:
 On Mon, May 19, 2014 at 1:08 PM, Inderpal Singh inderpa...@samsung.com 
 wrote:
 Hi Nishanth,

 Thanks for the review comments.

 On Mon, May 19, 2014 at 6:43 PM, Nishanth Menon n...@ti.com wrote:
 On 05/16/2014 04:09 AM, Inderpal Singh wrote:
 At the driver unloading time the associated opp table may need
 to be deleted. Otherwise it amounts to memory leak. The existing
 OPP library does not have provison to do so.

 Hence this patch implements the function to free the opp table.

 Signed-off-by: Inderpal Singh inderpa...@samsung.com
 ---
  drivers/base/power/opp.c | 41 +
  include/linux/pm_opp.h   |  6 ++
  2 files changed, 47 insertions(+)

 diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
 index d9e376a..d45ffd5 100644
 --- a/drivers/base/power/opp.c
 +++ b/drivers/base/power/opp.c
 @@ -654,4 +654,45 @@ int of_init_opp_table(struct device *dev)
   return 0;
  }
  EXPORT_SYMBOL_GPL(of_init_opp_table);
 +
 +/**
 + * dev_pm_opp_free_opp_table() - free the opp table
 + * @dev: device for which we do this operation
 + *
 + * Free up the allocated opp table
 + *
 + * Locking: The internal device_opp and opp structures are RCU protected.
 + * Hence this function internally uses RCU updater strategy with mutex 
 locks to
 + * keep the integrity of the internal data structures. Callers should 
 ensure
 + * that this function is *NOT* called under RCU protection or in contexts 
 where
 + * mutex locking or synchronize_rcu() blocking calls cannot be used.
 + */
 +void dev_pm_opp_free_opp_table(struct device *dev)
 +{
 + struct device_opp *dev_opp = NULL;
 + struct dev_pm_opp *opp;
 +
 if (!dev)
 return;


 missed it. Will take care in the next version.

 + /* Hold our list modification lock here */
 + mutex_lock(dev_opp_list_lock);
 +
 + /* Check for existing list for 'dev' */
 + dev_opp = find_device_opp(dev);
 + if (IS_ERR(dev_opp)) {
 + mutex_unlock(dev_opp_list_lock);
 + return;
 + }
 +
 + while (!list_empty(dev_opp-opp_list)) {
 + opp = list_entry_rcu(dev_opp-opp_list.next,
 + struct dev_pm_opp, node);
 + list_del_rcu(opp-node);
 + kfree_rcu(opp, head);
 + }

 How about the OPP notifiers? should'nt we add a new event
 OPP_EVENT_REMOVE?


 As this function is to free the whole opp table. Hence, I think,
 notifier may not be needed. It may be required for per opp removal as
 is the case with opp addition and enable/disable. But at present there
 are no users of these notifiers at all. Let me know your view.

 umm.. we do have devfreq which depends on OPPs :).

Yes, devfreq does depend on OPPs, but no devfreq driver is registering
its notifier_block to handle OPP notifications.


 To maintain non-dt behavior coherency, should'nt we rather add a
 opp_remove or an opp_del function?

 Yes we should have opp_remove as well, but what's the use case ?
 Should we go ahead and implement it Or, wait for the use-case?

 IMHO, if we are doing it properly, we should add the requisite
 function as well. we dont want to have differing behavior device tree
 Vs non-DT.

So we will have 2 functions then. One to remove the whole opp table
and the the other to remove the individual OPPs.
I will cover this in v2. Will also take care of the OPP_EVENT_REMOVE
notification part.


Regards,
Inder


 Regards,
 Nishanth Menon
 --
 To unsubscribe from this list: send the line unsubscribe linux-pm in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Inderpal Singh]

Hi Nishanth,

Thanks for the review comments.

On Mon, May 19, 2014 at 6:43 PM, Nishanth Menon n...@ti.com wrote:
 On 05/16/2014 04:09 AM, Inderpal Singh wrote:
 At the driver unloading time the associated opp table may need
 to be deleted. Otherwise it amounts to memory leak. The existing
 OPP library does not have provison to do so.

 Hence this patch implements the function to free the opp table.

 Signed-off-by: Inderpal Singh inderpa...@samsung.com
 ---
  drivers/base/power/opp.c | 41 +
  include/linux/pm_opp.h   |  6 ++
  2 files changed, 47 insertions(+)

 diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
 index d9e376a..d45ffd5 100644
 --- a/drivers/base/power/opp.c
 +++ b/drivers/base/power/opp.c
 @@ -654,4 +654,45 @@ int of_init_opp_table(struct device *dev)
   return 0;
  }
  EXPORT_SYMBOL_GPL(of_init_opp_table);
 +
 +/**
 + * dev_pm_opp_free_opp_table() - free the opp table
 + * @dev: device for which we do this operation
 + *
 + * Free up the allocated opp table
 + *
 + * Locking: The internal device_opp and opp structures are RCU protected.
 + * Hence this function internally uses RCU updater strategy with mutex 
 locks to
 + * keep the integrity of the internal data structures. Callers should ensure
 + * that this function is *NOT* called under RCU protection or in contexts 
 where
 + * mutex locking or synchronize_rcu() blocking calls cannot be used.
 + */
 +void dev_pm_opp_free_opp_table(struct device *dev)
 +{
 + struct device_opp *dev_opp = NULL;
 + struct dev_pm_opp *opp;
 +
 if (!dev)
 return;


missed it. Will take care in the next version.

 + /* Hold our list modification lock here */
 + mutex_lock(dev_opp_list_lock);
 +
 + /* Check for existing list for 'dev' */
 + dev_opp = find_device_opp(dev);
 + if (IS_ERR(dev_opp)) {
 + mutex_unlock(dev_opp_list_lock);
 + return;
 + }
 +
 + while (!list_empty(dev_opp-opp_list)) {
 + opp = list_entry_rcu(dev_opp-opp_list.next,
 + struct dev_pm_opp, node);
 + list_del_rcu(opp-node);
 + kfree_rcu(opp, head);
 + }

 How about the OPP notifiers? should'nt we add a new event
 OPP_EVENT_REMOVE?


As this function is to free the whole opp table. Hence, I think,
notifier may not be needed. It may be required for per opp removal as
is the case with opp addition and enable/disable. But at present there
are no users of these notifiers at all. Let me know your view.

 To maintain non-dt behavior coherency, should'nt we rather add a
 opp_remove or an opp_del function?

Yes we should have opp_remove as well, but what's the use case ?
Should we go ahead and implement it Or, wait for the use-case?

Thanks,
Inder


 +
 + list_del_rcu(dev_opp-node);
 + mutex_unlock(dev_opp_list_lock);
 + synchronize_rcu();
 + kfree(dev_opp);
 +}
 +EXPORT_SYMBOL_GPL(dev_pm_opp_free_opp_table);
  #endif
 diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
 index 0330217..3c29620 100644
 --- a/include/linux/pm_opp.h
 +++ b/include/linux/pm_opp.h
 @@ -50,6 +50,8 @@ int dev_pm_opp_enable(struct device *dev, unsigned long 
 freq);
  int dev_pm_opp_disable(struct device *dev, unsigned long freq);

  struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
 +
 +void dev_pm_opp_free_opp_table(struct device *dev);
  #else
  static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
  {
 @@ -105,6 +107,10 @@ static inline struct srcu_notifier_head 
 *dev_pm_opp_get_notifier(
  {
   return ERR_PTR(-EINVAL);
  }
 +
 +void dev_pm_opp_free_opp_table(struct device *dev)
 +{
 +}
  #endif   /* CONFIG_PM_OPP */

  #if defined(CONFIG_PM_OPP)  defined(CONFIG_OF)



 --
 Regards,
 Nishanth Menon
 --
 To unsubscribe from this list: send the line unsubscribe linux-pm in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Inderpal Singh]

On Fri, May 16, 2014 at 10:41 PM, Viresh Kumar  wrote:
> On 16 May 2014 22:38, Inderpal Singh  wrote:
>>>> +   while (!list_empty(_opp->opp_list)) {
>>>> +   opp = list_entry_rcu(dev_opp->opp_list.next,
>>>> +   struct dev_pm_opp, node);
>>>
>>> list_for_each_entry_rcu ?
>>>
>>
>> list_for_each_entry_rcu can not be used as opp is being deleted in the loop.
>
> So what? The above code can be replaced easily I think.
> This is how it is implemented:
>
> #define list_for_each_entry_rcu(pos, head, member) \
> for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
> >member != (head); \
> pos = list_entry_rcu(pos->member.next, typeof(*pos), member))

by the time "pos = list_entry_rcu(pos->member.
next, typeof(*pos), member)" is executed, the pos would have been
freed in the loop.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Inderpal Singh]

Hi Viresh,

Thanks for the review.

On Fri, May 16, 2014 at 3:36 PM, Viresh Kumar  wrote:
> On 16 May 2014 14:39, Inderpal Singh  wrote:
>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>> +void dev_pm_opp_free_opp_table(struct device *dev)
>> +{
>> +   struct device_opp *dev_opp = NULL;
>> +   struct dev_pm_opp *opp;
>> +
>> +   /* Hold our list modification lock here */
>> +   mutex_lock(_opp_list_lock);
>> +
>> +   /* Check for existing list for 'dev' */
>> +   dev_opp = find_device_opp(dev);
>> +   if (IS_ERR(dev_opp)) {
>> +   mutex_unlock(_opp_list_lock);
>> +   return;
>> +   }
>> +
>> +   while (!list_empty(_opp->opp_list)) {
>> +   opp = list_entry_rcu(dev_opp->opp_list.next,
>> +   struct dev_pm_opp, node);
>
> list_for_each_entry_rcu ?
>

list_for_each_entry_rcu can not be used as opp is being deleted in the loop.

Thanks,
Inder

>> +   list_del_rcu(>node);
>> +   kfree_rcu(opp, head);
>> +   }
>> +
>> +   list_del_rcu(_opp->node);
>> +   mutex_unlock(_opp_list_lock);
>> +   synchronize_rcu();
>> +   kfree(dev_opp);
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_opp_free_opp_table);
>>  #endif
>> diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
>> index 0330217..3c29620 100644
>> --- a/include/linux/pm_opp.h
>> +++ b/include/linux/pm_opp.h
>> @@ -50,6 +50,8 @@ int dev_pm_opp_enable(struct device *dev, unsigned long 
>> freq);
>>  int dev_pm_opp_disable(struct device *dev, unsigned long freq);
>>
>>  struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
>> +
>> +void dev_pm_opp_free_opp_table(struct device *dev);
>>  #else
>>  static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
>>  {
>> @@ -105,6 +107,10 @@ static inline struct srcu_notifier_head 
>> *dev_pm_opp_get_notifier(
>>  {
>> return ERR_PTR(-EINVAL);
>>  }
>> +
>> +void dev_pm_opp_free_opp_table(struct device *dev)
>> +{
>> +}
>>  #endif /* CONFIG_PM_OPP */
>>
>>  #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
>
> Otherwise looks fine.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] PM / OPP: Implement free_opp_table function

[Inderpal Singh]

At the driver unloading time the associated opp table may need
to be deleted. Otherwise it amounts to memory leak. The existing
OPP library does not have provison to do so.

Hence this patch implements the function to free the opp table.

Signed-off-by: Inderpal Singh 
---
 drivers/base/power/opp.c | 41 +
 include/linux/pm_opp.h   |  6 ++
 2 files changed, 47 insertions(+)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index d9e376a..d45ffd5 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -654,4 +654,45 @@ int of_init_opp_table(struct device *dev)
return 0;
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
+
+/**
+ * dev_pm_opp_free_opp_table() - free the opp table
+ * @dev:   device for which we do this operation
+ *
+ * Free up the allocated opp table
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks to
+ * keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex locking or synchronize_rcu() blocking calls cannot be used.
+ */
+void dev_pm_opp_free_opp_table(struct device *dev)
+{
+   struct device_opp *dev_opp = NULL;
+   struct dev_pm_opp *opp;
+
+   /* Hold our list modification lock here */
+   mutex_lock(_opp_list_lock);
+
+   /* Check for existing list for 'dev' */
+   dev_opp = find_device_opp(dev);
+   if (IS_ERR(dev_opp)) {
+   mutex_unlock(_opp_list_lock);
+   return;
+   }
+
+   while (!list_empty(_opp->opp_list)) {
+   opp = list_entry_rcu(dev_opp->opp_list.next,
+   struct dev_pm_opp, node);
+   list_del_rcu(>node);
+   kfree_rcu(opp, head);
+   }
+
+   list_del_rcu(_opp->node);
+   mutex_unlock(_opp_list_lock);
+   synchronize_rcu();
+   kfree(dev_opp);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_free_opp_table);
 #endif
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 0330217..3c29620 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -50,6 +50,8 @@ int dev_pm_opp_enable(struct device *dev, unsigned long freq);
 int dev_pm_opp_disable(struct device *dev, unsigned long freq);
 
 struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
+
+void dev_pm_opp_free_opp_table(struct device *dev);
 #else
 static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 {
@@ -105,6 +107,10 @@ static inline struct srcu_notifier_head 
*dev_pm_opp_get_notifier(
 {
return ERR_PTR(-EINVAL);
 }
+
+void dev_pm_opp_free_opp_table(struct device *dev)
+{
+}
 #endif /* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] PM / OPP: Implement free_opp_table function

[Inderpal Singh]

At the driver unloading time the associated opp table may need
to be deleted. Otherwise it amounts to memory leak. The existing
OPP library does not have provison to do so.

Hence this patch implements the function to free the opp table.

Signed-off-by: Inderpal Singh inderpa...@samsung.com
---
 drivers/base/power/opp.c | 41 +
 include/linux/pm_opp.h   |  6 ++
 2 files changed, 47 insertions(+)

diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
index d9e376a..d45ffd5 100644
--- a/drivers/base/power/opp.c
+++ b/drivers/base/power/opp.c
@@ -654,4 +654,45 @@ int of_init_opp_table(struct device *dev)
return 0;
 }
 EXPORT_SYMBOL_GPL(of_init_opp_table);
+
+/**
+ * dev_pm_opp_free_opp_table() - free the opp table
+ * @dev:   device for which we do this operation
+ *
+ * Free up the allocated opp table
+ *
+ * Locking: The internal device_opp and opp structures are RCU protected.
+ * Hence this function internally uses RCU updater strategy with mutex locks to
+ * keep the integrity of the internal data structures. Callers should ensure
+ * that this function is *NOT* called under RCU protection or in contexts where
+ * mutex locking or synchronize_rcu() blocking calls cannot be used.
+ */
+void dev_pm_opp_free_opp_table(struct device *dev)
+{
+   struct device_opp *dev_opp = NULL;
+   struct dev_pm_opp *opp;
+
+   /* Hold our list modification lock here */
+   mutex_lock(dev_opp_list_lock);
+
+   /* Check for existing list for 'dev' */
+   dev_opp = find_device_opp(dev);
+   if (IS_ERR(dev_opp)) {
+   mutex_unlock(dev_opp_list_lock);
+   return;
+   }
+
+   while (!list_empty(dev_opp-opp_list)) {
+   opp = list_entry_rcu(dev_opp-opp_list.next,
+   struct dev_pm_opp, node);
+   list_del_rcu(opp-node);
+   kfree_rcu(opp, head);
+   }
+
+   list_del_rcu(dev_opp-node);
+   mutex_unlock(dev_opp_list_lock);
+   synchronize_rcu();
+   kfree(dev_opp);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_free_opp_table);
 #endif
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 0330217..3c29620 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -50,6 +50,8 @@ int dev_pm_opp_enable(struct device *dev, unsigned long freq);
 int dev_pm_opp_disable(struct device *dev, unsigned long freq);
 
 struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
+
+void dev_pm_opp_free_opp_table(struct device *dev);
 #else
 static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
 {
@@ -105,6 +107,10 @@ static inline struct srcu_notifier_head 
*dev_pm_opp_get_notifier(
 {
return ERR_PTR(-EINVAL);
 }
+
+void dev_pm_opp_free_opp_table(struct device *dev)
+{
+}
 #endif /* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP)  defined(CONFIG_OF)
-- 
1.8.3.2

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Inderpal Singh]

Hi Viresh,

Thanks for the review.

On Fri, May 16, 2014 at 3:36 PM, Viresh Kumar viresh.ku...@linaro.org wrote:
 On 16 May 2014 14:39, Inderpal Singh inderpa...@samsung.com wrote:
 diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
 +void dev_pm_opp_free_opp_table(struct device *dev)
 +{
 +   struct device_opp *dev_opp = NULL;
 +   struct dev_pm_opp *opp;
 +
 +   /* Hold our list modification lock here */
 +   mutex_lock(dev_opp_list_lock);
 +
 +   /* Check for existing list for 'dev' */
 +   dev_opp = find_device_opp(dev);
 +   if (IS_ERR(dev_opp)) {
 +   mutex_unlock(dev_opp_list_lock);
 +   return;
 +   }
 +
 +   while (!list_empty(dev_opp-opp_list)) {
 +   opp = list_entry_rcu(dev_opp-opp_list.next,
 +   struct dev_pm_opp, node);

 list_for_each_entry_rcu ?


list_for_each_entry_rcu can not be used as opp is being deleted in the loop.

Thanks,
Inder

 +   list_del_rcu(opp-node);
 +   kfree_rcu(opp, head);
 +   }
 +
 +   list_del_rcu(dev_opp-node);
 +   mutex_unlock(dev_opp_list_lock);
 +   synchronize_rcu();
 +   kfree(dev_opp);
 +}
 +EXPORT_SYMBOL_GPL(dev_pm_opp_free_opp_table);
  #endif
 diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
 index 0330217..3c29620 100644
 --- a/include/linux/pm_opp.h
 +++ b/include/linux/pm_opp.h
 @@ -50,6 +50,8 @@ int dev_pm_opp_enable(struct device *dev, unsigned long 
 freq);
  int dev_pm_opp_disable(struct device *dev, unsigned long freq);

  struct srcu_notifier_head *dev_pm_opp_get_notifier(struct device *dev);
 +
 +void dev_pm_opp_free_opp_table(struct device *dev);
  #else
  static inline unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
  {
 @@ -105,6 +107,10 @@ static inline struct srcu_notifier_head 
 *dev_pm_opp_get_notifier(
  {
 return ERR_PTR(-EINVAL);
  }
 +
 +void dev_pm_opp_free_opp_table(struct device *dev)
 +{
 +}
  #endif /* CONFIG_PM_OPP */

  #if defined(CONFIG_PM_OPP)  defined(CONFIG_OF)

 Otherwise looks fine.
 --
 To unsubscribe from this list: send the line unsubscribe linux-pm in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PM / OPP: Implement free_opp_table function

[Inderpal Singh]

On Fri, May 16, 2014 at 10:41 PM, Viresh Kumar viresh.ku...@linaro.org wrote:
 On 16 May 2014 22:38, Inderpal Singh inderpa...@samsung.com wrote:
 +   while (!list_empty(dev_opp-opp_list)) {
 +   opp = list_entry_rcu(dev_opp-opp_list.next,
 +   struct dev_pm_opp, node);

 list_for_each_entry_rcu ?


 list_for_each_entry_rcu can not be used as opp is being deleted in the loop.

 So what? The above code can be replaced easily I think.
 This is how it is implemented:

 #define list_for_each_entry_rcu(pos, head, member) \
 for (pos = list_entry_rcu((head)-next, typeof(*pos), member); \
 pos-member != (head); \
 pos = list_entry_rcu(pos-member.next, typeof(*pos), member))

by the time pos = list_entry_rcu(pos-member.
next, typeof(*pos), member) is executed, the pos would have been
freed in the loop.

 --
 To unsubscribe from this list: send the line unsubscribe linux-pm in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Inderpal Singh]

On Thu, May 15, 2014 at 11:31 AM, Viresh Kumar  wrote:
> On 15 May 2014 11:16, Inderpal Singh  wrote:
>> I think I did not make myself clear.
>
> Probably I was the one who got confused :)
>
>> Devfreq will have its own opp table associated with its own device. It
>> does not uses the opp table of cpus.
>> Hence there may be need to free the table if driver (at least devfreq)
>> getting un-registered.
>
> We may have an unregister routine routine, I am not arguing about that.
> But we don't need to call that for CPU's opp, that's it.. For devices it might
> make sense to free memory.

Yes the provision should be there in the OPP framework and let the
individual drivers decide whether to invoke or not.


> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Inderpal Singh]

On Thu, May 15, 2014 at 11:31 AM, Viresh Kumar viresh.ku...@linaro.org wrote:
 On 15 May 2014 11:16, Inderpal Singh inderpa...@samsung.com wrote:
 I think I did not make myself clear.

 Probably I was the one who got confused :)

 Devfreq will have its own opp table associated with its own device. It
 does not uses the opp table of cpus.
 Hence there may be need to free the table if driver (at least devfreq)
 getting un-registered.

 We may have an unregister routine routine, I am not arguing about that.
 But we don't need to call that for CPU's opp, that's it.. For devices it might
 make sense to free memory.

Yes the provision should be there in the OPP framework and let the
individual drivers decide whether to invoke or not.


 --
 To unsubscribe from this list: send the line unsubscribe linux-pm in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Inderpal Singh]

On Thu, May 15, 2014 at 11:07 AM, Viresh Kumar  wrote:
> On 15 May 2014 11:00, Inderpal Singh  wrote:
>> On Thu, May 15, 2014 at 10:34 AM, Viresh Kumar  
>> wrote:
>>> On 15 May 2014 10:22, Inderpal Singh  wrote:
>>>
>>>> What i am saying that "what if we are not going to re-use again ? "
>>>
>>> That's what I said:
>>>
>>> Yes, it will keep occupying some space but there is only one instance
>>> of that per 'cluster' and is very much affordable instead of building it 
>>> again..
>>>
>>> So, we may not need to free it.
>>
>> Its not just about cpufreq. There may be others using OPPs as well.
>> For example devfreq.
>
> And who is stopping these to use the already built ones? Exactly for
> this reason I have been saying that lets not free OPPs already built.
> Devfreq can simply use the ones built by cpufreq, even if cpufreq isn't
> using it anymore.

I think I did not make myself clear.
Devfreq will have its own opp table associated with its own device. It
does not uses the opp table of cpus.
Hence there may be need to free the table if driver (at least devfreq)
getting un-registered.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Inderpal Singh]

On Thu, May 15, 2014 at 10:34 AM, Viresh Kumar  wrote:
> On 15 May 2014 10:22, Inderpal Singh  wrote:
>
>> What i am saying that "what if we are not going to re-use again ? "
>
> That's what I said:
>
> Yes, it will keep occupying some space but there is only one instance
> of that per 'cluster' and is very much affordable instead of building it 
> again..
>
> So, we may not need to free it.

Its not just about cpufreq. There may be others using OPPs as well.
For example devfreq.
The OPPs for cpu devices may be used again but for others I am not sure.

>
>> Also, I feel the driver who created the opp table at its registration
>> time should free it at its unregistration. Isn't it true in general?
>
> Probably case to case basis. You must free resources for two reasons:
> - They are not lost, like memory leak ..
> - They can be used by others ..
>
> And both these doesn't happen in this case. OPP tables can be used
> by any other framework and is more or less a core thing..
>
> Now, with this discussion I have another idea here..
>
> Why don't we build these tables automatically on boot from some core
> code, rather than asking drivers to do it ? That will get rid of duplication
> from all drivers and would also reduce confusion
>

I also felt the same at least for OPPs of cpu devices.

> @Rafael/Nishanth ??
>
> If things look right, then I can write a patch for fixing it quickly...
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Inderpal Singh]

On Thu, May 15, 2014 at 10:06 AM, Viresh Kumar  wrote:
> On 15 May 2014 10:02, Inderpal Singh  wrote:
>> I feel freeing of opps are needed at least at the driver unregistration
>> time, like we free cpufreq_table.
>> Otherwise it amounts to memory leak unless we assume that the same driver is
>> going to re-register and re-use the same opps.
>
> Its memory leak only if we have lost the pointer to allocated memory, which
> we haven't. Yes, it will keep occupying some space but there is only
> one instance
> of that per 'cluster' and is very much affordable instead of building it 
> again..
>
> There is a high chance that it will be used again by this or any other driver,
> cpufreq or outside of it.
>
> But, yes I do agree that the OPPs not added from dts, i.e. added from
> platform should be freed when they don't make a sense. But that's a different
> issue altogether.

What i am saying that "what if we are not going to re-use again ? " I
am not sure if its practical.
Also, I feel the driver who created the opp table at its registration
time should free it at its unregistration. Isn't it true in general?


> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Inderpal Singh]

On Thu, May 15, 2014 at 10:06 AM, Viresh Kumar viresh.ku...@linaro.org wrote:
 On 15 May 2014 10:02, Inderpal Singh inderpa...@samsung.com wrote:
 I feel freeing of opps are needed at least at the driver unregistration
 time, like we free cpufreq_table.
 Otherwise it amounts to memory leak unless we assume that the same driver is
 going to re-register and re-use the same opps.

 Its memory leak only if we have lost the pointer to allocated memory, which
 we haven't. Yes, it will keep occupying some space but there is only
 one instance
 of that per 'cluster' and is very much affordable instead of building it 
 again..

 There is a high chance that it will be used again by this or any other driver,
 cpufreq or outside of it.

 But, yes I do agree that the OPPs not added from dts, i.e. added from
 platform should be freed when they don't make a sense. But that's a different
 issue altogether.

What i am saying that what if we are not going to re-use again ?  I
am not sure if its practical.
Also, I feel the driver who created the opp table at its registration
time should free it at its unregistration. Isn't it true in general?


 --
 To unsubscribe from this list: send the line unsubscribe linux-pm in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Inderpal Singh]

On Thu, May 15, 2014 at 10:34 AM, Viresh Kumar viresh.ku...@linaro.org wrote:
 On 15 May 2014 10:22, Inderpal Singh inderpa...@samsung.com wrote:

 What i am saying that what if we are not going to re-use again ? 

 That's what I said:

 Yes, it will keep occupying some space but there is only one instance
 of that per 'cluster' and is very much affordable instead of building it 
 again..

 So, we may not need to free it.

Its not just about cpufreq. There may be others using OPPs as well.
For example devfreq.
The OPPs for cpu devices may be used again but for others I am not sure.


 Also, I feel the driver who created the opp table at its registration
 time should free it at its unregistration. Isn't it true in general?

 Probably case to case basis. You must free resources for two reasons:
 - They are not lost, like memory leak ..
 - They can be used by others ..

 And both these doesn't happen in this case. OPP tables can be used
 by any other framework and is more or less a core thing..

 Now, with this discussion I have another idea here..

 Why don't we build these tables automatically on boot from some core
 code, rather than asking drivers to do it ? That will get rid of duplication
 from all drivers and would also reduce confusion


I also felt the same at least for OPPs of cpu devices.

 @Rafael/Nishanth ??

 If things look right, then I can write a patch for fixing it quickly...
 --
 To unsubscribe from this list: send the line unsubscribe linux-pm in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] PM / OPP: discard duplicate OPP additions

[Inderpal Singh]

On Thu, May 15, 2014 at 11:07 AM, Viresh Kumar viresh.ku...@linaro.org wrote:
 On 15 May 2014 11:00, Inderpal Singh inderpa...@samsung.com wrote:
 On Thu, May 15, 2014 at 10:34 AM, Viresh Kumar viresh.ku...@linaro.org 
 wrote:
 On 15 May 2014 10:22, Inderpal Singh inderpa...@samsung.com wrote:

 What i am saying that what if we are not going to re-use again ? 

 That's what I said:

 Yes, it will keep occupying some space but there is only one instance
 of that per 'cluster' and is very much affordable instead of building it 
 again..

 So, we may not need to free it.

 Its not just about cpufreq. There may be others using OPPs as well.
 For example devfreq.

 And who is stopping these to use the already built ones? Exactly for
 this reason I have been saying that lets not free OPPs already built.
 Devfreq can simply use the ones built by cpufreq, even if cpufreq isn't
 using it anymore.

I think I did not make myself clear.
Devfreq will have its own opp table associated with its own device. It
does not uses the opp table of cpus.
Hence there may be need to free the table if driver (at least devfreq)
getting un-registered.

 --
 To unsubscribe from this list: send the line unsubscribe linux-pm in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[RFC PATCH] regulator: virtual: Introduce a new virtual locker regulator type

[Inderpal Singh]

On some SoCs there could be requirements that two or more voltage
regulators need to maintain certain skew for proper functioning.

This patch implements a new vitual locker type regulator which can
have multiple output and input regulators. The real regulators will
be hidden under the virtual output regulators. The consumer of the
real regulators need not change anything. Only the name of the real
regulators need to be changed.

This patch has been tested on exynos5420 based smdk5420.

Signed-off-by: Doug Anderson 
Signed-off-by: Inderpal Singh 
---
 .../bindings/regulator/locker-regulator.txt|  36 ++
 drivers/regulator/Kconfig  |   9 +
 drivers/regulator/Makefile |   1 +
 drivers/regulator/locker.c | 472 +
 include/linux/regulator/locker.h   |  35 ++
 5 files changed, 553 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/regulator/locker-regulator.txt
 create mode 100644 drivers/regulator/locker.c
 create mode 100644 include/linux/regulator/locker.h

diff --git a/Documentation/devicetree/bindings/regulator/locker-regulator.txt 
b/Documentation/devicetree/bindings/regulator/locker-regulator.txt
new file mode 100644
index 000..1ddba0a
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/locker-regulator.txt
@@ -0,0 +1,36 @@
+Locker Voltage regulators
+
+Required properties:
+- compatible: Must be "regulator-locker";
+- Must have atleast 2 vinX-supply nodes
+- Must have atleast 2 vout nodes
+- Must have maximum-spread
+- The counting for vout and vinX-supplies starts from 1 as shown below
+
+Any property defined as part of the core regulator
+binding, defined in regulator.txt, can also be used.
+
+Example:
+
+   int-voltage-locker {
+   compatible = "regulator-locker";
+   vin1-supply = <_reg>; /* real vdd_arm */
+   vin2-supply = <_reg>; /* real vdd_int */
+   maximum-spread = <30>;
+   locker-regulators {
+   vout1 {
+   regulator-name = "vdd_arm";
+   regulator-min-microvolt = <80>;
+   regulator-max-microvolt = <150>;
+   regulator-always-on;
+   regulator-boot-on;
+   };
+   vout2 {
+   regulator-name = "vdd_int";
+   regulator-min-microvolt = <80>;
+   regulator-max-microvolt = <140>;
+   regulator-always-on;
+   regulator-boot-on;
+   };
+   };
+   };
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 65e5d7d..9efc4b8 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -54,6 +54,15 @@ config REGULATOR_USERSPACE_CONSUMER
 
  If unsure, say no.
 
+config REGULATOR_LOCKER_VOLTAGE
+   tristate "Locker voltage regulator support"
+   help
+ This driver provides support for virtual voltage regulators,
+ useful for systems which has requirements to maintain a skew
+ between 2 or more regulators.
+
+ If unsure, say no.
+
 config REGULATOR_88PM800
tristate "Marvell 88PM800 Power regulators"
depends on MFD_88PM800
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index c14696b2..6cde7af 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_OF) += of_regulator.o
 obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
 obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
 obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
+obj-$(CONFIG_REGULATOR_LOCKER_VOLTAGE) += locker.o
 
 obj-$(CONFIG_REGULATOR_88PM800) += 88pm800.o
 obj-$(CONFIG_REGULATOR_88PM8607) += 88pm8607.o
diff --git a/drivers/regulator/locker.c b/drivers/regulator/locker.c
new file mode 100644
index 000..4a39f78
--- /dev/null
+++ b/drivers/regulator/locker.c
@@ -0,0 +1,472 @@
+/*
+ * locker.c
+ *
+ * Copyright  (C) 2014 Samsung Electronics
+ * Inderpal Singh 
+ * Doug Anderson 
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define CHECK_ALGO_ERRORS 1
+
+/* Assume <= 99 MAX_REGULATORS */
+#define VIN_SUPPLY_TEMPLATE "vin%d-supply"
+#define VIN_TEMPLATE "vin%d"
+#define VOUT_TEMPLATE "vout%d"
+
+#define VIN_S

[RFC PATCH] regulator: virtual: Introduce a new virtual locker regulator type

[Inderpal Singh]

On some SoCs there could be requirements that two or more voltage
regulators need to maintain certain skew for proper functioning.

This patch implements a new vitual locker type regulator which can
have multiple output and input regulators. The real regulators will
be hidden under the virtual output regulators. The consumer of the
real regulators need not change anything. Only the name of the real
regulators need to be changed.

This patch has been tested on exynos5420 based smdk5420.

Signed-off-by: Doug Anderson diand...@chromium.org
Signed-off-by: Inderpal Singh inderpa...@samsung.com
---
 .../bindings/regulator/locker-regulator.txt|  36 ++
 drivers/regulator/Kconfig  |   9 +
 drivers/regulator/Makefile |   1 +
 drivers/regulator/locker.c | 472 +
 include/linux/regulator/locker.h   |  35 ++
 5 files changed, 553 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/regulator/locker-regulator.txt
 create mode 100644 drivers/regulator/locker.c
 create mode 100644 include/linux/regulator/locker.h

diff --git a/Documentation/devicetree/bindings/regulator/locker-regulator.txt 
b/Documentation/devicetree/bindings/regulator/locker-regulator.txt
new file mode 100644
index 000..1ddba0a
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/locker-regulator.txt
@@ -0,0 +1,36 @@
+Locker Voltage regulators
+
+Required properties:
+- compatible: Must be regulator-locker;
+- Must have atleast 2 vinX-supply nodes
+- Must have atleast 2 vout nodes
+- Must have maximum-spread
+- The counting for vout and vinX-supplies starts from 1 as shown below
+
+Any property defined as part of the core regulator
+binding, defined in regulator.txt, can also be used.
+
+Example:
+
+   int-voltage-locker {
+   compatible = regulator-locker;
+   vin1-supply = buck2_reg; /* real vdd_arm */
+   vin2-supply = buck3_reg; /* real vdd_int */
+   maximum-spread = 30;
+   locker-regulators {
+   vout1 {
+   regulator-name = vdd_arm;
+   regulator-min-microvolt = 80;
+   regulator-max-microvolt = 150;
+   regulator-always-on;
+   regulator-boot-on;
+   };
+   vout2 {
+   regulator-name = vdd_int;
+   regulator-min-microvolt = 80;
+   regulator-max-microvolt = 140;
+   regulator-always-on;
+   regulator-boot-on;
+   };
+   };
+   };
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 65e5d7d..9efc4b8 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -54,6 +54,15 @@ config REGULATOR_USERSPACE_CONSUMER
 
  If unsure, say no.
 
+config REGULATOR_LOCKER_VOLTAGE
+   tristate Locker voltage regulator support
+   help
+ This driver provides support for virtual voltage regulators,
+ useful for systems which has requirements to maintain a skew
+ between 2 or more regulators.
+
+ If unsure, say no.
+
 config REGULATOR_88PM800
tristate Marvell 88PM800 Power regulators
depends on MFD_88PM800
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index c14696b2..6cde7af 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_OF) += of_regulator.o
 obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
 obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
 obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
+obj-$(CONFIG_REGULATOR_LOCKER_VOLTAGE) += locker.o
 
 obj-$(CONFIG_REGULATOR_88PM800) += 88pm800.o
 obj-$(CONFIG_REGULATOR_88PM8607) += 88pm8607.o
diff --git a/drivers/regulator/locker.c b/drivers/regulator/locker.c
new file mode 100644
index 000..4a39f78
--- /dev/null
+++ b/drivers/regulator/locker.c
@@ -0,0 +1,472 @@
+/*
+ * locker.c
+ *
+ * Copyright  (C) 2014 Samsung Electronics
+ * Inderpal Singh inderpa...@samsung.com
+ * Doug Anderson diand...@chromium.org
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ */
+
+#include linux/err.h
+#include linux/mutex.h
+#include linux/module.h
+#include linux/platform_device.h
+#include linux/regulator/driver.h
+#include linux/regulator/locker.h
+#include linux/of.h
+#include linux/regulator/of_regulator.h
+#include linux/regulator/machine.h
+
+#define CHECK_ALGO_ERRORS 1
+
+/* Assume = 99 MAX_REGULATORS */
+#define

Re: [PATCH] cpufreq: exynos: Show list of available frequencies

[Inderpal Singh]

+CC:
dg77@samsung.com, myungjoo@samsung.com, kyungmin.p...@samsung.com

On 8 January 2013 16:20, Inderpal Singh  wrote:
> Add freq_attr attribute to show list of available frequencies.
>
> Signed-off-by: Donggeun Kim 
> Signed-off-by: MyungJoo Ham 
> Signed-off-by: KyungMin Park 
> Signed-off-by: Inderpal Singh 
> ---
>  drivers/cpufreq/exynos-cpufreq.c |   13 +
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/cpufreq/exynos-cpufreq.c 
> b/drivers/cpufreq/exynos-cpufreq.c
> index 7012ea8..bc1e833 100644
> --- a/drivers/cpufreq/exynos-cpufreq.c
> +++ b/drivers/cpufreq/exynos-cpufreq.c
> @@ -244,13 +244,26 @@ static int exynos_cpufreq_cpu_init(struct 
> cpufreq_policy *policy)
> return cpufreq_frequency_table_cpuinfo(policy, 
> exynos_info->freq_table);
>  }
>
> +static int exynos_cpufreq_cpu_exit(struct cpufreq_policy *policy)
> +{
> +   cpufreq_frequency_table_put_attr(policy->cpu);
> +   return 0;
> +}
> +
> +static struct freq_attr *exynos_cpufreq_attr[] = {
> +   _freq_attr_scaling_available_freqs,
> +   NULL,
> +};
> +
>  static struct cpufreq_driver exynos_driver = {
> .flags  = CPUFREQ_STICKY,
> .verify = exynos_verify_speed,
> .target = exynos_target,
> .get= exynos_getspeed,
> .init   = exynos_cpufreq_cpu_init,
> +   .exit   = exynos_cpufreq_cpu_exit,
> .name   = "exynos_cpufreq",
> +   .attr   = exynos_cpufreq_attr,
>  #ifdef CONFIG_PM
> .suspend= exynos_cpufreq_suspend,
> .resume = exynos_cpufreq_resume,
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] cpufreq: exynos: Show list of available frequencies

[Inderpal Singh]

Add freq_attr attribute to show list of available frequencies.

Signed-off-by: Donggeun Kim 
Signed-off-by: MyungJoo Ham 
Signed-off-by: KyungMin Park 
Signed-off-by: Inderpal Singh 
---
 drivers/cpufreq/exynos-cpufreq.c |   13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/cpufreq/exynos-cpufreq.c b/drivers/cpufreq/exynos-cpufreq.c
index 7012ea8..bc1e833 100644
--- a/drivers/cpufreq/exynos-cpufreq.c
+++ b/drivers/cpufreq/exynos-cpufreq.c
@@ -244,13 +244,26 @@ static int exynos_cpufreq_cpu_init(struct cpufreq_policy 
*policy)
return cpufreq_frequency_table_cpuinfo(policy, exynos_info->freq_table);
 }
 
+static int exynos_cpufreq_cpu_exit(struct cpufreq_policy *policy)
+{
+   cpufreq_frequency_table_put_attr(policy->cpu);
+   return 0;
+}
+
+static struct freq_attr *exynos_cpufreq_attr[] = {
+   _freq_attr_scaling_available_freqs,
+   NULL,
+};
+
 static struct cpufreq_driver exynos_driver = {
.flags  = CPUFREQ_STICKY,
.verify = exynos_verify_speed,
.target = exynos_target,
.get= exynos_getspeed,
.init   = exynos_cpufreq_cpu_init,
+   .exit   = exynos_cpufreq_cpu_exit,
.name   = "exynos_cpufreq",
+   .attr   = exynos_cpufreq_attr,
 #ifdef CONFIG_PM
.suspend= exynos_cpufreq_suspend,
.resume = exynos_cpufreq_resume,
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] cpufreq: exynos: Show list of available frequencies

[Inderpal Singh]

Add freq_attr attribute to show list of available frequencies.

Signed-off-by: Donggeun Kim dg77@samsung.com
Signed-off-by: MyungJoo Ham myungjoo@samsung.com
Signed-off-by: KyungMin Park kyungmin.p...@samsung.com
Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
---
 drivers/cpufreq/exynos-cpufreq.c |   13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/cpufreq/exynos-cpufreq.c b/drivers/cpufreq/exynos-cpufreq.c
index 7012ea8..bc1e833 100644
--- a/drivers/cpufreq/exynos-cpufreq.c
+++ b/drivers/cpufreq/exynos-cpufreq.c
@@ -244,13 +244,26 @@ static int exynos_cpufreq_cpu_init(struct cpufreq_policy 
*policy)
return cpufreq_frequency_table_cpuinfo(policy, exynos_info-freq_table);
 }
 
+static int exynos_cpufreq_cpu_exit(struct cpufreq_policy *policy)
+{
+   cpufreq_frequency_table_put_attr(policy-cpu);
+   return 0;
+}
+
+static struct freq_attr *exynos_cpufreq_attr[] = {
+   cpufreq_freq_attr_scaling_available_freqs,
+   NULL,
+};
+
 static struct cpufreq_driver exynos_driver = {
.flags  = CPUFREQ_STICKY,
.verify = exynos_verify_speed,
.target = exynos_target,
.get= exynos_getspeed,
.init   = exynos_cpufreq_cpu_init,
+   .exit   = exynos_cpufreq_cpu_exit,
.name   = exynos_cpufreq,
+   .attr   = exynos_cpufreq_attr,
 #ifdef CONFIG_PM
.suspend= exynos_cpufreq_suspend,
.resume = exynos_cpufreq_resume,
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] cpufreq: exynos: Show list of available frequencies

[Inderpal Singh]

+CC:
dg77@samsung.com, myungjoo@samsung.com, kyungmin.p...@samsung.com

On 8 January 2013 16:20, Inderpal Singh inderpal.si...@linaro.org wrote:
 Add freq_attr attribute to show list of available frequencies.

 Signed-off-by: Donggeun Kim dg77@samsung.com
 Signed-off-by: MyungJoo Ham myungjoo@samsung.com
 Signed-off-by: KyungMin Park kyungmin.p...@samsung.com
 Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
 ---
  drivers/cpufreq/exynos-cpufreq.c |   13 +
  1 file changed, 13 insertions(+)

 diff --git a/drivers/cpufreq/exynos-cpufreq.c 
 b/drivers/cpufreq/exynos-cpufreq.c
 index 7012ea8..bc1e833 100644
 --- a/drivers/cpufreq/exynos-cpufreq.c
 +++ b/drivers/cpufreq/exynos-cpufreq.c
 @@ -244,13 +244,26 @@ static int exynos_cpufreq_cpu_init(struct 
 cpufreq_policy *policy)
 return cpufreq_frequency_table_cpuinfo(policy, 
 exynos_info-freq_table);
  }

 +static int exynos_cpufreq_cpu_exit(struct cpufreq_policy *policy)
 +{
 +   cpufreq_frequency_table_put_attr(policy-cpu);
 +   return 0;
 +}
 +
 +static struct freq_attr *exynos_cpufreq_attr[] = {
 +   cpufreq_freq_attr_scaling_available_freqs,
 +   NULL,
 +};
 +
  static struct cpufreq_driver exynos_driver = {
 .flags  = CPUFREQ_STICKY,
 .verify = exynos_verify_speed,
 .target = exynos_target,
 .get= exynos_getspeed,
 .init   = exynos_cpufreq_cpu_init,
 +   .exit   = exynos_cpufreq_cpu_exit,
 .name   = exynos_cpufreq,
 +   .attr   = exynos_cpufreq_attr,
  #ifdef CONFIG_PM
 .suspend= exynos_cpufreq_suspend,
 .resume = exynos_cpufreq_resume,
 --
 1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] regulator: s5m8767: Fix probe failure due to stack corruption

[Inderpal Singh]

The function sec_reg_read invokes regmap_read which expects unsigned int *
as the destination address. The existing driver is passing address of local
variable "val" which is u8. This causes the stack corruption and following
dump is observed during probe.

Hence change "val" from u8 to unsigned int.

Unable to handle kernel paging request at virtual address 02410020
pgd = c0004000
[02410020] *pgd=
Internal error: Oops: 8005 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 0Not tainted  (3.6.0-00696-g98a28b18-dirty #27)
PC is at 0x2410020
LR is at _regulator_get_voltage+0x3c/0x70
pc : [<02410020>]lr : []psr: 2013
sp : cf839b68  ip :   fp : cf92d410
r10: cfd0  r9 : c06d9878  r8 : f0a0
r7 : cf839b70  r6 : cf92d400  r5 : 0011  r4 : cf00
r3 : 02410020  r2 :   r1 : 0048  r0 : cf00
Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
...
.

[] (_regulator_get_voltage+0x3c/0x70) from [] 
(print_constraints+0x50/0x36c)
[] (print_constraints+0x50/0x36c) from [] 
(set_machine_constraints+0xe8/0x2b0)
[] (set_machine_constraints+0xe8/0x2b0) from [] 
(regulator_register+0x2fc/0x604)
[] (regulator_register+0x2fc/0x604) from [] 
(s5m8767_pmic_probe+0x688/0x718)
[] (s5m8767_pmic_probe+0x688/0x718) from [] 
(platform_drv_probe+0x18/0x1c)
[] (platform_drv_probe+0x18/0x1c) from [] 
(really_probe+0x68/0x1f4)
[] (really_probe+0x68/0x1f4) from [] 
(driver_probe_device+0x30/0x48)

Signed-off-by: Inderpal Singh 
---
 drivers/regulator/s5m8767.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
index 8ef5b33..9e6850f 100644
--- a/drivers/regulator/s5m8767.c
+++ b/drivers/regulator/s5m8767.c
@@ -214,7 +214,7 @@ static int s5m8767_reg_is_enabled(struct regulator_dev 
*rdev)
struct s5m8767_info *s5m8767 = rdev_get_drvdata(rdev);
int ret, reg;
int mask = 0xc0, enable_ctrl;
-   u8 val;
+   unsigned int val;
 
ret = s5m8767_get_register(rdev, , _ctrl);
if (ret == -EINVAL)
@@ -306,7 +306,7 @@ static int s5m8767_get_voltage_sel(struct regulator_dev 
*rdev)
struct s5m8767_info *s5m8767 = rdev_get_drvdata(rdev);
int reg, mask, ret;
int reg_id = rdev_get_id(rdev);
-   u8 val;
+   unsigned int val;
 
ret = s5m8767_get_voltage_register(rdev, );
if (ret)
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] regulator: s5m8767: Fix probe failure due to stack corruption

[Inderpal Singh]

The function sec_reg_read invokes regmap_read which expects unsigned int *
as the destination address. The existing driver is passing address of local
variable val which is u8. This causes the stack corruption and following
dump is observed during probe.

Hence change val from u8 to unsigned int.

Unable to handle kernel paging request at virtual address 02410020
pgd = c0004000
[02410020] *pgd=
Internal error: Oops: 8005 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 0Not tainted  (3.6.0-00696-g98a28b18-dirty #27)
PC is at 0x2410020
LR is at _regulator_get_voltage+0x3c/0x70
pc : [02410020]lr : [c02395d4]psr: 2013
sp : cf839b68  ip :   fp : cf92d410
r10: cfd0  r9 : c06d9878  r8 : f0a0
r7 : cf839b70  r6 : cf92d400  r5 : 0011  r4 : cf00
r3 : 02410020  r2 :   r1 : 0048  r0 : cf00
Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
...
.

[c02395d4] (_regulator_get_voltage+0x3c/0x70) from [c023ad80] 
(print_constraints+0x50/0x36c)
[c023ad80] (print_constraints+0x50/0x36c) from [c023e504] 
(set_machine_constraints+0xe8/0x2b0)
[c023e504] (set_machine_constraints+0xe8/0x2b0) from [c023e9c8] 
(regulator_register+0x2fc/0x604)
[c023e9c8] (regulator_register+0x2fc/0x604) from [c049d628] 
(s5m8767_pmic_probe+0x688/0x718)
[c049d628] (s5m8767_pmic_probe+0x688/0x718) from [c029915c] 
(platform_drv_probe+0x18/0x1c)
[c029915c] (platform_drv_probe+0x18/0x1c) from [c0297dd0] 
(really_probe+0x68/0x1f4)
[c0297dd0] (really_probe+0x68/0x1f4) from [c0298070] 
(driver_probe_device+0x30/0x48)

Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
---
 drivers/regulator/s5m8767.c |4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
index 8ef5b33..9e6850f 100644
--- a/drivers/regulator/s5m8767.c
+++ b/drivers/regulator/s5m8767.c
@@ -214,7 +214,7 @@ static int s5m8767_reg_is_enabled(struct regulator_dev 
*rdev)
struct s5m8767_info *s5m8767 = rdev_get_drvdata(rdev);
int ret, reg;
int mask = 0xc0, enable_ctrl;
-   u8 val;
+   unsigned int val;
 
ret = s5m8767_get_register(rdev, reg, enable_ctrl);
if (ret == -EINVAL)
@@ -306,7 +306,7 @@ static int s5m8767_get_voltage_sel(struct regulator_dev 
*rdev)
struct s5m8767_info *s5m8767 = rdev_get_drvdata(rdev);
int reg, mask, ret;
int reg_id = rdev_get_id(rdev);
-   u8 val;
+   unsigned int val;
 
ret = s5m8767_get_voltage_register(rdev, reg);
if (ret)
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/4] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

Hi Vinod,

On 29 October 2012 10:15, Vinod Koul  wrote:
> On Sat, 2012-10-27 at 15:50 +0530, Inderpal Singh wrote:
>> Hi Vinod,
>>
>> On 26 October 2012 10:15, Vinod Koul  wrote:
>> > On Thu, 2012-10-25 at 16:53 +0530, Inderpal Singh wrote:
>> >>
>> >> This code will get executed only in case of force removal of the
>> >> module which was discussed in the first version of the patch at [1].
>> >> Now, if we do not have to think about force removal then this patch
>> >> will go back to the first version.
>> > But why are you doing force removal of driver even when client is
>> > holding a reference to you.
>> >
>> > What happens when client finally tries to free the channel?
>> Since we return EBUSY so forced removal won't succeed. Client can free
>> the channel eventually.
> And that is my concern. You have forcefully removed the dma module.
> What happens then? How will the free calll get executed, wont you hit a
> panic.

Yes, you are correct, It will hit a panic.
The return value from remove is not being checked in
__device_release_driver because of which dma module is forcefully
removed even if we return EBUSY from driver's remove. Hence returning
error from .remove is not useful at all.

>>
>> >
>> > What is the problem you are trying to solve?
>> >>
>>
>> There was a long discussion about it in the first version of the
>> patch. Allow me to explain it to you.
>>
>> The existing driver does DMA_TERMINATE_ALL and frees resources for all
>> the channels in the _remove function.
> Which for starters may not be right thing to do.


Please consider v1 patch which removes DMA_TERMINATE_ALL and freeing
of resources from .remove function because in normal scenario if
remove is reached it is sure that no client is holding any reference
to the driver hence no need to flush and free the channels.

> Shouldn't you first
> make sure client has freed all references to your driver and then only
> remove. Freeing resources in .remove without keeping client in sync
> doesn't sound to be good idea to me.
>
>>  The first version of patch
>> removed this flushing and freeing of channel resources because they
>> are not getting allocated in the probe. Jassi pointed out that manual
>> flushing is needed if a force removal happens and some client is
>> queued. Then it was agreed that flushing is not needed, instead we
>> should return EBUSY if client is queued on some channel (this will
>> happen only in force removal case). Hence this additional check in v2
>> version so that force removal does not succeeds if any client is
>> queued.
>>
>> If you think force removal is not a practical scenario and we should
>> not be bothering about it, this check can be removed and the patch
>> will go back to first version which just removes flushing and freeing
>> of channels beacues they are not getting allocated in probe.
>>
>> Let me know your view.
>>
>> Regards,
>> Inder
>>
>>
>> >> Let me know your view.
>> >>
>> >> [1] https://patchwork.kernel.org/patch/1503171/
>> >>
>> >
>> >
>> > --
>> > Vinod Koul
>> > Intel Corp.
>> >
>
>
> --
> Vinod Koul
> Intel Corp.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/4] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

Hi Vinod,

On 29 October 2012 10:15, Vinod Koul vk...@infradead.org wrote:
 On Sat, 2012-10-27 at 15:50 +0530, Inderpal Singh wrote:
 Hi Vinod,

 On 26 October 2012 10:15, Vinod Koul vk...@infradead.org wrote:
  On Thu, 2012-10-25 at 16:53 +0530, Inderpal Singh wrote:
 
  This code will get executed only in case of force removal of the
  module which was discussed in the first version of the patch at [1].
  Now, if we do not have to think about force removal then this patch
  will go back to the first version.
  But why are you doing force removal of driver even when client is
  holding a reference to you.
 
  What happens when client finally tries to free the channel?
 Since we return EBUSY so forced removal won't succeed. Client can free
 the channel eventually.
 And that is my concern. You have forcefully removed the dma module.
 What happens then? How will the free calll get executed, wont you hit a
 panic.

Yes, you are correct, It will hit a panic.
The return value from remove is not being checked in
__device_release_driver because of which dma module is forcefully
removed even if we return EBUSY from driver's remove. Hence returning
error from .remove is not useful at all.


 
  What is the problem you are trying to solve?
 

 There was a long discussion about it in the first version of the
 patch. Allow me to explain it to you.

 The existing driver does DMA_TERMINATE_ALL and frees resources for all
 the channels in the _remove function.
 Which for starters may not be right thing to do.


Please consider v1 patch which removes DMA_TERMINATE_ALL and freeing
of resources from .remove function because in normal scenario if
remove is reached it is sure that no client is holding any reference
to the driver hence no need to flush and free the channels.

 Shouldn't you first
 make sure client has freed all references to your driver and then only
 remove. Freeing resources in .remove without keeping client in sync
 doesn't sound to be good idea to me.

  The first version of patch
 removed this flushing and freeing of channel resources because they
 are not getting allocated in the probe. Jassi pointed out that manual
 flushing is needed if a force removal happens and some client is
 queued. Then it was agreed that flushing is not needed, instead we
 should return EBUSY if client is queued on some channel (this will
 happen only in force removal case). Hence this additional check in v2
 version so that force removal does not succeeds if any client is
 queued.

 If you think force removal is not a practical scenario and we should
 not be bothering about it, this check can be removed and the patch
 will go back to first version which just removes flushing and freeing
 of channels beacues they are not getting allocated in probe.

 Let me know your view.

 Regards,
 Inder


  Let me know your view.
 
  [1] https://patchwork.kernel.org/patch/1503171/
 
 
 
  --
  Vinod Koul
  Intel Corp.
 


 --
 Vinod Koul
 Intel Corp.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/4] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

Hi Vinod,

On 26 October 2012 10:15, Vinod Koul  wrote:
> On Thu, 2012-10-25 at 16:53 +0530, Inderpal Singh wrote:
>>
>> This code will get executed only in case of force removal of the
>> module which was discussed in the first version of the patch at [1].
>> Now, if we do not have to think about force removal then this patch
>> will go back to the first version.
> But why are you doing force removal of driver even when client is
> holding a reference to you.
>
> What happens when client finally tries to free the channel?
Since we return EBUSY so forced removal won't succeed. Client can free
the channel eventually.

>
> What is the problem you are trying to solve?
>>

There was a long discussion about it in the first version of the
patch. Allow me to explain it to you.

The existing driver does DMA_TERMINATE_ALL and frees resources for all
the channels in the _remove function. The first version of patch
removed this flushing and freeing of channel resources because they
are not getting allocated in the probe. Jassi pointed out that manual
flushing is needed if a force removal happens and some client is
queued. Then it was agreed that flushing is not needed, instead we
should return EBUSY if client is queued on some channel (this will
happen only in force removal case). Hence this additional check in v2
version so that force removal does not succeeds if any client is
queued.

If you think force removal is not a practical scenario and we should
not be bothering about it, this check can be removed and the patch
will go back to first version which just removes flushing and freeing
of channels beacues they are not getting allocated in probe.

Let me know your view.

Regards,
Inder


>> Let me know your view.
>>
>> [1] https://patchwork.kernel.org/patch/1503171/
>>
>
>
> --
> Vinod Koul
> Intel Corp.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/4] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

Hi Vinod,

On 26 October 2012 10:15, Vinod Koul vk...@infradead.org wrote:
 On Thu, 2012-10-25 at 16:53 +0530, Inderpal Singh wrote:

 This code will get executed only in case of force removal of the
 module which was discussed in the first version of the patch at [1].
 Now, if we do not have to think about force removal then this patch
 will go back to the first version.
 But why are you doing force removal of driver even when client is
 holding a reference to you.

 What happens when client finally tries to free the channel?
Since we return EBUSY so forced removal won't succeed. Client can free
the channel eventually.


 What is the problem you are trying to solve?


There was a long discussion about it in the first version of the
patch. Allow me to explain it to you.

The existing driver does DMA_TERMINATE_ALL and frees resources for all
the channels in the _remove function. The first version of patch
removed this flushing and freeing of channel resources because they
are not getting allocated in the probe. Jassi pointed out that manual
flushing is needed if a force removal happens and some client is
queued. Then it was agreed that flushing is not needed, instead we
should return EBUSY if client is queued on some channel (this will
happen only in force removal case). Hence this additional check in v2
version so that force removal does not succeeds if any client is
queued.

If you think force removal is not a practical scenario and we should
not be bothering about it, this check can be removed and the patch
will go back to first version which just removes flushing and freeing
of channels beacues they are not getting allocated in probe.

Let me know your view.

Regards,
Inder


 Let me know your view.

 [1] https://patchwork.kernel.org/patch/1503171/



 --
 Vinod Koul
 Intel Corp.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/4] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

Hi Vinod,

On 24 October 2012 09:44, Vinod Koul  wrote:
> On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh wrote:
>> Since peripheral channel resources are not being allocated at probe,
>> no need to flush the channels and free the resources in remove function.
>> In case, the channel is in use by some client, return EBUSY.
>>
>> Signed-off-by: Inderpal Singh 
>> ---
>>  drivers/dma/pl330.c |   13 -
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index bf71ff7..4b7a34d 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -3009,18 +3009,21 @@ static int __devexit pl330_remove(struct amba_device 
>> *adev)
>>   if (!pdmac)
>>   return 0;
>>
>> + /* check if any client is using any channel */
>> + list_for_each_entry(pch, >ddma.channels,
>> + chan.device_node) {
>> +
>> + if (pch->chan.client_count)
>> + return -EBUSY;
>> + }
>> +
>>   while (!list_empty(>desc_pool)) {
>
> Did you get this code executed?
> I think No.
>
> The dmaengine holds the reference to channels, so if they are in use and
> not freed by client your remove wont be called. So this check is
> redundant
>

This code will get executed only in case of force removal of the
module which was discussed in the first version of the patch at [1].
Now, if we do not have to think about force removal then this patch
will go back to the first version.

Let me know your view.

[1] https://patchwork.kernel.org/patch/1503171/

Regards,
Inder
> --
> Vinod Koul
> Intel Corp.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 4/4] DMA: PL330: unregister dma_device in module's remove function

[Inderpal Singh]

On 24 October 2012 09:49, Vinod Koul  wrote:
> On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh wrote:
>> unregister dma_device in module's remove function.
>>
>> Signed-off-by: Inderpal Singh 
>> ---
>>  drivers/dma/pl330.c |2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index 4b7a34d..e7dc040 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -3017,6 +3017,8 @@ static int __devexit pl330_remove(struct amba_device 
>> *adev)
>>   return -EBUSY;
>>   }
>>
>> + dma_async_device_unregister(>ddma);
>> +
>>   amba_set_drvdata(adev, NULL);
>>
>>   list_for_each_entry_safe(pch, _p, >ddma.channels,
>
> Ok with this one :)
>
> Tried applying but didn't work out. You would need to regenerate this
> one.
>

I will regenerate this along with other patches and resend.

With Regards,
Inder

> Thanks
> --
> Vinod Koul
> Intel Corp.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] DMA: PL330: Change allocation method to properly free DMA descriptors

[Inderpal Singh]

On 24 October 2012 09:40, Vinod Koul  wrote:
> On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh wrote:
>> In probe, memory for multiple DMA descriptors were being allocated at once
>> and then it was being split and added into DMA pool one by one. The address
>> of this memory allocation is not being saved anywhere. To free this memory,
>> the address is required. Initially the first node of the pool will be
>> pointed by this address but as we use this pool the descs will shuffle and
>> hence we will lose the track of the address.
>>
>> This patch does following:
>>
>> 1. Allocates DMA descs one by one and then adds them to pool so that all
>>descs can be fetched and memory freed one by one. This way runtime
>>added descs can also be freed.
>> 2. Free DMA descs in case of error in probe and in module's remove function
> For probe, again you have cleaner code by using devm_kzalloc.
>
> On 1, if we use the devm_kzalloc then we don't need to allocate in
> chunks right?
>

Yes, if we use devm_kzalloc we wont have to allocate in chunks.
I will update this patch to use devm_kzalloc and send it again.


>>
>> Signed-off-by: Inderpal Singh 
>> ---
>>  drivers/dma/pl330.c |   35 +--
>>  1 file changed, 25 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index 10c6b6a..bf71ff7 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -2541,20 +2541,20 @@ static int add_desc(struct dma_pl330_dmac *pdmac, 
>> gfp_t flg, int count)
>>   if (!pdmac)
>>   return 0;
>>
>> - desc = kmalloc(count * sizeof(*desc), flg);
>> - if (!desc)
>> - return 0;
>> + for (i = 0; i < count; i++) {
>> + desc = kmalloc(sizeof(*desc), flg);
>> + if (!desc)
>> + break;
>> + _init_desc(desc);
>>
>> - spin_lock_irqsave(>pool_lock, flags);
>> + spin_lock_irqsave(>pool_lock, flags);
>>
>> - for (i = 0; i < count; i++) {
>> - _init_desc([i]);
>> - list_add_tail([i].node, >desc_pool);
>> - }
>> + list_add_tail(>node, >desc_pool);
>>
>> - spin_unlock_irqrestore(>pool_lock, flags);
>> + spin_unlock_irqrestore(>pool_lock, flags);
>> + }
>>
>> - return count;
>> + return i;
>>  }
>>
>>  static struct dma_pl330_desc *
>> @@ -2857,6 +2857,7 @@ pl330_probe(struct amba_device *adev, const struct 
>> amba_id *id)
>>   struct dma_pl330_platdata *pdat;
>>   struct dma_pl330_dmac *pdmac;
>>   struct dma_pl330_chan *pch;
>> + struct dma_pl330_desc *desc;
>>   struct pl330_info *pi;
>>   struct dma_device *pd;
>>   struct resource *res;
>> @@ -2978,6 +2979,12 @@ pl330_probe(struct amba_device *adev, const struct 
>> amba_id *id)
>>  probe_err5:
>>   kfree(pdmac->peripherals);
>>  probe_err4:
>> + while (!list_empty(>desc_pool)) {
>> + desc = list_entry(pdmac->desc_pool.next,
>> + struct dma_pl330_desc, node);
>> + list_del(>node);
>> + kfree(desc);
>> + }
>>   pl330_del(pi);
>>  probe_err3:
>>   free_irq(irq, pi);
>> @@ -2994,6 +3001,7 @@ static int __devexit pl330_remove(struct amba_device 
>> *adev)
>>  {
>>   struct dma_pl330_dmac *pdmac = amba_get_drvdata(adev);
>>   struct dma_pl330_chan *pch, *_p;
>> + struct dma_pl330_desc *desc;
>>   struct pl330_info *pi;
>>   struct resource *res;
>>   int irq;
>> @@ -3015,6 +3023,13 @@ static int __devexit pl330_remove(struct amba_device 
>> *adev)
>>   pl330_free_chan_resources(>chan);
>>   }
>>
>> + while (!list_empty(>desc_pool)) {
>> + desc = list_entry(pdmac->desc_pool.next,
>> + struct dma_pl330_desc, node);
>> + list_del(>node);
>> + kfree(desc);
>> + }
>> +
>>   pi = >pif;
>>
>>   pl330_del(pi);
>
>
> --
> Vinod Koul
> Intel Corp.
>
Regards,
Inder
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/4] DMA: PL330: Free memory allocated for peripheral channels

[Inderpal Singh]

Hi Vinod,

Thanks for reviewing.

On 24 October 2012 09:35, Vinod Koul  wrote:
> On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh wrote:
>> The allocated memory for peripheral channels is not being freed upon
>> failure in probe and in module's remove funtion. It will lead to memory
>> leakage. Hence free the allocated memory.
>>
>> Signed-off-by: Inderpal Singh 
>> ---
>>  drivers/dma/pl330.c |5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index 2ebd4cd..10c6b6a 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -2962,7 +2962,7 @@ pl330_probe(struct amba_device *adev, const struct 
>> amba_id *id)
>>   ret = dma_async_device_register(pd);
>>   if (ret) {
>>   dev_err(>dev, "unable to register DMAC\n");
>> - goto probe_err4;
>> + goto probe_err5;
>>   }
>>
>>   dev_info(>dev,
>> @@ -2975,6 +2975,8 @@ pl330_probe(struct amba_device *adev, const struct 
>> amba_id *id)
>>
>>   return 0;
>>
>> +probe_err5:
>> + kfree(pdmac->peripherals);
>>  probe_err4:
>>   pl330_del(pi);
>>  probe_err3:
>> @@ -3025,6 +3027,7 @@ static int __devexit pl330_remove(struct amba_device 
>> *adev)
>>   res = >res;
>>   release_mem_region(res->start, resource_size(res));
>>
>> + kfree(pdmac->peripherals);
>>   kfree(pdmac);
>>
>>   return 0;
>
> This looks fine, but if you use devm_ functions then you dont need to do
> all this.
> Can you do that conversion instead?
>

Good point.
I will do the conversion and send it again.

Regards,
Inder

> --
> Vinod Koul
> Intel Corp.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 
> in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 1/4] DMA: PL330: Free memory allocated for peripheral channels

[Inderpal Singh]

Hi Vinod,

Thanks for reviewing.

On 24 October 2012 09:35, Vinod Koul vk...@infradead.org wrote:
 On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh wrote:
 The allocated memory for peripheral channels is not being freed upon
 failure in probe and in module's remove funtion. It will lead to memory
 leakage. Hence free the allocated memory.

 Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
 ---
  drivers/dma/pl330.c |5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

 diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
 index 2ebd4cd..10c6b6a 100644
 --- a/drivers/dma/pl330.c
 +++ b/drivers/dma/pl330.c
 @@ -2962,7 +2962,7 @@ pl330_probe(struct amba_device *adev, const struct 
 amba_id *id)
   ret = dma_async_device_register(pd);
   if (ret) {
   dev_err(adev-dev, unable to register DMAC\n);
 - goto probe_err4;
 + goto probe_err5;
   }

   dev_info(adev-dev,
 @@ -2975,6 +2975,8 @@ pl330_probe(struct amba_device *adev, const struct 
 amba_id *id)

   return 0;

 +probe_err5:
 + kfree(pdmac-peripherals);
  probe_err4:
   pl330_del(pi);
  probe_err3:
 @@ -3025,6 +3027,7 @@ static int __devexit pl330_remove(struct amba_device 
 *adev)
   res = adev-res;
   release_mem_region(res-start, resource_size(res));

 + kfree(pdmac-peripherals);
   kfree(pdmac);

   return 0;

 This looks fine, but if you use devm_ functions then you dont need to do
 all this.
 Can you do that conversion instead?


Good point.
I will do the conversion and send it again.

Regards,
Inder

 --
 Vinod Koul
 Intel Corp.

 --
 To unsubscribe from this list: send the line unsubscribe linux-samsung-soc 
 in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/4] DMA: PL330: Change allocation method to properly free DMA descriptors

[Inderpal Singh]

On 24 October 2012 09:40, Vinod Koul vk...@infradead.org wrote:
 On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh wrote:
 In probe, memory for multiple DMA descriptors were being allocated at once
 and then it was being split and added into DMA pool one by one. The address
 of this memory allocation is not being saved anywhere. To free this memory,
 the address is required. Initially the first node of the pool will be
 pointed by this address but as we use this pool the descs will shuffle and
 hence we will lose the track of the address.

 This patch does following:

 1. Allocates DMA descs one by one and then adds them to pool so that all
descs can be fetched and memory freed one by one. This way runtime
added descs can also be freed.
 2. Free DMA descs in case of error in probe and in module's remove function
 For probe, again you have cleaner code by using devm_kzalloc.

 On 1, if we use the devm_kzalloc then we don't need to allocate in
 chunks right?


Yes, if we use devm_kzalloc we wont have to allocate in chunks.
I will update this patch to use devm_kzalloc and send it again.



 Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
 ---
  drivers/dma/pl330.c |   35 +--
  1 file changed, 25 insertions(+), 10 deletions(-)

 diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
 index 10c6b6a..bf71ff7 100644
 --- a/drivers/dma/pl330.c
 +++ b/drivers/dma/pl330.c
 @@ -2541,20 +2541,20 @@ static int add_desc(struct dma_pl330_dmac *pdmac, 
 gfp_t flg, int count)
   if (!pdmac)
   return 0;

 - desc = kmalloc(count * sizeof(*desc), flg);
 - if (!desc)
 - return 0;
 + for (i = 0; i  count; i++) {
 + desc = kmalloc(sizeof(*desc), flg);
 + if (!desc)
 + break;
 + _init_desc(desc);

 - spin_lock_irqsave(pdmac-pool_lock, flags);
 + spin_lock_irqsave(pdmac-pool_lock, flags);

 - for (i = 0; i  count; i++) {
 - _init_desc(desc[i]);
 - list_add_tail(desc[i].node, pdmac-desc_pool);
 - }
 + list_add_tail(desc-node, pdmac-desc_pool);

 - spin_unlock_irqrestore(pdmac-pool_lock, flags);
 + spin_unlock_irqrestore(pdmac-pool_lock, flags);
 + }

 - return count;
 + return i;
  }

  static struct dma_pl330_desc *
 @@ -2857,6 +2857,7 @@ pl330_probe(struct amba_device *adev, const struct 
 amba_id *id)
   struct dma_pl330_platdata *pdat;
   struct dma_pl330_dmac *pdmac;
   struct dma_pl330_chan *pch;
 + struct dma_pl330_desc *desc;
   struct pl330_info *pi;
   struct dma_device *pd;
   struct resource *res;
 @@ -2978,6 +2979,12 @@ pl330_probe(struct amba_device *adev, const struct 
 amba_id *id)
  probe_err5:
   kfree(pdmac-peripherals);
  probe_err4:
 + while (!list_empty(pdmac-desc_pool)) {
 + desc = list_entry(pdmac-desc_pool.next,
 + struct dma_pl330_desc, node);
 + list_del(desc-node);
 + kfree(desc);
 + }
   pl330_del(pi);
  probe_err3:
   free_irq(irq, pi);
 @@ -2994,6 +3001,7 @@ static int __devexit pl330_remove(struct amba_device 
 *adev)
  {
   struct dma_pl330_dmac *pdmac = amba_get_drvdata(adev);
   struct dma_pl330_chan *pch, *_p;
 + struct dma_pl330_desc *desc;
   struct pl330_info *pi;
   struct resource *res;
   int irq;
 @@ -3015,6 +3023,13 @@ static int __devexit pl330_remove(struct amba_device 
 *adev)
   pl330_free_chan_resources(pch-chan);
   }

 + while (!list_empty(pdmac-desc_pool)) {
 + desc = list_entry(pdmac-desc_pool.next,
 + struct dma_pl330_desc, node);
 + list_del(desc-node);
 + kfree(desc);
 + }
 +
   pi = pdmac-pif;

   pl330_del(pi);


 --
 Vinod Koul
 Intel Corp.

Regards,
Inder
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 4/4] DMA: PL330: unregister dma_device in module's remove function

[Inderpal Singh]

On 24 October 2012 09:49, Vinod Koul vk...@infradead.org wrote:
 On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh wrote:
 unregister dma_device in module's remove function.

 Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
 ---
  drivers/dma/pl330.c |2 ++
  1 file changed, 2 insertions(+)

 diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
 index 4b7a34d..e7dc040 100644
 --- a/drivers/dma/pl330.c
 +++ b/drivers/dma/pl330.c
 @@ -3017,6 +3017,8 @@ static int __devexit pl330_remove(struct amba_device 
 *adev)
   return -EBUSY;
   }

 + dma_async_device_unregister(pdmac-ddma);
 +
   amba_set_drvdata(adev, NULL);

   list_for_each_entry_safe(pch, _p, pdmac-ddma.channels,

 Ok with this one :)

 Tried applying but didn't work out. You would need to regenerate this
 one.


I will regenerate this along with other patches and resend.

With Regards,
Inder

 Thanks
 --
 Vinod Koul
 Intel Corp.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 3/4] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

Hi Vinod,

On 24 October 2012 09:44, Vinod Koul vk...@infradead.org wrote:
 On Fri, 2012-10-05 at 15:47 +0530, Inderpal Singh wrote:
 Since peripheral channel resources are not being allocated at probe,
 no need to flush the channels and free the resources in remove function.
 In case, the channel is in use by some client, return EBUSY.

 Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
 ---
  drivers/dma/pl330.c |   13 -
  1 file changed, 8 insertions(+), 5 deletions(-)

 diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
 index bf71ff7..4b7a34d 100644
 --- a/drivers/dma/pl330.c
 +++ b/drivers/dma/pl330.c
 @@ -3009,18 +3009,21 @@ static int __devexit pl330_remove(struct amba_device 
 *adev)
   if (!pdmac)
   return 0;

 + /* check if any client is using any channel */
 + list_for_each_entry(pch, pdmac-ddma.channels,
 + chan.device_node) {
 +
 + if (pch-chan.client_count)
 + return -EBUSY;
 + }
 +
   while (!list_empty(pdmac-desc_pool)) {

 Did you get this code executed?
 I think No.

 The dmaengine holds the reference to channels, so if they are in use and
 not freed by client your remove wont be called. So this check is
 redundant


This code will get executed only in case of force removal of the
module which was discussed in the first version of the patch at [1].
Now, if we do not have to think about force removal then this patch
will go back to the first version.

Let me know your view.

[1] https://patchwork.kernel.org/patch/1503171/

Regards,
Inder
 --
 Vinod Koul
 Intel Corp.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] MFD: SEC: Fix reg_offset for interrupt registers

[Inderpal Singh]

reg_offset is offset of the status/mask registers. Now, since status_base
and mask_base are pointing to corresponding first registers, reg_offset
should start from 0 otheriwse regmap_add_irq_chip will fail during probe.

Signed-off-by: Inderpal Singh 
---
It is based on Samuel's for-next-merge branch of mfd-2.6 tree. 

 drivers/mfd/sec-irq.c |  102 -
 1 file changed, 51 insertions(+), 51 deletions(-)

diff --git a/drivers/mfd/sec-irq.c b/drivers/mfd/sec-irq.c
index c901fa5..0dd84e9 100644
--- a/drivers/mfd/sec-irq.c
+++ b/drivers/mfd/sec-irq.c
@@ -24,67 +24,67 @@
 
 static struct regmap_irq s2mps11_irqs[] = {
[S2MPS11_IRQ_PWRONF] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S2MPS11_IRQ_PWRONF_MASK,
},
[S2MPS11_IRQ_PWRONR] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S2MPS11_IRQ_PWRONR_MASK,
},
[S2MPS11_IRQ_JIGONBF] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S2MPS11_IRQ_JIGONBF_MASK,
},
[S2MPS11_IRQ_JIGONBR] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S2MPS11_IRQ_JIGONBR_MASK,
},
[S2MPS11_IRQ_ACOKBF] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S2MPS11_IRQ_ACOKBF_MASK,
},
[S2MPS11_IRQ_ACOKBR] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S2MPS11_IRQ_ACOKBR_MASK,
},
[S2MPS11_IRQ_PWRON1S] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S2MPS11_IRQ_PWRON1S_MASK,
},
[S2MPS11_IRQ_MRB] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S2MPS11_IRQ_MRB_MASK,
},
[S2MPS11_IRQ_RTC60S] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S2MPS11_IRQ_RTC60S_MASK,
},
[S2MPS11_IRQ_RTCA1] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S2MPS11_IRQ_RTCA1_MASK,
},
[S2MPS11_IRQ_RTCA2] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S2MPS11_IRQ_RTCA2_MASK,
},
[S2MPS11_IRQ_SMPL] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S2MPS11_IRQ_SMPL_MASK,
},
[S2MPS11_IRQ_RTC1S] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S2MPS11_IRQ_RTC1S_MASK,
},
[S2MPS11_IRQ_WTSR] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S2MPS11_IRQ_WTSR_MASK,
},
[S2MPS11_IRQ_INT120C] = {
-   .reg_offset = 3,
+   .reg_offset = 2,
.mask = S2MPS11_IRQ_INT120C_MASK,
},
[S2MPS11_IRQ_INT140C] = {
-   .reg_offset = 3,
+   .reg_offset = 2,
.mask = S2MPS11_IRQ_INT140C_MASK,
},
 };
@@ -92,146 +92,146 @@ static struct regmap_irq s2mps11_irqs[] = {
 
 static struct regmap_irq s5m8767_irqs[] = {
[S5M8767_IRQ_PWRR] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S5M8767_IRQ_PWRR_MASK,
},
[S5M8767_IRQ_PWRF] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S5M8767_IRQ_PWRF_MASK,
},
[S5M8767_IRQ_PWR1S] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S5M8767_IRQ_PWR1S_MASK,
},
[S5M8767_IRQ_JIGR] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S5M8767_IRQ_JIGR_MASK,
},
[S5M8767_IRQ_JIGF] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S5M8767_IRQ_JIGF_MASK,
},
[S5M8767_IRQ_LOWBAT2] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S5M8767_IRQ_LOWBAT2_MASK,
},
[S5M8767_IRQ_LOWBAT1] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S5M8767_IRQ_LOWBAT1_MASK,
},
[S5M8767_IRQ_MRB] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S5M8767_IRQ_MRB_MASK,
},
[S5M8767_IRQ_DVSOK2] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S5M8767_IRQ_DVSOK2_MASK,
},
[S5M8767_IRQ_DVSOK3] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S5M8767_IRQ_DVSOK3_MASK,
},
[S5M8767_IRQ_DVSOK4] = {
-   .reg_offset = 2,
+   .reg_offset = 1

[PATCH] MFD: SEC: Fix reg_offset for interrupt registers

[Inderpal Singh]

reg_offset is offset of the status/mask registers. Now, since status_base
and mask_base are pointing to corresponding first registers, reg_offset
should start from 0 otheriwse regmap_add_irq_chip will fail during probe.

Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
---
It is based on Samuel's for-next-merge branch of mfd-2.6 tree. 

 drivers/mfd/sec-irq.c |  102 -
 1 file changed, 51 insertions(+), 51 deletions(-)

diff --git a/drivers/mfd/sec-irq.c b/drivers/mfd/sec-irq.c
index c901fa5..0dd84e9 100644
--- a/drivers/mfd/sec-irq.c
+++ b/drivers/mfd/sec-irq.c
@@ -24,67 +24,67 @@
 
 static struct regmap_irq s2mps11_irqs[] = {
[S2MPS11_IRQ_PWRONF] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S2MPS11_IRQ_PWRONF_MASK,
},
[S2MPS11_IRQ_PWRONR] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S2MPS11_IRQ_PWRONR_MASK,
},
[S2MPS11_IRQ_JIGONBF] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S2MPS11_IRQ_JIGONBF_MASK,
},
[S2MPS11_IRQ_JIGONBR] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S2MPS11_IRQ_JIGONBR_MASK,
},
[S2MPS11_IRQ_ACOKBF] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S2MPS11_IRQ_ACOKBF_MASK,
},
[S2MPS11_IRQ_ACOKBR] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S2MPS11_IRQ_ACOKBR_MASK,
},
[S2MPS11_IRQ_PWRON1S] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S2MPS11_IRQ_PWRON1S_MASK,
},
[S2MPS11_IRQ_MRB] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S2MPS11_IRQ_MRB_MASK,
},
[S2MPS11_IRQ_RTC60S] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S2MPS11_IRQ_RTC60S_MASK,
},
[S2MPS11_IRQ_RTCA1] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S2MPS11_IRQ_RTCA1_MASK,
},
[S2MPS11_IRQ_RTCA2] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S2MPS11_IRQ_RTCA2_MASK,
},
[S2MPS11_IRQ_SMPL] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S2MPS11_IRQ_SMPL_MASK,
},
[S2MPS11_IRQ_RTC1S] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S2MPS11_IRQ_RTC1S_MASK,
},
[S2MPS11_IRQ_WTSR] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S2MPS11_IRQ_WTSR_MASK,
},
[S2MPS11_IRQ_INT120C] = {
-   .reg_offset = 3,
+   .reg_offset = 2,
.mask = S2MPS11_IRQ_INT120C_MASK,
},
[S2MPS11_IRQ_INT140C] = {
-   .reg_offset = 3,
+   .reg_offset = 2,
.mask = S2MPS11_IRQ_INT140C_MASK,
},
 };
@@ -92,146 +92,146 @@ static struct regmap_irq s2mps11_irqs[] = {
 
 static struct regmap_irq s5m8767_irqs[] = {
[S5M8767_IRQ_PWRR] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S5M8767_IRQ_PWRR_MASK,
},
[S5M8767_IRQ_PWRF] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S5M8767_IRQ_PWRF_MASK,
},
[S5M8767_IRQ_PWR1S] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S5M8767_IRQ_PWR1S_MASK,
},
[S5M8767_IRQ_JIGR] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S5M8767_IRQ_JIGR_MASK,
},
[S5M8767_IRQ_JIGF] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S5M8767_IRQ_JIGF_MASK,
},
[S5M8767_IRQ_LOWBAT2] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S5M8767_IRQ_LOWBAT2_MASK,
},
[S5M8767_IRQ_LOWBAT1] = {
-   .reg_offset = 1,
+   .reg_offset = 0,
.mask = S5M8767_IRQ_LOWBAT1_MASK,
},
[S5M8767_IRQ_MRB] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S5M8767_IRQ_MRB_MASK,
},
[S5M8767_IRQ_DVSOK2] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S5M8767_IRQ_DVSOK2_MASK,
},
[S5M8767_IRQ_DVSOK3] = {
-   .reg_offset = 2,
+   .reg_offset = 1,
.mask = S5M8767_IRQ_DVSOK3_MASK,
},
[S5M8767_IRQ_DVSOK4] = {
-   .reg_offset = 2

[PATCH] DMA: PL330: Add runtime pm support

[Inderpal Singh]

At the pl330's probe point the device is already in runtime resume state.
Hence to manage the device with runtime, the probe should do pm_runtime_put
and remove should do pm_runtime_get to balance with probe.

And in between, the device is being get/put at alloc_chan_resources and
free_chan_resources.

Signed-off-by: Inderpal Singh 
---
This patch is based on slave-dma's next branch and on top
of the clean up patches at [1].

[1] https://lkml.org/lkml/2012/10/5/169

 drivers/dma/pl330.c |   10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 2ee1538..5ae19ea 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "dmaengine.h"
 #define PL330_MAX_CHAN 8
@@ -2384,6 +2385,8 @@ static int pl330_alloc_chan_resources(struct dma_chan 
*chan)
struct dma_pl330_dmac *pdmac = pch->dmac;
unsigned long flags;
 
+   pm_runtime_get_sync(pdmac->ddma.dev);
+
spin_lock_irqsave(>lock, flags);
 
dma_cookie_init(chan);
@@ -2392,6 +2395,7 @@ static int pl330_alloc_chan_resources(struct dma_chan 
*chan)
pch->pl330_chid = pl330_request_channel(>pif);
if (!pch->pl330_chid) {
spin_unlock_irqrestore(>lock, flags);
+   pm_runtime_put(pdmac->ddma.dev);
return -ENOMEM;
}
 
@@ -2470,6 +2474,8 @@ static void pl330_free_chan_resources(struct dma_chan 
*chan)
list_splice_tail_init(>work_list, >dmac->desc_pool);
 
spin_unlock_irqrestore(>lock, flags);
+
+   pm_runtime_put(pch->dmac->ddma.dev);
 }
 
 static enum dma_status
@@ -2974,6 +2980,8 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
pi->pcfg.data_bus_width / 8, pi->pcfg.num_chan,
pi->pcfg.num_peri, pi->pcfg.num_events);
 
+   pm_runtime_put(pd->dev);
+
return 0;
 
 probe_err5:
@@ -3017,6 +3025,8 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
return -EBUSY;
}
 
+   pm_runtime_get(pdmac->ddma.dev);
+
dma_async_device_unregister(>ddma);
 
amba_set_drvdata(adev, NULL);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/4] DMA: PL330: Fix mem leaks and balance probe/remove

[Inderpal Singh]

On 13 October 2012 16:33, Jassi Brar  wrote:
> On Fri, Oct 5, 2012 at 3:47 PM, Inderpal Singh
>  wrote:
>> The first 2 patches of this series fix memory leaks because the memory
>> allocated for peripheral channels and DMA descriptors were not getting
>> freed.
>>
>> The last 2 patches balance the module's remove function.
>>
>> This series depends on "61c6e7531d3b66b3 DMA: PL330: Check the
>> pointer returned by kzalloc" which is on slave-dma's "fixes" branch.
>> Hence slave-dma tree's "next" branch was merged with "fixes" and
>> applied patch at [1] to fix the build error.
>>
>> [1] http://permalink.gmane.org/gmane.linux.kernel.next/24274
>>
>> Changes since v1:
>>  - Protect only list_add_tail with spin_locks
>>  - Return EBUSY from remove if channel is in use
>>  - unregister dma_device in remove
>>
>> Inderpal Singh (4):
>>   DMA: PL330: Free memory allocated for peripheral channels
>>   DMA: PL330: Change allocation method to properly free  DMA
>> descriptors
>>   DMA: PL330: Balance module remove function with probe
>>   DMA: PL330: unregister dma_device in module's remove function
>>
>>  drivers/dma/pl330.c |   53 
>> ---
>>  1 file changed, 38 insertions(+), 15 deletions(-)
>>
> All seem fine.
> Acked-by: Jassi Brar 
>
Thanks Jassi.

Vinod,
I have tested this series against your latest slave-dma ->fixes branch
which is based on 3.7-rc1.
The patchset applies cleanly and works fine.

Thanks,
Inder

> Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/4] DMA: PL330: Fix mem leaks and balance probe/remove

[Inderpal Singh]

On 13 October 2012 16:33, Jassi Brar jassisinghb...@gmail.com wrote:
 On Fri, Oct 5, 2012 at 3:47 PM, Inderpal Singh
 inderpal.si...@linaro.org wrote:
 The first 2 patches of this series fix memory leaks because the memory
 allocated for peripheral channels and DMA descriptors were not getting
 freed.

 The last 2 patches balance the module's remove function.

 This series depends on 61c6e7531d3b66b3 DMA: PL330: Check the
 pointer returned by kzalloc which is on slave-dma's fixes branch.
 Hence slave-dma tree's next branch was merged with fixes and
 applied patch at [1] to fix the build error.

 [1] http://permalink.gmane.org/gmane.linux.kernel.next/24274

 Changes since v1:
  - Protect only list_add_tail with spin_locks
  - Return EBUSY from remove if channel is in use
  - unregister dma_device in remove

 Inderpal Singh (4):
   DMA: PL330: Free memory allocated for peripheral channels
   DMA: PL330: Change allocation method to properly free  DMA
 descriptors
   DMA: PL330: Balance module remove function with probe
   DMA: PL330: unregister dma_device in module's remove function

  drivers/dma/pl330.c |   53 
 ---
  1 file changed, 38 insertions(+), 15 deletions(-)

 All seem fine.
 Acked-by: Jassi Brar jassisinghb...@gmail.com

Thanks Jassi.

Vinod,
I have tested this series against your latest slave-dma -fixes branch
which is based on 3.7-rc1.
The patchset applies cleanly and works fine.

Thanks,
Inder

 Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] DMA: PL330: Add runtime pm support

[Inderpal Singh]

At the pl330's probe point the device is already in runtime resume state.
Hence to manage the device with runtime, the probe should do pm_runtime_put
and remove should do pm_runtime_get to balance with probe.

And in between, the device is being get/put at alloc_chan_resources and
free_chan_resources.

Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
---
This patch is based on slave-dma's next branch and on top
of the clean up patches at [1].

[1] https://lkml.org/lkml/2012/10/5/169

 drivers/dma/pl330.c |   10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 2ee1538..5ae19ea 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -25,6 +25,7 @@
 #include linux/amba/pl330.h
 #include linux/scatterlist.h
 #include linux/of.h
+#include linux/pm_runtime.h
 
 #include dmaengine.h
 #define PL330_MAX_CHAN 8
@@ -2384,6 +2385,8 @@ static int pl330_alloc_chan_resources(struct dma_chan 
*chan)
struct dma_pl330_dmac *pdmac = pch-dmac;
unsigned long flags;
 
+   pm_runtime_get_sync(pdmac-ddma.dev);
+
spin_lock_irqsave(pch-lock, flags);
 
dma_cookie_init(chan);
@@ -2392,6 +2395,7 @@ static int pl330_alloc_chan_resources(struct dma_chan 
*chan)
pch-pl330_chid = pl330_request_channel(pdmac-pif);
if (!pch-pl330_chid) {
spin_unlock_irqrestore(pch-lock, flags);
+   pm_runtime_put(pdmac-ddma.dev);
return -ENOMEM;
}
 
@@ -2470,6 +2474,8 @@ static void pl330_free_chan_resources(struct dma_chan 
*chan)
list_splice_tail_init(pch-work_list, pch-dmac-desc_pool);
 
spin_unlock_irqrestore(pch-lock, flags);
+
+   pm_runtime_put(pch-dmac-ddma.dev);
 }
 
 static enum dma_status
@@ -2974,6 +2980,8 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
pi-pcfg.data_bus_width / 8, pi-pcfg.num_chan,
pi-pcfg.num_peri, pi-pcfg.num_events);
 
+   pm_runtime_put(pd-dev);
+
return 0;
 
 probe_err5:
@@ -3017,6 +3025,8 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
return -EBUSY;
}
 
+   pm_runtime_get(pdmac-ddma.dev);
+
dma_async_device_unregister(pdmac-ddma);
 
amba_set_drvdata(adev, NULL);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/4] DMA: PL330: Fix mem leaks and balance probe/remove

[Inderpal Singh]

Hello,

On 5 October 2012 06:17, Inderpal Singh  wrote:
> The first 2 patches of this series fix memory leaks because the memory
> allocated for peripheral channels and DMA descriptors were not getting
> freed.
>
> The last 2 patches balance the module's remove function.
>
> This series depends on "61c6e7531d3b66b3 DMA: PL330: Check the
> pointer returned by kzalloc" which is on slave-dma's "fixes" branch.
> Hence slave-dma tree's "next" branch was merged with "fixes" and
> applied patch at [1] to fix the build error.
>
> [1] http://permalink.gmane.org/gmane.linux.kernel.next/24274
>
> Changes since v1:
>  - Protect only list_add_tail with spin_locks
>  - Return EBUSY from remove if channel is in use
>  - unregister dma_device in remove
>
> Inderpal Singh (4):
>   DMA: PL330: Free memory allocated for peripheral channels
>   DMA: PL330: Change allocation method to properly free  DMA
> descriptors
>   DMA: PL330: Balance module remove function with probe
>   DMA: PL330: unregister dma_device in module's remove function
>
>  drivers/dma/pl330.c |   53 
> ---
>  1 file changed, 38 insertions(+), 15 deletions(-)
>

Any comments on this v2 version of the patchset?

Thanks,
Inder

> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 0/4] DMA: PL330: Fix mem leaks and balance probe/remove

[Inderpal Singh]

Hello,

On 5 October 2012 06:17, Inderpal Singh inderpal.si...@linaro.org wrote:
 The first 2 patches of this series fix memory leaks because the memory
 allocated for peripheral channels and DMA descriptors were not getting
 freed.

 The last 2 patches balance the module's remove function.

 This series depends on 61c6e7531d3b66b3 DMA: PL330: Check the
 pointer returned by kzalloc which is on slave-dma's fixes branch.
 Hence slave-dma tree's next branch was merged with fixes and
 applied patch at [1] to fix the build error.

 [1] http://permalink.gmane.org/gmane.linux.kernel.next/24274

 Changes since v1:
  - Protect only list_add_tail with spin_locks
  - Return EBUSY from remove if channel is in use
  - unregister dma_device in remove

 Inderpal Singh (4):
   DMA: PL330: Free memory allocated for peripheral channels
   DMA: PL330: Change allocation method to properly free  DMA
 descriptors
   DMA: PL330: Balance module remove function with probe
   DMA: PL330: unregister dma_device in module's remove function

  drivers/dma/pl330.c |   53 
 ---
  1 file changed, 38 insertions(+), 15 deletions(-)


Any comments on this v2 version of the patchset?

Thanks,
Inder

 --
 1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 2/4] DMA: PL330: Change allocation method to properly free DMA descriptors

[Inderpal Singh]

In probe, memory for multiple DMA descriptors were being allocated at once
and then it was being split and added into DMA pool one by one. The address
of this memory allocation is not being saved anywhere. To free this memory,
the address is required. Initially the first node of the pool will be
pointed by this address but as we use this pool the descs will shuffle and
hence we will lose the track of the address.

This patch does following:

1. Allocates DMA descs one by one and then adds them to pool so that all
   descs can be fetched and memory freed one by one. This way runtime
   added descs can also be freed.
2. Free DMA descs in case of error in probe and in module's remove function

Signed-off-by: Inderpal Singh 
---
 drivers/dma/pl330.c |   35 +--
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 10c6b6a..bf71ff7 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2541,20 +2541,20 @@ static int add_desc(struct dma_pl330_dmac *pdmac, gfp_t 
flg, int count)
if (!pdmac)
return 0;
 
-   desc = kmalloc(count * sizeof(*desc), flg);
-   if (!desc)
-   return 0;
+   for (i = 0; i < count; i++) {
+   desc = kmalloc(sizeof(*desc), flg);
+   if (!desc)
+   break;
+   _init_desc(desc);
 
-   spin_lock_irqsave(>pool_lock, flags);
+   spin_lock_irqsave(>pool_lock, flags);
 
-   for (i = 0; i < count; i++) {
-   _init_desc([i]);
-   list_add_tail([i].node, >desc_pool);
-   }
+   list_add_tail(>node, >desc_pool);
 
-   spin_unlock_irqrestore(>pool_lock, flags);
+   spin_unlock_irqrestore(>pool_lock, flags);
+   }
 
-   return count;
+   return i;
 }
 
 static struct dma_pl330_desc *
@@ -2857,6 +2857,7 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
struct dma_pl330_platdata *pdat;
struct dma_pl330_dmac *pdmac;
struct dma_pl330_chan *pch;
+   struct dma_pl330_desc *desc;
struct pl330_info *pi;
struct dma_device *pd;
struct resource *res;
@@ -2978,6 +2979,12 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
 probe_err5:
kfree(pdmac->peripherals);
 probe_err4:
+   while (!list_empty(>desc_pool)) {
+   desc = list_entry(pdmac->desc_pool.next,
+   struct dma_pl330_desc, node);
+   list_del(>node);
+   kfree(desc);
+   }
pl330_del(pi);
 probe_err3:
free_irq(irq, pi);
@@ -2994,6 +3001,7 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
 {
struct dma_pl330_dmac *pdmac = amba_get_drvdata(adev);
struct dma_pl330_chan *pch, *_p;
+   struct dma_pl330_desc *desc;
struct pl330_info *pi;
struct resource *res;
int irq;
@@ -3015,6 +3023,13 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
pl330_free_chan_resources(>chan);
}
 
+   while (!list_empty(>desc_pool)) {
+   desc = list_entry(pdmac->desc_pool.next,
+   struct dma_pl330_desc, node);
+   list_del(>node);
+   kfree(desc);
+   }
+
pi = >pif;
 
pl330_del(pi);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 4/4] DMA: PL330: unregister dma_device in module's remove function

[Inderpal Singh]

unregister dma_device in module's remove function.

Signed-off-by: Inderpal Singh 
---
 drivers/dma/pl330.c |2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 4b7a34d..e7dc040 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -3017,6 +3017,8 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
return -EBUSY;
}
 
+   dma_async_device_unregister(>ddma);
+
amba_set_drvdata(adev, NULL);
 
list_for_each_entry_safe(pch, _p, >ddma.channels,
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 3/4] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

Since peripheral channel resources are not being allocated at probe,
no need to flush the channels and free the resources in remove function.
In case, the channel is in use by some client, return EBUSY.

Signed-off-by: Inderpal Singh 
---
 drivers/dma/pl330.c |   13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index bf71ff7..4b7a34d 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -3009,18 +3009,21 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
if (!pdmac)
return 0;
 
+   /* check if any client is using any channel */
+   list_for_each_entry(pch, >ddma.channels,
+   chan.device_node) {
+
+   if (pch->chan.client_count)
+   return -EBUSY;
+   }
+
amba_set_drvdata(adev, NULL);
 
-   /* Idle the DMAC */
list_for_each_entry_safe(pch, _p, >ddma.channels,
chan.device_node) {
 
/* Remove the channel */
list_del(>chan.device_node);
-
-   /* Flush the channel */
-   pl330_control(>chan, DMA_TERMINATE_ALL, 0);
-   pl330_free_chan_resources(>chan);
}
 
while (!list_empty(>desc_pool)) {
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 1/4] DMA: PL330: Free memory allocated for peripheral channels

[Inderpal Singh]

The allocated memory for peripheral channels is not being freed upon
failure in probe and in module's remove funtion. It will lead to memory
leakage. Hence free the allocated memory.

Signed-off-by: Inderpal Singh 
---
 drivers/dma/pl330.c |5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 2ebd4cd..10c6b6a 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2962,7 +2962,7 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
ret = dma_async_device_register(pd);
if (ret) {
dev_err(>dev, "unable to register DMAC\n");
-   goto probe_err4;
+   goto probe_err5;
}
 
dev_info(>dev,
@@ -2975,6 +2975,8 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
 
return 0;
 
+probe_err5:
+   kfree(pdmac->peripherals);
 probe_err4:
pl330_del(pi);
 probe_err3:
@@ -3025,6 +3027,7 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
res = >res;
release_mem_region(res->start, resource_size(res));
 
+   kfree(pdmac->peripherals);
kfree(pdmac);
 
return 0;
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 0/4] DMA: PL330: Fix mem leaks and balance probe/remove

[Inderpal Singh]

The first 2 patches of this series fix memory leaks because the memory
allocated for peripheral channels and DMA descriptors were not getting
freed.

The last 2 patches balance the module's remove function.

This series depends on "61c6e7531d3b66b3 DMA: PL330: Check the
pointer returned by kzalloc" which is on slave-dma's "fixes" branch. 
Hence slave-dma tree's "next" branch was merged with "fixes" and 
applied patch at [1] to fix the build error.

[1] http://permalink.gmane.org/gmane.linux.kernel.next/24274

Changes since v1:
 - Protect only list_add_tail with spin_locks
 - Return EBUSY from remove if channel is in use
 - unregister dma_device in remove
 
Inderpal Singh (4):
  DMA: PL330: Free memory allocated for peripheral channels
  DMA: PL330: Change allocation method to properly free  DMA
descriptors
  DMA: PL330: Balance module remove function with probe
  DMA: PL330: unregister dma_device in module's remove function

 drivers/dma/pl330.c |   53 ---
 1 file changed, 38 insertions(+), 15 deletions(-)

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 0/4] DMA: PL330: Fix mem leaks and balance probe/remove

[Inderpal Singh]

The first 2 patches of this series fix memory leaks because the memory
allocated for peripheral channels and DMA descriptors were not getting
freed.

The last 2 patches balance the module's remove function.

This series depends on 61c6e7531d3b66b3 DMA: PL330: Check the
pointer returned by kzalloc which is on slave-dma's fixes branch. 
Hence slave-dma tree's next branch was merged with fixes and 
applied patch at [1] to fix the build error.

[1] http://permalink.gmane.org/gmane.linux.kernel.next/24274

Changes since v1:
 - Protect only list_add_tail with spin_locks
 - Return EBUSY from remove if channel is in use
 - unregister dma_device in remove
 
Inderpal Singh (4):
  DMA: PL330: Free memory allocated for peripheral channels
  DMA: PL330: Change allocation method to properly free  DMA
descriptors
  DMA: PL330: Balance module remove function with probe
  DMA: PL330: unregister dma_device in module's remove function

 drivers/dma/pl330.c |   53 ---
 1 file changed, 38 insertions(+), 15 deletions(-)

-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 1/4] DMA: PL330: Free memory allocated for peripheral channels

[Inderpal Singh]

The allocated memory for peripheral channels is not being freed upon
failure in probe and in module's remove funtion. It will lead to memory
leakage. Hence free the allocated memory.

Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
---
 drivers/dma/pl330.c |5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 2ebd4cd..10c6b6a 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2962,7 +2962,7 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
ret = dma_async_device_register(pd);
if (ret) {
dev_err(adev-dev, unable to register DMAC\n);
-   goto probe_err4;
+   goto probe_err5;
}
 
dev_info(adev-dev,
@@ -2975,6 +2975,8 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
 
return 0;
 
+probe_err5:
+   kfree(pdmac-peripherals);
 probe_err4:
pl330_del(pi);
 probe_err3:
@@ -3025,6 +3027,7 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
res = adev-res;
release_mem_region(res-start, resource_size(res));
 
+   kfree(pdmac-peripherals);
kfree(pdmac);
 
return 0;
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 3/4] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

Since peripheral channel resources are not being allocated at probe,
no need to flush the channels and free the resources in remove function.
In case, the channel is in use by some client, return EBUSY.

Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
---
 drivers/dma/pl330.c |   13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index bf71ff7..4b7a34d 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -3009,18 +3009,21 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
if (!pdmac)
return 0;
 
+   /* check if any client is using any channel */
+   list_for_each_entry(pch, pdmac-ddma.channels,
+   chan.device_node) {
+
+   if (pch-chan.client_count)
+   return -EBUSY;
+   }
+
amba_set_drvdata(adev, NULL);
 
-   /* Idle the DMAC */
list_for_each_entry_safe(pch, _p, pdmac-ddma.channels,
chan.device_node) {
 
/* Remove the channel */
list_del(pch-chan.device_node);
-
-   /* Flush the channel */
-   pl330_control(pch-chan, DMA_TERMINATE_ALL, 0);
-   pl330_free_chan_resources(pch-chan);
}
 
while (!list_empty(pdmac-desc_pool)) {
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 4/4] DMA: PL330: unregister dma_device in module's remove function

[Inderpal Singh]

unregister dma_device in module's remove function.

Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
---
 drivers/dma/pl330.c |2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 4b7a34d..e7dc040 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -3017,6 +3017,8 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
return -EBUSY;
}
 
+   dma_async_device_unregister(pdmac-ddma);
+
amba_set_drvdata(adev, NULL);
 
list_for_each_entry_safe(pch, _p, pdmac-ddma.channels,
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 2/4] DMA: PL330: Change allocation method to properly free DMA descriptors

[Inderpal Singh]

In probe, memory for multiple DMA descriptors were being allocated at once
and then it was being split and added into DMA pool one by one. The address
of this memory allocation is not being saved anywhere. To free this memory,
the address is required. Initially the first node of the pool will be
pointed by this address but as we use this pool the descs will shuffle and
hence we will lose the track of the address.

This patch does following:

1. Allocates DMA descs one by one and then adds them to pool so that all
   descs can be fetched and memory freed one by one. This way runtime
   added descs can also be freed.
2. Free DMA descs in case of error in probe and in module's remove function

Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
---
 drivers/dma/pl330.c |   35 +--
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 10c6b6a..bf71ff7 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2541,20 +2541,20 @@ static int add_desc(struct dma_pl330_dmac *pdmac, gfp_t 
flg, int count)
if (!pdmac)
return 0;
 
-   desc = kmalloc(count * sizeof(*desc), flg);
-   if (!desc)
-   return 0;
+   for (i = 0; i  count; i++) {
+   desc = kmalloc(sizeof(*desc), flg);
+   if (!desc)
+   break;
+   _init_desc(desc);
 
-   spin_lock_irqsave(pdmac-pool_lock, flags);
+   spin_lock_irqsave(pdmac-pool_lock, flags);
 
-   for (i = 0; i  count; i++) {
-   _init_desc(desc[i]);
-   list_add_tail(desc[i].node, pdmac-desc_pool);
-   }
+   list_add_tail(desc-node, pdmac-desc_pool);
 
-   spin_unlock_irqrestore(pdmac-pool_lock, flags);
+   spin_unlock_irqrestore(pdmac-pool_lock, flags);
+   }
 
-   return count;
+   return i;
 }
 
 static struct dma_pl330_desc *
@@ -2857,6 +2857,7 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
struct dma_pl330_platdata *pdat;
struct dma_pl330_dmac *pdmac;
struct dma_pl330_chan *pch;
+   struct dma_pl330_desc *desc;
struct pl330_info *pi;
struct dma_device *pd;
struct resource *res;
@@ -2978,6 +2979,12 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
 probe_err5:
kfree(pdmac-peripherals);
 probe_err4:
+   while (!list_empty(pdmac-desc_pool)) {
+   desc = list_entry(pdmac-desc_pool.next,
+   struct dma_pl330_desc, node);
+   list_del(desc-node);
+   kfree(desc);
+   }
pl330_del(pi);
 probe_err3:
free_irq(irq, pi);
@@ -2994,6 +3001,7 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
 {
struct dma_pl330_dmac *pdmac = amba_get_drvdata(adev);
struct dma_pl330_chan *pch, *_p;
+   struct dma_pl330_desc *desc;
struct pl330_info *pi;
struct resource *res;
int irq;
@@ -3015,6 +3023,13 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
pl330_free_chan_resources(pch-chan);
}
 
+   while (!list_empty(pdmac-desc_pool)) {
+   desc = list_entry(pdmac-desc_pool.next,
+   struct dma_pl330_desc, node);
+   list_del(desc-node);
+   kfree(desc);
+   }
+
pi = pdmac-pif;
 
pl330_del(pi);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

On 28 September 2012 16:28, Jassi Brar  wrote:
> On Fri, Sep 28, 2012 at 10:03 AM, Inderpal Singh
>  wrote:
>>
>> Now, if we have to check if any client is using the channel and then
>> decide. We will have to traverse the channel list twice once to check
>> the usage and second time to delete the nodes from the list if we go
>> ahead with remove.
>> The remove will look like below:
>>
>> @@ -3008,18 +3008,19 @@ static int __devexit pl330_remove(struct
>> amba_device *adev)
>> if (!pdmac)
>> return 0;
>>
>> +   /* check if any client is using any channel */
>> +   list_for_each_entry_safe(pch, _p, >ddma.channels,
>> +   chan.device_node) {
>> +
>> +   if (pch->chan.client_count)
>> +   return -EBUSY;
>> +   }
>> +
> The iteration doesn't have to be the 'safe' version here. Other than
> that it seems OK.

Ok. I will update the patch and send again.

Thanks,
Inder
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

On 28 September 2012 16:28, Jassi Brar jassisinghb...@gmail.com wrote:
 On Fri, Sep 28, 2012 at 10:03 AM, Inderpal Singh
 inderpal.si...@linaro.org wrote:

 Now, if we have to check if any client is using the channel and then
 decide. We will have to traverse the channel list twice once to check
 the usage and second time to delete the nodes from the list if we go
 ahead with remove.
 The remove will look like below:

 @@ -3008,18 +3008,19 @@ static int __devexit pl330_remove(struct
 amba_device *adev)
 if (!pdmac)
 return 0;

 +   /* check if any client is using any channel */
 +   list_for_each_entry_safe(pch, _p, pdmac-ddma.channels,
 +   chan.device_node) {
 +
 +   if (pch-chan.client_count)
 +   return -EBUSY;
 +   }
 +
 The iteration doesn't have to be the 'safe' version here. Other than
 that it seems OK.

Ok. I will update the patch and send again.

Thanks,
Inder
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

On 27 September 2012 21:36, Jassi Brar  wrote:
> On Thu, Sep 27, 2012 at 9:11 PM, Inderpal Singh
>  wrote:
>> On 27 September 2012 15:18, Vinod Koul  wrote:
>>> On Wed, 2012-09-26 at 12:11 +0530, Inderpal Singh wrote:
>>>> If we fail pl330_remove while some client is queued, the force unload
>>>> will fail and the
>>>> force unload will lose its purpose.
>>>> How about conditionally DMA_TERMINATE_ALL and free resources like
>>>> below ?
>>> Why would you want to remove the driver when it is doing something
>>> useful? You have to ensure driver is not doing anything.
>>>
>>> What is point here?
>>>
>> As mentioned by jassi,  if the pl330 module is forced unloaded while
>> some client is queued, we have to manually do DMA_TERMINATE_ALL.
>>
> I meant that in the current situation. Not ideally.
>
>> If failing remove is a better option in case some client is queued, we
>> can do away with DMA_TERMINATE_ALL and free_chan_resources and simply
>> return a suitable error code from remove.
>>
> That was exactly what I suggested as an alternative.

Yes, but our discussion went about continue doing DMA_TERMINATE_ALL and freeing.

Now, if we have to check if any client is using the channel and then
decide. We will have to traverse the channel list twice once to check
the usage and second time to delete the nodes from the list if we go
ahead with remove.
The remove will look like below:

@@ -3008,18 +3008,19 @@ static int __devexit pl330_remove(struct
amba_device *adev)
if (!pdmac)
return 0;

+   /* check if any client is using any channel */
+   list_for_each_entry_safe(pch, _p, >ddma.channels,
+   chan.device_node) {
+
+   if (pch->chan.client_count)
+   return -EBUSY;
+   }
+
amba_set_drvdata(adev, NULL);

-   /* Idle the DMAC */
list_for_each_entry_safe(pch, _p, >ddma.channels,
chan.device_node) {

/* Remove the channel */
list_del(>chan.device_node);
-
-   /* Flush the channel */
-   pl330_control(>chan, DMA_TERMINATE_ALL, 0);
-   pl330_free_chan_resources(>chan);
}

Please suggest if there is any better way to do it.

Thanks,
Inder
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

On 27 September 2012 15:18, Vinod Koul  wrote:
> On Wed, 2012-09-26 at 12:11 +0530, Inderpal Singh wrote:
>> If we fail pl330_remove while some client is queued, the force unload
>> will fail and the
>> force unload will lose its purpose.
>> How about conditionally DMA_TERMINATE_ALL and free resources like
>> below ?
> Why would you want to remove the driver when it is doing something
> useful? You have to ensure driver is not doing anything.
>
> What is point here?
>
As mentioned by jassi,  if the pl330 module is forced unloaded while
some client is queued, we have to manually do DMA_TERMINATE_ALL.

If failing remove is a better option in case some client is queued, we
can do away with DMA_TERMINATE_ALL and free_chan_resources and simply
return a suitable error code from remove.

Kindly suggest.

Thanks,
Inder

> --
> ~Vinod
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

On 27 September 2012 15:18, Vinod Koul vinod.k...@linux.intel.com wrote:
 On Wed, 2012-09-26 at 12:11 +0530, Inderpal Singh wrote:
 If we fail pl330_remove while some client is queued, the force unload
 will fail and the
 force unload will lose its purpose.
 How about conditionally DMA_TERMINATE_ALL and free resources like
 below ?
 Why would you want to remove the driver when it is doing something
 useful? You have to ensure driver is not doing anything.

 What is point here?

As mentioned by jassi,  if the pl330 module is forced unloaded while
some client is queued, we have to manually do DMA_TERMINATE_ALL.

If failing remove is a better option in case some client is queued, we
can do away with DMA_TERMINATE_ALL and free_chan_resources and simply
return a suitable error code from remove.

Kindly suggest.

Thanks,
Inder

 --
 ~Vinod

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

On 27 September 2012 21:36, Jassi Brar jassisinghb...@gmail.com wrote:
 On Thu, Sep 27, 2012 at 9:11 PM, Inderpal Singh
 inderpal.si...@linaro.org wrote:
 On 27 September 2012 15:18, Vinod Koul vinod.k...@linux.intel.com wrote:
 On Wed, 2012-09-26 at 12:11 +0530, Inderpal Singh wrote:
 If we fail pl330_remove while some client is queued, the force unload
 will fail and the
 force unload will lose its purpose.
 How about conditionally DMA_TERMINATE_ALL and free resources like
 below ?
 Why would you want to remove the driver when it is doing something
 useful? You have to ensure driver is not doing anything.

 What is point here?

 As mentioned by jassi,  if the pl330 module is forced unloaded while
 some client is queued, we have to manually do DMA_TERMINATE_ALL.

 I meant that in the current situation. Not ideally.

 If failing remove is a better option in case some client is queued, we
 can do away with DMA_TERMINATE_ALL and free_chan_resources and simply
 return a suitable error code from remove.

 That was exactly what I suggested as an alternative.

Yes, but our discussion went about continue doing DMA_TERMINATE_ALL and freeing.

Now, if we have to check if any client is using the channel and then
decide. We will have to traverse the channel list twice once to check
the usage and second time to delete the nodes from the list if we go
ahead with remove.
The remove will look like below:

@@ -3008,18 +3008,19 @@ static int __devexit pl330_remove(struct
amba_device *adev)
if (!pdmac)
return 0;

+   /* check if any client is using any channel */
+   list_for_each_entry_safe(pch, _p, pdmac-ddma.channels,
+   chan.device_node) {
+
+   if (pch-chan.client_count)
+   return -EBUSY;
+   }
+
amba_set_drvdata(adev, NULL);

-   /* Idle the DMAC */
list_for_each_entry_safe(pch, _p, pdmac-ddma.channels,
chan.device_node) {

/* Remove the channel */
list_del(pch-chan.device_node);
-
-   /* Flush the channel */
-   pl330_control(pch-chan, DMA_TERMINATE_ALL, 0);
-   pl330_free_chan_resources(pch-chan);
}

Please suggest if there is any better way to do it.

Thanks,
Inder
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

On 27 September 2012 10:35, Jassi Brar  wrote:
> On Thu, Sep 27, 2012 at 9:43 AM, Inderpal Singh
>  wrote:
>>
>> Don't you think free_chan_resource should be done __only if__
>> alloc_chan_resource was successful ?
>>
> No, I don't think so. Thanks.

Thanks for quick response.
Please elaborate more on this as I find it against the general rule
and against the dmaengine implementation which checks on the same
condition before proceeding for free_chan_resouces in dma_chan_put
function.

Thanks,
Inder
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

On 26 September 2012 22:19, Jassi Brar  wrote:
> On Wed, Sep 26, 2012 at 4:25 PM, Inderpal Singh
>  wrote:
>> On 26 September 2012 15:02, Jassi Brar  wrote:
>>> On Wed, Sep 26, 2012 at 12:11 PM, Inderpal Singh
>>>  wrote:
>>>
>>>> How about conditionally DMA_TERMINATE_ALL and free resources like below ?
>>>>
>>>> @@ -3017,9 +3017,11 @@ static int __devexit pl330_remove(struct
>>>> amba_device *adev)
>>>> /* Remove the channel */
>>>> list_del(>chan.device_node);
>>>>
>>>> -   /* Flush the channel */
>>>> -   pl330_control(>chan, DMA_TERMINATE_ALL, 0);
>>>> -   pl330_free_chan_resources(>chan);
>>>> +   if (pch->chan.client_count != 0) {
>>>> +   /* Flush the channel */
>>>> +   pl330_control(>chan, DMA_TERMINATE_ALL, 0);
>>>> +   pl330_free_chan_resources(>chan);
>>>> +   }
>>>> }
>>>>
>>> It is perfectly safe to flush the channels that have client_count == 0
>>> as well. Which is already the case.
>>
>> As per my understanding, if client_count ==0, It may mean following:
>>
>> 1. This channel was never allocated to any client. Hence
>> alloc_chan_resources was not done for it.
>> So, no need to flush and free resources if it was never allocated at
>> the first place. If we free_chan_resources, it will not be balanced
>> against alloc_chan_resources. Scenario: insmod and then rmmod.
>>
>> Or,
>>
>> 2. The channel was allocated to some client, alloc_chan_resource was
>> done, the work for the client is completed and free_chan_resources was
>> performed. Now  since resources have already been freed, no need to
>> flush and free_chan_resources again in remove.
>>
>> The whole idea of this patch is to balance alloc_chan_resources and
>> free_chan_resources.
>>
> Do you see any issue with channels that have zero client_count ?

As I explained in my previous mail, If the client_count is zero,
either the alloc_chan_resources is not done(or failed) for that
channel or the free_chan_resource have already been done. Please refer
below code from dmaengine.c

static void dma_chan_put(struct dma_chan *chan)
{
if (!chan->client_count)
return; /* this channel failed alloc_chan_resources */
chan->client_count--;
module_put(dma_chan_to_owner(chan));
if (chan->client_count == 0)
chan->device->device_free_chan_resources(chan);
}

As you mentioned channel flush is needed if some client is queued and
force unload happens.
I am not against flushing and freeing channels. I am __only__
suggesting to flush and free channels for which alloc_chan_resource
was successful, which can be decided by "chan->client_count" as being
done in above function.

Don't you think free_chan_resource should be done __only if__
alloc_chan_resource was successful ?

Thanks,
Inder

> AFAIU there are already enough checks in the path to weed out unused
> channels, so let us please not uglify the code by adding new
> unnecessary checks.
>
> thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

On 26 September 2012 15:02, Jassi Brar  wrote:
> On Wed, Sep 26, 2012 at 12:11 PM, Inderpal Singh
>  wrote:
>
>> How about conditionally DMA_TERMINATE_ALL and free resources like below ?
>>
>> @@ -3017,9 +3017,11 @@ static int __devexit pl330_remove(struct
>> amba_device *adev)
>> /* Remove the channel */
>> list_del(>chan.device_node);
>>
>> -   /* Flush the channel */
>> -   pl330_control(>chan, DMA_TERMINATE_ALL, 0);
>> -   pl330_free_chan_resources(>chan);
>> +   if (pch->chan.client_count != 0) {
>> +   /* Flush the channel */
>> +   pl330_control(>chan, DMA_TERMINATE_ALL, 0);
>> +   pl330_free_chan_resources(>chan);
>> +   }
>> }
>>
> It is perfectly safe to flush the channels that have client_count == 0
> as well. Which is already the case.

As per my understanding, if client_count ==0, It may mean following:

1. This channel was never allocated to any client. Hence
alloc_chan_resources was not done for it.
So, no need to flush and free resources if it was never allocated at
the first place. If we free_chan_resources, it will not be balanced
against alloc_chan_resources. Scenario: insmod and then rmmod.

Or,

2. The channel was allocated to some client, alloc_chan_resource was
done, the work for the client is completed and free_chan_resources was
performed. Now  since resources have already been freed, no need to
flush and free_chan_resources again in remove.

The whole idea of this patch is to balance alloc_chan_resources and
free_chan_resources.

Thanks,
Inder
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

On 25 September 2012 18:47, Jassi Brar  wrote:
> On Tue, Sep 25, 2012 at 2:27 PM, Inderpal Singh
>  wrote:
>> Since peripheral channel resources are not being allocated at probe,
>> no need to flush the channels and free the resources in remove function.
>>
>> Signed-off-by: Inderpal Singh 
>> ---
>>  drivers/dma/pl330.c |8 +---
>>  1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index 04d83e6..6f06080 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -3012,16 +3012,10 @@ static int __devexit pl330_remove(struct amba_device 
>> *adev)
>>
>> /* Idle the DMAC */
>> list_for_each_entry_safe(pch, _p, >ddma.channels,
>> -   chan.device_node) {
>> -
>> +   chan.device_node)
>> /* Remove the channel */
>> list_del(>chan.device_node);
>>
>> -   /* Flush the channel */
>> -   pl330_control(>chan, DMA_TERMINATE_ALL, 0);
>> -   pl330_free_chan_resources(>chan);
>> -   }
>> -
>> while (!list_empty(>desc_pool)) {
>> desc = list_entry(pdmac->desc_pool.next,
>> struct dma_pl330_desc, node);
>
> I am not sure about this patch. The DMA_TERMINATE_ALL is only called
> by the client and if the pl330 module is forced unloaded while some
> client is queued, we have to manually do DMA_TERMINATE_ALL.
> A better option could be to simply fail pl330_remove if some client is
> queued on the DMAC.
>
If we fail pl330_remove while some client is queued, the force unload
will fail and the
force unload will lose its purpose.
How about conditionally DMA_TERMINATE_ALL and free resources like below ?

@@ -3017,9 +3017,11 @@ static int __devexit pl330_remove(struct
amba_device *adev)
/* Remove the channel */
list_del(>chan.device_node);

-   /* Flush the channel */
-   pl330_control(>chan, DMA_TERMINATE_ALL, 0);
-   pl330_free_chan_resources(>chan);
+   if (pch->chan.client_count != 0) {
+   /* Flush the channel */
+   pl330_control(>chan, DMA_TERMINATE_ALL, 0);
+   pl330_free_chan_resources(>chan);
+   }
}

while (!list_empty(>desc_pool)) {


Thanks,
Inder


> -jassi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

On 25 September 2012 18:47, Jassi Brar jassisinghb...@gmail.com wrote:
 On Tue, Sep 25, 2012 at 2:27 PM, Inderpal Singh
 inderpal.si...@linaro.org wrote:
 Since peripheral channel resources are not being allocated at probe,
 no need to flush the channels and free the resources in remove function.

 Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
 ---
  drivers/dma/pl330.c |8 +---
  1 file changed, 1 insertion(+), 7 deletions(-)

 diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
 index 04d83e6..6f06080 100644
 --- a/drivers/dma/pl330.c
 +++ b/drivers/dma/pl330.c
 @@ -3012,16 +3012,10 @@ static int __devexit pl330_remove(struct amba_device 
 *adev)

 /* Idle the DMAC */
 list_for_each_entry_safe(pch, _p, pdmac-ddma.channels,
 -   chan.device_node) {
 -
 +   chan.device_node)
 /* Remove the channel */
 list_del(pch-chan.device_node);

 -   /* Flush the channel */
 -   pl330_control(pch-chan, DMA_TERMINATE_ALL, 0);
 -   pl330_free_chan_resources(pch-chan);
 -   }
 -
 while (!list_empty(pdmac-desc_pool)) {
 desc = list_entry(pdmac-desc_pool.next,
 struct dma_pl330_desc, node);

 I am not sure about this patch. The DMA_TERMINATE_ALL is only called
 by the client and if the pl330 module is forced unloaded while some
 client is queued, we have to manually do DMA_TERMINATE_ALL.
 A better option could be to simply fail pl330_remove if some client is
 queued on the DMAC.

If we fail pl330_remove while some client is queued, the force unload
will fail and the
force unload will lose its purpose.
How about conditionally DMA_TERMINATE_ALL and free resources like below ?

@@ -3017,9 +3017,11 @@ static int __devexit pl330_remove(struct
amba_device *adev)
/* Remove the channel */
list_del(pch-chan.device_node);

-   /* Flush the channel */
-   pl330_control(pch-chan, DMA_TERMINATE_ALL, 0);
-   pl330_free_chan_resources(pch-chan);
+   if (pch-chan.client_count != 0) {
+   /* Flush the channel */
+   pl330_control(pch-chan, DMA_TERMINATE_ALL, 0);
+   pl330_free_chan_resources(pch-chan);
+   }
}

while (!list_empty(pdmac-desc_pool)) {


Thanks,
Inder


 -jassi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

On 26 September 2012 15:02, Jassi Brar jassisinghb...@gmail.com wrote:
 On Wed, Sep 26, 2012 at 12:11 PM, Inderpal Singh
 inderpal.si...@linaro.org wrote:

 How about conditionally DMA_TERMINATE_ALL and free resources like below ?

 @@ -3017,9 +3017,11 @@ static int __devexit pl330_remove(struct
 amba_device *adev)
 /* Remove the channel */
 list_del(pch-chan.device_node);

 -   /* Flush the channel */
 -   pl330_control(pch-chan, DMA_TERMINATE_ALL, 0);
 -   pl330_free_chan_resources(pch-chan);
 +   if (pch-chan.client_count != 0) {
 +   /* Flush the channel */
 +   pl330_control(pch-chan, DMA_TERMINATE_ALL, 0);
 +   pl330_free_chan_resources(pch-chan);
 +   }
 }

 It is perfectly safe to flush the channels that have client_count == 0
 as well. Which is already the case.

As per my understanding, if client_count ==0, It may mean following:

1. This channel was never allocated to any client. Hence
alloc_chan_resources was not done for it.
So, no need to flush and free resources if it was never allocated at
the first place. If we free_chan_resources, it will not be balanced
against alloc_chan_resources. Scenario: insmod and then rmmod.

Or,

2. The channel was allocated to some client, alloc_chan_resource was
done, the work for the client is completed and free_chan_resources was
performed. Now  since resources have already been freed, no need to
flush and free_chan_resources again in remove.

The whole idea of this patch is to balance alloc_chan_resources and
free_chan_resources.

Thanks,
Inder
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

On 26 September 2012 22:19, Jassi Brar jassisinghb...@gmail.com wrote:
 On Wed, Sep 26, 2012 at 4:25 PM, Inderpal Singh
 inderpal.si...@linaro.org wrote:
 On 26 September 2012 15:02, Jassi Brar jassisinghb...@gmail.com wrote:
 On Wed, Sep 26, 2012 at 12:11 PM, Inderpal Singh
 inderpal.si...@linaro.org wrote:

 How about conditionally DMA_TERMINATE_ALL and free resources like below ?

 @@ -3017,9 +3017,11 @@ static int __devexit pl330_remove(struct
 amba_device *adev)
 /* Remove the channel */
 list_del(pch-chan.device_node);

 -   /* Flush the channel */
 -   pl330_control(pch-chan, DMA_TERMINATE_ALL, 0);
 -   pl330_free_chan_resources(pch-chan);
 +   if (pch-chan.client_count != 0) {
 +   /* Flush the channel */
 +   pl330_control(pch-chan, DMA_TERMINATE_ALL, 0);
 +   pl330_free_chan_resources(pch-chan);
 +   }
 }

 It is perfectly safe to flush the channels that have client_count == 0
 as well. Which is already the case.

 As per my understanding, if client_count ==0, It may mean following:

 1. This channel was never allocated to any client. Hence
 alloc_chan_resources was not done for it.
 So, no need to flush and free resources if it was never allocated at
 the first place. If we free_chan_resources, it will not be balanced
 against alloc_chan_resources. Scenario: insmod and then rmmod.

 Or,

 2. The channel was allocated to some client, alloc_chan_resource was
 done, the work for the client is completed and free_chan_resources was
 performed. Now  since resources have already been freed, no need to
 flush and free_chan_resources again in remove.

 The whole idea of this patch is to balance alloc_chan_resources and
 free_chan_resources.

 Do you see any issue with channels that have zero client_count ?

As I explained in my previous mail, If the client_count is zero,
either the alloc_chan_resources is not done(or failed) for that
channel or the free_chan_resource have already been done. Please refer
below code from dmaengine.c

static void dma_chan_put(struct dma_chan *chan)
{
if (!chan-client_count)
return; /* this channel failed alloc_chan_resources */
chan-client_count--;
module_put(dma_chan_to_owner(chan));
if (chan-client_count == 0)
chan-device-device_free_chan_resources(chan);
}

As you mentioned channel flush is needed if some client is queued and
force unload happens.
I am not against flushing and freeing channels. I am __only__
suggesting to flush and free channels for which alloc_chan_resource
was successful, which can be decided by chan-client_count as being
done in above function.

Don't you think free_chan_resource should be done __only if__
alloc_chan_resource was successful ?

Thanks,
Inder

 AFAIU there are already enough checks in the path to weed out unused
 channels, so let us please not uglify the code by adding new
 unnecessary checks.

 thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

On 27 September 2012 10:35, Jassi Brar jassisinghb...@gmail.com wrote:
 On Thu, Sep 27, 2012 at 9:43 AM, Inderpal Singh
 inderpal.si...@linaro.org wrote:

 Don't you think free_chan_resource should be done __only if__
 alloc_chan_resource was successful ?

 No, I don't think so. Thanks.

Thanks for quick response.
Please elaborate more on this as I find it against the general rule
and against the dmaengine implementation which checks on the same
condition before proceeding for free_chan_resouces in dma_chan_put
function.

Thanks,
Inder
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] DMA: PL330: Change allocation method to properly free DMA descriptors

[Inderpal Singh]

On 25 September 2012 18:39, Jassi Brar  wrote:
> On Tue, Sep 25, 2012 at 2:27 PM, Inderpal Singh
>  wrote:
>> In probe, memory for multiple DMA descriptors were being allocated at once
>> and then it was being split and added into DMA pool one by one. The address
>> of this memory allocation is not being saved anywhere. To free this memory,
>> the address is required. Initially the first node of the pool will be
>> pointed by this address but as we use this pool the descs will shuffle and
>> hence we will lose the track of the address.
>>
>> This patch does following:
>>
>> 1. Allocates DMA descs one by one and then adds them to pool so that all
>>descs can be fetched and memory freed one by one. This way runtime
>>added descs can also be freed.
>> 2. Free DMA descs in case of error in probe and in module's remove function
>>
>> Signed-off-by: Inderpal Singh 
>> ---
>>  drivers/dma/pl330.c |   28 +---
>>  1 file changed, 21 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index 10c6b6a..04d83e6 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -2541,20 +2541,19 @@ static int add_desc(struct dma_pl330_dmac *pdmac, 
>> gfp_t flg, int count)
>> if (!pdmac)
>> return 0;
>>
>> -   desc = kmalloc(count * sizeof(*desc), flg);
>> -   if (!desc)
>> -   return 0;
>> -
>> spin_lock_irqsave(>pool_lock, flags);
>>
>> for (i = 0; i < count; i++) {
>> -   _init_desc([i]);
>> -   list_add_tail([i].node, >desc_pool);
>> +   desc = kmalloc(sizeof(*desc), flg);
>> +   if (!desc)
>> +   break;
>> +   _init_desc(desc);
>> +   list_add_tail(>node, >desc_pool);
>> }
>>
>> spin_unlock_irqrestore(>pool_lock, flags);
>>
>> -   return count;
>> +   return i;
>>  }
>>
> OK, but the kmalloc shouldn't be done with irqs disabled. Please
> protect only the list_add_tail()
>
Yes, I missed it. I will update and send it again.

Thanks,
Inder

> thanks.
>
>>  static struct dma_pl330_desc *
>> @@ -2857,6 +2856,7 @@ pl330_probe(struct amba_device *adev, const struct 
>> amba_id *id)
>> struct dma_pl330_platdata *pdat;
>> struct dma_pl330_dmac *pdmac;
>> struct dma_pl330_chan *pch;
>> +   struct dma_pl330_desc *desc;
>> struct pl330_info *pi;
>> struct dma_device *pd;
>> struct resource *res;
>> @@ -2978,6 +2978,12 @@ pl330_probe(struct amba_device *adev, const struct 
>> amba_id *id)
>>  probe_err5:
>> kfree(pdmac->peripherals);
>>  probe_err4:
>> +   while (!list_empty(>desc_pool)) {
>> +   desc = list_entry(pdmac->desc_pool.next,
>> +   struct dma_pl330_desc, node);
>> +   list_del(>node);
>> +   kfree(desc);
>> +   }
>> pl330_del(pi);
>>  probe_err3:
>> free_irq(irq, pi);
>> @@ -2994,6 +3000,7 @@ static int __devexit pl330_remove(struct amba_device 
>> *adev)
>>  {
>> struct dma_pl330_dmac *pdmac = amba_get_drvdata(adev);
>> struct dma_pl330_chan *pch, *_p;
>> +   struct dma_pl330_desc *desc;
>> struct pl330_info *pi;
>> struct resource *res;
>> int irq;
>> @@ -3015,6 +3022,13 @@ static int __devexit pl330_remove(struct amba_device 
>> *adev)
>> pl330_free_chan_resources(>chan);
>> }
>>
>> +   while (!list_empty(>desc_pool)) {
>> +   desc = list_entry(pdmac->desc_pool.next,
>> +   struct dma_pl330_desc, node);
>> +   list_del(>node);
>> +   kfree(desc);
>> +   }
>> +
>> pi = >pif;
>>
>> pl330_del(pi);
>> --
>> 1.7.9.5
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] DMA: PL330: Free memory allocated for peripheral channels

[Inderpal Singh]

On 25 September 2012 18:17, Jassi Brar  wrote:
> On Tue, Sep 25, 2012 at 2:27 PM, Inderpal Singh
>  wrote:
>> The allocated memory for peripheral channels is not being freed upon
>> failure in probe and in module's remove funtion. It will lead to memory
>> leakage. Hence free the allocated memory.
>>
>> Signed-off-by: Inderpal Singh 
>> ---
>>  drivers/dma/pl330.c |5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
>> index 2ebd4cd..10c6b6a 100644
>> --- a/drivers/dma/pl330.c
>> +++ b/drivers/dma/pl330.c
>> @@ -2962,7 +2962,7 @@ pl330_probe(struct amba_device *adev, const struct 
>> amba_id *id)
>> ret = dma_async_device_register(pd);
>> if (ret) {
>> dev_err(>dev, "unable to register DMAC\n");
>> -   goto probe_err4;
>> +   goto probe_err5;
>> }
>>
> Sorry this patch seems malformed. Against which tree did you prepare it ?
>
This patch depends on "61c6e7531d3b66b3 DMA: PL330: Check the
pointer returned by kzalloc" which is on vinod's slave-dma's "fixes"
branch. So I merged slave-dma's "next" and "fixes" branches. Now after
merging, build error occurs due to some conflict so I had to apply the
patch sent by Sachin at [1]

Same had been mentioned in the cover letter.

[1] http://permalink.gmane.org/gmane.linux.kernel.next/24274

Thanks,
Inder

> -jassi
>
>
>
>> dev_info(>dev,
>> @@ -2975,6 +2975,8 @@ pl330_probe(struct amba_device *adev, const struct 
>> amba_id *id)
>>
>> return 0;
>>
>> +probe_err5:
>> +   kfree(pdmac->peripherals);
>>  probe_err4:
>> pl330_del(pi);
>>  probe_err3:
>> @@ -3025,6 +3027,7 @@ static int __devexit pl330_remove(struct amba_device 
>> *adev)
>> res = >res;
>> release_mem_region(res->start, resource_size(res));
>>
>> +   kfree(pdmac->peripherals);
>> kfree(pdmac);
>>
>> return 0;
>> --
>> 1.7.9.5
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

Since peripheral channel resources are not being allocated at probe,
no need to flush the channels and free the resources in remove function.

Signed-off-by: Inderpal Singh 
---
 drivers/dma/pl330.c |8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 04d83e6..6f06080 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -3012,16 +3012,10 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
 
/* Idle the DMAC */
list_for_each_entry_safe(pch, _p, >ddma.channels,
-   chan.device_node) {
-
+   chan.device_node)
/* Remove the channel */
list_del(>chan.device_node);
 
-   /* Flush the channel */
-   pl330_control(>chan, DMA_TERMINATE_ALL, 0);
-   pl330_free_chan_resources(>chan);
-   }
-
while (!list_empty(>desc_pool)) {
desc = list_entry(pdmac->desc_pool.next,
struct dma_pl330_desc, node);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/3] DMA: PL330: Fix mem leaks and balance probe/remove

[Inderpal Singh]

The first 2 patches of this series fix memory leaks because the memory
allocated for peripheral channels and DMA descriptors were not getting
freed.

The third patch balances the module's remove function.

This patchset is based on slave-dma tree's "next" branch merged with
"fixes" branch and applied patch at [1] to fix the build error.

[1] http://permalink.gmane.org/gmane.linux.kernel.next/24274

Inderpal Singh (3):
  DMA: PL330: Free memory allocated for peripheral channels
  DMA: PL330: Change allocation method to properly free DMA descriptors
  DMA: PL330: Balance module remove function with probe

 drivers/dma/pl330.c |   37 -
 1 file changed, 24 insertions(+), 13 deletions(-)

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/3] DMA: PL330: Change allocation method to properly free DMA descriptors

[Inderpal Singh]

In probe, memory for multiple DMA descriptors were being allocated at once
and then it was being split and added into DMA pool one by one. The address
of this memory allocation is not being saved anywhere. To free this memory,
the address is required. Initially the first node of the pool will be
pointed by this address but as we use this pool the descs will shuffle and
hence we will lose the track of the address.

This patch does following:

1. Allocates DMA descs one by one and then adds them to pool so that all
   descs can be fetched and memory freed one by one. This way runtime
   added descs can also be freed.
2. Free DMA descs in case of error in probe and in module's remove function

Signed-off-by: Inderpal Singh 
---
 drivers/dma/pl330.c |   28 +---
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 10c6b6a..04d83e6 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2541,20 +2541,19 @@ static int add_desc(struct dma_pl330_dmac *pdmac, gfp_t 
flg, int count)
if (!pdmac)
return 0;
 
-   desc = kmalloc(count * sizeof(*desc), flg);
-   if (!desc)
-   return 0;
-
spin_lock_irqsave(>pool_lock, flags);
 
for (i = 0; i < count; i++) {
-   _init_desc([i]);
-   list_add_tail([i].node, >desc_pool);
+   desc = kmalloc(sizeof(*desc), flg);
+   if (!desc)
+   break;
+   _init_desc(desc);
+   list_add_tail(>node, >desc_pool);
}
 
spin_unlock_irqrestore(>pool_lock, flags);
 
-   return count;
+   return i;
 }
 
 static struct dma_pl330_desc *
@@ -2857,6 +2856,7 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
struct dma_pl330_platdata *pdat;
struct dma_pl330_dmac *pdmac;
struct dma_pl330_chan *pch;
+   struct dma_pl330_desc *desc;
struct pl330_info *pi;
struct dma_device *pd;
struct resource *res;
@@ -2978,6 +2978,12 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
 probe_err5:
kfree(pdmac->peripherals);
 probe_err4:
+   while (!list_empty(>desc_pool)) {
+   desc = list_entry(pdmac->desc_pool.next,
+   struct dma_pl330_desc, node);
+   list_del(>node);
+   kfree(desc);
+   }
pl330_del(pi);
 probe_err3:
free_irq(irq, pi);
@@ -2994,6 +3000,7 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
 {
struct dma_pl330_dmac *pdmac = amba_get_drvdata(adev);
struct dma_pl330_chan *pch, *_p;
+   struct dma_pl330_desc *desc;
struct pl330_info *pi;
struct resource *res;
int irq;
@@ -3015,6 +3022,13 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
pl330_free_chan_resources(>chan);
}
 
+   while (!list_empty(>desc_pool)) {
+   desc = list_entry(pdmac->desc_pool.next,
+   struct dma_pl330_desc, node);
+   list_del(>node);
+   kfree(desc);
+   }
+
pi = >pif;
 
pl330_del(pi);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/3] DMA: PL330: Free memory allocated for peripheral channels

[Inderpal Singh]

The allocated memory for peripheral channels is not being freed upon
failure in probe and in module's remove funtion. It will lead to memory
leakage. Hence free the allocated memory.

Signed-off-by: Inderpal Singh 
---
 drivers/dma/pl330.c |5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 2ebd4cd..10c6b6a 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2962,7 +2962,7 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
ret = dma_async_device_register(pd);
if (ret) {
dev_err(>dev, "unable to register DMAC\n");
-   goto probe_err4;
+   goto probe_err5;
}
 
dev_info(>dev,
@@ -2975,6 +2975,8 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
 
return 0;
 
+probe_err5:
+   kfree(pdmac->peripherals);
 probe_err4:
pl330_del(pi);
 probe_err3:
@@ -3025,6 +3027,7 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
res = >res;
release_mem_region(res->start, resource_size(res));
 
+   kfree(pdmac->peripherals);
kfree(pdmac);
 
return 0;
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/3] DMA: PL330: Free memory allocated for peripheral channels

[Inderpal Singh]

The allocated memory for peripheral channels is not being freed upon
failure in probe and in module's remove funtion. It will lead to memory
leakage. Hence free the allocated memory.

Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
---
 drivers/dma/pl330.c |5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 2ebd4cd..10c6b6a 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2962,7 +2962,7 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
ret = dma_async_device_register(pd);
if (ret) {
dev_err(adev-dev, unable to register DMAC\n);
-   goto probe_err4;
+   goto probe_err5;
}
 
dev_info(adev-dev,
@@ -2975,6 +2975,8 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
 
return 0;
 
+probe_err5:
+   kfree(pdmac-peripherals);
 probe_err4:
pl330_del(pi);
 probe_err3:
@@ -3025,6 +3027,7 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
res = adev-res;
release_mem_region(res-start, resource_size(res));
 
+   kfree(pdmac-peripherals);
kfree(pdmac);
 
return 0;
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/3] DMA: PL330: Change allocation method to properly free DMA descriptors

[Inderpal Singh]

In probe, memory for multiple DMA descriptors were being allocated at once
and then it was being split and added into DMA pool one by one. The address
of this memory allocation is not being saved anywhere. To free this memory,
the address is required. Initially the first node of the pool will be
pointed by this address but as we use this pool the descs will shuffle and
hence we will lose the track of the address.

This patch does following:

1. Allocates DMA descs one by one and then adds them to pool so that all
   descs can be fetched and memory freed one by one. This way runtime
   added descs can also be freed.
2. Free DMA descs in case of error in probe and in module's remove function

Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
---
 drivers/dma/pl330.c |   28 +---
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 10c6b6a..04d83e6 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2541,20 +2541,19 @@ static int add_desc(struct dma_pl330_dmac *pdmac, gfp_t 
flg, int count)
if (!pdmac)
return 0;
 
-   desc = kmalloc(count * sizeof(*desc), flg);
-   if (!desc)
-   return 0;
-
spin_lock_irqsave(pdmac-pool_lock, flags);
 
for (i = 0; i  count; i++) {
-   _init_desc(desc[i]);
-   list_add_tail(desc[i].node, pdmac-desc_pool);
+   desc = kmalloc(sizeof(*desc), flg);
+   if (!desc)
+   break;
+   _init_desc(desc);
+   list_add_tail(desc-node, pdmac-desc_pool);
}
 
spin_unlock_irqrestore(pdmac-pool_lock, flags);
 
-   return count;
+   return i;
 }
 
 static struct dma_pl330_desc *
@@ -2857,6 +2856,7 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
struct dma_pl330_platdata *pdat;
struct dma_pl330_dmac *pdmac;
struct dma_pl330_chan *pch;
+   struct dma_pl330_desc *desc;
struct pl330_info *pi;
struct dma_device *pd;
struct resource *res;
@@ -2978,6 +2978,12 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
 probe_err5:
kfree(pdmac-peripherals);
 probe_err4:
+   while (!list_empty(pdmac-desc_pool)) {
+   desc = list_entry(pdmac-desc_pool.next,
+   struct dma_pl330_desc, node);
+   list_del(desc-node);
+   kfree(desc);
+   }
pl330_del(pi);
 probe_err3:
free_irq(irq, pi);
@@ -2994,6 +3000,7 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
 {
struct dma_pl330_dmac *pdmac = amba_get_drvdata(adev);
struct dma_pl330_chan *pch, *_p;
+   struct dma_pl330_desc *desc;
struct pl330_info *pi;
struct resource *res;
int irq;
@@ -3015,6 +3022,13 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
pl330_free_chan_resources(pch-chan);
}
 
+   while (!list_empty(pdmac-desc_pool)) {
+   desc = list_entry(pdmac-desc_pool.next,
+   struct dma_pl330_desc, node);
+   list_del(desc-node);
+   kfree(desc);
+   }
+
pi = pdmac-pif;
 
pl330_del(pi);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/3] DMA: PL330: Fix mem leaks and balance probe/remove

[Inderpal Singh]

The first 2 patches of this series fix memory leaks because the memory
allocated for peripheral channels and DMA descriptors were not getting
freed.

The third patch balances the module's remove function.

This patchset is based on slave-dma tree's next branch merged with
fixes branch and applied patch at [1] to fix the build error.

[1] http://permalink.gmane.org/gmane.linux.kernel.next/24274

Inderpal Singh (3):
  DMA: PL330: Free memory allocated for peripheral channels
  DMA: PL330: Change allocation method to properly free DMA descriptors
  DMA: PL330: Balance module remove function with probe

 drivers/dma/pl330.c |   37 -
 1 file changed, 24 insertions(+), 13 deletions(-)

-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 3/3] DMA: PL330: Balance module remove function with probe

[Inderpal Singh]

Since peripheral channel resources are not being allocated at probe,
no need to flush the channels and free the resources in remove function.

Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
---
 drivers/dma/pl330.c |8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 04d83e6..6f06080 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -3012,16 +3012,10 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
 
/* Idle the DMAC */
list_for_each_entry_safe(pch, _p, pdmac-ddma.channels,
-   chan.device_node) {
-
+   chan.device_node)
/* Remove the channel */
list_del(pch-chan.device_node);
 
-   /* Flush the channel */
-   pl330_control(pch-chan, DMA_TERMINATE_ALL, 0);
-   pl330_free_chan_resources(pch-chan);
-   }
-
while (!list_empty(pdmac-desc_pool)) {
desc = list_entry(pdmac-desc_pool.next,
struct dma_pl330_desc, node);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] DMA: PL330: Free memory allocated for peripheral channels

[Inderpal Singh]

On 25 September 2012 18:17, Jassi Brar jassisinghb...@gmail.com wrote:
 On Tue, Sep 25, 2012 at 2:27 PM, Inderpal Singh
 inderpal.si...@linaro.org wrote:
 The allocated memory for peripheral channels is not being freed upon
 failure in probe and in module's remove funtion. It will lead to memory
 leakage. Hence free the allocated memory.

 Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
 ---
  drivers/dma/pl330.c |5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

 diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
 index 2ebd4cd..10c6b6a 100644
 --- a/drivers/dma/pl330.c
 +++ b/drivers/dma/pl330.c
 @@ -2962,7 +2962,7 @@ pl330_probe(struct amba_device *adev, const struct 
 amba_id *id)
 ret = dma_async_device_register(pd);
 if (ret) {
 dev_err(adev-dev, unable to register DMAC\n);
 -   goto probe_err4;
 +   goto probe_err5;
 }

 Sorry this patch seems malformed. Against which tree did you prepare it ?

This patch depends on 61c6e7531d3b66b3 DMA: PL330: Check the
pointer returned by kzalloc which is on vinod's slave-dma's fixes
branch. So I merged slave-dma's next and fixes branches. Now after
merging, build error occurs due to some conflict so I had to apply the
patch sent by Sachin at [1]

Same had been mentioned in the cover letter.

[1] http://permalink.gmane.org/gmane.linux.kernel.next/24274

Thanks,
Inder

 -jassi



 dev_info(adev-dev,
 @@ -2975,6 +2975,8 @@ pl330_probe(struct amba_device *adev, const struct 
 amba_id *id)

 return 0;

 +probe_err5:
 +   kfree(pdmac-peripherals);
  probe_err4:
 pl330_del(pi);
  probe_err3:
 @@ -3025,6 +3027,7 @@ static int __devexit pl330_remove(struct amba_device 
 *adev)
 res = adev-res;
 release_mem_region(res-start, resource_size(res));

 +   kfree(pdmac-peripherals);
 kfree(pdmac);

 return 0;
 --
 1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] DMA: PL330: Change allocation method to properly free DMA descriptors

[Inderpal Singh]

On 25 September 2012 18:39, Jassi Brar jassisinghb...@gmail.com wrote:
 On Tue, Sep 25, 2012 at 2:27 PM, Inderpal Singh
 inderpal.si...@linaro.org wrote:
 In probe, memory for multiple DMA descriptors were being allocated at once
 and then it was being split and added into DMA pool one by one. The address
 of this memory allocation is not being saved anywhere. To free this memory,
 the address is required. Initially the first node of the pool will be
 pointed by this address but as we use this pool the descs will shuffle and
 hence we will lose the track of the address.

 This patch does following:

 1. Allocates DMA descs one by one and then adds them to pool so that all
descs can be fetched and memory freed one by one. This way runtime
added descs can also be freed.
 2. Free DMA descs in case of error in probe and in module's remove function

 Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
 ---
  drivers/dma/pl330.c |   28 +---
  1 file changed, 21 insertions(+), 7 deletions(-)

 diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
 index 10c6b6a..04d83e6 100644
 --- a/drivers/dma/pl330.c
 +++ b/drivers/dma/pl330.c
 @@ -2541,20 +2541,19 @@ static int add_desc(struct dma_pl330_dmac *pdmac, 
 gfp_t flg, int count)
 if (!pdmac)
 return 0;

 -   desc = kmalloc(count * sizeof(*desc), flg);
 -   if (!desc)
 -   return 0;
 -
 spin_lock_irqsave(pdmac-pool_lock, flags);

 for (i = 0; i  count; i++) {
 -   _init_desc(desc[i]);
 -   list_add_tail(desc[i].node, pdmac-desc_pool);
 +   desc = kmalloc(sizeof(*desc), flg);
 +   if (!desc)
 +   break;
 +   _init_desc(desc);
 +   list_add_tail(desc-node, pdmac-desc_pool);
 }

 spin_unlock_irqrestore(pdmac-pool_lock, flags);

 -   return count;
 +   return i;
  }

 OK, but the kmalloc shouldn't be done with irqs disabled. Please
 protect only the list_add_tail()

Yes, I missed it. I will update and send it again.

Thanks,
Inder

 thanks.

  static struct dma_pl330_desc *
 @@ -2857,6 +2856,7 @@ pl330_probe(struct amba_device *adev, const struct 
 amba_id *id)
 struct dma_pl330_platdata *pdat;
 struct dma_pl330_dmac *pdmac;
 struct dma_pl330_chan *pch;
 +   struct dma_pl330_desc *desc;
 struct pl330_info *pi;
 struct dma_device *pd;
 struct resource *res;
 @@ -2978,6 +2978,12 @@ pl330_probe(struct amba_device *adev, const struct 
 amba_id *id)
  probe_err5:
 kfree(pdmac-peripherals);
  probe_err4:
 +   while (!list_empty(pdmac-desc_pool)) {
 +   desc = list_entry(pdmac-desc_pool.next,
 +   struct dma_pl330_desc, node);
 +   list_del(desc-node);
 +   kfree(desc);
 +   }
 pl330_del(pi);
  probe_err3:
 free_irq(irq, pi);
 @@ -2994,6 +3000,7 @@ static int __devexit pl330_remove(struct amba_device 
 *adev)
  {
 struct dma_pl330_dmac *pdmac = amba_get_drvdata(adev);
 struct dma_pl330_chan *pch, *_p;
 +   struct dma_pl330_desc *desc;
 struct pl330_info *pi;
 struct resource *res;
 int irq;
 @@ -3015,6 +3022,13 @@ static int __devexit pl330_remove(struct amba_device 
 *adev)
 pl330_free_chan_resources(pch-chan);
 }

 +   while (!list_empty(pdmac-desc_pool)) {
 +   desc = list_entry(pdmac-desc_pool.next,
 +   struct dma_pl330_desc, node);
 +   list_del(desc-node);
 +   kfree(desc);
 +   }
 +
 pi = pdmac-pif;

 pl330_del(pi);
 --
 1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] DMA: PL330: return ENOMEM instead of 0 from pl330_alloc_chan_resources

[Inderpal Singh]

Since 0 is not considered as error at dmaengine level, return ENOMEM
from pl330_alloc_chan_resources in case of failure.

Signed-off-by: Inderpal Singh 
---
 drivers/dma/pl330.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index e4feba6..14d881c 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2393,7 +2393,7 @@ static int pl330_alloc_chan_resources(struct dma_chan 
*chan)
pch->pl330_chid = pl330_request_channel(>pif);
if (!pch->pl330_chid) {
spin_unlock_irqrestore(>lock, flags);
-   return 0;
+   return -ENOMEM;
}
 
tasklet_init(>task, pl330_tasklet, (unsigned long) pch);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] DMA: PL330: return ENOMEM instead of 0 from pl330_alloc_chan_resources

[Inderpal Singh]

Since 0 is not considered as error at dmaengine level, return ENOMEM
from pl330_alloc_chan_resources in case of failure.

Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
---
 drivers/dma/pl330.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index e4feba6..14d881c 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2393,7 +2393,7 @@ static int pl330_alloc_chan_resources(struct dma_chan 
*chan)
pch-pl330_chid = pl330_request_channel(pdmac-pif);
if (!pch-pl330_chid) {
spin_unlock_irqrestore(pch-lock, flags);
-   return 0;
+   return -ENOMEM;
}
 
tasklet_init(pch-task, pl330_tasklet, (unsigned long) pch);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] DMA: PL330: Remove redundant runtime_suspend/resume functions

[Inderpal Singh]

The driver's  runtime_suspend/resume functions just disable/enable
the clock which is already being managed at AMBA bus level
runtime_suspend/resume functions.

Hence, remove the driver's runtime_suspend/resume functions.

Signed-off-by: Inderpal Singh 
---
 drivers/dma/pl330.c |   61 +--
 1 file changed, 5 insertions(+), 56 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index f70e783..d9e1433 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -23,7 +23,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 
@@ -586,8 +585,6 @@ struct dma_pl330_dmac {
 
/* Peripheral channels connected to this DMAC */
struct dma_pl330_chan *peripherals; /* keep at end */
-
-   struct clk *clk;
 };
 
 struct dma_pl330_desc {
@@ -2887,24 +2884,17 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
goto probe_err1;
}
 
-   pdmac->clk = clk_get(>dev, "dma");
-   if (IS_ERR(pdmac->clk)) {
-   dev_err(>dev, "Cannot get operation clock.\n");
-   ret = -EINVAL;
-   goto probe_err2;
-   }
-
amba_set_drvdata(adev, pdmac);
 
irq = adev->irq[0];
ret = request_irq(irq, pl330_irq_handler, 0,
dev_name(>dev), pi);
if (ret)
-   goto probe_err3;
+   goto probe_err2;
 
ret = pl330_add(pi);
if (ret)
-   goto probe_err4;
+   goto probe_err3;
 
INIT_LIST_HEAD(>desc_pool);
spin_lock_init(>pool_lock);
@@ -2964,7 +2954,7 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
ret = dma_async_device_register(pd);
if (ret) {
dev_err(>dev, "unable to register DMAC\n");
-   goto probe_err5;
+   goto probe_err4;
}
 
dev_info(>dev,
@@ -2977,12 +2967,10 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
 
return 0;
 
-probe_err5:
-   pl330_del(pi);
 probe_err4:
-   free_irq(irq, pi);
+   pl330_del(pi);
 probe_err3:
-   clk_put(pdmac->clk);
+   free_irq(irq, pi);
 probe_err2:
iounmap(pi->base);
 probe_err1:
@@ -3044,49 +3032,10 @@ static struct amba_id pl330_ids[] = {
 
 MODULE_DEVICE_TABLE(amba, pl330_ids);
 
-#ifdef CONFIG_PM_RUNTIME
-static int pl330_runtime_suspend(struct device *dev)
-{
-   struct dma_pl330_dmac *pdmac = dev_get_drvdata(dev);
-
-   if (!pdmac) {
-   dev_err(dev, "failed to get dmac\n");
-   return -ENODEV;
-   }
-
-   clk_disable(pdmac->clk);
-
-   return 0;
-}
-
-static int pl330_runtime_resume(struct device *dev)
-{
-   struct dma_pl330_dmac *pdmac = dev_get_drvdata(dev);
-
-   if (!pdmac) {
-   dev_err(dev, "failed to get dmac\n");
-   return -ENODEV;
-   }
-
-   clk_enable(pdmac->clk);
-
-   return 0;
-}
-#else
-#define pl330_runtime_suspend  NULL
-#define pl330_runtime_resume   NULL
-#endif /* CONFIG_PM_RUNTIME */
-
-static const struct dev_pm_ops pl330_pm_ops = {
-   .runtime_suspend = pl330_runtime_suspend,
-   .runtime_resume = pl330_runtime_resume,
-};
-
 static struct amba_driver pl330_driver = {
.drv = {
.owner = THIS_MODULE,
.name = "dma-pl330",
-   .pm = _pm_ops,
},
.id_table = pl330_ids,
.probe = pl330_probe,
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/2] DMA: PL330: Remove controller clock enable/disable

[Inderpal Singh]

The controller clock is being enabled/disabled in AMBA bus
infrastructre in probe/remove functions. Hence, its not required
at driver level probe/remove.

Signed-off-by: Inderpal Singh 
---
 drivers/dma/pl330.c |   12 
 1 file changed, 12 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index e4feba6..f70e783 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2896,11 +2896,6 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
 
amba_set_drvdata(adev, pdmac);
 
-#ifndef CONFIG_PM_RUNTIME
-   /* enable dma clk */
-   clk_enable(pdmac->clk);
-#endif
-
irq = adev->irq[0];
ret = request_irq(irq, pl330_irq_handler, 0,
dev_name(>dev), pi);
@@ -2987,9 +2982,6 @@ probe_err5:
 probe_err4:
free_irq(irq, pi);
 probe_err3:
-#ifndef CONFIG_PM_RUNTIME
-   clk_disable(pdmac->clk);
-#endif
clk_put(pdmac->clk);
 probe_err2:
iounmap(pi->base);
@@ -3037,10 +3029,6 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
res = >res;
release_mem_region(res->start, resource_size(res));
 
-#ifndef CONFIG_PM_RUNTIME
-   clk_disable(pdmac->clk);
-#endif
-
kfree(pdmac);
 
return 0;
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/2] DMA: PL330: Clock and runtime cleanup

[Inderpal Singh]

The controller clock is being managed at AMBA bus level probe/remove and
pm_runtime/suspend functions. The existing driver does the clock enable/disable
again in the same code paths, which unneccessarily increments the usage count of
the clock for the same device. 

The following patches remove the redundant clock enable/disable from the driver.

Inderpal Singh (2):
  DMA: PL330: Remove controller clock enable/disable
  DMA: PL330: Remove redundant runtime_suspend/resume functions

 drivers/dma/pl330.c |   73 ---
 1 file changed, 5 insertions(+), 68 deletions(-)

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/2] DMA: PL330: Clock and runtime cleanup

[Inderpal Singh]

The controller clock is being managed at AMBA bus level probe/remove and
pm_runtime/suspend functions. The existing driver does the clock enable/disable
again in the same code paths, which unneccessarily increments the usage count of
the clock for the same device. 

The following patches remove the redundant clock enable/disable from the driver.

Inderpal Singh (2):
  DMA: PL330: Remove controller clock enable/disable
  DMA: PL330: Remove redundant runtime_suspend/resume functions

 drivers/dma/pl330.c |   73 ---
 1 file changed, 5 insertions(+), 68 deletions(-)

-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/2] DMA: PL330: Remove controller clock enable/disable

[Inderpal Singh]

The controller clock is being enabled/disabled in AMBA bus
infrastructre in probe/remove functions. Hence, its not required
at driver level probe/remove.

Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
---
 drivers/dma/pl330.c |   12 
 1 file changed, 12 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index e4feba6..f70e783 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2896,11 +2896,6 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
 
amba_set_drvdata(adev, pdmac);
 
-#ifndef CONFIG_PM_RUNTIME
-   /* enable dma clk */
-   clk_enable(pdmac-clk);
-#endif
-
irq = adev-irq[0];
ret = request_irq(irq, pl330_irq_handler, 0,
dev_name(adev-dev), pi);
@@ -2987,9 +2982,6 @@ probe_err5:
 probe_err4:
free_irq(irq, pi);
 probe_err3:
-#ifndef CONFIG_PM_RUNTIME
-   clk_disable(pdmac-clk);
-#endif
clk_put(pdmac-clk);
 probe_err2:
iounmap(pi-base);
@@ -3037,10 +3029,6 @@ static int __devexit pl330_remove(struct amba_device 
*adev)
res = adev-res;
release_mem_region(res-start, resource_size(res));
 
-#ifndef CONFIG_PM_RUNTIME
-   clk_disable(pdmac-clk);
-#endif
-
kfree(pdmac);
 
return 0;
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/2] DMA: PL330: Remove redundant runtime_suspend/resume functions

[Inderpal Singh]

The driver's  runtime_suspend/resume functions just disable/enable
the clock which is already being managed at AMBA bus level
runtime_suspend/resume functions.

Hence, remove the driver's runtime_suspend/resume functions.

Signed-off-by: Inderpal Singh inderpal.si...@linaro.org
---
 drivers/dma/pl330.c |   61 +--
 1 file changed, 5 insertions(+), 56 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index f70e783..d9e1433 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -23,7 +23,6 @@
 #include linux/dmaengine.h
 #include linux/amba/bus.h
 #include linux/amba/pl330.h
-#include linux/pm_runtime.h
 #include linux/scatterlist.h
 #include linux/of.h
 
@@ -586,8 +585,6 @@ struct dma_pl330_dmac {
 
/* Peripheral channels connected to this DMAC */
struct dma_pl330_chan *peripherals; /* keep at end */
-
-   struct clk *clk;
 };
 
 struct dma_pl330_desc {
@@ -2887,24 +2884,17 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
goto probe_err1;
}
 
-   pdmac-clk = clk_get(adev-dev, dma);
-   if (IS_ERR(pdmac-clk)) {
-   dev_err(adev-dev, Cannot get operation clock.\n);
-   ret = -EINVAL;
-   goto probe_err2;
-   }
-
amba_set_drvdata(adev, pdmac);
 
irq = adev-irq[0];
ret = request_irq(irq, pl330_irq_handler, 0,
dev_name(adev-dev), pi);
if (ret)
-   goto probe_err3;
+   goto probe_err2;
 
ret = pl330_add(pi);
if (ret)
-   goto probe_err4;
+   goto probe_err3;
 
INIT_LIST_HEAD(pdmac-desc_pool);
spin_lock_init(pdmac-pool_lock);
@@ -2964,7 +2954,7 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
ret = dma_async_device_register(pd);
if (ret) {
dev_err(adev-dev, unable to register DMAC\n);
-   goto probe_err5;
+   goto probe_err4;
}
 
dev_info(adev-dev,
@@ -2977,12 +2967,10 @@ pl330_probe(struct amba_device *adev, const struct 
amba_id *id)
 
return 0;
 
-probe_err5:
-   pl330_del(pi);
 probe_err4:
-   free_irq(irq, pi);
+   pl330_del(pi);
 probe_err3:
-   clk_put(pdmac-clk);
+   free_irq(irq, pi);
 probe_err2:
iounmap(pi-base);
 probe_err1:
@@ -3044,49 +3032,10 @@ static struct amba_id pl330_ids[] = {
 
 MODULE_DEVICE_TABLE(amba, pl330_ids);
 
-#ifdef CONFIG_PM_RUNTIME
-static int pl330_runtime_suspend(struct device *dev)
-{
-   struct dma_pl330_dmac *pdmac = dev_get_drvdata(dev);
-
-   if (!pdmac) {
-   dev_err(dev, failed to get dmac\n);
-   return -ENODEV;
-   }
-
-   clk_disable(pdmac-clk);
-
-   return 0;
-}
-
-static int pl330_runtime_resume(struct device *dev)
-{
-   struct dma_pl330_dmac *pdmac = dev_get_drvdata(dev);
-
-   if (!pdmac) {
-   dev_err(dev, failed to get dmac\n);
-   return -ENODEV;
-   }
-
-   clk_enable(pdmac-clk);
-
-   return 0;
-}
-#else
-#define pl330_runtime_suspend  NULL
-#define pl330_runtime_resume   NULL
-#endif /* CONFIG_PM_RUNTIME */
-
-static const struct dev_pm_ops pl330_pm_ops = {
-   .runtime_suspend = pl330_runtime_suspend,
-   .runtime_resume = pl330_runtime_resume,
-};
-
 static struct amba_driver pl330_driver = {
.drv = {
.owner = THIS_MODULE,
.name = dma-pl330,
-   .pm = pl330_pm_ops,
},
.id_table = pl330_ids,
.probe = pl330_probe,
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/