The aml method _BIX of NEC LZ750/LS returns a broken package which
skip the first member "Revision" according ACPI 5.0 spec Table 10-234.
This patch is to add a quirk for this machine to skip member "Revision"
during parsing _BIX returned package.
Reported-and-tested-by: Francisco Castro <[email protected]>
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=67351
Cc: 3.8+ <[email protected]>
Signed-off-by: Lan Tianyu <[email protected]>
---
Change since v1:
Remove battery_bix_package_quirk() function and set
battery_bix_broken_package flag according the returned value
of dmi_check_system().
drivers/acpi/battery.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index e90ef8b..d21cc1a 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -61,6 +61,7 @@ MODULE_AUTHOR("Alexey Starikovskiy <[email protected]>");
MODULE_DESCRIPTION("ACPI Battery Driver");
MODULE_LICENSE("GPL");
+static int battery_bix_broken_package;
static unsigned int cache_time = 1000;
module_param(cache_time, uint, 0644);
MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
@@ -415,7 +416,12 @@ static int acpi_battery_get_info(struct acpi_battery *battery)
ACPI_EXCEPTION((AE_INFO, status, "Evaluating %s", name));
return -ENODEV;
}
- if (test_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags))
+
+ if (battery_bix_broken_package)
+ result = extract_package(battery, buffer.pointer,
+ extended_info_offsets + 1,
+ ARRAY_SIZE(extended_info_offsets) - 1);
+ else if (test_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags))
result = extract_package(battery, buffer.pointer,
extended_info_offsets,
ARRAY_SIZE(extended_info_offsets));
@@ -753,6 +759,17 @@ static int battery_notify(struct notifier_block *nb,
return 0;
}
+static struct dmi_system_id bat_dmi_table[] = {
+ {
+ .ident = "NEC LZ750/LS",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "NEC"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "PC-LZ750LS"),
+ },
+ },
+ {},
+};
+
static int acpi_battery_add(struct acpi_device *device)
{
int result = 0;
@@ -845,6 +862,9 @@ static void __init acpi_battery_init_async(void *unused, async_cookie_t cookie)
{
if (acpi_disabled)
return;
+
+ if (dmi_check_system(bat_dmi_table))
+ battery_bix_broken_package = 1;
acpi_bus_register_driver(&acpi_battery_driver);
}
--
1.8.2.1
Hi Lan,
On Mon, Jan 06, 2014 at 10:50:37PM +0800, Lan Tianyu wrote:
> The aml method _BIX of NEC LZ750/LS returns a broken package which
> skip the first member "Revision" according ACPI 5.0 spec Table 10-234.
>
> This patch is to add a quirk for this machine to skip member "Revision"
> during parsing _BIX returned package.
>
> Reported-and-tested-by: Francisco Castro <[email protected]>
> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=67351
> Cc: 3.8+ <[email protected]>
> Signed-off-by: Lan Tianyu <[email protected]>
> ---
> Change since v1:
> Remove battery_bix_package_quirk() function and set
> battery_bix_broken_package flag according the returned value
> of dmi_check_system().
>
> drivers/acpi/battery.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index e90ef8b..d21cc1a 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -61,6 +61,7 @@ MODULE_AUTHOR("Alexey Starikovskiy <[email protected]>");
> MODULE_DESCRIPTION("ACPI Battery Driver");
> MODULE_LICENSE("GPL");
>
> +static int battery_bix_broken_package;
> static unsigned int cache_time = 1000;
> module_param(cache_time, uint, 0644);
> MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
> @@ -415,7 +416,12 @@ static int acpi_battery_get_info(struct acpi_battery *battery)
> ACPI_EXCEPTION((AE_INFO, status, "Evaluating %s", name));
> return -ENODEV;
> }
> - if (test_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags))
> +
> + if (battery_bix_broken_package)
> + result = extract_package(battery, buffer.pointer,
> + extended_info_offsets + 1,
> + ARRAY_SIZE(extended_info_offsets) - 1);
> + else if (test_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags))
> result = extract_package(battery, buffer.pointer,
> extended_info_offsets,
> ARRAY_SIZE(extended_info_offsets));
> @@ -753,6 +759,17 @@ static int battery_notify(struct notifier_block *nb,
> return 0;
> }
>
> +static struct dmi_system_id bat_dmi_table[] = {
> + {
> + .ident = "NEC LZ750/LS",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "NEC"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "PC-LZ750LS"),
> + },
This does not appear at be indented properly. I see there some
inventive formatting in drivers/acpi, but I believe the proper form is:
static struct dmi_system_id bat_dmi_table[] = {
{
.ident = "NEC LZ750/LS",
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "NEC"),
DMI_MATCH(DMI_PRODUCT_NAME, "PC-LZ750LS"),
},
},
{}
};
Otherwise:
Reviewed-by: Dmitry Torokhov <[email protected]>
Thanks.
--
Dmitry
On Monday, January 06, 2014 09:59:12 AM Dmitry Torokhov wrote:
> Hi Lan,
>
> On Mon, Jan 06, 2014 at 10:50:37PM +0800, Lan Tianyu wrote:
> > The aml method _BIX of NEC LZ750/LS returns a broken package which
> > skip the first member "Revision" according ACPI 5.0 spec Table 10-234.
> >
> > This patch is to add a quirk for this machine to skip member "Revision"
> > during parsing _BIX returned package.
> >
> > Reported-and-tested-by: Francisco Castro <[email protected]>
> > Reference: https://bugzilla.kernel.org/show_bug.cgi?id=67351
> > Cc: 3.8+ <[email protected]>
> > Signed-off-by: Lan Tianyu <[email protected]>
Queued up as a fix for 3.13 (I fixed up the indentation).
Thanks!
> > ---
> > Change since v1:
> > Remove battery_bix_package_quirk() function and set
> > battery_bix_broken_package flag according the returned value
> > of dmi_check_system().
> >
> > drivers/acpi/battery.c | 22 +++++++++++++++++++++-
> > 1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> > index e90ef8b..d21cc1a 100644
> > --- a/drivers/acpi/battery.c
> > +++ b/drivers/acpi/battery.c
> > @@ -61,6 +61,7 @@ MODULE_AUTHOR("Alexey Starikovskiy <[email protected]>");
> > MODULE_DESCRIPTION("ACPI Battery Driver");
> > MODULE_LICENSE("GPL");
> >
> > +static int battery_bix_broken_package;
> > static unsigned int cache_time = 1000;
> > module_param(cache_time, uint, 0644);
> > MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
> > @@ -415,7 +416,12 @@ static int acpi_battery_get_info(struct acpi_battery *battery)
> > ACPI_EXCEPTION((AE_INFO, status, "Evaluating %s", name));
> > return -ENODEV;
> > }
> > - if (test_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags))
> > +
> > + if (battery_bix_broken_package)
> > + result = extract_package(battery, buffer.pointer,
> > + extended_info_offsets + 1,
> > + ARRAY_SIZE(extended_info_offsets) - 1);
> > + else if (test_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags))
> > result = extract_package(battery, buffer.pointer,
> > extended_info_offsets,
> > ARRAY_SIZE(extended_info_offsets));
> > @@ -753,6 +759,17 @@ static int battery_notify(struct notifier_block *nb,
> > return 0;
> > }
> >
> > +static struct dmi_system_id bat_dmi_table[] = {
> > + {
> > + .ident = "NEC LZ750/LS",
> > + .matches = {
> > + DMI_MATCH(DMI_SYS_VENDOR, "NEC"),
> > + DMI_MATCH(DMI_PRODUCT_NAME, "PC-LZ750LS"),
> > + },
>
> This does not appear at be indented properly. I see there some
> inventive formatting in drivers/acpi, but I believe the proper form is:
>
> static struct dmi_system_id bat_dmi_table[] = {
> {
> .ident = "NEC LZ750/LS",
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "NEC"),
> DMI_MATCH(DMI_PRODUCT_NAME, "PC-LZ750LS"),
> },
> },
> {}
> };
>
> Otherwise:
>
> Reviewed-by: Dmitry Torokhov <[email protected]>
>
> Thanks.
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Mon, Jan 06, 2014 at 11:25:53PM +0100, Rafael J. Wysocki wrote:
> Queued up as a fix for 3.13 (I fixed up the indentation).
Ah, sorry, I missed this chunk of the thread. If the system provides
valid _BIF data then we should possibly just fall back to that rather
than adding another quirk table.
--
Matthew Garrett | [email protected]
On Tuesday, January 14, 2014 04:06:01 PM Matthew Garrett wrote:
> On Mon, Jan 06, 2014 at 11:25:53PM +0100, Rafael J. Wysocki wrote:
>
> > Queued up as a fix for 3.13 (I fixed up the indentation).
>
> Ah, sorry, I missed this chunk of the thread. If the system provides
> valid _BIF data then we should possibly just fall back to that rather
> than adding another quirk table.
The problem is to know that _BIX is broken. If we could figure that out
upfront, we woulnd't need the quirk table in any case.
Tianyu, can we do some effort during the driver initialization to detect
this breakage and handle it without blacklisting systems?
Rafael
On Tue, Jan 14, 2014 at 10:37:02PM +0100, Rafael J. Wysocki wrote:
> On Tuesday, January 14, 2014 04:06:01 PM Matthew Garrett wrote:
> > On Mon, Jan 06, 2014 at 11:25:53PM +0100, Rafael J. Wysocki wrote:
> >
> > > Queued up as a fix for 3.13 (I fixed up the indentation).
> >
> > Ah, sorry, I missed this chunk of the thread. If the system provides
> > valid _BIF data then we should possibly just fall back to that rather
> > than adding another quirk table.
>
> The problem is to know that _BIX is broken. If we could figure that out
> upfront, we woulnd't need the quirk table in any case.
It's obvious that it is in this case - the package is the wrong size.
--
Matthew Garrett | [email protected]
On Tuesday, January 14, 2014 09:24:06 PM Matthew Garrett wrote:
> On Tue, Jan 14, 2014 at 10:37:02PM +0100, Rafael J. Wysocki wrote:
> > On Tuesday, January 14, 2014 04:06:01 PM Matthew Garrett wrote:
> > > On Mon, Jan 06, 2014 at 11:25:53PM +0100, Rafael J. Wysocki wrote:
> > >
> > > > Queued up as a fix for 3.13 (I fixed up the indentation).
> > >
> > > Ah, sorry, I missed this chunk of the thread. If the system provides
> > > valid _BIF data then we should possibly just fall back to that rather
> > > than adding another quirk table.
> >
> > The problem is to know that _BIX is broken. If we could figure that out
> > upfront, we woulnd't need the quirk table in any case.
>
> It's obvious that it is in this case - the package is the wrong size.
Then Tianyu should be able to come up with a better fix relatively easily. :-)
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 01/14/2014 03:37 PM, Rafael J. Wysocki wrote:
> On Tuesday, January 14, 2014 04:06:01 PM Matthew Garrett wrote:
>> On Mon, Jan 06, 2014 at 11:25:53PM +0100, Rafael J. Wysocki wrote:
>>
>>> Queued up as a fix for 3.13 (I fixed up the indentation).
>>
>> Ah, sorry, I missed this chunk of the thread. If the system provides
>> valid _BIF data then we should possibly just fall back to that rather
>> than adding another quirk table.
>
> The problem is to know that _BIX is broken. If we could figure that out
> upfront, we woulnd't need the quirk table in any case.
>
> Tianyu, can we do some effort during the driver initialization to detect
> this breakage and handle it without blacklisting systems?
Yes, the usual question in such cases is "how does Windows manage to
function on such systems, (almost certainly) without a system-specific
hack, and can we replicate that behavior?"
On 01/15/2014 06:17 AM, Rafael J. Wysocki wrote:
> On Tuesday, January 14, 2014 09:24:06 PM Matthew Garrett wrote:
>> On Tue, Jan 14, 2014 at 10:37:02PM +0100, Rafael J. Wysocki wrote:
>>> On Tuesday, January 14, 2014 04:06:01 PM Matthew Garrett wrote:
>>>> On Mon, Jan 06, 2014 at 11:25:53PM +0100, Rafael J. Wysocki wrote:
>>>>
>>>>> Queued up as a fix for 3.13 (I fixed up the indentation).
>>>>
>>>> Ah, sorry, I missed this chunk of the thread. If the system provides
>>>> valid _BIF data then we should possibly just fall back to that rather
>>>> than adding another quirk table.
>>>
>>> The problem is to know that _BIX is broken. If we could figure that out
>>> upfront, we woulnd't need the quirk table in any case.
>>
>> It's obvious that it is in this case - the package is the wrong size.
>
> Then Tianyu should be able to come up with a better fix relatively easily. :-)
>
Hi Rafael&Matthew:
I think we can evaluate _BIX before setting ACPI_BATTERY_XINFO_PRESENT
flag. CA routine(acpi_ns_check_package) will check the package size and
return error code when there is wrong size package.
Something like this.
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index fbf1ace..e98fa83 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -770,7 +770,7 @@ static int acpi_battery_add(struct acpi_device *device)
device->driver_data = battery;
mutex_init(&battery->lock);
mutex_init(&battery->sysfs_lock);
- if (acpi_has_method(battery->device->handle, "_BIX"))
+ if (acpi_evaluate_object(device->handle, "_BIX", NULL, &buffer);)
set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags);
result = acpi_battery_update(battery);
if (result)
--
Best Regards
Tianyu Lan
On Wed, Jan 15, 2014 at 10:42:31PM +0800, Lan Tianyu wrote:
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index fbf1ace..e98fa83 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -770,7 +770,7 @@ static int acpi_battery_add(struct acpi_device *device)
> device->driver_data = battery;
> mutex_init(&battery->lock);
> mutex_init(&battery->sysfs_lock);
> - if (acpi_has_method(battery->device->handle, "_BIX"))
> + if (acpi_evaluate_object(device->handle, "_BIX", NULL, &buffer);)
> set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags);
Doesn't acpi_evaluate_object() return 0 on success? I think:
if (ACPI_SUCESS(acpi_evaluate_object(device->handle, "_BIX", NULL,
&buffer))
But maybe we should check for existence first and give an FW_BUG message
to indicate an invalid _BIX?
--
Matthew Garrett | [email protected]
On 01/15/2014 10:47 PM, Matthew Garrett wrote:
> On Wed, Jan 15, 2014 at 10:42:31PM +0800, Lan Tianyu wrote:
>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>> index fbf1ace..e98fa83 100644
>> --- a/drivers/acpi/battery.c
>> +++ b/drivers/acpi/battery.c
>> @@ -770,7 +770,7 @@ static int acpi_battery_add(struct acpi_device *device)
>> device->driver_data = battery;
>> mutex_init(&battery->lock);
>> mutex_init(&battery->sysfs_lock);
>> - if (acpi_has_method(battery->device->handle, "_BIX"))
>> + if (acpi_evaluate_object(device->handle, "_BIX", NULL, &buffer);)
>> set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags);
>
> Doesn't acpi_evaluate_object() return 0 on success? I think:
>
> if (ACPI_SUCESS(acpi_evaluate_object(device->handle, "_BIX", NULL,
> &buffer))
>
Yes, Sorry for oops.
> But maybe we should check for existence first and give an FW_BUG message
> to indicate an invalid _BIX?
Yes, this is a good idea.
Another point, the acpi_evaluate_object should return different error
code for these two cases(no _BIX and wrong size.). I wonder whether we
can use the error code to determine it belong which case?
>
--
Best Regards
Tianyu Lan
If an object does not exist, AE_NOT_FOUND is returned by evaluate_object.
If the package is empty or has insufficient elements, something like AE_AML_OPERAND_VALUE is returned.
> -----Original Message-----
> From: Lan, Tianyu
> Sent: Wednesday, January 15, 2014 7:07 AM
> To: Matthew Garrett
> Cc: Rafael J. Wysocki; Dmitry Torokhov; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; Zheng, Lv; Moore, Robert
> Subject: Re: [PATCH V2] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS
>
> On 01/15/2014 10:47 PM, Matthew Garrett wrote:
> > On Wed, Jan 15, 2014 at 10:42:31PM +0800, Lan Tianyu wrote:
> >> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c index
> >> fbf1ace..e98fa83 100644
> >> --- a/drivers/acpi/battery.c
> >> +++ b/drivers/acpi/battery.c
> >> @@ -770,7 +770,7 @@ static int acpi_battery_add(struct acpi_device
> *device)
> >> device->driver_data = battery;
> >> mutex_init(&battery->lock);
> >> mutex_init(&battery->sysfs_lock);
> >> - if (acpi_has_method(battery->device->handle, "_BIX"))
> >> + if (acpi_evaluate_object(device->handle, "_BIX", NULL,
> >> + &buffer);)
> >> set_bit(ACPI_BATTERY_XINFO_PRESENT,
> >> &battery->flags);
> >
> > Doesn't acpi_evaluate_object() return 0 on success? I think:
> >
> > if (ACPI_SUCESS(acpi_evaluate_object(device->handle, "_BIX", NULL,
> > &buffer))
> >
>
> Yes, Sorry for oops.
>
> > But maybe we should check for existence first and give an FW_BUG
> > message to indicate an invalid _BIX?
>
> Yes, this is a good idea.
>
> Another point, the acpi_evaluate_object should return different error code
> for these two cases(no _BIX and wrong size.). I wonder whether we can use
> the error code to determine it belong which case?
> >
>
>
> --
> Best Regards
> Tianyu Lan
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?