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

PATCH Superseded Andy Shevchenko 3 participants 17 messages Dec 28, 2017 – Jan 1, 2018

Subsystems (3)

x86 x86 platform drivers Intel MID (Mobile Internet Device) platform

lore

Share & tools

lore

Andy Shevchenko

author

2017-12-28 10:08 UTC #0 lore raw

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

The annoying static analyzer follow up patches make a pain rather then fixing issues.

The one done by commit 276c87054751

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

made an obvious regression [BugLink] since the struct bt_sfi_data 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.

BugLink https://github.com/andy-shev/linux/issues/20

Cc Bhumika Goyal <[email protected]>

Cc [email protected]

Signed-off-by Andy Shevchenko <[email protected]>

---
 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

Ingo Molnar

2017-12-28 10:28 UTC #1 lore raw

Quoted 27 lines · Andy Shevchenko

The annoying static analyzer follow up patches make a pain rather then fixing issues.
The one done by commit 276c87054751
("x86/platform/intel-mid: Make 'bt_sfi_data' const")
made an obvious regression [BugLink] since the struct bt_sfi_data 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.
BugLink https://github.com/andy-shev/linux/issues/20
Cc Bhumika Goyal <[email protected]>
Cc [email protected]
Signed-off-by Andy Shevchenko <[email protected]>
---
 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 +0 -0
index dc036e511f48..5a0483e7bf66 100644

arch/x86/platform/intel-mid/device_libs/platform_bt.c static int __init tng_bt_sfi_setup(struct bt_sfi_data *ddata) · line 65

Quoted 7 lines

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,
 };

This is nasty, why didn't the compiler warn about this bug?

Normally when using a const data structure for a non-const purpose. (Unless there's a type cast which loses the type - one of the many reasons why type casts should be avoided.)

Thanks,

Ingo

Ingo Molnar

2017-12-28 11:12 UTC #3 lore raw

Quoted 50 lines · Julia Lawall

Quoted 40 lines · Ingo Molnar
Quoted 27 lines · Andy Shevchenko
The annoying static analyzer follow up patches make a pain rather then fixing issues.
The one done by commit 276c87054751
("x86/platform/intel-mid: Make 'bt_sfi_data' const")
made an obvious regression [BugLink] since the struct bt_sfi_data 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.
BugLink https://github.com/andy-shev/linux/issues/20
Cc Bhumika Goyal <[email protected]>
Cc [email protected]
Signed-off-by Andy Shevchenko <[email protected]>
---
 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 +0 -0
index dc036e511f48..5a0483e7bf66 100644
arch/x86/platform/intel-mid/device_libs/platform_bt.c static int __init tng_bt_sfi_setup(struct bt_sfi_data *ddata) · line 65
Quoted 7 lines
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,
 };
This is nasty, why didn't the compiler warn about this bug?
Quoted 3 lines
Normally when using a const data structure for a non-const purpose. (Unless there's a type cast which loses the type - one of the many reasons why type casts should be avoided.)
Indeed, because there is a cast:

Quoted 7 lines

#define ICPU(model, ddata)      \
        { X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, (kernel_ulong_t)&ddata }

static const struct x86_cpu_id bt_sfi_cpu_ids[] = {
	ICPU(INTEL_FAM6_ATOM_MERRIFIELD, tng_bt_sfi_data),
        {}
};

So the type is the following, in include/linux/mod_devicetable.h:

struct x86_cpu_id {
        __u16 vendor;
        __u16 family;
        __u16 model;
        __u16 feature;  /* bit index */
        kernel_ulong_t driver_data;
};

Is there a syntactic method that would allow the conversion to kernel_ulong_t, but would preserve any const-ness?

Barring that, maybe we could convert driver_data to 'void *', fix up all users, and not force the type - this would allow the preservation of the const attribute, I think.

BTW., a quick grep suggests similar type casting patterns here:

arch/x86/platform/intel-mid/pwr.c: { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), (kernel_ulong_t)&pnw_info },
arch/x86/platform/intel-mid/pwr.c: { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), (kernel_ulong_t)&tng_info },

Thanks,

Ingo

Julia Lawall

2017-12-28 11:23 UTC #4 lore raw

Quoted 74 lines · Ingo Molnar

Quoted 50 lines · Julia Lawall
Quoted 40 lines · Ingo Molnar
Quoted 27 lines · Andy Shevchenko
The annoying static analyzer follow up patches make a pain rather then fixing issues.
The one done by commit 276c87054751
("x86/platform/intel-mid: Make 'bt_sfi_data' const")
made an obvious regression [BugLink] since the struct bt_sfi_data 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.
BugLink https://github.com/andy-shev/linux/issues/20
Cc Bhumika Goyal <[email protected]>
Cc [email protected]
Signed-off-by Andy Shevchenko <[email protected]>
---
 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 +0 -0
index dc036e511f48..5a0483e7bf66 100644
arch/x86/platform/intel-mid/device_libs/platform_bt.c static int __init tng_bt_sfi_setup(struct bt_sfi_data *ddata) · line 65
Quoted 7 lines
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,
 };
This is nasty, why didn't the compiler warn about this bug?
Quoted 3 lines
Normally when using a const data structure for a non-const purpose. (Unless there's a type cast which loses the type - one of the many reasons why type casts should be avoided.)
Indeed, because there is a cast:
Quoted 7 lines
#define ICPU(model, ddata)      \
        { X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, (kernel_ulong_t)&ddata }
static const struct x86_cpu_id bt_sfi_cpu_ids[] = {
	ICPU(INTEL_FAM6_ATOM_MERRIFIELD, tng_bt_sfi_data),
        {}
};
So the type is the following, in include/linux/mod_devicetable.h:
struct x86_cpu_id {
        __u16 vendor;
        __u16 family;
        __u16 model;
        __u16 feature;  /* bit index */
        kernel_ulong_t driver_data;
};
Is there a syntactic method that would allow the conversion to kernel_ulong_t, but would preserve any const-ness?

Quoted 8 lines

Barring that, maybe we could convert driver_data to 'void *', fix up all users, and not force the type - this would allow the preservation of the const attribute, I think.
BTW., a quick grep suggests similar type casting patterns here:
arch/x86/platform/intel-mid/pwr.c: { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), (kernel_ulong_t)&pnw_info },
arch/x86/platform/intel-mid/pwr.c: { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), (kernel_ulong_t)&tng_info },

Would it be acceptable to move the cast out of the macro to the place where the variable is referenced? It wouldn't help with the compiler, but it would be slightly easier for the human to check.

julia

Quoted 5 lines

Thanks,
Ingo

Ingo Molnar

2017-12-28 11:59 UTC #5 lore raw

Quoted 17 lines · Julia Lawall

Quoted 17 lines
So the type is the following, in include/linux/mod_devicetable.h:
struct x86_cpu_id {
        __u16 vendor;
        __u16 family;
        __u16 model;
        __u16 feature;  /* bit index */
        kernel_ulong_t driver_data;
};
Is there a syntactic method that would allow the conversion to kernel_ulong_t, but would preserve any const-ness?
Barring that, maybe we could convert driver_data to 'void *', fix up all users, and not force the type - this would allow the preservation of the const attribute, I think.

Quoted 8 lines

Quoted 4 lines
BTW., a quick grep suggests similar type casting patterns here:
arch/x86/platform/intel-mid/pwr.c: { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), (kernel_ulong_t)&pnw_info },
arch/x86/platform/intel-mid/pwr.c: { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), (kernel_ulong_t)&tng_info },
Would it be acceptable to move the cast out of the macro to the place where the variable is referenced? It wouldn't help with the compiler, but it would be slightly easier for the human to check.

It would really be preferably to not stand in the way of the compiler here.

AFACS void * should solve the problem, and it's a kernel_ulong_t equivalent in terms of with, right?

So let's try and solve this for real. Forcing humans to discover such things is always a fragile concept.

Thanks,

Ingo

Andy Shevchenko

author

2017-12-28 12:12 UTC #6 lore raw

In reply to Ingo Molnar

Quoted 44 lines · Ingo Molnar

Quoted 31 lines · Andy Shevchenko
The annoying static analyzer follow up patches make a pain rather then
fixing issues.
The one done by commit 276c87054751
("x86/platform/intel-mid: Make 'bt_sfi_data' const")
made an obvious regression [BugLink] since the struct bt_sfi_data 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.
BugLink https://github.com/andy-shev/linux/issues/20
Cc Bhumika Goyal <[email protected]>
Cc [email protected]
Signed-off-by Andy Shevchenko <[email protected]>
---
 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 +0 -0
index dc036e511f48..5a0483e7bf66 100644
arch/x86/platform/intel-mid/device_libs/platform_bt.c static int __init tng_bt_sfi_setup(struct · line 66
Quoted 8 lines
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,
 };
This is nasty, why didn't the compiler warn about this bug?

Quoted 5 lines

Normally when using a const data structure for a non-const purpose. (Unless
there's a type cast which loses the type - one of the many reasons why type casts
should be avoided.)

Now I'm trying to get this.

First of all, the new dependency to hci_bcm makes this one not compiled at all.

Second, there is a cast as you truthfully predicted...

I would say that revert is needed, but it seems it wasn't a culprit for the bug (rather the new dependency is). So, it might need rewording of the commit message to low tone of the accusations.

Signature · 2 lines

Andy Shevchenko <[email protected]>
Intel Finland Oy

Ingo Molnar

2017-12-28 12:17 UTC #7 lore raw

Quoted 53 lines · Andy Shevchenko

Quoted 44 lines · Ingo Molnar
Quoted 31 lines · Andy Shevchenko
The annoying static analyzer follow up patches make a pain rather then
fixing issues.
The one done by commit 276c87054751
("x86/platform/intel-mid: Make 'bt_sfi_data' const")
made an obvious regression [BugLink] since the struct bt_sfi_data 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.
BugLink https://github.com/andy-shev/linux/issues/20
Cc Bhumika Goyal <[email protected]>
Cc [email protected]
Signed-off-by Andy Shevchenko <[email protected]>
---
 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 +0 -0
index dc036e511f48..5a0483e7bf66 100644
arch/x86/platform/intel-mid/device_libs/platform_bt.c static int __init tng_bt_sfi_setup(struct · line 66
Quoted 8 lines
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,
 };
This is nasty, why didn't the compiler warn about this bug?
Quoted 5 lines
Normally when using a const data structure for a non-const purpose. (Unless
there's a type cast which loses the type - one of the many reasons why type casts
should be avoided.)
Now I'm trying to get this.

Quoted 8 lines

First of all, the new dependency to hci_bcm makes this one not compiled at all.
Second, there is a cast as you truthfully predicted...
I would say that revert is needed, but it seems it wasn't a culprit for the bug (rather the new dependency is). So, it might need rewording of the commit message to low tone of the accusations.

Your fix is absolutely needed and welcome, but I'd first like to see a build error or build warning that avoids the introduction of this class of problems in the future - then apply your fix in a separate patch.

Constification patches are useful in general, and such breakages are hard to debug
...

Thanks,

Ingo

Julia Lawall

2017-12-28 12:25 UTC #8 lore raw

Quoted 65 lines · Ingo Molnar

Quoted 53 lines · Andy Shevchenko
Quoted 44 lines · Ingo Molnar
Quoted 31 lines · Andy Shevchenko
The annoying static analyzer follow up patches make a pain rather then
fixing issues.
The one done by commit 276c87054751
("x86/platform/intel-mid: Make 'bt_sfi_data' const")
made an obvious regression [BugLink] since the struct bt_sfi_data 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.
BugLink https://github.com/andy-shev/linux/issues/20
Cc Bhumika Goyal <[email protected]>
Cc [email protected]
Signed-off-by Andy Shevchenko <[email protected]>
---
 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 +0 -0
index dc036e511f48..5a0483e7bf66 100644
arch/x86/platform/intel-mid/device_libs/platform_bt.c static int __init tng_bt_sfi_setup(struct · line 66
Quoted 8 lines
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,
 };
