| Message ID | 1484686210-7211-2-git-send-email-jeremy.linton@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jeremy, On 2017/1/18 4:50, Jeremy Linton wrote: > The MADT parser in smp.c is now being used to parse > out NUMA, PMU and ACPI parking protocol information as > well as the GIC information for which it was originally > created. Rename it to avoid a misleading name. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > arch/arm64/kernel/smp.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > index cb87234..8ea244c 100644 > --- a/arch/arm64/kernel/smp.c > +++ b/arch/arm64/kernel/smp.c > @@ -517,13 +517,14 @@ static unsigned int cpu_count = 1; > > #ifdef CONFIG_ACPI > /* > - * acpi_map_gic_cpu_interface - parse processor MADT entry > + * acpi_verify_and_map_madt - parse processor MADT entry > * > * Carry out sanity checks on MADT processor entry and initialize > - * cpu_logical_map on success > + * cpu_logical_map, the ACPI parking protocol, NUMA mapping > + * and the PMU interrupts on success > */ > static void __init > -acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) > +acpi_verify_and_map_madt(struct acpi_madt_generic_interrupt *processor) Nit, MADT is a table includes multi type of table entries, we just need to map the the processor type, how about updating it to acpi_verify_and_map_madt_processor()? Thanks Hanjun
Hi, Thanks for taking a look at this! On 01/18/2017 09:51 PM, Hanjun Guo wrote: > Hi Jeremy, > > On 2017/1/18 4:50, Jeremy Linton wrote: >> The MADT parser in smp.c is now being used to parse >> out NUMA, PMU and ACPI parking protocol information as >> well as the GIC information for which it was originally >> created. Rename it to avoid a misleading name. >> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> --- >> arch/arm64/kernel/smp.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c >> index cb87234..8ea244c 100644 >> --- a/arch/arm64/kernel/smp.c >> +++ b/arch/arm64/kernel/smp.c >> @@ -517,13 +517,14 @@ static unsigned int cpu_count = 1; >> >> #ifdef CONFIG_ACPI >> /* >> - * acpi_map_gic_cpu_interface - parse processor MADT entry >> + * acpi_verify_and_map_madt - parse processor MADT entry >> * >> * Carry out sanity checks on MADT processor entry and initialize >> - * cpu_logical_map on success >> + * cpu_logical_map, the ACPI parking protocol, NUMA mapping >> + * and the PMU interrupts on success >> */ >> static void __init >> -acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt >> *processor) >> +acpi_verify_and_map_madt(struct acpi_madt_generic_interrupt *processor) > > Nit, MADT is a table includes multi type of table entries, we just > need to map the the processor type, how about updating it to > acpi_verify_and_map_madt_processor()? The rename was originally proposed in a previous review comment because the thought was that the code is now parsing more than just the GIC->CPU information. That is even though the subtable type its parsing is described as the "GIC CPU interface" in the ACPI specification. So, in a way, I think the original gic_cpu_interface() name is more descriptive than acpi_verify_and_map_madt_processor(), but I'm pretty agnostic about what the name is. Particularly, since MADT itself is misleading. So, I don't see a need to respin this, simply to rename it, unless someone has a strong opinion one way or the other. Primary, because I would like to get this set merged and the right decision might just be to drop this patch. Thanks,
On Tue, Jan 17, 2017 at 02:50:04PM -0600, Jeremy Linton wrote: > The MADT parser in smp.c is now being used to parse > out NUMA, PMU and ACPI parking protocol information as Nit: It is not used to parse PMU yet at this stage so either you remove PMU from the list or you move this patch to the end of the series. > well as the GIC information for which it was originally > created. Rename it to avoid a misleading name. It is funny smp.c never mapped _any_ GIC information :) (even though it parses the MADT GIC CPU interface entries). > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > arch/arm64/kernel/smp.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > index cb87234..8ea244c 100644 > --- a/arch/arm64/kernel/smp.c > +++ b/arch/arm64/kernel/smp.c > @@ -517,13 +517,14 @@ static unsigned int cpu_count = 1; > > #ifdef CONFIG_ACPI > /* > - * acpi_map_gic_cpu_interface - parse processor MADT entry > + * acpi_verify_and_map_madt - parse processor MADT entry > * > * Carry out sanity checks on MADT processor entry and initialize > - * cpu_logical_map on success > + * cpu_logical_map, the ACPI parking protocol, NUMA mapping > + * and the PMU interrupts on success > */ > static void __init > -acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) > +acpi_verify_and_map_madt(struct acpi_madt_generic_interrupt *processor) acpi_map_madt_cpu_entry() or just drop this patch to avoid useless bikeshedding, your choice. > { > u64 hwid = processor->arm_mpidr; > > @@ -577,7 +578,7 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) > } > > static int __init > -acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, > +acpi_parse_madt_common(struct acpi_subtable_header *header, > const unsigned long end) acpi_madt_parse_cpus() Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > { > struct acpi_madt_generic_interrupt *processor; > @@ -588,7 +589,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, > > acpi_table_print_madt_entry(header); > > - acpi_map_gic_cpu_interface(processor); > + acpi_verify_and_map_madt(processor); > > return 0; > } > @@ -672,7 +673,7 @@ void __init smp_init_cpus(void) > * we need for SMP init > */ > acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, > - acpi_parse_gic_cpu_interface, 0); > + acpi_parse_madt_common, 0); > > if (cpu_count > nr_cpu_ids) > pr_warn("Number of cores (%d) exceeds configured maximum of %d - clipping\n", > -- > 2.5.5 >
Hi, On 01/20/2017 10:22 AM, Lorenzo Pieralisi wrote: > On Tue, Jan 17, 2017 at 02:50:04PM -0600, Jeremy Linton wrote: >> The MADT parser in smp.c is now being used to parse >> out NUMA, PMU and ACPI parking protocol information as > > Nit: It is not used to parse PMU yet at this stage so > either you remove PMU from the list or you move this patch > to the end of the series. (per comments below) I think the best plan is to drop it in the next spin of the series, although as Will has shown that he is skillful at cherry picking patches out of this series, so maybe that won't be necessary? Of course 3/7 probably won't apply cleanly without this one. > >> well as the GIC information for which it was originally >> created. Rename it to avoid a misleading name. > > It is funny smp.c never mapped _any_ GIC information :) (even > though it parses the MADT GIC CPU interface entries). <OT chuckle> Ha, map is pretty generic outside of LK and more accurately reflects the "mapping" from one structure/element to another, more so than parse (of which I'm a prime offender, particularly in the previous comment). </OT chuckle> (trimming) >> static void __init >> -acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) >> +acpi_verify_and_map_madt(struct acpi_madt_generic_interrupt *processor) > > acpi_map_madt_cpu_entry() > > or just drop this patch to avoid useless bikeshedding, your choice. > >> { >> u64 hwid = processor->arm_mpidr; >> >> @@ -577,7 +578,7 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) >> } >> >> static int __init >> -acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, >> +acpi_parse_madt_common(struct acpi_subtable_header *header, >> const unsigned long end) > > acpi_madt_parse_cpus() > > Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > >> { >> struct acpi_madt_generic_interrupt *processor; (trimming)
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index cb87234..8ea244c 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -517,13 +517,14 @@ static unsigned int cpu_count = 1; #ifdef CONFIG_ACPI /* - * acpi_map_gic_cpu_interface - parse processor MADT entry + * acpi_verify_and_map_madt - parse processor MADT entry * * Carry out sanity checks on MADT processor entry and initialize - * cpu_logical_map on success + * cpu_logical_map, the ACPI parking protocol, NUMA mapping + * and the PMU interrupts on success */ static void __init -acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) +acpi_verify_and_map_madt(struct acpi_madt_generic_interrupt *processor) { u64 hwid = processor->arm_mpidr; @@ -577,7 +578,7 @@ acpi_map_gic_cpu_interface(struct acpi_madt_generic_interrupt *processor) } static int __init -acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, +acpi_parse_madt_common(struct acpi_subtable_header *header, const unsigned long end) { struct acpi_madt_generic_interrupt *processor; @@ -588,7 +589,7 @@ acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header, acpi_table_print_madt_entry(header); - acpi_map_gic_cpu_interface(processor); + acpi_verify_and_map_madt(processor); return 0; } @@ -672,7 +673,7 @@ void __init smp_init_cpus(void) * we need for SMP init */ acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, - acpi_parse_gic_cpu_interface, 0); + acpi_parse_madt_common, 0); if (cpu_count > nr_cpu_ids) pr_warn("Number of cores (%d) exceeds configured maximum of %d - clipping\n",
The MADT parser in smp.c is now being used to parse out NUMA, PMU and ACPI parking protocol information as well as the GIC information for which it was originally created. Rename it to avoid a misleading name. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- arch/arm64/kernel/smp.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)