| 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 | expand |
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
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;
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); }
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(-)