x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"
PATCH v2 1/1 x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"
The annoying static analyzer follow up patches sometimes make a pain rather than fixing issues.
The one done by commit 276c87054751
("x86/platform/intel-mid: Make 'bt_sfi_data' const")
convert the struct to be const while it is used as a temporary container for important data that is used to fill 'parent' and 'name' fields in struct platform_device_info.
That's why revert the commit which had been apparently done w/o reading the code.
Note, compiler doesn't notice this due to explicit cast, which would be addressed separately.
---
v2: low the tone of accusation that this made a regression
arch/x86/platform/intel-mid/device_libs/platform_bt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/platform/intel-mid/device_libs/platform_bt.c +1 -1
@@ -60,7 +60,7 @@ static int __init tng_bt_sfi_setup(struct bt_sfi_data *ddata)return 0 ; } -static const struct bt_sfi_data tng_bt_sfi_data__initdata = { +static struct bt_sfi_data tng_bt_sfi_data__initdata = { .setup = tng_bt_sfi_setup, };
Signature · 1 line
2.15.1
Quoted 1 line · Andy Shevchenko
v2: low the tone of accusation that this made a regression
BTW., don't worry about that aspect too much: after a long debugging session it's pretty natural to be upset at whoever introduced a regression.
( In fact a number of times I too got upset at the moron who wrote a particular
piece of buggy code, only for 'git annotate' to remind me that the moron was me. )
I personally just ignore the emotional attributes, and I usually edit changelogs accordingly as well so the temporary state of mind of finding a regression doesn't trickle upstream.
Plus in this particular case if we can help type propagation for driver data to become a bit cleaner then the kernel project has gained a bit through all this pain.
Thanks,
Ingo
Quoted 7 lines · Ingo Molnar
Quoted 1 line · Andy Shevchenko
v2: low the tone of accusation that this made a regression
BTW., don't worry about that aspect too much: after a long debugging session it's
pretty natural to be upset at whoever introduced a regression.
It appears that regression has been introduced by a new dependency to the hci_bcm.c.
In any case, can we apply this one to 4.15 cycle to make others prevent do an actual regressions further:
commit 03838ae1e8f692dd2bdbd49820ed668d4b7bfbc2
Author: Andi Kleen <[email protected]> Date: Fri Jan 5 13:26:44 2018 +1100
arch/x86/platform/intel-mid/device_libs/platform_bt.c: fix const
confusion
Quoted 12 lines
( In fact a number of times I too got upset at the moron who wrote a particular
piece of buggy code, only for 'git annotate' to remind me that the
moron was me. )I personally just ignore the emotional attributes, and I usually edit changelogs
accordingly as well so the temporary state of mind of finding a regression doesn't
trickle upstream.
Quoted 5 lines
Plus in this particular case if we can help type propagation for driver data to
become a bit cleaner then the kernel project has gained a bit through all this
pain.
I has been thinking if 0day can complain about these:
1) castings in new code
2) applying const to older *working* code
Fengguang, what do you think?
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy
Quoted 40 lines · Andy Shevchenko
Quoted 7 lines · Ingo Molnar
Quoted 1 line · Andy Shevchenko
v2: low the tone of accusation that this made a regression
BTW., don't worry about that aspect too much: after a long debugging session it's
pretty natural to be upset at whoever introduced a regression.It appears that regression has been introduced by a new dependency to the
hci_bcm.c.In any case, can we apply this one to 4.15 cycle to make others prevent do an actual regressions further:
commit
03838ae1e8f692dd2bdbd49820ed668d4b7bfbc2Author: Andi Kleen <[email protected]> Date: Fri Jan 5 13:26:44 2018 +1100
arch/x86/platform/intel-mid/device_libs/platform_bt.c: fix const
confusionQuoted 12 lines
( In fact a number of times I too got upset at the moron who wrote a particular
piece of buggy code, only for 'git annotate' to remind me that the
moron was me. )I personally just ignore the emotional attributes, and I usually edit changelogs
accordingly as well so the temporary state of mind of finding a regression doesn't
trickle upstream.Quoted 5 lines
Plus in this particular case if we can help type propagation for driver data to
become a bit cleaner then the kernel project has gained a bit through all this
pain.
Quoted 5 lines
I has been thinking if 0day can complain about these:
1) castings in new code
2) applying const to older *working* codeFengguang, what do you think?
As suggested previously, I think it would be better if code that retrieves const structures from nonconst fields would store the result in a const variable. Then the compiler would pick up the verification from there. A Coccinelle script can detect cases where this property does not hold, at least within a single file. This would have protected the given code and still allowed constant structures to be made const.
julia
tip-bot for Andy Shevchenko
merge botx86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"
Commit-ID: 9d0513d82f1a8fe17b41f113ac5922fa57dbaf5c Gitweb: https://git.kernel.org/tip/9d0513d82f1a8fe17b41f113ac5922fa57dbaf5c Author: Andy Shevchenko <[email protected]> AuthorDate: Thu, 28 Dec 2017 14:25:23 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Mon, 8 Jan 2018 20:01:44 +0100
x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"
So one of the constification patches unearthed a type casting fragility of the underlying code:
276c87054751 ("x86/platform/intel-mid: Make 'bt_sfi_data' const")
converted the struct to be const while it is also used as a temporary container for important data that is used to fill 'parent' and 'name' fields in struct platform_device_info.
The compiler doesn't notice this due to an explicit type cast that loses the const - which fragility will be fixed separately.
This type cast turned a seemingly trivial const propagation patch into a hard to debug data corruptor and crasher bug.
--- arch/x86/platform/intel-mid/device_libs/platform_bt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/platform/intel-mid/device_libs/platform_bt.c +1 -1
@@ -60,7 +60,7 @@ static int __init tng_bt_sfi_setup(struct bt_sfi_data *ddata)return 0 ; } -static const struct bt_sfi_data tng_bt_sfi_data__initdata = { +static struct bt_sfi_data tng_bt_sfi_data__initdata = { .setup = tng_bt_sfi_setup, };
Quoted 1 line · tip-bot for Andy Shevchenko
Committer: Ingo Molnar <[email protected]>
Thank you!
Signature · 2 lines
Andy Shevchenko <[email protected]> Intel Finland Oy