| Message ID | 20191115052710.46880-1-yongxin.liu@windriver.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | Revert "platform/x86: wmi: Destroy on cleanup rather than unregister" | expand |
Add more logs for this issue.
When loading wmi driver module,
device class 'wmi_bus': registering
bus: 'wmi': registered
bus: 'platform': add driver acpi-wmi
bus: 'platform': driver_probe_device: matched device PNP0C14:00 with driver acpi-wmi
bus: 'platform': really_probe: probing driver acpi-wmi with device PNP0C14:00
acpi-wmi PNP0C14:00: no default pinctrl state
device: 'wakeup28': device_add
device: 'wmi_bus-PNP0C14:00': device_add
PM: Adding info for No Bus:wmi_bus-PNP0C14:00
device: '86CCFD48-205E-4A77-9C48-2021CBEDE341': device_add
bus: 'wmi': add device 86CCFD48-205E-4A77-9C48-2021CBEDE341
PM: Adding info for wmi:86CCFD48-205E-4A77-9C48-2021CBEDE341
driver: 'acpi-wmi': driver_bound: bound to device 'PNP0C14:00'
bus: 'platform': really_probe: bound device PNP0C14:00 to driver acpi-wmi
bus: 'platform': driver_probe_device: matched device PNP0C14:01 with driver acpi-wmi
bus: 'platform': really_probe: probing driver acpi-wmi with device PNP0C14:01
acpi-wmi PNP0C14:01: no default pinctrl state
device: 'wakeup29': device_add
device: 'wmi_bus-PNP0C14:01': device_add
PM: Adding info for No Bus:wmi_bus-PNP0C14:01
device: '2BC49DEF-7B15-4F05-8BB7-EE37B9547C0B': device_add
bus: 'wmi': add device 2BC49DEF-7B15-4F05-8BB7-EE37B9547C0B
PM: Adding info for wmi:2BC49DEF-7B15-4F05-8BB7-EE37B9547C0B
device: 'A6FEA33E-DABF-46F5-BFC8-460D961BEC9F': device_add
bus: 'wmi': add device A6FEA33E-DABF-46F5-BFC8-460D961BEC9F
PM: Adding info for wmi:A6FEA33E-DABF-46F5-BFC8-460D961BEC9F
device: '05901221-D566-11D1-B2F0-00A0C9062910': device_add
bus: 'wmi': add device 05901221-D566-11D1-B2F0-00A0C9062910
PM: Adding info for wmi:05901221-D566-11D1-B2F0-00A0C9062910
driver: 'acpi-wmi': driver_bound: bound to device 'PNP0C14:01'
bus: 'platform': really_probe: bound device PNP0C14:01 to driver acpi-wmi
bus: 'platform': driver_probe_device: matched device PNP0C14:02 with driver acpi-wmi
bus: 'platform': really_probe: probing driver acpi-wmi with device PNP0C14:02
acpi-wmi PNP0C14:02: no default pinctrl state
device: 'wakeup30': device_add
device: 'wmi_bus-PNP0C14:02': device_add
PM: Adding info for No Bus:wmi_bus-PNP0C14:02
acpi PNP0C14:02: duplicate WMI GUID 05901221-D566-11D1-B2F0-00A0C9062910 (first instance was on PNP0C14:01)
device: '1F13AB7F-6220-4210-8F8E-8BB5E71EE969': device_add
bus: 'wmi': add device 1F13AB7F-6220-4210-8F8E-8BB5E71EE969
PM: Adding info for wmi:1F13AB7F-6220-4210-8F8E-8BB5E71EE969
driver: 'acpi-wmi': driver_bound: bound to device 'PNP0C14:02'
bus: 'platform': really_probe: bound device PNP0C14:02 to driver acpi-wmi
When unloading wmi driver module, without this patch, there is calltrace.
bus: 'platform': remove driver acpi-wmi
device: '1F13AB7F-6220-4210-8F8E-8BB5E71EE969': device_unregister
bus: 'wmi': remove device 1F13AB7F-6220-4210-8F8E-8BB5E71EE969
PM: Removing info for wmi:1F13AB7F-6220-4210-8F8E-8BB5E71EE969
device: 'wmi_bus-PNP0C14:00': device_unregister
PM: Removing info for No Bus:wmi_bus-PNP0C14:00
device: 'wakeup29': device_unregister
device: '2BC49DEF-7B15-4F05-8BB7-EE37B9547C0B': device_unregister
bus: 'wmi': remove device 2BC49DEF-7B15-4F05-8BB7-EE37B9547C0B
PM: Removing info for wmi:2BC49DEF-7B15-4F05-8BB7-EE37B9547C0B
device: 'A6FEA33E-DABF-46F5-BFC8-460D961BEC9F': device_unregister
bus: 'wmi': remove device A6FEA33E-DABF-46F5-BFC8-460D961BEC9F
PM: Removing info for wmi:A6FEA33E-DABF-46F5-BFC8-460D961BEC9F
device: '05901221-D566-11D1-B2F0-00A0C9062910': device_unregister
bus: 'wmi': remove device 05901221-D566-11D1-B2F0-00A0C9062910
PM: Removing info for wmi:05901221-D566-11D1-B2F0-00A0C9062910
device: 'wmi_bus-PNP0C14:01': device_unregister
PM: Removing info for No Bus:wmi_bus-PNP0C14:01
device: 'wmi_bus-PNP0C14:01': device_create_release
device: 'wakeup28': device_unregister
device: '86CCFD48-205E-4A77-9C48-2021CBEDE341': device_unregister
------------[ cut here ]------------
sysfs group 'power' not found for kobject '86CCFD48-205E-4A77-9C48-2021CBEDE341'
WARNING: CPU: 8 PID: 636 at fs/sysfs/group.c:280 sysfs_remove_group+0x80/0x90
Modules linked in: wmi(-) snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic i915 snd_hda_intel snd_intel_nhlt intel_rapl_msr snd_hda_codec intel_rapl_common x86_pkg_temp_thermal intel_powerclamp coretemp crct10dif_pclmul crct10dif_common snd_hda_core snd_pcm iTCO_wdt iTCO_vendor_support sch_fq_codel watchdog aesni_intel video thermal idma64 glue_helper efi_pstore snd_timer i2c_i801 crypto_simd backlight acpi_pad efivars openvswitch cryptd fan nsh nf_conncount nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 [last unloaded: wmi_bmof]
CPU: 8 PID: 636 Comm: rmmod Tainted: G W 5.4.0-rc7 #1
Hardware name: Intel Corporation CoffeeLake Client Platform/CoffeeLake S 82 UDIMM RVP, BIOS CNLSFWX1.R00.X151.B01.1807201217 07/20/2018
RIP: 0010:sysfs_remove_group+0x80/0x90
Code: e8 a5 b3 ff ff 5b 41 5c 41 5d 5d c3 48 89 df e8 46 ae ff ff eb c6 49 8b 55 00 48 c7 c7 c8 9b bc 9b 49 8b 34 24 e8 30 7b cd ff <0f> 0b 5b 41 5c 41 5d 5d c3 0f 1f 80 00 00 00 00 0f 1f 44 00 00 48
RSP: 0018:ffffbaacc063bd38 EFLAGS: 00010282
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000001 RSI: ffffffff9a586023 RDI: ffffffff9a586023
RBP: ffffbaacc063bd50 R08: 0000000000000001 R09: 0000000000000000
R10: 00000000ffba64cc R11: 0000000000000000 R12: ffffffff9b8d9d20
R13: ffff9c3843c85000 R14: dead000000000100 R15: ffff9c384facc000
FS: 00007f5a591fa740(0000) GS:ffff9c385ac00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055b3f2513d28 CR3: 0000000441ed6005 CR4: 00000000003606e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
dpm_sysfs_remove+0x58/0x60
device_del+0x77/0x380
? acpi_ut_release_mutex+0x71/0x76
device_unregister+0x41/0x60
acpi_wmi_remove+0xdd/0x120 [wmi]
platform_drv_remove+0x25/0x50
device_release_driver_internal+0xec/0x1b0
driver_detach+0x4d/0xa0
bus_remove_driver+0x80/0xe0
driver_unregister+0x2f/0x50
platform_driver_unregister+0x12/0x20
acpi_wmi_exit+0x10/0x169 [wmi]
__x64_sys_delete_module+0x15b/0x240
? lockdep_hardirqs_on+0xe8/0x1c0
? do_syscall_64+0x17/0x1c0
? entry_SYSCALL_64_after_hwframe+0x49/0xbe
do_syscall_64+0x55/0x1c0
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x7f5a592f6767
Code: 73 01 c3 48 8b 0d 19 c7 0b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 b8 b0 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d e9 c6 0b 00 f7 d8 64 89 01 48
RSP: 002b:00007ffd85909808 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f5a592f6767
RDX: 000000000000000a RSI: 0000000000000800 RDI: 000055b3f25097c8
RBP: 00007ffd85909858 R08: 0000000000000000 R09: 0000000000000000
R10: 00007f5a59366ac0 R11: 0000000000000206 R12: 00007ffd85909a20
R13: 00007ffd85909e36 R14: 000055b3f25092a0 R15: 000055b3f2509760
irq event stamp: 7152
hardirqs last enabled at (7151): [<ffffffff9b2b26ec>] _raw_spin_unlock_irq+0x2c/0x50
hardirqs last disabled at (7152): [<ffffffff9a401eba>] trace_hardirqs_off_thunk+0x1a/0x20
softirqs last enabled at (7146): [<ffffffff9b6002d9>] __do_softirq+0x2d9/0x4ef
softirqs last disabled at (7133): [<ffffffff9a504ba4>] irq_exit+0xc4/0xd0
---[ end trace 61882efbb33d050e ]---
bus: 'wmi': remove device 86CCFD48-205E-4A77-9C48-2021CBEDE341
PM: Removing info for wmi:86CCFD48-205E-4A77-9C48-2021CBEDE341
device: 'wmi_bus-PNP0C14:00': device_create_release
device: 'wmi_bus-PNP0C14:02': device_unregister
PM: Removing info for No Bus:wmi_bus-PNP0C14:02
device: 'wmi_bus-PNP0C14:02': device_create_release
device: 'wakeup27': device_unregister
driver: 'acpi-wmi': driver_release
bus: 'wmi': unregistering
device class 'wmi_bus': unregistering
class 'wmi_bus': release.
class 'wmi_bus' does not have a release() function, be careful
Thanks,
Yongxin
Yongxin Liu <yongxin.liu@windriver.com> writes: > This reverts commit 7b11e8989618581bc0226ad313264cdc05d48d86. > > Consider the following hardware setting. > > |-PNP0C14:00 > | |-- device #1 > |-PNP0C14:01 > | |-- device #2 > > When unloading wmi driver module, device #2 will be first unregistered. > But device_destroy() using MKDEV(0, 0) will locate PNP0C14:00 first > and unregister it. This is incorrect. Should use device_unregister() to > unregister the real parent device. > > Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com> > --- > drivers/platform/x86/wmi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c > index 59e9aa0f9643..e16f660aa117 100644 > --- a/drivers/platform/x86/wmi.c > +++ b/drivers/platform/x86/wmi.c > @@ -1347,7 +1347,7 @@ static int acpi_wmi_remove(struct platform_device *device) > acpi_remove_address_space_handler(acpi_device->handle, > ACPI_ADR_SPACE_EC, &acpi_wmi_ec_space_handler); > wmi_free_devices(acpi_device); > - device_destroy(&wmi_bus_class, MKDEV(0, 0)); > + device_unregister((struct device *)dev_get_drvdata(&device->dev)); > > return 0; > } > @@ -1401,7 +1401,7 @@ static int acpi_wmi_probe(struct platform_device *device) > return 0; > > err_remove_busdev: > - device_destroy(&wmi_bus_class, MKDEV(0, 0)); > + device_unregister(wmi_bus_dev); > > err_remove_notify_handler: > acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY, Definitely! Good catch! device_create() will allow registering multiple devices with a zero major. Using device_destroy() with MKDEV(0, 0) will unregister an arbitrary one of them. I believe all of these should be reviewed and fixed up: drivers/nvme/host/fabrics.c: device_destroy(nvmf_class, MKDEV(0, 0)); drivers/nvme/host/fabrics.c: device_destroy(nvmf_class, MKDEV(0, 0)); drivers/nvme/host/fc.c: device_destroy(&fc_class, MKDEV(0, 0)); drivers/nvme/host/fc.c: device_destroy(&fc_class, MKDEV(0, 0)); drivers/nvme/target/fcloop.c: device_destroy(fcloop_class, MKDEV(0, 0)); drivers/platform/x86/wmi.c: device_destroy(&wmi_bus_class, MKDEV(0, 0)); drivers/platform/x86/wmi.c: device_destroy(&wmi_bus_class, MKDEV(0, 0)); drivers/staging/comedi/drivers/comedi_test.c: device_destroy(ctcls, MKDEV(0, 0)); drivers/staging/comedi/drivers/comedi_test.c: device_destroy(ctcls, MKDEV(0, 0)); drivers/video/fbdev/core/fbcon.c: device_destroy(fb_class, MKDEV(0, 0)); net/netfilter/xt_IDLETIMER.c: device_destroy(idletimer_tg_class, MKDEV(0, 0)); net/netfilter/xt_IDLETIMER.c: device_destroy(idletimer_tg_class, MKDEV(0, 0)); Note that most of these probably are not bugs. yet... But there is no reason to look up the device by dev_t for drivers allowing only one device anyway. Using device_unregister() directly makes the code easier to follow and prevents future bugs in case someone decides to support more devices. Maybe we should add a WARN_ON(!MAJOR(devt)) or similar to device_destroy() to prevent similar future problems? Bjørn
Bjørn Mork <bjorn@mork.no> writes: > Maybe we should add a WARN_ON(!MAJOR(devt)) or similar to > device_destroy() to prevent similar future problems? No, that's definitely not a good idea. We have examples like drivers/tty/vt/vt.c which (ab)use the devt with zero major and a unique minor to keep track of devices. So forget about any warning. But the device_destroy's with a static MKDEV(0,0) should be removed. Bjørn
Hi, Quick self intro: I have take over drivers/platform/x86 maintainership from Andy; and I'm working my way through the backlog of old patches in patchwork: https://patchwork.kernel.org/project/platform-driver-x86/list/ On 11/15/19 6:27 AM, Yongxin Liu wrote: > This reverts commit 7b11e8989618581bc0226ad313264cdc05d48d86. > > Consider the following hardware setting. > > |-PNP0C14:00 > | |-- device #1 > |-PNP0C14:01 > | |-- device #2 > > When unloading wmi driver module, device #2 will be first unregistered. > But device_destroy() using MKDEV(0, 0) will locate PNP0C14:00 first > and unregister it. This is incorrect. Should use device_unregister() to > unregister the real parent device. > > Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up there once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > drivers/platform/x86/wmi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c > index 59e9aa0f9643..e16f660aa117 100644 > --- a/drivers/platform/x86/wmi.c > +++ b/drivers/platform/x86/wmi.c > @@ -1347,7 +1347,7 @@ static int acpi_wmi_remove(struct platform_device *device) > acpi_remove_address_space_handler(acpi_device->handle, > ACPI_ADR_SPACE_EC, &acpi_wmi_ec_space_handler); > wmi_free_devices(acpi_device); > - device_destroy(&wmi_bus_class, MKDEV(0, 0)); > + device_unregister((struct device *)dev_get_drvdata(&device->dev)); > > return 0; > } > @@ -1401,7 +1401,7 @@ static int acpi_wmi_probe(struct platform_device *device) > return 0; > > err_remove_busdev: > - device_destroy(&wmi_bus_class, MKDEV(0, 0)); > + device_unregister(wmi_bus_dev); > > err_remove_notify_handler: > acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY, >
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c index 59e9aa0f9643..e16f660aa117 100644 --- a/drivers/platform/x86/wmi.c +++ b/drivers/platform/x86/wmi.c @@ -1347,7 +1347,7 @@ static int acpi_wmi_remove(struct platform_device *device) acpi_remove_address_space_handler(acpi_device->handle, ACPI_ADR_SPACE_EC, &acpi_wmi_ec_space_handler); wmi_free_devices(acpi_device); - device_destroy(&wmi_bus_class, MKDEV(0, 0)); + device_unregister((struct device *)dev_get_drvdata(&device->dev)); return 0; } @@ -1401,7 +1401,7 @@ static int acpi_wmi_probe(struct platform_device *device) return 0; err_remove_busdev: - device_destroy(&wmi_bus_class, MKDEV(0, 0)); + device_unregister(wmi_bus_dev); err_remove_notify_handler: acpi_remove_notify_handler(acpi_device->handle, ACPI_DEVICE_NOTIFY,
This reverts commit 7b11e8989618581bc0226ad313264cdc05d48d86. Consider the following hardware setting. |-PNP0C14:00 | |-- device #1 |-PNP0C14:01 | |-- device #2 When unloading wmi driver module, device #2 will be first unregistered. But device_destroy() using MKDEV(0, 0) will locate PNP0C14:00 first and unregister it. This is incorrect. Should use device_unregister() to unregister the real parent device. Signed-off-by: Yongxin Liu <yongxin.liu@windriver.com> --- drivers/platform/x86/wmi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)