linux-kernel - Re: [PATCH 3/5] iio: adc: xilinx: use a devres action to disable and unprepare the clock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 29 Oct 2020 15:38:38 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Bartosz Golaszewski <brgl@...ev.pl>
Cc:     Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Michal Simek <michal.simek@...inx.com>,
        linux-iio@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org,
        Bartosz Golaszewski <bgolaszewski@...libre.com>
Subject: Re: [PATCH 3/5] iio: adc: xilinx: use a devres action to disable
 and unprepare the clock

On Mon, 26 Oct 2020 14:36:07 +0100
Bartosz Golaszewski <brgl@...ev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@...libre.com>
> 
> In order to simplify resource management and error paths in probe(), add
> a devm action that calls clk_disable_unprepare() at driver detach.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@...libre.com>
> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index b516280ccbd4..e0da6258092c 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -1185,6 +1185,13 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
>  	return 0;
>  }
>  
> +static void xadc_clk_disable_unprepare(void *data)
> +{
> +	struct clk *clk = data;
> +
> +	clk_disable_unprepare(clk);
> +}
> +
>  static int xadc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -1262,6 +1269,11 @@ static int xadc_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_free_samplerate_trigger;
>  

Hi Bartosz,

> +	ret = devm_add_action_or_reset(dev,
> +				       xadc_clk_disable_unprepare, xadc->clk);

Take advantage of flexibility in line length limit and put that on line line.

The moment I see a devm call with a goto in the error path I get suspicious about
ordering.  

So why do we have actions in the error paths of probe that don't turn up
in the remove function?  Fix that before you do this change.

There are some cases in here already that push the limits on this
such as clock allocation but they are easily argued as safe.  This is getting
less obvious.


Jonathan


> +	if (ret)
> +		goto err_free_samplerate_trigger;
> +
>  	/*
>  	 * Make sure not to exceed the maximum samplerate since otherwise the
>  	 * resulting interrupt storm will soft-lock the system.
> @@ -1280,7 +1292,7 @@ static int xadc_probe(struct platform_device *pdev)
>  	ret = request_irq(xadc->irq, xadc->ops->interrupt_handler, 0,
>  			  dev_name(dev), indio_dev);
>  	if (ret)
> -		goto err_clk_disable_unprepare;
> +		goto err_free_samplerate_trigger;
>  
>  	ret = xadc->ops->setup(pdev, indio_dev, xadc->irq);
>  	if (ret)
> @@ -1344,8 +1356,6 @@ static int xadc_probe(struct platform_device *pdev)
>  err_free_irq:
>  	free_irq(xadc->irq, indio_dev);
>  	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
> -err_clk_disable_unprepare:
> -	clk_disable_unprepare(xadc->clk);
>  err_free_samplerate_trigger:
>  	if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
>  		iio_trigger_free(xadc->samplerate_trigger);
> @@ -1372,7 +1382,6 @@ static int xadc_remove(struct platform_device *pdev)
>  	}
>  	free_irq(xadc->irq, indio_dev);
>  	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
> -	clk_disable_unprepare(xadc->clk);
>  
>  	return 0;
>  }