| Message ID | 20230131102951.2012021-1-marten.lindahl@axis.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] iio: light: vcnl4000: Fix WARN_ON on uninitialized lock | expand |
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)
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.
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 >
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)
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 >>
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.
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
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
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)
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(-)