Skip to contentSkip to conversation

x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

PATCH In review Andy Shevchenko 4 participants 6 messages Dec 28, 2017 – Jan 9, 2018

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

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
index dc036e511f48..5a0483e7bf66 100644
@@ -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
#1 lore raw
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

#2 lore raw
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
#3 lore raw
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 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.

Quoted 5 lines

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?

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

#4 lore raw
In reply to Andy Shevchenko
x86/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
index dc036e5..5a0483e 100644
@@ -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,
 };