[PATCH 0/2] i2c: designware: Implement atomic transfer suppot

[PATCH 0/2] i2c: designware: Implement atomic transfer suppot

[Jisheng Zhang]

Implement atomic transfer support for designware i2c. This increases
complexity but is necessary for atomic I2C transfers required by some
hardware configurations, e.g., to trigger reboots on an external PMIC chip.

patch1 is bug fix
patch2 is the atomic transfer implementation patch.

Jisheng Zhang (2):
  i2c: designware: Avoid taking clk_prepare mutex in PM callbacks
  i2c: designware: Implement atomic transfer suppot

 drivers/i2c/busses/i2c-designware-common.c | 47 +++++++++----
 drivers/i2c/busses/i2c-designware-core.h   |  5 +-
 drivers/i2c/busses/i2c-designware-master.c | 80 +++++++++++++++++++---
 3 files changed, 106 insertions(+), 26 deletions(-)

-- 
2.50.1

[PATCH 1/2] i2c: designware: Avoid taking clk_prepare mutex in PM callbacks

[Jisheng Zhang]

This is unsafe, as the runtime PM callbacks are called from the PM
workqueue, so this may deadlock when handling an i2c attached clock,
which may already hold the clk_prepare mutex from another context.

Fixes: 1fc2fe204cb9 ("i2c: designware: Add runtime PM hooks")
Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/i2c/busses/i2c-designware-common.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 5b1e8f74c4ac..b4e38bc0f876 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -776,7 +776,8 @@ static int i2c_dw_runtime_suspend(struct device *device)
 		return 0;
 
 	i2c_dw_disable(dev);
-	i2c_dw_prepare_clk(dev, false);
+	clk_disable(dev->clk);
+	clk_disable(dev->pclk);
 
 	return 0;
 }
@@ -794,8 +795,10 @@ static int i2c_dw_runtime_resume(struct device *device)
 {
 	struct dw_i2c_dev *dev = dev_get_drvdata(device);
 
-	if (!dev->shared_with_punit)
-		i2c_dw_prepare_clk(dev, true);
+	if (!dev->shared_with_punit) {
+		clk_enable(dev->pclk);
+		clk_enable(dev->clk);
+	}
 
 	dev->init(dev);
 
-- 
2.50.1

[PATCH 2/2] i2c: designware: Implement atomic transfer suppot

[Jisheng Zhang]

Rework the read and write code paths in the driver to support operation
in atomic contexts. To achieve this, the driver must not rely on IRQs
or perform any scheduling, e.g., via a sleep or schedule routine.

Implement atomic, sleep-free, and IRQ-less operation. This increases
complexity but is necessary for atomic I2C transfers required by some
hardware configurations, e.g., to trigger reboots on an external PMIC chip.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/i2c/busses/i2c-designware-common.c | 38 +++++++---
 drivers/i2c/busses/i2c-designware-core.h   |  5 +-
 drivers/i2c/busses/i2c-designware-master.c | 80 +++++++++++++++++++---
 3 files changed, 100 insertions(+), 23 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index b4e38bc0f876..0b24ac0357ad 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -532,15 +532,23 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev)
 			 * 25us) to ensure the I2C ENABLE bit is already set
 			 * as described in the DesignWare I2C databook.
 			 */
-			fsleep(DIV_ROUND_CLOSEST_ULL(10 * MICRO, t->bus_freq_hz));
+			if (dev->atomic)
+				udelay(DIV_ROUND_CLOSEST_ULL(10 * MICRO, t->bus_freq_hz));
+			else
+				fsleep(DIV_ROUND_CLOSEST_ULL(10 * MICRO, t->bus_freq_hz));
 			/* Set ENABLE bit before setting ABORT */
 			enable |= DW_IC_ENABLE_ENABLE;
 		}
 
 		regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
-		ret = regmap_read_poll_timeout(dev->map, DW_IC_ENABLE, enable,
-					       !(enable & DW_IC_ENABLE_ABORT), 10,
-					       100);
+		if (dev->atomic)
+			ret = regmap_read_poll_timeout_atomic(dev->map, DW_IC_ENABLE, enable,
+							      !(enable & DW_IC_ENABLE_ABORT), 10,
+							      100);
+		else
+			ret = regmap_read_poll_timeout(dev->map, DW_IC_ENABLE, enable,
+						       !(enable & DW_IC_ENABLE_ABORT), 10,
+						       100);
 		if (ret)
 			dev_err(dev->dev, "timeout while trying to abort current transfer\n");
 	}
@@ -560,7 +568,10 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev)
 		 * transfer supported by the driver (for 400kHz this is
 		 * 25us) as described in the DesignWare I2C databook.
 		 */
-		usleep_range(25, 250);
+		if (dev->atomic)
+			udelay(25);
+		else
+			usleep_range(25, 250);
 	} while (timeout--);
 
 	dev_warn(dev->dev, "timeout in disabling adapter\n");
@@ -607,7 +618,7 @@ int i2c_dw_acquire_lock(struct dw_i2c_dev *dev)
 {
 	int ret;
 
-	if (!dev->acquire_lock)
+	if (dev->atomic || !dev->acquire_lock)
 		return 0;
 
 	ret = dev->acquire_lock();
@@ -621,7 +632,7 @@ int i2c_dw_acquire_lock(struct dw_i2c_dev *dev)
 
 void i2c_dw_release_lock(struct dw_i2c_dev *dev)
 {
-	if (dev->release_lock)
+	if (!dev->atomic && dev->release_lock)
 		dev->release_lock();
 }
 
@@ -633,11 +644,18 @@ int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
 	unsigned int status;
 	int ret;
 
-	ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
-				       !(status & DW_IC_STATUS_ACTIVITY),
-				       1100, 20000);
+	if (dev->atomic)
+		ret = regmap_read_poll_timeout_atomic(dev->map, DW_IC_STATUS, status,
+						      !(status & DW_IC_STATUS_ACTIVITY),
+						      1100, 20000);
+	else
+		ret = regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
+					       !(status & DW_IC_STATUS_ACTIVITY),
+					       1100, 20000);
 	if (ret) {
 		dev_warn(dev->dev, "timeout waiting for bus ready\n");
+		if (dev->atomic)
+			return ret;
 
 		i2c_recover_bus(&dev->adapter);
 
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 347843b4f5dd..7384ec5a7ab7 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -299,6 +299,7 @@ struct dw_i2c_dev {
 	void			(*release_lock)(void);
 	int			semaphore_idx;
 	bool			shared_with_punit;
+	bool			atomic;
 	int			(*init)(struct dw_i2c_dev *dev);
 	int			(*set_sda_hold_time)(struct dw_i2c_dev *dev);
 	int			mode;
@@ -365,7 +366,7 @@ static inline void __i2c_dw_disable_nowait(struct dw_i2c_dev *dev)
 static inline void __i2c_dw_write_intr_mask(struct dw_i2c_dev *dev,
 					    unsigned int intr_mask)
 {
-	unsigned int val = dev->flags & ACCESS_POLLING ? 0 : intr_mask;
+	unsigned int val = (dev->atomic || dev->flags & ACCESS_POLLING) ? 0 : intr_mask;
 
 	regmap_write(dev->map, DW_IC_INTR_MASK, val);
 	dev->sw_mask = intr_mask;
@@ -374,7 +375,7 @@ static inline void __i2c_dw_write_intr_mask(struct dw_i2c_dev *dev,
 static inline void __i2c_dw_read_intr_mask(struct dw_i2c_dev *dev,
 					   unsigned int *intr_mask)
 {
-	if (!(dev->flags & ACCESS_POLLING))
+	if (!(dev->flags & ACCESS_POLLING) && !dev->atomic)
 		regmap_read(dev->map, DW_IC_INTR_MASK, intr_mask);
 	else
 		*intr_mask = dev->sw_mask;
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index cbd88ffa5610..333ec9bacae4 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -312,9 +312,14 @@ static bool i2c_dw_is_controller_active(struct dw_i2c_dev *dev)
 	if (!(status & DW_IC_STATUS_MASTER_ACTIVITY))
 		return false;
 
-	return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
-				       !(status & DW_IC_STATUS_MASTER_ACTIVITY),
-				       1100, 20000) != 0;
+	if (dev->atomic)
+		return regmap_read_poll_timeout_atomic(dev->map, DW_IC_STATUS, status,
+						       !(status & DW_IC_STATUS_MASTER_ACTIVITY),
+						       1100, 20000) != 0;
+	else
+		return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
+					       !(status & DW_IC_STATUS_MASTER_ACTIVITY),
+					       1100, 20000) != 0;
 }
 
 static int i2c_dw_check_stopbit(struct dw_i2c_dev *dev)
@@ -654,7 +659,7 @@ static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
 	 *
 	 * The raw version might be useful for debugging purposes.
 	 */
-	if (!(dev->flags & ACCESS_POLLING)) {
+	if (!(dev->flags & ACCESS_POLLING) && !dev->atomic) {
 		regmap_read(dev->map, DW_IC_INTR_STAT, &stat);
 	} else {
 		regmap_read(dev->map, DW_IC_RAW_INTR_STAT, &stat);
@@ -801,11 +806,32 @@ static int i2c_dw_wait_transfer(struct dw_i2c_dev *dev)
 	return ret ? 0 : -ETIMEDOUT;
 }
 
+static int i2c_dw_wait_transfer_atomic(struct dw_i2c_dev *dev)
+{
+	ktime_t timeout = ktime_add_us(ktime_get(), jiffies_to_usecs(dev->adapter.timeout));
+	unsigned int stat;
+	int ret;
+
+	do {
+		ret = try_wait_for_completion(&dev->cmd_complete);
+		if (ret)
+			break;
+
+		stat = i2c_dw_read_clear_intrbits(dev);
+		if (stat)
+			i2c_dw_process_transfer(dev, stat);
+		else
+			udelay(15);
+	} while (ktime_compare(ktime_get(), timeout) < 0);
+
+	return ret ? 0 : -ETIMEDOUT;
+}
+
 /*
  * Prepare controller for a transaction and call i2c_dw_xfer_msg.
  */
 static int
-i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+i2c_dw_xfer_core(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 {
 	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
 	int ret;
@@ -816,13 +842,19 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	switch (dev->flags & MODEL_MASK) {
 	case MODEL_AMD_NAVI_GPU:
+		if (dev->atomic) {
+			ret = -EOPNOTSUPP;
+			goto done_nolock;
+		}
+
 		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
 		goto done_nolock;
 	default:
 		break;
 	}
 
-	reinit_completion(&dev->cmd_complete);
+	if (!dev->atomic)
+		reinit_completion(&dev->cmd_complete);
 	dev->msgs = msgs;
 	dev->msgs_num = num;
 	dev->cmd_err = 0;
@@ -845,12 +877,18 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	i2c_dw_xfer_init(dev);
 
 	/* Wait for tx to complete */
-	ret = i2c_dw_wait_transfer(dev);
+	if (dev->atomic)
+		ret = i2c_dw_wait_transfer_atomic(dev);
+	else
+		ret = i2c_dw_wait_transfer(dev);
+
 	if (ret) {
 		dev_err(dev->dev, "controller timed out\n");
-		/* i2c_dw_init_master() implicitly disables the adapter */
-		i2c_recover_bus(&dev->adapter);
-		i2c_dw_init_master(dev);
+		if (!dev->atomic) {
+			/* i2c_dw_init_master() implicitly disables the adapter */
+			i2c_recover_bus(&dev->adapter);
+			i2c_dw_init_master(dev);
+		}
 		goto done;
 	}
 
@@ -907,7 +945,25 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	return ret;
 }
 
-static const struct i2c_algorithm i2c_dw_algo = {
+static int
+i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+{
+	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+
+	dev->atomic = false;
+	return i2c_dw_xfer_core(adap, msgs, num);
+}
+
+static int
+i2c_dw_xfer_atomic(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
+{
+	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
+
+	dev->atomic = true;
+	return i2c_dw_xfer_core(adap, msgs, num);
+}
+
+static struct i2c_algorithm i2c_dw_algo = {
 	.xfer = i2c_dw_xfer,
 	.functionality = i2c_dw_func,
 };
@@ -1048,6 +1104,8 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev)
 			  "Synopsys DesignWare I2C adapter");
 	adap->retries = 3;
 	adap->algo = &i2c_dw_algo;
+	if (!dev->acquire_lock)
+		i2c_dw_algo.xfer_atomic = i2c_dw_xfer_atomic,
 	adap->quirks = &i2c_dw_quirks;
 	adap->dev.parent = dev->dev;
 	i2c_set_adapdata(adap, dev);
-- 
2.50.1

Re: [PATCH 1/2] i2c: designware: Avoid taking clk_prepare mutex in PM callbacks

[Andy Shevchenko]

On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote:
> This is unsafe, as the runtime PM callbacks are called from the PM
> workqueue, so this may deadlock when handling an i2c attached clock,
> which may already hold the clk_prepare mutex from another context.

Can you be more specific? What is the actual issue in practice?
Do you have traces and lockdep warnings?

AFAICS it seems related to the bus recovery mechanism. Is this what you have in
mind?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] i2c: designware: Avoid taking clk_prepare mutex in PM callbacks

[Jisheng Zhang]

On Wed, Aug 20, 2025 at 07:05:42PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote:
> > This is unsafe, as the runtime PM callbacks are called from the PM
> > workqueue, so this may deadlock when handling an i2c attached clock,
> > which may already hold the clk_prepare mutex from another context.
> 
> Can you be more specific? What is the actual issue in practice?
> Do you have traces and lockdep warnings?

Assume we use i2c designware to control any i2c based clks, e.g the
clk-si5351.c driver. In its .clk_prepare, we'll get the prepare_lock
mutex, then we call i2c adapter to operate the regs, to runtime resume
the i2c adapter, we call clk_prepare_enable() which will try to get
the prepare_lock mutex again.

Regards.

> 
> AFAICS it seems related to the bus recovery mechanism. Is this what you have in
> mind?
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 2/2] i2c: designware: Implement atomic transfer suppot

[Andy Shevchenko]

On Wed, Aug 20, 2025 at 11:31:25PM +0800, Jisheng Zhang wrote:
> Rework the read and write code paths in the driver to support operation
> in atomic contexts. To achieve this, the driver must not rely on IRQs
> or perform any scheduling, e.g., via a sleep or schedule routine.
> 
> Implement atomic, sleep-free, and IRQ-less operation. This increases
> complexity but is necessary for atomic I2C transfers required by some
> hardware configurations, e.g., to trigger reboots on an external PMIC chip.

...

> -		usleep_range(25, 250);
> +		if (dev->atomic)
> +			udelay(25);
> +		else
> +			usleep_range(25, 250);

Wondering why this delay is not being properly calculated. Why in atomic case
is okay to use the shortest one?

...

> -	if (!dev->acquire_lock)
> +	if (dev->atomic || !dev->acquire_lock)

I think basically we should no allow atomic transfers at all when the lock is
in use. Otherwise it will be interesting case if HW (FW) wants to have an
exclusive access while OS wants to perform an atomic transfer.

...

> +	if (dev->atomic)
> +		return regmap_read_poll_timeout_atomic(dev->map, DW_IC_STATUS, status,
> +						       !(status & DW_IC_STATUS_MASTER_ACTIVITY),
> +						       1100, 20000) != 0;
> +	else
> +		return regmap_read_poll_timeout(dev->map, DW_IC_STATUS, status,
> +					       !(status & DW_IC_STATUS_MASTER_ACTIVITY),
> +					       1100, 20000) != 0;

Please, drop ' != 0' parts at the same time, they are redundant.

...

> +static int i2c_dw_wait_transfer_atomic(struct dw_i2c_dev *dev)
> +{
> +	ktime_t timeout = ktime_add_us(ktime_get(), jiffies_to_usecs(dev->adapter.timeout));
> +	unsigned int stat;
> +	int ret;
> +
> +	do {
> +		ret = try_wait_for_completion(&dev->cmd_complete);
> +		if (ret)
> +			break;
> +
> +		stat = i2c_dw_read_clear_intrbits(dev);
> +		if (stat)
> +			i2c_dw_process_transfer(dev, stat);
> +		else
> +			udelay(15);

No explanation about this value.

> +	} while (ktime_compare(ktime_get(), timeout) < 0);

Whe have _before() and _after() APIs, use them.

> +	return ret ? 0 : -ETIMEDOUT;
> +}

...

>  	switch (dev->flags & MODEL_MASK) {
>  	case MODEL_AMD_NAVI_GPU:
> +		if (dev->atomic) {
> +			ret = -EOPNOTSUPP;
> +			goto done_nolock;
> +		}

Why only AMD case?

>  		ret = amd_i2c_dw_xfer_quirk(adap, msgs, num);
>  		goto done_nolock;
>  	default:
>  		break;
>  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] i2c: designware: Avoid taking clk_prepare mutex in PM callbacks

[Jarkko Nikula]

On 8/20/25 7:33 PM, Jisheng Zhang wrote:
> On Wed, Aug 20, 2025 at 07:05:42PM +0300, Andy Shevchenko wrote:
>> On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote:
>>> This is unsafe, as the runtime PM callbacks are called from the PM
>>> workqueue, so this may deadlock when handling an i2c attached clock,
>>> which may already hold the clk_prepare mutex from another context.
>>
>> Can you be more specific? What is the actual issue in practice?
>> Do you have traces and lockdep warnings?
> 
> Assume we use i2c designware to control any i2c based clks, e.g the
> clk-si5351.c driver. In its .clk_prepare, we'll get the prepare_lock
> mutex, then we call i2c adapter to operate the regs, to runtime resume
> the i2c adapter, we call clk_prepare_enable() which will try to get
> the prepare_lock mutex again.
> 
I'd also like to see the issue here. I'm blind to see what's the 
relation between the clocks managed by the clk-si5351.c and clocks to 
the i2c-designware IP.

Re: [PATCH 1/2] i2c: designware: Avoid taking clk_prepare mutex in PM callbacks

[Andy Shevchenko]

On Thu, Aug 21, 2025 at 03:45:43PM +0300, Jarkko Nikula wrote:
> On 8/20/25 7:33 PM, Jisheng Zhang wrote:
> > On Wed, Aug 20, 2025 at 07:05:42PM +0300, Andy Shevchenko wrote:
> > > On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote:
> > > > This is unsafe, as the runtime PM callbacks are called from the PM
> > > > workqueue, so this may deadlock when handling an i2c attached clock,
> > > > which may already hold the clk_prepare mutex from another context.
> > > 
> > > Can you be more specific? What is the actual issue in practice?
> > > Do you have traces and lockdep warnings?
> > 
> > Assume we use i2c designware to control any i2c based clks, e.g the
> > clk-si5351.c driver. In its .clk_prepare, we'll get the prepare_lock
> > mutex, then we call i2c adapter to operate the regs, to runtime resume
> > the i2c adapter, we call clk_prepare_enable() which will try to get
> > the prepare_lock mutex again.
> > 
> I'd also like to see the issue here. I'm blind to see what's the relation
> between the clocks managed by the clk-si5351.c and clocks to the
> i2c-designware IP.

I believe they try to make an example when clk-si5351 is the provider of
the clock to I²C host controller (DesignWare).

But I'm still not sure about the issues here... Without (even simulated with
specific delay injections) lockdep warnings it would be rather theoretical.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] i2c: designware: Avoid taking clk_prepare mutex in PM callbacks

