diff mbox series

[v2,2/9] media: qcom: camss: Fix V4L2 async notifier error path

Message ID 20230822200626.1931129-3-bryan.odonoghue@linaro.org
State Superseded
Headers show
Series media: qcom: camss: Bugfix series | expand

Commit Message

Bryan O'Donoghue Aug. 22, 2023, 8:06 p.m. UTC
Previously the jump label err_cleanup was used higher in the probe()
function to release the async notifier however the async notifier
registration was moved later in the code rendering the previous four jumps
redundant.

Rename the label from err_cleanup to err_v4l2_device_register to capture
what the jump does.

Fixes: 51397a4ec75d ("media: qcom: Initialise V4L2 async notifier later")
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Laurent Pinchart Aug. 28, 2023, 5:05 p.m. UTC | #1
Hi Bryan,

Thank you for the patch.

On Tue, Aug 22, 2023 at 09:06:19PM +0100, Bryan O'Donoghue wrote:
> Previously the jump label err_cleanup was used higher in the probe()
> function to release the async notifier however the async notifier
> registration was moved later in the code rendering the previous four jumps
> redundant.
> 
> Rename the label from err_cleanup to err_v4l2_device_register to capture
> what the jump does.

Shouldn't it be named err_v4l2_device_unregister then ? As the
err_register_subdevs label should also be renamed err_unregister_subdevs
you could rename them all in a separate patch.

> Fixes: 51397a4ec75d ("media: qcom: Initialise V4L2 async notifier later")
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/platform/qcom/camss/camss.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 75991d849b571..f4948bdf3f8f9 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1617,21 +1617,21 @@ static int camss_probe(struct platform_device *pdev)
>  
>  	ret = camss_icc_get(camss);
>  	if (ret < 0)
> -		goto err_cleanup;
> +		return ret;
>  
>  	ret = camss_configure_pd(camss);
>  	if (ret < 0) {
>  		dev_err(dev, "Failed to configure power domains: %d\n", ret);
> -		goto err_cleanup;
> +		return ret;
>  	}
>  
>  	ret = camss_init_subdevices(camss);
>  	if (ret < 0)
> -		goto err_cleanup;
> +		return ret;
>  
>  	ret = dma_set_mask_and_coherent(dev, 0xffffffff);
>  	if (ret)
> -		goto err_cleanup;
> +		return ret;

This doesn't seem right, you should call v4l2_async_nf_cleanup() when
v4l2_async_nf_init() has been called, and that's done before
camss_icc_get().

