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 <[email protected]>
---
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,
--
2.14.4
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 <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]>
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,
>