fixes for twl6040-vibra
Subsystems (2)
cover fixes for twl6040-vibra
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(-)
Signature · 1 line
2.7.3
PATCH 1/5 input: twl6040-vibra: fix DT node memory management
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()
--- drivers/input/misc/twl6040-vibra.c | 1 + 1 file changed, 1 insertion(+)
drivers/input/misc/twl6040-vibra.c +1 -0
@@ -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) {
Signature · 1 line
2.7.3
Dmitry Torokhov
maintainerRe: [PATCH 1/5] input: twl6040-vibra: fix DT node memory management
Quoted 9 lines · H. Nikolaus Schaller
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 callof_node_puton()for the twl6040 parent node passed in as a parameter. This causes trouble later on.Solution: we must call
of_node_get()beforeof_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?
Quoted 21 lines
Signed-off-by H. Nikolaus Schaller <[email protected]>--- drivers/input/misc/twl6040-vibra.c | 1 + 1 file changed, 1 insertion(+)drivers/input/misc/twl6040-vibra.c +1
-0@@ -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) {Signature · 1 line
2.7.3
Signature · 1 line
Dmitry
Re: [PATCH 1/5] input: twl6040-vibra: fix DT node memory management
Quoted 8 lines
Am 18.04.2016 um 23:22 schrieb Dmitry Torokhov <[email protected]>:
Quoted 4 lines · H. Nikolaus Schaller
commit
e7ec014a47e4("Input: twl6040-vibra - update for device tree support")made the separate vibra DT node to a subnode of the twl6040.
Quoted 7 lines
Quoted 5 lines
It now calls
of_find_node_by_name()to locate the "vibra" subnode. This function has a side effect to callof_node_puton()for the twl6040 parent node passed in as a parameter. This causes trouble later on.Solution: we must call
of_node_get()beforeof_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.
Quoted 2 lines
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?
Quoted 25 lines
Quoted 22 lines
Signed-off-by H. Nikolaus Schaller <[email protected]>---
drivers/input/misc/twl6040-vibra.c| 1 +1 file changed, 1 insertion(+)drivers/input/misc/twl6040-vibra.c +1
-0@@ -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) {Signature · 1 line
2.7.3Signature · 1 line
Dmitry
Dmitry Torokhov
maintainerRe: [PATCH 1/5] input: twl6040-vibra: fix DT node memory management
Quoted 19 lines · H. Nikolaus Schaller
Quoted 8 lines
Am 18.04.2016 um 23:22 schrieb Dmitry Torokhov <[email protected]>:
Quoted 4 lines · H. Nikolaus Schaller
commit
e7ec014a47e4("Input: twl6040-vibra - update for device tree support")made the separate vibra DT node to a subnode of the twl6040.
Quoted 7 lines
Quoted 5 lines
It now calls
of_find_node_by_name()to locate the "vibra" subnode. This function has a side effect to callof_node_puton()for the twl6040 parent node passed in as a parameter. This causes trouble later on.Solution: we must call
of_node_get()beforeof_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.
Quoted 8 lines
Usually it does not become obvious because often
CONFIG_OF_DYNAMIC=n. This disables allof_noderefcounting completely so such bugs remain unnoticed.Quoted 2 lines
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...
Signature · 1 line
Dmitry
Re: [PATCH 1/5] input: twl6040-vibra: fix DT node memory management
Quoted 26 lines
Am 19.04.2016 um 19:06 schrieb Dmitry Torokhov <[email protected]>:
Quoted 22 lines · H. Nikolaus Schaller
Quoted 8 lines
Am 18.04.2016 um 23:22 schrieb Dmitry Torokhov <[email protected]>:
Quoted 4 lines · H. Nikolaus Schaller
commit
e7ec014a47e4("Input: twl6040-vibra - update for device tree support")made the separate vibra DT node to a subnode of the twl6040.
Quoted 7 lines
Quoted 5 lines
It now calls
of_find_node_by_name()to locate the "vibra" subnode. This function has a side effect to callof_node_puton()for the twl6040 parent node passed in as a parameter. This causes trouble later on.Solution: we must call
of_node_get()beforeof_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 allof_noderefcounting completely so such bugs remain unnoticed.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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
Re: [Kernel] [PATCH 1/5] input: twl6040-vibra: fix DT node memory management
Hi Dmitry,
Quoted 31 lines
Am 20.04.2016 um 11:03 schrieb H. Nikolaus Schaller <[email protected]>:
Quoted 28 lines
Am 19.04.2016 um 19:06 schrieb Dmitry Torokhov <[email protected]>:
Quoted 25 lines · H. Nikolaus Schaller
Quoted 8 lines
Am 18.04.2016 um 23:22 schrieb Dmitry Torokhov <[email protected]>:
Quoted 4 lines · H. Nikolaus Schaller
commit
e7ec014a47e4("Input: twl6040-vibra - update for device tree support")made the separate vibra DT node to a subnode of the twl6040.
Quoted 7 lines
Quoted 5 lines
It now calls
of_find_node_by_name()to locate the "vibra" subnode. This function has a side effect to callof_node_puton()for the twl6040 parent node passed in as a parameter. This causes trouble later on.Solution: we must call
of_node_get()beforeof_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 allof_noderefcounting completely so such bugs remain unnoticed.Quoted 2 lines
Any chance we can make it a bit more sane and
not drop the reference inside it instead?
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
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
Dmitry Torokhov
maintainerRe: [Kernel] [PATCH 1/5] input: twl6040-vibra: fix DT node memory management
Quoted 36 lines · H. Nikolaus Schaller
Hi Dmitry,
Quoted 34 lines
Am 20.04.2016 um 11:03 schrieb H. Nikolaus Schaller <[email protected]>:
Quoted 31 lines
Am 19.04.2016 um 19:06 schrieb Dmitry Torokhov <[email protected]>:
Quoted 27 lines · H. Nikolaus Schaller
Quoted 8 lines
Am 18.04.2016 um 23:22 schrieb Dmitry Torokhov <[email protected]>:
Quoted 4 lines · H. Nikolaus Schaller
commit
e7ec014a47e4("Input: twl6040-vibra - update for device tree support")made the separate vibra DT node to a subnode of the twl6040.
Quoted 7 lines
Quoted 5 lines
It now calls
of_find_node_by_name()to locate the "vibra" subnode. This function has a side effect to callof_node_puton()for the twl6040 parent node passed in as a parameter. This causes trouble later on.Solution: we must call
of_node_get()beforeof_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 allof_noderefcounting completely so such bugs remain unnoticed.Quoted 2 lines
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?
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
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.
Signature · 1 line
Dmitry
PATCH 2/5 input: twl6040-vibra: add handler to unregister input if module is removed
In reply to H. Nikolaus Schallercommit 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
--- drivers/input/misc/twl6040-vibra.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
drivers/input/misc/twl6040-vibra.c +10 -0
@@ -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,
Signature · 1 line
2.7.3
Dmitry Torokhov
maintainerRe: [PATCH 2/5] input: twl6040-vibra: add handler to unregister input if module is removed
Quoted 4 lines · H. Nikolaus Schaller
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.
Quoted 34 lines
Solution: add back
twl6040_vibra_removeto callinput_unregister_deviceSigned-off-by H. Nikolaus Schaller <[email protected]>--- drivers/input/misc/twl6040-vibra.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/input/misc/twl6040-vibra.c +10
-0@@ -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,Signature · 1 line
2.7.3
Thanks.
Signature · 1 line
Dmitry
Re: [PATCH 2/5] input: twl6040-vibra: add handler to unregister input if module is removed
Quoted 11 lines
Am 18.04.2016 um 23:12 schrieb Dmitry Torokhov <[email protected]>:
Quoted 4 lines · H. Nikolaus Schaller
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):
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
Quoted 40 lines
Quoted 35 lines
Solution: add back
twl6040_vibra_removeto callinput_unregister_deviceSigned-off-by H. Nikolaus Schaller <[email protected]>---
drivers/input/misc/twl6040-vibra.c| 10 ++++++++++1 file changed, 10 insertions(+)drivers/input/misc/twl6040-vibra.c +10
-0@@ -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,Signature · 1 line
2.7.3Thanks.
Signature · 1 line
Dmitry
Dmitry Torokhov
maintainerRe: [PATCH 2/5] input: twl6040-vibra: add handler to unregister input if module is removed
Quoted 14 lines · H. Nikolaus Schaller
Quoted 11 lines
Am 18.04.2016 um 23:12 schrieb Dmitry Torokhov <[email protected]>:
Quoted 4 lines · H. Nikolaus Schaller
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):
Quoted 7 lines
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.
Signature · 1 line
Dmitry
Re: [PATCH 2/5] input: twl6040-vibra: add handler to unregister input if module is removed
Quoted 36 lines
Am 19.04.2016 um 09:57 schrieb Dmitry Torokhov <[email protected]>:
Quoted 14 lines · H. Nikolaus Schaller
Quoted 11 lines
Am 18.04.2016 um 23:12 schrieb Dmitry Torokhov <[email protected]>:
Quoted 4 lines · H. Nikolaus Schaller
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):
Quoted 7 lines
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 structinput_devor %NULL.
*
* Managed input devices do not need to be explicitly unregistered or^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Quoted 12 lines
* 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
Dmitry Torokhov
maintainerRe: [PATCH 2/5] input: twl6040-vibra: add handler to unregister input if module is removed
Quoted 49 lines · H. Nikolaus Schaller
Quoted 36 lines
Am 19.04.2016 um 09:57 schrieb Dmitry Torokhov <[email protected]>:
Quoted 14 lines · H. Nikolaus Schaller
Quoted 11 lines
Am 18.04.2016 um 23:12 schrieb Dmitry Torokhov <[email protected]>:
Quoted 4 lines · H. Nikolaus Schaller
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):
Quoted 7 lines
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 structinput_devor %NULL.
*
* Managed input devices do not need to be explicitly unregistered or^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^Quoted 12 lines
* 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.
*/
Quoted 3 lines
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.
Signature · 14 lines
Dmitry Input: twl6040-vibra - do not reparent to grandparent From: Dmitry Torokhov <[email protected]> For devm-managed input devices we should not modify input device's parent, otherwise automatic release of resources will not work properly. Signed-off-by: Dmitry Torokhov <[email protected]> --- drivers/input/misc/twl6040-vibra.c | 1 - 1 file changed, 1 deletion(-)
drivers/input/misc/twl6040-vibra.c +0 -1
@@ -357,7 +357,6 @@ static int twl6040_vibra_probe(struct platform_device *pdev) info->input_dev->name ="twl6040:vibrator" ; info->input_dev->id.version =1 ; - info->input_dev->dev.parent = pdev->dev.parent; info->input_dev->close = twl6040_vibra_close;__set_bit (FF_RUMBLE , info->input_dev->ffbit);
Re: [PATCH 2/5] input: twl6040-vibra: add handler to unregister input if module is removed
Hi Dmitry,
Quoted 52 lines
Am 19.04.2016 um 18:53 schrieb Dmitry Torokhov <[email protected]>:
Quoted 49 lines · H. Nikolaus Schaller
Quoted 48 lines
Am 19.04.2016 um 09:57 schrieb Dmitry Torokhov <[email protected]>:
Quoted 14 lines · H. Nikolaus Schaller
Quoted 11 lines
Am 18.04.2016 um 23:12 schrieb Dmitry Torokhov <[email protected]>:
Quoted 4 lines · H. Nikolaus Schaller
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):
Quoted 7 lines
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 structinput_devor %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.
*/
Quoted 7 lines
Quoted 3 lines
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
PATCH 3/5 input: twl6040-vibra: fix NULL pointer dereference by removing workqueue
In reply to H. Nikolaus Schallercommit 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
--- drivers/input/misc/twl6040-vibra.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/input/misc/twl6040-vibra.c +1 -2
@@ -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;
Signature · 1 line
2.7.3
Re: [PATCH 3/5] input: twl6040-vibra: fix NULL pointer dereference by removing workqueue
Quoted 7 lines · H. Nikolaus Schaller
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
Quoted 7 lines
Instead it calls
queue_work()on uninitializedinfo->workqueue.The result is a NULL pointer dereference in
vibra_play().Solution: use
schedule_workSigned-off-by H. Nikolaus Schaller <[email protected]>
Applied, thank you.
Quoted 28 lines
--- drivers/input/misc/twl6040-vibra.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)drivers/input/misc/twl6040-vibra.c +1
-2@@ -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;Signature · 1 line
2.7.3
Signature · 1 line
Dmitry
PATCH 4/5 input: twl6040-vibra: ignore return value of schedule_work
In reply to H. Nikolaus Schallerreturning 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.
--- drivers/input/misc/twl6040-vibra.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/input/misc/twl6040-vibra.c +1 -5
@@ -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 ; }
Signature · 1 line
2.7.3
Re: [PATCH 4/5] input: twl6040-vibra: ignore return value of schedule_work
Quoted 6 lines · H. Nikolaus Schaller
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.calso ignores the return value.Signed-off-by H. Nikolaus Schaller <[email protected]>
Applied, thank you.
Quoted 24 lines
--- drivers/input/misc/twl6040-vibra.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)drivers/input/misc/twl6040-vibra.c +1
-5@@ -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 ; }Signature · 1 line
2.7.3
Signature · 1 line
Dmitry
PATCH 5/5 input: twl6040-vibra: remove mutex
In reply to H. Nikolaus SchallerThe mutex does not seem to be needed. twl4030-vibra doesn't use one either.
--- drivers/input/misc/twl6040-vibra.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)
drivers/input/misc/twl6040-vibra.c +1 -14
@@ -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,
Signature · 1 line
2.7.3
Dmitry Torokhov
maintainerRe: [PATCH 5/5] input: twl6040-vibra: remove mutex
Quoted 1 line · H. Nikolaus Schaller
The mutex does not seem to be needed.
twl6040_vibra_suspend() and vibra_play_work() may run concurrently, no?
Quoted 2 lines
twl4030-vibra doesn't
use one either.
It probably should.
Thanks.
Quoted 75 lines
Signed-off-by H. Nikolaus Schaller <[email protected]>--- drivers/input/misc/twl6040-vibra.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-)drivers/input/misc/twl6040-vibra.c +1
-14@@ -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,Signature · 1 line
2.7.3
Signature · 1 line
Dmitry
Re: [PATCH 5/5] input: twl6040-vibra: remove mutex
Quoted 7 lines
Am 18.04.2016 um 23:20 schrieb Dmitry Torokhov <[email protected]>:
Quoted 1 line · H. Nikolaus Schaller
The mutex does not seem to be needed.
twl6040_vibra_suspend()andvibra_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.
Quoted 86 lines
Quoted 3 lines
twl4030-vibra doesn't
use one either.It probably should.
Thanks.
Quoted 75 lines
Signed-off-by H. Nikolaus Schaller <[email protected]>---
drivers/input/misc/twl6040-vibra.c| 15 +--------------1 file changed, 1 insertion(+), 14 deletions(-)drivers/input/misc/twl6040-vibra.c +1
-14@@ -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,Signature · 1 line
2.7.3Signature · 1 line
Dmitry
Dmitry Torokhov
maintainerRe: [PATCH 5/5] input: twl6040-vibra: remove mutex
Quoted 9 lines · H. Nikolaus Schaller
Quoted 7 lines
Am 18.04.2016 um 23:20 schrieb Dmitry Torokhov <[email protected]>:
Quoted 1 line · H. Nikolaus Schaller
The mutex does not seem to be needed.
twl6040_vibra_suspend()andvibra_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.
Signature · 1 line
Dmitry
Re: [PATCH 5/5] input: twl6040-vibra: remove mutex
Quoted 7 lines
Am 19.04.2016 um 10:01 schrieb Dmitry Torokhov <[email protected]>:
Quoted 3 lines · H. Nikolaus Schaller
Quoted 3 lines
Am 18.04.2016 um 23:20 schrieb Dmitry Torokhov <[email protected]>:
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines · H. Nikolaus Schaller
Quoted 1 line
The mutex does not seem to be needed.
twl6040_vibra_suspend()andvibra_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.
Quoted 9 lines
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
Re: [Letux-kernel] [PATCH 5/5] input: twl6040-vibra: remove mutex
Quoted 31 lines
Am 19.04.2016 um 10:08 schrieb H. Nikolaus Schaller <[email protected]>:
Quoted 6 lines
Am 19.04.2016 um 10:01 schrieb Dmitry Torokhov <[email protected]>:
Quoted 3 lines · H. Nikolaus Schaller
Quoted 3 lines
Am 18.04.2016 um 23:20 schrieb Dmitry Torokhov <[email protected]>:
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines · H. Nikolaus Schaller
Quoted 1 line
The mutex does not seem to be needed.
twl6040_vibra_suspend()andvibra_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.
Quoted 9 lines
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.
Quoted 3 lines
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=refs/heads/letux-4.6-rc4
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)Dmitry Torokhov
maintainerRe: [Letux-kernel] [PATCH 5/5] input: twl6040-vibra: remove mutex
Quoted 48 lines · H. Nikolaus Schaller
Quoted 31 lines
Am 19.04.2016 um 10:08 schrieb H. Nikolaus Schaller <[email protected]>:
Quoted 6 lines
Am 19.04.2016 um 10:01 schrieb Dmitry Torokhov <[email protected]>:
Quoted 3 lines · H. Nikolaus Schaller
Quoted 3 lines
Am 18.04.2016 um 23:20 schrieb Dmitry Torokhov <[email protected]>:
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines · H. Nikolaus Schaller
Quoted 1 line
The mutex does not seem to be needed.
twl6040_vibra_suspend()andvibra_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.
Quoted 9 lines
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.Quoted 3 lines
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 atomicreport which sometimes ends in a kernel panic.
I have attached such a log.
vibra.pyis here:http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=Letux/root/vibra.py;hb=refs/heads/letux-4.6-rc4Basically it does an ioctl(EVIOCSFF) which triggers
vibra_play.
Quoted 7 lines
Maybe, can you decipher from the log what the reason could be?
I only conjecture that it happens when
vibra_playtries to read the regmap of the twl6040 to gettwl6040_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.
Signature · 1 line
Dmitry
Re: [Letux-kernel] [PATCH 5/5] input: twl6040-vibra: remove mutex
Hi,
Quoted 57 lines
Am 20.04.2016 um 19:49 schrieb Dmitry Torokhov <[email protected]>:
Quoted 54 lines · H. Nikolaus Schaller
Quoted 31 lines
Am 19.04.2016 um 10:08 schrieb H. Nikolaus Schaller <[email protected]>:
Quoted 6 lines
Am 19.04.2016 um 10:01 schrieb Dmitry Torokhov <[email protected]>:
Quoted 3 lines · H. Nikolaus Schaller
Quoted 3 lines
Am 18.04.2016 um 23:20 schrieb Dmitry Torokhov <[email protected]>:
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines · H. Nikolaus Schaller
Quoted 1 line
The mutex does not seem to be needed.
twl6040_vibra_suspend()andvibra_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.
Quoted 9 lines
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.Quoted 3 lines
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 atomicreport which sometimes ends in a kernel panic.
I have attached such a log.
vibra.pyis here:http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=Letux/root/vibra.py;hb=refs/heads/letux-4.6-rc4Basically 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_playtries to read the regmap of the twl6040 to gettwl6040_get_vibralr_status(which has no pendant in the twl4030 driver).
Quoted 6 lines
Quoted 1 line
Do we have to configure the twl6040 regmap differently?
Right,
vibra_playis called with interrupts disabled (that's why we have workqueue to enable/disable regulators, etc, when we start or stop vibration), sotwl6040_get_vibralr_status()should not sleep, but apparently it does.
Yes, regmap using i2c communication may sleep.
Quoted 2 lines
Maybe the check for audio configuration should be
moved intovibra_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
Dmitry Torokhov
maintainerRe: [Letux-kernel] [PATCH 5/5] input: twl6040-vibra: remove mutex
Quoted 66 lines · H. Nikolaus Schaller
Hi,
Quoted 57 lines
Am 20.04.2016 um 19:49 schrieb Dmitry Torokhov <[email protected]>:
Quoted 54 lines · H. Nikolaus Schaller
Quoted 31 lines
Am 19.04.2016 um 10:08 schrieb H. Nikolaus Schaller <[email protected]>:
Quoted 6 lines
Am 19.04.2016 um 10:01 schrieb Dmitry Torokhov <[email protected]>:
Quoted 3 lines · H. Nikolaus Schaller
Quoted 3 lines
Am 18.04.2016 um 23:20 schrieb Dmitry Torokhov <[email protected]>:
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines · H. Nikolaus Schaller
Quoted 1 line
The mutex does not seem to be needed.
twl6040_vibra_suspend()andvibra_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.
Quoted 9 lines
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.Quoted 3 lines
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 atomicreport which sometimes ends in a kernel panic.
I have attached such a log.
vibra.pyis here:http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=Letux/root/vibra.py;hb=refs/heads/letux-4.6-rc4Basically 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_playtries to read the regmap of the twl6040 to gettwl6040_get_vibralr_status(which has no pendant in the twl4030 driver).Quoted 6 lines
Quoted 1 line
Do we have to configure the twl6040 regmap differently?
Right,
vibra_playis called with interrupts disabled (that's why we have workqueue to enable/disable regulators, etc, when we start or stop vibration), sotwl6040_get_vibralr_status()should not sleep, but apparently it does.
Quoted 7 lines
Yes, regmap using i2c communication may sleep.
Quoted 2 lines
Maybe the check for audio configuration should be
moved intovibra_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.
Quoted 3 lines
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.
Quoted 25 lines
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#L462This 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_statusnot 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 totwl6040_get_vibralr_status()and ignoring the result.
Quoted 6 lines
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_statusin 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.
Signature · 1 line
Dmitry
Re: [Letux-kernel] [PATCH 5/5] input: twl6040-vibra: remove mutex
Hi Dmitry,
Quoted 71 lines
Am 20.04.2016 um 20:15 schrieb Dmitry Torokhov <[email protected]>:
Quoted 68 lines · H. Nikolaus Schaller
Hi,
Quoted 57 lines
Am 20.04.2016 um 19:49 schrieb Dmitry Torokhov <[email protected]>:
Quoted 54 lines · H. Nikolaus Schaller
Quoted 31 lines
Am 19.04.2016 um 10:08 schrieb H. Nikolaus Schaller <[email protected]>:
Quoted 6 lines
Am 19.04.2016 um 10:01 schrieb Dmitry Torokhov <[email protected]>:
Quoted 3 lines · H. Nikolaus Schaller
Quoted 3 lines
Am 18.04.2016 um 23:20 schrieb Dmitry Torokhov <[email protected]>:
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines · H. Nikolaus Schaller
Quoted 1 line
The mutex does not seem to be needed.
twl6040_vibra_suspend()andvibra_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.
Quoted 9 lines
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.Quoted 3 lines
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 atomicreport which sometimes ends in a kernel panic.
I have attached such a log.
vibra.pyis here:http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=Letux/root/vibra.py;hb=refs/heads/letux-4.6-rc4Basically 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_playtries to read the regmap of the twl6040 to gettwl6040_get_vibralr_status(which has no pendant in the twl4030 driver).Quoted 6 lines
Quoted 1 line
Do we have to configure the twl6040 regmap differently?
Right,
vibra_playis called with interrupts disabled (that's why we have workqueue to enable/disable regulators, etc, when we start or stop vibration), sotwl6040_get_vibralr_status()should not sleep, but apparently it does.Yes, regmap using i2c communication may sleep.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Maybe the check for audio configuration should be
moved intovibra_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
-EBUSYwas completly useless.
Ah, indeed:
http://lxr.free-electrons.com/source/drivers/input/ff-memless.c#L410
Quoted 6 lines
Quoted 4 lines
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.
Quoted 26 lines
Quoted 26 lines
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#L462This 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_statusnot 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 totwl6040_get_vibralr_status()and ignoring the result.
Quoted 6 lines
Quoted 6 lines
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_statusin 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.
Quoted 3 lines
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