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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Fri, Apr 3, 2020 at 11:19 AM Alex Elder <[email protected]> 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 <[email protected]>
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
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 <[email protected]>
> ---
> 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);
>
>
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 <[email protected]>
> ---
> 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;
> }
>