| Message ID | 20171228100801.67744-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
* Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > 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 <bhumirks@gmail.com> > Cc: julia.lawall@lip6.fr > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > arch/x86/platform/intel-mid/device_libs/platform_bt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/platform/intel-mid/device_libs/platform_bt.c b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > index dc036e511f48..5a0483e7bf66 100644 > --- a/arch/x86/platform/intel-mid/device_libs/platform_bt.c > +++ b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > @@ -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
On Thu, 28 Dec 2017, Ingo Molnar wrote: > > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > 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 <bhumirks@gmail.com> > > Cc: julia.lawall@lip6.fr > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > arch/x86/platform/intel-mid/device_libs/platform_bt.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/x86/platform/intel-mid/device_libs/platform_bt.c b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > index dc036e511f48..5a0483e7bf66 100644 > > --- a/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > +++ b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > @@ -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.) Indeed, because there is a cast: #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), {} }; julia
* Julia Lawall <julia.lawall@lip6.fr> wrote: > > > On Thu, 28 Dec 2017, Ingo Molnar wrote: > > > > > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > 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 <bhumirks@gmail.com> > > > Cc: julia.lawall@lip6.fr > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > arch/x86/platform/intel-mid/device_libs/platform_bt.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/platform/intel-mid/device_libs/platform_bt.c b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > index dc036e511f48..5a0483e7bf66 100644 > > > --- a/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > +++ b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > @@ -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.) > > Indeed, because there is a cast: > > #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
On Thu, 28 Dec 2017, Ingo Molnar wrote: > > * Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > > > > On Thu, 28 Dec 2017, Ingo Molnar wrote: > > > > > > > > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > 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 <bhumirks@gmail.com> > > > > Cc: julia.lawall@lip6.fr > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > --- > > > > arch/x86/platform/intel-mid/device_libs/platform_bt.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/arch/x86/platform/intel-mid/device_libs/platform_bt.c b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > > index dc036e511f48..5a0483e7bf66 100644 > > > > --- a/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > > +++ b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > > @@ -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.) > > > > Indeed, because there is a cast: > > > > #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 }, 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 > > Thanks, > > Ingo >
* Julia Lawall <julia.lawall@lip6.fr> wrote: > > 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 }, > > 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
On Thu, 2017-12-28 at 11:28 +0100, Ingo Molnar wrote: > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > 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 <bhumirks@gmail.com> > > Cc: julia.lawall@lip6.fr > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > arch/x86/platform/intel-mid/device_libs/platform_bt.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > index dc036e511f48..5a0483e7bf66 100644 > > --- a/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > +++ b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > @@ -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.) 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.
* Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Thu, 2017-12-28 at 11:28 +0100, Ingo Molnar wrote: > > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > 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 <bhumirks@gmail.com> > > > Cc: julia.lawall@lip6.fr > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > arch/x86/platform/intel-mid/device_libs/platform_bt.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > index dc036e511f48..5a0483e7bf66 100644 > > > --- a/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > +++ b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > @@ -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.) > > 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. 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
On Thu, 28 Dec 2017, Ingo Molnar wrote: > > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Thu, 2017-12-28 at 11:28 +0100, Ingo Molnar wrote: > > > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > 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 <bhumirks@gmail.com> > > > > Cc: julia.lawall@lip6.fr > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > --- > > > > arch/x86/platform/intel-mid/device_libs/platform_bt.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > > b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > > index dc036e511f48..5a0483e7bf66 100644 > > > > --- a/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > > +++ b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > > @@ -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.) > > > > 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. > > 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
* Julia Lawall <julia.lawall@lip6.fr> wrote: > > > On Thu, 28 Dec 2017, Ingo Molnar wrote: > > > > > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Thu, 2017-12-28 at 11:28 +0100, Ingo Molnar wrote: > > > > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > > 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 <bhumirks@gmail.com> > > > > > Cc: julia.lawall@lip6.fr > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > --- > > > > > arch/x86/platform/intel-mid/device_libs/platform_bt.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > > > b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > > > index dc036e511f48..5a0483e7bf66 100644 > > > > > --- a/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > > > +++ b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > > > @@ -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.) > > > > > > 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. > > > > 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! > [...] 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
On Thu, 28 Dec 2017, Ingo Molnar wrote: > > * Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > > > > On Thu, 28 Dec 2017, Ingo Molnar wrote: > > > > > > > > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > On Thu, 2017-12-28 at 11:28 +0100, Ingo Molnar wrote: > > > > > * Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > > > > 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 <bhumirks@gmail.com> > > > > > > Cc: julia.lawall@lip6.fr > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > > > --- > > > > > > arch/x86/platform/intel-mid/device_libs/platform_bt.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > > > > b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > > > > index dc036e511f48..5a0483e7bf66 100644 > > > > > > --- a/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > > > > +++ b/arch/x86/platform/intel-mid/device_libs/platform_bt.c > > > > > > @@ -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.) > > > > > > > > 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. > > > > > > 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! > > > [...] 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
* Julia Lawall <julia.lawall@lip6.fr> wrote: > > > [...] 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). 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
On Thu, 28 Dec 2017, Ingo Molnar wrote: > > * Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > > [...] 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). > > 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
* Julia Lawall <julia.lawall@lip6.fr> wrote: > > > On Thu, 28 Dec 2017, Ingo Molnar wrote: > > > > > * Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > > > > [...] 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). > > > > 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. > [...] 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. > > 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
On Thu, 28 Dec 2017, Ingo Molnar wrote: > > * Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > > > > On Thu, 28 Dec 2017, Ingo Molnar wrote: > > > > > > > > * Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > > > > > > [...] 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). > > > > > > 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. 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. > It's the (pointer)->(pointer) and (pointer)->(long) conversions that are the most > dangerous ones. > > > [...] 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. OK this looks promising. I will try it. > > > > 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? Seems probable. julia
On Thu, 28 Dec 2017, Ingo Molnar wrote: > > * Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > > > > On Thu, 28 Dec 2017, Ingo Molnar wrote: > > > > > > > > * Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > > > > > > [...] 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). > > > > > > 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. > > > [...] 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. 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 > > > > > 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 >
On Thu, 28 Dec 2017, Ingo Molnar wrote: > > * Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > > > > On Thu, 28 Dec 2017, Ingo Molnar wrote: > > > > > > > > * Julia Lawall <julia.lawall@lip6.fr> wrote: > > > > > > > > > [...] 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). > > > > > > 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. > > > [...] 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 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 > > that way the fundamental types remains. > > > > > 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 >
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_bt.c b/arch/x86/platform/intel-mid/device_libs/platform_bt.c index dc036e511f48..5a0483e7bf66 100644 --- a/arch/x86/platform/intel-mid/device_libs/platform_bt.c +++ b/arch/x86/platform/intel-mid/device_libs/platform_bt.c @@ -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, };
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 <bhumirks@gmail.com> Cc: julia.lawall@lip6.fr Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- arch/x86/platform/intel-mid/device_libs/platform_bt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)