Message-ID: <3500149.e9J7NaK4W3@fw-rgant>
Date: Wed, 22 Oct 2025 10:05:40 +0200
From: Romain Gantois <romain.gantois@...tlin.com>
To: Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Jonathan Cameron <jic23@...nel.org>,
 David Lechner <dlechner@...libre.com>,
 Nuno Sá <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>
Cc: Hans de Goede <hansg@...nel.org>,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 linux-iio@...r.kernel.org, Herve Codina <herve.codina@...tlin.com>
Subject:
 Re: [PATCH v2 5/5] regulator: ltm8054: Support output current limit control

Hello everyone,

I've encountered a rather troublesome issue with this particular patch,
which has delayed version 3 of this series. I'd like to describe it here,
so that you can tell me if you have any suggestions for an upstreamable
solution.
 
The problem concerns the set_current_limit() and get_current_limit()
callbacks:

On Thursday, 25 September 2025 14:37:37 CEST Romain Gantois wrote:
...
> -static const struct regulator_ops ltm8054_regulator_ops = { };
> +static int ltm8054_set_current_limit(struct regulator_dev *rdev, int
> min_uA, int max_uA) +{
> +	struct ltm8054_priv *priv = rdev_get_drvdata(rdev);
> +	u64 vdac_uV;
> +
> +	min_uA = clamp_t(int, min_uA, priv->min_uA, priv->max_uA);
> +
> +	/* adjusted current limit = Rsense current limit * CTL pin voltage / 
max
> CTL pin voltage */ +	vdac_uV = (u64)min_uA * LTM8054_MAX_CTL_uV;
> +	do_div(vdac_uV, priv->max_uA);
> +
> +	dev_dbg(&rdev->dev,
> +		"Setting current limit to %duA, CTL pin to %duV\n", min_uA,
> (int)vdac_uV); +
> +	/* Standard IIO voltage unit is mV, scale accordingly. */
> +	return iio_write_channel_processed_scale(priv->ctl_dac, vdac_uV, 
1000);
> +}
> +
> +static int ltm8054_get_current_limit(struct regulator_dev *rdev)
> +{
> +	struct ltm8054_priv *priv = rdev_get_drvdata(rdev);
> +	int ret, vdac_uv;
> +	u64 uA;
> +
> +	ret = iio_read_channel_processed_scale(priv->ctl_dac, &vdac_uv, 1000);
> +	if (ret < 0) {
> +		dev_err(&rdev->dev, "failed to read CTL DAC voltage, err %d\n", 
ret);
> +		return ret;
> +	}
> +
> +	uA = (u64)vdac_uv * priv->max_uA;
> +	do_div(uA, LTM8054_MAX_CTL_uV);
> +
> +	return uA;
> +}
> +
> +static const struct regulator_ops ltm8054_regulator_ops = {
> +	.set_current_limit = ltm8054_set_current_limit,
> +	.get_current_limit = ltm8054_get_current_limit,
> +};
> +
...

I've encountered a lockdep splat while testing these callbacks. I've
included a summary of the splat at the end of this email [1].
 
After investigating, it seems like the issue lies with IIO callbacks in the
ad5592r driver being called with the LTM8054 regulator device lock held.
 
The ad5592r callbacks themselves call into the regulator core to enable the
DAC's regulators, which might try the LTM8054 lock again in the same
thread, causing a deadlock. This would only happen if the LTM8054 was
supplying voltage to the ad5592r.
 
There are two parts to this issue:

1. Making sure that the CTL IIO channel used by an LTM8054 device isn't
supplied by the LTM8054 itself (or a consumer of the LTM8054). Solving this 
removes the risk of an actual deadlock.
 
2. Silencing the lockdep splat. The splat seems to be triggered by the IIO
driver taking the general regulator ww_mutex context, which means it will
still occur even if we've made sure that the IIO channel isn't a consumer
of the LTM8054 regulator.

For part 1., a potential solution would be to create a device link with the
LTM8054 device as a consumer and the CTL IIO channel as a supplier. IIUC
device links do not tolerate cycles, so this should ensure that the IIO
channel isn't a direct or indirect consumer of the LTM8054.

However, the LTM8054 driver cannot access the IIO device struct to create the 
link, so adding a new IIO consumer API function could be necessary.
 
For part 2., I'm having more trouble finding a proper solution. One
potential fix would be to put the IIO channel reads/writes in a LTM8054
driver work item and have them run without the regulator lock held. This
would incidentally also solve part 1., however it would make the current
limit operations asynchronous, and it seems like a lot of unnecessary
complexity.
 
Please tell me if you have any suggestions for solving this, I'll keep
searching on my side in the meantime.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[1] lockdep splat summary
```
WARNING: possible circular locking dependency detected
6.17.0-rc6+ #9 Not tainted
------------------------------------------------------
kworker/u17:0/34 is trying to acquire lock:
(&iio_dev_opaque->info_exist_lock){+.+.}-{4:4}, at: 
iio_read_channel_processed_scale+0x40/0x120
   
but task is already holding lock:
(regulator_ww_class_mutex){+.+.}-{4:4}, at: ww_mutex_trylock+0x184/0x3a0
   
which lock already depends on the new lock.
   
   
the existing dependency chain (in reverse order) is:
   
-> #2 (regulator_ww_class_mutex){+.+.}-{4:4}:
       lock_acquire+0xf0/0x2c0
       regulator_lock_dependent+0x120/0x270
       regulator_enable+0x38/0xd0
       ad5592r_probe+0xcc/0x630
       ad5593r_i2c_probe+0x58/0x80
   ...
       ret_from_fork+0x10/0x20
   
-> #1 (regulator_ww_class_acquire){+.+.}-{0:0}:
       reacquire_held_locks+0xd4/0x1c0
       lock_release+0x148/0x2c0
       __mutex_unlock_slowpath+0x3c/0x2f0
       mutex_unlock+0x1c/0x30
       regulator_lock_dependent+0x1d4/0x270
       regulator_get_voltage+0x34/0xd0
       ad5592r_read_raw+0x154/0x2f0
       iio_channel_read.isra.0+0xac/0xd0
       iio_write_channel_processed_scale+0x64/0x1e0
       ltm8054_set_current_limit+0x70/0xd0
   ...
       ret_from_fork+0x10/0x20
   
-> #0 (&iio_dev_opaque->info_exist_lock){+.+.}-{4:4}:
       check_prev_add+0x104/0xc60
       __lock_acquire+0x12a4/0x15c0
       lock_acquire+0xf0/0x2c0
       __mutex_lock+0x90/0xc80
       mutex_lock_nested+0x28/0x40
       iio_read_channel_processed_scale+0x40/0x120
       ltm8054_get_current_limit+0x34/0xa0
       kthread+0x11c/0x1f0
   ...
       ret_from_fork+0x10/0x20
   
other info that might help us debug this:
   
Chain exists of:
  &iio_dev_opaque->info_exist_lock --> regulator_ww_class_acquire --> 
regulator_ww_class_mutex
   
 Possible unsafe locking scenario:
   
       CPU0                    CPU1
       ----                    ----
  lock(regulator_ww_class_mutex);
                               lock(regulator_ww_class_acquire);
                               lock(regulator_ww_class_mutex);
  lock(&iio_dev_opaque->info_exist_lock);
   
 *** DEADLOCK ***
```

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists