Re: [PATCH v2] iio: adc: mxs-lradc: fix the order of two cleanup operations

From: Jonathan Cameron <jic23@kernel.org>
To: Jiakai Luo <jkluo@hust.edu.cn>
Cc: Lars-Peter Clausen <lars@metafoo.de>,
	Shawn Guo <shawnguo@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	Ksenija Stanojevic <ksenija.stanojevic@gmail.com>,
	Lee Jones <lee.jones@linaro.org>, Marek Vasut <marex@denx.de>,
	linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] iio: adc: mxs-lradc: fix the order of two cleanup operations
Date: Sun, 16 Apr 2023 13:28:16 +0100	[thread overview]
Message-ID: <20230416132816.70f4814e@jic23-huawei> (raw)
In-Reply-To: <20230416072157.57388-1-jkluo@hust.edu.cn>

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