* camss: Missing cleanup on probe error
@ 2023-01-24 20:02 Luca Weiss
2023-01-25 11:34 ` Bryan O'Donoghue
2023-01-25 11:34 ` Bryan O'Donoghue
0 siblings, 2 replies; 3+ messages in thread
From: Luca Weiss @ 2023-01-24 20:02 UTC (permalink / raw)
To: Todor Tomov, Robert Foss
Cc: linux-media, linux-arm-msm, linux-kernel,
~postmarketos/upstreaming
Hi all,
I hit an issue that was already fixed with upstream commit 3d658980e6da
("media: camss: Do not attach an already attached power domain on MSM8916
platform") but this showed that some error handling is missing in camss_probe.
After these errors the resources aren't cleaned up and e.g. sensor drivers
still try to probe but fail quite badly because some things are NULL or
whatever.
[ 1.979098] qcom-camss fda0ac00.camss: Failed to configure power domains:
-17
[ 1.989327] qcom-camss: probe of fda0ac00.camss failed with error -17
The commit causing this (or at least part of this) is 2f6f8af67203 ("media:
camss: Refactor VFE power domain toggling")
I tried a bit to add some error handling but in this case it now fails at
v4l2_device_unregister_subdev -> media_device_unregister_entity ->
__media_device_unregister_entity -> ida_free.
And I'm not really sure how to fix (and honestly not super motivated).
See following patch (formatting probably messed up, sorry). Maybe someone who
knows a bit more about the driver or the media subsystem can fix this, thanks!
Regards
Luca
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/
platform/qcom/camss/camss.c
index 16545cecc4f4..0ba3c378d241 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1836,21 +1836,26 @@ static int camss_probe(struct platform_device *pdev)
}
}
- ret = camss_configure_pd(camss);
+ //ret = camss_configure_pd(camss);
+ ret = -17;
if (ret < 0) {
dev_err(dev, "Failed to configure power domains: %d\n",
ret);
- return ret;
+ goto err_configure_pd;
}
pm_runtime_enable(dev);
return 0;
+err_configure_pd:
+ media_device_unregister(&camss->media_dev);
+ media_device_cleanup(&camss->media_dev);
err_register_subdevs:
camss_unregister_entities(camss);
err_register_entities:
v4l2_device_unregister(&camss->v4l2_dev);
err_cleanup:
+ v4l2_async_nf_unregister(&camss->notifier);
v4l2_async_nf_cleanup(&camss->notifier);
return ret;
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: camss: Missing cleanup on probe error
2023-01-24 20:02 camss: Missing cleanup on probe error Luca Weiss
@ 2023-01-25 11:34 ` Bryan O'Donoghue
2023-01-25 11:34 ` Bryan O'Donoghue
1 sibling, 0 replies; 3+ messages in thread
From: Bryan O'Donoghue @ 2023-01-25 11:34 UTC (permalink / raw)
To: Luca Weiss, Todor Tomov, Robert Foss
Cc: linux-media, linux-arm-msm, linux-kernel,
~postmarketos/upstreaming
On 24/01/2023 20:02, Luca Weiss wrote:
> Hi all,
>
> I hit an issue that was already fixed with upstream commit 3d658980e6da
> ("media: camss: Do not attach an already attached power domain on MSM8916
> platform") but this showed that some error handling is missing in camss_probe.
>
> After these errors the resources aren't cleaned up and e.g. sensor drivers
> still try to probe but fail quite badly because some things are NULL or
> whatever.
>
> [ 1.979098] qcom-camss fda0ac00.camss: Failed to configure power domains:
> -17
> [ 1.989327] qcom-camss: probe of fda0ac00.camss failed with error -17
>
> The commit causing this (or at least part of this) is 2f6f8af67203 ("media:
> camss: Refactor VFE power domain toggling")
>
> I tried a bit to add some error handling but in this case it now fails at
> v4l2_device_unregister_subdev -> media_device_unregister_entity ->
> __media_device_unregister_entity -> ida_free.
> And I'm not really sure how to fix (and honestly not super motivated).
>
> See following patch (formatting probably messed up, sorry). Maybe someone who
> knows a bit more about the driver or the media subsystem can fix this, thanks!
>
> Regards
> Luca
>
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/
> platform/qcom/camss/camss.c
> index 16545cecc4f4..0ba3c378d241 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1836,21 +1836,26 @@ static int camss_probe(struct platform_device *pdev)
> }
> }
>
> - ret = camss_configure_pd(camss);
> + //ret = camss_configure_pd(camss);
> + ret = -17;
> if (ret < 0) {
> dev_err(dev, "Failed to configure power domains: %d\n",
> ret);
> - return ret;
> + goto err_configure_pd;
> }
>
> pm_runtime_enable(dev);
>
> return 0;
>
> +err_configure_pd:
> + media_device_unregister(&camss->media_dev);
> + media_device_cleanup(&camss->media_dev);
> err_register_subdevs:
> camss_unregister_entities(camss);
> err_register_entities:
> v4l2_device_unregister(&camss->v4l2_dev);
> err_cleanup:
> + v4l2_async_nf_unregister(&camss->notifier);
> v4l2_async_nf_cleanup(&camss->notifier);
>
> return ret;
>
>
>
I'll take a look
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: camss: Missing cleanup on probe error
2023-01-24 20:02 camss: Missing cleanup on probe error Luca Weiss
2023-01-25 11:34 ` Bryan O'Donoghue
@ 2023-01-25 11:34 ` Bryan O'Donoghue
1 sibling, 0 replies; 3+ messages in thread
From: Bryan O'Donoghue @ 2023-01-25 11:34 UTC (permalink / raw)
To: Luca Weiss, Todor Tomov, Robert Foss
Cc: linux-media, linux-arm-msm, linux-kernel,
~postmarketos/upstreaming
On 24/01/2023 20:02, Luca Weiss wrote:
> Hi all,
>
> I hit an issue that was already fixed with upstream commit 3d658980e6da
> ("media: camss: Do not attach an already attached power domain on MSM8916
> platform") but this showed that some error handling is missing in camss_probe.
>
> After these errors the resources aren't cleaned up and e.g. sensor drivers
> still try to probe but fail quite badly because some things are NULL or
> whatever.
>
> [ 1.979098] qcom-camss fda0ac00.camss: Failed to configure power domains:
> -17
> [ 1.989327] qcom-camss: probe of fda0ac00.camss failed with error -17
>
> The commit causing this (or at least part of this) is 2f6f8af67203 ("media:
> camss: Refactor VFE power domain toggling")
>
> I tried a bit to add some error handling but in this case it now fails at
> v4l2_device_unregister_subdev -> media_device_unregister_entity ->
> __media_device_unregister_entity -> ida_free.
> And I'm not really sure how to fix (and honestly not super motivated).
>
> See following patch (formatting probably messed up, sorry). Maybe someone who
> knows a bit more about the driver or the media subsystem can fix this, thanks!
>
> Regards
> Luca
>
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/
> platform/qcom/camss/camss.c
> index 16545cecc4f4..0ba3c378d241 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1836,21 +1836,26 @@ static int camss_probe(struct platform_device *pdev)
> }
> }
>
> - ret = camss_configure_pd(camss);
> + //ret = camss_configure_pd(camss);
> + ret = -17;
> if (ret < 0) {
> dev_err(dev, "Failed to configure power domains: %d\n",
> ret);
> - return ret;
> + goto err_configure_pd;
> }
>
> pm_runtime_enable(dev);
>
> return 0;
>
> +err_configure_pd:
> + media_device_unregister(&camss->media_dev);
> + media_device_cleanup(&camss->media_dev);
> err_register_subdevs:
> camss_unregister_entities(camss);
> err_register_entities:
> v4l2_device_unregister(&camss->v4l2_dev);
> err_cleanup:
> + v4l2_async_nf_unregister(&camss->notifier);
> v4l2_async_nf_cleanup(&camss->notifier);
>
> return ret;
>
>
>
I'll take a look
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-01-25 11:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-24 20:02 camss: Missing cleanup on probe error Luca Weiss
2023-01-25 11:34 ` Bryan O'Donoghue
2023-01-25 11:34 ` Bryan O'Donoghue
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox