linux-kernel - Re: [PATCH v1 3/3] i2c: tegra: Fix suspending in active runtime PM state

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:   Fri, 13 Dec 2019 14:47:46 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Jonathan Hunter <jonathanh@...dia.com>,
        Laxman Dewangan <ldewangan@...dia.com>,
        Wolfram Sang <wsa@...-dreams.de>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>, linux-i2c@...r.kernel.org,
        linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 3/3] i2c: tegra: Fix suspending in active runtime PM
 state

On Fri, Dec 13, 2019 at 02:34:28AM +0300, Dmitry Osipenko wrote:
> I noticed that sometime I2C clock is kept enabled during suspend-resume.
> This happens because runtime PM defers dynamic suspension and thus it may
> happen that runtime PM is in active state when system enters into suspend.
> In particular I2C controller that is used for CPU's DVFS is often kept ON
> during suspend because CPU's voltage scaling happens quite often.
> 
> Note: we marked runtime PM as IRQ-safe during the driver's probe in the
> "Support atomic transfers" patch, thus it's okay to enforce runtime PM
> suspend/resume in the NOIRQ phase which is used for the system-level
> suspend/resume of the driver.
> 
> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

I've recently discussed this with Rafael in the context of runtime PM
support in the Tegra DRM driver and my understanding is that you're not
supposed to force runtime PM suspension like this.

I had meant to send out an alternative patch to fix this, which I've
done now:

	http://patchwork.ozlabs.org/patch/1209148/

That's more in line with what Rafael and I had discussed in the other
thread and should address the issue that you're seeing as well.

Thierry

> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index b3ecdd87e91f..d309a314f4d6 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -1790,9 +1790,14 @@ static int tegra_i2c_remove(struct platform_device *pdev)
>  static int __maybe_unused tegra_i2c_suspend(struct device *dev)
>  {
>  	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
> +	int err;
>  
>  	i2c_mark_adapter_suspended(&i2c_dev->adapter);
>  
> +	err = pm_runtime_force_suspend(dev);
> +	if (err < 0)
> +		return err;
> +
>  	return 0;
>  }
>  
> @@ -1813,6 +1818,10 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)
>  	if (err)
>  		return err;
>  
> +	err = pm_runtime_force_resume(dev);
> +	if (err < 0)
> +		return err;
> +
>  	i2c_mark_adapter_resumed(&i2c_dev->adapter);
>  
>  	return 0;
> -- 
> 2.24.0
> 

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)