linux-kernel - Re: [PATCH 3/6] iio: imx7d_adc: Use devm_iio_device

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]

Message-ID: <20190407120735.6fe78a5b@archlinux>
Date:   Sun, 7 Apr 2019 12:07:35 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Andrey Smirnov <andrew.smirnov@...il.com>
Cc:     linux-iio@...r.kernel.org, Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Chris Healy <cphealy@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/6] iio: imx7d_adc: Use devm_iio_device_register()

On Wed,  3 Apr 2019 00:03:22 -0700
Andrey Smirnov <andrew.smirnov@...il.com> wrote:

> Use devm_iio_device_register() and drop explicit call to
> iio_device_unregister().
> 
> Signed-off-by: Andrey Smirnov <andrew.smirnov@...il.com>
> Cc: Jonathan Cameron <jic23@...nel.org>
> Cc: Hartmut Knaack <knaack.h@....de>
> Cc: Lars-Peter Clausen <lars@...afoo.de>
> Cc: Peter Meerwald-Stadler <pmeerw@...erw.net>
> Cc: Chris Healy <cphealy@...il.com>
> Cc: linux-iio@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
No to this one.  The thing to think about is the resulting order
of the unwinding that happens in remove.

Do take a look at the code flow, but in short what happens is:

1. driver.remove()
2. Devm release functions run in the opposite order to they were
   called during setup.

The upshot of the change you just made here is that we turn the power
off to the device before we remove the userspace interfaces, giving
potentially interesting failures..

There are two options to avoid this:

1. Make everything use devm_ calls (often using devm_add_action_or_reset
for the ones that don't have their own versions).

2. Stop using devm managed functions at the first non devm_ call that needs
unwinding.  From that point onwards in probe / remove you have to do everything
manually.

Jonathan

> ---
>  drivers/iio/adc/imx7d_adc.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c
> index 72cfe9834bcb..9a46838ec7cf 100644
> --- a/drivers/iio/adc/imx7d_adc.c
> +++ b/drivers/iio/adc/imx7d_adc.c
> @@ -517,7 +517,7 @@ static int imx7d_adc_probe(struct platform_device *pdev)
>  	imx7d_adc_feature_config(info);
>  	imx7d_adc_hw_init(info);
>  
> -	ret = iio_device_register(indio_dev);
> +	ret = devm_iio_device_register(dev, indio_dev);
>  	if (ret) {
>  		imx7d_adc_power_down(info);
>  		dev_err(&pdev->dev, "Couldn't register the device.\n");
> @@ -539,8 +539,6 @@ static int imx7d_adc_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct imx7d_adc *info = iio_priv(indio_dev);
>  
> -	iio_device_unregister(indio_dev);
> -
>  	imx7d_adc_power_down(info);
>  
>  	clk_disable_unprepare(info->clk);