public inbox for linux-acpi@vger.kernel.org
 help / color / mirror / Atom feed
* [RFC]  [PATCH] ACPI :Remove the EC space handler explicitly when failing in _REG object
@ 2008-11-26  8:50 Zhao Yakui
  2008-11-26 20:03 ` Alexey Starikovskiy
  0 siblings, 1 reply; 9+ messages in thread
From: Zhao Yakui @ 2008-11-26  8:50 UTC (permalink / raw)
  To: linux-acpi; +Cc: lenb, astarikovskiy

Hi, All

    On some laptops as it fails in evaluating the _REG object, which brings that
EC device can't be initialized correctly. But the EC space handler is not removed.
In such case maybe EC internal register will still be accessed in AML code. It is
not reasonable.
    The following patch is to fix the above issue. When it fails in evaluating the
_REG object, the EC space handler should be removed explicitly. 
    Thanks for the comments.
    

    On some laptops the incorrect status is returned by acpi_install_space
_handler because of the buggy _REG object, which causes that the EC device can't
be initialized correctly. 
    In such case it will be appropriate that the EC space handler is removed
explicitly if OS fails in evaluating _REG object.

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

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

Index: linux-2.6/drivers/acpi/ec.c
===================================================================
--- linux-2.6.orig/drivers/acpi/ec.c
+++ linux-2.6/drivers/acpi/ec.c
@@ -916,6 +916,17 @@ static int ec_install_handlers(struct ac
 			printk(KERN_ERR "Fail in evaluating the _REG object"
 				" of EC device. Broken bios is suspected.\n");
 		} else {
+			/*
+			 * If incorrect status is returned because of the buggy
+			 * _REG object, the EC space handler should be
+			 * uninstalled explicitly.
+			 * Otherwise maybe the EC internal register is still
+			 * accessed in AML code although EC device is not
+			 * initialized correctly.
+			 */
+			acpi_remove_address_space_handler(ec->handle,
+						ACPI_ADR_SPACE_EC,
+						&acpi_ec_space_handler);
 			acpi_remove_gpe_handler(NULL, ec->gpe,
 				&acpi_ec_gpe_handler);
 			return -ENODEV;



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC]  [PATCH] ACPI :Remove the EC space handler explicitly when failing in _REG object
  2008-11-26  8:50 [RFC] [PATCH] ACPI :Remove the EC space handler explicitly when failing in _REG object Zhao Yakui
@ 2008-11-26 20:03 ` Alexey Starikovskiy
  2008-11-27  1:29   ` Zhao Yakui
  0 siblings, 1 reply; 9+ messages in thread
From: Alexey Starikovskiy @ 2008-11-26 20:03 UTC (permalink / raw)
  To: Zhao Yakui; +Cc: linux-acpi, lenb, astarikovskiy

This is hardly a "fix", you just disable EC on this machine... Famous 
question
"how windows runs on this machine?" is not answered...

Regards,
Alex.

Zhao Yakui wrote:
> Hi, All
>
>     On some laptops as it fails in evaluating the _REG object, which brings that
> EC device can't be initialized correctly. But the EC space handler is not removed.
> In such case maybe EC internal register will still be accessed in AML code. It is
> not reasonable.
>     The following patch is to fix the above issue. When it fails in evaluating the
> _REG object, the EC space handler should be removed explicitly. 
>     Thanks for the comments.
>     
>
>     On some laptops the incorrect status is returned by acpi_install_space
> _handler because of the buggy _REG object, which causes that the EC device can't
> be initialized correctly. 
>     In such case it will be appropriate that the EC space handler is removed
> explicitly if OS fails in evaluating _REG object.
>
> http://bugzilla.kernel.org/show_bug.cgi?id=11884
>
> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> cc:Alexey Starikovskiy <astarikovskiy@suse.de>
> ---
>  drivers/acpi/ec.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> Index: linux-2.6/drivers/acpi/ec.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/ec.c
> +++ linux-2.6/drivers/acpi/ec.c
> @@ -916,6 +916,17 @@ static int ec_install_handlers(struct ac
>  			printk(KERN_ERR "Fail in evaluating the _REG object"
>  				" of EC device. Broken bios is suspected.\n");
>  		} else {
> +			/*
> +			 * If incorrect status is returned because of the buggy
> +			 * _REG object, the EC space handler should be
> +			 * uninstalled explicitly.
> +			 * Otherwise maybe the EC internal register is still
> +			 * accessed in AML code although EC device is not
> +			 * initialized correctly.
> +			 */
> +			acpi_remove_address_space_handler(ec->handle,
> +						ACPI_ADR_SPACE_EC,
> +						&acpi_ec_space_handler);
>  			acpi_remove_gpe_handler(NULL, ec->gpe,
>  				&acpi_ec_gpe_handler);
>  			return -ENODEV;
>
>
> --
> 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
>   


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC]  [PATCH] ACPI :Remove the EC space handler explicitly when failing in _REG object
  2008-11-26 20:03 ` Alexey Starikovskiy