This is nasty, why didn't the compiler warn about this bug?
Quoted 5 lines
Normally when using a const data structure for a non-const purpose. (Unless
there's a type cast which loses the type - one of the many reasons why type casts
should be avoided.)
Now I'm trying to get this.
Quoted 8 lines
First of all, the new dependency to hci_bcm makes this one not compiled at all.
Second, there is a cast as you truthfully predicted...
I would say that revert is needed, but it seems it wasn't a culprit for the bug (rather the new dependency is). So, it might need rewording of the commit message to low tone of the accusations.

Quoted 5 lines

Your fix is absolutely needed and welcome, but I'd first like to see a build error or build warning that avoids the introduction of this class of problems in the future - then apply your fix in a separate patch.
Constification patches are useful in general, and such breakages are hard to debug

I will try to make the type adjustment. There does seem to be a few cases where the field actually does hold an integer. I guess this is not a problem?

julia

Ingo Molnar

2017-12-28 12:29 UTC #9 lore raw

Quoted 69 lines · Julia Lawall

Quoted 65 lines · Ingo Molnar
Quoted 61 lines · Andy Shevchenko
Quoted 44 lines · Ingo Molnar
Quoted 31 lines · Andy Shevchenko
The annoying static analyzer follow up patches make a pain rather then
fixing issues.
The one done by commit 276c87054751
("x86/platform/intel-mid: Make 'bt_sfi_data' const")
made an obvious regression [BugLink] since the struct bt_sfi_data 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.
BugLink https://github.com/andy-shev/linux/issues/20
Cc Bhumika Goyal <[email protected]>
Cc [email protected]
Signed-off-by Andy Shevchenko <[email protected]>
---
 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 +0 -0
index dc036e511f48..5a0483e7bf66 100644
arch/x86/platform/intel-mid/device_libs/platform_bt.c static int __init tng_bt_sfi_setup(struct · line 66
Quoted 8 lines
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,
 };
This is nasty, why didn't the compiler warn about this bug?
Quoted 5 lines
Normally when using a const data structure for a non-const purpose. (Unless
there's a type cast which loses the type - one of the many reasons why type casts
should be avoided.)
Now I'm trying to get this.
First of all, the new dependency to hci_bcm makes this one not compiled at all.
Second, there is a cast as you truthfully predicted...
I would say that revert is needed, but it seems it wasn't a culprit for the bug (rather the new dependency is). So, it might need rewording of the commit message to low tone of the accusations.

Quoted 7 lines

Quoted 5 lines
Your fix is absolutely needed and welcome, but I'd first like to see a build error or build warning that avoids the introduction of this class of problems in the future - then apply your fix in a separate patch.
Constification patches are useful in general, and such breakages are hard to debug
I will try to make the type adjustment.

Assuming it's all a natural improvement to the affected code. I'm really just guessing blindly here - reality might interfere!

Quoted 2 lines

[...] There does seem to be a few cases where the field actually does hold an integer. I guess this is not a problem?

Could you point to such an example?

Thanks,

Ingo

Julia Lawall

2017-12-28 12:38 UTC #10 lore raw

Quoted 80 lines · Ingo Molnar

Quoted 69 lines · Julia Lawall
Quoted 65 lines · Ingo Molnar
Quoted 61 lines · Andy Shevchenko
Quoted 44 lines · Ingo Molnar
Quoted 31 lines · Andy Shevchenko
The annoying static analyzer follow up patches make a pain rather then
fixing issues.
The one done by commit 276c87054751
("x86/platform/intel-mid: Make 'bt_sfi_data' const")
made an obvious regression [BugLink] since the struct bt_sfi_data 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.
BugLink https://github.com/andy-shev/linux/issues/20
Cc Bhumika Goyal <[email protected]>
Cc [email protected]
Signed-off-by Andy Shevchenko <[email protected]>
---
 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 +0 -0
index dc036e511f48..5a0483e7bf66 100644
arch/x86/platform/intel-mid/device_libs/platform_bt.c static int __init tng_bt_sfi_setup(struct · line 66
Quoted 8 lines
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,
 };
