All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS
@ 2014-01-06 14:50 Lan Tianyu
  2014-01-06 17:59 ` Dmitry Torokhov
  0 siblings, 1 reply; 13+ messages in thread
From: Lan Tianyu @ 2014-01-06 14:50 UTC (permalink / raw)
  To: lenb, rjw
  Cc: Lan Tianyu, linux-acpi, linux-kernel, fcr, l, dmitry.torokhov, 3.8+

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

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

* Re: [PATCH V2] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS
  2014-01-06 14:50 [PATCH V2] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS Lan Tianyu
@ 2014-01-06 17:59 ` Dmitry Torokhov
  2014-01-06 22:25   ` Rafael J. Wysocki
  0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Torokhov @ 2014-01-06 17:59 UTC (permalink / raw)
  To: Lan Tianyu; +Cc: lenb, rjw, linux-acpi, linux-kernel, fcr, l, 3.8+

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

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

* Re: [PATCH V2] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS
  2014-01-06 17:59 ` Dmitry Torokhov
@ 2014-01-06 22:25   ` Rafael J. Wysocki
  2014-01-14 16:06     ` Matthew Garrett
  0 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2014-01-06 22:25 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Lan Tianyu, lenb, linux-acpi, linux-kernel, fcr, l, 3.8+

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.

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

* Re: [PATCH V2] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS
  2014-01-06 22:25   ` Rafael J. Wysocki
@ 2014-01-14 16:06     ` Matthew Garrett
  2014-01-14 21:37       ` Rafael J. Wysocki
  0 siblings, 1 reply; 13+ messages in thread
From: Matthew Garrett @ 2014-01-14 16:06 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Dmitry Torokhov, Lan Tianyu, lenb, linux-acpi, linux-kernel, fcr,
	l, 3.8+

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

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

* Re: [PATCH V2] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS
  2014-01-14 21:37       ` Rafael J. Wysocki
@ 2014-01-14 21:24         ` Matthew Garrett
  2014-01-14 22:17           ` Rafael J. Wysocki
  2014-01-15  6:17         ` Robert Hancock
  1 sibling, 1 reply; 13+ messages in thread
From: Matthew Garrett @ 2014-01-14 21:24 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Lan Tianyu, Dmitry Torokhov, lenb, linux-acpi, linux-kernel, fcr, l

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

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

* Re: [PATCH V2] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS
  2014-01-14 16:06     ` Matthew Garrett
@ 2014-01-14 21:37       ` Rafael J. Wysocki
  2014-01-14 21:24         ` Matthew Garrett
  2014-01-15  6:17         ` Robert Hancock
  0 siblings, 2 replies; 13+ messages in thread
From: Rafael J. Wysocki @ 2014-01-14 21:37 UTC (permalink / raw)
  To: Matthew Garrett, Lan Tianyu
  Cc: Dmitry Torokhov, lenb, linux-acpi, linux-kernel, fcr, l

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

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

* Re: [PATCH V2] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS
  2014-01-14 21:24         ` Matthew Garrett
@ 2014-01-14 22:17           ` Rafael J. Wysocki
  2014-01-15 14:42             ` Lan Tianyu
  0 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2014-01-14 22:17 UTC (permalink / raw)
  To: Matthew Garrett
  Cc: Lan Tianyu, Dmitry Torokhov, lenb, linux-acpi, linux-kernel, fcr, l

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.

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

* Re: [PATCH V2] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS
  2014-01-14 21:37       ` Rafael J. Wysocki
  2014-01-14 21:24         ` Matthew Garrett
@ 2014-01-15  6:17         ` Robert Hancock
  1 sibling, 0 replies; 13+ messages in thread
From: Robert Hancock @ 2014-01-15  6:17 UTC (permalink / raw)
  To: Rafael J. Wysocki, Matthew Garrett, Lan Tianyu
  Cc: Dmitry Torokhov, lenb, linux-acpi, linux-kernel, fcr, l

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


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

* Re: [PATCH V2] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS
  2014-01-14 22:17           ` Rafael J. Wysocki
@ 2014-01-15 14:42             ` Lan Tianyu
  2014-01-15 14:47               ` Matthew Garrett
  0 siblings, 1 reply; 13+ messages in thread
From: Lan Tianyu @ 2014-01-15 14:42 UTC (permalink / raw)
  To: Rafael J. Wysocki, Matthew Garrett
  Cc: Dmitry Torokhov, lenb, linux-acpi, linux-kernel, fcr, l, Zheng,
	Lv, robert.moore

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

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

* Re: [PATCH V2] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS
  2014-01-15 14:42             ` Lan Tianyu
@ 2014-01-15 14:47               ` Matthew Garrett
  2014-01-15 15:06                 ` Lan Tianyu
  0 siblings, 1 reply; 13+ messages in thread
From: Matthew Garrett @ 2014-01-15 14:47 UTC (permalink / raw)
  To: Lan Tianyu
  Cc: Rafael J. Wysocki, Dmitry Torokhov, lenb, linux-acpi,
	linux-kernel, fcr, l, Zheng, Lv, robert.moore

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

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

* Re: [PATCH V2] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS
  2014-01-15 14:47               ` Matthew Garrett
@ 2014-01-15 15:06                 ` Lan Tianyu
  2014-01-15 16:48                     ` Moore, Robert
  0 siblings, 1 reply; 13+ messages in thread
From: Lan Tianyu @ 2014-01-15 15:06 UTC (permalink / raw)
  To: Matthew Garrett
  Cc: Rafael J. Wysocki, Dmitry Torokhov, lenb, linux-acpi,
	linux-kernel, fcr, l, Zheng, Lv, robert.moore

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

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

* RE: [PATCH V2] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS
  2014-01-15 15:06                 ` Lan Tianyu
@ 2014-01-15 16:48                     ` Moore, Robert
  0 siblings, 0 replies; 13+ messages in thread
From: Moore, Robert @ 2014-01-15 16:48 UTC (permalink / raw)
  To: Lan, Tianyu, Matthew Garrett
  Cc: Rafael J. Wysocki, Dmitry Torokhov, lenb, linux-acpi,
	linux-kernel, fcr, l, Zheng, Lv

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

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

* RE: [PATCH V2] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS
@ 2014-01-15 16:48                     ` Moore, Robert
  0 siblings, 0 replies; 13+ messages in thread
From: Moore, Robert @ 2014-01-15 16:48 UTC (permalink / raw)
  To: Lan, Tianyu, Matthew Garrett
  Cc: Rafael J. Wysocki, Dmitry Torokhov, lenb, linux-acpi,
	linux-kernel, fcr, l, Zheng, Lv

[-- 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¥

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

end of thread, other threads:[~2014-01-15 16:49 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-06 14:50 [PATCH V2] ACPI/Battery: Add a _BIX quirk for NEC LZ750/LS Lan Tianyu
2014-01-06 17:59 ` Dmitry Torokhov
2014-01-06 22:25   ` Rafael J. Wysocki
2014-01-14 16:06     ` Matthew Garrett
2014-01-14 21:37       ` Rafael J. Wysocki
2014-01-14 21:24         ` Matthew Garrett
2014-01-14 22:17           ` Rafael J. Wysocki
2014-01-15 14:42             ` Lan Tianyu
2014-01-15 14:47               ` Matthew Garrett
2014-01-15 15:06                 ` Lan Tianyu
2014-01-15 16:48                   ` Moore, Robert
2014-01-15 16:48                     ` Moore, Robert
2014-01-15  6:17         ` Robert Hancock

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.