[PATCH] Revert "platform/x86: wmi: Destroy on cleanup rather than unregister"

[PATCH] Revert "platform/x86: wmi: Destroy on cleanup rather than unregister"

[Yongxin Liu]

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,
-- 
2.14.4

RE: [PATCH] Revert "platform/x86: wmi: Destroy on cleanup rather than unregister"

[Liu, Yongxin]

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

Re: [PATCH] Revert "platform/x86: wmi: Destroy on cleanup rather than unregister"

[Bjørn Mork]

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

Re: [PATCH] Revert "platform/x86: wmi: Destroy on cleanup rather than unregister"

[Bjørn Mork]

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

Re: [PATCH] Revert "platform/x86: wmi: Destroy on cleanup rather than unregister"

[Hans de Goede]

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,
>