Bug 195689

Summary: HP x2 210: battery not detected
Product: ACPI Reporter: Dainius Masiliūnas (pastas4)
Component: Power-BatteryAssignee: Zhang Rui (rui.zhang)
Status: RESOLVED CODE_FIX    
Severity: normal CC: hedley.robertson, neko, rui.zhang, yu.c.chen
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 4.10 Subsystem:
Regression: No Bisected commit-id:
Attachments: dmesg
acpidump
dsdt.dsl
dmesg (linux-sunxi)
config (linux-sunxi)
sys grep output
dmesg (initcall_debug)
skip acpi dependency check for INT33F5
re-enumerate acpi devices on dollar cove ti probe

Description Dainius Masiliūnas 2017-05-08 20:08:42 UTC
Created attachment 256281 [details]
dmesg

On an HP x2 210 G2 detachable tablet, the battery status is not displayed; /proc/acpi does not have a battery entry altogether.

This happens at least on kernel 4.10 (Ubuntu Zesty) as well as previous kernels and variants.

dmesg and acpidump attached.
Comment 1 Dainius Masiliūnas 2017-05-08 20:09:33 UTC
Created attachment 256283 [details]
acpidump
Comment 2 Dainius Masiliūnas 2017-05-22 18:49:54 UTC
Created attachment 256669 [details]
dsdt.dsl

Decompiled DSDT table. iasl did complain about it referring to other tables, though. Let me know if you need anything extra.
Comment 3 Dainius Masiliūnas 2017-06-05 19:36:05 UTC
I tried the current git master, no changes. I also tried the sunxi kernel with Cherry Trail additions (https://github.com/jwrdegoede/linux-sunxi), no change either.

Looking at the DSDT, looks like issue #193891 might be relevant to this. I am going to try out the topic branches listed there next. Also, the DSDT tables mention TBQ24296, which does not seem to be mentioned in the mainline kernel, but is set as an alias of the bq24192_charger in some of the Android sources (like https://github.com/ZenfoneArea/android_kernel_asus_zenfone5/blob/master/linux/modules/drivers/power/bq24192_charger.c). So it could be a case of missing IDs.
Comment 4 Zhang Rui 2017-06-12 03:18:53 UTC
it is probably related with the following errors in dmesg
   50.743697] ------------[ cut here ]------------
[   50.743712] WARNING: CPU: 2 PID: 1261 at /build/linux-Fk60NP/linux-4.10.0/drivers/i2c/busses/i2c-designware-baytrail.c:106 baytrail_i2c_acquire+0x13c/0x1f0 [i2c_designware_platform]
[   50.743714] Modules linked in: snd_seq_midi snd_seq_midi_event btqca snd_rawmidi snd_seq dw_dmac btintel dw_dmac_core snd_seq_device bluetooth i2c_designware_platform(+) rfkill_gpio snd_timer i2c_designware_core pwm_lpss_platform mei_txe intel_hid shpchp lpc_ich mei pwm_lpss 8250_dw processor_thermal_device snd spi_pxa2xx_platform intel_vbtn soc_button_array intel_soc_dts_iosf soundcore tpm_crb sparse_keymap int3400_thermal int3406_thermal dptf_power mac_hid int3403_thermal acpi_thermal_rel int340x_thermal_zone acpi_pad parport_pc ppdev lp parport ip_tables x_tables autofs4 aufs nls_utf8 isofs udf crc_itu_t btrfs xor raid6_pq nls_iso8859_1 dm_mirror dm_region_hash dm_log hid_sensor_custom hid_sensor_hub intel_ishtp_hid hid_generic uas usb_storage usbhid mmc_block i915 i2c_algo_bit drm_kms_helper
[   50.743789]  syscopyarea sysfillrect sysimgblt intel_ish_ipc fb_sys_fops drm intel_ishtp wmi i2c_hid fjes video hid sdhci_acpi sdhci
[   50.743809] CPU: 2 PID: 1261 Comm: systemd-udevd Not tainted 4.10.0-19-generic #21-Ubuntu
[   50.743811] Hardware name: HP HP x2 210 G2/82F4, BIOS F.13 12/20/2016
[   50.743812] Call Trace:
[   50.743823]  dump_stack+0x63/0x81
[   50.743828]  __warn+0xcb/0xf0
[   50.743832]  warn_slowpath_null+0x1d/0x20
[   50.743836]  baytrail_i2c_acquire+0x13c/0x1f0 [i2c_designware_platform]
[   50.743841]  i2c_dw_acquire_lock+0x21/0x50 [i2c_designware_core]
[   50.743845]  i2c_dw_init+0x1e/0x610 [i2c_designware_core]
[   50.743848]  i2c_dw_probe+0x39/0x8c0 [i2c_designware_core]
[   50.743854]  ? pm_runtime_forbid+0x5b/0x60
[   50.743858]  dw_i2c_plat_probe+0x299/0x530 [i2c_designware_platform]
[   50.743861]  platform_drv_probe+0x3b/0xa0
[   50.743865]  ? acpi_lpss_activate+0x4b/0x53
[   50.743870]  driver_probe_device+0x2bb/0x460
[   50.743874]  __driver_attach+0xdf/0xf0
[   50.743877]  ? driver_probe_device+0x460/0x460
[   50.743880]  bus_for_each_dev+0x6c/0xc0
[   50.743884]  driver_attach+0x1e/0x20
[   50.743887]  bus_add_driver+0x170/0x270
[   50.743889]  driver_register+0x60/0xe0
[   50.743892]  ? 0xffffffffc0647000
[   50.743895]  __platform_driver_register+0x36/0x40
[   50.743899]  dw_i2c_init_driver+0x17/0x1000 [i2c_designware_platform]
[   50.743903]  do_one_initcall+0x52/0x1b0
[   50.743909]  ? kmem_cache_alloc_trace+0x142/0x190
[   50.743913]  do_init_module+0x5f/0x200
[   50.743917]  load_module+0x190b/0x1c70
[   50.743920]  ? __symbol_put+0x60/0x60
[   50.743925]  ? ima_post_read_file+0x7e/0xa0
[   50.743928]  ? security_kernel_post_read_file+0x6b/0x80
[   50.743932]  SYSC_finit_module+0xdf/0x110
[   50.743936]  SyS_finit_module+0xe/0x10
[   50.743939]  do_syscall_64+0x5b/0xc0
[   50.743944]  entry_SYSCALL64_slow_path+0x25/0x25
[   50.743946] RIP: 0033:0x7f29f0697df9
[   50.743948] RSP: 002b:00007ffc47faa4d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[   50.743951] RAX: ffffffffffffffda RBX: 0000563f56e1b940 RCX: 00007f29f0697df9
[   50.743953] RDX: 0000000000000000 RSI: 00007f29f0fbbe23 RDI: 000000000000000f
[   50.743954] RBP: 00007f29f0fbbe23 R08: 0000000000000000 R09: 0000000000000000
[   50.743956] R10: 000000000000000f R11: 0000000000000246 R12: 0000000000000000
[   50.743958] R13: 0000563f56e1ba80 R14: 0000000000020000 R15: 0000563f56704c3c
[   50.743960] ---[ end trace a657130b729ad831 ]---
Comment 5 Zhang Rui 2017-06-12 03:21:01 UTC
let's see if the problem still exists after the above problem fixed.
Comment 6 Dainius Masiliūnas 2017-06-12 07:16:37 UTC
No, it's not, that's an unrelated issue (https://bugzilla.kernel.org/show_bug.cgi?id=155241). The fix is already in de Goede's sunxi kernel. I can upload the dmesg from it if you'd like.
Comment 7 Zhang Rui 2017-06-12 11:21:53 UTC
ok, reassign back.

please attach the dmesg output with the I2C problem fixed.
Comment 8 Dainius Masiliūnas 2017-06-12 15:33:34 UTC
Created attachment 256959 [details]
dmesg (linux-sunxi)
Comment 9 Dainius Masiliūnas 2017-06-12 15:36:39 UTC
Created attachment 256961 [details]
config (linux-sunxi)

Attached the new dmesg from de Goede's linux-sunxi tree, as well as the .config I'm using for it.
Comment 10 Zhang Rui 2017-06-20 02:24:52 UTC
please attach the output of "grep . /sys/class/power_supply/*/*"
Comment 11 Zhang Rui 2017-06-20 02:26:54 UTC
there are also some ACPI battery devices in the DSDT, please attach the output of
"grep . /sys/bus/acpi/devices/PNP0C0A\:*/*"
Comment 12 Zhang Rui 2017-06-20 02:30:28 UTC
please also attach the output of "grep . /sys/bus/i2c/devices/*/firmware_node/*"
Comment 13 Dainius Masiliūnas 2017-06-20 07:23:27 UTC
Created attachment 257083 [details]
sys grep output

Attached.
Comment 14 Zhang Rui 2017-07-01 06:55:37 UTC
/sys/bus/acpi/devices/PNP0C0A:01/hid:PNP0C0A
/sys/bus/acpi/devices/PNP0C0A:01/modalias:acpi:PNP0C0A:
/sys/bus/acpi/devices/PNP0C0A:01/path:\_SB_.PCI0.I2C3.BATC
/sys/bus/acpi/devices/PNP0C0A:01/status:31
/sys/bus/acpi/devices/PNP0C0A:01/uevent:MODALIAS=acpi:PNP0C0A:
/sys/bus/acpi/devices/PNP0C0A:01/uid:1

so there is one ACPI battery on this platform, don't know why it is not registered to power_supply class.
please check if the ACPI battery driver is loaded or not.
please attach the dmesg output after boot, with boot option "initcall_debug"
Comment 15 Dainius Masiliūnas 2017-07-01 09:13:40 UTC
Created attachment 257277 [details]
dmesg (initcall_debug)

The ACPI battery driver is built-in right now. Should I try building it as a module?

dmesg with initcall_debug attached.
Comment 16 Alexandrov Stanislav 2017-07-01 21:07:27 UTC
Created attachment 257283 [details]
skip acpi dependency check for INT33F5

This battery depends on I2C7.PMI2 which is dollar cove ti pmic (snd9039), currently there is no driver in upstream for this pmic, and this driver is responsible for installing OperationRegion which acpi should use. 
https://bugzilla.kernel.org/show_bug.cgi?id=193891

But, after some testing on my machine with patch attached here, battery is working fine without pmic driver. Inside dsdt table there is another OperationRegion on I2C3, which is looks like battery itself. And almost all data is retrieved from this region.

But, even with patches for dcove ti opregion and mfd driver battery not detected. looks like dcove ti driver is loaded too late after acpi scan and battery stays unititialized.
Comment 17 Dainius Masiliūnas 2017-07-01 21:42:36 UTC
Hm, I can confirm that this patch makes the battery appear. I'll still need to check whether the battery readings are correct, though.
Comment 18 Zhang Rui 2017-07-02 15:40:42 UTC
(In reply to Alexandrov Stanislav from comment #16)
> Created attachment 257283 [details]
> skip acpi dependency check for INT33F5
> 
> This battery depends on I2C7.PMI2 which is dollar cove ti pmic (snd9039),
> currently there is no driver in upstream for this pmic, and this driver is
> responsible for installing OperationRegion which acpi should use. 
> https://bugzilla.kernel.org/show_bug.cgi?id=193891
> 
Cool. thanks for the information.

> But, after some testing on my machine with patch attached here, battery is
> working fine without pmic driver. Inside dsdt table there is another
> OperationRegion on I2C3, which is looks like battery itself. And almost all
> data is retrieved from this region.
> 
I don't know why PMI2 is added into _DEP in the first place.
Do you mean everything works well if we just ignore this dependency?

> But, even with patches for dcove ti opregion and mfd driver battery not
> detected. looks like dcove ti driver is loaded too late after acpi scan and
> battery stays unititialized.

Why? the ACPI battery driver will not be loaded (deferred probe) until dcov ti pmic driver loaded, this is how _DEP is supposed to work in Linux, no?
Comment 19 Alexandrov Stanislav 2017-07-02 17:44:00 UTC
(In reply to Zhang Rui from comment #18)

> I don't know why PMI2 is added into _DEP in the first place.
> Do you mean everything works well if we just ignore this dependency?
> 

I'm using my notebook with this patch for a week or so and i didn't noticed any issues with battery readings. But however there is hardcoded values in _BIX:

  "SR Real Battery",
  123456789",
  "LION",
  "Intel SR 1"

which is incorrect of course. Looks like correct data about battery vendor must be received through wmi request, but this is not acpi issue.

> > But, even with patches for dcove ti opregion and mfd driver battery not
> > detected. looks like dcove ti driver is loaded too late after acpi scan and
> > battery stays unititialized.
> 
> Why? the ACPI battery driver will not be loaded (deferred probe) until dcov
> ti pmic driver loaded, this is how _DEP is supposed to work in Linux, no?

Well, i'm not totally sure what exactly going on here, so this is just assumption.

Battery driver's callback acpi_battery_add is not executed at all without patch, so no deferred probe.

Inside acpi/bus.c there is function which compares device with driver data:

static int acpi_bus_match(struct device *dev, struct device_driver *drv)
{
        struct acpi_device *acpi_dev = to_acpi_device(dev);
        struct acpi_driver *acpi_drv = to_acpi_driver(drv);

        return acpi_dev->flags.match_driver
                && !acpi_match_device_ids(acpi_dev, acpi_drv->ids);
}

Problem is when real battery acpi_dev comapred with driver data acpi_dev->flags.match_driver is set to 0(and other flags too), so even if acpi_match_device_ids succeeded acpi_bus_match returns 0, telling bus subsystem that there is no match for this driver at all. 

Only when i manually set match_driver flag here to 1, acpi_battery_add function is executed, and deferred due to dep_unmet is not 0.

Original code for dependency checking is originated from this bug report: https://bugzilla.kernel.org/show_bug.cgi?id=69011

Maybe dcove ti pmic driver is not complete, or there is some issues in dependency resolving, i'm don't know yet. This need further investigation.
Comment 20 Alexandrov Stanislav 2017-07-03 05:27:05 UTC
Created attachment 257301 [details]
re-enumerate acpi devices on dollar cove ti probe

well, looks like i've found more correct way to resolve dependencies here with dollar cove ti driver.

first, you will need two patches:

1. dollar cove ti mfd driver: 
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/patch/?id=654e90905946d94ed066e1d051f47dfba19d0142

2. dollar cove ti opregion driver:
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/patch/?id=9bd9475ea11430c2b51a73c1141c450f785add20

3. and attached patch for re-enumeration

So, when dollar cove ti opregion driver is loaded we just executing acpi_walk_dep_device_list and battery becomes operational.
Comment 21 Zhang Rui 2017-07-03 05:42:24 UTC
(In reply to Alexandrov Stanislav from comment #20)
> Created attachment 257301 [details]
> re-enumerate acpi devices on dollar cove ti probe
> 
> well, looks like i've found more correct way to resolve dependencies here
> with dollar cove ti driver.
> 
> first, you will need two patches:
> 
> 1. dollar cove ti mfd driver: 
> https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/patch/
> ?id=654e90905946d94ed066e1d051f47dfba19d0142
> 
> 2. dollar cove ti opregion driver:
> https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/patch/
> ?id=9bd9475ea11430c2b51a73c1141c450f785add20
> 
> 3. and attached patch for re-enumeration
> 
> So, when dollar cove ti opregion driver is loaded we just executing
> acpi_walk_dep_device_list and battery becomes operational.

yes, the master needs to invoke acpi_walk_dep_device_list explicitly to notify the slaves after it's ready.
Please submit the patch in comment #20.

So I think all the problem is clear, and all we need is the proper dcove driver support.

Bug marked as Resovled.
Comment 22 Dainius Masiliūnas 2017-08-10 11:00:27 UTC
I just tried the attached patch to the dollar cove PMIC and indeed it works too.

However, it's still not present in tiwai/sound.git Dollar Cove branch yet, I'm not sure if it has been submitted?
Comment 23 Hedley Robertson 2020-02-22 22:06:12 UTC
Is this fix applied to current kernels?  I recently installed a 5.3.0 kernel ( ubuntu 19.10 ) and there is still no battery.