camss: Missing cleanup on probe error

Posted by Luca Weiss 2 years, 7 months ago

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;

Re: camss: Missing cleanup on probe error

Posted by Bryan O'Donoghue 2 years, 7 months ago

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

Re: camss: Missing cleanup on probe error

Posted by Bryan O'Donoghue 2 years, 7 months ago

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