@ 2008-11-27  1:29   ` Zhao Yakui
  2008-11-27  9:42     ` Alexey Starikovskiy
  0 siblings, 1 reply; 9+ messages in thread
From: Zhao Yakui @ 2008-11-27  1:29 UTC (permalink / raw)
  To: Alexey Starikovskiy
  Cc: linux-acpi@vger.kernel.org, lenb@kernel.org,
	astarikovskiy@suse.de

On Thu, 2008-11-27 at 04:03 +0800, Alexey Starikovskiy wrote:
> This is hardly a "fix", you just disable EC on this machine... Famous 
Yes. This can't fix the issue on the box of bug11884. 
But in theory if the incorrect status is returned by
acpi_install_address_space_handler, the EC space handler should be
removed explicitly. Right? In such case the EC device will be disabled
on this box. 
    But if the EC space handler is not removed and EC flag in AML code
still indicates that EC operation region is already accessible, the EC
internal register will be accessed in AML code. At the same time the
memory region pointed by acpi_ec is already freed. This is the fourth
argument of ec_space_handler.
    In such case if the EC space handler is still accessed, OS will try
to access the memory region already freed. Maybe the kernel panic will
be reported and the system can't be booted.
    In fact we have a similar bug 10237. And on that box of bug10237 OS
fails in evaluating _REG object and then kernel panic is reported. 
    The workaround patch is that OS ignores the error and continue to