This is nasty, why didn't the compiler warn about this bug?
Quoted 5 lines
Normally when using a const data structure for a non-const purpose. (Unless
there's a type cast which loses the type - one of the many reasons why type casts
should be avoided.)
Now I'm trying to get this.
First of all, the new dependency to hci_bcm makes this one not compiled at all.
Second, there is a cast as you truthfully predicted...
I would say that revert is needed, but it seems it wasn't a culprit for the bug (rather the new dependency is). So, it might need rewording of the commit message to low tone of the accusations.
Quoted 7 lines
Quoted 5 lines
Your fix is absolutely needed and welcome, but I'd first like to see a build error or build warning that avoids the introduction of this class of problems in the future - then apply your fix in a separate patch.
Constification patches are useful in general, and such breakages are hard to debug
I will try to make the type adjustment.

Quoted 7 lines

Assuming it's all a natural improvement to the affected code. I'm really just guessing blindly here - reality might interfere!
Quoted 2 lines
[...] There does seem to be a few cases where the field actually does hold an integer. I guess this is not a problem?
Could you point to such an example?

drivers/thermal/intel_soc_dts_thermal.c:#define BYT_SOC_DTS_APIC_IRQ 86

and then:

static const struct x86_cpu_id soc_thermal_ids[] = {
        { X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_SILVERMONT1, 0,
                BYT_SOC_DTS_APIC_IRQ},
        {}
};

and finally:

soc_dts_thres_irq = (int)match_cpu->driver_data;

Also:

arch/x86/kernel/apic/apic.c

#define DEADLINE_MODEL_MATCH_REV(model, rev) \
{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, (unsigned long)rev
}

DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_BROADWELL_X, 0x0b000020),
DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_HASWELL_CORE, 0x22),
etc. (all 2-digit numbers in the remaining case).

julia

Ingo Molnar

2017-12-28 12:53 UTC #11 lore raw

Quoted 23 lines · Julia Lawall

Quoted 4 lines
Quoted 2 lines
[...] There does seem to be a few cases where the field actually does hold an integer. I guess this is not a problem?
Could you point to such an example?
drivers/thermal/intel_soc_dts_thermal.c:#define BYT_SOC_DTS_APIC_IRQ 86
and then:
static const struct x86_cpu_id soc_thermal_ids[] = {
        { X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_SILVERMONT1, 0,
                BYT_SOC_DTS_APIC_IRQ},
        {}
};
and finally:
soc_dts_thres_irq = (int)match_cpu->driver_data;
Also:
arch/x86/kernel/apic/apic.c

Quoted 7 lines

#define DEADLINE_MODEL_MATCH_REV(model, rev) \
{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, (unsigned long)rev
}
DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_BROADWELL_X, 0x0b000020),
DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_HASWELL_CORE, 0x22),
etc. (all 2-digit numbers in the remaining case).

Ok - I think in these cases the resulting long->pointer type conversion is a _lot_ less dangerous than the pointer->long conversion which caused the regression.

So unless the resulting code is excessively ugly, this feels like the right approach to me.

Thanks,

Ingo

Julia Lawall

2017-12-28 14:00 UTC #12 lore raw

Quoted 34 lines · Ingo Molnar

Quoted 23 lines · Julia Lawall
Quoted 4 lines
Quoted 2 lines
[...] There does seem to be a few cases where the field actually does hold an integer. I guess this is not a problem?
Could you point to such an example?
drivers/thermal/intel_soc_dts_thermal.c:#define BYT_SOC_DTS_APIC_IRQ 86
and then:
static const struct x86_cpu_id soc_thermal_ids[] = {
        { X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_SILVERMONT1, 0,
                BYT_SOC_DTS_APIC_IRQ},
        {}
};
and finally:
soc_dts_thres_irq = (int)match_cpu->driver_data;
Also:
arch/x86/kernel/apic/apic.c
Quoted 7 lines
#define DEADLINE_MODEL_MATCH_REV(model, rev) \
{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, (unsigned long)rev
}
DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_BROADWELL_X, 0x0b000020),
DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_HASWELL_CORE, 0x22),
etc. (all 2-digit numbers in the remaining case).

Quoted 5 lines

Ok - I think in these cases the resulting long->pointer type conversion is a _lot_ less dangerous than the pointer->long conversion which caused the regression.
So unless the resulting code is excessively ugly, this feels like the right approach to me.

The problem is that this case will inevitably have a cast somewhere. Many of the values put into the driver_data field really are const, so the type has to be const void *. When the value is extracted from the structure, there will thus need to be a cast on it, and the current cast

ddata = (struct bt_sfi_data *)id->driver_data;

works fine, whether the original structure is const or not.

I also got a couple of complaints about non-pointer types:

arch/x86/kernel/apic/apic.c:621:9: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
rev = (u32)m->driver_data;

drivers/thermal/intel_soc_dts_thermal.c:68:22: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
soc_dts_thres_irq = (int)match_cpu->driver_data;

julia

Ingo Molnar

2017-12-28 15:24 UTC #13 lore raw

Quoted 38 lines · Julia Lawall

Quoted 34 lines · Ingo Molnar
Quoted 30 lines · Julia Lawall
Quoted 4 lines
Quoted 2 lines
[...] There does seem to be a few cases where the field actually does hold an integer. I guess this is not a problem?
Could you point to such an example?
drivers/thermal/intel_soc_dts_thermal.c:#define BYT_SOC_DTS_APIC_IRQ 86
and then:
static const struct x86_cpu_id soc_thermal_ids[] = {
        { X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_SILVERMONT1, 0,
                BYT_SOC_DTS_APIC_IRQ},
        {}
};
and finally:
soc_dts_thres_irq = (int)match_cpu->driver_data;
Also:
arch/x86/kernel/apic/apic.c
#define DEADLINE_MODEL_MATCH_REV(model, rev) \
{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, (unsigned long)rev
}
DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_BROADWELL_X, 0x0b000020),
DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_HASWELL_CORE, 0x22),
etc. (all 2-digit numbers in the remaining case).

Quoted 7 lines

Quoted 5 lines
Ok - I think in these cases the resulting long->pointer type conversion is a _lot_ less dangerous than the pointer->long conversion which caused the regression.
So unless the resulting code is excessively ugly, this feels like the right approach to me.
The problem is that this case will inevitably have a cast somewhere.

That's OK as long as the cast is dominantly (long)->(pointer), because that doesn't really risk losing the underlying type.

It's the (pointer)->(pointer) and (pointer)->(long) conversions that are the most dangerous ones.

Quoted 7 lines

[...] Many of the values put into the driver_data field really are const, so the type has to be const void *. When the value is extracted from the structure, there will thus need to be a cast on it, and the current cast
ddata = (struct bt_sfi_data *)id->driver_data;
works fine, whether the original structure is const or not.

So since this data structure is not size critical, I'd really suggest using two or three fields:

	->driver_data.ptr
	->driver_data.const_ptr
	->driver_data.long

that way the fundamental types remains.

Quoted 10 lines

I also got a couple of complaints about non-pointer types:
arch/x86/kernel/apic/apic.c:621:9: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
rev = (u32)m->driver_data;
drivers/thermal/intel_soc_dts_thermal.c:68:22: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
soc_dts_thres_irq = (int)match_cpu->driver_data;

These could use driver_data.long or so?

Thanks,

Ingo

Julia Lawall

2017-12-28 15:28 UTC #14 lore raw

Quoted 44 lines · Ingo Molnar

Quoted 41 lines · Julia Lawall
Quoted 37 lines · Ingo Molnar
Quoted 23 lines · Julia Lawall
Quoted 4 lines
Quoted 2 lines
[...] There does seem to be a few cases where the field actually does hold an integer. I guess this is not a problem?
Could you point to such an example?
drivers/thermal/intel_soc_dts_thermal.c:#define BYT_SOC_DTS_APIC_IRQ 86
and then:
static const struct x86_cpu_id soc_thermal_ids[] = {
        { X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_SILVERMONT1, 0,
                BYT_SOC_DTS_APIC_IRQ},
        {}
};
and finally:
soc_dts_thres_irq = (int)match_cpu->driver_data;
Also:
arch/x86/kernel/apic/apic.c
Quoted 7 lines
#define DEADLINE_MODEL_MATCH_REV(model, rev) \
{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, (unsigned long)rev
}
DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_BROADWELL_X, 0x0b000020),
DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_HASWELL_CORE, 0x22),
etc. (all 2-digit numbers in the remaining case).
Ok - I think in these cases the resulting long->pointer type conversion is a _lot_ less dangerous than the pointer->long conversion which caused the regression.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
So unless the resulting code is excessively ugly, this feels like the right approach to me.
The problem is that this case will inevitably have a cast somewhere.
That's OK as long as the cast is dominantly (long)->(pointer), because that doesn't really risk losing the underlying type.

Not sure to follow. Currently, with the const void * the bad code compiles fine. A cast before putting the value into the structure has been replaced by a cast on extracting it form the structure, but there is still a cast and the const declaration of the original structure is still lost.

Quoted 11 lines

It's the (pointer)->(pointer) and (pointer)->(long) conversions that are the most dangerous ones.
Quoted 7 lines
[...] Many of the values put into the driver_data field really are const, so the type has to be const void *. When the value is extracted from the structure, there will thus need to be a cast on it, and the current cast
ddata = (struct bt_sfi_data *)id->driver_data;
works fine, whether the original structure is const or not.

Quoted 8 lines

So since this data structure is not size critical, I'd really suggest using two or three fields:
	->driver_data.ptr
	->driver_data.const_ptr
	->driver_data.long
that way the fundamental types remains.

OK this looks promising. I will try it.

Quoted 7 lines

Quoted 7 lines
I also got a couple of complaints about non-pointer types:
arch/x86/kernel/apic/apic.c:621:9: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
rev = (u32)m->driver_data;

Quoted 5 lines

Quoted 3 lines
drivers/thermal/intel_soc_dts_thermal.c:68:22: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
soc_dts_thres_irq = (int)match_cpu->driver_data;
These could use driver_data.long or so?

Seems probable.

julia

Julia Lawall

2017-12-28 21:22 UTC #15 lore raw

In reply to Ingo Molnar

Quoted 63 lines · Ingo Molnar

Quoted 38 lines · Julia Lawall
Quoted 34 lines · Ingo Molnar
Quoted 30 lines · Julia Lawall
Quoted 4 lines
Quoted 2 lines
[...] There does seem to be a few cases where the field actually does hold an integer. I guess this is not a problem?
Could you point to such an example?
drivers/thermal/intel_soc_dts_thermal.c:#define BYT_SOC_DTS_APIC_IRQ 86
and then:
static const struct x86_cpu_id soc_thermal_ids[] = {
        { X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_SILVERMONT1, 0,
                BYT_SOC_DTS_APIC_IRQ},
        {}
};
and finally:
soc_dts_thres_irq = (int)match_cpu->driver_data;
Also:
arch/x86/kernel/apic/apic.c
#define DEADLINE_MODEL_MATCH_REV(model, rev) \
{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, (unsigned long)rev
}
DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_BROADWELL_X, 0x0b000020),
DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_HASWELL_CORE, 0x22),
etc. (all 2-digit numbers in the remaining case).
Quoted 7 lines
Quoted 5 lines
Ok - I think in these cases the resulting long->pointer type conversion is a _lot_ less dangerous than the pointer->long conversion which caused the regression.
So unless the resulting code is excessively ugly, this feels like the right approach to me.
The problem is that this case will inevitably have a cast somewhere.
That's OK as long as the cast is dominantly (long)->(pointer), because that doesn't really risk losing the underlying type.
It's the (pointer)->(pointer) and (pointer)->(long) conversions that are the most dangerous ones.
Quoted 7 lines
[...] Many of the values put into the driver_data field really are const, so the type has to be const void *. When the value is extracted from the structure, there will thus need to be a cast on it, and the current cast
ddata = (struct bt_sfi_data *)id->driver_data;
works fine, whether the original structure is const or not.

Quoted 8 lines

So since this data structure is not size critical, I'd really suggest using two or three fields:
	->driver_data.ptr
	->driver_data.const_ptr
	->driver_data.long
that way the fundamental types remains.

Nothing will ensure that the data is extracted via the same field that was used to store it. But one will get a message from the compiler if one tries to change the properties of the stored value without changing the field that it is stored into.

Should there be a union?

Is there some other kernel code that takes this solution?

thanks,
julia

Quoted 18 lines

Quoted 11 lines
I also got a couple of complaints about non-pointer types:
arch/x86/kernel/apic/apic.c:621:9: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
rev = (u32)m->driver_data;
drivers/thermal/intel_soc_dts_thermal.c:68:22: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
soc_dts_thres_irq = (int)match_cpu->driver_data;
These could use driver_data.long or so?
Thanks,
Ingo

Julia Lawall

2018-01-01 15:15 UTC #16 lore raw

In reply to Ingo Molnar

Quoted 61 lines · Ingo Molnar

Quoted 38 lines · Julia Lawall
Quoted 34 lines · Ingo Molnar
Quoted 30 lines · Julia Lawall
Quoted 4 lines
Quoted 2 lines
[...] There does seem to be a few cases where the field actually does hold an integer. I guess this is not a problem?
Could you point to such an example?
drivers/thermal/intel_soc_dts_thermal.c:#define BYT_SOC_DTS_APIC_IRQ 86
and then:
static const struct x86_cpu_id soc_thermal_ids[] = {
        { X86_VENDOR_INTEL, 6, INTEL_FAM6_ATOM_SILVERMONT1, 0,
                BYT_SOC_DTS_APIC_IRQ},
        {}
};
and finally:
soc_dts_thres_irq = (int)match_cpu->driver_data;
Also:
arch/x86/kernel/apic/apic.c
#define DEADLINE_MODEL_MATCH_REV(model, rev) \
{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, (unsigned long)rev
}
DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_BROADWELL_X, 0x0b000020),
DEADLINE_MODEL_MATCH_REV ( INTEL_FAM6_HASWELL_CORE, 0x22),
etc. (all 2-digit numbers in the remaining case).
Quoted 7 lines
Quoted 5 lines
Ok - I think in these cases the resulting long->pointer type conversion is a _lot_ less dangerous than the pointer->long conversion which caused the regression.
So unless the resulting code is excessively ugly, this feels like the right approach to me.
The problem is that this case will inevitably have a cast somewhere.
That's OK as long as the cast is dominantly (long)->(pointer), because that doesn't really risk losing the underlying type.
It's the (pointer)->(pointer) and (pointer)->(long) conversions that are the most dangerous ones.
Quoted 6 lines
[...] Many of the values put into the driver_data field really are const, so the type has to be const void *. When the value is extracted from the structure, there will thus need to be a cast on it, and the current cast
ddata = (struct bt_sfi_data *)id->driver_data;

Quoted 8 lines

Quoted 1 line
works fine, whether the original structure is const or not.
So since this data structure is not size critical, I'd really suggest using two or three fields:
	->driver_data.ptr
	->driver_data.const_ptr
	->driver_data.long

I'm still not sure this is a good idea. If it really is a structure, there will be uninitialized fields that can be accessed by mistake. And nothing checks if the code consistently uses the right field.

Another approach would be to make a semantic patch that finds the problem without too many false positives, and get that into the kernel so that the 0-day testing service will run it on all new patches. The idea would be that if there are some const structures of a given type, then all of the other occurrences of that type in the same file should be const. It wouldn't find all occurrences of the problem, but it should find the one in question. In general, the problem arises with uninterpreted data, and such data is typically used only in the file where it was defined (or perhaps in other files implementing the same driver).

julia

Quoted 20 lines

that way the fundamental types remains.
Quoted 10 lines
I also got a couple of complaints about non-pointer types:
arch/x86/kernel/apic/apic.c:621:9: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
rev = (u32)m->driver_data;
drivers/thermal/intel_soc_dts_thermal.c:68:22: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
soc_dts_thres_irq = (int)match_cpu->driver_data;
These could use driver_data.long or so?
Thanks,
Ingo