Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS

5 views
Skip to first unread message

Lan Tianyu

unread,
Jan 2, 2014, 2:50:01 AM1/2/14
to
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 <f...@adinet.com.uy>
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=67351
Cc: 3.8+ <sta...@vger.kernel.org>
Signed-off-by: Lan Tianyu <tiany...@intel.com>
---
drivers/acpi/battery.c | 28 +++++++++++++++++++++++++++-
1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index fbf1ace..3d64a87 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -62,6 +62,7 @@ MODULE_AUTHOR("Alexey Starikovskiy <astari...@suse.de>");
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");
@@ -416,7 +417,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));
@@ -754,6 +760,24 @@ static int battery_notify(struct notifier_block *nb,
return 0;
}

+static int battery_bix_package_quirk(const struct dmi_system_id *id)
+{
+ battery_bix_broken_package = 1;
+ return 0;
+}
+
+static struct dmi_system_id bat_dmi_table[] = {
+ {
+ .callback = battery_bix_package_quirk,
+ .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;
@@ -846,6 +870,8 @@ static void __init acpi_battery_init_async(void *unused, async_cookie_t cookie)
{
if (acpi_disabled)
return;
+
+ dmi_check_system(bat_dmi_table);
acpi_bus_register_driver(&acpi_battery_driver);
}

--
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Leandro Dorileo

unread,
Jan 3, 2014, 2:10:02 PM1/3/14
to
Do you really need this callback? Why don't you just do:

if (dmi_check_system(bat_dmi_table))
battery_bix_broken_package = 1;


> +static struct dmi_system_id bat_dmi_table[] = {
> + {
> + .callback = battery_bix_package_quirk,
> + .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;
> @@ -846,6 +870,8 @@ static void __init acpi_battery_init_async(void *unused, async_cookie_t cookie)
> {
> if (acpi_disabled)
> return;
> +
> + dmi_check_system(bat_dmi_table);
> acpi_bus_register_driver(&acpi_battery_driver);
> }
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majo...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Leandro Dorileo

Dmitry Torokhov

unread,
Jan 3, 2014, 6:50:01 PM1/3/14
to
Callback would be useful if we were to print the DMI data of the match,
otherwise not so much.

--
Dmitry

Lan Tianyu

unread,
Jan 6, 2014, 9:10:02 AM1/6/14
to
Hi Leandro & Dmitry:
Thanks for your review. I select to callback mode as it's convenient to
add a new quirk for other machines if need. This Just likes what we have
done for the other ACPI drivers(E,G ac, processor_idle, video, thermal
and so on.).

--
Best Regards
Tianyu Lan

Rafael J. Wysocki

unread,
Jan 6, 2014, 9:20:02 AM1/6/14
to
We can switch to callbacks when we actually need them. Please keep things
as simple as reasonably possible.

Thanks!

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Lan Tianyu

unread,
Jan 6, 2014, 9:30:02 AM1/6/14
to
Ok. I will update it.

>
> Thanks!
>


--
Best Regards
Tianyu Lan

Lan Tianyu

unread,
Jan 6, 2014, 10:00:02 AM1/6/14
to
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 <f...@adinet.com.uy>
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=67351
Cc: 3.8+ <sta...@vger.kernel.org>
Signed-off-by: Lan Tianyu <tiany...@intel.com>
---
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 <astari...@suse.de>");
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

Dmitry Torokhov

unread,
Jan 6, 2014, 1:00:02 PM1/6/14
to
Hi Lan,
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 <dmitry....@gmail.com>

Thanks.

--
Dmitry

Rafael J. Wysocki

unread,
Jan 6, 2014, 5:20:02 PM1/6/14
to
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 <f...@adinet.com.uy>
> > Reference: https://bugzilla.kernel.org/show_bug.cgi?id=67351
> > Cc: 3.8+ <sta...@vger.kernel.org>
> > Signed-off-by: Lan Tianyu <tiany...@intel.com>

Queued up as a fix for 3.13 (I fixed up the indentation).

Thanks!
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Matthew Garrett

unread,
Jan 14, 2014, 11:10:01 AM1/14/14
to
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 | mj...@srcf.ucam.org

Matthew Garrett

unread,
Jan 14, 2014, 11:10:02 AM1/14/14
to
On Thu, Jan 02, 2014 at 03:37:57PM +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.

Why don't we just ignore an invalid _BIX and fall back to _BIF? That
seems more reasonable than setting a precedent for fixing up invalid
data.

--
Matthew Garrett | mj...@srcf.ucam.org

Rafael J. Wysocki

unread,
Jan 14, 2014, 4:30:02 PM1/14/14
to
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

Matthew Garrett

unread,
Jan 14, 2014, 4:30:03 PM1/14/14
to
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 | mj...@srcf.ucam.org

Rafael J. Wysocki

unread,
Jan 14, 2014, 5:10:02 PM1/14/14
to
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.

Robert Hancock

unread,
Jan 15, 2014, 1:20:01 AM1/15/14
to
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?"

Lan Tianyu

unread,
Jan 15, 2014, 9:50:02 AM1/15/14
to
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

Matthew Garrett

unread,
Jan 15, 2014, 9:50:02 AM1/15/14
to
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 | mj...@srcf.ucam.org

Lan Tianyu

unread,
Jan 15, 2014, 10:10:01 AM1/15/14
to
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

Moore, Robert

unread,
Jan 15, 2014, 11:50:02 AM1/15/14
to
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; le...@kernel.org; linux-
> ac...@vger.kernel.org; linux-...@vger.kernel.org; f...@adinet.com.uy;
> l...@dorileo.org; 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);)
0 new messages