[Jisheng Zhang]

On Thu, Aug 21, 2025 at 04:01:55PM +0300, Andy Shevchenko wrote:
> On Thu, Aug 21, 2025 at 03:45:43PM +0300, Jarkko Nikula wrote:
> > On 8/20/25 7:33 PM, Jisheng Zhang wrote:
> > > On Wed, Aug 20, 2025 at 07:05:42PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote:
> > > > > This is unsafe, as the runtime PM callbacks are called from the PM
> > > > > workqueue, so this may deadlock when handling an i2c attached clock,
> > > > > which may already hold the clk_prepare mutex from another context.
> > > > 
> > > > Can you be more specific? What is the actual issue in practice?
> > > > Do you have traces and lockdep warnings?
> > > 
> > > Assume we use i2c designware to control any i2c based clks, e.g the
> > > clk-si5351.c driver. In its .clk_prepare, we'll get the prepare_lock
> > > mutex, then we call i2c adapter to operate the regs, to runtime resume
> > > the i2c adapter, we call clk_prepare_enable() which will try to get
> > > the prepare_lock mutex again.
> > > 
> > I'd also like to see the issue here. I'm blind to see what's the relation
> > between the clocks managed by the clk-si5351.c and clocks to the
> > i2c-designware IP.

The key here is: all clks in the system share the same prepare_lock
mutex, so the global prepare_lock mutex is locked by clk-si5351
.prepare(), then in this exact .prepare(), the i2c-designware's runtime
resume will try to lock the same prepare_lock again due to
clk_prepare_enable()
can you plz check clk_prepare_lock() in drivers/clk/clk.c?

And if we take a look at other i2c adapters' drivers, we'll see
some of them have ever met this issue and already fixed it, such
as 

i2c-exynos5, by commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA
deadlock by keeping I2C clock prepared")

i2c-imx, by commit d9a22d713acb ("i2c: imx: avoid taking clk_prepare
mutex in PM callbacks")
> 
> I believe they try to make an example when clk-si5351 is the provider of
> the clock to I²C host controller (DesignWare).

Nope, the example case is using i2c host controller to operate the clk-si5351
> 
> But I'm still not sure about the issues here... Without (even simulated with
> specific delay injections) lockdep warnings it would be rather theoretical.

No, it happened in real world.

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 1/2] i2c: designware: Avoid taking clk_prepare mutex in PM callbacks

[Andy Shevchenko]

On Fri, Aug 22, 2025 at 12:32:57AM +0800, Jisheng Zhang wrote:
> On Thu, Aug 21, 2025 at 04:01:55PM +0300, Andy Shevchenko wrote:
> > On Thu, Aug 21, 2025 at 03:45:43PM +0300, Jarkko Nikula wrote:
> > > On 8/20/25 7:33 PM, Jisheng Zhang wrote:
> > > > On Wed, Aug 20, 2025 at 07:05:42PM +0300, Andy Shevchenko wrote:
> > > > > On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote:
> > > > > > This is unsafe, as the runtime PM callbacks are called from the PM
> > > > > > workqueue, so this may deadlock when handling an i2c attached clock,
> > > > > > which may already hold the clk_prepare mutex from another context.
> > > > > 
> > > > > Can you be more specific? What is the actual issue in practice?
> > > > > Do you have traces and lockdep warnings?
> > > > 
> > > > Assume we use i2c designware to control any i2c based clks, e.g the
> > > > clk-si5351.c driver. In its .clk_prepare, we'll get the prepare_lock
> > > > mutex, then we call i2c adapter to operate the regs, to runtime resume
> > > > the i2c adapter, we call clk_prepare_enable() which will try to get
> > > > the prepare_lock mutex again.
> > > > 
> > > I'd also like to see the issue here. I'm blind to see what's the relation
> > > between the clocks managed by the clk-si5351.c and clocks to the
> > > i2c-designware IP.
> 
> The key here is: all clks in the system share the same prepare_lock
> mutex, so the global prepare_lock mutex is locked by clk-si5351
> .prepare(), then in this exact .prepare(), the i2c-designware's runtime
> resume will try to lock the same prepare_lock again due to
> clk_prepare_enable()
> can you plz check clk_prepare_lock() in drivers/clk/clk.c?
> 
> And if we take a look at other i2c adapters' drivers, we'll see
> some of them have ever met this issue and already fixed it, such
> as 
> 
> i2c-exynos5, by commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA
> deadlock by keeping I2C clock prepared")
> 
> i2c-imx, by commit d9a22d713acb ("i2c: imx: avoid taking clk_prepare
> mutex in PM callbacks")

Why is this an I²C driver problem?

> > I believe they try to make an example when clk-si5351 is the provider of
> > the clock to I²C host controller (DesignWare).
> 
> Nope, the example case is using i2c host controller to operate the clk-si5351

Okay, so that chip is controlled over I²C, but how their clocks even related to
the I²C host controller clock?! I am sorry, I am lost here.

> > But I'm still not sure about the issues here... Without (even simulated with
> > specific delay injections) lockdep warnings it would be rather theoretical.
> 
> No, it happened in real world.