initialize EC device if AE_NOT_FOUND is returned by the _REG object.
In such case the system can be booted very normally.
> "how windows runs on this machine?" is not answered...
Windows can work well on such boxes while Linux can't work well.This is
the gap between Linux and windows. It is our target to narrow the gap.
Maybe on windows the incorrect status returned by EC _REG object is
ignored by EC driver. In such case the EC device can be initialized
correctly. 
> Regards,
> Alex.
> 
> Zhao Yakui wrote:
> > Hi, All
> >
> >     On some laptops as it fails in evaluating the _REG object, which brings that
> > EC device can't be initialized correctly. But the EC space handler is not removed.
> > In such case maybe EC internal register will still be accessed in AML code. It is
> > not reasonable.
> >     The following patch is to fix the above issue. When it fails in evaluating the
> > _REG object, the EC space handler should be removed explicitly. 
> >     Thanks for the comments.
> >     
> >
> >     On some laptops the incorrect status is returned by acpi_install_space
> > _handler because of the buggy _REG object, which causes that the EC device can't
> > be initialized correctly. 
> >     In such case it will be appropriate that the EC space handler is removed
> > explicitly if OS fails in evaluating _REG object.
> >
> > http://bugzilla.kernel.org/show_bug.cgi?id=11884
> >
> > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> > cc:Alexey Starikovskiy <astarikovskiy@suse.de>
> > ---
> >  drivers/acpi/ec.c |   11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > Index: linux-2.6/drivers/acpi/ec.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/ec.c
> > +++ linux-2.6/drivers/acpi/ec.c
> > @@ -916,6 +916,17 @@ static int ec_install_handlers(struct ac
> >  			printk(KERN_ERR "Fail in evaluating the _REG object"
> >  				" of EC device. Broken bios is suspected.\n");
> >  		} else {
> > +			/*
> > +			 * If incorrect status is returned because of the buggy
> > +			 * _REG object, the EC space handler should be
> > +			 * uninstalled explicitly.
> > +			 * Otherwise maybe the EC internal register is still
> > +			 * accessed in AML code although EC device is not
> > +			 * initialized correctly.
> > +			 */
> > +			acpi_remove_address_space_handler(ec->handle,
> > +						ACPI_ADR_SPACE_EC,
> > +						&acpi_ec_space_handler);
> >  			acpi_remove_gpe_handler(NULL, ec->gpe,
> >  				&acpi_ec_gpe_handler);
> >  			return -ENODEV;
> >
> >
> > --
> > 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
> >   
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC]  [PATCH] ACPI :Remove the EC space handler explicitly when failing in _REG object
  2008-11-27  1:29   ` Zhao Yakui
@ 2008-11-27  9:42     ` Alexey Starikovskiy
  2008-11-28  2:57       ` Zhao Yakui
  0 siblings, 1 reply; 9+ messages in thread
From: Alexey Starikovskiy @ 2008-11-27  9:42 UTC (permalink / raw)
  To: Zhao Yakui
  Cc: Alexey Starikovskiy, linux-acpi@vger.kernel.org, lenb@kernel.org

Zhao Yakui wrote:
> On Thu, 2008-11-27 at 04:03 +0800, Alexey Starikovskiy wrote:
>> This is hardly a "fix", you just disable EC on this machine... Famous 
> Yes. This can't fix the issue on the box of bug11884. 
> But in theory if the incorrect status is returned by
> acpi_install_address_space_handler, the EC space handler should be
> removed explicitly. Right? In such case the EC device will be disabled
> on this box. 
Wrong. If any function is allowed to fail, you should not call counter function
to recover -- you don't call free() on failed malloc(), you don't call close() on
failed open().
>     But if the EC space handler is not removed and EC flag in AML code
> still indicates that EC operation region is already accessible, the EC
> internal register will be accessed in AML code. At the same time the
> memory region pointed by acpi_ec is already freed. This is the fourth
> argument of ec_space_handler.
I already told you and here is second time -- your fixes come in wrong place.
You should fix acpi_install_address_space_handler(), not all the callers of it.
>     In such case if the EC space handler is still accessed, OS will try
> to access the memory region already freed. Maybe the kernel panic will
> be reported and the system can't be booted.
>     In fact we have a similar bug 10237. And on that box of bug10237 OS
> fails in evaluating _REG object and then kernel panic is reported. 
>     The workaround patch is that OS ignores the error and continue to
> initialize EC device if AE_NOT_FOUND is returned by the _REG object.
> In such case the system can be booted very normally.
Yes, and this workaround should not be in EC driver too.
>> "how windows runs on this machine?" is not answered...
> Windows can work well on such boxes while Linux can't work well.This is
> the gap between Linux and windows. It is our target to narrow the gap.
> Maybe on windows the incorrect status returned by EC _REG object is
> ignored by EC driver. In such case the EC device can be initialized
> correctly.
So, why you disable EC driver instead of ignoring error from _REG completely?
By doing this, you are _widening_ the gap, not narrowing it.
Please consider moving 20edd74fcf9ad02c19efba0c13670a7b6b045099 out of the ec.c and
widen it to ignore failure from _REG method completely.
>> Regards,
>> Alex.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC]  [PATCH] ACPI :Remove the EC space handler explicitly when failing in _REG object
  2008-11-27  9:42     ` Alexey Starikovskiy
@ 2008-11-28  2:57       ` Zhao Yakui
  2008-11-28  8:08         ` Alexey Starikovskiy
  0 siblings, 1 reply; 9+ messages in thread
From: Zhao Yakui @ 2008-11-28  2:57 UTC (permalink / raw)
  To: Alexey Starikovskiy
  Cc: Alexey Starikovskiy, linux-acpi@vger.kernel.org, lenb@kernel.org

On Thu, 2008-11-27 at 17:42 +0800, Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
> > On Thu, 2008-11-27 at 04:03 +0800, Alexey Starikovskiy wrote:
> >> This is hardly a "fix", you just disable EC on this machine... Famous 
> > Yes. This can't fix the issue on the box of bug11884. 
> > But in theory if the incorrect status is returned by
> > acpi_install_address_space_handler, the EC space handler should be
> > removed explicitly. Right? In such case the EC device will be disabled
> > on this box. 
> Wrong. If any function is allowed to fail, you should not call counter function
> to recover -- you don't call free() on failed malloc(), you don't call close() on
> failed open().

In the function of acpi_install_address_space_handler the following two
steps are done in ACPCA:
   a. Install the space handler for one operation region. This space
