[v2] drivers/irqchip: Remove function callback casts
diff mbox series

Message ID 20200524162220.10186-1-oscar.carter@gmx.com
State New
Headers show
Series
  • [v2] drivers/irqchip: Remove function callback casts
Related show

Commit Message

Oscar Carter May 24, 2020, 4:22 p.m. UTC
In an effort to enable -Wcast-function-type in the top-level Makefile to
support Control Flow Integrity builds, remove all the function callback
casts.

To do this, create a new macro called ACPI_DECLARE_SUBTABLE_PROBE_ENTRY
to initialize the acpi_probe_entry struct using the probe_subtbl field
instead of the probe_table field. Then, modify the IRQCHIP_ACPI_DECLARE
macro to use this new defined macro.

Even though these two commented fields are part of a union, this is
necessary to avoid function cast mismatches. That is, due to the
IRQCHIP_ACPI_DECLARE invocations use as last parameter a function with
the protoype "int (*func)(struct acpi_subtable_header *, const unsigned
long)" it's necessary that this macro initialize the probe_subtbl field
of the acpi_probe_entry struct and not the probe_table field.

Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
---
Changelog v1->v2
- Add more details in the commit changelog to clarify the changes (Marc
  Zyngier)
- Declare a new macro called ACPI_DECLARE_SUBTABLE_PROBE_ENTRY (Marc
  Zyngier)
- In the IRQCHIP_ACPI_DECLARE use the new defined macro (Marc Zyngier)

 include/linux/acpi.h    | 11 +++++++++++
 include/linux/irqchip.h |  5 +++--
 2 files changed, 14 insertions(+), 2 deletions(-)

--
2.20.1

Comments

Marc Zyngier May 25, 2020, 10:55 a.m. UTC | #1
On Sun, 24 May 2020 17:22:20 +0100,
Oscar Carter <oscar.carter@gmx.com> wrote:
> 
> In an effort to enable -Wcast-function-type in the top-level Makefile to
> support Control Flow Integrity builds, remove all the function callback
> casts.
> 
> To do this, create a new macro called ACPI_DECLARE_SUBTABLE_PROBE_ENTRY
> to initialize the acpi_probe_entry struct using the probe_subtbl field
> instead of the probe_table field. Then, modify the IRQCHIP_ACPI_DECLARE
> macro to use this new defined macro.
> 
> Even though these two commented fields are part of a union, this is
> necessary to avoid function cast mismatches. That is, due to the
> IRQCHIP_ACPI_DECLARE invocations use as last parameter a function with
> the protoype "int (*func)(struct acpi_subtable_header *, const unsigned
> long)" it's necessary that this macro initialize the probe_subtbl field
> of the acpi_probe_entry struct and not the probe_table field.
> 
> Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> ---
> Changelog v1->v2
> - Add more details in the commit changelog to clarify the changes (Marc
>   Zyngier)
> - Declare a new macro called ACPI_DECLARE_SUBTABLE_PROBE_ENTRY (Marc
>   Zyngier)
> - In the IRQCHIP_ACPI_DECLARE use the new defined macro (Marc Zyngier)
> 
>  include/linux/acpi.h    | 11 +++++++++++

You now need to Cc the ACPI maintainers.

>  include/linux/irqchip.h |  5 +++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index d661cd0ee64d..fed49b276a90 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1154,6 +1154,17 @@ struct acpi_probe_entry {
>  			.driver_data = data, 				\
>  		   }
> 
> +#define ACPI_DECLARE_SUBTABLE_PROBE_ENTRY(table, name, table_id,	\
> +					  subtable, valid, data, fn)	\
> +	static const struct acpi_probe_entry __acpi_probe_##name	\
> +		__used __section(__##table##_acpi_probe_table) = {	\
> +			.id = table_id,					\
> +			.type = subtable,				\
> +			.subtable_valid = valid,			\
> +			.probe_subtbl = (acpi_tbl_entry_handler)fn,	\

It strikes me that under the guise of removing function casts, you are
actually adding one! And this cast is actually hiding all sorts of
sins (remove it, and see things exploding). I've fixed it with the
patch below (ACPI is such a mess of data structure case...).

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index d7006ef18a0d..3870e9d4d3a8 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -2117,7 +2117,7 @@ static void __init gic_acpi_setup_kvm_info(void)
 }
 
 static int __init
-gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
+gic_acpi_init(union acpi_subtable_headers *header, const unsigned long end)
 {
 	struct acpi_madt_generic_distributor *dist;
 	struct fwnode_handle *domain_handle;
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 30ab623343d3..fc431857ce90 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1593,7 +1593,7 @@ static void __init gic_acpi_setup_kvm_info(void)
 	gic_set_kvm_info(&gic_v2_kvm_info);
 }
 
-static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
+static int __init gic_v2_acpi_init(union acpi_subtable_headers *header,
 				   const unsigned long end)
 {
 	struct acpi_madt_generic_distributor *dist;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index fed49b276a90..4f4ddbfce3d3 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1150,7 +1150,7 @@ struct acpi_probe_entry {
 			.id = table_id,					\
 			.type = subtable,				\
 			.subtable_valid = valid,			\
-			.probe_table = (acpi_tbl_table_handler)fn,	\
+			.probe_table = fn,				\
 			.driver_data = data, 				\
 		   }
 
@@ -1161,7 +1161,7 @@ struct acpi_probe_entry {
 			.id = table_id,					\
 			.type = subtable,				\
 			.subtable_valid = valid,			\
-			.probe_subtbl = (acpi_tbl_entry_handler)fn,	\
+			.probe_subtbl = fn,				\
 			.driver_data = data,				\
 		}
 
Thanks,

	M.
Oscar Carter May 26, 2020, 5:25 p.m. UTC | #2
On Mon, May 25, 2020 at 11:55:33AM +0100, Marc Zyngier wrote:
> On Sun, 24 May 2020 17:22:20 +0100,
> Oscar Carter <oscar.carter@gmx.com> wrote:
> >
> >  include/linux/acpi.h    | 11 +++++++++++
>
> You now need to Cc the ACPI maintainers.

Sorry for forgetting.

> >  include/linux/irqchip.h |  5 +++--
> >  2 files changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index d661cd0ee64d..fed49b276a90 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -1154,6 +1154,17 @@ struct acpi_probe_entry {
> >  			.driver_data = data, 				\
> >  		   }
> >
> > +#define ACPI_DECLARE_SUBTABLE_PROBE_ENTRY(table, name, table_id,	\
> > +					  subtable, valid, data, fn)	\
> > +	static const struct acpi_probe_entry __acpi_probe_##name	\
> > +		__used __section(__##table##_acpi_probe_table) = {	\
> > +			.id = table_id,					\
> > +			.type = subtable,				\
> > +			.subtable_valid = valid,			\
> > +			.probe_subtbl = (acpi_tbl_entry_handler)fn,	\
>
> It strikes me that under the guise of removing function casts, you are
> actually adding one! And this cast is actually hiding all sorts of
> sins (remove it, and see things exploding).

Yes, if I remove it I see things exploiding. I'm very sorry.

> I've fixed it with the patch below (ACPI is such a mess of data
> structure case...).

> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index d7006ef18a0d..3870e9d4d3a8 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -2117,7 +2117,7 @@ static void __init gic_acpi_setup_kvm_info(void)
>  }
>
>  static int __init
> -gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
> +gic_acpi_init(union acpi_subtable_headers *header, const unsigned long end)
>  {
>  	struct acpi_madt_generic_distributor *dist;
>  	struct fwnode_handle *domain_handle;
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 30ab623343d3..fc431857ce90 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -1593,7 +1593,7 @@ static void __init gic_acpi_setup_kvm_info(void)
>  	gic_set_kvm_info(&gic_v2_kvm_info);
>  }
>
> -static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
> +static int __init gic_v2_acpi_init(union acpi_subtable_headers *header,
>  				   const unsigned long end)
>  {
>  	struct acpi_madt_generic_distributor *dist;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index fed49b276a90..4f4ddbfce3d3 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1150,7 +1150,7 @@ struct acpi_probe_entry {
>  			.id = table_id,					\
>  			.type = subtable,				\
>  			.subtable_valid = valid,			\
> -			.probe_table = (acpi_tbl_table_handler)fn,	\
> +			.probe_table = fn,				\
>  			.driver_data = data, 				\
>  		   }
>
> @@ -1161,7 +1161,7 @@ struct acpi_probe_entry {
>  			.id = table_id,					\
>  			.type = subtable,				\
>  			.subtable_valid = valid,			\
> -			.probe_subtbl = (acpi_tbl_entry_handler)fn,	\
> +			.probe_subtbl = fn,				\
>  			.driver_data = data,				\
>  		}
>

Thanks for your help with this patch. I will do all the necessary changes
and I will resend a new version.

Do you mind if I add these two lines:

Co-developed-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Marc Zyngier <maz@kernel.org>

in the next version to give you credit?

> Thanks,
>
> 	M.
>
> --
> Without deviation from the norm, progress is not possible.

Thanks,
Oscar
Marc Zyngier May 26, 2020, 7:51 p.m. UTC | #3
On Tue, 26 May 2020 18:25:27 +0100,
Oscar Carter <oscar.carter@gmx.com> wrote:
> 
> On Mon, May 25, 2020 at 11:55:33AM +0100, Marc Zyngier wrote:
> > On Sun, 24 May 2020 17:22:20 +0100,
> > Oscar Carter <oscar.carter@gmx.com> wrote:
> > >
> > >  include/linux/acpi.h    | 11 +++++++++++
> >
> > You now need to Cc the ACPI maintainers.
> 
> Sorry for forgetting.

No worries. In general, use scripts/get_maintainers.pl to find out who
to Cc.

> 
> > >  include/linux/irqchip.h |  5 +++--
> > >  2 files changed, 14 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > > index d661cd0ee64d..fed49b276a90 100644
> > > --- a/include/linux/acpi.h
> > > +++ b/include/linux/acpi.h
> > > @@ -1154,6 +1154,17 @@ struct acpi_probe_entry {
> > >  			.driver_data = data, 				\
> > >  		   }
> > >
> > > +#define ACPI_DECLARE_SUBTABLE_PROBE_ENTRY(table, name, table_id,	\
> > > +					  subtable, valid, data, fn)	\
> > > +	static const struct acpi_probe_entry __acpi_probe_##name	\
> > > +		__used __section(__##table##_acpi_probe_table) = {	\
> > > +			.id = table_id,					\
> > > +			.type = subtable,				\
> > > +			.subtable_valid = valid,			\
> > > +			.probe_subtbl = (acpi_tbl_entry_handler)fn,	\
> >
> > It strikes me that under the guise of removing function casts, you are
> > actually adding one! And this cast is actually hiding all sorts of
> > sins (remove it, and see things exploding).
> 
> Yes, if I remove it I see things exploiding. I'm very sorry.

No need to be sorry. We all learn from past mistakes.

> 
> > I've fixed it with the patch below (ACPI is such a mess of data
> > structure case...).
> 
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index d7006ef18a0d..3870e9d4d3a8 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -2117,7 +2117,7 @@ static void __init gic_acpi_setup_kvm_info(void)
> >  }
> >
> >  static int __init
> > -gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
> > +gic_acpi_init(union acpi_subtable_headers *header, const unsigned long end)
> >  {
> >  	struct acpi_madt_generic_distributor *dist;
> >  	struct fwnode_handle *domain_handle;
> > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> > index 30ab623343d3..fc431857ce90 100644
> > --- a/drivers/irqchip/irq-gic.c
> > +++ b/drivers/irqchip/irq-gic.c
> > @@ -1593,7 +1593,7 @@ static void __init gic_acpi_setup_kvm_info(void)
> >  	gic_set_kvm_info(&gic_v2_kvm_info);
> >  }
> >
> > -static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
> > +static int __init gic_v2_acpi_init(union acpi_subtable_headers *header,
> >  				   const unsigned long end)
> >  {
> >  	struct acpi_madt_generic_distributor *dist;
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index fed49b276a90..4f4ddbfce3d3 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -1150,7 +1150,7 @@ struct acpi_probe_entry {
> >  			.id = table_id,					\
> >  			.type = subtable,				\
> >  			.subtable_valid = valid,			\
> > -			.probe_table = (acpi_tbl_table_handler)fn,	\
> > +			.probe_table = fn,				\
> >  			.driver_data = data, 				\
> >  		   }
> >
> > @@ -1161,7 +1161,7 @@ struct acpi_probe_entry {
> >  			.id = table_id,					\
> >  			.type = subtable,				\
> >  			.subtable_valid = valid,			\
> > -			.probe_subtbl = (acpi_tbl_entry_handler)fn,	\
> > +			.probe_subtbl = fn,				\
> >  			.driver_data = data,				\
> >  		}
> >
> 
> Thanks for your help with this patch. I will do all the necessary changes
> and I will resend a new version.
> 
> Do you mind if I add these two lines:
> 
> Co-developed-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> in the next version to give you credit?

Sure, that's very kind of you to offer.

Thanks,

	M.

Patch
diff mbox series

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d661cd0ee64d..fed49b276a90 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1154,6 +1154,17 @@  struct acpi_probe_entry {
 			.driver_data = data, 				\
 		   }

+#define ACPI_DECLARE_SUBTABLE_PROBE_ENTRY(table, name, table_id,	\
+					  subtable, valid, data, fn)	\
+	static const struct acpi_probe_entry __acpi_probe_##name	\
+		__used __section(__##table##_acpi_probe_table) = {	\
+			.id = table_id,					\
+			.type = subtable,				\
+			.subtable_valid = valid,			\
+			.probe_subtbl = (acpi_tbl_entry_handler)fn,	\
+			.driver_data = data,				\
+		}
+
 #define ACPI_PROBE_TABLE(name)		__##name##_acpi_probe_table
 #define ACPI_PROBE_TABLE_END(name)	__##name##_acpi_probe_table_end

diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h
index 950e4b2458f0..447f22880a69 100644
--- a/include/linux/irqchip.h
+++ b/include/linux/irqchip.h
@@ -39,8 +39,9 @@ 
  * @fn: initialization function
  */
 #define IRQCHIP_ACPI_DECLARE(name, subtable, validate, data, fn)	\
-	ACPI_DECLARE_PROBE_ENTRY(irqchip, name, ACPI_SIG_MADT, 		\
-				 subtable, validate, data, fn)
+	ACPI_DECLARE_SUBTABLE_PROBE_ENTRY(irqchip, name,		\
+					  ACPI_SIG_MADT, subtable,	\
+					  validate, data, fn)

 #ifdef CONFIG_IRQCHIP
 void irqchip_init(void);