[RFC] [Patch 1/2] ACPI :Compare EC device in DSDT table with that in ECDT table

[RFC] [Patch 1/2] ACPI :Compare EC device in DSDT table with that in ECDT table

[Zhao Yakui]

Subject: ACPI: Compare EC device in DSDT table with that in ECDT table
From: Zhao Yakui <yakui.zhao@intel.com>

On some broken laptops the EC device defined in ECDT table is incorrect. For
example:
        The incorrect Command/Status I/O Port
        The incorrect Data I/O port
        Sometimes the GPE number is incorrect.
        Sometimes the EC namepath is also incorrect.
In such case the EC device can't be initialized correctly. Maybe it is
appropriate to compare the EC device in DSDT table with that in ECDT table.
If they mismatch, the BIOS bug will be reported and the EC device parsed
in DSDT table will be initialized instead of that defined in ECDT table.

http://bugzilla.kernel.org/show_bug.cgi?id=9399
http://bugzilla.kernel.org/show_bug.cgi?id=11880

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
cc: Alexey Starikovskiy <astarikovskiy@suse.de>
---
 drivers/acpi/ec.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/acpi/ec.c
===================================================================
--- linux-2.6.orig/drivers/acpi/ec.c
+++ linux-2.6/drivers/acpi/ec.c
@@ -990,6 +990,7 @@ int __init acpi_ec_ecdt_probe(void)
 	status = acpi_get_table(ACPI_SIG_ECDT, 1,
 				(struct acpi_table_header **)&ecdt_ptr);
 	if (ACPI_SUCCESS(status)) {
+		struct acpi_ec *ec_dsdt;
 		pr_info(PREFIX "EC description table is found, configuring boot EC\n");
 		boot_ec->command_addr = ecdt_ptr->control.address;
 		boot_ec->data_addr = ecdt_ptr->data.address;
@@ -1004,7 +1005,63 @@ int __init acpi_ec_ecdt_probe(void)
 		}
 		boot_ec->gpe = ecdt_ptr->gpe;
 		boot_ec->handle = ACPI_ROOT_OBJECT;
-		acpi_get_handle(ACPI_ROOT_OBJECT, ecdt_ptr->id, &boot_ec->handle);
+		status = acpi_get_handle(ACPI_ROOT_OBJECT, ecdt_ptr->id,
+					&boot_ec->handle);
+		if (ACPI_FAILURE(status)) {
+			/*
+			 * If the failure is returned by the function of
+			 * acpi_get_handle, it indicates that the EC namepath
+			 * is given by ECDT table. Print the BIOS bug.
+			 */
+			printk(KERN_INFO PREFIX "BIOS bug. The incorrect "
+				"namepath is given in ECDT table\n");
+		}
+		/*
+		 * Don't trust the namepath given by ECDT table. So we should
+		 * walk ACPI namespace tree to get the EC device in DSDT table.
+		 */
+		ec_dsdt = NULL;
+		ec_dsdt = kzalloc(sizeof(struct acpi_ec), GFP_KERNEL);
+		status = acpi_get_devices(ec_device_ids[0].id,
+				ec_parse_device, ec_dsdt, NULL);
+		/* check that acpi_get_devices actually find something
+		 */
+		if (ACPI_FAILURE(status) || !ec_dsdt->handle) {
+			printk(KERN_INFO PREFIX "BIOS bug: Incorrect "
+				"EC device in DSDT table\n");
+			kfree(ec_dsdt);
+			ec_dsdt = NULL;
+		}
+		/*
+		 * When EC device is parsed successfully in DSDT table, compare
+		 * it with that in ECDT table. If they don't mismatch, the EC
+		 * device is initialized instead of that in ECDT table
+		 */
+		if (ACPI_SUCCESS(status) && ec_dsdt) {
+			/* Compare the Cmd/Status I/O port */
+			if (ec_dsdt->command_addr !=
+				boot_ec->command_addr) {
+				printk(KERN_INFO PREFIX "BIOS bug: Incorrect "
+				"EC cmd/status I/O port in ECDT table\n");
+				boot_ec->command_addr = ec_dsdt->
+							command_addr;
+			}
+			/* Compare the Data I/O port */
+			if (ec_dsdt->data_addr != boot_ec->data_addr) {
+				printk(KERN_INFO PREFIX "BIOS bug: "
+				"Incorrect EC Data I/O port in ECDT table\n");
+				boot_ec->data_addr = ec_dsdt->data_addr;
+			}
+			/* Compare the EC GPE number */
+			if (ec_dsdt->gpe != boot_ec->gpe) {
+				printk(KERN_INFO PREFIX "BIOS bug: "
+				"Incorrect EC GPE number in ECDT table\n");
+				boot_ec->gpe = ec_dsdt->gpe;
+			}
+			boot_ec->global_lock = ec_dsdt->global_lock;
+			kfree(ec_dsdt);
+			ec_dsdt = NULL;
+		}
 	} else {
 		/* This workaround is needed only on some broken machines,
 		 * which require early EC, but fail to provide ECDT */

[RFC] [Patch 1/2] ACPI :Compare EC device in DSDT table with that in ECDT table

[Zhao Yakui]

Subject: ACPI: Compare EC device in DSDT table with that in ECDT table
From: Zhao Yakui <yakui.zhao@intel.com>

On some broken laptops the EC device defined in ECDT table is incorrect. For
example:
        The incorrect Command/Status I/O Port
        The incorrect Data I/O port
        Sometimes the GPE number is incorrect.
        Sometimes the EC namepath is also incorrect.
In such case the EC device can't be initialized correctly. Maybe it is
appropriate to compare the EC device in DSDT table with that in ECDT table.
If they mismatch, the BIOS bug will be reported and the EC device parsed
in DSDT table will be initialized instead of that defined in ECDT table.

http://bugzilla.kernel.org/show_bug.cgi?id=9399
http://bugzilla.kernel.org/show_bug.cgi?id=11880

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
cc: Alexey Starikovskiy <astarikovskiy@suse.de>
---
 drivers/acpi/ec.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/acpi/ec.c
===================================================================
--- linux-2.6.orig/drivers/acpi/ec.c
+++ linux-2.6/drivers/acpi/ec.c
@@ -990,6 +990,7 @@ int __init acpi_ec_ecdt_probe(void)
 	status = acpi_get_table(ACPI_SIG_ECDT, 1,
 				(struct acpi_table_header **)&ecdt_ptr);
 	if (ACPI_SUCCESS(status)) {
+		struct acpi_ec *ec_dsdt;
 		pr_info(PREFIX "EC description table is found, configuring boot EC\n");
 		boot_ec->command_addr = ecdt_ptr->control.address;
 		boot_ec->data_addr = ecdt_ptr->data.address;
@@ -1004,7 +1005,63 @@ int __init acpi_ec_ecdt_probe(void)
 		}
 		boot_ec->gpe = ecdt_ptr->gpe;
 		boot_ec->handle = ACPI_ROOT_OBJECT;
-		acpi_get_handle(ACPI_ROOT_OBJECT, ecdt_ptr->id, &boot_ec->handle);
+		status = acpi_get_handle(ACPI_ROOT_OBJECT, ecdt_ptr->id,
+					&boot_ec->handle);
+		if (ACPI_FAILURE(status)) {
+			/*
+			 * If the failure is returned by the function of
+			 * acpi_get_handle, it indicates that the EC namepath
+			 * is given by ECDT table. Print the BIOS bug.
+			 */
+			printk(KERN_INFO PREFIX "BIOS bug. The incorrect "
+				"namepath is given in ECDT table\n");
+		}
+		/*
+		 * Don't trust the namepath given by ECDT table. So we should
+		 * walk ACPI namespace tree to get the EC device in DSDT table.
+		 */
+		ec_dsdt = NULL;
+		ec_dsdt = kzalloc(sizeof(struct acpi_ec), GFP_KERNEL);
+		status = acpi_get_devices(ec_device_ids[0].id,
+				ec_parse_device, ec_dsdt, NULL);
+		/* check that acpi_get_devices actually find something
+		 */
+		if (ACPI_FAILURE(status) || !ec_dsdt->handle) {
+			printk(KERN_INFO PREFIX "BIOS bug: Incorrect "
+				"EC device in DSDT table\n");
+			kfree(ec_dsdt);
+			ec_dsdt = NULL;
+		}
+		/*
+		 * When EC device is parsed successfully in DSDT table, compare
+		 * it with that in ECDT table. If they don't mismatch, the EC
+		 * device is initialized instead of that in ECDT table
+		 */
+		if (ACPI_SUCCESS(status) && ec_dsdt) {
+			/* Compare the Cmd/Status I/O port */
+			if (ec_dsdt->command_addr !=
+				boot_ec->command_addr) {
+				printk(KERN_INFO PREFIX "BIOS bug: Incorrect "
+				"EC cmd/status I/O port in ECDT table\n");
+				boot_ec->command_addr = ec_dsdt->
+							command_addr;
+			}
+			/* Compare the Data I/O port */
+			if (ec_dsdt->data_addr != boot_ec->data_addr) {
+				printk(KERN_INFO PREFIX "BIOS bug: "
+				"Incorrect EC Data I/O port in ECDT table\n");
+				boot_ec->data_addr = ec_dsdt->data_addr;
+			}
+			/* Compare the EC GPE number */
+			if (ec_dsdt->gpe != boot_ec->gpe) {
+				printk(KERN_INFO PREFIX "BIOS bug: "
+				"Incorrect EC GPE number in ECDT table\n");
+				boot_ec->gpe = ec_dsdt->gpe;
+			}
+			boot_ec->global_lock = ec_dsdt->global_lock;
+			kfree(ec_dsdt);
+			ec_dsdt = NULL;
+		}
 	} else {
 		/* This workaround is needed only on some broken machines,
 		 * which require early EC, but fail to provide ECDT */

Re: [RFC] [Patch 1/2] ACPI :Compare EC device in DSDT table with that in ECDT table

[Alexey Starikovskiy]

Hi Yakui,

Couple of concerns -- I think, I already mentioned one, but still...
1. By parsing DSDT unconditionally, we loose all performance (if any) benefits of
having ECDT table. If we just don't look into ECDT, we will save some efforts.
2. We are speaking about broken (hopefully rare) BIOS -- it might not be wise to punish everyone (see #1).
How about adding check in acpi_ec_start(), that with the same GPE we found difference in addresses?
And if this check triggers, print "Your ECDT does not conform to DSDT. Add acpi_ec.skip_ecdt=1 kernel option."
Add acpi_ec.skip_ecdt option and skip ECDT parse if it is set...

3. This patch seems to imply that DSDT contains single EC. As this is first place with such assumption,
please re-think it.

Regards,
Alex.

Zhao Yakui wrote:
> Subject: ACPI: Compare EC device in DSDT table with that in ECDT table
> From: Zhao Yakui <yakui.zhao@intel.com>
> 
> On some broken laptops the EC device defined in ECDT table is incorrect. For
> example:
>         The incorrect Command/Status I/O Port
>         The incorrect Data I/O port
>         Sometimes the GPE number is incorrect.
>         Sometimes the EC namepath is also incorrect.
> In such case the EC device can't be initialized correctly. Maybe it is
> appropriate to compare the EC device in DSDT table with that in ECDT table.
> If they mismatch, the BIOS bug will be reported and the EC device parsed
> in DSDT table will be initialized instead of that defined in ECDT table.
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=9399
> http://bugzilla.kernel.org/show_bug.cgi?id=11880
> 
> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> cc: Alexey Starikovskiy <astarikovskiy@suse.de>
> ---
>  drivers/acpi/ec.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 58 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/acpi/ec.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/ec.c
> +++ linux-2.6/drivers/acpi/ec.c
> @@ -990,6 +990,7 @@ int __init acpi_ec_ecdt_probe(void)
>  	status = acpi_get_table(ACPI_SIG_ECDT, 1,
>  				(struct acpi_table_header **)&ecdt_ptr);
>  	if (ACPI_SUCCESS(status)) {
> +		struct acpi_ec *ec_dsdt;
>  		pr_info(PREFIX "EC description table is found, configuring boot EC\n");
>  		boot_ec->command_addr = ecdt_ptr->control.address;
>  		boot_ec->data_addr = ecdt_ptr->data.address;
> @@ -1004,7 +1005,63 @@ int __init acpi_ec_ecdt_probe(void)
>  		}
>  		boot_ec->gpe = ecdt_ptr->gpe;
>  		boot_ec->handle = ACPI_ROOT_OBJECT;
> -		acpi_get_handle(ACPI_ROOT_OBJECT, ecdt_ptr->id, &boot_ec->handle);
> +		status = acpi_get_handle(ACPI_ROOT_OBJECT, ecdt_ptr->id,
> +					&boot_ec->handle);
> +		if (ACPI_FAILURE(status)) {
> +			/*
> +			 * If the failure is returned by the function of
> +			 * acpi_get_handle, it indicates that the EC namepath
> +			 * is given by ECDT table. Print the BIOS bug.
> +			 */
> +			printk(KERN_INFO PREFIX "BIOS bug. The incorrect "
> +				"namepath is given in ECDT table\n");
> +		}
> +		/*
> +		 * Don't trust the namepath given by ECDT table. So we should
> +		 * walk ACPI namespace tree to get the EC device in DSDT table.
> +		 */
> +		ec_dsdt = NULL;
> +		ec_dsdt = kzalloc(sizeof(struct acpi_ec), GFP_KERNEL);
> +		status = acpi_get_devices(ec_device_ids[0].id,
> +				ec_parse_device, ec_dsdt, NULL);
> +		/* check that acpi_get_devices actually find something
> +		 */
> +		if (ACPI_FAILURE(status) || !ec_dsdt->handle) {
> +			printk(KERN_INFO PREFIX "BIOS bug: Incorrect "
> +				"EC device in DSDT table\n");
> +			kfree(ec_dsdt);
> +			ec_dsdt = NULL;
> +		}
> +		/*
> +		 * When EC device is parsed successfully in DSDT table, compare
> +		 * it with that in ECDT table. If they don't mismatch, the EC
> +		 * device is initialized instead of that in ECDT table
> +		 */
> +		if (ACPI_SUCCESS(status) && ec_dsdt) {
> +			/* Compare the Cmd/Status I/O port */
> +			if (ec_dsdt->command_addr !=
> +				boot_ec->command_addr) {
> +				printk(KERN_INFO PREFIX "BIOS bug: Incorrect "
> +				"EC cmd/status I/O port in ECDT table\n");
> +				boot_ec->command_addr = ec_dsdt->
> +							command_addr;
> +			}
> +			/* Compare the Data I/O port */
> +			if (ec_dsdt->data_addr != boot_ec->data_addr) {
> +				printk(KERN_INFO PREFIX "BIOS bug: "
> +				"Incorrect EC Data I/O port in ECDT table\n");
> +				boot_ec->data_addr = ec_dsdt->data_addr;
> +			}
> +			/* Compare the EC GPE number */
> +			if (ec_dsdt->gpe != boot_ec->gpe) {
> +				printk(KERN_INFO PREFIX "BIOS bug: "
> +				"Incorrect EC GPE number in ECDT table\n");
> +				boot_ec->gpe = ec_dsdt->gpe;
> +			}
> +			boot_ec->global_lock = ec_dsdt->global_lock;
> +			kfree(ec_dsdt);
> +			ec_dsdt = NULL;
> +		}
>  	} else {
>  		/* This workaround is needed only on some broken machines,
>  		 * which require early EC, but fail to provide ECDT */
> 
> 

Re: [RFC] [Patch 1/2] ACPI :Compare EC device in DSDT table with that in ECDT table

[Zhao Yakui]

On Mon, 2008-11-17 at 16:28 +0800, Alexey Starikovskiy wrote:
> Hi Yakui,
> 
Thanks for the good advices.
> Couple of concerns -- I think, I already mentioned one, but still...
> 1. By parsing DSDT unconditionally, we loose all performance (if any) benefits of
> having ECDT table. If we just don't look into ECDT, we will save some efforts.
Agree with what you said. It is not appropriate to punish the system
that already works well. It will take more time to execute the function
of acpi_ec_ecdt_probe if ECDT table exists. OS can't confirm whether the
ECDT table is correct before this check. Only after the check is added,
we can know whether the info defined in ECDT table is reliable. So in my
patch it is assumed that the info defined in ECDT table is not
reliable. 
> 2. We are speaking about broken (hopefully rare) BIOS -- it might not be wise to punish everyone (see #1).
> How about adding check in acpi_ec_start(), that with the same GPE we found difference in addresses?
   It is a good point to add the check in the acpi_ec_start. But in such
case there exists two issues:
     a. OS will scan the ACPI namespace to build the ACPI device tree
before the ACPI ec driver is registered. As the EC device is already
initialized, maybe the EC will be accessed in the scanning of ACPI
device tree.(For example: in the _STA object of battery, AC Adapter). In
such case some warning message will be reported.
     >ACPI: EC: input buffer is not empty, aborting transaction
    >[18.125459] ACPI Exception (evregion-0420): AE_TIME, Returned by Handler
for [EmbeddedControl] [20070126]
    
    After the acpi ec driver is registered, the above message won't be reported again.
But it seems very confusing.

    b. On the laptops of bug 11880 all the info in ECDT table is incorrect. 
The EC GPE number and EC namepath are incorrect besides the EC Cmd/Status/Data I/O port.
    As there exists the ECDT table, the EC device will be initialized and the incorrect 
EC GPE is enabled before building ACPI device tree.  In such case although the EC address
info can be corrected by calling the ec_parse_device in acpi_ec_start, the EC GPE can't be
corrected(Still the incorrect GPE number is enabled for EC). 
> And if this check triggers, print "Your ECDT does not conform to DSDT. Add acpi_ec.skip_ecdt=1 kernel option."
> Add acpi_ec.skip_ecdt option and skip ECDT parse if it is set...
> 3. This patch seems to imply that DSDT contains single EC. As this is first place with such assumption,
    Yes. What you said is right. One EC device is assumed in my patch.
In theory there exists more than one EC device. But it seems that now no
laptops have more than one EC devices. Can this case be ignored?
    
> please re-think it.

> 
> Regards,
> Alex.
> 
> Zhao Yakui wrote:
> > Subject: ACPI: Compare EC device in DSDT table with that in ECDT table
> > From: Zhao Yakui <yakui.zhao@intel.com>
> > 
> > On some broken laptops the EC device defined in ECDT table is incorrect. For
> > example:
> >         The incorrect Command/Status I/O Port
> >         The incorrect Data I/O port
> >         Sometimes the GPE number is incorrect.
> >         Sometimes the EC namepath is also incorrect.
> > In such case the EC device can't be initialized correctly. Maybe it is
> > appropriate to compare the EC device in DSDT table with that in ECDT table.
> > If they mismatch, the BIOS bug will be reported and the EC device parsed
> > in DSDT table will be initialized instead of that defined in ECDT table.
> > 
> > http://bugzilla.kernel.org/show_bug.cgi?id=9399
> > http://bugzilla.kernel.org/show_bug.cgi?id=11880
> > 
> > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> > cc: Alexey Starikovskiy <astarikovskiy@suse.de>
> > ---
> >  drivers/acpi/ec.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 58 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6/drivers/acpi/ec.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/ec.c
> > +++ linux-2.6/drivers/acpi/ec.c
> > @@ -990,6 +990,7 @@ int __init acpi_ec_ecdt_probe(void)
> >  	status = acpi_get_table(ACPI_SIG_ECDT, 1,
> >  				(struct acpi_table_header **)&ecdt_ptr);
> >  	if (ACPI_SUCCESS(status)) {
> > +		struct acpi_ec *ec_dsdt;
> >  		pr_info(PREFIX "EC description table is found, configuring boot EC\n");
> >  		boot_ec->command_addr = ecdt_ptr->control.address;
> >  		boot_ec->data_addr = ecdt_ptr->data.address;
> > @@ -1004,7 +1005,63 @@ int __init acpi_ec_ecdt_probe(void)
> >  		}
> >  		boot_ec->gpe = ecdt_ptr->gpe;
> >  		boot_ec->handle = ACPI_ROOT_OBJECT;
> > -		acpi_get_handle(ACPI_ROOT_OBJECT, ecdt_ptr->id, &boot_ec->handle);
> > +		status = acpi_get_handle(ACPI_ROOT_OBJECT, ecdt_ptr->id,
> > +					&boot_ec->handle);
> > +		if (ACPI_FAILURE(status)) {
> > +			/*
> > +			 * If the failure is returned by the function of
> > +			 * acpi_get_handle, it indicates that the EC namepath
> > +			 * is given by ECDT table. Print the BIOS bug.
> > +			 */
> > +			printk(KERN_INFO PREFIX "BIOS bug. The incorrect "
> > +				"namepath is given in ECDT table\n");
> > +		}
> > +		/*
> > +		 * Don't trust the namepath given by ECDT table. So we should
> > +		 * walk ACPI namespace tree to get the EC device in DSDT table.
> > +		 */
> > +		ec_dsdt = NULL;
> > +		ec_dsdt = kzalloc(sizeof(struct acpi_ec), GFP_KERNEL);
> > +		status = acpi_get_devices(ec_device_ids[0].id,
> > +				ec_parse_device, ec_dsdt, NULL);
> > +		/* check that acpi_get_devices actually find something
> > +		 */
> > +		if (ACPI_FAILURE(status) || !ec_dsdt->handle) {
> > +			printk(KERN_INFO PREFIX "BIOS bug: Incorrect "
> > +				"EC device in DSDT table\n");
> > +			kfree(ec_dsdt);
> > +			ec_dsdt = NULL;
> > +		}
> > +		/*
> > +		 * When EC device is parsed successfully in DSDT table, compare
> > +		 * it with that in ECDT table. If they don't mismatch, the EC
> > +		 * device is initialized instead of that in ECDT table
> > +		 */
> > +		if (ACPI_SUCCESS(status) && ec_dsdt) {
> > +			/* Compare the Cmd/Status I/O port */
> > +			if (ec_dsdt->command_addr !=
> > +				boot_ec->command_addr) {
> > +				printk(KERN_INFO PREFIX "BIOS bug: Incorrect "
> > +				"EC cmd/status I/O port in ECDT table\n");
> > +				boot_ec->command_addr = ec_dsdt->
> > +							command_addr;
> > +			}
> > +			/* Compare the Data I/O port */
> > +			if (ec_dsdt->data_addr != boot_ec->data_addr) {
> > +				printk(KERN_INFO PREFIX "BIOS bug: "
> > +				"Incorrect EC Data I/O port in ECDT table\n");
> > +				boot_ec->data_addr = ec_dsdt->data_addr;
> > +			}
> > +			/* Compare the EC GPE number */
> > +			if (ec_dsdt->gpe != boot_ec->gpe) {
> > +				printk(KERN_INFO PREFIX "BIOS bug: "
> > +				"Incorrect EC GPE number in ECDT table\n");
> > +				boot_ec->gpe = ec_dsdt->gpe;
> > +			}
> > +			boot_ec->global_lock = ec_dsdt->global_lock;
> > +			kfree(ec_dsdt);
> > +			ec_dsdt = NULL;
> > +		}
> >  	} else {
> >  		/* This workaround is needed only on some broken machines,
> >  		 * which require early EC, but fail to provide ECDT */
> > 
> > 
> 

Re: [RFC] [Patch 1/2] ACPI :Compare EC device in DSDT table with that in ECDT table

[Alexey Starikovskiy]

Zhao Yakui wrote:
>> Couple of concerns -- I think, I already mentioned one, but still...
>> 1. By parsing DSDT unconditionally, we loose all performance (if any) benefits of
>> having ECDT table. If we just don't look into ECDT, we will save some efforts.
>>     
> Agree with what you said. It is not appropriate to punish the system
> that already works well. It will take more time to execute the function
> of acpi_ec_ecdt_probe if ECDT table exists. OS can't confirm whether the
> ECDT table is correct before this check. Only after the check is added,
> we can know whether the info defined in ECDT table is reliable. So in my
> patch it is assumed that the info defined in ECDT table is not
> reliable.
So, either ECDT parse should go the way of do-do,
or DSDT parse in presence of ECDT should not happen.
I prefer second way -- as it is closer to the specification :)
>  
>   
>> 2. We are speaking about broken (hopefully rare) BIOS -- it might not be wise to punish everyone (see #1).
>> How about adding check in acpi_ec_start(), that with the same GPE we found difference in addresses?
>>     
>    It is a good point to add the check in the acpi_ec_start. But in such
> case there exists two issues:
>      a. OS will scan the ACPI namespace to build the ACPI device tree
> before the ACPI ec driver is registered. As the EC device is already
> initialized, maybe the EC will be accessed in the scanning of ACPI
> device tree.(For example: in the _STA object of battery, AC Adapter). In
> such case some warning message will be reported.
>      >ACPI: EC: input buffer is not empty, aborting transaction
>     >[18.125459] ACPI Exception (evregion-0420): AE_TIME, Returned by Handler
> for [EmbeddedControl] [20070126]
>   
Remember, we are speaking about broken hardware, it is OK to spill some 
errors, while
we provide solution to user in the end. In this case only user of broken 
hardware is punished, not everybody.
>     
>     After the acpi ec driver is registered, the above message won't be reported again.
> But it seems very confusing.
>
>     b. On the laptops of bug 11880 all the info in ECDT table is incorrect. 
> The EC GPE number and EC namepath are incorrect besides the EC Cmd/Status/Data I/O port.
>     As there exists the ECDT table, the EC device will be initialized and the incorrect 
> EC GPE is enabled before building ACPI device tree.  In such case although the EC address
> info can be corrected by calling the ec_parse_device in acpi_ec_start, the EC GPE can't be
> corrected(Still the incorrect GPE number is enabled for EC).
You could insert some check like if ("errors in boot_ec" || 
"acpi_ec_start does not find boot ec"), print same advice...

>  
>   
>> And if this check triggers, print "Your ECDT does not conform to DSDT. Add acpi_ec.skip_ecdt=1 kernel option."
>> Add acpi_ec.skip_ecdt option and skip ECDT parse if it is set...
>> 3. This patch seems to imply that DSDT contains single EC. As this is first place with such assumption,
>>     
>     Yes. What you said is right. One EC device is assumed in my patch.
> In theory there exists more than one EC device. But it seems that now no
> laptops have more than one EC devices. Can this case be ignored?
>   
I have notebook  with 2 EC on my desk at the moment, made by FIC/VIA.
I don't like to break working hardware to enable broken.

Regards,
Alex.

Re: [RFC] [Patch 1/2] ACPI :Compare EC device in DSDT table with that in ECDT table

[Alexey Starikovskiy]

> Zhao Yakui wrote:
>>     b. On the laptops of bug 11880 all the info in ECDT table is 
>> incorrect. The EC GPE number and EC namepath are incorrect besides the 
>> EC Cmd/Status/Data I/O port.
Actually, 11880 does not require any magic or comparison of DSDT and ECDT,
you may just compare command and data addresses, and if they are the same,
skip ECDT.
> Regards,
> Alex.

Re: [RFC] [Patch 1/2] ACPI :Compare EC device in DSDT table with that in ECDT table

[Zhao Yakui]

On Mon, 2008-11-17 at 18:46 +0800, Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
> >> Couple of concerns -- I think, I already mentioned one, but still...
> >> 1. By parsing DSDT unconditionally, we loose all performance (if any) benefits of
> >> having ECDT table. If we just don't look into ECDT, we will save some efforts.
> >>     
> > Agree with what you said. It is not appropriate to punish the system
> > that already works well. It will take more time to execute the function
> > of acpi_ec_ecdt_probe if ECDT table exists. OS can't confirm whether the
> > ECDT table is correct before this check. Only after the check is added,
> > we can know whether the info defined in ECDT table is reliable. So in my
> > patch it is assumed that the info defined in ECDT table is not
> > reliable.
> So, either ECDT parse should go the way of do-do,
> or DSDT parse in presence of ECDT should not happen.
> I prefer second way -- as it is closer to the specification :)
> > for [EmbeddedControl] [20070126]

> >  
> >   
> >> 2. We are speaking about broken (hopefully rare) BIOS -- it might not be wise to punish everyone (see #1).
> >> How about adding check in acpi_ec_start(), that with the same GPE we found difference in addresses?
> >>     
> >    It is a good point to add the check in the acpi_ec_start. But in such
> > case there exists two issues:
> >      a. OS will scan the ACPI namespace to build the ACPI device tree
> > before the ACPI ec driver is registered. As the EC device is already
> > initialized, maybe the EC will be accessed in the scanning of ACPI
> > device tree.(For example: in the _STA object of battery, AC Adapter). In
> > such case some warning message will be reported.
> >      >ACPI: EC: input buffer is not empty, aborting transaction
> >     >[18.125459] ACPI Exception (evregion-0420): AE_TIME, Returned by Handler
> > for [EmbeddedControl] [20070126]
> >   
Yes. It is not good to add the strict check on the "good vendors". But
how can we know that it is "good vendors" before checking? Now it is
realized by our checking the ACPI tables manually. After the bug
reporters open a new bug and attach the acpidump, we check the acpidump
and find that the BIOS is broken(incorrect ECDT table).
          
> Remember, we are speaking about broken hardware, it is OK to spill some 
> errors, while
> we provide solution to user in the end. In this case only user of broken 
> hardware is punished, not everybody.
> >     
> >     After the acpi ec driver is registered, the above message won't be reported again.
> > But it seems very confusing.
> >
> >     b. On the laptops of bug 11880 all the info in ECDT table is incorrect. 
> > The EC GPE number and EC namepath are incorrect besides the EC Cmd/Status/Data I/O port.
> >     As there exists the ECDT table, the EC device will be initialized and the incorrect 
> > EC GPE is enabled before building ACPI device tree.  In such case although the EC address
> > info can be corrected by calling the ec_parse_device in acpi_ec_start, the EC GPE can't be
> > corrected(Still the incorrect GPE number is enabled for EC).
> You could insert some check like if ("errors in boot_ec" || 
> "acpi_ec_start does not find boot ec"), print same advice...
> 
> >  
> >   
> >> And if this check triggers, print "Your ECDT does not conform to DSDT. Add acpi_ec.skip_ecdt=1 kernel option."
> >> Add acpi_ec.skip_ecdt option and skip ECDT parse if it is set...
> >> 3. This patch seems to imply that DSDT contains single EC. As this is first place with such assumption,
> >>     
> >     Yes. What you said is right. One EC device is assumed in my patch.
> > In theory there exists more than one EC device. But it seems that now no
> > laptops have more than one EC devices. Can this case be ignored?
> >   
> I have notebook  with 2 EC on my desk at the moment, made by FIC/VIA.
> I don't like to break working hardware to enable broken.
Does there exist the ECDT table for one EC device on your laptop?
     In my patch it is meaningful for the laptop with ECDT table. If
there is no ECDT table, it will continue its original routine.

Do you mind that I reassign the bug 11880 to you?
Thanks.
> 
> Regards,
> Alex.