There are some small bugs in the twl6040-vibra driver and differences to how twl4030 vibra works.
This patch series addresses them.
H. Nikolaus Schaller (5): input: twl6040-vibra: fix DT node memory management input: twl6040-vibra: add handler to unregister input if module is removed input: twl6040-vibra: fix NULL pointer dereference by removing workqueue input: twl6040-vibra: ignore return value of schedule_work input: twl6040-vibra: remove mutex
drivers/input/misc/twl6040-vibra.c | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-)
commit e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")
made the separate vibra DT node to a subnode of the twl6040.
It now calls of_find_node_by_name() to locate the "vibra" subnode. This function has a side effect to call of_node_put on() for the twl6040 parent node passed in as a parameter. This causes trouble later on.
Solution: we must call of_node_get() before of_find_node_by_name()
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com --- drivers/input/misc/twl6040-vibra.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c index ea63fad..7221a00 100644 --- a/drivers/input/misc/twl6040-vibra.c +++ b/drivers/input/misc/twl6040-vibra.c @@ -262,6 +262,7 @@ static int twl6040_vibra_probe(struct platform_device *pdev) int vddvibr_uV = 0; int error;
+ of_node_get(twl6040_core_dev->of_node); twl6040_core_node = of_find_node_by_name(twl6040_core_dev->of_node, "vibra"); if (!twl6040_core_node) {
On Mon, Apr 18, 2016 at 09:55:37PM +0200, H. Nikolaus Schaller wrote:
commit e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")
made the separate vibra DT node to a subnode of the twl6040.
It now calls of_find_node_by_name() to locate the "vibra" subnode. This function has a side effect to call of_node_put on() for the twl6040 parent node passed in as a parameter. This causes trouble later on.
Solution: we must call of_node_get() before of_find_node_by_name()
God, what messed up API. Any chance we can make it a bit more sane and not drop the reference inside it instead?
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com
drivers/input/misc/twl6040-vibra.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c index ea63fad..7221a00 100644 --- a/drivers/input/misc/twl6040-vibra.c +++ b/drivers/input/misc/twl6040-vibra.c @@ -262,6 +262,7 @@ static int twl6040_vibra_probe(struct platform_device *pdev) int vddvibr_uV = 0; int error;
- of_node_get(twl6040_core_dev->of_node); twl6040_core_node = of_find_node_by_name(twl6040_core_dev->of_node, "vibra"); if (!twl6040_core_node) {
-- 2.7.3
Am 18.04.2016 um 23:22 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Mon, Apr 18, 2016 at 09:55:37PM +0200, H. Nikolaus Schaller wrote:
commit e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")
made the separate vibra DT node to a subnode of the twl6040.
It now calls of_find_node_by_name() to locate the "vibra" subnode. This function has a side effect to call of_node_put on() for the twl6040 parent node passed in as a parameter. This causes trouble later on.
Solution: we must call of_node_get() before of_find_node_by_name()
God, what messed up API.
Yes, indeed. It is opposite to the usual object ownership rule that the code fragment that asks for a handle has to release it.
Usually it does not become obvious because often CONFIG_OF_DYNAMIC=n. This disables all of_node refcounting completely so such bugs remain unnoticed.
Any chance we can make it a bit more sane and not drop the reference inside it instead?
Well, if you want to change ~2000 files, test on all platforms and ask Linus for agreement?
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com
drivers/input/misc/twl6040-vibra.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c index ea63fad..7221a00 100644 --- a/drivers/input/misc/twl6040-vibra.c +++ b/drivers/input/misc/twl6040-vibra.c @@ -262,6 +262,7 @@ static int twl6040_vibra_probe(struct platform_device *pdev) int vddvibr_uV = 0; int error;
- of_node_get(twl6040_core_dev->of_node); twl6040_core_node = of_find_node_by_name(twl6040_core_dev->of_node, "vibra"); if (!twl6040_core_node) {
-- 2.7.3
-- Dmitry
On Tue, Apr 19, 2016 at 09:43:08AM +0200, H. Nikolaus Schaller wrote:
Am 18.04.2016 um 23:22 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Mon, Apr 18, 2016 at 09:55:37PM +0200, H. Nikolaus Schaller wrote:
commit e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")
made the separate vibra DT node to a subnode of the twl6040.
It now calls of_find_node_by_name() to locate the "vibra" subnode. This function has a side effect to call of_node_put on() for the twl6040 parent node passed in as a parameter. This causes trouble later on.
Solution: we must call of_node_get() before of_find_node_by_name()
God, what messed up API.
Yes, indeed. It is opposite to the usual object ownership rule that the code fragment that asks for a handle has to release it.
Usually it does not become obvious because often CONFIG_OF_DYNAMIC=n. This disables all of_node refcounting completely so such bugs remain unnoticed.
Any chance we can make it a bit more sane and not drop the reference inside it instead?
Well, if you want to change ~2000 files, test on all platforms and ask Linus for agreement?
It's not that bad, let's see what DT maintainers say to the patch I posted...
Am 19.04.2016 um 19:06 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Tue, Apr 19, 2016 at 09:43:08AM +0200, H. Nikolaus Schaller wrote:
Am 18.04.2016 um 23:22 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Mon, Apr 18, 2016 at 09:55:37PM +0200, H. Nikolaus Schaller wrote:
commit e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")
made the separate vibra DT node to a subnode of the twl6040.
It now calls of_find_node_by_name() to locate the "vibra" subnode. This function has a side effect to call of_node_put on() for the twl6040 parent node passed in as a parameter. This causes trouble later on.
Solution: we must call of_node_get() before of_find_node_by_name()
God, what messed up API.
Yes, indeed. It is opposite to the usual object ownership rule that the code fragment that asks for a handle has to release it.
Usually it does not become obvious because often CONFIG_OF_DYNAMIC=n. This disables all of_node refcounting completely so such bugs remain unnoticed.
Any chance we can make it a bit more sane and not drop the reference inside it instead?
Well, if you want to change ~2000 files, test on all platforms and ask Linus for agreement?
It's not that bad, let's see what DT maintainers say to the patch I posted...
Thanks! Would make me more happy a well.
Nikolaus
Hi Dmitry,
Am 20.04.2016 um 11:03 schrieb H. Nikolaus Schaller hns@goldelico.com:
Am 19.04.2016 um 19:06 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Tue, Apr 19, 2016 at 09:43:08AM +0200, H. Nikolaus Schaller wrote:
Am 18.04.2016 um 23:22 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Mon, Apr 18, 2016 at 09:55:37PM +0200, H. Nikolaus Schaller wrote:
commit e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")
made the separate vibra DT node to a subnode of the twl6040.
It now calls of_find_node_by_name() to locate the "vibra" subnode. This function has a side effect to call of_node_put on() for the twl6040 parent node passed in as a parameter. This causes trouble later on.
Solution: we must call of_node_get() before of_find_node_by_name()
God, what messed up API.
Yes, indeed. It is opposite to the usual object ownership rule that the code fragment that asks for a handle has to release it.
Usually it does not become obvious because often CONFIG_OF_DYNAMIC=n. This disables all of_node refcounting completely so such bugs remain unnoticed.
Any chance we can make it a bit more sane and not drop the reference inside it instead?
Well, if you want to change ~2000 files, test on all platforms and ask Linus for agreement?
It's not that bad, let's see what DT maintainers say to the patch I posted...
Thanks! Would make me more happy a well.
Any progress on this?
BR, Nikolaus
On Sun, May 08, 2016 at 08:49:27AM +0200, H. Nikolaus Schaller wrote:
Hi Dmitry,
Am 20.04.2016 um 11:03 schrieb H. Nikolaus Schaller hns@goldelico.com:
Am 19.04.2016 um 19:06 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Tue, Apr 19, 2016 at 09:43:08AM +0200, H. Nikolaus Schaller wrote:
Am 18.04.2016 um 23:22 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Mon, Apr 18, 2016 at 09:55:37PM +0200, H. Nikolaus Schaller wrote:
commit e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")
made the separate vibra DT node to a subnode of the twl6040.
It now calls of_find_node_by_name() to locate the "vibra" subnode. This function has a side effect to call of_node_put on() for the twl6040 parent node passed in as a parameter. This causes trouble later on.
Solution: we must call of_node_get() before of_find_node_by_name()
God, what messed up API.
Yes, indeed. It is opposite to the usual object ownership rule that the code fragment that asks for a handle has to release it.
Usually it does not become obvious because often CONFIG_OF_DYNAMIC=n. This disables all of_node refcounting completely so such bugs remain unnoticed.
Any chance we can make it a bit more sane and not drop the reference inside it instead?
Well, if you want to change ~2000 files, test on all platforms and ask Linus for agreement?
It's not that bad, let's see what DT maintainers say to the patch I posted...
Thanks! Would make me more happy a well.
Any progress on this?
I'll apply your patch for now and then will try to get mine worked in.
Thanks.
commit 1f9e1470ab34 ("Input: twl6040-vibra - use devm functions")
converted everything to devm but we still need to call input_unregister_device(info->input_dev)
Solution: add back twl6040_vibra_remove to call input_unregister_device
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com --- drivers/input/misc/twl6040-vibra.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c index 7221a00..1e9902d 100644 --- a/drivers/input/misc/twl6040-vibra.c +++ b/drivers/input/misc/twl6040-vibra.c @@ -384,8 +384,18 @@ static int twl6040_vibra_probe(struct platform_device *pdev) return 0; }
+static int twl6040_vibra_remove(struct platform_device *pdev) +{ + struct vibra_info *info = platform_get_drvdata(pdev); + + input_unregister_device(info->input_dev); + + return 0; + } + static struct platform_driver twl6040_vibra_driver = { .probe = twl6040_vibra_probe, + .remove = twl6040_vibra_remove, .driver = { .name = "twl6040-vibra", .pm = &twl6040_vibra_pm_ops,
On Mon, Apr 18, 2016 at 09:55:38PM +0200, H. Nikolaus Schaller wrote:
commit 1f9e1470ab34 ("Input: twl6040-vibra - use devm functions")
converted everything to devm but we still need to call input_unregister_device(info->input_dev)
No, this is not needed, because devm-managed input devices will be unregistered automatically.
Solution: add back twl6040_vibra_remove to call input_unregister_device
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com
drivers/input/misc/twl6040-vibra.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c index 7221a00..1e9902d 100644 --- a/drivers/input/misc/twl6040-vibra.c +++ b/drivers/input/misc/twl6040-vibra.c @@ -384,8 +384,18 @@ static int twl6040_vibra_probe(struct platform_device *pdev) return 0; }
+static int twl6040_vibra_remove(struct platform_device *pdev) +{
- struct vibra_info *info = platform_get_drvdata(pdev);
- input_unregister_device(info->input_dev);
- return 0;
- }
static struct platform_driver twl6040_vibra_driver = { .probe = twl6040_vibra_probe,
- .remove = twl6040_vibra_remove, .driver = { .name = "twl6040-vibra", .pm = &twl6040_vibra_pm_ops,
-- 2.7.3
Thanks.
Am 18.04.2016 um 23:12 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Mon, Apr 18, 2016 at 09:55:38PM +0200, H. Nikolaus Schaller wrote:
commit 1f9e1470ab34 ("Input: twl6040-vibra - use devm functions")
converted everything to devm but we still need to call input_unregister_device(info->input_dev)
No, this is not needed, because devm-managed input devices will be unregistered automatically.
Well, it would, if the current driver would use devm for registration. But it appears not to do it that way (line 375):
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers...
So we either have to fix line 375 or add the proposed explicit unregister call.
Research shows me that you did propose devm_input_register_device() some years ago but this API was never included:
https://lkml.org/lkml/2012/10/29/855
Solution: add back twl6040_vibra_remove to call input_unregister_device
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com
drivers/input/misc/twl6040-vibra.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c index 7221a00..1e9902d 100644 --- a/drivers/input/misc/twl6040-vibra.c +++ b/drivers/input/misc/twl6040-vibra.c @@ -384,8 +384,18 @@ static int twl6040_vibra_probe(struct platform_device *pdev) return 0; }
+static int twl6040_vibra_remove(struct platform_device *pdev) +{
- struct vibra_info *info = platform_get_drvdata(pdev);
- input_unregister_device(info->input_dev);
- return 0;
- }
static struct platform_driver twl6040_vibra_driver = { .probe = twl6040_vibra_probe,
- .remove = twl6040_vibra_remove, .driver = { .name = "twl6040-vibra", .pm = &twl6040_vibra_pm_ops,
-- 2.7.3
Thanks.
-- Dmitry
On Tue, Apr 19, 2016 at 09:33:10AM +0200, H. Nikolaus Schaller wrote:
Am 18.04.2016 um 23:12 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Mon, Apr 18, 2016 at 09:55:38PM +0200, H. Nikolaus Schaller wrote:
commit 1f9e1470ab34 ("Input: twl6040-vibra - use devm functions")
converted everything to devm but we still need to call input_unregister_device(info->input_dev)
No, this is not needed, because devm-managed input devices will be unregistered automatically.
Well, it would, if the current driver would use devm for registration. But it appears not to do it that way (line 375):
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers...
So we either have to fix line 375 or add the proposed explicit unregister call.
Research shows me that you did propose devm_input_register_device() some years ago but this API was never included:
That's because we have what I mentioned in _OR_ portion of that email:
/** * devm_input_allocate_device - allocate managed input device * @dev: device owning the input device being created * * Returns prepared struct input_dev or %NULL. * * Managed input devices do not need to be explicitly unregistered or ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ * freed as it will be done automatically when owner device unbinds from * its driver (or binding fails). Once managed input device is allocated, * it is ready to be set up and registered in the same fashion as regular * input device. There are no special devm_input_device_[un]register() * variants, regular ones work with both managed and unmanaged devices, * should you need them. In most cases however, managed input device need * not be explicitly unregistered or freed. * * NOTE: the owner device is set up as parent of input device and users * should not override it. */
Thanks.
Am 19.04.2016 um 09:57 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Tue, Apr 19, 2016 at 09:33:10AM +0200, H. Nikolaus Schaller wrote:
Am 18.04.2016 um 23:12 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Mon, Apr 18, 2016 at 09:55:38PM +0200, H. Nikolaus Schaller wrote:
commit 1f9e1470ab34 ("Input: twl6040-vibra - use devm functions")
converted everything to devm but we still need to call input_unregister_device(info->input_dev)
No, this is not needed, because devm-managed input devices will be unregistered automatically.
Well, it would, if the current driver would use devm for registration. But it appears not to do it that way (line 375):
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers...
So we either have to fix line 375 or add the proposed explicit unregister call.
Research shows me that you did propose devm_input_register_device() some years ago but this API was never included:
That's because we have what I mentioned in _OR_ portion of that email:
/**
- devm_input_allocate_device - allocate managed input device
- @dev: device owning the input device being created
- Returns prepared struct input_dev or %NULL.
- Managed input devices do not need to be explicitly unregistered or ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
- freed as it will be done automatically when owner device unbinds from
- its driver (or binding fails). Once managed input device is allocated,
- it is ready to be set up and registered in the same fashion as regular
- input device. There are no special devm_input_device_[un]register()
- variants, regular ones work with both managed and unmanaged devices,
- should you need them. In most cases however, managed input device need
- not be explicitly unregistered or freed.
- NOTE: the owner device is set up as parent of input device and users
- should not override it.
*/
Hm. If I remember correctly (it is a while ago) we had to add this explicit unregister mechanism because we got ugly kernel panics otherwise. Indicating that the input device was still registered and *not* unregistered automatically.
I will test.
BR and thanks, Nikolaus
On Tue, Apr 19, 2016 at 10:05:08AM +0200, H. Nikolaus Schaller wrote:
Am 19.04.2016 um 09:57 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Tue, Apr 19, 2016 at 09:33:10AM +0200, H. Nikolaus Schaller wrote:
Am 18.04.2016 um 23:12 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Mon, Apr 18, 2016 at 09:55:38PM +0200, H. Nikolaus Schaller wrote:
commit 1f9e1470ab34 ("Input: twl6040-vibra - use devm functions")
converted everything to devm but we still need to call input_unregister_device(info->input_dev)
No, this is not needed, because devm-managed input devices will be unregistered automatically.
Well, it would, if the current driver would use devm for registration. But it appears not to do it that way (line 375):
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers...
So we either have to fix line 375 or add the proposed explicit unregister call.
Research shows me that you did propose devm_input_register_device() some years ago but this API was never included:
That's because we have what I mentioned in _OR_ portion of that email:
/**
- devm_input_allocate_device - allocate managed input device
- @dev: device owning the input device being created
- Returns prepared struct input_dev or %NULL.
- Managed input devices do not need to be explicitly unregistered or ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
- freed as it will be done automatically when owner device unbinds from
- its driver (or binding fails). Once managed input device is allocated,
- it is ready to be set up and registered in the same fashion as regular
- input device. There are no special devm_input_device_[un]register()
- variants, regular ones work with both managed and unmanaged devices,
- should you need them. In most cases however, managed input device need
- not be explicitly unregistered or freed.
- NOTE: the owner device is set up as parent of input device and users
- should not override it.
*/
Hm. If I remember correctly (it is a while ago) we had to add this explicit unregister mechanism because we got ugly kernel panics otherwise. Indicating that the input device was still registered and *not* unregistered automatically.
Ah, it is probably because we are messing up with input device's parent. Can you please try the patch below?
Thanks.
Hi Dmitry,
Am 19.04.2016 um 18:53 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Tue, Apr 19, 2016 at 10:05:08AM +0200, H. Nikolaus Schaller wrote:
Am 19.04.2016 um 09:57 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Tue, Apr 19, 2016 at 09:33:10AM +0200, H. Nikolaus Schaller wrote:
Am 18.04.2016 um 23:12 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Mon, Apr 18, 2016 at 09:55:38PM +0200, H. Nikolaus Schaller wrote:
commit 1f9e1470ab34 ("Input: twl6040-vibra - use devm functions")
converted everything to devm but we still need to call input_unregister_device(info->input_dev)
No, this is not needed, because devm-managed input devices will be unregistered automatically.
Well, it would, if the current driver would use devm for registration. But it appears not to do it that way (line 375):
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers...
So we either have to fix line 375 or add the proposed explicit unregister call.
Research shows me that you did propose devm_input_register_device() some years ago but this API was never included:
That's because we have what I mentioned in _OR_ portion of that email:
/**
- devm_input_allocate_device - allocate managed input device
- @dev: device owning the input device being created
- Returns prepared struct input_dev or %NULL.
- Managed input devices do not need to be explicitly unregistered or ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
- freed as it will be done automatically when owner device unbinds from
- its driver (or binding fails). Once managed input device is allocated,
- it is ready to be set up and registered in the same fashion as regular
- input device. There are no special devm_input_device_[un]register()
- variants, regular ones work with both managed and unmanaged devices,
- should you need them. In most cases however, managed input device need
- not be explicitly unregistered or freed.
- NOTE: the owner device is set up as parent of input device and users
- should not override it.
*/
Hm. If I remember correctly (it is a while ago) we had to add this explicit unregister mechanism because we got ugly kernel panics otherwise. Indicating that the input device was still registered and *not* unregistered automatically.
Ah, it is probably because we are messing up with input device's parent. Can you please try the patch below?
Appears to have no new side-effects and the original problem does not appear again.
BR and thanks, Nikolaus
commit 21fb9f0d5e91 ("Input: twl6040-vibra - use system workqueue")
says that it switches to use the system workqueue but it did neither
- remove the workqueue struct variable - replace code to really use the system workqueue
Instead it calls queue_work() on uninitialized info->workqueue.
The result is a NULL pointer dereference in vibra_play().
Solution: use schedule_work
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com --- drivers/input/misc/twl6040-vibra.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c index 1e9902d..3805129 100644 --- a/drivers/input/misc/twl6040-vibra.c +++ b/drivers/input/misc/twl6040-vibra.c @@ -45,7 +45,6 @@ struct vibra_info { struct device *dev; struct input_dev *input_dev; - struct workqueue_struct *workqueue; struct work_struct play_work; struct mutex mutex; int irq; @@ -213,7 +212,7 @@ static int vibra_play(struct input_dev *input, void *data, info->strong_speed = effect->u.rumble.strong_magnitude; info->direction = effect->direction < EFFECT_DIR_180_DEG ? 1 : -1;
- ret = queue_work(info->workqueue, &info->play_work); + ret = schedule_work(&info->play_work); if (!ret) { dev_info(&input->dev, "work is already on queue\n"); return ret;
On Mon, Apr 18, 2016 at 09:55:39PM +0200, H. Nikolaus Schaller wrote:
commit 21fb9f0d5e91 ("Input: twl6040-vibra - use system workqueue")
says that it switches to use the system workqueue but it did neither
- remove the workqueue struct variable
- replace code to really use the system workqueue
Instead it calls queue_work() on uninitialized info->workqueue.
The result is a NULL pointer dereference in vibra_play().
Solution: use schedule_work
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com
Applied, thank you.
drivers/input/misc/twl6040-vibra.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c index 1e9902d..3805129 100644 --- a/drivers/input/misc/twl6040-vibra.c +++ b/drivers/input/misc/twl6040-vibra.c @@ -45,7 +45,6 @@ struct vibra_info { struct device *dev; struct input_dev *input_dev;
- struct workqueue_struct *workqueue; struct work_struct play_work; struct mutex mutex; int irq;
@@ -213,7 +212,7 @@ static int vibra_play(struct input_dev *input, void *data, info->strong_speed = effect->u.rumble.strong_magnitude; info->direction = effect->direction < EFFECT_DIR_180_DEG ? 1 : -1;
- ret = queue_work(info->workqueue, &info->play_work);
- ret = schedule_work(&info->play_work); if (!ret) { dev_info(&input->dev, "work is already on queue\n"); return ret;
-- 2.7.3
returning ret is wrong. And checking for an error as well. User space may call multiple times until the work is really scheduled.
twl4030-vibra.c also ignores the return value.
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com --- drivers/input/misc/twl6040-vibra.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c index 3805129..69c5940 100644 --- a/drivers/input/misc/twl6040-vibra.c +++ b/drivers/input/misc/twl6040-vibra.c @@ -212,11 +212,7 @@ static int vibra_play(struct input_dev *input, void *data, info->strong_speed = effect->u.rumble.strong_magnitude; info->direction = effect->direction < EFFECT_DIR_180_DEG ? 1 : -1;
- ret = schedule_work(&info->play_work); - if (!ret) { - dev_info(&input->dev, "work is already on queue\n"); - return ret; - } + schedule_work(&info->play_work);
return 0; }
On Mon, Apr 18, 2016 at 09:55:40PM +0200, H. Nikolaus Schaller wrote:
returning ret is wrong. And checking for an error as well. User space may call multiple times until the work is really scheduled.
twl4030-vibra.c also ignores the return value.
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com
Applied, thank you.
drivers/input/misc/twl6040-vibra.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c index 3805129..69c5940 100644 --- a/drivers/input/misc/twl6040-vibra.c +++ b/drivers/input/misc/twl6040-vibra.c @@ -212,11 +212,7 @@ static int vibra_play(struct input_dev *input, void *data, info->strong_speed = effect->u.rumble.strong_magnitude; info->direction = effect->direction < EFFECT_DIR_180_DEG ? 1 : -1;
- ret = schedule_work(&info->play_work);
- if (!ret) {
dev_info(&input->dev, "work is already on queue\n");
return ret;
- }
schedule_work(&info->play_work);
return 0;
}
2.7.3
The mutex does not seem to be needed. twl4030-vibra doesn't use one either.
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com --- drivers/input/misc/twl6040-vibra.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c index 69c5940..dcc926b 100644 --- a/drivers/input/misc/twl6040-vibra.c +++ b/drivers/input/misc/twl6040-vibra.c @@ -46,7 +46,7 @@ struct vibra_info { struct device *dev; struct input_dev *input_dev; struct work_struct play_work; - struct mutex mutex; + int irq;
bool enabled; @@ -182,8 +182,6 @@ static void vibra_play_work(struct work_struct *work) struct vibra_info *info = container_of(work, struct vibra_info, play_work);
- mutex_lock(&info->mutex); - if (info->weak_speed || info->strong_speed) { if (!info->enabled) twl6040_vibra_enable(info); @@ -192,7 +190,6 @@ static void vibra_play_work(struct work_struct *work) } else if (info->enabled) twl6040_vibra_disable(info);
- mutex_unlock(&info->mutex); }
static int vibra_play(struct input_dev *input, void *data, @@ -223,12 +220,8 @@ static void twl6040_vibra_close(struct input_dev *input)
cancel_work_sync(&info->play_work);
- mutex_lock(&info->mutex); - if (info->enabled) twl6040_vibra_disable(info); - - mutex_unlock(&info->mutex); }
static int __maybe_unused twl6040_vibra_suspend(struct device *dev) @@ -236,13 +229,9 @@ static int __maybe_unused twl6040_vibra_suspend(struct device *dev) struct platform_device *pdev = to_platform_device(dev); struct vibra_info *info = platform_get_drvdata(pdev);
- mutex_lock(&info->mutex); - if (info->enabled) twl6040_vibra_disable(info);
- mutex_unlock(&info->mutex); - return 0; }
@@ -301,8 +290,6 @@ static int twl6040_vibra_probe(struct platform_device *pdev) return -EINVAL; }
- mutex_init(&info->mutex); - error = devm_request_threaded_irq(&pdev->dev, info->irq, NULL, twl6040_vib_irq_handler, IRQF_ONESHOT,
On Mon, Apr 18, 2016 at 09:55:41PM +0200, H. Nikolaus Schaller wrote:
The mutex does not seem to be needed.
twl6040_vibra_suspend() and vibra_play_work() may run concurrently, no?
twl4030-vibra doesn't use one either.
It probably should.
Thanks.
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com
drivers/input/misc/twl6040-vibra.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c index 69c5940..dcc926b 100644 --- a/drivers/input/misc/twl6040-vibra.c +++ b/drivers/input/misc/twl6040-vibra.c @@ -46,7 +46,7 @@ struct vibra_info { struct device *dev; struct input_dev *input_dev; struct work_struct play_work;
- struct mutex mutex;
int irq;
bool enabled;
@@ -182,8 +182,6 @@ static void vibra_play_work(struct work_struct *work) struct vibra_info *info = container_of(work, struct vibra_info, play_work);
- mutex_lock(&info->mutex);
- if (info->weak_speed || info->strong_speed) { if (!info->enabled) twl6040_vibra_enable(info);
@@ -192,7 +190,6 @@ static void vibra_play_work(struct work_struct *work) } else if (info->enabled) twl6040_vibra_disable(info);
- mutex_unlock(&info->mutex);
}
static int vibra_play(struct input_dev *input, void *data, @@ -223,12 +220,8 @@ static void twl6040_vibra_close(struct input_dev *input)
cancel_work_sync(&info->play_work);
- mutex_lock(&info->mutex);
- if (info->enabled) twl6040_vibra_disable(info);
- mutex_unlock(&info->mutex);
}
static int __maybe_unused twl6040_vibra_suspend(struct device *dev) @@ -236,13 +229,9 @@ static int __maybe_unused twl6040_vibra_suspend(struct device *dev) struct platform_device *pdev = to_platform_device(dev); struct vibra_info *info = platform_get_drvdata(pdev);
mutex_lock(&info->mutex);
if (info->enabled) twl6040_vibra_disable(info);
mutex_unlock(&info->mutex);
return 0;
}
@@ -301,8 +290,6 @@ static int twl6040_vibra_probe(struct platform_device *pdev) return -EINVAL; }
- mutex_init(&info->mutex);
- error = devm_request_threaded_irq(&pdev->dev, info->irq, NULL, twl6040_vib_irq_handler, IRQF_ONESHOT,
-- 2.7.3
Am 18.04.2016 um 23:20 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Mon, Apr 18, 2016 at 09:55:41PM +0200, H. Nikolaus Schaller wrote:
The mutex does not seem to be needed.
twl6040_vibra_suspend() and vibra_play_work() may run concurrently, no?
Hm. I don't know about the rule that would give an answer to this question...
I will test a little to see if the mutex has unexpected side-effects.
twl4030-vibra doesn't use one either.
It probably should.
Thanks.
Signed-off-by: H. Nikolaus Schaller hns@goldelico.com
drivers/input/misc/twl6040-vibra.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/drivers/input/misc/twl6040-vibra.c b/drivers/input/misc/twl6040-vibra.c index 69c5940..dcc926b 100644 --- a/drivers/input/misc/twl6040-vibra.c +++ b/drivers/input/misc/twl6040-vibra.c @@ -46,7 +46,7 @@ struct vibra_info { struct device *dev; struct input_dev *input_dev; struct work_struct play_work;
- struct mutex mutex;
int irq;
bool enabled;
@@ -182,8 +182,6 @@ static void vibra_play_work(struct work_struct *work) struct vibra_info *info = container_of(work, struct vibra_info, play_work);
- mutex_lock(&info->mutex);
- if (info->weak_speed || info->strong_speed) { if (!info->enabled) twl6040_vibra_enable(info);
@@ -192,7 +190,6 @@ static void vibra_play_work(struct work_struct *work) } else if (info->enabled) twl6040_vibra_disable(info);
- mutex_unlock(&info->mutex);
}
static int vibra_play(struct input_dev *input, void *data, @@ -223,12 +220,8 @@ static void twl6040_vibra_close(struct input_dev *input)
cancel_work_sync(&info->play_work);
- mutex_lock(&info->mutex);
- if (info->enabled) twl6040_vibra_disable(info);
- mutex_unlock(&info->mutex);
}
static int __maybe_unused twl6040_vibra_suspend(struct device *dev) @@ -236,13 +229,9 @@ static int __maybe_unused twl6040_vibra_suspend(struct device *dev) struct platform_device *pdev = to_platform_device(dev); struct vibra_info *info = platform_get_drvdata(pdev);
mutex_lock(&info->mutex);
if (info->enabled) twl6040_vibra_disable(info);
mutex_unlock(&info->mutex);
return 0;
}
@@ -301,8 +290,6 @@ static int twl6040_vibra_probe(struct platform_device *pdev) return -EINVAL; }
- mutex_init(&info->mutex);
- error = devm_request_threaded_irq(&pdev->dev, info->irq, NULL, twl6040_vib_irq_handler, IRQF_ONESHOT,
-- 2.7.3
-- Dmitry
On Tue, Apr 19, 2016 at 09:49:01AM +0200, H. Nikolaus Schaller wrote:
Am 18.04.2016 um 23:20 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Mon, Apr 18, 2016 at 09:55:41PM +0200, H. Nikolaus Schaller wrote:
The mutex does not seem to be needed.
twl6040_vibra_suspend() and vibra_play_work() may run concurrently, no?
Hm. I don't know about the rule that would give an answer to this question...
Sorry, that was actually a statement, not really a question. It is possible (although very unlikely) that userspace posts play request and workqueue will not run until after suspend callback.
Thinking about it some more I wonder if we better do what twl6040_vibra_close() does and cancel the work before shutting off the device, so that there is no chance of work executing after suspend callback and reenabling the device. This way we can indeed remove the mutex.
Thanks.
Am 19.04.2016 um 10:01 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Tue, Apr 19, 2016 at 09:49:01AM +0200, H. Nikolaus Schaller wrote:
Am 18.04.2016 um 23:20 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Mon, Apr 18, 2016 at 09:55:41PM +0200, H. Nikolaus Schaller wrote:
The mutex does not seem to be needed.
twl6040_vibra_suspend() and vibra_play_work() may run concurrently, no?
Hm. I don't know about the rule that would give an answer to this question...
Sorry, that was actually a statement, not really a question.
Indeed. In doubt about the answer we should take measures for the worst case.
It is possible (although very unlikely) that userspace posts play request and workqueue will not run until after suspend callback.
Thinking about it some more I wonder if we better do what twl6040_vibra_close() does and cancel the work before shutting off the device, so that there is no chance of work executing after suspend callback and reenabling the device. This way we can indeed remove the mutex.
Ok, I am fine with this.
Will post an update ASAP.
BR and thanks, Nikolaus
Am 19.04.2016 um 10:08 schrieb H. Nikolaus Schaller hns@goldelico.com:
Am 19.04.2016 um 10:01 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Tue, Apr 19, 2016 at 09:49:01AM +0200, H. Nikolaus Schaller wrote:
Am 18.04.2016 um 23:20 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Mon, Apr 18, 2016 at 09:55:41PM +0200, H. Nikolaus Schaller wrote:
The mutex does not seem to be needed.
twl6040_vibra_suspend() and vibra_play_work() may run concurrently, no?
Hm. I don't know about the rule that would give an answer to this question...
Sorry, that was actually a statement, not really a question.
Indeed. In doubt about the answer we should take measures for the worst case.
It is possible (although very unlikely) that userspace posts play request and workqueue will not run until after suspend callback.
Thinking about it some more I wonder if we better do what twl6040_vibra_close() does and cancel the work before shutting off the device, so that there is no chance of work executing after suspend callback and reenabling the device. This way we can indeed remove the mutex.
Ok, I am fine with this.
Will post an update ASAP.
While doing testing I did run again into another locking related issue which I had not yet tried to address with my patch set. It is a:
BUG: scheduling while atomic
report which sometimes ends in a kernel panic.
I have attached such a log. vibra.py is here:
http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=Letux/root/vibra.py;hb...
Basically it does an ioctl(EVIOCSFF) which triggers vibra_play.
Maybe, can you decipher from the log what the reason could be?
I only conjecture that it happens when vibra_play tries to read the regmap of the twl6040 to get twl6040_get_vibralr_status (which has no pendant in the twl4030 driver).
Do we have to configure the twl6040 regmap differently?
BR and thanks, Nikolaus
root@letux:~# ./vibra.py [ 503.058501] BUG: scheduling while atomic: python/2591/0x00000002 [ 503.064866] 4 locks held by python/2591: [ 503.069010] #0: (&evdev->mutex){+.+.+.}, at: [<c068e9b0>] evdev_write+0x38/0xc0 [ 503.077000] #1: (&(&dev->event_lock)->rlock){......}, at: [<c0688ba4>] input_inject_event+0x40/0x1ac [ 503.086913] #2: (rcu_read_lock){......}, at: [<c0688b98>] input_inject_event+0x34/0x1ac [ 503.095616] #3: (twl6040:642:(&twl6040_regmap_config)->lock){+.+...}, at: [<c05c5500>] regmap_read+0x30/0x58 [ 503.106269] Modules linked in: bluetooth autofs4 usb_f_ecm g_ether usb_f_rndis u_ether libcomposite configfs ipv6 arc4 wl18xx wlcore mac80211 omapdrm cfg80211 drm_kms_helper cfbfillrect syscopyarea cfbimgblt sysfillrect snd_soc_omap_hdmi_audio sysimgblt panel_mipi_debug fb_sys_fops dwc3 cfbcopyarea drm connector_hdmi encoder_tpd12s015 snd_soc_omap_abe_twl6040 w2cbw003_bluetooth snd_soc_twl6040 omapdss wwan_on_off leds_gpio wlcore_sdio pwm_bl pwm_omap_dmtimer ehci_omap dwc3_omap snd_soc_ts3a225e leds_is31fl319x leds_tca6507 tsc2007 bq27xxx_battery_i2c bq2429x_charger bq27xxx_battery gpio_twl6040 twl6040_vibra ina2xx palmas_gpadc tca8418_keypad bmp085_i2c palmas_pwrbutton as5013 bmg160_i2c usb3503 bmp280 bmp085 bma150 bmg160_core input_polldev snd_soc_omap_mcbsp snd_soc_omap_mcpdm snd_soc_omap snd_pcm_dmaengine [ 503.182770] irq event stamp: 29172 [ 503.186366] hardirqs last enabled at (29171): [<c0809f94>] _raw_spin_unlock_irq+0x24/0x60 [ 503.195114] hardirqs last disabled at (29172): [<c08097e8>] _raw_spin_lock_irqsave+0x18/0x54 [ 503.204028] softirqs last enabled at (29154): [<c024ff80>] __do_softirq+0x5bc/0x66c [ 503.212216] softirqs last disabled at (29041): [<c0250328>] irq_exit+0x98/0x118 [ 503.219954] Preemption disabled at:[< (null)>] (null) [ 503.225571] [ 503.227162] CPU: 1 PID: 2591 Comm: python Tainted: G W 4.6.0-rc4-letux+ #69 [ 503.235693] Hardware name: Generic OMAP5 (Flattened Device Tree) [ 503.242062] [<c022789c>] (unwind_backtrace) from [<c022403c>] (show_stack+0x10/0x14) [ 503.250259] [<c022403c>] (show_stack) from [<c0517e44>] (dump_stack+0x88/0xc0) [ 503.257897] [<c0517e44>] (dump_stack) from [<c0318e38>] (__schedule_bug+0xa8/0xcc) [ 503.265900] [<c0318e38>] (__schedule_bug) from [<c08042e4>] (__schedule+0x70/0x7fc) [ 503.273993] [<c08042e4>] (__schedule) from [<c0804ca8>] (schedule+0x9c/0xc0) [ 503.281451] [<c0804ca8>] (schedule) from [<c0804ed4>] (schedule_preempt_disabled+0x14/0x20) [ 503.290278] [<c0804ed4>] (schedule_preempt_disabled) from [<c0806ab4>] (mutex_lock_nested+0x258/0x43c) [ 503.300115] [<c0806ab4>] (mutex_lock_nested) from [<c05c5500>] (regmap_read+0x30/0x58) [ 503.308488] [<c05c5500>] (regmap_read) from [<c05d5208>] (twl6040_get_vibralr_status+0x18/0x48) [ 503.317680] [<c05d5208>] (twl6040_get_vibralr_status) from [<bf080674>] (vibra_play+0x18/0x7c [twl6040_vibra]) [ 503.328333] [<bf080674>] (vibra_play [twl6040_vibra]) from [<c068a94c>] (ml_play_effects+0x34/0x5c) [ 503.337897] [<c068a94c>] (ml_play_effects) from [<c068aa2c>] (ml_ff_playback+0x88/0x94) [ 503.346359] [<c068aa2c>] (ml_ff_playback) from [<c0689bf4>] (input_ff_event+0x9c/0xa4) [ 503.354722] [<c0689bf4>] (input_ff_event) from [<c0688a7c>] (input_handle_event+0x4c/0x134) [ 503.363543] [<c0688a7c>] (input_handle_event) from [<c0688c7c>] (input_inject_event+0x118/0x1ac) [ 503.372824] [<c0688c7c>] (input_inject_event) from [<c068ea00>] (evdev_write+0x88/0xc0) [ 503.381281] [<c068ea00>] (evdev_write) from [<c036eff8>] (__vfs_write+0x18/0x38) [ 503.389092] [<c036eff8>] (__vfs_write) from [<c036fdc8>] (vfs_write+0xac/0x130) [ 503.396819] [<c036fdc8>] (vfs_write) from [<c036ffec>] (SyS_write+0x40/0x78) [ 503.404275] [<c036ffec>] (SyS_write) from [<c0220740>] (ret_fast_syscall+0x0/0x1c) [ 503.414230] [ 503.415818] ========================================================= [ 503.422608] [ INFO: possible irq lock inversion dependency detected ] [ 503.429403] 4.6.0-rc4-letux+ #69 Tainted: G W [ 503.435207] --------------------------------------------------------- [ 503.441998] swapper/1/0 just changed the state of lock: [ 503.447510] (&(&dev->event_lock)->rlock){..-...}, at: [<c068aa50>] ml_effect_timer+0x18/0x34 [ 503.456578] but this lock took another, SOFTIRQ-unsafe lock in the past: [ 503.463646] (twl6040:642:(&twl6040_regmap_config)->lock){+.+...}
and interrupts could create inverse lock ordering between them.
[ 503.476449] [ 503.476449] other info that might help us debug this: [ 503.483339] Possible interrupt unsafe locking scenario: [ 503.483339] [ 503.490516] CPU0 CPU1 [ 503.495307] ---- ---- [ 503.500094] lock(twl6040:642:(&twl6040_regmap_config)->lock); [ 503.506370] local_irq_disable(); [ 503.512627] lock(&(&dev->event_lock)->rlock); [ 503.520092] lock(twl6040:642:(&twl6040_regmap_config)->lock); [ 503.529020] <Interrupt> [ 503.531793] lock(&(&dev->event_lock)->rlock); [ 503.536781] [ 503.536781] *** DEADLOCK *** [ 503.536781] [ 503.543033] 1 lock held by swapper/1/0: [ 503.547090] #0: (((&ml->timer))){+.-...}, at: [<c02b75b8>] call_timer_fn+0x0/0x47c [ 503.555344] [ 503.555344] the shortest dependencies between 2nd lock and 1st lock: [ 503.563643] -> (twl6040:642:(&twl6040_regmap_config)->lock){+.+...} ops: 93 { [ 503.571339] HARDIRQ-ON-W at: [ 503.574757] [<c0299fc8>] lock_acquire+0x25c/0x290 [ 503.581785] [<c08068a8>] mutex_lock_nested+0x4c/0x43c [ 503.589162] [<c05c78f8>] regmap_register_patch+0xa0/0xfc [ 503.596819] [<c05d53a4>] twl6040_probe+0x150/0x414 [ 503.603917] [<c0698dc8>] i2c_device_probe+0x198/0x1ec [ 503.611304] [<c05aa3ac>] really_probe+0xfc/0x258 [ 503.618227] [<c05aa6ac>] driver_probe_device+0x44/0x70 [ 503.625706] [<c05a8e2c>] bus_for_each_drv+0x4c/0x84 [ 503.632916] [<c05aa5d4>] __device_attach+0x84/0xf8 [ 503.640025] [<c05a99e8>] bus_probe_device+0x28/0x84 [ 503.647218] [<c05a8068>] device_add+0x204/0x34c [ 503.654050] [<c0699e7c>] i2c_new_device+0x10c/0x18c [ 503.661253] [<c069a1d8>] of_i2c_register_device+0x15c/0x180 [ 503.669187] [<c069a49c>] i2c_register_adapter+0x1e0/0x30c [ 503.676947] [<c069e8e4>] omap_i2c_probe+0x330/0x42c [ 503.684148] [<c05ac2d8>] platform_drv_probe+0x50/0x98 [ 503.691531] [<c05aa3ac>] really_probe+0xfc/0x258 [ 503.698455] [<c05aa6ac>] driver_probe_device+0x44/0x70 [ 503.705926] [<c05a8e2c>] bus_for_each_drv+0x4c/0x84 [ 503.713119] [<c05aa5d4>] __device_attach+0x84/0xf8 [ 503.720221] [<c05a99e8>] bus_probe_device+0x28/0x84 [ 503.727418] [<c05a9e50>] deferred_probe_work_func+0x5c/0x8c [ 503.735351] [<c02653c8>] process_one_work+0x3fc/0x770 [ 503.742738] [<c0265964>] worker_thread+0x1f8/0x2fc [ 503.749840] [<c026ad08>] kthread+0xdc/0xf0 [ 503.756210] [<c02207c8>] ret_from_fork+0x14/0x2c [ 503.763126] SOFTIRQ-ON-W at: [ 503.766544] [<c0299fc8>] lock_acquire+0x25c/0x290 [ 503.773562] [<c08068a8>] mutex_lock_nested+0x4c/0x43c [ 503.780953] [<c05c78f8>] regmap_register_patch+0xa0/0xfc [ 503.788594] [<c05d53a4>] twl6040_probe+0x150/0x414 [ 503.795698] [<c0698dc8>] i2c_device_probe+0x198/0x1ec [ 503.803069] [<c05aa3ac>] really_probe+0xfc/0x258 [ 503.809972] [<c05aa6ac>] driver_probe_device+0x44/0x70 [ 503.817440] [<c05a8e2c>] bus_for_each_drv+0x4c/0x84 [ 503.824627] [<c05aa5d4>] __device_attach+0x84/0xf8 [ 503.831726] [<c05a99e8>] bus_probe_device+0x28/0x84 [ 503.838929] [<c05a8068>] device_add+0x204/0x34c [ 503.845762] [<c0699e7c>] i2c_new_device+0x10c/0x18c [ 503.852949] [<c069a1d8>] of_i2c_register_device+0x15c/0x180 [ 503.860874] [<c069a49c>] i2c_register_adapter+0x1e0/0x30c [ 503.868634] [<c069e8e4>] omap_i2c_probe+0x330/0x42c [ 503.875836] [<c05ac2d8>] platform_drv_probe+0x50/0x98 [ 503.883211] [<c05aa3ac>] really_probe+0xfc/0x258 [ 503.890130] [<c05aa6ac>] driver_probe_device+0x44/0x70 [ 503.897602] [<c05a8e2c>] bus_for_each_drv+0x4c/0x84 [ 503.904794] [<c05aa5d4>] __device_attach+0x84/0xf8 [ 503.911903] [<c05a99e8>] bus_probe_device+0x28/0x84 [ 503.919104] [<c05a9e50>] deferred_probe_work_func+0x5c/0x8c [ 503.927034] [<c02653c8>] process_one_work+0x3fc/0x770 [ 503.934418] [<c0265964>] worker_thread+0x1f8/0x2fc [ 503.941537] [<c026ad08>] kthread+0xdc/0xf0 [ 503.947902] [<c02207c8>] ret_from_fork+0x14/0x2c [ 503.954815] INITIAL USE at: [ 503.958145] [<c08068a8>] mutex_lock_nested+0x4c/0x43c [ 503.965433] [<c05c78f8>] regmap_register_patch+0xa0/0xfc [ 503.973003] [<c05d53a4>] twl6040_probe+0x150/0x414 [ 503.980022] [<c0698dc8>] i2c_device_probe+0x198/0x1ec [ 503.987314] [<c05aa3ac>] really_probe+0xfc/0x258 [ 503.994145] [<c05aa6ac>] driver_probe_device+0x44/0x70 [ 504.001518] [<c05a8e2c>] bus_for_each_drv+0x4c/0x84 [ 504.008629] [<c05aa5d4>] __device_attach+0x84/0xf8 [ 504.015643] [<c05a99e8>] bus_probe_device+0x28/0x84 [ 504.022734] [<c05a8068>] device_add+0x204/0x34c [ 504.029475] [<c0699e7c>] i2c_new_device+0x10c/0x18c [ 504.036577] [<c069a1d8>] of_i2c_register_device+0x15c/0x180 [ 504.044421] [<c069a49c>] i2c_register_adapter+0x1e0/0x30c [ 504.052072] [<c069e8e4>] omap_i2c_probe+0x330/0x42c [ 504.059173] [<c05ac2d8>] platform_drv_probe+0x50/0x98 [ 504.066472] [<c05aa3ac>] really_probe+0xfc/0x258 [ 504.073285] [<c05aa6ac>] driver_probe_device+0x44/0x70 [ 504.080657] [<c05a8e2c>] bus_for_each_drv+0x4c/0x84 [ 504.087760] [<c05aa5d4>] __device_attach+0x84/0xf8 [ 504.094773] [<c05a99e8>] bus_probe_device+0x28/0x84 [ 504.101880] [<c05a9e50>] deferred_probe_work_func+0x5c/0x8c [ 504.109712] [<c02653c8>] process_one_work+0x3fc/0x770 [ 504.117007] [<c0265964>] worker_thread+0x1f8/0x2fc [ 504.124021] [<c026ad08>] kthread+0xdc/0xf0 [ 504.130299] [<c02207c8>] ret_from_fork+0x14/0x2c [ 504.137126] } [ 504.138974] ... key at: [<c186b9e0>] _key.26438+0x0/0x8 [ 504.145144] ... acquired at: [ 504.148371] [<c0299748>] __lock_acquire+0x7d0/0x8d0 [ 504.153806] [<c0299fc8>] lock_acquire+0x25c/0x290 [ 504.159060] [<c08068a8>] mutex_lock_nested+0x4c/0x43c [ 504.164674] [<c05c5500>] regmap_read+0x30/0x58 [ 504.169655] [<c05d5208>] twl6040_get_vibralr_status+0x18/0x48 [ 504.176007] [<bf080674>] vibra_play+0x18/0x7c [twl6040_vibra] [ 504.182368] [<c068a94c>] ml_play_effects+0x34/0x5c [ 504.187726] [<c068aa2c>] ml_ff_playback+0x88/0x94 [ 504.192985] [<c0689bf4>] input_ff_event+0x9c/0xa4 [ 504.198246] [<c0688a7c>] input_handle_event+0x4c/0x134 [ 504.203968] [<c0688c7c>] input_inject_event+0x118/0x1ac [ 504.209777] [<c068ea00>] evdev_write+0x88/0xc0 [ 504.214755] [<c036eff8>] __vfs_write+0x18/0x38 [ 504.219735] [<c036fdc8>] vfs_write+0xac/0x130 [ 504.224623] [<c036ffec>] SyS_write+0x40/0x78 [ 504.229417] [<c0220740>] ret_fast_syscall+0x0/0x1c [ 504.234763] [ 504.236341] -> (&(&dev->event_lock)->rlock){..-...} ops: 95 { [ 504.242479] IN-SOFTIRQ-W at: [ 504.245808] [<c0299fc8>] lock_acquire+0x25c/0x290 [ 504.252636] [<c0809810>] _raw_spin_lock_irqsave+0x40/0x54 [ 504.260195] [<c068aa50>] ml_effect_timer+0x18/0x34 [ 504.267124] [<c02b77dc>] call_timer_fn+0x224/0x47c [ 504.274044] [<c02b7dc4>] run_timer_softirq+0x390/0x3f8 [ 504.281320] [<c024fca8>] __do_softirq+0x2e4/0x66c [ 504.288147] [<c0250328>] irq_exit+0x98/0x118 [ 504.294521] [<c02a5a78>] __handle_domain_irq+0xa0/0xdc [ 504.301801] [<c0201578>] gic_handle_irq+0x50/0x8c [ 504.308629] [<c080af04>] __irq_svc+0x44/0x7c [ 504.314996] [<c0809f98>] _raw_spin_unlock_irq+0x28/0x60 [ 504.322364] [<c0809f98>] _raw_spin_unlock_irq+0x28/0x60 [ 504.329749] [<c0271eb8>] finish_task_switch+0x188/0x2c0 [ 504.337136] [<c08049a8>] __schedule+0x734/0x7fc [ 504.343790] [<c0804ca8>] schedule+0x9c/0xc0 [ 504.350073] [<c0804ed4>] schedule_preempt_disabled+0x14/0x20 [ 504.357911] [<c028f4d4>] cpu_idle_loop+0x37c/0x3ac [ 504.364840] [<c028f51c>] cpupri_find+0x0/0xe4 [ 504.371296] INITIAL USE at: [ 504.374529] [<c0809810>] _raw_spin_lock_irqsave+0x40/0x54 [ 504.382004] [<c0688d48>] input_event+0x38/0x60 [ 504.388460] [<c0691a60>] gpio_keys_gpio_report_event+0x88/0xa4 [ 504.396390] [<c0691ab8>] gpio_keys_report_state+0x3c/0x68 [ 504.403879] [<c0691b14>] gpio_keys_open+0x30/0x38 [ 504.410621] [<c0686a88>] input_open_device+0x68/0xa4 [ 504.417634] [<c0589d58>] kbd_connect+0x50/0x7c [ 504.424098] [<c0686e80>] input_attach_handler+0x34/0x64 [ 504.431397] [<c0687428>] input_register_device+0x18c/0x248 [ 504.438974] [<c0692674>] gpio_keys_probe+0x198/0x208 [ 504.445981] [<c05ac2d8>] platform_drv_probe+0x50/0x98 [ 504.453090] [<c05aa3ac>] really_probe+0xfc/0x258 [ 504.459728] [<c05aa6ac>] driver_probe_device+0x44/0x70 [ 504.466921] [<c05aa760>] __driver_attach+0x88/0xac [ 504.473738] [<c05a8eb4>] bus_for_each_dev+0x50/0x84 [ 504.480659] [<c05a9bfc>] bus_add_driver+0xcc/0x1e4 [ 504.487485] [<c05ab6d0>] driver_register+0xac/0xf4 [ 504.494321] [<c02018c0>] do_one_initcall+0x100/0x1b8 [ 504.501331] [<c0e00d28>] do_basic_setup+0x98/0xd4 [ 504.508077] [<c0e00de8>] kernel_init_freeable+0x84/0x124 [ 504.515458] [<c080381c>] kernel_init+0x8/0x110 [ 504.521923] [<c02207c8>] ret_from_fork+0x14/0x2c [ 504.528570] } [ 504.530327] ... key at: [<c186df3c>] __key.23309+0x0/0x8 [ 504.536498] ... acquired at: [ 504.539636] [<c031a9f8>] mark_lock_irq+0xec/0x28c [ 504.544897] [<c0298c8c>] mark_lock+0x2e8/0x448 [ 504.549875] [<c0298e8c>] mark_irqflags+0xa0/0x18c [ 504.555126] [<c02995b8>] __lock_acquire+0x640/0x8d0 [ 504.560559] [<c0299fc8>] lock_acquire+0x25c/0x290 [ 504.565809] [<c0809810>] _raw_spin_lock_irqsave+0x40/0x54 [ 504.571798] [<c068aa50>] ml_effect_timer+0x18/0x34 [ 504.577147] [<c02b77dc>] call_timer_fn+0x224/0x47c [ 504.582495] [<c02b7dc4>] run_timer_softirq+0x390/0x3f8 [ 504.588209] [<c024fca8>] __do_softirq+0x2e4/0x66c [ 504.593465] [<c0250328>] irq_exit+0x98/0x118 [ 504.598264] [<c02a5a78>] __handle_domain_irq+0xa0/0xdc [ 504.603984] [<c0201578>] gic_handle_irq+0x50/0x8c [ 504.609241] [<c080af04>] __irq_svc+0x44/0x7c [ 504.614037] [<c0809f98>] _raw_spin_unlock_irq+0x28/0x60 [ 504.619845] [<c0809f98>] _raw_spin_unlock_irq+0x28/0x60 [ 504.625649] [<c0271eb8>] finish_task_switch+0x188/0x2c0 [ 504.631465] [<c08049a8>] __schedule+0x734/0x7fc [ 504.636533] [<c0804ca8>] schedule+0x9c/0xc0 [ 504.641238] [<c0804ed4>] schedule_preempt_disabled+0x14/0x20 [ 504.647503] [<c028f4d4>] cpu_idle_loop+0x37c/0x3ac [ 504.652856] [<c028f51c>] cpupri_find+0x0/0xe4 [ 504.657756] [ 504.659341] [ 504.659341] stack backtrace: [ 504.663948] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G W 4.6.0-rc4-letux+ #69 [ 504.672497] Hardware name: Generic OMAP5 (Flattened Device Tree) [ 504.678871] [<c022789c>] (unwind_backtrace) from [<c022403c>] (show_stack+0x10/0x14) [ 504.687083] [<c022403c>] (show_stack) from [<c0517e44>] (dump_stack+0x88/0xc0) [ 504.694743] [<c0517e44>] (dump_stack) from [<c031a6b8>] (print_irq_inversion_bug.part.12+0x16c/0x1a4) [ 504.704509] [<c031a6b8>] (print_irq_inversion_bug.part.12) from [<c029726c>] (print_irq_inversion_bug+0x74/0x84) [ 504.715299] [<c029726c>] (print_irq_inversion_bug) from [<c029735c>] (check_usage_forwards+0xe0/0x10c) [ 504.725152] [<c029735c>] (check_usage_forwards) from [<c031a9f8>] (mark_lock_irq+0xec/0x28c) [ 504.734094] [<c031a9f8>] (mark_lock_irq) from [<c0298c8c>] (mark_lock+0x2e8/0x448) [ 504.742114] [<c0298c8c>] (mark_lock) from [<c0298e8c>] (mark_irqflags+0xa0/0x18c) [ 504.750026] [<c0298e8c>] (mark_irqflags) from [<c02995b8>] (__lock_acquire+0x640/0x8d0) [ 504.758491] [<c02995b8>] (__lock_acquire) from [<c0299fc8>] (lock_acquire+0x25c/0x290) [ 504.766878] [<c0299fc8>] (lock_acquire) from [<c0809810>] (_raw_spin_lock_irqsave+0x40/0x54) [ 504.775823] [<c0809810>] (_raw_spin_lock_irqsave) from [<c068aa50>] (ml_effect_timer+0x18/0x34) [ 504.785043] [<c068aa50>] (ml_effect_timer) from [<c02b77dc>] (call_timer_fn+0x224/0x47c) [ 504.793613] [<c02b77dc>] (call_timer_fn) from [<c02b7dc4>] (run_timer_softirq+0x390/0x3f8) [ 504.802367] [<c02b7dc4>] (run_timer_softirq) from [<c024fca8>] (__do_softirq+0x2e4/0x66c) [ 504.811025] [<c024fca8>] (__do_softirq) from [<c0250328>] (irq_exit+0x98/0x118) [ 504.818768] [<c0250328>] (irq_exit) from [<c02a5a78>] (__handle_domain_irq+0xa0/0xdc) [ 504.827062] [<c02a5a78>] (__handle_domain_irq) from [<c0201578>] (gic_handle_irq+0x50/0x8c) [ 504.835910] [<c0201578>] (gic_handle_irq) from [<c080af04>] (__irq_svc+0x44/0x7c) [ 504.843825] Exception stack(0xee0dbee8 to 0xee0dbf30) [ 504.849178] bee0: 00000001 00000004 00000000 ee0d8e80 eefab600 eefab600 [ 504.857845] bf00: 00000000 ee0d8e80 c08049a8 00000000 00000002 ee0dbf74 00000000 ee0dbf38 [ 504.866504] bf20: c029aa18 c0809f98 200f0013 ffffffff [ 504.871860] [<c080af04>] (__irq_svc) from [<c0809f98>] (_raw_spin_unlock_irq+0x28/0x60) [ 504.880338] [<c0809f98>] (_raw_spin_unlock_irq) from [<c0271eb8>] (finish_task_switch+0x188/0x2c0) [ 504.889829] [<c0271eb8>] (finish_task_switch) from [<c08049a8>] (__schedule+0x734/0x7fc) [ 504.898398] [<c08049a8>] (__schedule) from [<c0804ca8>] (schedule+0x9c/0xc0) [ 504.905871] [<c0804ca8>] (schedule) from [<c0804ed4>] (schedule_preempt_disabled+0x14/0x20) [ 504.914720] [<c0804ed4>] (schedule_preempt_disabled) from [<c028f4d4>] (cpu_idle_loop+0x37c/0x3ac) [ 504.924210] [<c028f4d4>] (cpu_idle_loop) from [<c028f51c>] (cpupri_find+0x0/0xe4) [ 506.142849] BUG: spinlock lockup suspected on CPU#1, swapper/1/0 [ 506.149219] lock: 0xed1a019c, .magic: dead4ead, .owner: python/2591, .owner_cpu: 1 [ 506.157338] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G W 4.6.0-rc4-letux+ #69 [ 506.165873] Hardware name: Generic OMAP5 (Flattened Device Tree) [ 506.172231] [<c022789c>] (unwind_backtrace) from [<c022403c>] (show_stack+0x10/0x14) [ 506.180427] [<c022403c>] (show_stack) from [<c0517e44>] (dump_stack+0x88/0xc0) [ 506.188081] [<c0517e44>] (dump_stack) from [<c031ae24>] (__spin_lock_debug+0x8c/0xdc) [ 506.196384] [<c031ae24>] (__spin_lock_debug) from [<c029d15c>] (do_raw_spin_lock+0xa8/0xd8) [ 506.205230] [<c029d15c>] (do_raw_spin_lock) from [<c0809818>] (_raw_spin_lock_irqsave+0x48/0x54) [ 506.214539] [<c0809818>] (_raw_spin_lock_irqsave) from [<c068aa50>] (ml_effect_timer+0x18/0x34) [ 506.223742] [<c068aa50>] (ml_effect_timer) from [<c02b77dc>] (call_timer_fn+0x224/0x47c) [ 506.232311] [<c02b77dc>] (call_timer_fn) from [<c02b7dc4>] (run_timer_softirq+0x390/0x3f8) [ 506.241070] [<c02b7dc4>] (run_timer_softirq) from [<c024fca8>] (__do_softirq+0x2e4/0x66c) [ 506.249730] [<c024fca8>] (__do_softirq) from [<c0250328>] (irq_exit+0x98/0x118) [ 506.257483] [<c0250328>] (irq_exit) from [<c02a5a78>] (__handle_domain_irq+0xa0/0xdc) [ 506.265788] [<c02a5a78>] (__handle_domain_irq) from [<c0201578>] (gic_handle_irq+0x50/0x8c) [ 506.274637] [<c0201578>] (gic_handle_irq) from [<c080af04>] (__irq_svc+0x44/0x7c) [ 506.282558] Exception stack(0xee0dbee8 to 0xee0dbf30) [ 506.287891] bee0: 00000001 00000004 00000000 ee0d8e80 eefab600 eefab600 [ 506.296529] bf00: 00000000 ee0d8e80 c08049a8 00000000 00000002 ee0dbf74 00000000 ee0dbf38 [ 506.305162] bf20: c029aa18 c0809f98 200f0013 ffffffff [ 506.310501] [<c080af04>] (__irq_svc) from [<c0809f98>] (_raw_spin_unlock_irq+0x28/0x60) [ 506.318972] [<c0809f98>] (_raw_spin_unlock_irq) from [<c0271eb8>] (finish_task_switch+0x188/0x2c0) [ 506.328460] [<c0271eb8>] (finish_task_switch) from [<c08049a8>] (__schedule+0x734/0x7fc) [ 506.337041] [<c08049a8>] (__schedule) from [<c0804ca8>] (schedule+0x9c/0xc0) [ 506.344512] [<c0804ca8>] (schedule) from [<c0804ed4>] (schedule_preempt_disabled+0x14/0x20) [ 506.353359] [<c0804ed4>] (schedule_preempt_disabled) from [<c028f4d4>] (cpu_idle_loop+0x37c/0x3ac) [ 506.362845] [<c028f4d4>] (cpu_idle_loop) from [<c028f51c>] (cpupri_find+0x0/0xe4) [ 506.370766] Sending NMI to all CPUs: [ 506.375928] NMI backtrace for cpu 0 [ 506.379616] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 4.6.0-rc4-letux+ #69 [ 506.388171] Hardware name: Generic OMAP5 (Flattened Device Tree) [ 506.394529] task: c1005e80 ti: c1000000 task.ti: c1000000 [ 506.400238] PC is at arch_cpu_idle+0x30/0x3c [ 506.404752] LR is at arch_cpu_idle+0x2c/0x3c [ 506.409277] pc : [<c02211e0>] lr : [<c02211dc>] psr: 60000013 [ 506.415916] sp : c1001fa8 ip : c1001f88 fp : 00000000 [ 506.421453] r10: 00000000 r9 : 412fc0f2 r8 : 80007000 [ 506.426981] r7 : c1009a00 r6 : c10025d4 r5 : c100256c r4 : 0000002b [ 506.433886] r3 : 00000000 r2 : fe600000 r1 : c1001fa8 r0 : c02211dc [ 506.440799] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel [ 506.448528] Control: 30c5387d Table: a8b8ff00 DAC: fffffffd [ 506.454608] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 4.6.0-rc4-letux+ #69 [ 506.463165] Hardware name: Generic OMAP5 (Flattened Device Tree) [ 506.469520] [<c022789c>] (unwind_backtrace) from [<c022403c>] (show_stack+0x10/0x14) [ 506.477714] [<c022403c>] (show_stack) from [<c0517e44>] (dump_stack+0x88/0xc0) [ 506.485354] [<c0517e44>] (dump_stack) from [<c051c0ec>] (nmi_cpu_backtrace+0xc4/0x108) [ 506.493715] [<c051c0ec>] (nmi_cpu_backtrace) from [<c0225f50>] (handle_IPI+0x240/0x3dc) [ 506.502180] [<c0225f50>] (handle_IPI) from [<c02015a8>] (gic_handle_irq+0x80/0x8c) [ 506.510189] [<c02015a8>] (gic_handle_irq) from [<c080af04>] (__irq_svc+0x44/0x7c) [ 506.518102] Exception stack(0xc1001f58 to 0xc1001fa0) [ 506.523444] 1f40: c02211dc c1001fa8 [ 506.532097] 1f60: fe600000 00000000 0000002b c100256c c10025d4 c1009a00 80007000 412fc0f2 [ 506.540754] 1f80: 00000000 00000000 c1001f88 c1001fa8 c02211dc c02211e0 60000013 ffffffff [ 506.549403] [<c080af04>] (__irq_svc) from [<c02211e0>] (arch_cpu_idle+0x30/0x3c) [ 506.557234] [<c02211e0>] (arch_cpu_idle) from [<c028f470>] (cpu_idle_loop+0x318/0x3ac) [ 506.565615] [<c028f470>] (cpu_idle_loop) from [<c028f51c>] (cpupri_find+0x0/0xe4) [ 506.573528] [<c028f51c>] (cpupri_find) from [<c10a54c0>] (processor_id+0x0/0x40) [ 506.581346] NMI backtrace for cpu 1 [ 506.585033] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G W 4.6.0-rc4-letux+ #69 [ 506.593591] Hardware name: Generic OMAP5 (Flattened Device Tree) [ 506.599949] [<c022789c>] (unwind_backtrace) from [<c022403c>] (show_stack+0x10/0x14) [ 506.608147] [<c022403c>] (show_stack) from [<c0517e44>] (dump_stack+0x88/0xc0) [ 506.615784] [<c0517e44>] (dump_stack) from [<c051c0f4>] (nmi_cpu_backtrace+0xcc/0x108) [ 506.624155] [<c051c0f4>] (nmi_cpu_backtrace) from [<c0225598>] (raise_nmi+0x44/0x54) [ 506.632343] [<c0225598>] (raise_nmi) from [<c051c280>] (nmi_trigger_all_cpu_backtrace+0xf0/0x254) [ 506.641732] [<c051c280>] (nmi_trigger_all_cpu_backtrace) from [<c031ae2c>] (__spin_lock_debug+0x94/0xdc) [ 506.651766] [<c031ae2c>] (__spin_lock_debug) from [<c029d15c>] (do_raw_spin_lock+0xa8/0xd8) [ 506.660607] [<c029d15c>] (do_raw_spin_lock) from [<c0809818>] (_raw_spin_lock_irqsave+0x48/0x54) [ 506.669901] [<c0809818>] (_raw_spin_lock_irqsave) from [<c068aa50>] (ml_effect_timer+0x18/0x34) [ 506.679103] [<c068aa50>] (ml_effect_timer) from [<c02b77dc>] (call_timer_fn+0x224/0x47c) [ 506.687655] [<c02b77dc>] (call_timer_fn) from [<c02b7dc4>] (run_timer_softirq+0x390/0x3f8) [ 506.696407] [<c02b7dc4>] (run_timer_softirq) from [<c024fca8>] (__do_softirq+0x2e4/0x66c) [ 506.705067] [<c024fca8>] (__do_softirq) from [<c0250328>] (irq_exit+0x98/0x118) [ 506.712807] [<c0250328>] (irq_exit) from [<c02a5a78>] (__handle_domain_irq+0xa0/0xdc) [ 506.721086] [<c02a5a78>] (__handle_domain_irq) from [<c0201578>] (gic_handle_irq+0x50/0x8c) [ 506.729911] [<c0201578>] (gic_handle_irq) from [<c080af04>] (__irq_svc+0x44/0x7c) [ 506.737818] Exception stack(0xee0dbee8 to 0xee0dbf30) [ 506.743169] bee0: 00000001 00000004 00000000 ee0d8e80 eefab600 eefab600 [ 506.751820] bf00: 00000000 ee0d8e80 c08049a8 00000000 00000002 ee0dbf74 00000000 ee0dbf38 [ 506.760464] bf20: c029aa18 c0809f98 200f0013 ffffffff [ 506.765800] [<c080af04>] (__irq_svc) from [<c0809f98>] (_raw_spin_unlock_irq+0x28/0x60) [ 506.774273] [<c0809f98>] (_raw_spin_unlock_irq) from [<c0271eb8>] (finish_task_switch+0x188/0x2c0) [ 506.783731] [<c0271eb8>] (finish_task_switch) from [<c08049a8>] (__schedule+0x734/0x7fc) [ 506.792279] [<c08049a8>] (__schedule) from [<c0804ca8>] (schedule+0x9c/0xc0) [ 506.799745] [<c0804ca8>] (schedule) from [<c0804ed4>] (schedule_preempt_disabled+0x14/0x20) [ 506.808577] [<c0804ed4>] (schedule_preempt_disabled) from [<c028f4d4>] (cpu_idle_loop+0x37c/0x3ac) [ 506.818058] [<c028f4d4>] (cpu_idle_loop) from [<c028f51c>] (cpupri_find+0x0/0xe4)
On Wed, Apr 20, 2016 at 11:22:53AM +0200, H. Nikolaus Schaller wrote:
Am 19.04.2016 um 10:08 schrieb H. Nikolaus Schaller hns@goldelico.com:
Am 19.04.2016 um 10:01 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Tue, Apr 19, 2016 at 09:49:01AM +0200, H. Nikolaus Schaller wrote:
Am 18.04.2016 um 23:20 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Mon, Apr 18, 2016 at 09:55:41PM +0200, H. Nikolaus Schaller wrote:
The mutex does not seem to be needed.
twl6040_vibra_suspend() and vibra_play_work() may run concurrently, no?
Hm. I don't know about the rule that would give an answer to this question...
Sorry, that was actually a statement, not really a question.
Indeed. In doubt about the answer we should take measures for the worst case.
It is possible (although very unlikely) that userspace posts play request and workqueue will not run until after suspend callback.
Thinking about it some more I wonder if we better do what twl6040_vibra_close() does and cancel the work before shutting off the device, so that there is no chance of work executing after suspend callback and reenabling the device. This way we can indeed remove the mutex.
Ok, I am fine with this.
Will post an update ASAP.
While doing testing I did run again into another locking related issue which I had not yet tried to address with my patch set. It is a:
BUG: scheduling while atomic
report which sometimes ends in a kernel panic.
I have attached such a log. vibra.py is here:
http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=Letux/root/vibra.py;hb...
Basically it does an ioctl(EVIOCSFF) which triggers vibra_play.
Maybe, can you decipher from the log what the reason could be?
I only conjecture that it happens when vibra_play tries to read the regmap of the twl6040 to get twl6040_get_vibralr_status (which has no pendant in the twl4030 driver).
Do we have to configure the twl6040 regmap differently?
Right, vibra_play is called with interrupts disabled (that's why we have workqueue to enable/disable regulators, etc, when we start or stop vibration), so twl6040_get_vibralr_status() should not sleep, but apparently it does. Maybe the check for audio configuration should be moved into vibra_play_work().
Thanks.
Hi,
Am 20.04.2016 um 19:49 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Wed, Apr 20, 2016 at 11:22:53AM +0200, H. Nikolaus Schaller wrote:
Am 19.04.2016 um 10:08 schrieb H. Nikolaus Schaller hns@goldelico.com:
Am 19.04.2016 um 10:01 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Tue, Apr 19, 2016 at 09:49:01AM +0200, H. Nikolaus Schaller wrote:
Am 18.04.2016 um 23:20 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Mon, Apr 18, 2016 at 09:55:41PM +0200, H. Nikolaus Schaller wrote: > The mutex does not seem to be needed.
twl6040_vibra_suspend() and vibra_play_work() may run concurrently, no?
Hm. I don't know about the rule that would give an answer to this question...
Sorry, that was actually a statement, not really a question.
Indeed. In doubt about the answer we should take measures for the worst case.
It is possible (although very unlikely) that userspace posts play request and workqueue will not run until after suspend callback.
Thinking about it some more I wonder if we better do what twl6040_vibra_close() does and cancel the work before shutting off the device, so that there is no chance of work executing after suspend callback and reenabling the device. This way we can indeed remove the mutex.
Ok, I am fine with this.
Will post an update ASAP.
While doing testing I did run again into another locking related issue which I had not yet tried to address with my patch set. It is a:
BUG: scheduling while atomic
report which sometimes ends in a kernel panic.
I have attached such a log. vibra.py is here:
http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=Letux/root/vibra.py;hb...
Basically it does an ioctl(EVIOCSFF) which triggers vibra_play.
Maybe, can you decipher from the log what the reason could be?
I only conjecture that it happens when vibra_play tries to read the regmap of the twl6040 to get twl6040_get_vibralr_status (which has no pendant in the twl4030 driver).
Do we have to configure the twl6040 regmap differently?
Right, vibra_play is called with interrupts disabled (that's why we have workqueue to enable/disable regulators, etc, when we start or stop vibration), so twl6040_get_vibralr_status() should not sleep, but apparently it does.
Yes, regmap using i2c communication may sleep.
Maybe the check for audio configuration should be moved into vibra_play_work().
Hm. It is there to disable while in audio routing mode, but a workqueue can't report error values back to the scheduling thread...
So we can either silently make vibra not work (or just report in the kernel log) if "Vibra is configured for audio".
Or we need to get a different mechanism to know the vibra status.
Hm.
Research shows that it regmap was introduced long ago but between 3.11 and 3.12 a private cache for these control registers was removed.
http://lxr.free-electrons.com/source/drivers/mfd/twl6040.c?v=3.11#L66 http://lxr.free-electrons.com/source/drivers/mfd/twl6040.c?v=3.11#L85 http://lxr.free-electrons.com/source/drivers/mfd/twl6040.c?v=3.11#L462
This had been non-blocking and was probably there exactly to protect against this locking issue!
After removing the private cache the code must rely on twl6040_get_vibralr_status not fetching from the chip.
Ah, this correlates to that I see this issue only once and then everything works.
This means we have to fetch the current vibra control registers once outside of vibra_play(). Probably during probing by a single call to twl6040_get_vibralr_status() and ignoring the result.
After it has been fetched once (to know any status from the last reboot) the regmap should track all changes arriving through the sound subsystem (audio vibra enable) and the call to twl6040_get_vibralr_status in interrupt context should no longer block.
Does this sound reasonable?
BR and thanks, Nikolaus
On Wed, Apr 20, 2016 at 08:10:28PM +0200, H. Nikolaus Schaller wrote:
Hi,
Am 20.04.2016 um 19:49 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Wed, Apr 20, 2016 at 11:22:53AM +0200, H. Nikolaus Schaller wrote:
Am 19.04.2016 um 10:08 schrieb H. Nikolaus Schaller hns@goldelico.com:
Am 19.04.2016 um 10:01 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Tue, Apr 19, 2016 at 09:49:01AM +0200, H. Nikolaus Schaller wrote:
> Am 18.04.2016 um 23:20 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com: > > On Mon, Apr 18, 2016 at 09:55:41PM +0200, H. Nikolaus Schaller wrote: >> The mutex does not seem to be needed. > > twl6040_vibra_suspend() and vibra_play_work() may run concurrently, no?
Hm. I don't know about the rule that would give an answer to this question...
Sorry, that was actually a statement, not really a question.
Indeed. In doubt about the answer we should take measures for the worst case.
It is possible (although very unlikely) that userspace posts play request and workqueue will not run until after suspend callback.
Thinking about it some more I wonder if we better do what twl6040_vibra_close() does and cancel the work before shutting off the device, so that there is no chance of work executing after suspend callback and reenabling the device. This way we can indeed remove the mutex.
Ok, I am fine with this.
Will post an update ASAP.
While doing testing I did run again into another locking related issue which I had not yet tried to address with my patch set. It is a:
BUG: scheduling while atomic
report which sometimes ends in a kernel panic.
I have attached such a log. vibra.py is here:
http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=Letux/root/vibra.py;hb...
Basically it does an ioctl(EVIOCSFF) which triggers vibra_play.
Maybe, can you decipher from the log what the reason could be?
I only conjecture that it happens when vibra_play tries to read the regmap of the twl6040 to get twl6040_get_vibralr_status (which has no pendant in the twl4030 driver).
Do we have to configure the twl6040 regmap differently?
Right, vibra_play is called with interrupts disabled (that's why we have workqueue to enable/disable regulators, etc, when we start or stop vibration), so twl6040_get_vibralr_status() should not sleep, but apparently it does.
Yes, regmap using i2c communication may sleep.
Maybe the check for audio configuration should be moved into vibra_play_work().
Hm. It is there to disable while in audio routing mode, but a workqueue can't report error values back to the scheduling thread...
Nothing checks result of ->play() anyways, so that -EBUSY was completly useless.
So we can either silently make vibra not work (or just report in the kernel log) if "Vibra is configured for audio".
We might want to ratelimit that message.
Or we need to get a different mechanism to know the vibra status.
Hm.
Research shows that it regmap was introduced long ago but between 3.11 and 3.12 a private cache for these control registers was removed.
http://lxr.free-electrons.com/source/drivers/mfd/twl6040.c?v=3.11#L66 http://lxr.free-electrons.com/source/drivers/mfd/twl6040.c?v=3.11#L85 http://lxr.free-electrons.com/source/drivers/mfd/twl6040.c?v=3.11#L462
This had been non-blocking and was probably there exactly to protect against this locking issue!
After removing the private cache the code must rely on twl6040_get_vibralr_status not fetching from the chip.
Ah, this correlates to that I see this issue only once and then everything works.
This means we have to fetch the current vibra control registers once outside of vibra_play(). Probably during probing by a single call to twl6040_get_vibralr_status() and ignoring the result.
After it has been fetched once (to know any status from the last reboot) the regmap should track all changes arriving through the sound subsystem (audio vibra enable) and the call to twl6040_get_vibralr_status in interrupt context should no longer block.
Does this sound reasonable?
Yes, as long as you document this so that it does not get removed by mistake later.
Thanks.
Hi Dmitry,
Am 20.04.2016 um 20:15 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Wed, Apr 20, 2016 at 08:10:28PM +0200, H. Nikolaus Schaller wrote:
Hi,
Am 20.04.2016 um 19:49 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Wed, Apr 20, 2016 at 11:22:53AM +0200, H. Nikolaus Schaller wrote:
Am 19.04.2016 um 10:08 schrieb H. Nikolaus Schaller hns@goldelico.com:
Am 19.04.2016 um 10:01 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com:
On Tue, Apr 19, 2016 at 09:49:01AM +0200, H. Nikolaus Schaller wrote: > >> Am 18.04.2016 um 23:20 schrieb Dmitry Torokhov dmitry.torokhov@gmail.com: >> >> On Mon, Apr 18, 2016 at 09:55:41PM +0200, H. Nikolaus Schaller wrote: >>> The mutex does not seem to be needed. >> >> twl6040_vibra_suspend() and vibra_play_work() may run concurrently, no? > > Hm. I don't know about the rule that would give an answer to this question...
Sorry, that was actually a statement, not really a question.
Indeed. In doubt about the answer we should take measures for the worst case.
It is possible (although very unlikely) that userspace posts play request and workqueue will not run until after suspend callback.
Thinking about it some more I wonder if we better do what twl6040_vibra_close() does and cancel the work before shutting off the device, so that there is no chance of work executing after suspend callback and reenabling the device. This way we can indeed remove the mutex.
Ok, I am fine with this.
Will post an update ASAP.
While doing testing I did run again into another locking related issue which I had not yet tried to address with my patch set. It is a:
BUG: scheduling while atomic
report which sometimes ends in a kernel panic.
I have attached such a log. vibra.py is here:
http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=Letux/root/vibra.py;hb...
Basically it does an ioctl(EVIOCSFF) which triggers vibra_play.
Maybe, can you decipher from the log what the reason could be?
I only conjecture that it happens when vibra_play tries to read the regmap of the twl6040 to get twl6040_get_vibralr_status (which has no pendant in the twl4030 driver).
Do we have to configure the twl6040 regmap differently?
Right, vibra_play is called with interrupts disabled (that's why we have workqueue to enable/disable regulators, etc, when we start or stop vibration), so twl6040_get_vibralr_status() should not sleep, but apparently it does.
Yes, regmap using i2c communication may sleep.
Maybe the check for audio configuration should be moved into vibra_play_work().
Hm. It is there to disable while in audio routing mode, but a workqueue can't report error values back to the scheduling thread...
Nothing checks result of ->play() anyways, so that -EBUSY was completly useless.
Ah, indeed:
http://lxr.free-electrons.com/source/drivers/input/ff-memless.c#L410
So we can either silently make vibra not work (or just report in the kernel log) if "Vibra is configured for audio".
We might want to ratelimit that message.
Or not report it at all.
Or we need to get a different mechanism to know the vibra status.
Hm.
Research shows that it regmap was introduced long ago but between 3.11 and 3.12 a private cache for these control registers was removed.
http://lxr.free-electrons.com/source/drivers/mfd/twl6040.c?v=3.11#L66 http://lxr.free-electrons.com/source/drivers/mfd/twl6040.c?v=3.11#L85 http://lxr.free-electrons.com/source/drivers/mfd/twl6040.c?v=3.11#L462
This had been non-blocking and was probably there exactly to protect against this locking issue!
After removing the private cache the code must rely on twl6040_get_vibralr_status not fetching from the chip.
Ah, this correlates to that I see this issue only once and then everything works.
This means we have to fetch the current vibra control registers once outside of vibra_play(). Probably during probing by a single call to twl6040_get_vibralr_status() and ignoring the result.
After it has been fetched once (to know any status from the last reboot) the regmap should track all changes arriving through the sound subsystem (audio vibra enable) and the call to twl6040_get_vibralr_status in interrupt context should no longer block.
Does this sound reasonable?
I think if the -EBUSY isn't evaluated at all, it is simpler to move this test to the worker. This has the benefit of avoiding a potential race between changing the vibra source selection (e.g. through amixer) and running the work function.
Yes, as long as you document this so that it does not get removed by mistake later.
I will prepare a patch and test asap.
BR and thanks, Nikolaus