linux-kernel - Re: [PATCH v7 30/34] i2c: tegra: Clean up variable names

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: <49498b9c-3b75-ad97-1859-5d6442b27b0c@gmail.com>
Date:   Thu, 17 Sep 2020 18:43:28 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Thierry Reding <thierry.reding@...il.com>
Cc:     Jonathan Hunter <jonathanh@...dia.com>,
        Laxman Dewangan <ldewangan@...dia.com>,
        Wolfram Sang <wsa@...-dreams.de>,
        Michał Mirosław <mirq-linux@...e.qmqm.pl>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        linux-i2c@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 30/34] i2c: tegra: Clean up variable names

17.09.2020 15:21, Thierry Reding пишет:
> On Wed, Sep 09, 2020 at 01:40:02AM +0300, Dmitry Osipenko wrote:
>> Rename "ret" variables to "err" in order to make code a bit more
>> expressive, emphasizing that the returned value is an error code.
>> Same vice versa, where appropriate.
>>
>> Rename variable "reg" to "val" in order to better reflect the actual
>> usage of the variable in the code and to make naming consistent with
>> the rest of the code.
>>
>> Use briefer names for a few members of the tegra_i2c_dev structure in
>> order to improve readability of the code.
>>
>> All dev/&pdev->dev are replaced with i2c_dev->dev in order to have uniform
>> code style across the driver.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
>> ---
>>  drivers/i2c/busses/i2c-tegra.c | 173 ++++++++++++++++-----------------
>>  1 file changed, 86 insertions(+), 87 deletions(-)
> 
> That's indeed a nice improvement. One thing did spring out at me,
> though.
> 
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> [...]
>> @@ -1831,20 +1830,20 @@ static int __maybe_unused tegra_i2c_runtime_suspend(struct device *dev)
>>  
>>  	clk_bulk_disable(i2c_dev->nclocks, i2c_dev->clocks);
>>  
>> -	return pinctrl_pm_select_idle_state(i2c_dev->dev);
>> +	return pinctrl_pm_select_idle_state(dev);
>>  }
>>  
>>  static int __maybe_unused tegra_i2c_suspend(struct device *dev)
>>  {
>>  	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
>> -	int err = 0;
>> +	int ret = 0;
>>  
>>  	i2c_mark_adapter_suspended(&i2c_dev->adapter);
>>  
>>  	if (!pm_runtime_status_suspended(dev))
>> -		err = tegra_i2c_runtime_suspend(dev);
>> +		ret = tegra_i2c_runtime_suspend(dev);
>>  
>> -	return err;
>> +	return ret;
>>  }
> 
> Isn't this exactly the opposite of what the commit message says (and the
> rest of the patch does)?

This change makes it to be consistent with the rest of the code. You may
notice that "Factor out hardware initialization into separate function"
made a similar change.

The reason I'm doing this is that the "err" suggests that code returns a
error failure code, while it could be a success too and you don't know
for sure by looking only at the part of code. Hence it's cleaner to use
"err" when error code is returned.

It is possible (and maybe even better) to rewrite this function as:

static int __maybe_unused tegra_i2c_suspend(struct device *dev)
{
	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);

	i2c_mark_adapter_suspended(&i2c_dev->adapter);

	if (!pm_runtime_status_suspended(dev)) {
		int err = tegra_i2c_runtime_suspend(dev);
		if (err)
			return err;
	}

	return 0;
}