| Message ID | 1389019837-13619-1-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
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.
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. > >
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.
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.
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 -- 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
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. :-)
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?" -- 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
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); }
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(-)