Re: [PATCH] i2c: core: fix lockdep warning for sparsely nested adapter chain

[Wolfram Sang <wsa@kernel.org>]

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

Hi Daniel,

On Wed, Oct 18, 2023 at 11:46:13AM +0200, Daniel Mack wrote:
> When adapters are chained in a sparse manner (with intermediate MFD devices,

So, you have an MFD including an i2c-mux or something?

> for instance) the code currently fails to use the correct subclass for
> the adapter's bus_lock which leads to false-positive lockdep warnings.
> 
> Fix this by walking the entire pedigree of the device and count all
> adapters along the way instead of just checking the immediate parent.

Sounds reasonable to me.

> 
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> ---
> This hit me when during the development of a driver stack that isn't
> submitted mainline yet. This patch could however be discussed
> independently I think.

Yes, it can :)

> 
>  drivers/i2c/i2c-core-base.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 60746652fd52..4692a1e5ea0a 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1189,9 +1189,11 @@ static void i2c_adapter_dev_release(struct device *dev)
>  unsigned int i2c_adapter_depth(struct i2c_adapter *adapter)
>  {
>  	unsigned int depth = 0;
> +	struct device *parent;
>  
> -	while ((adapter = i2c_parent_is_i2c_adapter(adapter)))

I never noticed we overwrite the 'adapter' function argument. Much
better with your version and the local variable.

> -		depth++;
> +	for (parent = adapter->dev.parent; parent; parent = parent->parent)
> +		if (parent->type == &i2c_adapter_type)
> +			depth++;

I am not sure myself. Is the code explaining itself or should we add a
short comment why we use a for-loop? I tend to leave it as is.

Thanks for this patch!

   Wolfram


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