Re: [PATCH 1/1] i2c: tegra: fix error handling in tegra_i2c_xfer()

[Jon Hunter <jonathanh@nvidia.com>]

On 06/05/2026 20:53, Saurav Sachidanand wrote:
> Fix two bugs in the SW mutex path introduced by commit 6077cfd716fb
> ("i2c: tegra: Add support for SW mutex register"):
> 
> 1. If tegra_i2c_mutex_lock() fails, the function returns without calling
>     pm_runtime_put(), leaking the runtime PM reference acquired by the
>     preceding pm_runtime_get_sync(). Add the missing pm_runtime_put()
>     before returning.
> 
> 2. tegra_i2c_mutex_unlock() unconditionally overwrites ret, which may
>     already hold a transfer error from tegra_i2c_xfer_msg(). If the
>     transfer failed but the unlock succeeds, the error is silently lost
>     and the function incorrectly reports success. Use a separate variable
>     for the unlock return value and preserve error priority:
>     transfer error > unlock error > message count.
> 
> Fixes: 6077cfd716fb ("i2c: tegra: Add support for SW mutex register")

Although this is fixing issues associated with one patch, this is fixing 
two different issues and so I think that this should be split into 2 
patches.

> Signed-off-by: Saurav Sachidanand <sauravsc@amazon.com>
> ---
>   drivers/i2c/busses/i2c-tegra.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 9fd5ade774a0b..704942d10d69d 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -1656,7 +1656,7 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
>   			  int num)
>   {
>   	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
> -	int i, ret;
> +	int i, ret, ret2;
>   
>   	ret = pm_runtime_get_sync(i2c_dev->dev);
>   	if (ret < 0) {
> @@ -1666,8 +1666,10 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
>   	}
>   
>   	ret = tegra_i2c_mutex_lock(i2c_dev);
> -	if (ret)
> +	if (ret) {
> +		pm_runtime_put(i2c_dev->dev);
>   		return ret;
> +	}
>   
>   	for (i = 0; i < num; i++) {
>   		enum msg_end_type end_type = MSG_END_STOP;
> @@ -1698,10 +1700,10 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
>   			break;
>   	}
>   
> -	ret = tegra_i2c_mutex_unlock(i2c_dev);
> +	ret2 = tegra_i2c_mutex_unlock(i2c_dev);
>   	pm_runtime_put(i2c_dev->dev);
>   
> -	return ret ?: i;
> +	return ret ?: ret2 ?: i;

I can't say I am a fan of this. I wonder if we would be better off 
removing the return value from tegra_i2c_mutex_unlock() and just WARN if 
this ever happens? If the unlock did fail, the actual I2C message may 
still have been sent and the next time we try to send a message I assume 
that the lock would fail anyway and we would not be able to send further 
messages.

Jon

-- 
nvpublic