RE: [PATCH v2 05/18] x86, acpi: Split acpi_boot_table_init() into two parts.

["Zheng, Lv" <lv.zheng@intel.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 6342 bytes --]

> From: linux-acpi-owner@vger.kernel.org
> Sent: Friday, August 02, 2013 7:32 AM
> 
> On Thu, 2013-08-01 at 15:06 +0800, Tang Chen wrote:
> > In ACPI, SRAT(System Resource Affinity Table) contains NUMA info.
> > The memory affinities in SRAT record every memory range in the
> > system, and also, flags specifying if the memory range is
> > hotpluggable.
> > (Please refer to ACPI spec 5.0 5.2.16)
> >
> > memblock starts to work at very early time, and SRAT has not been
> > parsed. So we don't know which memory is hotpluggable. In order
> > to use memblock to reserve hotpluggable memory, we need to obtain
> > SRAT memory affinity info earlier.
> >
> > In the current acpi_boot_table_init(), it does the following:
> > 1. Parse RSDT, so that we can find all the tables.
> > 2. Initialize acpi_gbl_root_table_list, an array of acpi table
> >    descriptorsused to store each table's address, length, signature,

A missing space here.

> >    and so on.
> > 3. Check if there is any table in initrd intending to override
> >    tables from firmware. If so, override the firmware tables.
> > 4. Initialize all the data in acpi_gbl_root_table_list.
> >
> > In order to parse SRAT at early time, we need to do similar job as
> > step 1 and 2 above earlier to obtain SRAT. It will be very convenient
> > if we have acpi_gbl_root_table_list initialized. We can use address
> > and signature to find SRAT.
> >
> > Since step 1 and 2 allocates no memory, it is OK to do these two
> > steps earlier.
> >
> > But step 3 will check acpi initrd table override, not just SRAT,
> > but also all the other tables. So it is better to keep it untouched.
> >
> > This patch splits acpi_boot_table_init() into two steps:
> > 1. Parse RSDT, which cannot be overrided, and initialize
> >    acpi_gbl_root_table_list. (step 1 + 2 above)
> > 2. Install all ACPI tables into acpi_gbl_root_table_list.
> >    (step 3 + 4 above)
> >
> > In later patches, we will do step 1 + 2 earlier.
> >
> > Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> > Reviewed-by: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
> > ---
> >  drivers/acpi/acpica/tbutils.c |   25 ++++++++++++++++++++++---
> >  drivers/acpi/tables.c         |    2 ++
> >  include/acpi/acpixf.h         |    2 ++
> >  3 files changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
> 
> This patch needs to be reviewed by ACPICA folks.  I'd suggest to change
> the patch title to "x86, ACPICA:".  I added Bob to the list.
> 
> Thanks,
> -Toshi
> 
> 
> 
> > index ce3d5db..9d68ffc 100644
> > --- a/drivers/acpi/acpica/tbutils.c
> > +++ b/drivers/acpi/acpica/tbutils.c
> > @@ -766,9 +766,30 @@
> acpi_tb_parse_root_table(acpi_physical_address rsdp_address)
> >  	 */
> >  	acpi_os_unmap_memory(table, length);
> >
> > +	return_ACPI_STATUS(AE_OK);
> > +}
> > +
> >

I don't think you can split the function here.
ACPICA still need to continue to parse the table using the logic implemented in the acpi_tb_install_table() and acpi_tb_parse_fadt(). (for example, endianess of the signature).
You'd better to keep them as is and split some codes from 'acpi_tb_install_table' to form another function: acpi_tb_override_table().

> +/*********************************************************
> **********************
> > + *
> > + * FUNCTION:    acpi_tb_install_root_table

I think this function should be acpi_tb_override_tables, and call acpi_tb_override_table() inside this function for each table.

> > + *
> > + * DESCRIPTION: This function installs all the ACPI tables in RSDT into
> > + *              acpi_gbl_root_table_list.
> > + *
> > +
> **********************************************************
> ********************/
> > +
> > +void __init

Basically, ACPICA will use acpi_status as return value.

> > +acpi_tb_install_root_table()

(void)?

> > +{
> > +	int i;

Please use u32 instead of int.

> > +
> >  	/*
> >  	 * Complete the initialization of the root table array by examining
> > -	 * the header of each table
> > +	 * the header of each table.
> > +	 *
> > +	 * First two entries in the table array are reserved for the DSDT
> > +	 * and FACS, which are not actually present in the RSDT/XSDT - they
> > +	 * come from the FADT.
> >  	 */
> >  	for (i = 2; i < acpi_gbl_root_table_list.current_table_count; i++) {
> >  		acpi_tb_install_table(acpi_gbl_root_table_list.tables[i].
> > @@ -782,6 +803,4 @@
> acpi_tb_parse_root_table(acpi_physical_address rsdp_address)
> >  			acpi_tb_parse_fadt(i);
> >  		}
> >  	}
> > -
> > -	return_ACPI_STATUS(AE_OK);
> >  }
> > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> > index d67a1fe..8860e79 100644
> > --- a/drivers/acpi/tables.c
> > +++ b/drivers/acpi/tables.c
> > @@ -353,6 +353,8 @@ int __init acpi_table_init(void)
> >  	if (ACPI_FAILURE(status))
> >  		return 1;
> >
> > +	acpi_tb_install_root_table();
> > +

I think you shouldn't call a function named as acpi_tb_xxx directly in a file belongs to drivers/acpi not drivers/acpi/acpica.
This kind of a function name can only be used inside ACPICA.
You need an interface wrapper in drivers/acpi/acpica/tbxface.c to call such internal functions.

> >  	check_multiple_madt();
> >  	return 0;
> >  }
> > diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> > index 454881e..f5549b5 100644
> > --- a/include/acpi/acpixf.h
> > +++ b/include/acpi/acpixf.h
> > @@ -116,6 +116,8 @@ acpi_status
> >  acpi_initialize_tables(struct acpi_table_desc *initial_storage,
> >  		       u32 initial_table_count, u8 allow_resize);
> >
> > +void acpi_tb_install_root_table(void);
> > +

The reason is same as the above.
It doesn't make sense that a function with such name appears in the acpixf.h.

Thanks and best regards
-Lv

> >  acpi_status __init acpi_initialize_subsystem(void);
> >
> >  acpi_status acpi_enable_subsystem(u32 flags);
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
N‹§²æìr¸›zǧu©ž²Æ {\b­†éì¹»\x1c®&Þ–)îÆi¢žØ^n‡r¶‰šŽŠÝ¢j$½§$¢¸\x05¢¹¨­è§~Š'.)îÄÃ,yèm¶ŸÿÃ\f%Š{±šj+ƒðèž×¦j)Z†·Ÿ