| Message ID | 20241105092055.255127-1-joe@pf.is.s.u-tokyo.ac.jp (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | remoteproc: qcom: pas: Remove subdevs on the error path of adsp_probe() | expand |
Hi Joe, On 11/5/2024 1:20 AM, Joe Hattori wrote: > Current implementation of adsp_probe() does not remove the subdevs of > struct qcom_adsp *adsp on the error path. Fix this bug by calling > qcom_remove_{ssr,sysmon,pdm,smd,glink}_subdev(), qcom_q6v5_deinit(), and > adsp_unassign_memory_region() appropriately. > > Fixes: 5b9f51b200dc ("remoteproc: qcom: enable in-kernel PD mapper") The ssr, sysmon, glink subdevs and qcom_q6v5_init() are the calls that leak resources. I think this issue existed before the in-kernel PD mapper patches so this Fixes tag isn't accurate. Would this apply to the other remoteproc files like qcom_q6v5_adsp.c? > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > --- > drivers/remoteproc/qcom_q6v5_pas.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c > index ef82835e98a4..27b23a000c7a 100644 > --- a/drivers/remoteproc/qcom_q6v5_pas.c > +++ b/drivers/remoteproc/qcom_q6v5_pas.c > @@ -759,16 +759,16 @@ static int adsp_probe(struct platform_device *pdev) > > ret = adsp_init_clock(adsp); > if (ret) > - goto free_rproc; > + goto unassign_mem; > > ret = adsp_init_regulator(adsp); > if (ret) > - goto free_rproc; > + goto unassign_mem; > > ret = adsp_pds_attach(&pdev->dev, adsp->proxy_pds, > desc->proxy_pd_names); > if (ret < 0) > - goto free_rproc; > + goto unassign_mem; > adsp->proxy_pd_count = ret; > > ret = qcom_q6v5_init(&adsp->q6v5, pdev, rproc, desc->crash_reason_smem, desc->load_state, > @@ -784,18 +784,28 @@ static int adsp_probe(struct platform_device *pdev) > desc->ssctl_id); > if (IS_ERR(adsp->sysmon)) { > ret = PTR_ERR(adsp->sysmon); > - goto detach_proxy_pds; > + goto deinit_remove_pdm_smd_glink; > } > > qcom_add_ssr_subdev(rproc, &adsp->ssr_subdev, desc->ssr_name); > ret = rproc_add(rproc); > if (ret) > - goto detach_proxy_pds; > + goto remove_ssr_sysmon; > > return 0; > > +remove_ssr_sysmon: > + qcom_remove_ssr_subdev(rproc, &adsp->ssr_subdev); > + qcom_remove_sysmon_subdev(adsp->sysmon); > +deinit_remove_pdm_smd_glink: > + qcom_remove_pdm_subdev(rproc, &adsp->pdm_subdev); > + qcom_remove_smd_subdev(rproc, &adsp->smd_subdev); > + qcom_remove_glink_subdev(rproc, &adsp->glink_subdev); > + qcom_q6v5_deinit(&adsp->q6v5); > detach_proxy_pds: > adsp_pds_detach(adsp, adsp->proxy_pds, adsp->proxy_pd_count); > +unassign_mem: > + adsp_unassign_memory_region(adsp); > free_rproc: > device_init_wakeup(adsp->dev, false); >
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c index ef82835e98a4..27b23a000c7a 100644 --- a/drivers/remoteproc/qcom_q6v5_pas.c +++ b/drivers/remoteproc/qcom_q6v5_pas.c @@ -759,16 +759,16 @@ static int adsp_probe(struct platform_device *pdev) ret = adsp_init_clock(adsp); if (ret) - goto free_rproc; + goto unassign_mem; ret = adsp_init_regulator(adsp); if (ret) - goto free_rproc; + goto unassign_mem; ret = adsp_pds_attach(&pdev->dev, adsp->proxy_pds, desc->proxy_pd_names); if (ret < 0) - goto free_rproc; + goto unassign_mem; adsp->proxy_pd_count = ret; ret = qcom_q6v5_init(&adsp->q6v5, pdev, rproc, desc->crash_reason_smem, desc->load_state, @@ -784,18 +784,28 @@ static int adsp_probe(struct platform_device *pdev) desc->ssctl_id); if (IS_ERR(adsp->sysmon)) { ret = PTR_ERR(adsp->sysmon); - goto detach_proxy_pds; + goto deinit_remove_pdm_smd_glink; } qcom_add_ssr_subdev(rproc, &adsp->ssr_subdev, desc->ssr_name); ret = rproc_add(rproc); if (ret) - goto detach_proxy_pds; + goto remove_ssr_sysmon; return 0; +remove_ssr_sysmon: + qcom_remove_ssr_subdev(rproc, &adsp->ssr_subdev); + qcom_remove_sysmon_subdev(adsp->sysmon); +deinit_remove_pdm_smd_glink: + qcom_remove_pdm_subdev(rproc, &adsp->pdm_subdev); + qcom_remove_smd_subdev(rproc, &adsp->smd_subdev); + qcom_remove_glink_subdev(rproc, &adsp->glink_subdev); + qcom_q6v5_deinit(&adsp->q6v5); detach_proxy_pds: adsp_pds_detach(adsp, adsp->proxy_pds, adsp->proxy_pd_count); +unassign_mem: + adsp_unassign_memory_region(adsp); free_rproc: device_init_wakeup(adsp->dev, false);
Current implementation of adsp_probe() does not remove the subdevs of struct qcom_adsp *adsp on the error path. Fix this bug by calling qcom_remove_{ssr,sysmon,pdm,smd,glink}_subdev(), qcom_q6v5_deinit(), and adsp_unassign_memory_region() appropriately. Fixes: 5b9f51b200dc ("remoteproc: qcom: enable in-kernel PD mapper") Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> --- drivers/remoteproc/qcom_q6v5_pas.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)