Can you provide the asked traces and lockdep warnigns and/or other stuff to see
what's going on there?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] i2c: designware: Avoid taking clk_prepare mutex in PM callbacks

[Andy Shevchenko]

On Fri, Aug 22, 2025 at 12:18:43PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 22, 2025 at 12:32:57AM +0800, Jisheng Zhang wrote:
> > On Thu, Aug 21, 2025 at 04:01:55PM +0300, Andy Shevchenko wrote:
> > > On Thu, Aug 21, 2025 at 03:45:43PM +0300, Jarkko Nikula wrote:
> > > > On 8/20/25 7:33 PM, Jisheng Zhang wrote:
> > > > > On Wed, Aug 20, 2025 at 07:05:42PM +0300, Andy Shevchenko wrote:
> > > > > > On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote:
> > > > > > > This is unsafe, as the runtime PM callbacks are called from the PM
> > > > > > > workqueue, so this may deadlock when handling an i2c attached clock,
> > > > > > > which may already hold the clk_prepare mutex from another context.
> > > > > > 
> > > > > > Can you be more specific? What is the actual issue in practice?
> > > > > > Do you have traces and lockdep warnings?
> > > > > 
> > > > > Assume we use i2c designware to control any i2c based clks, e.g the
> > > > > clk-si5351.c driver. In its .clk_prepare, we'll get the prepare_lock
> > > > > mutex, then we call i2c adapter to operate the regs, to runtime resume
> > > > > the i2c adapter, we call clk_prepare_enable() which will try to get
> > > > > the prepare_lock mutex again.
> > > > > 
> > > > I'd also like to see the issue here. I'm blind to see what's the relation
> > > > between the clocks managed by the clk-si5351.c and clocks to the
> > > > i2c-designware IP.
> > 
> > The key here is: all clks in the system share the same prepare_lock
> > mutex, so the global prepare_lock mutex is locked by clk-si5351
> > .prepare(), then in this exact .prepare(), the i2c-designware's runtime
> > resume will try to lock the same prepare_lock again due to
> > clk_prepare_enable()
> > can you plz check clk_prepare_lock() in drivers/clk/clk.c?
> > 
> > And if we take a look at other i2c adapters' drivers, we'll see
> > some of them have ever met this issue and already fixed it, such
> > as 
> > 
> > i2c-exynos5, by commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA
> > deadlock by keeping I2C clock prepared")
> > 
> > i2c-imx, by commit d9a22d713acb ("i2c: imx: avoid taking clk_prepare
> > mutex in PM callbacks")

> Why is this an I²C driver problem?

I just read these two and one more referenced from one of the changes.

I do not think this is a correct fix. Seems to me like papering over a special
(corner case). I would agree on this change if and only if the CLK maintainers
tell us that there is no other way.

My understanding is that the I²C clock and client's clocks (when it's a clock
provider) are independent. There should not be such a clash to begin with. The
clock framework should operate on a clock subtrees and not having yet another
Global Kernel Lock.

That said, I think this is a design issue in CLK framework, we should not go and
"fix" all the drivers. Today it's I²C, tomorrow SPI and I³C and so on...
This is not a scalable solution.

Here is formal NAK until it will be worked with CLK maintainers to provide an
agreed roadmap for this(ese) issue(s).

> > > I believe they try to make an example when clk-si5351 is the provider of
> > > the clock to I²C host controller (DesignWare).
> > 
> > Nope, the example case is using i2c host controller to operate the clk-si5351
> 
> Okay, so that chip is controlled over I²C, but how their clocks even related to
> the I²C host controller clock?! I am sorry, I am lost here.
> 
> > > But I'm still not sure about the issues here... Without (even simulated with
> > > specific delay injections) lockdep warnings it would be rather theoretical.
> > 
> > No, it happened in real world.
> 
> Can you provide the asked traces and lockdep warnigns and/or other stuff to see
> what's going on there?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] i2c: designware: Avoid taking clk_prepare mutex in PM callbacks

[Jisheng Zhang]

On Fri, Aug 22, 2025 at 12:34:07PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 22, 2025 at 12:18:43PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 22, 2025 at 12:32:57AM +0800, Jisheng Zhang wrote:
> > > On Thu, Aug 21, 2025 at 04:01:55PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Aug 21, 2025 at 03:45:43PM +0300, Jarkko Nikula wrote:
> > > > > On 8/20/25 7:33 PM, Jisheng Zhang wrote:
> > > > > > On Wed, Aug 20, 2025 at 07:05:42PM +0300, Andy Shevchenko wrote:
> > > > > > > On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote:
> > > > > > > > This is unsafe, as the runtime PM callbacks are called from the PM
> > > > > > > > workqueue, so this may deadlock when handling an i2c attached clock,
> > > > > > > > which may already hold the clk_prepare mutex from another context.
> > > > > > > 
> > > > > > > Can you be more specific? What is the actual issue in practice?
> > > > > > > Do you have traces and lockdep warnings?
> > > > > > 
> > > > > > Assume we use i2c designware to control any i2c based clks, e.g the
> > > > > > clk-si5351.c driver. In its .clk_prepare, we'll get the prepare_lock
> > > > > > mutex, then we call i2c adapter to operate the regs, to runtime resume
> > > > > > the i2c adapter, we call clk_prepare_enable() which will try to get
> > > > > > the prepare_lock mutex again.
> > > > > > 
> > > > > I'd also like to see the issue here. I'm blind to see what's the relation
> > > > > between the clocks managed by the clk-si5351.c and clocks to the
> > > > > i2c-designware IP.
> > > 
> > > The key here is: all clks in the system share the same prepare_lock
> > > mutex, so the global prepare_lock mutex is locked by clk-si5351
> > > .prepare(), then in this exact .prepare(), the i2c-designware's runtime
> > > resume will try to lock the same prepare_lock again due to
> > > clk_prepare_enable()
> > > can you plz check clk_prepare_lock() in drivers/clk/clk.c?
> > > 
> > > And if we take a look at other i2c adapters' drivers, we'll see
> > > some of them have ever met this issue and already fixed it, such
> > > as 
> > > 
> > > i2c-exynos5, by commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA
> > > deadlock by keeping I2C clock prepared")
> > > 
> > > i2c-imx, by commit d9a22d713acb ("i2c: imx: avoid taking clk_prepare
> > > mutex in PM callbacks")
> 
> > Why is this an I²C driver problem?
> 
> I just read these two and one more referenced from one of the changes.
> 
> I do not think this is a correct fix. Seems to me like papering over a special
> (corner case). I would agree on this change if and only if the CLK maintainers
> tell us that there is no other way.
> 
> My understanding is that the I²C clock and client's clocks (when it's a clock
> provider) are independent. There should not be such a clash to begin with. The
> clock framework should operate on a clock subtrees and not having yet another
> Global Kernel Lock.
> 
> That said, I think this is a design issue in CLK framework, we should not go and
> "fix" all the drivers. Today it's I²C, tomorrow SPI and I³C and so on...
> This is not a scalable solution.

The fact is there's no SPI attached clks. Current all clks are either
mmio based or i2c attached clks. And yep you're right, I3C has the same
problem if we use i3c controller to operate those i2c attached clks.

> 
> Here is formal NAK until it will be worked with CLK maintainers to provide an
> agreed roadmap for this(ese) issue(s).

+ clk maintainers

Hi Michael and Stephen,

If we use i2c designware adapter to operate the i2c attached clks such
as clk-si5351, then there's a deadlock issue -- This issue is well
explained by commit d9a22d713acb ("i2c: imx: avoid taking clk_prepare
mutex in PM callbacks")

I propsed a similar fix for the i2c-designware adapter, but Andy
disagree with this fix. Instead, he thinks the issue is from the clk
framework. Could you please comment?

Thanks

> 
> > > > I believe they try to make an example when clk-si5351 is the provider of
> > > > the clock to I²C host controller (DesignWare).
> > > 
> > > Nope, the example case is using i2c host controller to operate the clk-si5351
> > 
> > Okay, so that chip is controlled over I²C, but how their clocks even related to
> > the I²C host controller clock?! I am sorry, I am lost here.
> > 
> > > > But I'm still not sure about the issues here... Without (even simulated with
> > > > specific delay injections) lockdep warnings it would be rather theoretical.
> > > 
> > > No, it happened in real world.
> > 
> > Can you provide the asked traces and lockdep warnigns and/or other stuff to see
> > what's going on there?
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 1/2] i2c: designware: Avoid taking clk_prepare mutex in PM callbacks

[Jisheng Zhang]

On Fri, Aug 22, 2025 at 12:34:07PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 22, 2025 at 12:18:43PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 22, 2025 at 12:32:57AM +0800, Jisheng Zhang wrote:
> > > On Thu, Aug 21, 2025 at 04:01:55PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Aug 21, 2025 at 03:45:43PM +0300, Jarkko Nikula wrote:
> > > > > On 8/20/25 7:33 PM, Jisheng Zhang wrote:
> > > > > > On Wed, Aug 20, 2025 at 07:05:42PM +0300, Andy Shevchenko wrote:
> > > > > > > On Wed, Aug 20, 2025 at 11:31:24PM +0800, Jisheng Zhang wrote:
> > > > > > > > This is unsafe, as the runtime PM callbacks are called from the PM
> > > > > > > > workqueue, so this may deadlock when handling an i2c attached clock,
> > > > > > > > which may already hold the clk_prepare mutex from another context.
> > > > > > > 
> > > > > > > Can you be more specific? What is the actual issue in practice?
> > > > > > > Do you have traces and lockdep warnings?
> > > > > > 
> > > > > > Assume we use i2c designware to control any i2c based clks, e.g the
> > > > > > clk-si5351.c driver. In its .clk_prepare, we'll get the prepare_lock
> > > > > > mutex, then we call i2c adapter to operate the regs, to runtime resume
> > > > > > the i2c adapter, we call clk_prepare_enable() which will try to get
> > > > > > the prepare_lock mutex again.
> > > > > > 
> > > > > I'd also like to see the issue here. I'm blind to see what's the relation
> > > > > between the clocks managed by the clk-si5351.c and clocks to the
> > > > > i2c-designware IP.
> > > 
> > > The key here is: all clks in the system share the same prepare_lock
> > > mutex, so the global prepare_lock mutex is locked by clk-si5351
> > > .prepare(), then in this exact .prepare(), the i2c-designware's runtime
> > > resume will try to lock the same prepare_lock again due to
> > > clk_prepare_enable()
> > > can you plz check clk_prepare_lock() in drivers/clk/clk.c?
> > > 
> > > And if we take a look at other i2c adapters' drivers, we'll see
> > > some of them have ever met this issue and already fixed it, such
> > > as 
> > > 
> > > i2c-exynos5, by commit 10ff4c5239a1 ("i2c: exynos5: Fix possible ABBA
> > > deadlock by keeping I2C clock prepared")
> > > 
> > > i2c-imx, by commit d9a22d713acb ("i2c: imx: avoid taking clk_prepare
> > > mutex in PM callbacks")
> 
> > Why is this an I²C driver problem?
> 
> I just read these two and one more referenced from one of the changes.
> 
> I do not think this is a correct fix. Seems to me like papering over a special
> (corner case). I would agree on this change if and only if the CLK maintainers
> tell us that there is no other way.
> 
> My understanding is that the I²C clock and client's clocks (when it's a clock
> provider) are independent. There should not be such a clash to begin with. The
> clock framework should operate on a clock subtrees and not having yet another
> Global Kernel Lock.
> 
> That said, I think this is a design issue in CLK framework, we should not go and

After some thoughts, let me show you another case where this patch is
needed and not related with CLK framework at all: As can be seen in
patch2, atomic transfer support is added, but if IIRC, the
clk_prepare_enable() can't be used in atomic context. Then how to
support atomic transfer if patch1 is NAKed? Any comment is appreciated.

Thanks

> "fix" all the drivers. Today it's I²C, tomorrow SPI and I³C and so on...
> This is not a scalable solution.
> 
> Here is formal NAK until it will be worked with CLK maintainers to provide an
> agreed roadmap for this(ese) issue(s).
> 
> > > > I believe they try to make an example when clk-si5351 is the provider of
> > > > the clock to I²C host controller (DesignWare).
> > > 
> > > Nope, the example case is using i2c host controller to operate the clk-si5351
> > 
> > Okay, so that chip is controlled over I²C, but how their clocks even related to
> > the I²C host controller clock?! I am sorry, I am lost here.
> > 
> > > > But I'm still not sure about the issues here... Without (even simulated with
> > > > specific delay injections) lockdep warnings it would be rather theoretical.
> > > 
> > > No, it happened in real world.
> > 
> > Can you provide the asked traces and lockdep warnigns and/or other stuff to see
> > what's going on there?
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>