linux-kernel - Re: [PATCH v6 1/4] i2c: tegra: disable clock before returning error

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 for Android: free password hash cracker in your pocket

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

Date:	Mon, 25 Apr 2016 13:35:17 +0100
From:	Jon Hunter <jonathanh@...dia.com>
To:	Shardar Shariff Md <smohammed@...dia.com>, <ldewangan@...dia.com>,
	<wsa@...-dreams.de>, <swarren@...dotorg.org>,
	<thierry.reding@...il.com>, <gnurou@...il.com>,
	<linux-i2c@...r.kernel.org>, <linux-tegra@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 1/4] i2c: tegra: disable clock before returning error


On 25/04/16 12:57, Shardar Shariff Md wrote:
> Disable clock before returning error in tegra_i2c_init()

May be explain why we should do this? It looks like currently we will
leave the clock on in the case of an error and so we may never turn it
off again as now we have an unbalanced enable/disable, etc.

> Signed-off-by: Shardar Shariff Md <smohammed@...dia.com>
> 
> ---
> Changes in v5:
> - Move current patch as separate patch
> 
> Changes in v6:
> - Remove unnecessary line
> ---
>  drivers/i2c/busses/i2c-tegra.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index d764d64..445398c3 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -483,19 +483,20 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>  			if (time_after(jiffies, timeout)) {
>  				dev_warn(i2c_dev->dev,
>  					"timeout waiting for config load\n");
> -				return -ETIMEDOUT;
> +				err = -ETIMEDOUT;
> +				goto err;
>  			}
>  			msleep(1);
>  		}
>  	}
>  
> -	tegra_i2c_clock_disable(i2c_dev);
> -
>  	if (i2c_dev->irq_disabled) {
>  		i2c_dev->irq_disabled = 0;
>  		enable_irq(i2c_dev->irq);
>  	}
>  
> +err:
> +	tegra_i2c_clock_disable(i2c_dev);

Thierry, Stephen, thinking about this some more I wonder if we should
not change the order here, in case of a potential race between an
interrupt and disabling the clock. May be it is safer to disable the
clock first? However, this is in the init and so may be that is not likely?

Cheers
Jon