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 <[email protected]>
Reviewed-by: Dongliang Mu <[email protected]>
---
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
On Sun, 16 Apr 2023 00:21:57 -0700
Jiakai Luo <[email protected]> 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 <[email protected]>
> Reviewed-by: Dongliang Mu <[email protected]>
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;
> }
>
On Sun, 16 Apr 2023 00:21:57 -0700
Jiakai Luo <[email protected]> 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 <[email protected]>
> Reviewed-by: Dongliang Mu <[email protected]>
> ---
> 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;
> }
>
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 <[email protected]>
Reviewed-by: Dongliang Mu <[email protected]>
---
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
On Sat, 22 Apr 2023 06:34:06 -0700
Jiakai Luo <[email protected]> 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 <[email protected]>
> Reviewed-by: Dongliang Mu <[email protected]>
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;
> }