[PATCH] i2c: jz4780: Cache clock rate at probe to prevent CCF prepare_lock deadlock

[PATCH] i2c: jz4780: Cache clock rate at probe to prevent CCF prepare_lock deadlock

[H. Nikolaus Schaller]

Fix a severe AB/BA deadlock between the Common Clock Framework (CCF)
and the I2C adapter lock, which triggers when an I2C-controlled clock
generator (like the Si5351) is registered or modified under the CCF.

During a clock frequency change, the CCF acquires its global 'prepare_lock'
mutex and calls i2c_transfer() to update the chip registers, stalling
for the adapter's I2C bus lock.

Concurrently, a parallel transfer on the same bus (e.g., a GPIO expander
handling LEDs) can hold the I2C adapter lock. Inside the transfer path,
jz4780_i2c_set_speed() calls clk_get_rate() to dynamically calculate
timings. This call attempts to acquire the blocked CCF 'prepare_lock',
creating a circular dependency that freezes the system or at least
the involved processes and workers.

Eliminate the synchronous clk_get_rate() call from the active transfer
path by caching the peripheral clock rate once - inside the private
jz4780_i2c structure during jz4780_i2c_probe(). Update
jz4780_i2c_set_speed() to use this cached value, decoupling active I2C
transactions from the CCF internal locks.

Assisted-by web based Google AI.

Fixes: ba92222ed63a12 ("i2c: jz4780: Add i2c bus controller driver for Ingenic JZ4780")
Cc: stable@vger.kernel.org
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/i2c/busses/i2c-jz4780.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-jz4780.c b/drivers/i2c/busses/i2c-jz4780.c
index 664a5471d9335..d729cec9cdf2c 100644
--- a/drivers/i2c/busses/i2c-jz4780.c
+++ b/drivers/i2c/busses/i2c-jz4780.c
@@ -141,6 +141,7 @@ struct jz4780_i2c {
 	void __iomem		*iomem;
 	int			 irq;
 	struct clk		*clk;
+	unsigned		long clk_rate;
 	struct i2c_adapter	 adap;
 	const struct ingenic_i2c_config *cdata;
 
@@ -246,7 +247,7 @@ static int jz4780_i2c_set_target(struct jz4780_i2c *i2c, unsigned char address)
 
 static int jz4780_i2c_set_speed(struct jz4780_i2c *i2c)
 {
-	int dev_clk_khz = clk_get_rate(i2c->clk) / 1000;
+	int dev_clk_khz = i2c->clk_rate / 1000;
 	int cnt_high = 0;	/* HIGH period count of the SCL clock */
 	int cnt_low = 0;	/* LOW period count of the SCL clock */
 	int cnt_period = 0;	/* period count of the SCL clock */
@@ -796,6 +797,8 @@ static int jz4780_i2c_probe(struct platform_device *pdev)
 	if (IS_ERR(i2c->clk))
 		return PTR_ERR(i2c->clk);
 
+	i2c->clk_rate = clk_get_rate(i2c->clk);
+
 	ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency",
 				   &clk_freq);
 	if (ret) {
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] i2c: jz4780: Cache clock rate at probe to prevent CCF prepare_lock deadlock

[Andi Shyti]

Hi Nikolaus,

On Fri, Jul 10, 2026 at 08:58:35AM +0200, H. Nikolaus Schaller wrote:
> Fix a severe AB/BA deadlock between the Common Clock Framework (CCF)
> and the I2C adapter lock, which triggers when an I2C-controlled clock
> generator (like the Si5351) is registered or modified under the CCF.
> 
> During a clock frequency change, the CCF acquires its global 'prepare_lock'
> mutex and calls i2c_transfer() to update the chip registers, stalling
> for the adapter's I2C bus lock.

I don't think caching the clock rate once at probe is safe.

If the controller clock rate changes afterwards,
jz4780_i2c_set_speed() will keep using the stale cached value and
calculate incorrect bus timings.

Andi

> Concurrently, a parallel transfer on the same bus (e.g., a GPIO expander
> handling LEDs) can hold the I2C adapter lock. Inside the transfer path,
> jz4780_i2c_set_speed() calls clk_get_rate() to dynamically calculate
> timings. This call attempts to acquire the blocked CCF 'prepare_lock',
> creating a circular dependency that freezes the system or at least
> the involved processes and workers.
> 
> Eliminate the synchronous clk_get_rate() call from the active transfer
> path by caching the peripheral clock rate once - inside the private
> jz4780_i2c structure during jz4780_i2c_probe(). Update
> jz4780_i2c_set_speed() to use this cached value, decoupling active I2C
> transactions from the CCF internal locks.
> 
> Assisted-by web based Google AI.
> 
> Fixes: ba92222ed63a12 ("i2c: jz4780: Add i2c bus controller driver for Ingenic JZ4780")
> Cc: stable@vger.kernel.org
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>

Re: [PATCH] i2c: jz4780: Cache clock rate at probe to prevent CCF prepare_lock deadlock

[H. Nikolaus Schaller]

Hi Andi,

> Am 16.07.2026 um 00:07 schrieb Andi Shyti <andi.shyti@kernel.org>:
> 
> Hi Nikolaus,
> 
> On Fri, Jul 10, 2026 at 08:58:35AM +0200, H. Nikolaus Schaller wrote:
>> Fix a severe AB/BA deadlock between the Common Clock Framework (CCF)
>> and the I2C adapter lock, which triggers when an I2C-controlled clock
>> generator (like the Si5351) is registered or modified under the CCF.
>> 
>> During a clock frequency change, the CCF acquires its global 'prepare_lock'
>> mutex and calls i2c_transfer() to update the chip registers, stalling
>> for the adapter's I2C bus lock.
> 
> I don't think caching the clock rate once at probe is safe.

Ok, valid point to discuss.


> If the controller clock rate changes afterwards,
> jz4780_i2c_set_speed() will keep using the stale cached value and
> calculate incorrect bus timings.

But: is clock rate ever changed during operation? Usually it is defined
by the device tree constant.

What is the API to change the clock rate during transfers? Is that even
possible?

BCM2835 appears to have a hackish solutions to register its own clock
divider and provides clk_bcm2835_i2c_set_rate(). This never happens
during normal i2c transfers so would not block.

OMAP does the same as we propose: call clk_get_rate() once during
probe inside omap_i2c_init() and never during omap_i2c_xfer_data().

Finally: how else can you prevent the synchronous call to clk_get_rate()
for each i2c transfer?

BR and thanks,
Nikolaus

Re: [PATCH] i2c: jz4780: Cache clock rate at probe to prevent CCF prepare_lock deadlock

[Andi Shyti]

On Thu, Jul 16, 2026 at 07:44:20AM +0200, H. Nikolaus Schaller wrote:
> Hi Andi,
> 
> > Am 16.07.2026 um 00:07 schrieb Andi Shyti <andi.shyti@kernel.org>:
> > 
> > Hi Nikolaus,
> > 
> > On Fri, Jul 10, 2026 at 08:58:35AM +0200, H. Nikolaus Schaller wrote:
> >> Fix a severe AB/BA deadlock between the Common Clock Framework (CCF)
> >> and the I2C adapter lock, which triggers when an I2C-controlled clock
> >> generator (like the Si5351) is registered or modified under the CCF.
> >> 
> >> During a clock frequency change, the CCF acquires its global 'prepare_lock'
> >> mutex and calls i2c_transfer() to update the chip registers, stalling
> >> for the adapter's I2C bus lock.
> > 
> > I don't think caching the clock rate once at probe is safe.
> 
> Ok, valid point to discuss.
> 
> 
> > If the controller clock rate changes afterwards,
> > jz4780_i2c_set_speed() will keep using the stale cached value and
> > calculate incorrect bus timings.
> 
> But: is clock rate ever changed during operation? Usually it is defined
> by the device tree constant.

That's what you are saying in your commit message.

Andi

Re: [PATCH] i2c: jz4780: Cache clock rate at probe to prevent CCF prepare_lock deadlock

[H. Nikolaus Schaller]

> Am 16.07.2026 um 09:43 schrieb Andi Shyti <andi.shyti@kernel.org>:
> 
> On Thu, Jul 16, 2026 at 07:44:20AM +0200, H. Nikolaus Schaller wrote:
>> Hi Andi,
>> 
>>> Am 16.07.2026 um 00:07 schrieb Andi Shyti <andi.shyti@kernel.org>:
>>> 
>>> Hi Nikolaus,
>>> 
>>> On Fri, Jul 10, 2026 at 08:58:35AM +0200, H. Nikolaus Schaller wrote:
>>>> Fix a severe AB/BA deadlock between the Common Clock Framework (CCF)
>>>> and the I2C adapter lock, which triggers when an I2C-controlled clock
>>>> generator (like the Si5351) is registered or modified under the CCF.
>>>> 
>>>> During a clock frequency change, the CCF acquires its global 'prepare_lock'
>>>> mutex and calls i2c_transfer() to update the chip registers, stalling
>>>> for the adapter's I2C bus lock.
>>> 
>>> I don't think caching the clock rate once at probe is safe.
>> 
>> Ok, valid point to discuss.
>> 
>> 
>>> If the controller clock rate changes afterwards,
>>> jz4780_i2c_set_speed() will keep using the stale cached value and
>>> calculate incorrect bus timings.
>> 
>> But: is clock rate ever changed during operation? Usually it is defined
>> by the device tree constant.
> 
> That's what you are saying in your commit message.

The Si5351 clock generator driver is connected through this i2c bus. And that
is referred to by "when an I2C-controlled clock generator (like the Si5351) is 
registered or modified under the CCF".

Only then, this bug faces surface. Because that driver changes it's clock through
sending i2c commands which also ask for the lock.

But again: how would you solve this?

BR and thanks,
Nikolaus

Re: [PATCH] i2c: jz4780: Cache clock rate at probe to prevent CCF prepare_lock deadlock

[Andi Shyti]

On Thu, Jul 16, 2026 at 10:33:56AM +0200, H. Nikolaus Schaller wrote:
> > Am 16.07.2026 um 09:43 schrieb Andi Shyti <andi.shyti@kernel.org>:
> > On Thu, Jul 16, 2026 at 07:44:20AM +0200, H. Nikolaus Schaller wrote:
> >>> Am 16.07.2026 um 00:07 schrieb Andi Shyti <andi.shyti@kernel.org>:
> >>> On Fri, Jul 10, 2026 at 08:58:35AM +0200, H. Nikolaus Schaller wrote:
> >>>> Fix a severe AB/BA deadlock between the Common Clock Framework (CCF)
> >>>> and the I2C adapter lock, which triggers when an I2C-controlled clock
> >>>> generator (like the Si5351) is registered or modified under the CCF.
> >>>> 
> >>>> During a clock frequency change, the CCF acquires its global 'prepare_lock'
> >>>> mutex and calls i2c_transfer() to update the chip registers, stalling
> >>>> for the adapter's I2C bus lock.
> >>> 
> >>> I don't think caching the clock rate once at probe is safe.
> >> 
> >> Ok, valid point to discuss.
> >> 
> >> 
> >>> If the controller clock rate changes afterwards,
> >>> jz4780_i2c_set_speed() will keep using the stale cached value and
> >>> calculate incorrect bus timings.
> >> 
> >> But: is clock rate ever changed during operation? Usually it is defined
> >> by the device tree constant.
> > 
> > That's what you are saying in your commit message.
> 
> The Si5351 clock generator driver is connected through this i2c bus. And that
> is referred to by "when an I2C-controlled clock generator (like the Si5351) is 
> registered or modified under the CCF".
> 
> Only then, this bug faces surface. Because that driver changes it's clock through
> sending i2c commands which also ask for the lock.

If you make a claim in the commit message, I expect the code to
support it.

You say that the clock rate can change, but then you store it
only once at probe time. That immediately suggests that later
rate changes will be ignored by the transfer path.

> But again: how would you solve this?

It's not my role to give you a solution. I'm just commenting on
what I see.

Besides that, is that an issue you have encountered yourself? Is
it a frequent issue?

Thanks,
Andi

Re: [Letux-kernel] [PATCH] i2c: jz4780: Cache clock rate at probe to prevent CCF prepare_lock deadlock

[H. Nikolaus Schaller]

Hi Andi,

> Am 16.07.2026 um 13:41 schrieb Andi Shyti <andi.shyti@kernel.org>:
> 
> On Thu, Jul 16, 2026 at 10:33:56AM +0200, H. Nikolaus Schaller wrote:
>>> Am 16.07.2026 um 09:43 schrieb Andi Shyti <andi.shyti@kernel.org>:
>>> On Thu, Jul 16, 2026 at 07:44:20AM +0200, H. Nikolaus Schaller wrote:
>>>>> Am 16.07.2026 um 00:07 schrieb Andi Shyti <andi.shyti@kernel.org>:
>>>>> On Fri, Jul 10, 2026 at 08:58:35AM +0200, H. Nikolaus Schaller wrote:
>>>>>> Fix a severe AB/BA deadlock between the Common Clock Framework (CCF)
>>>>>> and the I2C adapter lock, which triggers when an I2C-controlled clock
>>>>>> generator (like the Si5351) is registered or modified under the CCF.
>>>>>> 
>>>>>> During a clock frequency change, the CCF acquires its global 'prepare_lock'
>>>>>> mutex and calls i2c_transfer() to update the chip registers, stalling
>>>>>> for the adapter's I2C bus lock.
>>>>> 
>>>>> I don't think caching the clock rate once at probe is safe.
>>>> 
>>>> Ok, valid point to discuss.
>>>> 
>>>> 
>>>>> If the controller clock rate changes afterwards,
>>>>> jz4780_i2c_set_speed() will keep using the stale cached value and
>>>>> calculate incorrect bus timings.
>>>> 
>>>> But: is clock rate ever changed during operation? Usually it is defined
>>>> by the device tree constant.
>>> 
>>> That's what you are saying in your commit message.
>> 
>> The Si5351 clock generator driver is connected through this i2c bus. And that
>> is referred to by "when an I2C-controlled clock generator (like the Si5351) is 
>> registered or modified under the CCF".
>> 
>> Only then, this bug faces surface. Because that driver changes it's clock through
>> sending i2c commands which also ask for the lock.
> 
> If you make a claim in the commit message, I expect the code to
> support it.

I still do not see where you read the claim you criticize.

The commit messages says:

> Fix a severe AB/BA deadlock between the Common Clock Framework (CCF)
> and the I2C adapter lock, which triggers when an I2C-controlled clock
> generator (like the Si5351) is registered or modified under the CCF.

This clearly says that the clock generator is I2C controlled.

And that the problem arises if the frequency *of the clock generator* is changed.

It does not say anything about *changing the I2C bus clock* itself.

> During a clock frequency change, the CCF acquires its global 'prepare_lock'
> mutex and calls i2c_transfer() to update the chip registers, stalling
> for the adapter's I2C bus lock.

This describes what happens in the current code:
- someone wants to change the clock frequency of the Si5321
- this acquires the CCF lock for this change
- the driver tries to read/write some registers from the i2c chip
- for this it does some I2C transfer
- which locks the I2C bus
- the jz4780 driver tries to read the I2C frequency in its I2C bus driver
- and the system (including I2C) hangs

What is not clear here?

The patch fixes this by caching the i2c clock rate and avoiding that
an i2c transfer asks the CCF for anything.

> 
> You say that the clock rate can change, but then you store it
> only once at probe time. That immediately suggests that later
> rate changes will be ignored by the transfer path.

Where does the commit message say that the *I2C clock rate* is changed
or can change?

It can't be changed by standard API and therefore its clock rate can be
cached as proposed during probe of the jz4780 bus driver.

> 
>> But again: how would you solve this?
> 
> It's not my role to give you a solution. I'm just commenting on
> what I see.

If you can comment, I simply expect you to have experience to give
a hint how to solve it differently.

> Besides that, is that an issue you have encountered yourself? Is
> it a frequent issue?

Yes. It comes 100% if you connect an Si5351 to the CI20 board or
others board which use the same driver (e.g. some X1600 based one).

It is likely that any other i2c connected device which registers some
clocks is running into the same problem.

Therefore I consider this as a fundamental design flaw which is solved
by this patch.

BR and thanks,
Nikolaus

Re: [Letux-kernel] [PATCH] i2c: jz4780: Cache clock rate at probe to prevent CCF prepare_lock deadlock

[Andi Shyti]

Nikolaus,

> Therefore I consider this as a fundamental design flaw which is solved
> by this patch.

I'm not arguing the patch here. I haven't even mentioned a single
line from the code. I made a simple question and you keep
hammering on the same argument: in the commit message you say
that when the clock changes it generates a deadlock through
i2c_xfer. I simply asked if the clock changes how is the new
clock value taken by the i2c?

If the commit message is leaving room for questions then you need
to make your commit message clearer.

Andi

Re: [Letux-kernel] [PATCH] i2c: jz4780: Cache clock rate at probe to prevent CCF prepare_lock deadlock

[H. Nikolaus Schaller]

Hi Andi,

> Am 16.07.2026 um 17:08 schrieb Andi Shyti <andi.shyti@kernel.org>:
> 
> Nikolaus,
> 
>> Therefore I consider this as a fundamental design flaw which is solved
>> by this patch.
> 
> I'm not arguing the patch here. I haven't even mentioned a single
> line from the code. I made a simple question and you keep

Sorry, but I did not read this as a question but an argument against the patch itself:

"I don't think caching the clock rate once at probe is safe."

This can be read as a "nack" and request to fundamentally change the code. Therefore,
I asked for suggestions how to solve differently.

> If the commit message is leaving room for questions then you need
> to make your commit message clearer.

It took me long to understand your misunderstanding, and now I believe I have found it.
It is because there are two clocks involved which can be mixed up.


So would you be more happy with:

[PATCH v2] i2c: jz4780: Cache host clock rate at probe to prevent CCF prepare_lock deadlock

Fix a severe AB/BA deadlock between the Common Clock Framework (CCF)
and the I2C adapter lock, which triggers when an I2C-controlled clock
generator client (like the Si5351) is registered or modified under the CCF.

During an i2c client clock (generator) frequency change, the CCF acquires its global
'prepare_lock' mutex and calls i2c_transfer() to update the client's chip registers,
stalling for the adapter's I2C bus lock.

Concurrently, an independent, parallel transfer on the same bus (e.g., a GPIO 
expander handling LEDs) can hold the I2C adapter lock. Inside this parallel 
transfer path, jz4780_i2c_set_speed() calls clk_get_rate() on the host 
controller's input clock to calculate bus timings. This call attempts to acquire 
the blocked CCF 'prepare_lock', creating a circular dependency that freezes 
the system.

The jz4780 host controller clock itself is static and never changes at runtime. 
However, calling clk_get_rate() inside the active transfer path introduces 
an unnecessary dependency on the CCF internal locks.

Eliminate the synchronous clk_get_rate() call from the active transfer
path by caching the static host peripheral clock rate once - inside the private
jz4780_i2c structure during jz4780_i2c_probe(). Update jz4780_i2c_set_speed()
to use this cached value, safely decoupling active I2C transactions from the
CCF internal locks without any risk of stale timings.

BR and thanks,
Nikolaus