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

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

[Lan Tianyu]

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 <fcr@adinet.com.uy>
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=67351
Cc: 3.8+ <stable@vger.kernel.org>
Signed-off-by: Lan Tianyu <tianyu.lan@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 <astarikovskiy@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

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

[Dmitry Torokhov]

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 <fcr@adinet.com.uy>
> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=67351
> Cc: 3.8+ <stable@vger.kernel.org>
> Signed-off-by: Lan Tianyu <tianyu.lan@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 <astarikovskiy@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"),
> +		},

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.torokhov@gmail.com>

Thanks.

-- 
Dmitry

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

[Rafael J. Wysocki]

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 <fcr@adinet.com.uy>
> > Reference: https://bugzilla.kernel.org/show_bug.cgi?id=67351
> > Cc: 3.8+ <stable@vger.kernel.org>
> > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>

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 <astarikovskiy@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"),
> > +		},
> 
> 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.torokhov@gmail.com>
> 
> Thanks.
> 
> 

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

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

[Matthew Garrett]

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 | mjg59@srcf.ucam.org

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

[Rafael J. Wysocki]

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

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

[Matthew Garrett]

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 | mjg59@srcf.ucam.org

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

[Rafael J. Wysocki]

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.

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

[Lan Tianyu]

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

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

[Matthew Garrett]

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 | mjg59@srcf.ucam.org

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

[Lan Tianyu]

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

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

[Moore, Robert]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2030 bytes --]

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; lenb@kernel.org; linux-
> acpi@vger.kernel.org; linux-kernel@vger.kernel.org; fcr@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);)
> >>                  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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Robert Hancock]

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?"