| Message ID | 1388320370-28732-1-git-send-email-marcel.a@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Dec 29, 2013 at 02:32:50PM +0200, Marcel Apfelbaum wrote: > It seems that iasl has an issue when disassembles > some ACPI tables using the command line: > iasl -e DSDT -e SSDT -d HPET > I opened a bug on iasl project: > https://github.com/acpica/acpica/issues/20 > > Modified the iasl command line to "iasl -d HPET" > until the problem is solved. The command line > remained the same for DSDT and SSDT tables. > > Reported-by:Michael S. Tsirkin <mst@redhat.com> space needed after : > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > --- > tests/acpi-test.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/tests/acpi-test.c b/tests/acpi-test.c > index 2d32b69..26d2e26 100644 > --- a/tests/acpi-test.c > +++ b/tests/acpi-test.c > @@ -403,7 +403,7 @@ static void dump_aml_files(test_data *data, bool rebuild) > } > } > > -static void load_asl(GArray *sdts, AcpiSdtTable *sdt) > +static void load_asl(GArray *sdts, AcpiSdtTable *sdt, bool referenceDsdt) > { > AcpiSdtTable *temp; > GError *error = NULL; > @@ -419,9 +419,11 @@ static void load_asl(GArray *sdts, AcpiSdtTable *sdt) > > /* build command line */ > g_string_append_printf(command_line, " -p %s ", sdt->asl_file); > - for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ > - temp = &g_array_index(sdts, AcpiSdtTable, i); > - g_string_append_printf(command_line, "-e %s ", temp->aml_file); > + if (referenceDsdt) { > + for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ > + temp = &g_array_index(sdts, AcpiSdtTable, i); > + g_string_append_printf(command_line, "-e %s ", temp->aml_file); > + } > } > g_string_append_printf(command_line, "-d %s", sdt->aml_file); > > @@ -510,14 +512,14 @@ static void test_acpi_asl(test_data *data) > dump_aml_files(data, false); > for (i = 0; i < data->ssdt_tables->len; ++i) { > GString *asl, *exp_asl; > - > + bool referenceDsdt = (i < 2); /* only for DSDT and SSDT */ Please don't use camelCase for variables. Can we check the table signature instead of looking at the index? > sdt = &g_array_index(data->ssdt_tables, AcpiSdtTable, i); > exp_sdt = &g_array_index(exp_data.ssdt_tables, AcpiSdtTable, i); > > - load_asl(data->ssdt_tables, sdt); > + load_asl(data->ssdt_tables, sdt, referenceDsdt); > asl = normalize_asl(sdt->asl); > > - load_asl(exp_data.ssdt_tables, exp_sdt); > + load_asl(exp_data.ssdt_tables, exp_sdt, referenceDsdt); > exp_asl = normalize_asl(exp_sdt->asl); > > g_assert(!g_strcmp0(asl->str, exp_asl->str)); > -- > 1.8.3.1
On Mon, 2014-01-06 at 13:43 +0200, Michael S. Tsirkin wrote: > On Sun, Dec 29, 2013 at 02:32:50PM +0200, Marcel Apfelbaum wrote: > > It seems that iasl has an issue when disassembles > > some ACPI tables using the command line: > > iasl -e DSDT -e SSDT -d HPET > > I opened a bug on iasl project: > > https://github.com/acpica/acpica/issues/20 > > > > Modified the iasl command line to "iasl -d HPET" > > until the problem is solved. The command line > > remained the same for DSDT and SSDT tables. > > > > Reported-by:Michael S. Tsirkin <mst@redhat.com> > > space needed after : Sure, missed that > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > --- > > tests/acpi-test.c | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/tests/acpi-test.c b/tests/acpi-test.c > > index 2d32b69..26d2e26 100644 > > --- a/tests/acpi-test.c > > +++ b/tests/acpi-test.c > > @@ -403,7 +403,7 @@ static void dump_aml_files(test_data *data, bool rebuild) > > } > > } > > > > -static void load_asl(GArray *sdts, AcpiSdtTable *sdt) > > +static void load_asl(GArray *sdts, AcpiSdtTable *sdt, bool referenceDsdt) > > { > > AcpiSdtTable *temp; > > GError *error = NULL; > > @@ -419,9 +419,11 @@ static void load_asl(GArray *sdts, AcpiSdtTable *sdt) > > > > /* build command line */ > > g_string_append_printf(command_line, " -p %s ", sdt->asl_file); > > - for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ > > - temp = &g_array_index(sdts, AcpiSdtTable, i); > > - g_string_append_printf(command_line, "-e %s ", temp->aml_file); > > + if (referenceDsdt) { > > + for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ > > + temp = &g_array_index(sdts, AcpiSdtTable, i); > > + g_string_append_printf(command_line, "-e %s ", temp->aml_file); > > + } > > } > > g_string_append_printf(command_line, "-d %s", sdt->aml_file); > > > > @@ -510,14 +512,14 @@ static void test_acpi_asl(test_data *data) > > dump_aml_files(data, false); > > for (i = 0; i < data->ssdt_tables->len; ++i) { > > GString *asl, *exp_asl; > > - > > + bool referenceDsdt = (i < 2); /* only for DSDT and SSDT */ > > > Please don't use camelCase for variables. Sure, no idea why I did that in the first place... > Can we check the table signature instead of looking at the index? It is possible, but not really necessary because: 1. The array of expected tables it is created as a shadow of the original tables. (We load the expected tables in the exact order as the originals) 2. The loading is done based on table signatures. So going over the index in *this* case it means going over the signatures. Thanks for the review, Marcel > > > sdt = &g_array_index(data->ssdt_tables, AcpiSdtTable, i); > > exp_sdt = &g_array_index(exp_data.ssdt_tables, AcpiSdtTable, i); > > > > - load_asl(data->ssdt_tables, sdt); > > + load_asl(data->ssdt_tables, sdt, referenceDsdt); > > asl = normalize_asl(sdt->asl); > > > > - load_asl(exp_data.ssdt_tables, exp_sdt); > > + load_asl(exp_data.ssdt_tables, exp_sdt, referenceDsdt); > > exp_asl = normalize_asl(exp_sdt->asl); > > > > g_assert(!g_strcmp0(asl->str, exp_asl->str)); > > -- > > 1.8.3.1
On Mon, Jan 06, 2014 at 03:21:52PM +0200, Marcel Apfelbaum wrote: > On Mon, 2014-01-06 at 13:43 +0200, Michael S. Tsirkin wrote: > > On Sun, Dec 29, 2013 at 02:32:50PM +0200, Marcel Apfelbaum wrote: > > > It seems that iasl has an issue when disassembles > > > some ACPI tables using the command line: > > > iasl -e DSDT -e SSDT -d HPET > > > I opened a bug on iasl project: > > > https://github.com/acpica/acpica/issues/20 > > > > > > Modified the iasl command line to "iasl -d HPET" > > > until the problem is solved. The command line > > > remained the same for DSDT and SSDT tables. > > > > > > Reported-by:Michael S. Tsirkin <mst@redhat.com> > > > > space needed after : > Sure, missed that > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > --- > > > tests/acpi-test.c | 16 +++++++++------- > > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > > > diff --git a/tests/acpi-test.c b/tests/acpi-test.c > > > index 2d32b69..26d2e26 100644 > > > --- a/tests/acpi-test.c > > > +++ b/tests/acpi-test.c > > > @@ -403,7 +403,7 @@ static void dump_aml_files(test_data *data, bool rebuild) > > > } > > > } > > > > > > -static void load_asl(GArray *sdts, AcpiSdtTable *sdt) > > > +static void load_asl(GArray *sdts, AcpiSdtTable *sdt, bool referenceDsdt) > > > { > > > AcpiSdtTable *temp; > > > GError *error = NULL; > > > @@ -419,9 +419,11 @@ static void load_asl(GArray *sdts, AcpiSdtTable *sdt) > > > > > > /* build command line */ > > > g_string_append_printf(command_line, " -p %s ", sdt->asl_file); > > > - for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ > > > - temp = &g_array_index(sdts, AcpiSdtTable, i); > > > - g_string_append_printf(command_line, "-e %s ", temp->aml_file); > > > + if (referenceDsdt) { > > > + for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ > > > + temp = &g_array_index(sdts, AcpiSdtTable, i); > > > + g_string_append_printf(command_line, "-e %s ", temp->aml_file); > > > + } > > > } > > > g_string_append_printf(command_line, "-d %s", sdt->aml_file); > > > > > > @@ -510,14 +512,14 @@ static void test_acpi_asl(test_data *data) > > > dump_aml_files(data, false); > > > for (i = 0; i < data->ssdt_tables->len; ++i) { > > > GString *asl, *exp_asl; > > > - > > > + bool referenceDsdt = (i < 2); /* only for DSDT and SSDT */ > > > > > > Please don't use camelCase for variables. > Sure, no idea why I did that in the first place... > > > Can we check the table signature instead of looking at the index? > It is possible, but not really necessary because: > 1. The array of expected tables it is created as a shadow of the > original tables. (We load the expected tables in the exact order as the originals) > 2. The loading is done based on table signatures. > So going over the index in *this* case it means going over the signatures. Yes but it's really not a good idea to assume that there's a single SSDT, the fact you need a comment shows that it's fragile. How about a simple API like compare_signature(GArray *, const char *signature) We should also probably rename ssdt_tables to something else if it holds other tables besides ssdt. all_tables? > Thanks for the review, > Marcel > > > > > > sdt = &g_array_index(data->ssdt_tables, AcpiSdtTable, i); > > > exp_sdt = &g_array_index(exp_data.ssdt_tables, AcpiSdtTable, i); > > > > > > - load_asl(data->ssdt_tables, sdt); > > > + load_asl(data->ssdt_tables, sdt, referenceDsdt); > > > asl = normalize_asl(sdt->asl); > > > > > > - load_asl(exp_data.ssdt_tables, exp_sdt); > > > + load_asl(exp_data.ssdt_tables, exp_sdt, referenceDsdt); > > > exp_asl = normalize_asl(exp_sdt->asl); > > > > > > g_assert(!g_strcmp0(asl->str, exp_asl->str)); > > > -- > > > 1.8.3.1 > >
On Mon, 2014-01-06 at 16:35 +0200, Michael S. Tsirkin wrote: > On Mon, Jan 06, 2014 at 03:21:52PM +0200, Marcel Apfelbaum wrote: > > On Mon, 2014-01-06 at 13:43 +0200, Michael S. Tsirkin wrote: > > > On Sun, Dec 29, 2013 at 02:32:50PM +0200, Marcel Apfelbaum wrote: > > > > It seems that iasl has an issue when disassembles > > > > some ACPI tables using the command line: > > > > iasl -e DSDT -e SSDT -d HPET > > > > I opened a bug on iasl project: > > > > https://github.com/acpica/acpica/issues/20 > > > > > > > > Modified the iasl command line to "iasl -d HPET" > > > > until the problem is solved. The command line > > > > remained the same for DSDT and SSDT tables. > > > > > > > > Reported-by:Michael S. Tsirkin <mst@redhat.com> > > > > > > space needed after : > > Sure, missed that > > > > > > > Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> > > > > --- > > > > tests/acpi-test.c | 16 +++++++++------- > > > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/tests/acpi-test.c b/tests/acpi-test.c > > > > index 2d32b69..26d2e26 100644 > > > > --- a/tests/acpi-test.c > > > > +++ b/tests/acpi-test.c > > > > @@ -403,7 +403,7 @@ static void dump_aml_files(test_data *data, bool rebuild) > > > > } > > > > } > > > > > > > > -static void load_asl(GArray *sdts, AcpiSdtTable *sdt) > > > > +static void load_asl(GArray *sdts, AcpiSdtTable *sdt, bool referenceDsdt) > > > > { > > > > AcpiSdtTable *temp; > > > > GError *error = NULL; > > > > @@ -419,9 +419,11 @@ static void load_asl(GArray *sdts, AcpiSdtTable *sdt) > > > > > > > > /* build command line */ > > > > g_string_append_printf(command_line, " -p %s ", sdt->asl_file); > > > > - for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ > > > > - temp = &g_array_index(sdts, AcpiSdtTable, i); > > > > - g_string_append_printf(command_line, "-e %s ", temp->aml_file); > > > > + if (referenceDsdt) { > > > > + for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ > > > > + temp = &g_array_index(sdts, AcpiSdtTable, i); > > > > + g_string_append_printf(command_line, "-e %s ", temp->aml_file); > > > > + } > > > > } > > > > g_string_append_printf(command_line, "-d %s", sdt->aml_file); > > > > > > > > @@ -510,14 +512,14 @@ static void test_acpi_asl(test_data *data) > > > > dump_aml_files(data, false); > > > > for (i = 0; i < data->ssdt_tables->len; ++i) { > > > > GString *asl, *exp_asl; > > > > - > > > > + bool referenceDsdt = (i < 2); /* only for DSDT and SSDT */ > > > > > > > > > Please don't use camelCase for variables. > > Sure, no idea why I did that in the first place... > > > > > Can we check the table signature instead of looking at the index? > > It is possible, but not really necessary because: > > 1. The array of expected tables it is created as a shadow of the > > original tables. (We load the expected tables in the exact order as the originals) > > 2. The loading is done based on table signatures. > > So going over the index in *this* case it means going over the signatures. > > Yes but it's really not a good idea to assume that there's > a single SSDT, the fact you need a comment shows that > it's fragile. > How about a simple API like > compare_signature(GArray *, const char *signature) > > > We should also probably rename ssdt_tables to something else > if it holds other tables besides ssdt. all_tables? OK, I'll follow this path Thanks, Marcel > > > Thanks for the review, > > Marcel > > > > > > > > > sdt = &g_array_index(data->ssdt_tables, AcpiSdtTable, i); > > > > exp_sdt = &g_array_index(exp_data.ssdt_tables, AcpiSdtTable, i); > > > > > > > > - load_asl(data->ssdt_tables, sdt); > > > > + load_asl(data->ssdt_tables, sdt, referenceDsdt); > > > > asl = normalize_asl(sdt->asl); > > > > > > > > - load_asl(exp_data.ssdt_tables, exp_sdt); > > > > + load_asl(exp_data.ssdt_tables, exp_sdt, referenceDsdt); > > > > exp_asl = normalize_asl(exp_sdt->asl); > > > > > > > > g_assert(!g_strcmp0(asl->str, exp_asl->str)); > > > > -- > > > > 1.8.3.1 > > > >
diff --git a/tests/acpi-test.c b/tests/acpi-test.c index 2d32b69..26d2e26 100644 --- a/tests/acpi-test.c +++ b/tests/acpi-test.c @@ -403,7 +403,7 @@ static void dump_aml_files(test_data *data, bool rebuild) } } -static void load_asl(GArray *sdts, AcpiSdtTable *sdt) +static void load_asl(GArray *sdts, AcpiSdtTable *sdt, bool referenceDsdt) { AcpiSdtTable *temp; GError *error = NULL; @@ -419,9 +419,11 @@ static void load_asl(GArray *sdts, AcpiSdtTable *sdt) /* build command line */ g_string_append_printf(command_line, " -p %s ", sdt->asl_file); - for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ - temp = &g_array_index(sdts, AcpiSdtTable, i); - g_string_append_printf(command_line, "-e %s ", temp->aml_file); + if (referenceDsdt) { + for (i = 0; i < 2; ++i) { /* reference DSDT and SSDT */ + temp = &g_array_index(sdts, AcpiSdtTable, i); + g_string_append_printf(command_line, "-e %s ", temp->aml_file); + } } g_string_append_printf(command_line, "-d %s", sdt->aml_file); @@ -510,14 +512,14 @@ static void test_acpi_asl(test_data *data) dump_aml_files(data, false); for (i = 0; i < data->ssdt_tables->len; ++i) { GString *asl, *exp_asl; - + bool referenceDsdt = (i < 2); /* only for DSDT and SSDT */ sdt = &g_array_index(data->ssdt_tables, AcpiSdtTable, i); exp_sdt = &g_array_index(exp_data.ssdt_tables, AcpiSdtTable, i); - load_asl(data->ssdt_tables, sdt); + load_asl(data->ssdt_tables, sdt, referenceDsdt); asl = normalize_asl(sdt->asl); - load_asl(exp_data.ssdt_tables, exp_sdt); + load_asl(exp_data.ssdt_tables, exp_sdt, referenceDsdt); exp_asl = normalize_asl(exp_sdt->asl); g_assert(!g_strcmp0(asl->str, exp_asl->str));
It seems that iasl has an issue when disassembles some ACPI tables using the command line: iasl -e DSDT -e SSDT -d HPET I opened a bug on iasl project: https://github.com/acpica/acpica/issues/20 Modified the iasl command line to "iasl -d HPET" until the problem is solved. The command line remained the same for DSDT and SSDT tables. Reported-by:Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com> --- tests/acpi-test.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)