| Message ID | 20240123184632.725054-6-afd@ti.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/9] remoteproc: imx_dsp_rproc: Use devm_rproc_alloc() helper | expand |
On 1/23/2024 10:46 AM, Andrew Davis wrote: > Use the device lifecycle managed allocation function. This helps prevent > mistakes like freeing out of order in cleanup functions and forgetting to > free on error paths. > > Signed-off-by: Andrew Davis <afd@ti.com> > --- > drivers/remoteproc/qcom_q6v5_wcss.c | 24 +++++++++--------------- > 1 file changed, 9 insertions(+), 15 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c > index cff1fa07d1def..94f68c919ee62 100644 > --- a/drivers/remoteproc/qcom_q6v5_wcss.c > +++ b/drivers/remoteproc/qcom_q6v5_wcss.c > @@ -1011,8 +1011,8 @@ static int q6v5_wcss_probe(struct platform_device *pdev) > if (!desc) > return -EINVAL; > > - rproc = rproc_alloc(&pdev->dev, pdev->name, desc->ops, > - desc->firmware_name, sizeof(*wcss)); > + rproc = devm_rproc_alloc(&pdev->dev, pdev->name, desc->ops, > + desc->firmware_name, sizeof(*wcss)); > if (!rproc) { > dev_err(&pdev->dev, "failed to allocate rproc\n"); > return -ENOMEM; > @@ -1027,29 +1027,29 @@ static int q6v5_wcss_probe(struct platform_device *pdev) > > ret = q6v5_wcss_init_mmio(wcss, pdev); > if (ret) > - goto free_rproc; > + return ret; > > ret = q6v5_alloc_memory_region(wcss); > if (ret) > - goto free_rproc; > + return ret; > > if (wcss->version == WCSS_QCS404) { > ret = q6v5_wcss_init_clock(wcss); > if (ret) > - goto free_rproc; > + return ret; > > ret = q6v5_wcss_init_regulator(wcss); > if (ret) > - goto free_rproc; > + return ret; > } > > ret = q6v5_wcss_init_reset(wcss, desc); > if (ret) > - goto free_rproc; > + return ret; > > ret = qcom_q6v5_init(&wcss->q6v5, pdev, rproc, desc->crash_reason_smem, NULL, NULL); > if (ret) > - goto free_rproc; > + return ret; > > qcom_add_glink_subdev(rproc, &wcss->glink_subdev, "q6wcss"); > qcom_add_ssr_subdev(rproc, &wcss->ssr_subdev, "q6wcss"); > @@ -1061,16 +1061,11 @@ static int q6v5_wcss_probe(struct platform_device *pdev) > > ret = rproc_add(rproc); > if (ret) > - goto free_rproc; > + return ret; > > platform_set_drvdata(pdev, rproc); > > return 0; > - > -free_rproc: > - rproc_free(rproc); > - > - return ret; This return statement should stay, right? > } > > static void q6v5_wcss_remove(struct platform_device *pdev) > @@ -1080,7 +1075,6 @@ static void q6v5_wcss_remove(struct platform_device *pdev) > > qcom_q6v5_deinit(&wcss->q6v5); > rproc_del(rproc); > - rproc_free(rproc); > } > > static const struct wcss_data wcss_ipq8074_res_init = {
On 2/2/24 2:51 PM, Unnathi Chalicheemala wrote: > > > On 1/23/2024 10:46 AM, Andrew Davis wrote: >> Use the device lifecycle managed allocation function. This helps prevent >> mistakes like freeing out of order in cleanup functions and forgetting to >> free on error paths. >> >> Signed-off-by: Andrew Davis <afd@ti.com> >> --- >> drivers/remoteproc/qcom_q6v5_wcss.c | 24 +++++++++--------------- >> 1 file changed, 9 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c >> index cff1fa07d1def..94f68c919ee62 100644 >> --- a/drivers/remoteproc/qcom_q6v5_wcss.c >> +++ b/drivers/remoteproc/qcom_q6v5_wcss.c >> @@ -1011,8 +1011,8 @@ static int q6v5_wcss_probe(struct platform_device *pdev) >> if (!desc) >> return -EINVAL; >> >> - rproc = rproc_alloc(&pdev->dev, pdev->name, desc->ops, >> - desc->firmware_name, sizeof(*wcss)); >> + rproc = devm_rproc_alloc(&pdev->dev, pdev->name, desc->ops, >> + desc->firmware_name, sizeof(*wcss)); >> if (!rproc) { >> dev_err(&pdev->dev, "failed to allocate rproc\n"); >> return -ENOMEM; >> @@ -1027,29 +1027,29 @@ static int q6v5_wcss_probe(struct platform_device *pdev) >> >> ret = q6v5_wcss_init_mmio(wcss, pdev); >> if (ret) >> - goto free_rproc; >> + return ret; >> >> ret = q6v5_alloc_memory_region(wcss); >> if (ret) >> - goto free_rproc; >> + return ret; >> >> if (wcss->version == WCSS_QCS404) { >> ret = q6v5_wcss_init_clock(wcss); >> if (ret) >> - goto free_rproc; >> + return ret; >> >> ret = q6v5_wcss_init_regulator(wcss); >> if (ret) >> - goto free_rproc; >> + return ret; >> } >> >> ret = q6v5_wcss_init_reset(wcss, desc); >> if (ret) >> - goto free_rproc; >> + return ret; >> >> ret = qcom_q6v5_init(&wcss->q6v5, pdev, rproc, desc->crash_reason_smem, NULL, NULL); >> if (ret) >> - goto free_rproc; >> + return ret; >> >> qcom_add_glink_subdev(rproc, &wcss->glink_subdev, "q6wcss"); >> qcom_add_ssr_subdev(rproc, &wcss->ssr_subdev, "q6wcss"); >> @@ -1061,16 +1061,11 @@ static int q6v5_wcss_probe(struct platform_device *pdev) >> >> ret = rproc_add(rproc); >> if (ret) >> - goto free_rproc; >> + return ret; >> >> platform_set_drvdata(pdev, rproc); >> >> return 0; >> - >> -free_rproc: >> - rproc_free(rproc); >> - >> - return ret; > > This return statement should stay, right? > No path goes to "free_rproc" anymore, so we always do the "return 0;" above on non-error paths. Andrew >> } >> >> static void q6v5_wcss_remove(struct platform_device *pdev) >> @@ -1080,7 +1075,6 @@ static void q6v5_wcss_remove(struct platform_device *pdev) >> >> qcom_q6v5_deinit(&wcss->q6v5); >> rproc_del(rproc); >> - rproc_free(rproc); >> } >> >> static const struct wcss_data wcss_ipq8074_res_init = {
On 2/2/2024 1:14 PM, Andrew Davis wrote: > On 2/2/24 2:51 PM, Unnathi Chalicheemala wrote: >> >> >> On 1/23/2024 10:46 AM, Andrew Davis wrote: >>> Use the device lifecycle managed allocation function. This helps prevent >>> mistakes like freeing out of order in cleanup functions and forgetting to >>> free on error paths. >>> >>> Signed-off-by: Andrew Davis <afd@ti.com> >>> --- >>> drivers/remoteproc/qcom_q6v5_wcss.c | 24 +++++++++--------------- >>> 1 file changed, 9 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c >>> index cff1fa07d1def..94f68c919ee62 100644 >>> --- a/drivers/remoteproc/qcom_q6v5_wcss.c >>> +++ b/drivers/remoteproc/qcom_q6v5_wcss.c >>> @@ -1011,8 +1011,8 @@ static int q6v5_wcss_probe(struct platform_device *pdev) >>> if (!desc) >>> return -EINVAL; >>> - rproc = rproc_alloc(&pdev->dev, pdev->name, desc->ops, >>> - desc->firmware_name, sizeof(*wcss)); >>> + rproc = devm_rproc_alloc(&pdev->dev, pdev->name, desc->ops, >>> + desc->firmware_name, sizeof(*wcss)); >>> if (!rproc) { >>> dev_err(&pdev->dev, "failed to allocate rproc\n"); >>> return -ENOMEM; >>> @@ -1027,29 +1027,29 @@ static int q6v5_wcss_probe(struct platform_device *pdev) >>> ret = q6v5_wcss_init_mmio(wcss, pdev); >>> if (ret) >>> - goto free_rproc; >>> + return ret; >>> ret = q6v5_alloc_memory_region(wcss); >>> if (ret) >>> - goto free_rproc; >>> + return ret; >>> if (wcss->version == WCSS_QCS404) { >>> ret = q6v5_wcss_init_clock(wcss); >>> if (ret) >>> - goto free_rproc; >>> + return ret; >>> ret = q6v5_wcss_init_regulator(wcss); >>> if (ret) >>> - goto free_rproc; >>> + return ret; >>> } >>> ret = q6v5_wcss_init_reset(wcss, desc); >>> if (ret) >>> - goto free_rproc; >>> + return ret; >>> ret = qcom_q6v5_init(&wcss->q6v5, pdev, rproc, desc->crash_reason_smem, NULL, NULL); >>> if (ret) >>> - goto free_rproc; >>> + return ret; >>> qcom_add_glink_subdev(rproc, &wcss->glink_subdev, "q6wcss"); >>> qcom_add_ssr_subdev(rproc, &wcss->ssr_subdev, "q6wcss"); >>> @@ -1061,16 +1061,11 @@ static int q6v5_wcss_probe(struct platform_device *pdev) >>> ret = rproc_add(rproc); >>> if (ret) >>> - goto free_rproc; >>> + return ret; >>> platform_set_drvdata(pdev, rproc); >>> return 0; >>> - >>> -free_rproc: >>> - rproc_free(rproc); >>> - >>> - return ret; >> >> This return statement should stay, right? >> > > No path goes to "free_rproc" anymore, so we always do the "return 0;" > above on non-error paths. > > Andrew > You're right, thanks all QCOM peripheral patches LGTM. >>> } >>> static void q6v5_wcss_remove(struct platform_device *pdev) >>> @@ -1080,7 +1075,6 @@ static void q6v5_wcss_remove(struct platform_device *pdev) >>> qcom_q6v5_deinit(&wcss->q6v5); >>> rproc_del(rproc); >>> - rproc_free(rproc); >>> } >>> static const struct wcss_data wcss_ipq8074_res_init = {
diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c b/drivers/remoteproc/qcom_q6v5_wcss.c index cff1fa07d1def..94f68c919ee62 100644 --- a/drivers/remoteproc/qcom_q6v5_wcss.c +++ b/drivers/remoteproc/qcom_q6v5_wcss.c @@ -1011,8 +1011,8 @@ static int q6v5_wcss_probe(struct platform_device *pdev) if (!desc) return -EINVAL; - rproc = rproc_alloc(&pdev->dev, pdev->name, desc->ops, - desc->firmware_name, sizeof(*wcss)); + rproc = devm_rproc_alloc(&pdev->dev, pdev->name, desc->ops, + desc->firmware_name, sizeof(*wcss)); if (!rproc) { dev_err(&pdev->dev, "failed to allocate rproc\n"); return -ENOMEM; @@ -1027,29 +1027,29 @@ static int q6v5_wcss_probe(struct platform_device *pdev) ret = q6v5_wcss_init_mmio(wcss, pdev); if (ret) - goto free_rproc; + return ret; ret = q6v5_alloc_memory_region(wcss); if (ret) - goto free_rproc; + return ret; if (wcss->version == WCSS_QCS404) { ret = q6v5_wcss_init_clock(wcss); if (ret) - goto free_rproc; + return ret; ret = q6v5_wcss_init_regulator(wcss); if (ret) - goto free_rproc; + return ret; } ret = q6v5_wcss_init_reset(wcss, desc); if (ret) - goto free_rproc; + return ret; ret = qcom_q6v5_init(&wcss->q6v5, pdev, rproc, desc->crash_reason_smem, NULL, NULL); if (ret) - goto free_rproc; + return ret; qcom_add_glink_subdev(rproc, &wcss->glink_subdev, "q6wcss"); qcom_add_ssr_subdev(rproc, &wcss->ssr_subdev, "q6wcss"); @@ -1061,16 +1061,11 @@ static int q6v5_wcss_probe(struct platform_device *pdev) ret = rproc_add(rproc); if (ret) - goto free_rproc; + return ret; platform_set_drvdata(pdev, rproc); return 0; - -free_rproc: - rproc_free(rproc); - - return ret; } static void q6v5_wcss_remove(struct platform_device *pdev) @@ -1080,7 +1075,6 @@ static void q6v5_wcss_remove(struct platform_device *pdev) qcom_q6v5_deinit(&wcss->q6v5); rproc_del(rproc); - rproc_free(rproc); } static const struct wcss_data wcss_ipq8074_res_init = {
Use the device lifecycle managed allocation function. This helps prevent mistakes like freeing out of order in cleanup functions and forgetting to free on error paths. Signed-off-by: Andrew Davis <afd@ti.com> --- drivers/remoteproc/qcom_q6v5_wcss.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)