linux-kernel - Re: [PATCH v1 1/1] iio: adc: meson: fix core clock enable/disable moment

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: <20230715182121.29f83d76@jic23-huawei>
Date:   Sat, 15 Jul 2023 18:21:21 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     George Stark <gnstark@...rdevices.ru>
Cc:     <lars@...afoo.de>, <neil.armstrong@...aro.org>,
        <khilman@...libre.com>, <jbrunet@...libre.com>,
        <martin.blumenstingl@...glemail.com>,
        <andriy.shevchenko@...ux.intel.com>, <nuno.sa@...log.com>,
        <linux-iio@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-amlogic@...ts.infradead.org>, <kernel@...rdevices.ru>
Subject: Re: [PATCH v1 1/1] iio: adc: meson: fix core clock enable/disable
 moment

On Fri, 14 Jul 2023 22:03:15 +0300
George Stark <gnstark@...rdevices.ru> wrote:

> Enable core clock at probe stage and disable it at remove stage.
> Core clock is responsible for turning on/off the entire SoC module so
> it should be on before the first module register is touched and be off
> at very last moment.
> 
> Signed-off-by: George Stark <gnstark@...rdevices.ru>

This sounds to me like this should perhaps have a fixes tag?

Given where it is in the new sequence you can also use
devm_clk_get_enabled() though that makes a potential backport of
the fix trickier...

I'd go with it anyway as it will make this change quite a bit simpler.

More comments inline

Jonathan

> ---
>  drivers/iio/adc/meson_saradc.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> index fe066c9fab83..5a5bb5cc1320 100644
> --- a/drivers/iio/adc/meson_saradc.c
> +++ b/drivers/iio/adc/meson_saradc.c
> @@ -1055,12 +1055,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev)
>  		goto err_vref;
>  	}
>  
> -	ret = clk_prepare_enable(priv->core_clk);
> -	if (ret) {
> -		dev_err(dev, "failed to enable core clk\n");
> -		goto err_core_clk;
> -	}
> -
>  	regval = FIELD_PREP(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, 1);
>  	regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
>  			   MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval);
> @@ -1087,8 +1081,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev)
>  	regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG3,
>  			   MESON_SAR_ADC_REG3_ADC_EN, 0);
>  	meson_sar_adc_set_bandgap(indio_dev, false);
> -	clk_disable_unprepare(priv->core_clk);
> -err_core_clk:
>  	regulator_disable(priv->vref);
>  err_vref:
>  	meson_sar_adc_unlock(indio_dev);
> @@ -1116,8 +1108,6 @@ static void meson_sar_adc_hw_disable(struct iio_dev *indio_dev)
>  
>  	meson_sar_adc_set_bandgap(indio_dev, false);
>  
> -	clk_disable_unprepare(priv->core_clk);
> -
>  	regulator_disable(priv->vref);
>  
>  	if (!ret)
> @@ -1420,6 +1410,12 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>  			ARRAY_SIZE(meson_sar_adc_iio_channels);
>  	}
>  
> +	ret = clk_prepare_enable(priv->core_clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable core clk\n");
> +		goto err;

If clk_prepare_enable() failed, then you shoudl not call clk_disable_unprepare()

> +	}
> +
>  	ret = meson_sar_adc_init(indio_dev);
>  	if (ret)
>  		goto err;
> @@ -1445,17 +1441,21 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>  err_hw:
>  	meson_sar_adc_hw_disable(indio_dev);
>  err:
> +	clk_disable_unprepare(priv->core_clk);

Nothing to do with your patch, but this driver previously did some odd mixing
of direct returns and going to an empty label which definitely doesn't help
make this clear to follow.

>  	return ret;
>  }
>  
>  static int meson_sar_adc_remove(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
>  
>  	iio_device_unregister(indio_dev);
>  
>  	meson_sar_adc_hw_disable(indio_dev);
>  
> +	clk_disable_unprepare(priv->core_clk);
> +
>  	return 0;
>  }
>