* [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock
@ 2023-01-31 10:29 Mårten Lindahl
2023-01-31 12:33 ` Andy Shevchenko
2023-01-31 12:46 ` Cai Huoqing
0 siblings, 2 replies; 9+ messages in thread
From: Mårten Lindahl @ 2023-01-31 10:29 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Andy Shevchenko, linux-iio, kernel,
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock
2023-01-31 10:29 [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock Mårten Lindahl
@ 2023-01-31 12:33 ` Andy Shevchenko
2023-01-31 12:37 ` Andy Shevchenko
2023-01-31 13:27 ` Mårten Lindahl
2023-01-31 12:46 ` Cai Huoqing
1 sibling, 2 replies; 9+ messages in thread
From: Andy Shevchenko @ 2023-01-31 12:33 UTC (permalink / raw)
To: Mårten Lindahl
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio, kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock
2023-01-31 12:33 ` Andy Shevchenko
@ 2023-01-31 12:37 ` Andy Shevchenko
2023-01-31 13:27 ` Mårten Lindahl
1 sibling, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2023-01-31 12:37 UTC (permalink / raw)
To: Mårten Lindahl
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio, kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock
2023-01-31 10:29 [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock Mårten Lindahl
2023-01-31 12:33 ` Andy Shevchenko
@ 2023-01-31 12:46 ` Cai Huoqing
2023-01-31 13:33 ` Mårten Lindahl
1 sibling, 1 reply; 9+ messages in thread
From: Cai Huoqing @ 2023-01-31 12:46 UTC (permalink / raw)
To: Mårten Lindahl
Cc: Jonathan Cameron, Lars-Peter Clausen, Andy Shevchenko, linux-iio, kernel
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock
2023-01-31 12:33 ` Andy Shevchenko
2023-01-31 12:37 ` Andy Shevchenko
@ 2023-01-31 13:27 ` Mårten Lindahl
2023-01-31 13:37 ` Andy Shevchenko
1 sibling, 1 reply; 9+ messages in thread
From: Mårten Lindahl @ 2023-01-31 13:27 UTC (permalink / raw)
To: Andy Shevchenko, Mårten Lindahl
Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio, kernel
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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock
2023-01-31 12:46 ` Cai Huoqing
@ 2023-01-31 13:33 ` Mårten Lindahl
2023-01-31 13:36 ` Andy Shevchenko
0 siblings, 1 reply; 9+ messages in thread
From: Mårten Lindahl @ 2023-01-31 13:33 UTC (permalink / raw)
To: Cai Huoqing
Cc: Jonathan Cameron, Lars-Peter Clausen, Andy Shevchenko, linux-iio, kernel
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
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock
2023-01-31 13:33 ` Mårten Lindahl
@ 2023-01-31 13:36 ` Andy Shevchenko
0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2023-01-31 13:36 UTC (permalink / raw)
To: Mårten Lindahl
Cc: Cai Huoqing, Jonathan Cameron, Lars-Peter Clausen, linux-iio, kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock
2023-01-31 13:27 ` Mårten Lindahl
@ 2023-01-31 13:37 ` Andy Shevchenko
2023-01-31 13:42 ` Mårten Lindahl
0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2023-01-31 13:37 UTC (permalink / raw)
To: Mårten Lindahl
Cc: Mårten Lindahl, Jonathan Cameron, Lars-Peter Clausen,
linux-iio, kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock
2023-01-31 13:37 ` Andy Shevchenko
@ 2023-01-31 13:42 ` Mårten Lindahl
0 siblings, 0 replies; 9+ messages in thread
From: Mårten Lindahl @ 2023-01-31 13:42 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Jonathan Cameron, Lars-Peter Clausen, linux-iio, kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-01-31 13:42 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-31 10:29 [PATCH v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock Mårten Lindahl
2023-01-31 12:33 ` Andy Shevchenko
2023-01-31 12:37 ` Andy Shevchenko
2023-01-31 13:27 ` Mårten Lindahl
2023-01-31 13:37 ` Andy Shevchenko
2023-01-31 13:42 ` Mårten Lindahl
2023-01-31 12:46 ` Cai Huoqing
2023-01-31 13:33 ` Mårten Lindahl
2023-01-31 13:36 ` Andy Shevchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.