public inbox for linux-arm-kernel@lists.infradead.org
 help / color / mirror / Atom feed
* [PATCH v2] iio: adc: mxs-lradc: fix the order of two cleanup operations
@ 2023-04-16  7:21 Jiakai Luo
  2023-04-16 12:28 ` Jonathan Cameron
  2023-04-16 12:29 ` Jonathan Cameron
  0 siblings, 2 replies; 8+ messages in thread
From: Jiakai Luo @ 2023-04-16  7:21 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Ksenija Stanojevic, Lee Jones, Marek Vasut
  Cc: Jiakai Luo, linux-iio, linux-arm-kernel, linux-kernel

Smatch reports:
drivers/iio/adc/mxs-lradc-adc.c:766 mxs_lradc_adc_probe() warn:
missing unwind goto?

the order of three init operation:
1.mxs_lradc_adc_trigger_init
2.iio_triggered_buffer_setup
3.mxs_lradc_adc_hw_init

thus, the order of three cleanup operation should be:
1.mxs_lradc_adc_hw_stop
2.iio_triggered_buffer_cleanup
3.mxs_lradc_adc_trigger_remove

we exchange the order of two cleanup operations,
introducing the following differences:
1.if mxs_lradc_adc_trigger_init fails, returns directly;
2.if trigger_init succeeds but iio_triggered_buffer_setup fails,
goto err_trig and remove the trigger.

v1->v2: exchange the order of mxs_lradc_adc_trigger_remove()
and iio_triggered_buffer_cleanup() in error handling labels

Fixes: 6dd112b9f85e ("iio: adc: mxs-lradc: Add support for ADC driver")
Signed-off-by: Jiakai Luo <jkluo@hust.edu.cn>
Reviewed-by: Dongliang Mu <dzm91@hust.edu.cn>
---
 drivers/iio/adc/mxs-lradc-adc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/mxs-lradc-adc.c b/drivers/iio/adc/mxs-lradc-adc.c
index bca79a93cbe4..d32e9b1d03ce 100644
--- a/drivers/iio/adc/mxs-lradc-adc.c
+++ b/drivers/iio/adc/mxs-lradc-adc.c
@@ -757,13 +757,13 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
 
 	ret = mxs_lradc_adc_trigger_init(iio);
 	if (ret)
-		goto err_trig;
+		return ret;
 
 	ret = iio_triggered_buffer_setup(iio, &iio_pollfunc_store_time,
 					 &mxs_lradc_adc_trigger_handler,
 					 &mxs_lradc_adc_buffer_ops);
 	if (ret)
-		return ret;
+		goto err_trig;
 
 	adc->vref_mv = mxs_lradc_adc_vref_mv[lradc->soc];
 
@@ -801,9 +801,9 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
 
 err_dev:
 	mxs_lradc_adc_hw_stop(adc);
-	mxs_lradc_adc_trigger_remove(iio);
-err_trig:
 	iio_triggered_buffer_cleanup(iio);
+err_trig:
+	mxs_lradc_adc_trigger_remove(iio);
 	return ret;
 }
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] iio: adc: mxs-lradc: fix the order of two cleanup operations
  2023-04-16  7:21 Jiakai Luo
@ 2023-04-16 12:28 ` Jonathan Cameron
  2023-04-16 12:29 ` Jonathan Cameron
  1 sibling, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2023-04-16 12:28 UTC (permalink / raw)
  To: Jiakai Luo
  Cc: Lars-Peter Clausen, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Ksenija Stanojevic, Lee Jones, Marek Vasut, linux-iio,
	linux-arm-kernel, linux-kernel

On Sun, 16 Apr 2023 00:21:57 -0700
Jiakai Luo <jkluo@hust.edu.cn> wrote:

> Smatch reports:
> drivers/iio/adc/mxs-lradc-adc.c:766 mxs_lradc_adc_probe() warn:
> missing unwind goto?
> 
> the order of three init operation:
> 1.mxs_lradc_adc_trigger_init
> 2.iio_triggered_buffer_setup
> 3.mxs_lradc_adc_hw_init
> 
> thus, the order of three cleanup operation should be:
> 1.mxs_lradc_adc_hw_stop
> 2.iio_triggered_buffer_cleanup
> 3.mxs_lradc_adc_trigger_remove
> 
> we exchange the order of two cleanup operations,
> introducing the following differences:
> 1.if mxs_lradc_adc_trigger_init fails, returns directly;
> 2.if trigger_init succeeds but iio_triggered_buffer_setup fails,
> goto err_trig and remove the trigger.
> 
> v1->v2: exchange the order of mxs_lradc_adc_trigger_remove()
> and iio_triggered_buffer_cleanup() in error handling labels
> 
> Fixes: 6dd112b9f85e ("iio: adc: mxs-lradc: Add support for ADC driver")
> Signed-off-by: Jiakai Luo <jkluo@hust.edu.cn>
> Reviewed-by: Dongliang Mu <dzm91@hust.edu.cn>

Thanks for the patch.

I agree with your analysis. 

Please also reorder the unwind that goes on in the remove() callback
to match the new ordering.  That way things remain consistent between
the remove() calls and the error handling.  I doubt there is a bug 
due to the ordering in remove() but there might be.

Jonathan


> ---
>  drivers/iio/adc/mxs-lradc-adc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/mxs-lradc-adc.c b/drivers/iio/adc/mxs-lradc-adc.c
> index bca79a93cbe4..d32e9b1d03ce 100644
> --- a/drivers/iio/adc/mxs-lradc-adc.c
> +++ b/drivers/iio/adc/mxs-lradc-adc.c
> @@ -757,13 +757,13 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
>  
>  	ret = mxs_lradc_adc_trigger_init(iio);
>  	if (ret)
> -		goto err_trig;
> +		return ret;
>  
>  	ret = iio_triggered_buffer_setup(iio, &iio_pollfunc_store_time,
>  					 &mxs_lradc_adc_trigger_handler,
>  					 &mxs_lradc_adc_buffer_ops);
>  	if (ret)
> -		return ret;
> +		goto err_trig;
>  
>  	adc->vref_mv = mxs_lradc_adc_vref_mv[lradc->soc];
>  
> @@ -801,9 +801,9 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
>  
>  err_dev:
>  	mxs_lradc_adc_hw_stop(adc);
> -	mxs_lradc_adc_trigger_remove(iio);
> -err_trig:
>  	iio_triggered_buffer_cleanup(iio);
> +err_trig:
> +	mxs_lradc_adc_trigger_remove(iio);
>  	return ret;
>  }
>  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] iio: adc: mxs-lradc: fix the order of two cleanup operations
  2023-04-16  7:21 Jiakai Luo
  2023-04-16 12:28 ` Jonathan Cameron
@ 2023-04-16 12:29 ` Jonathan Cameron
  2023-04-22 13:34   ` Jiakai Luo
  1 sibling, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2023-04-16 12:29 UTC (permalink / raw)
  To: Jiakai Luo
  Cc: Lars-Peter Clausen, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Ksenija Stanojevic, Lee Jones, Marek Vasut, linux-iio,
	linux-arm-kernel, linux-kernel

On Sun, 16 Apr 2023 00:21:57 -0700
Jiakai Luo <jkluo@hust.edu.cn> wrote:

> Smatch reports:
> drivers/iio/adc/mxs-lradc-adc.c:766 mxs_lradc_adc_probe() warn:
> missing unwind goto?
> 
> the order of three init operation:
> 1.mxs_lradc_adc_trigger_init
> 2.iio_triggered_buffer_setup
> 3.mxs_lradc_adc_hw_init
> 
> thus, the order of three cleanup operation should be:
> 1.mxs_lradc_adc_hw_stop
> 2.iio_triggered_buffer_cleanup
> 3.mxs_lradc_adc_trigger_remove
> 
> we exchange the order of two cleanup operations,
> introducing the following differences:
> 1.if mxs_lradc_adc_trigger_init fails, returns directly;
> 2.if trigger_init succeeds but iio_triggered_buffer_setup fails,
> goto err_trig and remove the trigger.
> 
> v1->v2: exchange the order of mxs_lradc_adc_trigger_remove()
> and iio_triggered_buffer_cleanup() in error handling labels
Move your change log to after the --- marking below.

We don't want to retain that level of detail in the commit logs in the
git tree.

Thanks,

Jonathan

> 
> Fixes: 6dd112b9f85e ("iio: adc: mxs-lradc: Add support for ADC driver")
> Signed-off-by: Jiakai Luo <jkluo@hust.edu.cn>
> Reviewed-by: Dongliang Mu <dzm91@hust.edu.cn>
> ---
>  drivers/iio/adc/mxs-lradc-adc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/mxs-lradc-adc.c b/drivers/iio/adc/mxs-lradc-adc.c
> index bca79a93cbe4..d32e9b1d03ce 100644
> --- a/drivers/iio/adc/mxs-lradc-adc.c
> +++ b/drivers/iio/adc/mxs-lradc-adc.c
> @@ -757,13 +757,13 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
>  
>  	ret = mxs_lradc_adc_trigger_init(iio);
>  	if (ret)
> -		goto err_trig;
> +		return ret;
>  
>  	ret = iio_triggered_buffer_setup(iio, &iio_pollfunc_store_time,
>  					 &mxs_lradc_adc_trigger_handler,
>  					 &mxs_lradc_adc_buffer_ops);
>  	if (ret)
> -		return ret;
> +		goto err_trig;
>  
>  	adc->vref_mv = mxs_lradc_adc_vref_mv[lradc->soc];
>  
> @@ -801,9 +801,9 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
>  
>  err_dev:
>  	mxs_lradc_adc_hw_stop(adc);
> -	mxs_lradc_adc_trigger_remove(iio);
> -err_trig:
>  	iio_triggered_buffer_cleanup(iio);
> +err_trig:
> +	mxs_lradc_adc_trigger_remove(iio);
>  	return ret;
>  }
>  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] iio: adc: mxs-lradc: fix the order of two cleanup operations
@ 2023-04-17  2:47 Jiakai Luo
  2023-04-23 10:50 ` Jonathan Cameron
  0 siblings, 1 reply; 8+ messages in thread
From: Jiakai Luo @ 2023-04-17  2:47 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Ksenija Stanojevic, Lee Jones, Marek Vasut
  Cc: hust-os-kernel-patches, Jiakai Luo, linux-iio, linux-arm-kernel,
	linux-kernel

Smatch reports:
drivers/iio/adc/mxs-lradc-adc.c:766 mxs_lradc_adc_probe() warn:
missing unwind goto?

the order of three init operation:
1.mxs_lradc_adc_trigger_init
2.iio_triggered_buffer_setup
3.mxs_lradc_adc_hw_init

thus, the order of three cleanup operation should be:
1.mxs_lradc_adc_hw_stop
2.iio_triggered_buffer_cleanup
3.mxs_lradc_adc_trigger_remove

we exchange the order of two cleanup operations,
introducing the following differences:
1.if mxs_lradc_adc_trigger_init fails, returns directly;
2.if trigger_init succeeds but iio_triggered_buffer_setup fails,
goto err_trig and remove the trigger.

In addition, we also reorder the unwind that goes on in the
remove() callback to match the new ordering.

Fixes: 6dd112b9f85e ("iio: adc: mxs-lradc: Add support for ADC driver")
Signed-off-by: Jiakai Luo <jkluo@hust.edu.cn>
Reviewed-by: Dongliang Mu <dzm91@hust.edu.cn>
---
The issue is found by static analysis and remains untested.
---
 drivers/iio/adc/mxs-lradc-adc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/mxs-lradc-adc.c b/drivers/iio/adc/mxs-lradc-adc.c
index bca79a93cbe4..85882509b7d9 100644
--- a/drivers/iio/adc/mxs-lradc-adc.c
+++ b/drivers/iio/adc/mxs-lradc-adc.c
@@ -757,13 +757,13 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
 
 	ret = mxs_lradc_adc_trigger_init(iio);
 	if (ret)
-		goto err_trig;
+		return ret;
 
 	ret = iio_triggered_buffer_setup(iio, &iio_pollfunc_store_time,
 					 &mxs_lradc_adc_trigger_handler,
 					 &mxs_lradc_adc_buffer_ops);
 	if (ret)
-		return ret;
+		goto err_trig;
 
 	adc->vref_mv = mxs_lradc_adc_vref_mv[lradc->soc];
 
@@ -801,9 +801,9 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
 
 err_dev:
 	mxs_lradc_adc_hw_stop(adc);
-	mxs_lradc_adc_trigger_remove(iio);
-err_trig:
 	iio_triggered_buffer_cleanup(iio);
+err_trig:
+	mxs_lradc_adc_trigger_remove(iio);
 	return ret;
 }
 
@@ -814,8 +814,8 @@ static int mxs_lradc_adc_remove(struct platform_device *pdev)
 
 	iio_device_unregister(iio);
 	mxs_lradc_adc_hw_stop(adc);
-	mxs_lradc_adc_trigger_remove(iio);
 	iio_triggered_buffer_cleanup(iio);
+	mxs_lradc_adc_trigger_remove(iio);

 	return 0;
 }
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2] iio: adc: mxs-lradc: fix the order of two cleanup operations
  2023-04-16 12:29 ` Jonathan Cameron
@ 2023-04-22 13:34   ` Jiakai Luo
  2023-04-23 10:40     ` Jonathan Cameron
  0 siblings, 1 reply; 8+ messages in thread
From: Jiakai Luo @ 2023-04-22 13:34 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Lee Jones,
	Marek Vasut, Ksenija Stanojevic
  Cc: hust-os-kernel-patches, Jiakai Luo, linux-iio, linux-arm-kernel,
	linux-kernel

Smatch reports:
drivers/iio/adc/mxs-lradc-adc.c:766 mxs_lradc_adc_probe() warn:
missing unwind goto?

the order of three init operation:
1.mxs_lradc_adc_trigger_init
2.iio_triggered_buffer_setup
3.mxs_lradc_adc_hw_init

thus, the order of three cleanup operation should be:
1.mxs_lradc_adc_hw_stop
2.iio_triggered_buffer_cleanup
3.mxs_lradc_adc_trigger_remove

we exchange the order of two cleanup operations,
introducing the following differences:
1.if mxs_lradc_adc_trigger_init fails, returns directly;
2.if trigger_init succeeds but iio_triggered_buffer_setup fails,
goto err_trig and remove the trigger.

In addition, we also reorder the unwind that goes on in the
remove() callback to match the new ordering.

Fixes: 6dd112b9f85e ("iio: adc: mxs-lradc: Add support for ADC driver")
Signed-off-by: Jiakai Luo <jkluo@hust.edu.cn>
Reviewed-by: Dongliang Mu <dzm91@hust.edu.cn>
---
The issue is found by static analysis and remains untested.
---
 drivers/iio/adc/mxs-lradc-adc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/mxs-lradc-adc.c b/drivers/iio/adc/mxs-lradc-adc.c
index bca79a93cbe4..85882509b7d9 100644
--- a/drivers/iio/adc/mxs-lradc-adc.c
+++ b/drivers/iio/adc/mxs-lradc-adc.c
@@ -757,13 +757,13 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
 
 	ret = mxs_lradc_adc_trigger_init(iio);
 	if (ret)
-		goto err_trig;
+		return ret;
 
 	ret = iio_triggered_buffer_setup(iio, &iio_pollfunc_store_time,
 					 &mxs_lradc_adc_trigger_handler,
 					 &mxs_lradc_adc_buffer_ops);
 	if (ret)
-		return ret;
+		goto err_trig;
 
 	adc->vref_mv = mxs_lradc_adc_vref_mv[lradc->soc];
 
@@ -801,9 +801,9 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
 
 err_dev:
 	mxs_lradc_adc_hw_stop(adc);
-	mxs_lradc_adc_trigger_remove(iio);
-err_trig:
 	iio_triggered_buffer_cleanup(iio);
+err_trig:
+	mxs_lradc_adc_trigger_remove(iio);
 	return ret;
 }
 
@@ -814,8 +814,8 @@ static int mxs_lradc_adc_remove(struct platform_device *pdev)
 
 	iio_device_unregister(iio);
 	mxs_lradc_adc_hw_stop(adc);
-	mxs_lradc_adc_trigger_remove(iio);
 	iio_triggered_buffer_cleanup(iio);
+	mxs_lradc_adc_trigger_remove(iio);

 	return 0;
 }
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] iio: adc: mxs-lradc: fix the order of two cleanup operations
  2023-04-22 13:34   ` Jiakai Luo
@ 2023-04-23 10:40     ` Jonathan Cameron
  0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2023-04-23 10:40 UTC (permalink / raw)
  To: Jiakai Luo
  Cc: Lars-Peter Clausen, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Lee Jones,
	Marek Vasut, Ksenija Stanojevic, hust-os-kernel-patches,
	linux-iio, linux-arm-kernel, linux-kernel

On Sat, 22 Apr 2023 06:34:06 -0700
Jiakai Luo <jkluo@hust.edu.cn> wrote:

> Smatch reports:
> drivers/iio/adc/mxs-lradc-adc.c:766 mxs_lradc_adc_probe() warn:
> missing unwind goto?
> 
> the order of three init operation:
> 1.mxs_lradc_adc_trigger_init
> 2.iio_triggered_buffer_setup
> 3.mxs_lradc_adc_hw_init
> 
> thus, the order of three cleanup operation should be:
> 1.mxs_lradc_adc_hw_stop
> 2.iio_triggered_buffer_cleanup
> 3.mxs_lradc_adc_trigger_remove
> 
> we exchange the order of two cleanup operations,
> introducing the following differences:
> 1.if mxs_lradc_adc_trigger_init fails, returns directly;
> 2.if trigger_init succeeds but iio_triggered_buffer_setup fails,
> goto err_trig and remove the trigger.
> 
> In addition, we also reorder the unwind that goes on in the
> remove() callback to match the new ordering.
> 
> Fixes: 6dd112b9f85e ("iio: adc: mxs-lradc: Add support for ADC driver")
> Signed-off-by: Jiakai Luo <jkluo@hust.edu.cn>
> Reviewed-by: Dongliang Mu <dzm91@hust.edu.cn>

Applied to the fixes-togreg branch of iio.git and marked for backporting to
stable. At this stage I'll probably wait until around rc1 to send out a pull
request with this in.  

Thanks,

Jonathan

> ---
> The issue is found by static analysis and remains untested.
> ---
>  drivers/iio/adc/mxs-lradc-adc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/mxs-lradc-adc.c b/drivers/iio/adc/mxs-lradc-adc.c
> index bca79a93cbe4..85882509b7d9 100644
> --- a/drivers/iio/adc/mxs-lradc-adc.c
> +++ b/drivers/iio/adc/mxs-lradc-adc.c
> @@ -757,13 +757,13 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
>  
>  	ret = mxs_lradc_adc_trigger_init(iio);
>  	if (ret)
> -		goto err_trig;
> +		return ret;
>  
>  	ret = iio_triggered_buffer_setup(iio, &iio_pollfunc_store_time,
>  					 &mxs_lradc_adc_trigger_handler,
>  					 &mxs_lradc_adc_buffer_ops);
>  	if (ret)
> -		return ret;
> +		goto err_trig;
>  
>  	adc->vref_mv = mxs_lradc_adc_vref_mv[lradc->soc];
>  
> @@ -801,9 +801,9 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
>  
>  err_dev:
>  	mxs_lradc_adc_hw_stop(adc);
> -	mxs_lradc_adc_trigger_remove(iio);
> -err_trig:
>  	iio_triggered_buffer_cleanup(iio);
> +err_trig:
> +	mxs_lradc_adc_trigger_remove(iio);
>  	return ret;
>  }
>  
> @@ -814,8 +814,8 @@ static int mxs_lradc_adc_remove(struct platform_device *pdev)
>  
>  	iio_device_unregister(iio);
>  	mxs_lradc_adc_hw_stop(adc);
> -	mxs_lradc_adc_trigger_remove(iio);
>  	iio_triggered_buffer_cleanup(iio);
> +	mxs_lradc_adc_trigger_remove(iio);
> 
>  	return 0;
>  }


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] iio: adc: mxs-lradc: fix the order of two cleanup operations
  2023-04-17  2:47 [PATCH v2] iio: adc: mxs-lradc: fix the order of two cleanup operations Jiakai Luo
@ 2023-04-23 10:50 ` Jonathan Cameron
  2023-04-23 12:01   ` Dongliang Mu
  0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2023-04-23 10:50 UTC (permalink / raw)
  To: Jiakai Luo
  Cc: Lars-Peter Clausen, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Ksenija Stanojevic, Lee Jones, Marek Vasut,
	hust-os-kernel-patches, linux-iio, linux-arm-kernel, linux-kernel

On Sun, 16 Apr 2023 19:47:45 -0700
Jiakai Luo <jkluo@hust.edu.cn> wrote:

> Smatch reports:
> drivers/iio/adc/mxs-lradc-adc.c:766 mxs_lradc_adc_probe() warn:
> missing unwind goto?
> 
> the order of three init operation:
> 1.mxs_lradc_adc_trigger_init
> 2.iio_triggered_buffer_setup
> 3.mxs_lradc_adc_hw_init
> 
> thus, the order of three cleanup operation should be:
> 1.mxs_lradc_adc_hw_stop
> 2.iio_triggered_buffer_cleanup
> 3.mxs_lradc_adc_trigger_remove
> 
> we exchange the order of two cleanup operations,
> introducing the following differences:
> 1.if mxs_lradc_adc_trigger_init fails, returns directly;
> 2.if trigger_init succeeds but iio_triggered_buffer_setup fails,
> goto err_trig and remove the trigger.
> 
> In addition, we also reorder the unwind that goes on in the
> remove() callback to match the new ordering.
> 
> Fixes: 6dd112b9f85e ("iio: adc: mxs-lradc: Add support for ADC driver")
> Signed-off-by: Jiakai Luo <jkluo@hust.edu.cn>
> Reviewed-by: Dongliang Mu <dzm91@hust.edu.cn>
If resending please state why.  I'm guessing on this occasion it was because
you realised a fresh thread is expected for a new patch.

Also, even if you are just amending the patch description, please increase
the version number so that we can be sure we are looking at latest version.

I already picked it from the earlier posting and this appears unchanged
so all's well that ends well!

Jonathan

> ---
> The issue is found by static analysis and remains untested.
> ---
>  drivers/iio/adc/mxs-lradc-adc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/mxs-lradc-adc.c b/drivers/iio/adc/mxs-lradc-adc.c
> index bca79a93cbe4..85882509b7d9 100644
> --- a/drivers/iio/adc/mxs-lradc-adc.c
> +++ b/drivers/iio/adc/mxs-lradc-adc.c
> @@ -757,13 +757,13 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
>  
>  	ret = mxs_lradc_adc_trigger_init(iio);
>  	if (ret)
> -		goto err_trig;
> +		return ret;
>  
>  	ret = iio_triggered_buffer_setup(iio, &iio_pollfunc_store_time,
>  					 &mxs_lradc_adc_trigger_handler,
>  					 &mxs_lradc_adc_buffer_ops);
>  	if (ret)
> -		return ret;
> +		goto err_trig;
>  
>  	adc->vref_mv = mxs_lradc_adc_vref_mv[lradc->soc];
>  
> @@ -801,9 +801,9 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
>  
>  err_dev:
>  	mxs_lradc_adc_hw_stop(adc);
> -	mxs_lradc_adc_trigger_remove(iio);
> -err_trig:
>  	iio_triggered_buffer_cleanup(iio);
> +err_trig:
> +	mxs_lradc_adc_trigger_remove(iio);
>  	return ret;
>  }
>  
> @@ -814,8 +814,8 @@ static int mxs_lradc_adc_remove(struct platform_device *pdev)
>  
>  	iio_device_unregister(iio);
>  	mxs_lradc_adc_hw_stop(adc);
> -	mxs_lradc_adc_trigger_remove(iio);
>  	iio_triggered_buffer_cleanup(iio);
> +	mxs_lradc_adc_trigger_remove(iio);
> 
>  	return 0;
>  }


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] iio: adc: mxs-lradc: fix the order of two cleanup operations
  2023-04-23 10:50 ` Jonathan Cameron
@ 2023-04-23 12:01   ` Dongliang Mu
  0 siblings, 0 replies; 8+ messages in thread
From: Dongliang Mu @ 2023-04-23 12:01 UTC (permalink / raw)
  To: Jonathan Cameron, Jiakai Luo
  Cc: Lars-Peter Clausen, Shawn Guo, Sascha Hauer,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
	Ksenija Stanojevic, Lee Jones, Marek Vasut,
	hust-os-kernel-patches, linux-iio, linux-arm-kernel, linux-kernel


On 2023/4/23 18:50, Jonathan Cameron wrote:
> On Sun, 16 Apr 2023 19:47:45 -0700
> Jiakai Luo <jkluo@hust.edu.cn> wrote:
>
>> Smatch reports:
>> drivers/iio/adc/mxs-lradc-adc.c:766 mxs_lradc_adc_probe() warn:
>> missing unwind goto?
>>
>> the order of three init operation:
>> 1.mxs_lradc_adc_trigger_init
>> 2.iio_triggered_buffer_setup
>> 3.mxs_lradc_adc_hw_init
>>
>> thus, the order of three cleanup operation should be:
>> 1.mxs_lradc_adc_hw_stop
>> 2.iio_triggered_buffer_cleanup
>> 3.mxs_lradc_adc_trigger_remove
>>
>> we exchange the order of two cleanup operations,
>> introducing the following differences:
>> 1.if mxs_lradc_adc_trigger_init fails, returns directly;
>> 2.if trigger_init succeeds but iio_triggered_buffer_setup fails,
>> goto err_trig and remove the trigger.
>>
>> In addition, we also reorder the unwind that goes on in the
>> remove() callback to match the new ordering.
>>
>> Fixes: 6dd112b9f85e ("iio: adc: mxs-lradc: Add support for ADC driver")
>> Signed-off-by: Jiakai Luo <jkluo@hust.edu.cn>
>> Reviewed-by: Dongliang Mu <dzm91@hust.edu.cn>
> If resending please state why.  I'm guessing on this occasion it was because
> you realised a fresh thread is expected for a new patch.
>
> Also, even if you are just amending the patch description, please increase
> the version number so that we can be sure we are looking at latest version.
>
> I already picked it from the earlier posting and this appears unchanged
> so all's well that ends well!
>
> Jonathan

Hi JC,

Jiakai originally would like to send a reminder about his patch, but 
mistakenly sent a v2 version.

Please ignore this v2 version since this version is the same with v1 
version.

Sorry for the mistake.

>
>> ---
>> The issue is found by static analysis and remains untested.
>> ---
>>   drivers/iio/adc/mxs-lradc-adc.c | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/adc/mxs-lradc-adc.c b/drivers/iio/adc/mxs-lradc-adc.c
>> index bca79a93cbe4..85882509b7d9 100644
>> --- a/drivers/iio/adc/mxs-lradc-adc.c
>> +++ b/drivers/iio/adc/mxs-lradc-adc.c
>> @@ -757,13 +757,13 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
>>   
>>   	ret = mxs_lradc_adc_trigger_init(iio);
>>   	if (ret)
>> -		goto err_trig;
>> +		return ret;
>>   
>>   	ret = iio_triggered_buffer_setup(iio, &iio_pollfunc_store_time,
>>   					 &mxs_lradc_adc_trigger_handler,
>>   					 &mxs_lradc_adc_buffer_ops);
>>   	if (ret)
>> -		return ret;
>> +		goto err_trig;
>>   
>>   	adc->vref_mv = mxs_lradc_adc_vref_mv[lradc->soc];
>>   
>> @@ -801,9 +801,9 @@ static int mxs_lradc_adc_probe(struct platform_device *pdev)
>>   
>>   err_dev:
>>   	mxs_lradc_adc_hw_stop(adc);
>> -	mxs_lradc_adc_trigger_remove(iio);
>> -err_trig:
>>   	iio_triggered_buffer_cleanup(iio);
>> +err_trig:
>> +	mxs_lradc_adc_trigger_remove(iio);
>>   	return ret;
>>   }
>>   
>> @@ -814,8 +814,8 @@ static int mxs_lradc_adc_remove(struct platform_device *pdev)
>>   
>>   	iio_device_unregister(iio);
>>   	mxs_lradc_adc_hw_stop(adc);
>> -	mxs_lradc_adc_trigger_remove(iio);
>>   	iio_triggered_buffer_cleanup(iio);
>> +	mxs_lradc_adc_trigger_remove(iio);
>>
>>   	return 0;
>>   }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-04-23 12:04 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-17  2:47 [PATCH v2] iio: adc: mxs-lradc: fix the order of two cleanup operations Jiakai Luo
2023-04-23 10:50 ` Jonathan Cameron
2023-04-23 12:01   ` Dongliang Mu
  -- strict thread matches above, loose matches on Subject: below --
2023-04-16  7:21 Jiakai Luo
2023-04-16 12:28 ` Jonathan Cameron
2023-04-16 12:29 ` Jonathan Cameron
2023-04-22 13:34   ` Jiakai Luo
2023-04-23 10:40     ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox