2020-11-19 12:49:09

by Uwe Kleine-König

[permalink] [raw]
Subject: [PATCH 3/3] driver core: platform: use bus_type functions

This works towards the goal mentioned in 2006 in commit 594c8281f905
("[PATCH] Add bus_type probe, remove, shutdown methods.").

The functions are moved to where the other bus_type functions are
defined and renamed to match the already established naming scheme.

Signed-off-by: Uwe Kleine-König <[email protected]>
---
drivers/base/platform.c | 132 ++++++++++++++++++++--------------------
1 file changed, 65 insertions(+), 67 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index b847f5f8f992..8ad06daa2eaa 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -743,70 +743,6 @@ struct platform_device *platform_device_register_full(
}
EXPORT_SYMBOL_GPL(platform_device_register_full);

-static int platform_probe_fail(struct platform_device *pdev);
-
-static int platform_drv_probe(struct device *_dev)
-{
- struct platform_driver *drv = to_platform_driver(_dev->driver);
- struct platform_device *dev = to_platform_device(_dev);
- int ret;
-
- /*
- * A driver registered using platform_driver_probe() cannot be bound
- * again later because the probe function usually lives in __init code
- * and so is gone. For these drivers .probe is set to
- * platform_probe_fail in __platform_driver_probe(). Don't even
- * prepare clocks and PM domains for these to match the traditional
- * behaviour.
- */
- if (unlikely(drv->probe == platform_probe_fail))
- return -ENXIO;
-
- ret = of_clk_set_defaults(_dev->of_node, false);
- if (ret < 0)
- return ret;
-
- ret = dev_pm_domain_attach(_dev, true);
- if (ret)
- goto out;
-
- if (drv->probe) {
- ret = drv->probe(dev);
- if (ret)
- dev_pm_domain_detach(_dev, true);
- }
-
-out:
- if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
- dev_warn(_dev, "probe deferral not supported\n");
- ret = -ENXIO;
- }
-
- return ret;
-}
-
-static int platform_drv_remove(struct device *_dev)
-{
- struct platform_driver *drv = to_platform_driver(_dev->driver);
- struct platform_device *dev = to_platform_device(_dev);
- int ret = 0;
-
- if (drv->remove)
- ret = drv->remove(dev);
- dev_pm_domain_detach(_dev, true);
-
- return ret;
-}
-
-static void platform_drv_shutdown(struct device *_dev)
-{
- struct platform_driver *drv = to_platform_driver(_dev->driver);
- struct platform_device *dev = to_platform_device(_dev);
-
- if (drv->shutdown)
- drv->shutdown(dev);
-}
-
/**
* __platform_driver_register - register a driver for platform-level devices
* @drv: platform driver structure
@@ -817,9 +753,6 @@ int __platform_driver_register(struct platform_driver *drv,
{
drv->driver.owner = owner;
drv->driver.bus = &platform_bus_type;
- drv->driver.probe = platform_drv_probe;
- drv->driver.remove = platform_drv_remove;
- drv->driver.shutdown = platform_drv_shutdown;

return driver_register(&drv->driver);
}
@@ -1349,6 +1282,68 @@ static int platform_uevent(struct device *dev, struct kobj_uevent_env *env)
return 0;
}

+static int platform_probe(struct device *_dev)
+{
+ struct platform_driver *drv = to_platform_driver(_dev->driver);
+ struct platform_device *dev = to_platform_device(_dev);
+ int ret;
+
+ /*
+ * A driver registered using platform_driver_probe() cannot be bound
+ * again later because the probe function usually lives in __init code
+ * and so is gone. For these drivers .probe is set to
+ * platform_probe_fail in __platform_driver_probe(). Don't even prepare
+ * clocks and PM domains for these to match the traditional behaviour.
+ */
+ if (unlikely(drv->probe == platform_probe_fail))
+ return -ENXIO;
+
+ ret = of_clk_set_defaults(_dev->of_node, false);
+ if (ret < 0)
+ return ret;
+
+ ret = dev_pm_domain_attach(_dev, true);
+ if (ret)
+ goto out;
+
+ if (drv->probe) {
+ ret = drv->probe(dev);
+ if (ret)
+ dev_pm_domain_detach(_dev, true);
+ }
+
+out:
+ if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
+ dev_warn(_dev, "probe deferral not supported\n");
+ ret = -ENXIO;
+ }
+
+ return ret;
+}
+
+static int platform_remove(struct device *_dev)
+{
+ struct platform_driver *drv = to_platform_driver(_dev->driver);
+ struct platform_device *dev = to_platform_device(_dev);
+ int ret = 0;
+
+ if (drv->remove)
+ ret = drv->remove(dev);
+ dev_pm_domain_detach(_dev, true);
+
+ return ret;
+}
+
+static void platform_shutdown(struct device *_dev)
+{
+ struct platform_driver *drv = to_platform_driver(_dev->driver);
+ struct platform_device *dev = to_platform_device(_dev);
+
+ if (drv->shutdown)
+ drv->shutdown(dev);
+}
+
+
int platform_dma_configure(struct device *dev)
{
enum dev_dma_attr attr;
@@ -1375,6 +1370,9 @@ struct bus_type platform_bus_type = {
.dev_groups = platform_dev_groups,
.match = platform_match,
.uevent = platform_uevent,
+ .probe = platform_probe,
+ .remove = platform_remove,
+ .shutdown = platform_shutdown,
.dma_configure = platform_dma_configure,
.pm = &platform_dev_pm_ops,
};
--
2.28.0


2020-12-11 18:32:00

by Marek Szyprowski

[permalink] [raw]
Subject: Re: [PATCH 3/3] driver core: platform: use bus_type functions

Hi Uwe,

On 19.11.2020 13:46, Uwe Kleine-König wrote:
> This works towards the goal mentioned in 2006 in commit 594c8281f905
> ("[PATCH] Add bus_type probe, remove, shutdown methods.").
>
> The functions are moved to where the other bus_type functions are
> defined and renamed to match the already established naming scheme.
>
> Signed-off-by: Uwe Kleine-König <[email protected]>
> ---
> drivers/base/platform.c | 132 ++++++++++++++++++++--------------------
> 1 file changed, 65 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index b847f5f8f992..8ad06daa2eaa 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -743,70 +743,6 @@ struct platform_device *platform_device_register_full(
> }
> EXPORT_SYMBOL_GPL(platform_device_register_full);
>
> -static int platform_probe_fail(struct platform_device *pdev);
> -
> -static int platform_drv_probe(struct device *_dev)
> -{
> - struct platform_driver *drv = to_platform_driver(_dev->driver);
> - struct platform_device *dev = to_platform_device(_dev);
> - int ret;
> -
> - /*
> - * A driver registered using platform_driver_probe() cannot be bound
> - * again later because the probe function usually lives in __init code
> - * and so is gone. For these drivers .probe is set to
> - * platform_probe_fail in __platform_driver_probe(). Don't even
> - * prepare clocks and PM domains for these to match the traditional
> - * behaviour.
> - */
> - if (unlikely(drv->probe == platform_probe_fail))
> - return -ENXIO;
> -
> - ret = of_clk_set_defaults(_dev->of_node, false);
> - if (ret < 0)
> - return ret;
> -
> - ret = dev_pm_domain_attach(_dev, true);
> - if (ret)
> - goto out;
> -
> - if (drv->probe) {
> - ret = drv->probe(dev);
> - if (ret)
> - dev_pm_domain_detach(_dev, true);
> - }
> -
> -out:
> - if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> - dev_warn(_dev, "probe deferral not supported\n");
> - ret = -ENXIO;
> - }
> -
> - return ret;
> -}
> -
> -static int platform_drv_remove(struct device *_dev)
> -{
> - struct platform_driver *drv = to_platform_driver(_dev->driver);
> - struct platform_device *dev = to_platform_device(_dev);
> - int ret = 0;
> -
> - if (drv->remove)
> - ret = drv->remove(dev);
> - dev_pm_domain_detach(_dev, true);
> -
> - return ret;
> -}
> -
> -static void platform_drv_shutdown(struct device *_dev)
> -{
> - struct platform_driver *drv = to_platform_driver(_dev->driver);
> - struct platform_device *dev = to_platform_device(_dev);
> -
> - if (drv->shutdown)
> - drv->shutdown(dev);
> -}
> -
> /**
> * __platform_driver_register - register a driver for platform-level devices
> * @drv: platform driver structure
> @@ -817,9 +753,6 @@ int __platform_driver_register(struct platform_driver *drv,
> {
> drv->driver.owner = owner;
> drv->driver.bus = &platform_bus_type;
> - drv->driver.probe = platform_drv_probe;
> - drv->driver.remove = platform_drv_remove;
> - drv->driver.shutdown = platform_drv_shutdown;
>
> return driver_register(&drv->driver);
> }
> @@ -1349,6 +1282,68 @@ static int platform_uevent(struct device *dev, struct kobj_uevent_env *env)
> return 0;
> }
>
> +static int platform_probe(struct device *_dev)
> +{
> + struct platform_driver *drv = to_platform_driver(_dev->driver);
> + struct platform_device *dev = to_platform_device(_dev);
> + int ret;
> +
> + /*
> + * A driver registered using platform_driver_probe() cannot be bound
> + * again later because the probe function usually lives in __init code
> + * and so is gone. For these drivers .probe is set to
> + * platform_probe_fail in __platform_driver_probe(). Don't even prepare
> + * clocks and PM domains for these to match the traditional behaviour.
> + */
> + if (unlikely(drv->probe == platform_probe_fail))
> + return -ENXIO;
> +
> + ret = of_clk_set_defaults(_dev->of_node, false);
> + if (ret < 0)
> + return ret;
> +
> + ret = dev_pm_domain_attach(_dev, true);
> + if (ret)
> + goto out;
> +
> + if (drv->probe) {
> + ret = drv->probe(dev);
> + if (ret)
> + dev_pm_domain_detach(_dev, true);
> + }
> +
> +out:
> + if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> + dev_warn(_dev, "probe deferral not supported\n");
> + ret = -ENXIO;
> + }
> +
> + return ret;
> +}
> +
> +static int platform_remove(struct device *_dev)
> +{
> + struct platform_driver *drv = to_platform_driver(_dev->driver);
> + struct platform_device *dev = to_platform_device(_dev);
> + int ret = 0;
> +
> + if (drv->remove)
> + ret = drv->remove(dev);
> + dev_pm_domain_detach(_dev, true);
> +
> + return ret;
> +}
> +
> +static void platform_shutdown(struct device *_dev)
> +{
> + struct platform_driver *drv = to_platform_driver(_dev->driver);
> + struct platform_device *dev = to_platform_device(_dev);
> +
> + if (drv->shutdown)
> + drv->shutdown(dev);
> +}

This will be called on unbound devices, what causes crash (observed on
today's linux-next):

Will now restart.
8<--- cut here ---
Unable to handle kernel paging request at virtual address fffffff4
pgd = 289f4b67
[fffffff4] *pgd=6ffff841, *pte=00000000, *ppte=00000000
Internal error: Oops: 27 [#1] SMP ARM
Modules linked in: usb_f_ecm g_ether usb_f_rndis u_ether libcomposite
brcmfmac sha256_generic libsha256 sha256_arm cfg80211 brcmutil
panel_samsung_s6e8aa0 s5p_csi
CPU: 0 PID: 1715 Comm: reboot Not tainted
5.10.0-rc7-next-20201211-00069-g1e8aa883315f #9935
Hardware name: Samsung Exynos (Flattened Device Tree)
PC is at platform_shutdown+0x8/0x18
LR is at device_shutdown+0x18c/0x25c
...
Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 4348404a  DAC: 00000051
Process reboot (pid: 1715, stack limit = 0xd050391e)
Stack: (0xc3405e60 to 0xc3406000)
[<c0a38bfc>] (platform_shutdown) from [<c0a34f78>]
(device_shutdown+0x18c/0x25c)
[<c0a34f78>] (device_shutdown) from [<c036d3cc>] (kernel_restart+0xc/0x68)
[<c036d3cc>] (kernel_restart) from [<c036d680>]
(__do_sys_reboot+0x154/0x1f0)
[<c036d680>] (__do_sys_reboot) from [<c03000c0>] (ret_fast_syscall+0x0/0x58)
Exception stack(0xc3405fa8 to 0xc3405ff0)
...
---[ end trace f39e94d5d6fd45bf ]---


> +
> +
> int platform_dma_configure(struct device *dev)
> {
> enum dev_dma_attr attr;
> @@ -1375,6 +1370,9 @@ struct bus_type platform_bus_type = {
> .dev_groups = platform_dev_groups,
> .match = platform_match,
> .uevent = platform_uevent,
> + .probe = platform_probe,
> + .remove = platform_remove,
> + .shutdown = platform_shutdown,
> .dma_configure = platform_dma_configure,
> .pm = &platform_dev_pm_ops,
> };

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

2020-12-11 18:47:42

by Qian Cai

[permalink] [raw]
Subject: Re: [PATCH 3/3] driver core: platform: use bus_type functions

On Thu, 2020-11-19 at 13:46 +0100, Uwe Kleine-König wrote:
> This works towards the goal mentioned in 2006 in commit 594c8281f905
> ("[PATCH] Add bus_type probe, remove, shutdown methods.").
>
> The functions are moved to where the other bus_type functions are
> defined and renamed to match the already established naming scheme.
>
> Signed-off-by: Uwe Kleine-König <[email protected]>

Reverting this commit from today's linux-next fixed a crash during shutdown.

.config: https://cailca.coding.net/public/linux/mm/git/files/master/x86.config

[ 9771.596916][T113465] BUG: unable to handle page fault for address: ffffffffffffffe8
[ 9771.604627][T113465] #PF: supervisor read access in kernel mode
[ 9771.610581][T113465] #PF: error_code(0x0000) - not-present page
[ 9771.616533][T113465] PGD 19c1e17067 P4D 19c1e17067 PUD 19c1e19067 PMD 0
[ 9771.623279][T113465] Oops: 0000 [#1] SMP KASAN PTI
[ 9771.628098][T113465] CPU: 22 PID: 113465 Comm: reboot Tainted: G IO 5.10.0-rc7-next-20201211 #1
[ 9771.638071][T113465] Hardware name: HPE ProLiant DL560 Gen10/ProLiant DL560 Gen10, BIOS U34 11/13/2019
[ 9771.647431][T113465] RIP: 0010:platform_shutdown+0x44/0x70
platform_shutdown at drivers/base/platform.c:1357
[ 9771.652956][T113465] Code: fa 48 c1 ea 03 80 3c 02 00 75 3d 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 68 48 8d 7d e8 48 89 fa 48 c1 ea 03 80 3c 02 00 75 17 <48> 8b 45 e8 48 85 c0 74 0b 48 8d 7b f0 5b 5d e9 08 45 6c 00 5b 5d
[ 9771.672623][T113465] RSP: 0018:ffffc90008a77d38 EFLAGS: 00010246
[ 9771.678665][T113465] RAX: dffffc0000000000 RBX: ffff888860d78810 RCX: ffff888860d78870
[ 9771.686628][T113465] RDX: 1ffffffffffffffd RSI: 0000000000000001 RDI: ffffffffffffffe8
[ 9771.694591][T113465] RBP: 0000000000000000 R08: ffffed110c1af166 R09: ffffed110c1af166
[ 9771.702555][T113465] R10: ffff888860d78b2b R11: ffffed110c1af165 R12: ffff888860d78810
[ 9771.710516][T113465] R13: ffff888860d78920 R14: fffffbfff2db0008 R15: ffff888860d78818
[ 9771.718478][T113465] FS: 00007f3434549540(0000) GS:ffff88901f500000(0000) knlGS:0000000000000000
[ 9771.727402][T113465] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 9771.733966][T113465] CR2: ffffffffffffffe8 CR3: 000000092e9c0004 CR4: 00000000007706e0
[ 9771.741929][T113465] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 9771.749890][T113465] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 9771.757852][T113465] PKRU: 55555554
[ 9771.761359][T113465] Call Trace:
[ 9771.764604][T113465] device_shutdown+0x2ec/0x540
[ 9771.769335][T113465] kernel_restart+0xe/0x40
[ 9771.773721][T113465] __do_sys_reboot+0x143/0x2b0
[ 9771.778450][T113465] ? kernel_power_off+0xa0/0xa0
[ 9771.783269][T113465] ? debug_object_deactivate+0x3b0/0x3b0
[ 9771.788877][T113465] ? syscall_enter_from_user_mode+0x17/0x40
[ 9771.794747][T113465] ? rcu_read_lock_sched_held+0xa1/0xd0
[ 9771.800267][T113465] ? lockdep_hardirqs_on_prepare+0x27c/0x3d0
[ 9771.806221][T113465] ? syscall_enter_from_user_mode+0x1c/0x40
[ 9771.812087][T113465] do_syscall_64+0x33/0x40
[ 9771.816472][T113465] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[ 9771.822340][T113465] RIP: 0033:0x7f343379b857
[ 9771.826724][T113465] Code: 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 89 fa be 69 19 12 28 bf ad de e1 fe b8 a9 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 01 c3 48 8b 15 01 86 2c 00 f7 d8 64 89 02 b8
[ 9771.846392][T113465] RSP: 002b:00007ffef9f85e58 EFLAGS: 00000246 ORIG_RAX: 00000000000000a9
[ 9771.854791][T113465] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f343379b857
[ 9771.862752][T113465] RDX: 0000000001234567 RSI: 0000000028121969 RDI: 00000000fee1dead
[ 9771.870713][T113465] RBP: 00007ffef9f85ea0 R08: 0000000000000002 R09: 0000000000000000
[ 9771.878673][T113465] R10: 000000000000004b R11: 0000000000000246 R12: 0000000000000001
[ 9771.886635][T113465] R13: 00000000fffffffe R14: 0000000000000006 R15: 0000000000000000
[ 9771.894596][T113465] Modules linked in: isofs cdrom fuse loop nls_ascii nls_cp437 vfat fat kvm_intel kvm ses enclosure irqbypass efivarfs ip_tables x_tables sd_mod tg3 nvme firmware_class smartpqi nvme_core libphy scsi_transport_sas dm_mirror dm_region_hash dm_log dm_mod [last unloaded: dummy_del_mod]
[ 9771.921472][T113465] CR2: ffffffffffffffe8
[ 9771.925590][T113465] ---[ end trace 8a3c9cffc1068bd2 ]---
[ 9771.931017][T113465] RIP: 0010:platform_shutdown+0x44/0x70
[ 9771.936535][T113465] Code: fa 48 c1 ea 03 80 3c 02 00 75 3d 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 68 48 8d 7d e8 48 89 fa 48 c1 ea 03 80 3c 02 00 75 17 <48> 8b 45 e8 48 85 c0 74 0b 48 8d 7b f0 5b 5d e9 08 45 6c 00 5b 5d
[ 9771.956204][T113465] RSP: 0018:ffffc90008a77d38 EFLAGS: 00010246

> ---
> drivers/base/platform.c | 132 ++++++++++++++++++++--------------------
> 1 file changed, 65 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index b847f5f8f992..8ad06daa2eaa 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -743,70 +743,6 @@ struct platform_device *platform_device_register_full(
> }
> EXPORT_SYMBOL_GPL(platform_device_register_full);
>
> -static int platform_probe_fail(struct platform_device *pdev);
> -
> -static int platform_drv_probe(struct device *_dev)
> -{
> - struct platform_driver *drv = to_platform_driver(_dev->driver);
> - struct platform_device *dev = to_platform_device(_dev);
> - int ret;
> -
> - /*
> - * A driver registered using platform_driver_probe() cannot be bound
> - * again later because the probe function usually lives in __init code
> - * and so is gone. For these drivers .probe is set to
> - * platform_probe_fail in __platform_driver_probe(). Don't even
> - * prepare clocks and PM domains for these to match the traditional
> - * behaviour.
> - */
> - if (unlikely(drv->probe == platform_probe_fail))
> - return -ENXIO;
> -
> - ret = of_clk_set_defaults(_dev->of_node, false);
> - if (ret < 0)
> - return ret;
> -
> - ret = dev_pm_domain_attach(_dev, true);
> - if (ret)
> - goto out;
> -
> - if (drv->probe) {
> - ret = drv->probe(dev);
> - if (ret)
> - dev_pm_domain_detach(_dev, true);
> - }
> -
> -out:
> - if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> - dev_warn(_dev, "probe deferral not supported\n");
> - ret = -ENXIO;
> - }
> -
> - return ret;
> -}
> -
> -static int platform_drv_remove(struct device *_dev)
> -{
> - struct platform_driver *drv = to_platform_driver(_dev->driver);
> - struct platform_device *dev = to_platform_device(_dev);
> - int ret = 0;
> -
> - if (drv->remove)
> - ret = drv->remove(dev);
> - dev_pm_domain_detach(_dev, true);
> -
> - return ret;
> -}
> -
> -static void platform_drv_shutdown(struct device *_dev)
> -{
> - struct platform_driver *drv = to_platform_driver(_dev->driver);
> - struct platform_device *dev = to_platform_device(_dev);
> -
> - if (drv->shutdown)
> - drv->shutdown(dev);
> -}
> -
> /**
> * __platform_driver_register - register a driver for platform-level devices
> * @drv: platform driver structure
> @@ -817,9 +753,6 @@ int __platform_driver_register(struct platform_driver *drv,
> {
> drv->driver.owner = owner;
> drv->driver.bus = &platform_bus_type;
> - drv->driver.probe = platform_drv_probe;
> - drv->driver.remove = platform_drv_remove;
> - drv->driver.shutdown = platform_drv_shutdown;
>
> return driver_register(&drv->driver);
> }
> @@ -1349,6 +1282,68 @@ static int platform_uevent(struct device *dev, struct kobj_uevent_env *env)
> return 0;
> }
>
> +static int platform_probe(struct device *_dev)
> +{
> + struct platform_driver *drv = to_platform_driver(_dev->driver);
> + struct platform_device *dev = to_platform_device(_dev);
> + int ret;
> +
> + /*
> + * A driver registered using platform_driver_probe() cannot be bound
> + * again later because the probe function usually lives in __init code
> + * and so is gone. For these drivers .probe is set to
> + * platform_probe_fail in __platform_driver_probe(). Don't even prepare
> + * clocks and PM domains for these to match the traditional behaviour.
> + */
> + if (unlikely(drv->probe == platform_probe_fail))
> + return -ENXIO;
> +
> + ret = of_clk_set_defaults(_dev->of_node, false);
> + if (ret < 0)
> + return ret;
> +
> + ret = dev_pm_domain_attach(_dev, true);
> + if (ret)
> + goto out;
> +
> + if (drv->probe) {
> + ret = drv->probe(dev);
> + if (ret)
> + dev_pm_domain_detach(_dev, true);
> + }
> +
> +out:
> + if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> + dev_warn(_dev, "probe deferral not supported\n");
> + ret = -ENXIO;
> + }
> +
> + return ret;
> +}
> +
> +static int platform_remove(struct device *_dev)
> +{
> + struct platform_driver *drv = to_platform_driver(_dev->driver);
> + struct platform_device *dev = to_platform_device(_dev);
> + int ret = 0;
> +
> + if (drv->remove)
> + ret = drv->remove(dev);
> + dev_pm_domain_detach(_dev, true);
> +
> + return ret;
> +}
> +
> +static void platform_shutdown(struct device *_dev)
> +{
> + struct platform_driver *drv = to_platform_driver(_dev->driver);
> + struct platform_device *dev = to_platform_device(_dev);
> +
> + if (drv->shutdown)
> + drv->shutdown(dev);
> +}
> +
> +
> int platform_dma_configure(struct device *dev)
> {
> enum dev_dma_attr attr;
> @@ -1375,6 +1370,9 @@ struct bus_type platform_bus_type = {
> .dev_groups = platform_dev_groups,
> .match = platform_match,
> .uevent = platform_uevent,
> + .probe = platform_probe,
> + .remove = platform_remove,
> + .shutdown = platform_shutdown,
> .dma_configure = platform_dma_configure,
> .pm = &platform_dev_pm_ops,
> };

2020-12-13 14:02:13

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 3/3] driver core: platform: use bus_type functions

On Thu, Nov 19, 2020 at 01:46:11PM +0100, Uwe Kleine-K?nig wrote:
> This works towards the goal mentioned in 2006 in commit 594c8281f905
> ("[PATCH] Add bus_type probe, remove, shutdown methods.").
>
> The functions are moved to where the other bus_type functions are
> defined and renamed to match the already established naming scheme.
>
> Signed-off-by: Uwe Kleine-K?nig <[email protected]>

Qemu test results:
total: 426 pass: 91 fail: 335

This patch isn't responsible for all the crashes (-next is in pretty bad
shape), but for a good chunk of it.

Guenter

---
Bisect results for arbitrary arm64 test:

# bad: [3cc2bd440f2171f093b3a8480a4b54d8c270ed38] Add linux-next specific files for 20201211
# good: [0477e92881850d44910a7e94fc2c46f96faa131f] Linux 5.10-rc7
git bisect start 'HEAD' 'v5.10-rc7'
# good: [0a701401d4e29d9e73f0f3cc02179fc6c9191646] Merge remote-tracking branch 'crypto/master'
git bisect good 0a701401d4e29d9e73f0f3cc02179fc6c9191646
# good: [6fd39ad603b113e9c68180b9138084710c036e34] Merge remote-tracking branch 'spi/for-next'
git bisect good 6fd39ad603b113e9c68180b9138084710c036e34
# bad: [c96b2eec436e87b8c673213b203559bed9e551b9] Merge remote-tracking branch 'vfio/next'
git bisect bad c96b2eec436e87b8c673213b203559bed9e551b9
# good: [f99c2fbbff522300c309e517be1f5bed4bd34704] Merge remote-tracking branch 'kvm-arm/next'
git bisect good f99c2fbbff522300c309e517be1f5bed4bd34704
# bad: [0e3f63470c00704498be2bfac586076cfa93214f] Merge remote-tracking branch 'usb-chipidea-next/for-usb-next'
git bisect bad 0e3f63470c00704498be2bfac586076cfa93214f
# bad: [903821bc4404ae12d4e50e95fb5c2d7b46f4d1c6] Merge remote-tracking branch 'driver-core/driver-core-next'
git bisect bad 903821bc4404ae12d4e50e95fb5c2d7b46f4d1c6
# good: [0cd3f561efa9adce840140720e0581355db3e554] platform/x86: ISST: Mark mmio_range_devid_0 and mmio_range_devid_1 with static keyword
git bisect good 0cd3f561efa9adce840140720e0581355db3e554
# good: [bd7cf676c3ed2fc91e777d91c3bf9220e84da2ad] Merge remote-tracking branch 'chrome-platform/for-next'
git bisect good bd7cf676c3ed2fc91e777d91c3bf9220e84da2ad
# good: [d475f8ea98a039e51d27f5557dc17333cf8a52f6] driver core: Fix a couple of typos
git bisect good d475f8ea98a039e51d27f5557dc17333cf8a52f6
# good: [16c1af8b52ea282b098c9b7506f3f4d0d3953260] Merge remote-tracking branch 'leds/for-next'
git bisect good 16c1af8b52ea282b098c9b7506f3f4d0d3953260
# bad: [feaba5932b6f4bfc875c874a3b7a28c7f05f5a77] vfio: platform: Switch to use platform_get_mem_or_io()
git bisect bad feaba5932b6f4bfc875c874a3b7a28c7f05f5a77
# bad: [9c30921fe7994907e0b3e0637b2c8c0fc4b5171f] driver core: platform: use bus_type functions
git bisect bad 9c30921fe7994907e0b3e0637b2c8c0fc4b5171f
# good: [e21d740a3fe5ad2db7b5f5c2331fe2b713b1edba] driver core: platform: reorder functions
git bisect good e21d740a3fe5ad2db7b5f5c2331fe2b713b1edba
# good: [16085668eacdc56c46652d0f3bfef81ecace57de] driver core: platform: change logic implementing platform_driver_probe
git bisect good 16085668eacdc56c46652d0f3bfef81ecace57de
# first bad commit: [9c30921fe7994907e0b3e0637b2c8c0fc4b5171f] driver core: platform: use bus_type functions

2020-12-13 17:12:01

by Uwe Kleine-König

[permalink] [raw]
Subject: Re: [PATCH 3/3] driver core: platform: use bus_type functions

Hi Marek,

On Fri, Dec 11, 2020 at 05:12:16PM +0100, Marek Szyprowski wrote:
> On 19.11.2020 13:46, Uwe Kleine-K?nig wrote:
> > +static void platform_shutdown(struct device *_dev)
> > +{
> > + struct platform_driver *drv = to_platform_driver(_dev->driver);
> > + struct platform_device *dev = to_platform_device(_dev);
> > +
> > + if (drv->shutdown)
> > + drv->shutdown(dev);
> > +}
>
> This will be called on unbound devices, what causes crash (observed on
> today's linux-next):
>
> Will now restart.
> 8<--- cut here ---
> Unable to handle kernel paging request at virtual address fffffff4
> pgd = 289f4b67
> [fffffff4] *pgd=6ffff841, *pte=00000000, *ppte=00000000
> Internal error: Oops: 27 [#1] SMP ARM
> Modules linked in: usb_f_ecm g_ether usb_f_rndis u_ether libcomposite
> brcmfmac sha256_generic libsha256 sha256_arm cfg80211 brcmutil
> panel_samsung_s6e8aa0 s5p_csi
> CPU: 0 PID: 1715 Comm: reboot Not tainted
> 5.10.0-rc7-next-20201211-00069-g1e8aa883315f #9935
> Hardware name: Samsung Exynos (Flattened Device Tree)
> PC is at platform_shutdown+0x8/0x18
> LR is at device_shutdown+0x18c/0x25c
> ...
> Flags: nZCv? IRQs on? FIQs on? Mode SVC_32? ISA ARM? Segment none
> Control: 10c5387d? Table: 4348404a? DAC: 00000051
> Process reboot (pid: 1715, stack limit = 0xd050391e)
> Stack: (0xc3405e60 to 0xc3406000)
> [<c0a38bfc>] (platform_shutdown) from [<c0a34f78>]
> (device_shutdown+0x18c/0x25c)
> [<c0a34f78>] (device_shutdown) from [<c036d3cc>] (kernel_restart+0xc/0x68)
> [<c036d3cc>] (kernel_restart) from [<c036d680>]
> (__do_sys_reboot+0x154/0x1f0)
> [<c036d680>] (__do_sys_reboot) from [<c03000c0>] (ret_fast_syscall+0x0/0x58)
> Exception stack(0xc3405fa8 to 0xc3405ff0)
> ...
> ---[ end trace f39e94d5d6fd45bf ]---

Dmitry Baryshkov already proposed a fix, see
https://lore.kernel.org/r/[email protected]

I expect a v2 soon.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |


Attachments:
(No filename) (2.07 kB)
signature.asc (499.00 B)
Download all attachments