[PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

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

Re: [PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[Ingo Molnar]

* 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

Re: [PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[Julia Lawall]

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

Re: [PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[Ingo Molnar]

* 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

Re: [PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[Julia Lawall]

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
>

Re: [PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[Ingo Molnar]

* 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

Re: [PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[Andy Shevchenko]

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>
Intel Finland Oy

Re: [PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[Ingo Molnar]

* 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

Re: [PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[Julia Lawall]

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

Re: [PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[Ingo Molnar]

* 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

Re: [PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[Julia Lawall]

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

Re: [PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[Ingo Molnar]

* 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

Re: [PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[Julia Lawall]

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

Re: [PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[Ingo Molnar]

* 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

Re: [PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[Julia Lawall]

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

Re: [PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[Julia Lawall]

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
>

Re: [PATCH v1] x86/platform/intel-mid: Revert "Make 'bt_sfi_data' const"

[Julia Lawall]

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
>