[1/5] interconnect: qcom: sdm845: Walk the list safely on node removal

Message ID 20191128133435.25667-1-georgi.djakov@linaro.org
State New
Headers show
Series
  • [1/5] interconnect: qcom: sdm845: Walk the list safely on node removal
Related show

Commit Message

Georgi Djakov Nov. 28, 2019, 1:34 p.m.
As we will remove items off the list using list_del(), we need to use the
safe version of list_for_each_entry().

Fixes: b5d2f741077a ("interconnect: qcom: Add sdm845 interconnect provider driver")
Reported-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>

---
 drivers/interconnect/qcom/sdm845.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Georgi Djakov Nov. 28, 2019, 1:52 p.m. | #1
On 28.11.19 г. 15:43 ч., Dmitry Osipenko wrote:
> 28.11.2019 16:34, Georgi Djakov пишет:

>> There is a new helper function for removing all nodes. Let's use it instead

>> of duplicating the code.

>>

>> In addition to the above, instead of duplicating the code, simplify the

>> probe function error path by calling driver removal function directly.

>>

>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>

>> ---

>>   drivers/interconnect/qcom/msm8974.c | 40 ++++++++++-------------------

>>   drivers/interconnect/qcom/qcs404.c  | 31 ++++++++--------------

>>   drivers/interconnect/qcom/sdm845.c  | 29 +++++++--------------

>>   3 files changed, 33 insertions(+), 67 deletions(-)

>>

[..]>> +static int qnoc_remove(struct platform_device *pdev)
>> +{

>> +	struct qcom_icc_provider *qp = platform_get_drvdata(pdev);

>> +

>> +	icc_nodes_remove(&qp->provider);

>> +	return icc_provider_del(&qp->provider);

>> +}

>> +

>>   static int qnoc_probe(struct platform_device *pdev)

>>   {

>>   	const struct qcom_icc_desc *desc;

>> @@ -855,29 +863,10 @@ static int qnoc_probe(struct platform_device *pdev)

>>   

>>   	return ret;

>>   err:

>> -	list_for_each_entry(node, &provider->nodes, node_list) {

>> -		icc_node_del(node);

>> -		icc_node_destroy(node->id);

>> -	}

>> -

>> -	icc_provider_del(provider);

>> +	qnoc_remove(pdev);

> 

> This is wrong because platform_set_drvdata() is invoked at the end of

> qnoc_probe(), thus platform_get_drvdata() of qnoc_remove() returns NULL

> here.


True! Will fix it! Thank you!

BR,
Georgi
Bjorn Andersson Nov. 28, 2019, 6:20 p.m. | #2
On Thu 28 Nov 05:34 PST 2019, Georgi Djakov wrote:

> The removal of all nodes from a provider seem to be a common functionality

> for all existing users and it would make sense to factor out this into a

> a common helper function.

> 

> Suggested-by: Dmitry Osipenko <digetx@gmail.com>

> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>


Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>


> ---

>  drivers/interconnect/core.c           | 22 ++++++++++++++++++++++

>  include/linux/interconnect-provider.h |  6 ++++++

>  2 files changed, 28 insertions(+)

> 

> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c

> index 467c539310f3..0e4852feb395 100644

> --- a/drivers/interconnect/core.c

> +++ b/drivers/interconnect/core.c

> @@ -735,6 +735,28 @@ void icc_node_del(struct icc_node *node)

>  }

>  EXPORT_SYMBOL_GPL(icc_node_del);

>  

> +/**

> + * icc_nodes_remove() - remove all previously added nodes from provider

> + * @provider: the interconnect provider we are removing nodes from

> + *

> + * Return: 0 on success, or an error code otherwise

> + */

> +int icc_nodes_remove(struct icc_provider *provider)

> +{

> +	struct icc_node *n, *tmp;

> +

> +	if (WARN_ON(IS_ERR_OR_NULL(provider)))

> +		return -EINVAL;

> +

> +	list_for_each_entry_safe_reverse(n, tmp, &provider->nodes, node_list) {

> +		icc_node_del(n);

> +		icc_node_destroy(n->id);

> +	}

> +

> +	return 0;

> +}

> +EXPORT_SYMBOL_GPL(icc_nodes_remove);

> +

>  /**

>   * icc_provider_add() - add a new interconnect provider

>   * @provider: the interconnect provider that will be added into topology

> diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h

> index b16f9effa555..31440c921216 100644

> --- a/include/linux/interconnect-provider.h

> +++ b/include/linux/interconnect-provider.h

> @@ -98,6 +98,7 @@ int icc_link_create(struct icc_node *node, const int dst_id);

>  int icc_link_destroy(struct icc_node *src, struct icc_node *dst);

>  void icc_node_add(struct icc_node *node, struct icc_provider *provider);

>  void icc_node_del(struct icc_node *node);

> +int icc_nodes_remove(struct icc_provider *provider);

>  int icc_provider_add(struct icc_provider *provider);

>  int icc_provider_del(struct icc_provider *provider);

>  

> @@ -130,6 +131,11 @@ void icc_node_del(struct icc_node *node)

>  {

>  }

>  

> +static inline int icc_nodes_remove(struct icc_provider *provider)

> +{

> +	return -ENOTSUPP;

> +}

> +

>  static inline int icc_provider_add(struct icc_provider *provider)

>  {

>  	return -ENOTSUPP;

Patch

diff --git a/drivers/interconnect/qcom/sdm845.c b/drivers/interconnect/qcom/sdm845.c
index 502a6c22b41e..924c2d056d85 100644
--- a/drivers/interconnect/qcom/sdm845.c
+++ b/drivers/interconnect/qcom/sdm845.c
@@ -870,7 +870,7 @@  static int qnoc_remove(struct platform_device *pdev)
 	struct icc_provider *provider = &qp->provider;
 	struct icc_node *n;
 
-	list_for_each_entry(n, &provider->nodes, node_list) {
+	list_for_each_entry_safe(n, &provider->nodes, node_list) {
 		icc_node_del(n);
 		icc_node_destroy(n->id);
 	}