[PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock

[PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock

[Mårten Lindahl]

There are different init functions for the sensors in this driver in
which only one initialize the generic vcnl4000_lock. With commit
e21b5b1f2669 ("iio: light: vcnl4000: Preserve conf bits when toggle power")
the vcnl4040 sensor started to depend on the lock, but it was missed to
initialize it in vcnl4040's init function. This has not been visible
until we run lockdep on it:

  DEBUG_LOCKS_WARN_ON(lock->magic != lock)
  at kernel/locking/mutex.c:575 __mutex_lock+0x4f8/0x890
  Call trace:
  __mutex_lock
  mutex_lock_nested
  vcnl4200_set_power_state
  vcnl4200_init
  vcnl4000_probe
  i2c_device_probe
  really_probe
  __driver_probe_device
  driver_probe_device
  __driver_attach
  bus_for_each_dev
  driver_attach
  bus_add_driver
  driver_register
  i2c_register_driver
  vcnl4000_driver_init
  do_one_initcall
  do_init_module
  load_module
  __do_sys_finit_module

Fix this by initializing the lock in the probe function instead of doing
it in the chip specific init functions.

Fixes: e21b5b1f2669 ("iio: light: vcnl4000: Preserve conf bits when toggle power")
Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---

v2:
 - Trimmed backtrace in commit message
 - Have 12 digit sha-1 id in Fixes tag
 - Make the lock initialization in probe instead of in _init function

 drivers/iio/light/vcnl4000.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index b5d398228289..caa2fff9f486 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -208,7 +208,6 @@ static int vcnl4000_init(struct vcnl4000_data *data)
 
 	data->rev = ret & 0xf;
 	data->al_scale = 250000;
-	mutex_init(&data->vcnl4000_lock);
 
 	return data->chip_spec->set_power_state(data, true);
 };
@@ -1366,6 +1365,7 @@ static int vcnl4000_probe(struct i2c_client *client,
 	data->client = client;
 	data->id = id->driver_data;
 	data->chip_spec = &vcnl4000_chip_spec_cfg[data->id];
+	mutex_init(&data->vcnl4000_lock);
 
 	ret = data->chip_spec->init(data);
 	if (ret < 0)
-- 
2.30.2

Re: [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock

[Andy Shevchenko]

On Tue, Jan 31, 2023 at 11:29:51AM +0100, Mårten Lindahl wrote:
> There are different init functions for the sensors in this driver in
> which only one initialize the generic vcnl4000_lock. With commit

initializes ?

> e21b5b1f2669 ("iio: light: vcnl4000: Preserve conf bits when toggle power")
> the vcnl4040 sensor started to depend on the lock, but it was missed to
> initialize it in vcnl4040's init function. This has not been visible
> until we run lockdep on it:
> 
>   DEBUG_LOCKS_WARN_ON(lock->magic != lock)
>   at kernel/locking/mutex.c:575 __mutex_lock+0x4f8/0x890
>   Call trace:
>   __mutex_lock
>   mutex_lock_nested
>   vcnl4200_set_power_state
>   vcnl4200_init
>   vcnl4000_probe
>   i2c_device_probe
>   really_probe
>   __driver_probe_device
>   driver_probe_device
>   __driver_attach
>   bus_for_each_dev
>   driver_attach
>   bus_add_driver
>   driver_register
>   i2c_register_driver
>   vcnl4000_driver_init

E.g. the following can be cut without losing significance of the data
(see below as well).

>   do_one_initcall
>   do_init_module
>   load_module
>   __do_sys_finit_module

> Fix this by initializing the lock in the probe function instead of doing
> it in the chip specific init functions.
> 
> Fixes: e21b5b1f2669 ("iio: light: vcnl4000: Preserve conf bits when toggle power")
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> ---
> 
> v2:
>  - Trimmed backtrace in commit message

Not enough, please try harder. The ideal is to have ~3-5 lines of traceback.

>  - Have 12 digit sha-1 id in Fixes tag
>  - Make the lock initialization in probe instead of in _init function

...

>  	data->client = client;
>  	data->id = id->driver_data;
>  	data->chip_spec = &vcnl4000_chip_spec_cfg[data->id];

+ Blank line.

> +	mutex_init(&data->vcnl4000_lock);
>  
>  	ret = data->chip_spec->init(data);
>  	if (ret < 0)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock

[Andy Shevchenko]

On Tue, Jan 31, 2023 at 02:33:54PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 31, 2023 at 11:29:51AM +0100, Mårten Lindahl wrote:

...

> >  - Trimmed backtrace in commit message
> 
> Not enough, please try harder. The ideal is to have ~3-5 lines of traceback.

Just to clarify, the above example (in previous mail) under "E.g." is just to
show the way of thinking about the traceback data significance. Looking at the
whole traceback I believe ~7 lines is what should be left in the result.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock

[Mårten Lindahl]

On 1/31/23 13:33, Andy Shevchenko wrote:
> On Tue, Jan 31, 2023 at 11:29:51AM +0100, Mårten Lindahl wrote:
>> There are different init functions for the sensors in this driver in
>> which only one initialize the generic vcnl4000_lock. With commit
> initializes ?
>
>> e21b5b1f2669 ("iio: light: vcnl4000: Preserve conf bits when toggle power")
>> the vcnl4040 sensor started to depend on the lock, but it was missed to
>> initialize it in vcnl4040's init function. This has not been visible
>> until we run lockdep on it:
>>
>>    DEBUG_LOCKS_WARN_ON(lock->magic != lock)
>>    at kernel/locking/mutex.c:575 __mutex_lock+0x4f8/0x890
>>    Call trace:
>>    __mutex_lock
>>    mutex_lock_nested
>>    vcnl4200_set_power_state
>>    vcnl4200_init
>>    vcnl4000_probe
>>    i2c_device_probe
>>    really_probe
>>    __driver_probe_device
>>    driver_probe_device
>>    __driver_attach
>>    bus_for_each_dev
>>    driver_attach
>>    bus_add_driver
>>    driver_register
>>    i2c_register_driver
>>    vcnl4000_driver_init
> E.g. the following can be cut without losing significance of the data
> (see below as well).
>
>>    do_one_initcall
>>    do_init_module
>>    load_module
>>    __do_sys_finit_module
>> Fix this by initializing the lock in the probe function instead of doing
>> it in the chip specific init functions.
>>
>> Fixes: e21b5b1f2669 ("iio: light: vcnl4000: Preserve conf bits when toggle power")
>> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
>> ---
>>
>> v2:
>>   - Trimmed backtrace in commit message
> Not enough, please try harder. The ideal is to have ~3-5 lines of traceback.

Hi Andy, Maybe I get it right soon ;). Could it perhaps be stripped like 
this?

Call trace:
__mutex_lock
mutex_lock_nested
vcnl4200_set_power_state
vcnl4200_init
vcnl4000_probe
i2c_device_probe


Kind regards

Mårten

>
>>   - Have 12 digit sha-1 id in Fixes tag
>>   - Make the lock initialization in probe instead of in _init function
> ...
>
>>   	data->client = client;
>>   	data->id = id->driver_data;
>>   	data->chip_spec = &vcnl4000_chip_spec_cfg[data->id];
> + Blank line.
>
>> +	mutex_init(&data->vcnl4000_lock);
>>   
>>   	ret = data->chip_spec->init(data);
>>   	if (ret < 0)

Re: [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock

[Andy Shevchenko]

On Tue, Jan 31, 2023 at 02:27:51PM +0100, Mårten Lindahl wrote:
> On 1/31/23 13:33, Andy Shevchenko wrote:
> > On Tue, Jan 31, 2023 at 11:29:51AM +0100, Mårten Lindahl wrote:

...

> > >   - Trimmed backtrace in commit message
> > Not enough, please try harder. The ideal is to have ~3-5 lines of traceback.
> 
> Hi Andy, Maybe I get it right soon ;). Could it perhaps be stripped like
> this?

Lockdep warning is important.

Assuming you left the warning still in place, almost there.

> Call trace:
> __mutex_lock
> mutex_lock_nested
> vcnl4200_set_power_state
> vcnl4200_init
> vcnl4000_probe

Can be cut out:

> i2c_device_probe

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock

[Mårten Lindahl]

On 1/31/23 14:37, Andy Shevchenko wrote:
> On Tue, Jan 31, 2023 at 02:27:51PM +0100, Mårten Lindahl wrote:
>> On 1/31/23 13:33, Andy Shevchenko wrote:
>>> On Tue, Jan 31, 2023 at 11:29:51AM +0100, Mårten Lindahl wrote:
> ...
>
>>>>    - Trimmed backtrace in commit message
>>> Not enough, please try harder. The ideal is to have ~3-5 lines of traceback.
>> Hi Andy, Maybe I get it right soon ;). Could it perhaps be stripped like
>> this?
> Lockdep warning is important.
>
> Assuming you left the warning still in place, almost there.
>
>> Call trace:
>> __mutex_lock
>> mutex_lock_nested
>> vcnl4200_set_power_state
>> vcnl4200_init
>> vcnl4000_probe
> Can be cut out:
Ok, I'll have it like this then:

   DEBUG_LOCKS_WARN_ON(lock->magic != lock)
   at kernel/locking/mutex.c:575 __mutex_lock+0x4f8/0x890
   Call trace:
   __mutex_lock
   mutex_lock_nested
   vcnl4200_set_power_state
   vcnl4200_init
   vcnl4000_probe

Kind regards

Mårten

>
>> i2c_device_probe

Re: [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock

[Cai Huoqing]

On 31 1月 23 11:29:51, Mårten Lindahl wrote:
> There are different init functions for the sensors in this driver in
> which only one initialize the generic vcnl4000_lock. With commit
> e21b5b1f2669 ("iio: light: vcnl4000: Preserve conf bits when toggle power")
> the vcnl4040 sensor started to depend on the lock, but it was missed to
> initialize it in vcnl4040's init function. This has not been visible
> until we run lockdep on it:
> 
>   DEBUG_LOCKS_WARN_ON(lock->magic != lock)
>   at kernel/locking/mutex.c:575 __mutex_lock+0x4f8/0x890
>   Call trace:
>   __mutex_lock
>   mutex_lock_nested
>   vcnl4200_set_power_state
>   vcnl4200_init
>   vcnl4000_probe
>   i2c_device_probe
>   really_probe
>   __driver_probe_device
>   driver_probe_device
>   __driver_attach
>   bus_for_each_dev
>   driver_attach
>   bus_add_driver
>   driver_register
>   i2c_register_driver
>   vcnl4000_driver_init
>   do_one_initcall
>   do_init_module
>   load_module
>   __do_sys_finit_module
> 
> Fix this by initializing the lock in the probe function instead of doing
> it in the chip specific init functions.
> 
> Fixes: e21b5b1f2669 ("iio: light: vcnl4000: Preserve conf bits when toggle power")
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> ---
> 
> v2:
>  - Trimmed backtrace in commit message
>  - Have 12 digit sha-1 id in Fixes tag
>  - Make the lock initialization in probe instead of in _init function
> 
>  drivers/iio/light/vcnl4000.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index b5d398228289..caa2fff9f486 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -208,7 +208,6 @@ static int vcnl4000_init(struct vcnl4000_data *data)
>  
>  	data->rev = ret & 0xf;
>  	data->al_scale = 250000;
> -	mutex_init(&data->vcnl4000_lock);
>  
>  	return data->chip_spec->set_power_state(data, true);
>  };
> @@ -1366,6 +1365,7 @@ static int vcnl4000_probe(struct i2c_client *client,
>  	data->client = client;
>  	data->id = id->driver_data;
>  	data->chip_spec = &vcnl4000_chip_spec_cfg[data->id];
> +	mutex_init(&data->vcnl4000_lock);
>  
>  	ret = data->chip_spec->init(data);
>  	if (ret < 0)
Why not add mutex_init(&data->vcnl4000_lock) in vcnl4200_init.
like this

@@ -330,6 +330,7 @@ static int vcnl4200_init(struct vcnl4000_data *data)
        }
        mutex_init(&data->vcnl4200_al.lock);
        mutex_init(&data->vcnl4200_ps.lock);
+       mutex_init(&data->vcnl4000_lock);

        ret = data->chip_spec->set_power_state(data, true);
        if (ret < 0)

Perfer adding mutex_init to vcnl4200_init.
> -- 
> 2.30.2
> 

Re: [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock

[Mårten Lindahl]

On 1/31/23 13:46, Cai Huoqing wrote:
> On 31 1月 23 11:29:51, Mårten Lindahl wrote:
>> There are different init functions for the sensors in this driver in
>> which only one initialize the generic vcnl4000_lock. With commit
>> e21b5b1f2669 ("iio: light: vcnl4000: Preserve conf bits when toggle power")
>> the vcnl4040 sensor started to depend on the lock, but it was missed to
>> initialize it in vcnl4040's init function. This has not been visible
>> until we run lockdep on it:
>>
>>    DEBUG_LOCKS_WARN_ON(lock->magic != lock)
>>    at kernel/locking/mutex.c:575 __mutex_lock+0x4f8/0x890
>>    Call trace:
>>    __mutex_lock
>>    mutex_lock_nested
>>    vcnl4200_set_power_state
>>    vcnl4200_init
>>    vcnl4000_probe
>>    i2c_device_probe
>>    really_probe
>>    __driver_probe_device
>>    driver_probe_device
>>    __driver_attach
>>    bus_for_each_dev
>>    driver_attach
>>    bus_add_driver
>>    driver_register
>>    i2c_register_driver
>>    vcnl4000_driver_init
>>    do_one_initcall
>>    do_init_module
>>    load_module
>>    __do_sys_finit_module
>>
>> Fix this by initializing the lock in the probe function instead of doing
>> it in the chip specific init functions.
>>
>> Fixes: e21b5b1f2669 ("iio: light: vcnl4000: Preserve conf bits when toggle power")
>> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
>> ---
>>
>> v2:
>>   - Trimmed backtrace in commit message
>>   - Have 12 digit sha-1 id in Fixes tag
>>   - Make the lock initialization in probe instead of in _init function
>>
>>   drivers/iio/light/vcnl4000.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
>> index b5d398228289..caa2fff9f486 100644
>> --- a/drivers/iio/light/vcnl4000.c
>> +++ b/drivers/iio/light/vcnl4000.c
>> @@ -208,7 +208,6 @@ static int vcnl4000_init(struct vcnl4000_data *data)
>>   
>>   	data->rev = ret & 0xf;
>>   	data->al_scale = 250000;
>> -	mutex_init(&data->vcnl4000_lock);
>>   
>>   	return data->chip_spec->set_power_state(data, true);
>>   };
>> @@ -1366,6 +1365,7 @@ static int vcnl4000_probe(struct i2c_client *client,
>>   	data->client = client;
>>   	data->id = id->driver_data;
>>   	data->chip_spec = &vcnl4000_chip_spec_cfg[data->id];
>> +	mutex_init(&data->vcnl4000_lock);
>>   
>>   	ret = data->chip_spec->init(data);
>>   	if (ret < 0)
> Why not add mutex_init(&data->vcnl4000_lock) in vcnl4200_init.
> like this
>
> @@ -330,6 +330,7 @@ static int vcnl4200_init(struct vcnl4000_data *data)
>          }
>          mutex_init(&data->vcnl4200_al.lock);
>          mutex_init(&data->vcnl4200_ps.lock);
> +       mutex_init(&data->vcnl4000_lock);
>
>          ret = data->chip_spec->set_power_state(data, true);
>          if (ret < 0)
>
> Perfer adding mutex_init to vcnl4200_init.

Hi Cai!

This is what I did in v1, but I got a suggestion to move it to the probe 
instead.

Having it in probe takes one mutex_init. Otherwise it needs to be in two 
places (both init functions).

Kind regards

Mårten

>> -- 
>> 2.30.2
>>

Re: [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock

[Andy Shevchenko]

On Tue, Jan 31, 2023 at 02:33:38PM +0100, Mårten Lindahl wrote:
> On 1/31/23 13:46, Cai Huoqing wrote:
> > On 31 1月 23 11:29:51, Mårten Lindahl wrote:

...

> > Why not add mutex_init(&data->vcnl4000_lock) in vcnl4200_init.
> > like this
> > 
> > @@ -330,6 +330,7 @@ static int vcnl4200_init(struct vcnl4000_data *data)
> >          }
> >          mutex_init(&data->vcnl4200_al.lock);
> >          mutex_init(&data->vcnl4200_ps.lock);
> > +       mutex_init(&data->vcnl4000_lock);
> > 
> >          ret = data->chip_spec->set_power_state(data, true);
> >          if (ret < 0)
> > 
> > Perfer adding mutex_init to vcnl4200_init.
> 
> Hi Cai!
> 
> This is what I did in v1, but I got a suggestion to move it to the probe
> instead.
> 
> Having it in probe takes one mutex_init. Otherwise it needs to be in two
> places (both init functions).

Exactly and if one more device will be added, we won't miss it, so it's less
error prone.

-- 
With Best Regards,
Andy Shevchenko