| Message ID | 20200524162220.10186-1-oscar.carter@gmx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drivers/irqchip: Remove function callback casts | expand |
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.
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
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.
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);
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