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

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

[Saurav Sachidanand]

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")
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;
 }
 
 static int tegra_i2c_xfer_atomic(struct i2c_adapter *adap,
-- 
2.47.3

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

[Jon Hunter]

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

[PATCH v2 0/2] i2c: tegra: fix error handling in tegra_i2c_xfer()

[Saurav Sachidanand]

Fix two bugs in the SW mutex path introduced by commit 6077cfd716fb
("i2c: tegra: Add support for SW mutex register"):

1/2: Fix pm_runtime reference leak when tegra_i2c_mutex_lock() fails.
2/2: Make tegra_i2c_mutex_unlock() return void with WARN to avoid
     silently losing transfer errors.

Changes since v1:
- Split into two patches (Jon)
- Make tegra_i2c_mutex_unlock() return void with WARN instead of
  propagating the error value (Jon)

v1: https://lore.kernel.org/all/20260506195319.44810-1-sauravsc@amazon.com/

Saurav Sachidanand (2):
  i2c: tegra: fix pm_runtime leak on mutex_lock failure
  i2c: tegra: make tegra_i2c_mutex_unlock() return void

 drivers/i2c/busses/i2c-tegra.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

-- 
2.47.3

[PATCH v2 1/2] i2c: tegra: fix pm_runtime leak on mutex_lock failure

[Saurav Sachidanand]

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(). This prevents the device from ever
entering runtime suspend.

Add the missing pm_runtime_put() before returning on lock failure.

Fixes: 6077cfd716fb ("i2c: tegra: Add support for SW mutex register")
Signed-off-by: Saurav Sachidanand <sauravsc@amazon.com>
---
 drivers/i2c/busses/i2c-tegra.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 9fd5ade774a0b..c24b8de0a9c7b 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -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;
-- 
2.47.3

[PATCH v2 2/2] i2c: tegra: make tegra_i2c_mutex_unlock() return void

[Saurav Sachidanand]

tegra_i2c_mutex_unlock() returning an error that overwrites the transfer
result causes silent loss of I2C transfer errors. If the transfer failed
but the unlock succeeded, the error was lost and the function incorrectly
reported success.

Rather than propagating the unlock error (which is not actionable by the
caller - the I2C message may have been sent regardless), convert the
function to return void and WARN on the unexpected condition. If the
unlock fails, subsequent lock attempts will fail anyway, making the error
visible on the next transfer.

Fixes: 6077cfd716fb ("i2c: tegra: Add support for SW mutex register")
Signed-off-by: Saurav Sachidanand <sauravsc@amazon.com>
---
 drivers/i2c/busses/i2c-tegra.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index c24b8de0a9c7b..479a1667e88d5 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -589,25 +589,22 @@ static int tegra_i2c_mutex_lock(struct tegra_i2c_dev *i2c_dev)
 	return ret;
 }
 
-static int tegra_i2c_mutex_unlock(struct tegra_i2c_dev *i2c_dev)
+static void tegra_i2c_mutex_unlock(struct tegra_i2c_dev *i2c_dev)
 {
 	unsigned int reg = i2c_dev->hw->regs->sw_mutex;
 	u32 val, id;
 
 	if (!i2c_dev->hw->has_mutex)
-		return 0;
+		return;
 
 	val = readl(i2c_dev->base + reg);
 
 	id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
-	if (id && id != I2C_SW_MUTEX_ID_CCPLEX) {
-		dev_warn(i2c_dev->dev, "unable to unlock mutex, mutex is owned by: %u\n", id);
-		return -EPERM;
-	}
+	if (WARN(id && id != I2C_SW_MUTEX_ID_CCPLEX,
+		 "unable to unlock mutex, mutex is owned by: %u\n", id))
+		return;
 
 	writel(0, i2c_dev->base + reg);
-
-	return 0;
 }
 
 static void tegra_i2c_mask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
@@ -1700,7 +1697,7 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 			break;
 	}
 
-	ret = tegra_i2c_mutex_unlock(i2c_dev);
+	tegra_i2c_mutex_unlock(i2c_dev);
 	pm_runtime_put(i2c_dev->dev);
 
 	return ret ?: i;
-- 
2.47.3

Re: [PATCH v2 2/2] i2c: tegra: make tegra_i2c_mutex_unlock() return void

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]

On Thu, May 07, 2026 at 10:11:45PM +0000, Saurav Sachidanand wrote:
> tegra_i2c_mutex_unlock() returning an error that overwrites the transfer
> result causes silent loss of I2C transfer errors. If the transfer failed
> but the unlock succeeded, the error was lost and the function incorrectly
> reported success.
> 
> Rather than propagating the unlock error (which is not actionable by the
> caller - the I2C message may have been sent regardless), convert the
> function to return void and WARN on the unexpected condition. If the
> unlock fails, subsequent lock attempts will fail anyway, making the error
> visible on the next transfer.

Technically I don't think it's guaranteed that a subsequent lock attempt
will fail. For example, if the SW mutex was somehow held by some other
owner while trying to unlock, by the time we try to lock later on that
owner might have released the SW mutex again.

Obviously if we've managed to lock the SW mutex but fail to unlock
because somebody else was holding it, it means that the other party did
not respect the SW mutex protocol, in which case anything goes.

Anyway, this looks good to me, so:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] i2c: tegra: fix pm_runtime leak on mutex_lock failure

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

On Thu, May 07, 2026 at 10:11:44PM +0000, Saurav Sachidanand wrote:
> 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(). This prevents the device from ever
> entering runtime suspend.
> 
> Add the missing pm_runtime_put() before returning on lock failure.
> 
> Fixes: 6077cfd716fb ("i2c: tegra: Add support for SW mutex register")
> Signed-off-by: Saurav Sachidanand <sauravsc@amazon.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] i2c: tegra: fix pm_runtime leak on mutex_lock failure

[Jon Hunter]

On 08/05/2026 11:24, Thierry Reding wrote:
> On Thu, May 07, 2026 at 10:11:44PM +0000, Saurav Sachidanand wrote:
>> 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(). This prevents the device from ever
>> entering runtime suspend.
>>
>> Add the missing pm_runtime_put() before returning on lock failure.
>>
>> Fixes: 6077cfd716fb ("i2c: tegra: Add support for SW mutex register")
>> Signed-off-by: Saurav Sachidanand <sauravsc@amazon.com>
>> ---
>>   drivers/i2c/busses/i2c-tegra.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Acked-by: Thierry Reding <treding@nvidia.com>


Reviewed-by: Jon Hunter <jonathanh@nvidia.com>

Thanks
Jon

-- 
nvpublic

Re: [PATCH v2 2/2] i2c: tegra: make tegra_i2c_mutex_unlock() return void

[Jon Hunter]

On 08/05/2026 11:24, Thierry Reding wrote:
> On Thu, May 07, 2026 at 10:11:45PM +0000, Saurav Sachidanand wrote:
>> tegra_i2c_mutex_unlock() returning an error that overwrites the transfer
>> result causes silent loss of I2C transfer errors. If the transfer failed
>> but the unlock succeeded, the error was lost and the function incorrectly
>> reported success.
>>
>> Rather than propagating the unlock error (which is not actionable by the
>> caller - the I2C message may have been sent regardless), convert the
>> function to return void and WARN on the unexpected condition. If the
>> unlock fails, subsequent lock attempts will fail anyway, making the error
>> visible on the next transfer.
> 
> Technically I don't think it's guaranteed that a subsequent lock attempt
> will fail. For example, if the SW mutex was somehow held by some other
> owner while trying to unlock, by the time we try to lock later on that
> owner might have released the SW mutex again.
> 
> Obviously if we've managed to lock the SW mutex but fail to unlock
> because somebody else was holding it, it means that the other party did
> not respect the SW mutex protocol, in which case anything goes.
> 
> Anyway, this looks good to me, so:
> 
> Acked-by: Thierry Reding <treding@nvidia.com>


Reviewed-by: Jon Hunter <jonathanh@nvidia.com>

Thanks
Jon

-- 
nvpublic