* [PATCH 0/3] remoteproc: bug fixes
@ 2020-04-03 17:50 Alex Elder
2020-04-03 17:50 ` [PATCH 1/3] remoteproc: fix a bug in rproc_alloc() Alex Elder
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Alex Elder @ 2020-04-03 17:50 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Andy Gross
Cc: linux-remoteproc, linux-arm-msm, linux-kernel
This series fixes some bugs in remoteproc error paths. The last
patch is derived from something I posted before, but has been
updated to based on Linus' current master branch.
-Alex
Alex Elder (3):
remoteproc: fix a bug in rproc_alloc()
remoteproc: qcom_q6v5_mss: fix a bug in q6v5_probe()
remoteproc: qcom_q6v5_mss: fix q6v5_probe() error paths
drivers/remoteproc/qcom_q6v5_mss.c | 33 +++++++++++++++++-----------
drivers/remoteproc/remoteproc_core.c | 4 ++--
2 files changed, 22 insertions(+), 15 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] remoteproc: fix a bug in rproc_alloc() 2020-04-03 17:50 [PATCH 0/3] remoteproc: bug fixes Alex Elder @ 2020-04-03 17:50 ` Alex Elder 2020-04-08 22:16 ` Mathieu Poirier 2020-04-03 17:50 ` [PATCH 2/3] remoteproc: qcom_q6v5_mss: fix a bug in q6v5_probe() Alex Elder ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Alex Elder @ 2020-04-03 17:50 UTC (permalink / raw) To: Bjorn Andersson, Ohad Ben-Cohen, Andy Gross Cc: linux-remoteproc, linux-arm-msm, linux-kernel If ida_simple_get() returns an error when called in rproc_alloc(), put_device() is called to clean things up. By this time the rproc device type has been assigned, with rproc_type_release() as the release function. The first thing rproc_type_release() does is call: idr_destroy(&rproc->notifyids); But at the time the ida_simple_get() call is made, the notifyids field in the remoteproc structure has not been initialized. I'm not actually sure this case causes an observable problem, but it's incorrect. Fix this by initializing the notifyids field before calling ida_simple_get() in rproc_alloc(). Signed-off-by: Alex Elder <elder@linaro.org> --- drivers/remoteproc/remoteproc_core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index e12a54e67588..59b6eb22f01c 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -2054,6 +2054,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, rproc->dev.class = &rproc_class; rproc->dev.driver_data = rproc; + idr_init(&rproc->notifyids); + /* Assign a unique device index and name */ rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL); if (rproc->index < 0) { @@ -2078,8 +2080,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, mutex_init(&rproc->lock); - idr_init(&rproc->notifyids); - INIT_LIST_HEAD(&rproc->carveouts); INIT_LIST_HEAD(&rproc->mappings); INIT_LIST_HEAD(&rproc->traces); -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] remoteproc: fix a bug in rproc_alloc() 2020-04-03 17:50 ` [PATCH 1/3] remoteproc: fix a bug in rproc_alloc() Alex Elder @ 2020-04-08 22:16 ` Mathieu Poirier 2020-04-08 22:18 ` Alex Elder 0 siblings, 1 reply; 11+ messages in thread From: Mathieu Poirier @ 2020-04-08 22:16 UTC (permalink / raw) To: Alex Elder Cc: Bjorn Andersson, Ohad Ben-Cohen, Andy Gross, linux-remoteproc, linux-arm-msm, Linux Kernel Mailing List Hi Alex, On Fri, 3 Apr 2020 at 11:50, Alex Elder <elder@linaro.org> wrote: > > If ida_simple_get() returns an error when called in rproc_alloc(), > put_device() is called to clean things up. By this time the rproc > device type has been assigned, with rproc_type_release() as the > release function. > > The first thing rproc_type_release() does is call: > idr_destroy(&rproc->notifyids); > > But at the time the ida_simple_get() call is made, the notifyids > field in the remoteproc structure has not been initialized. > > I'm not actually sure this case causes an observable problem, but > it's incorrect. Fix this by initializing the notifyids field before > calling ida_simple_get() in rproc_alloc(). > Both Suman and I are meddling in function rproc_alloc() for our respective work [1][2]. I will add this patch to a set that refactors rproc_alloc() as soon as v5.7-rc1 comes out. That way we can all base our work on the same foundation and Bjorn doesn't have to fix 3 different merge conflicts. Thanks, Mathieu [1]. https://patchwork.kernel.org/patch/11456385/ [2]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=261069 > Signed-off-by: Alex Elder <elder@linaro.org> > --- > drivers/remoteproc/remoteproc_core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index e12a54e67588..59b6eb22f01c 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -2054,6 +2054,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > rproc->dev.class = &rproc_class; > rproc->dev.driver_data = rproc; > > + idr_init(&rproc->notifyids); > + > /* Assign a unique device index and name */ > rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL); > if (rproc->index < 0) { > @@ -2078,8 +2080,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > > mutex_init(&rproc->lock); > > - idr_init(&rproc->notifyids); > - > INIT_LIST_HEAD(&rproc->carveouts); > INIT_LIST_HEAD(&rproc->mappings); > INIT_LIST_HEAD(&rproc->traces); > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] remoteproc: fix a bug in rproc_alloc() 2020-04-08 22:16 ` Mathieu Poirier @ 2020-04-08 22:18 ` Alex Elder 0 siblings, 0 replies; 11+ messages in thread From: Alex Elder @ 2020-04-08 22:18 UTC (permalink / raw) To: Mathieu Poirier Cc: Bjorn Andersson, Ohad Ben-Cohen, Andy Gross, linux-remoteproc, linux-arm-msm, Linux Kernel Mailing List On 4/8/20 5:16 PM, Mathieu Poirier wrote: > Hi Alex, > > On Fri, 3 Apr 2020 at 11:50, Alex Elder <elder@linaro.org> wrote: >> >> If ida_simple_get() returns an error when called in rproc_alloc(), >> put_device() is called to clean things up. By this time the rproc >> device type has been assigned, with rproc_type_release() as the >> release function. >> >> The first thing rproc_type_release() does is call: >> idr_destroy(&rproc->notifyids); >> >> But at the time the ida_simple_get() call is made, the notifyids >> field in the remoteproc structure has not been initialized. >> >> I'm not actually sure this case causes an observable problem, but >> it's incorrect. Fix this by initializing the notifyids field before >> calling ida_simple_get() in rproc_alloc(). >> > > Both Suman and I are meddling in function rproc_alloc() for our > respective work [1][2]. I will add this patch to a set that refactors > rproc_alloc() as soon as v5.7-rc1 comes out. That way we can all base > our work on the same foundation and Bjorn doesn't have to fix 3 > different merge conflicts. > > Thanks, > Mathieu Fine with me. Thanks a lot. -Alex > [1]. https://patchwork.kernel.org/patch/11456385/ > [2]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=261069 > >> Signed-off-by: Alex Elder <elder@linaro.org> >> --- >> drivers/remoteproc/remoteproc_core.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >> index e12a54e67588..59b6eb22f01c 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -2054,6 +2054,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, >> rproc->dev.class = &rproc_class; >> rproc->dev.driver_data = rproc; >> >> + idr_init(&rproc->notifyids); >> + >> /* Assign a unique device index and name */ >> rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL); >> if (rproc->index < 0) { >> @@ -2078,8 +2080,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, >> >> mutex_init(&rproc->lock); >> >> - idr_init(&rproc->notifyids); >> - >> INIT_LIST_HEAD(&rproc->carveouts); >> INIT_LIST_HEAD(&rproc->mappings); >> INIT_LIST_HEAD(&rproc->traces); >> -- >> 2.20.1 >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] remoteproc: qcom_q6v5_mss: fix a bug in q6v5_probe() 2020-04-03 17:50 [PATCH 0/3] remoteproc: bug fixes Alex Elder 2020-04-03 17:50 ` [PATCH 1/3] remoteproc: fix a bug in rproc_alloc() Alex Elder @ 2020-04-03 17:50 ` Alex Elder 2020-04-17 16:26 ` Suman Anna 2020-04-03 17:50 ` [PATCH 3/3] remoteproc: qcom_q6v5_mss: fix q6v5_probe() error paths Alex Elder 2020-04-17 5:51 ` [PATCH 0/3] remoteproc: bug fixes John Stultz 3 siblings, 1 reply; 11+ messages in thread From: Alex Elder @ 2020-04-03 17:50 UTC (permalink / raw) To: Bjorn Andersson, Ohad Ben-Cohen, Andy Gross Cc: linux-remoteproc, linux-arm-msm, linux-kernel If looking up the DT "firmware-name" property fails in q6v6_probe(), the function returns without freeing the remoteproc structure that has been allocated. Fix this by jumping to the free_rproc label, which takes care of this. Signed-off-by: Alex Elder <elder@linaro.org> --- drivers/remoteproc/qcom_q6v5_mss.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c index ce49c3236ff7..60cdf699ea80 100644 --- a/drivers/remoteproc/qcom_q6v5_mss.c +++ b/drivers/remoteproc/qcom_q6v5_mss.c @@ -1675,7 +1675,7 @@ static int q6v5_probe(struct platform_device *pdev) ret = of_property_read_string_index(pdev->dev.of_node, "firmware-name", 1, &qproc->hexagon_mdt_image); if (ret < 0 && ret != -EINVAL) - return ret; + goto free_rproc; platform_set_drvdata(pdev, qproc); -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] remoteproc: qcom_q6v5_mss: fix a bug in q6v5_probe() @ 2020-04-17 16:26 ` Suman Anna 0 siblings, 0 replies; 11+ messages in thread From: Suman Anna @ 2020-04-17 16:26 UTC (permalink / raw) To: Alex Elder, Bjorn Andersson, Ohad Ben-Cohen, Andy Gross Cc: linux-remoteproc, linux-arm-msm, linux-kernel On 4/3/20 12:50 PM, Alex Elder wrote: > If looking up the DT "firmware-name" property fails in q6v6_probe(), > the function returns without freeing the remoteproc structure > that has been allocated. Fix this by jumping to the free_rproc > label, which takes care of this. > Please add the Fixes: line. > Signed-off-by: Alex Elder <elder@linaro.org> > --- > drivers/remoteproc/qcom_q6v5_mss.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c > index ce49c3236ff7..60cdf699ea80 100644 > --- a/drivers/remoteproc/qcom_q6v5_mss.c > +++ b/drivers/remoteproc/qcom_q6v5_mss.c > @@ -1675,7 +1675,7 @@ static int q6v5_probe(struct platform_device *pdev) > ret = of_property_read_string_index(pdev->dev.of_node, "firmware-name", > 1, &qproc->hexagon_mdt_image); > if (ret < 0 && ret != -EINVAL) > - return ret; > + goto free_rproc; > > platform_set_drvdata(pdev, qproc); > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] remoteproc: qcom_q6v5_mss: fix a bug in q6v5_probe() @ 2020-04-17 16:26 ` Suman Anna 0 siblings, 0 replies; 11+ messages in thread From: Suman Anna @ 2020-04-17 16:26 UTC (permalink / raw) To: Alex Elder, Bjorn Andersson, Ohad Ben-Cohen, Andy Gross Cc: linux-remoteproc, linux-arm-msm, linux-kernel On 4/3/20 12:50 PM, Alex Elder wrote: > If looking up the DT "firmware-name" property fails in q6v6_probe(), > the function returns without freeing the remoteproc structure > that has been allocated. Fix this by jumping to the free_rproc > label, which takes care of this. > Please add the Fixes: line. > Signed-off-by: Alex Elder <elder@linaro.org> > --- > drivers/remoteproc/qcom_q6v5_mss.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c > index ce49c3236ff7..60cdf699ea80 100644 > --- a/drivers/remoteproc/qcom_q6v5_mss.c > +++ b/drivers/remoteproc/qcom_q6v5_mss.c > @@ -1675,7 +1675,7 @@ static int q6v5_probe(struct platform_device *pdev) > ret = of_property_read_string_index(pdev->dev.of_node, "firmware-name", > 1, &qproc->hexagon_mdt_image); > if (ret < 0 && ret != -EINVAL) > - return ret; > + goto free_rproc; > > platform_set_drvdata(pdev, qproc); > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] remoteproc: qcom_q6v5_mss: fix q6v5_probe() error paths 2020-04-03 17:50 [PATCH 0/3] remoteproc: bug fixes Alex Elder 2020-04-03 17:50 ` [PATCH 1/3] remoteproc: fix a bug in rproc_alloc() Alex Elder 2020-04-03 17:50 ` [PATCH 2/3] remoteproc: qcom_q6v5_mss: fix a bug in q6v5_probe() Alex Elder @ 2020-04-03 17:50 ` Alex Elder 2020-04-17 16:28 ` Suman Anna 2020-04-17 5:51 ` [PATCH 0/3] remoteproc: bug fixes John Stultz 3 siblings, 1 reply; 11+ messages in thread From: Alex Elder @ 2020-04-03 17:50 UTC (permalink / raw) To: Bjorn Andersson, Ohad Ben-Cohen, Andy Gross Cc: linux-remoteproc, linux-arm-msm, linux-kernel If an error occurs in q6v5_probe() after the proxy power domains are attached, but before qcom_add_ipa_notify_subdev() is called, qcom_remove_ipa_notify_subdev() is called in the error path, which is a bug. Fix this by having that call be reached through a different label. Additionally, if qcom_add_sysmon_subdev() returns an error, the subdevs that had already been added will not be properly removed. Fix this by having the added subdevs (including the IPA notify one) be removed in this case. Finally, arrange for the sysmon subdev to be removed before the rest in the event rproc_add() returns an error. Have cleanup activity done in q6v5_remove() be done in the reverse order they are set up in q6v5_probe() (the same order they're done in the q6v5_probe() error path). Use a local variable for the remoteproc pointer, which is used repeatedly. Remove errant semicolons at the end of two function blocks. Signed-off-by: Alex Elder <elder@linaro.org> --- drivers/remoteproc/qcom_q6v5_mss.c | 31 ++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c index 60cdf699ea80..5475d4f808a8 100644 --- a/drivers/remoteproc/qcom_q6v5_mss.c +++ b/drivers/remoteproc/qcom_q6v5_mss.c @@ -367,7 +367,7 @@ static int q6v5_pds_enable(struct q6v5 *qproc, struct device **pds, } return ret; -}; +} static void q6v5_pds_disable(struct q6v5 *qproc, struct device **pds, size_t pd_count) @@ -1527,7 +1527,7 @@ static int q6v5_pds_attach(struct device *dev, struct device **devs, dev_pm_domain_detach(devs[i], false); return ret; -}; +} static void q6v5_pds_detach(struct q6v5 *qproc, struct device **pds, size_t pd_count) @@ -1766,17 +1766,23 @@ static int q6v5_probe(struct platform_device *pdev) qproc->sysmon = qcom_add_sysmon_subdev(rproc, "modem", 0x12); if (IS_ERR(qproc->sysmon)) { ret = PTR_ERR(qproc->sysmon); - goto detach_proxy_pds; + goto remove_subdevs; } ret = rproc_add(rproc); if (ret) - goto detach_proxy_pds; + goto remove_sysmon_subdev; return 0; +remove_sysmon_subdev: + qcom_remove_sysmon_subdev(qproc->sysmon); +remove_subdevs: + qcom_remove_ipa_notify_subdev(qproc->rproc, &qproc->ipa_notify_subdev); + qcom_remove_ssr_subdev(rproc, &qproc->ssr_subdev); + qcom_remove_smd_subdev(rproc, &qproc->smd_subdev); + qcom_remove_glink_subdev(rproc, &qproc->glink_subdev); detach_proxy_pds: - qcom_remove_ipa_notify_subdev(qproc->rproc, &qproc->ipa_notify_subdev); q6v5_pds_detach(qproc, qproc->proxy_pds, qproc->proxy_pd_count); detach_active_pds: q6v5_pds_detach(qproc, qproc->active_pds, qproc->active_pd_count); @@ -1789,19 +1795,20 @@ static int q6v5_probe(struct platform_device *pdev) static int q6v5_remove(struct platform_device *pdev) { struct q6v5 *qproc = platform_get_drvdata(pdev); + struct rproc *rproc = qproc->rproc; - rproc_del(qproc->rproc); + rproc_del(rproc); qcom_remove_sysmon_subdev(qproc->sysmon); - qcom_remove_ipa_notify_subdev(qproc->rproc, &qproc->ipa_notify_subdev); - qcom_remove_glink_subdev(qproc->rproc, &qproc->glink_subdev); - qcom_remove_smd_subdev(qproc->rproc, &qproc->smd_subdev); - qcom_remove_ssr_subdev(qproc->rproc, &qproc->ssr_subdev); + qcom_remove_ipa_notify_subdev(rproc, &qproc->ipa_notify_subdev); + qcom_remove_ssr_subdev(rproc, &qproc->ssr_subdev); + qcom_remove_smd_subdev(rproc, &qproc->smd_subdev); + qcom_remove_glink_subdev(rproc, &qproc->glink_subdev); - q6v5_pds_detach(qproc, qproc->active_pds, qproc->active_pd_count); q6v5_pds_detach(qproc, qproc->proxy_pds, qproc->proxy_pd_count); + q6v5_pds_detach(qproc, qproc->active_pds, qproc->active_pd_count); - rproc_free(qproc->rproc); + rproc_free(rproc); return 0; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] remoteproc: qcom_q6v5_mss: fix q6v5_probe() error paths @ 2020-04-17 16:28 ` Suman Anna 0 siblings, 0 replies; 11+ messages in thread From: Suman Anna @ 2020-04-17 16:28 UTC (permalink / raw) To: Alex Elder, Bjorn Andersson, Ohad Ben-Cohen, Andy Gross Cc: linux-remoteproc, linux-arm-msm, linux-kernel On 4/3/20 12:50 PM, Alex Elder wrote: > If an error occurs in q6v5_probe() after the proxy power domains > are attached, but before qcom_add_ipa_notify_subdev() is called, > qcom_remove_ipa_notify_subdev() is called in the error path, which > is a bug. Fix this by having that call be reached through a > different label. > > Additionally, if qcom_add_sysmon_subdev() returns an error, the > subdevs that had already been added will not be properly removed. > Fix this by having the added subdevs (including the IPA notify one) > be removed in this case. > > Finally, arrange for the sysmon subdev to be removed before the rest > in the event rproc_add() returns an error. > > Have cleanup activity done in q6v5_remove() be done in the reverse > order they are set up in q6v5_probe() (the same order they're done > in the q6v5_probe() error path). Use a local variable for the > remoteproc pointer, which is used repeatedly. > > Remove errant semicolons at the end of two function blocks. > Same as Patch 2, Fixes: line would be good. regards Suman > Signed-off-by: Alex Elder <elder@linaro.org> > --- > drivers/remoteproc/qcom_q6v5_mss.c | 31 ++++++++++++++++++------------ > 1 file changed, 19 insertions(+), 12 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c > index 60cdf699ea80..5475d4f808a8 100644 > --- a/drivers/remoteproc/qcom_q6v5_mss.c > +++ b/drivers/remoteproc/qcom_q6v5_mss.c > @@ -367,7 +367,7 @@ static int q6v5_pds_enable(struct q6v5 *qproc, struct device **pds, > } > > return ret; > -}; > +} > > static void q6v5_pds_disable(struct q6v5 *qproc, struct device **pds, > size_t pd_count) > @@ -1527,7 +1527,7 @@ static int q6v5_pds_attach(struct device *dev, struct device **devs, > dev_pm_domain_detach(devs[i], false); > > return ret; > -}; > +} > > static void q6v5_pds_detach(struct q6v5 *qproc, struct device **pds, > size_t pd_count) > @@ -1766,17 +1766,23 @@ static int q6v5_probe(struct platform_device *pdev) > qproc->sysmon = qcom_add_sysmon_subdev(rproc, "modem", 0x12); > if (IS_ERR(qproc->sysmon)) { > ret = PTR_ERR(qproc->sysmon); > - goto detach_proxy_pds; > + goto remove_subdevs; > } > > ret = rproc_add(rproc); > if (ret) > - goto detach_proxy_pds; > + goto remove_sysmon_subdev; > > return 0; > > +remove_sysmon_subdev: > + qcom_remove_sysmon_subdev(qproc->sysmon); > +remove_subdevs: > + qcom_remove_ipa_notify_subdev(qproc->rproc, &qproc->ipa_notify_subdev); > + qcom_remove_ssr_subdev(rproc, &qproc->ssr_subdev); > + qcom_remove_smd_subdev(rproc, &qproc->smd_subdev); > + qcom_remove_glink_subdev(rproc, &qproc->glink_subdev); > detach_proxy_pds: > - qcom_remove_ipa_notify_subdev(qproc->rproc, &qproc->ipa_notify_subdev); > q6v5_pds_detach(qproc, qproc->proxy_pds, qproc->proxy_pd_count); > detach_active_pds: > q6v5_pds_detach(qproc, qproc->active_pds, qproc->active_pd_count); > @@ -1789,19 +1795,20 @@ static int q6v5_probe(struct platform_device *pdev) > static int q6v5_remove(struct platform_device *pdev) > { > struct q6v5 *qproc = platform_get_drvdata(pdev); > + struct rproc *rproc = qproc->rproc; > > - rproc_del(qproc->rproc); > + rproc_del(rproc); > > qcom_remove_sysmon_subdev(qproc->sysmon); > - qcom_remove_ipa_notify_subdev(qproc->rproc, &qproc->ipa_notify_subdev); > - qcom_remove_glink_subdev(qproc->rproc, &qproc->glink_subdev); > - qcom_remove_smd_subdev(qproc->rproc, &qproc->smd_subdev); > - qcom_remove_ssr_subdev(qproc->rproc, &qproc->ssr_subdev); > + qcom_remove_ipa_notify_subdev(rproc, &qproc->ipa_notify_subdev); > + qcom_remove_ssr_subdev(rproc, &qproc->ssr_subdev); > + qcom_remove_smd_subdev(rproc, &qproc->smd_subdev); > + qcom_remove_glink_subdev(rproc, &qproc->glink_subdev); > > - q6v5_pds_detach(qproc, qproc->active_pds, qproc->active_pd_count); > q6v5_pds_detach(qproc, qproc->proxy_pds, qproc->proxy_pd_count); > + q6v5_pds_detach(qproc, qproc->active_pds, qproc->active_pd_count); > > - rproc_free(qproc->rproc); > + rproc_free(rproc); > > return 0; > } > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] remoteproc: qcom_q6v5_mss: fix q6v5_probe() error paths @ 2020-04-17 16:28 ` Suman Anna 0 siblings, 0 replies; 11+ messages in thread From: Suman Anna @ 2020-04-17 16:28 UTC (permalink / raw) To: Alex Elder, Bjorn Andersson, Ohad Ben-Cohen, Andy Gross Cc: linux-remoteproc, linux-arm-msm, linux-kernel On 4/3/20 12:50 PM, Alex Elder wrote: > If an error occurs in q6v5_probe() after the proxy power domains > are attached, but before qcom_add_ipa_notify_subdev() is called, > qcom_remove_ipa_notify_subdev() is called in the error path, which > is a bug. Fix this by having that call be reached through a > different label. > > Additionally, if qcom_add_sysmon_subdev() returns an error, the > subdevs that had already been added will not be properly removed. > Fix this by having the added subdevs (including the IPA notify one) > be removed in this case. > > Finally, arrange for the sysmon subdev to be removed before the rest > in the event rproc_add() returns an error. > > Have cleanup activity done in q6v5_remove() be done in the reverse > order they are set up in q6v5_probe() (the same order they're done > in the q6v5_probe() error path). Use a local variable for the > remoteproc pointer, which is used repeatedly. > > Remove errant semicolons at the end of two function blocks. > Same as Patch 2, Fixes: line would be good. regards Suman > Signed-off-by: Alex Elder <elder@linaro.org> > --- > drivers/remoteproc/qcom_q6v5_mss.c | 31 ++++++++++++++++++------------ > 1 file changed, 19 insertions(+), 12 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c > index 60cdf699ea80..5475d4f808a8 100644 > --- a/drivers/remoteproc/qcom_q6v5_mss.c > +++ b/drivers/remoteproc/qcom_q6v5_mss.c > @@ -367,7 +367,7 @@ static int q6v5_pds_enable(struct q6v5 *qproc, struct device **pds, > } > > return ret; > -}; > +} > > static void q6v5_pds_disable(struct q6v5 *qproc, struct device **pds, > size_t pd_count) > @@ -1527,7 +1527,7 @@ static int q6v5_pds_attach(struct device *dev, struct device **devs, > dev_pm_domain_detach(devs[i], false); > > return ret; > -}; > +} > > static void q6v5_pds_detach(struct q6v5 *qproc, struct device **pds, > size_t pd_count) > @@ -1766,17 +1766,23 @@ static int q6v5_probe(struct platform_device *pdev) > qproc->sysmon = qcom_add_sysmon_subdev(rproc, "modem", 0x12); > if (IS_ERR(qproc->sysmon)) { > ret = PTR_ERR(qproc->sysmon); > - goto detach_proxy_pds; > + goto remove_subdevs; > } > > ret = rproc_add(rproc); > if (ret) > - goto detach_proxy_pds; > + goto remove_sysmon_subdev; > > return 0; > > +remove_sysmon_subdev: > + qcom_remove_sysmon_subdev(qproc->sysmon); > +remove_subdevs: > + qcom_remove_ipa_notify_subdev(qproc->rproc, &qproc->ipa_notify_subdev); > + qcom_remove_ssr_subdev(rproc, &qproc->ssr_subdev); > + qcom_remove_smd_subdev(rproc, &qproc->smd_subdev); > + qcom_remove_glink_subdev(rproc, &qproc->glink_subdev); > detach_proxy_pds: > - qcom_remove_ipa_notify_subdev(qproc->rproc, &qproc->ipa_notify_subdev); > q6v5_pds_detach(qproc, qproc->proxy_pds, qproc->proxy_pd_count); > detach_active_pds: > q6v5_pds_detach(qproc, qproc->active_pds, qproc->active_pd_count); > @@ -1789,19 +1795,20 @@ static int q6v5_probe(struct platform_device *pdev) > static int q6v5_remove(struct platform_device *pdev) > { > struct q6v5 *qproc = platform_get_drvdata(pdev); > + struct rproc *rproc = qproc->rproc; > > - rproc_del(qproc->rproc); > + rproc_del(rproc); > > qcom_remove_sysmon_subdev(qproc->sysmon); > - qcom_remove_ipa_notify_subdev(qproc->rproc, &qproc->ipa_notify_subdev); > - qcom_remove_glink_subdev(qproc->rproc, &qproc->glink_subdev); > - qcom_remove_smd_subdev(qproc->rproc, &qproc->smd_subdev); > - qcom_remove_ssr_subdev(qproc->rproc, &qproc->ssr_subdev); > + qcom_remove_ipa_notify_subdev(rproc, &qproc->ipa_notify_subdev); > + qcom_remove_ssr_subdev(rproc, &qproc->ssr_subdev); > + qcom_remove_smd_subdev(rproc, &qproc->smd_subdev); > + qcom_remove_glink_subdev(rproc, &qproc->glink_subdev); > > - q6v5_pds_detach(qproc, qproc->active_pds, qproc->active_pd_count); > q6v5_pds_detach(qproc, qproc->proxy_pds, qproc->proxy_pd_count); > + q6v5_pds_detach(qproc, qproc->active_pds, qproc->active_pd_count); > > - rproc_free(qproc->rproc); > + rproc_free(rproc); > > return 0; > } > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] remoteproc: bug fixes 2020-04-03 17:50 [PATCH 0/3] remoteproc: bug fixes Alex Elder ` (2 preceding siblings ...) 2020-04-03 17:50 ` [PATCH 3/3] remoteproc: qcom_q6v5_mss: fix q6v5_probe() error paths Alex Elder @ 2020-04-17 5:51 ` John Stultz 3 siblings, 0 replies; 11+ messages in thread From: John Stultz @ 2020-04-17 5:51 UTC (permalink / raw) To: Alex Elder Cc: Bjorn Andersson, Ohad Ben-Cohen, Andy Gross, linux-remoteproc, linux-arm-msm, Linux Kernel Mailing List On Fri, Apr 3, 2020 at 11:19 AM Alex Elder <elder@linaro.org> wrote: > > This series fixes some bugs in remoteproc error paths. The last > patch is derived from something I posted before, but has been > updated to based on Linus' current master branch. > Oof. Apparently I didn't hit reply all but only sent my tested-by to just Alex (now 10 days ago) Tested-by: John Stultz <john.stultz@linaro.org> This set resolves regressions we've seen with 5.7-rc1 on db845c, and it would be really nice to see them land in 5.7-rc2. thanks -john ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-04-17 16:28 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-03 17:50 [PATCH 0/3] remoteproc: bug fixes Alex Elder 2020-04-03 17:50 ` [PATCH 1/3] remoteproc: fix a bug in rproc_alloc() Alex Elder 2020-04-08 22:16 ` Mathieu Poirier 2020-04-08 22:18 ` Alex Elder 2020-04-03 17:50 ` [PATCH 2/3] remoteproc: qcom_q6v5_mss: fix a bug in q6v5_probe() Alex Elder 2020-04-17 16:26 ` Suman Anna 2020-04-17 16:26 ` Suman Anna 2020-04-03 17:50 ` [PATCH 3/3] remoteproc: qcom_q6v5_mss: fix q6v5_probe() error paths Alex Elder 2020-04-17 16:28 ` Suman Anna 2020-04-17 16:28 ` Suman Anna 2020-04-17 5:51 ` [PATCH 0/3] remoteproc: bug fixes John Stultz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.