>  
>  	camss->media_dev.dev = camss->dev;
>  	strscpy(camss->media_dev.model, "Qualcomm Camera Subsystem",
> @@ -1643,7 +1643,7 @@ static int camss_probe(struct platform_device *pdev)
>  	ret = v4l2_device_register(camss->dev, &camss->v4l2_dev);
>  	if (ret < 0) {
>  		dev_err(dev, "Failed to register V4L2 device: %d\n", ret);
> -		goto err_cleanup;
> +		return ret;
>  	}
>  
>  	v4l2_async_nf_init(&camss->notifier, &camss->v4l2_dev);
> @@ -1651,12 +1651,12 @@ static int camss_probe(struct platform_device *pdev)
>  	num_subdevs = camss_of_parse_ports(camss);
>  	if (num_subdevs < 0) {
>  		ret = num_subdevs;
> -		goto err_cleanup;
> +		goto err_v4l2_device_register;
>  	}
>  
>  	ret = camss_register_entities(camss);
>  	if (ret < 0)
> -		goto err_cleanup;
> +		goto err_v4l2_device_register;
>  
>  	if (num_subdevs) {
>  		camss->notifier.ops = &camss_subdev_notifier_ops;
> @@ -1690,7 +1690,7 @@ static int camss_probe(struct platform_device *pdev)
>  
>  err_register_subdevs:
>  	camss_unregister_entities(camss);
> -err_cleanup:
> +err_v4l2_device_register:
>  	v4l2_device_unregister(&camss->v4l2_dev);
>  	v4l2_async_nf_cleanup(&camss->notifier);
>
Bryan O'Donoghue Aug. 29, 2023, 10:17 a.m. UTC | #2
On 28/08/2023 18:05, Laurent Pinchart wrote:
> Hi Bryan,
> 
> Thank you for the patch.
> 
> On Tue, Aug 22, 2023 at 09:06:19PM +0100, Bryan O'Donoghue wrote:
>> Previously the jump label err_cleanup was used higher in the probe()
>> function to release the async notifier however the async notifier
>> registration was moved later in the code rendering the previous four jumps
>> redundant.
>>
>> Rename the label from err_cleanup to err_v4l2_device_register to capture
>> what the jump does.
> 
> Shouldn't it be named err_v4l2_device_unregister then ? As the
> err_register_subdevs label should also be renamed err_unregister_subdevs
> you could rename them all in a separate patch.
> 
>> Fixes: 51397a4ec75d ("media: qcom: Initialise V4L2 async notifier later")
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>   drivers/media/platform/qcom/camss/camss.c | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
>> index 75991d849b571..f4948bdf3f8f9 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -1617,21 +1617,21 @@ static int camss_probe(struct platform_device *pdev)
>>   
>>   	ret = camss_icc_get(camss);
>>   	if (ret < 0)
>> -		goto err_cleanup;
>> +		return ret;
>>   
>>   	ret = camss_configure_pd(camss);
>>   	if (ret < 0) {
>>   		dev_err(dev, "Failed to configure power domains: %d\n", ret);
>> -		goto err_cleanup;
>> +		return ret;
>>   	}
>>   
>>   	ret = camss_init_subdevices(camss);
>>   	if (ret < 0)
>> -		goto err_cleanup;
>> +		return ret;
>>   
>>   	ret = dma_set_mask_and_coherent(dev, 0xffffffff);
>>   	if (ret)
>> -		goto err_cleanup;
>> +		return ret;
> 
> This doesn't seem right, you should call v4l2_async_nf_cleanup() when
> v4l2_async_nf_init() has been called, and that's done before
> camss_icc_get().

Ah no, after 51397a4ec75d ("media: qcom: Initialise V4L2 async notifier 
later") the behaviour changes.

-       v4l2_async_nf_init(&camss->notifier);
-
-       num_subdevs = camss_of_parse_ports(camss);
-       if (num_subdevs < 0) {
-               ret = num_subdevs;
-               goto err_cleanup;
-       }
-
         ret = camss_icc_get(camss);
         if (ret < 0)
                 goto err_cleanup;
@@ -1648,9 +1640,17 @@ static int camss_probe(struct platform_device *pdev)
                 goto err_cleanup;
         }

+       v4l2_async_nf_init(&camss->notifier);
+
+       num_subdevs = camss_of_parse_ports(camss);
+       if (num_subdevs < 0) {
+               ret = num_subdevs;
+               goto err_cleanup;
+       }


This patch is still a good opportunity to fix the jump labels for 
example genpd which you mentioned earlier.

---
bod
diff mbox series

Patch

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 75991d849b571..f4948bdf3f8f9 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1617,21 +1617,21 @@  static int camss_probe(struct platform_device *pdev)
 
 	ret = camss_icc_get(camss);
 	if (ret < 0)
-		goto err_cleanup;
+		return ret;
 
 	ret = camss_configure_pd(camss);
 	if (ret < 0) {
 		dev_err(dev, "Failed to configure power domains: %d\n", ret);
-		goto err_cleanup;
+		return ret;
 	}
 
 	ret = camss_init_subdevices(camss);
 	if (ret < 0)
-		goto err_cleanup;
+		return ret;
 
 	ret = dma_set_mask_and_coherent(dev, 0xffffffff);
 	if (ret)
-		goto err_cleanup;
+		return ret;
 
 	camss->media_dev.dev = camss->dev;
 	strscpy(camss->media_dev.model, "Qualcomm Camera Subsystem",
@@ -1643,7 +1643,7 @@  static int camss_probe(struct platform_device *pdev)
 	ret = v4l2_device_register(camss->dev, &camss->v4l2_dev);
 	if (ret < 0) {
 		dev_err(dev, "Failed to register V4L2 device: %d\n", ret);
-		goto err_cleanup;
+		return ret;
 	}
 
 	v4l2_async_nf_init(&camss->notifier, &camss->v4l2_dev);
@@ -1651,12 +1651,12 @@  static int camss_probe(struct platform_device *pdev)
 	num_subdevs = camss_of_parse_ports(camss);
 	if (num_subdevs < 0) {
 		ret = num_subdevs;
-		goto err_cleanup;
+		goto err_v4l2_device_register;
 	}
 
 	ret = camss_register_entities(camss);
 	if (ret < 0)
-		goto err_cleanup;
+		goto err_v4l2_device_register;
 
 	if (num_subdevs) {
 		camss->notifier.ops = &camss_subdev_notifier_ops;
@@ -1690,7 +1690,7 @@  static int camss_probe(struct platform_device *pdev)
 
 err_register_subdevs:
 	camss_unregister_entities(camss);
-err_cleanup:
+err_v4l2_device_register:
 	v4l2_device_unregister(&camss->v4l2_dev);
 	v4l2_async_nf_cleanup(&camss->notifier);