handler is applied for the same type operation region of every device.
   b. run the _REG object for all the operation regions using the space
id. It will enumerate the ACPI namespace to execute the _REG object for
the same operation region

If the space handler is removed in the function of
acpi_install_address_space_handler when failure in _REG object happens,
it is unnecessary to remove the space handler explicitly. 
   But in fact maybe there exists the _REG object for other operation
region besides EC operation region. If failure in _REG object happens,
maybe the space handler should not be applied for the corresponding
device. PCI space handler is used by all the PCI devices. It is
unreasonable that all the PCI devices can't use the pci space handler if
OS fails in the _REG object of one PCI device.
   So it is difficult to deal with this issue in the function of
acpi_install_address_space_handler.

Maybe it will be easy to deal with such issue in driver.

> >     But if the EC space handler is not removed and EC flag in AML code
> > still indicates that EC operation region is already accessible, the EC
> > internal register will be accessed in AML code. At the same time the
> > memory region pointed by acpi_ec is already freed. This is the fourth
> > argument of ec_space_handler.
> I already told you and here is second time -- your fixes come in wrong place.
> You should fix acpi_install_address_space_handler(), not all the callers of it.
It is difficult for me to fix this issue in
acpi_install_address_space_handler. I am not willing to change the code
of ACPICA. It is not very reasonable that all the failures in _REG
object for all the operations region are ignored.
And it is also unreasonable that the space handler is removed for the
same type operation region of all the devices only because OS fails in
_REG object of one device.
If you have better method to fix such issue, please write it. 

> >     In such case if the EC space handler is still accessed, OS will try
> > to access the memory region already freed. Maybe the kernel panic will
> > be reported and the system can't be booted.
> >     In fact we have a similar bug 10237. And on that box of bug10237 OS
> > fails in evaluating _REG object and then kernel panic is reported. 
> >     The workaround patch is that OS ignores the error and continue to
> > initialize EC device if AE_NOT_FOUND is returned by the _REG object.
> > In such case the system can be booted very normally.
> Yes, and this workaround should not be in EC driver too.
> >> "how windows runs on this machine?" is not answered...
> > Windows can work well on such boxes while Linux can't work well.This is
> > the gap between Linux and windows. It is our target to narrow the gap.
> > Maybe on windows the incorrect status returned by EC _REG object is
> > ignored by EC driver. In such case the EC device can be initialized
> > correctly.
> So, why you disable EC driver instead of ignoring error from _REG completely?
> By doing this, you are _widening_ the gap, not narrowing it.
> Please consider moving 20edd74fcf9ad02c19efba0c13670a7b6b045099 out of the ec.c and
> widen it to ignore failure from _REG method completely.
> >> Regards,
> >> Alex.
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC]  [PATCH] ACPI :Remove the EC space handler explicitly when failing in _REG object
  2008-11-28  2:57       ` Zhao Yakui
@ 2008-11-28  8:08         ` Alexey Starikovskiy
  2008-12-01  1:53           ` Zhao Yakui
                             ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Alexey Starikovskiy @ 2008-11-28  8:08 UTC (permalink / raw)
  To: Zhao Yakui
  Cc: Alexey Starikovskiy, linux-acpi@vger.kernel.org, lenb@kernel.org

Zhao Yakui wrote:
> On Thu, 2008-11-27 at 17:42 +0800, Alexey Starikovskiy wrote:
>> Zhao Yakui wrote:
>>> On Thu, 2008-11-27 at 04:03 +0800, Alexey Starikovskiy wrote:
>>>> This is hardly a "fix", you just disable EC on this machine... Famous 
>>> Yes. This can't fix the issue on the box of bug11884. 
>>> But in theory if the incorrect status is returned by
>>> acpi_install_address_space_handler, the EC space handler should be
>>> removed explicitly. Right? In such case the EC device will be disabled
>>> on this box. 
>> Wrong. If any function is allowed to fail, you should not call counter function
>> to recover -- you don't call free() on failed malloc(), you don't call close() on
>> failed open().
> 
> In the function of acpi_install_address_space_handler the following two
> steps are done in ACPCA:
>    a. Install the space handler for one operation region. This space
> handler is applied for the same type operation region of every device.
>    b. run the _REG object for all the operation regions using the space
> id. It will enumerate the ACPI namespace to execute the _REG object for
> the same operation region
Unregister space handler will do all these things in opposite order, thus calling 
failed _REG object once again, causing more damage.
> 
> If the space handler is removed in the function of
> acpi_install_address_space_handler when failure in _REG object happens,
> it is unnecessary to remove the space handler explicitly. 
>    But in fact maybe there exists the _REG object for other operation
> region besides EC operation region. If failure in _REG object happens,
> maybe the space handler should not be applied for the corresponding
> device. PCI space handler is used by all the PCI devices. It is
> unreasonable that all the PCI devices can't use the pci space handler if
> OS fails in the _REG object of one PCI device.
>    So it is difficult to deal with this issue in the function of
> acpi_install_address_space_handler.
> 
> Maybe it will be easy to deal with such issue in driver.
> 
>>>     But if the EC space handler is not removed and EC flag in AML code
>>> still indicates that EC operation region is already accessible, the EC
>>> internal register will be accessed in AML code. At the same time the
>>> memory region pointed by acpi_ec is already freed. This is the fourth
>>> argument of ec_space_handler.
>> I already told you and here is second time -- your fixes come in wrong place.
>> You should fix acpi_install_address_space_handler(), not all the callers of it.
> It is difficult for me to fix this issue in
> acpi_install_address_space_handler. I am not willing to change the code
> of ACPICA. It is not very reasonable that all the failures in _REG
> object for all the operations region are ignored.
You have address space identifier in this function, so if you want your 
workaround to be "only for EC", check it. 
> And it is also unreasonable that the space handler is removed for the
> same type operation region of all the devices only because OS fails in
> _REG object of one device.
> If you have better method to fix such issue, please write it. 
Yakui, why should I complete all your jobs?

Alex.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC]  [PATCH] ACPI :Remove the EC space handler explicitly when failing in _REG object
  2008-11-28  8:08         ` Alexey Starikovskiy
@ 2008-12-01  1:53           ` Zhao Yakui
  2008-12-01  2:47           ` Zhao Yakui
  2008-12-01  8:12           ` the issue about the bogus ECDT table Zhao Yakui
  2 siblings, 0 replies; 9+ messages in thread
From: Zhao Yakui @ 2008-12-01  1:53 UTC (permalink / raw)
  To: Alexey Starikovskiy
  Cc: Alexey Starikovskiy, linux-acpi@vger.kernel.org, lenb@kernel.org

On Fri, 2008-11-28 at 16:08 +0800, Alexey Starikovskiy wrote:
> Zhao Yakui wrote:
> > On Thu, 2008-11-27 at 17:42 +0800, Alexey Starikovskiy wrote:
> >> Zhao Yakui wrote:
> >>> On Thu, 2008-11-27 at 04:03 +0800, Alexey Starikovskiy wrote:
> >>>> This is hardly a "fix", you just disable EC on this machine... Famous 
> >>> Yes. This can't fix the issue on the box of bug11884. 
> >>> But in theory if the incorrect status is returned by
> >>> acpi_install_address_space_handler, the EC space handler should be
> >>> removed explicitly. Right? In such case the EC device will be disabled
> >>> on this box. 
> >> Wrong. If any function is allowed to fail, you should not call counter function
> >> to recover -- you don't call free() on failed malloc(), you don't call close() on
> >> failed open().
> > 
> > In the function of acpi_install_address_space_handler the following two
> > steps are done in ACPCA:
> >    a. Install the space handler for one operation region. This space
> > handler is applied for the same type operation region of every device.
> >    b. run the _REG object for all the operation regions using the space
> > id. It will enumerate the ACPI namespace to execute the _REG object for
> > the same operation region
> Unregister space handler will do all these things in opposite order, thus calling 
> failed _REG object once again, causing more damage.
Uninstall the space handler will do the above things in opposite order.
First the _REG object will be called(The second argument of _REG is
zero) and then the space handler will be removed. 
    For the EC device. When the _REG object is called, it can be used to
indicate whether the EC operation region is enabled or disabled. 
     If the second argument of _REG object is one, it indicates that the
EC operation region is accessible. And the flag in AML code will be set
to one.
     If the second argument of _REG object is zero, it indicates that
the EC operation region is not accessible. And the flag in AML code will
be set to zero again.
    In such case if the failed _REG object is not called again, the EC
flag in AML code indicates that the EC operation is accessible. But the
EC device is not initialized properly. It will be inconsistent.

    Will you please describe the damage in detail if the failed _REG is
called again?   
> >
> You have address space identifier in this function, so if you want your 
> workaround to be "only for EC", check it. 
Of course it can also be workaround by checking the space id in the
function of acpi_install_address_space_handler. But on most laptops
there is no issue of failed _REG object. And it is not appropriate that
such workaround source code is added in ACPICA for so rare case.
Maybe it is convenient that the workaround is added in EC driver for so
rare case. 
Thanks.
> > And it is also unreasonable that the space handler is removed for the
> > same type operation region of all the devices only because OS fails in
> > _REG object of one device.
> > If you have better method to fix such issue, please write it. 
> Yakui, why should I complete all your jobs?
> 
> Alex.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC]  [PATCH] ACPI :Remove the EC space handler explicitly when failing in _REG object
  2008-11-28  8:08         ` Alexey Starikovskiy
  2008-12-01  1:53           ` Zhao Yakui
@ 2008-12-01  2:47           ` Zhao Yakui
  2008-12-01  8:12           ` the issue about the bogus ECDT table Zhao Yakui
  2 siblings, 0 replies; 9+ messages in thread
From: Zhao Yakui @ 2008-12-01  2:47 UTC (permalink / raw)
  To: Alexey Starikovskiy
  Cc: Alexey Starikovskiy, linux-acpi@vger.kernel.org, lenb@kernel.org

On Fri, 2008-11-28 at 16:08 +0800, Alexey Starikovskiy wrote:

> > In the function of acpi_install_address_space_handler the following two
> > steps are done in ACPCA:
> >    a. Install the space handler for one operation region. This space
> > handler is applied for the same type operation region of every device.
> >    b. run the _REG object for all the operation regions using the space
> > id. It will enumerate the ACPI namespace to execute the _REG object for
> > the same operation region
> Unregister space handler will do all these things in opposite order, thus calling 
> failed _REG object once again, causing more damage.
According to the Spec the main function of _REG object is to inform the
AML code of change in the availability of an operation region. And the
spec doesn't state what should be done when failed _REG object happens.

On Linux when OS fails in evaluating the _REG object for EC device, the
EC device can't be initialized correctly. In such case the warning
message will be reported and the system can't work well.

But windows can work well on such broken BIOS. Maybe the incorrect
status of EC _REG object is ignored. But we can't confirm whether all
the incorrect status of EC _REG object is ignored on windows. At the
same time we can't confirm that the incorrect status of EC _REG object
is ignored in ACPICA interface function or EC device driver. 

Maybe on Linux the incorrect status of EC _REG object should be also
ignored. And it will be more reasonable that such workaround is added in
EC device driver(High level driver decides whether the incorrect stautus
can be ignored). At the same time as we can't know whether all the
incorrect status of EC _REG object is ignored on windows, we can do as
the following step:
    a. For the specific incorrect status, we can ignore it and continue
to initialize the EC device. For example: bug10237, the incorrect status
is AE_NOT_FOUND
    b. For the most incorrect status, we will remove the EC space
handler explicitly. (If not removed, maybe the kernel panic will be
reported)
    c. When more other incorrect status is reported by EC _REG object on
some other laptops, we can ignore the incorrect status of EC _REG object
and continue to initialize EC device.

Thanks.

> > 
> > If the space handler is removed in the function of
> > acpi_install_address_space_handler when failure in _REG object happens,
> > it is unnecessary to remove the space handler explicitly. 
> >    But in fact maybe there exists the _REG object for other operation
> > region besides EC operation region. If failure in _REG object happens,
> > maybe the space handler should not be applied for the corresponding
> > device. PCI space handler is used by all the PCI devices. It is
> > unreasonable that all the PCI devices can't use the pci space handler if
> > OS fails in the _REG object of one PCI device.
> >    So it is difficult to deal with this issue in the function of
> > acpi_install_address_space_handler.
> > 
> > Maybe it will be easy to deal with such issue in driver.
> > 
> >>>     But if the EC space handler is not removed and EC flag in AML code
> >>> still indicates that EC operation region is already accessible, the EC
> >>> internal register will be accessed in AML code. At the same time the
> >>> memory region pointed by acpi_ec is already freed. This is the fourth
> >>> argument of ec_space_handler.
> >> I already told you and here is second time -- your fixes come in wrong place.
> >> You should fix acpi_install_address_space_handler(), not all the callers of it.
> > It is difficult for me to fix this issue in
> > acpi_install_address_space_handler. I am not willing to change the code
> > of ACPICA. It is not very reasonable that all the failures in _REG
> > object for all the operations region are ignored.
> You have address space identifier in this function, so if you want your 
> workaround to be "only for EC", check it. 
> > And it is also unreasonable that the space handler is removed for the
> > same type operation region of all the devices only because OS fails in
> > _REG object of one device.
> > If you have better method to fix such issue, please write it. 
> Yakui, why should I complete all your jobs?
> 
> Alex.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* the issue about the bogus ECDT table
  2008-11-28  8:08         ` Alexey Starikovskiy
  2008-12-01  1:53           ` Zhao Yakui
  2008-12-01  2:47           ` Zhao Yakui
@ 2008-12-01  8:12           ` Zhao Yakui
  2 siblings, 0 replies; 9+ messages in thread
From: Zhao Yakui @ 2008-12-01  8:12 UTC (permalink / raw)
  To: Alexey Starikovskiy
  Cc: Alexey Starikovskiy, linux-acpi@vger.kernel.org, lenb@kernel.org

Hi, Alexey
    Now the patch in comment #20 of http://bugzilla.kernel.org/show_bug.cgi?id=11880 is
already added to Linux test tree.
    The laptops of bug 11880, 12116 can be fixed by the patch. In this patch if the ECDT Data 
I/O address is the same as the EC Command I/O address, the EC device will be parsed from
the DSDT table. Of course it is correct.
    In fact the root cause of this issue is that incorrect ECDT table is provided on the 
broken laptops. For example:
    The ECDT Command I/O data address is zero. (Incorrect)
    The ECDT Data I/O address is zero. (Incorrect)
    The GPE number is zero (Incorrect)
    The EC namepath is incorrect (Incorrect)
   
    At the same time there exists other bug about the bogus ECDT table. For example: Bug8709,
9399. On the two laptops of bug8709, 9399 the EC Command/Data IO address defined in ECDT table
is exchanged, which brings that EC can't work well in the upstream kernel. Now the problem 
still exists.
    
    Although the error of bug9399 is different with that in bug11880, they have the same issue
that the bogus ECDT table is provided. So the generic solution is that when there exists the ECDT
table OS still parses the EC device in DSDT table and compare it with that defined in ECDT table.
If they are different, the BIOS bug is reported and the device parsed in DSDT is initialized.
The following info will be compared.
    a. EC Data I/O address
    b. EC Command I/O address
    c. EC GPE number
  
   
   I use the KVM to verify this issue on windows. Windows still can work well even when bogus ECDT
table is defined. The EC GPE number is obtained from DSDT table and the I/O resource is obtained from
the DSDT table. The following types of bogus ECDT table are tested on windows:
    a. zero ECDT table (the info in ECDT table is all zero)
    b. exchanged EC data/command I/O address
    c. incorrect GPE number in ECDT table(the address is correct)
    d. incorrect EC I/O resource( The EC data I/O address is correct. But the EC command I/O address is
incorrect)

   At the same time there is another interesting issue on windows.(This is also done using KVM).
   There exists the _INI object under the scope of EC device. And the ECDT table is also provided(The ECDT table
is correct)
   On windows the EC _INI object is evaluated before the EC device is initialized. But On Linux the EC _INI 
object is evaluated after EC device is initialized.
   From the test it seems that the EC initial flowchart on Linux is different with that on windows. 
   
Thanks.
   
Best regards.
   yakui

    
   
    
   
    
    


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2008-12-01  8:05 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-11-26  8:50 [RFC] [PATCH] ACPI :Remove the EC space handler explicitly when failing in _REG object Zhao Yakui
2008-11-26 20:03 ` Alexey Starikovskiy
2008-11-27  1:29   ` Zhao Yakui
2008-11-27  9:42     ` Alexey Starikovskiy
2008-11-28  2:57       ` Zhao Yakui
2008-11-28  8:08         ` Alexey Starikovskiy
2008-12-01  1:53           ` Zhao Yakui
2008-12-01  2:47           ` Zhao Yakui
2008-12-01  8:12           ` the issue about the bogus ECDT table Zhao Yakui

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox