* [PATCH] firmware: arm_scmi: Fix duplicate workqueue name
@ 2020-10-14 2:17 Florian Fainelli
2020-10-14 9:18 ` Sudeep Holla
2020-10-14 20:29 ` Sudeep Holla
0 siblings, 2 replies; 12+ messages in thread
From: Florian Fainelli @ 2020-10-14 2:17 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Florian Fainelli, Sudeep Holla, Jonathan Cameron,
Cristian Marussi, linux-kernel
When more than a single SCMI device are present in the system, the
creation of the notification workqueue with the WQ_SYSFS flag will lead
to the following sysfs duplicate node warning:
[ 9.259990] sysfs: cannot create duplicate filename '/devices/virtual/workqueue/scmi_notify'
[ 9.260024] CPU: 0 PID: 20 Comm: kworker/0:1 Not tainted 5.9.0-gdf4dd84a3f7d #29
[ 9.260045] Hardware name: Broadcom STB (Flattened Device Tree)
[ 9.260083] Workqueue: events deferred_probe_work_func
[ 9.260099] Backtrace:
[ 9.260127] [<c02120b4>] (dump_backtrace) from [<c02123d8>] (show_stack+0x20/0x24)
[ 9.260155] r9:ffffffff r8:00000000 r7:c298e3c0 r6:60000013 r5:00000000 r4:c298e3c0
[ 9.260186] [<c02123b8>] (show_stack) from [<c08852a0>] (dump_stack+0xbc/0xe0)
[ 9.260216] [<c08851e4>] (dump_stack) from [<c054442c>] (sysfs_warn_dup+0x70/0x80)
[ 9.260243] r10:e472d814 r9:ffffffef r8:e4614a50 r7:c2806d48 r6:e0e50f80 r5:e4614a50
[ 9.260265] r4:e0e82000 r3:c5b17c34
[ 9.260285] [<c05443bc>] (sysfs_warn_dup) from [<c05445fc>] (sysfs_create_dir_ns+0x15c/0x1a4)
[ 9.260310] r7:c2806d48 r6:ffffffef r5:e0dff008 r4:bb8cd6e4
[ 9.260335] [<c05444a0>] (sysfs_create_dir_ns) from [<c088ebac>] (kobject_add_internal+0x140/0x4d0)
[ 9.260363] r9:e451fa80 r8:e0dff01c r7:e0dff014 r6:e45b1400 r5:e0dff018 r4:e0dff008
[ 9.260390] [<c088ea6c>] (kobject_add_internal) from [<c088f004>] (kobject_add+0xc8/0x138)
[ 9.260416] r10:e0dff030 r9:00000000 r8:e45b1400 r7:00000000 r6:c2806d48 r5:e0dff008
[ 9.260436] r4:bb8cd708
[ 9.260455] [<c088ef3c>] (kobject_add) from [<c0a0bc70>] (device_add+0x1dc/0xc20)
[ 9.260476] r3:e0dff014 r2:00000000
[ 9.260495] r9:00000000 r8:e466b980 r7:e45b1400 r6:c2806d48 r5:bb8cd724 r4:e0dff008
[ 9.260521] [<c0a0ba94>] (device_add) from [<c0a0c6d8>] (device_register+0x24/0x28)
[ 9.260547] r10:c2831340 r9:e0dfec08 r8:e0dff028 r7:e0dfecc0 r6:e0dfec00 r5:e0dff008
[ 9.260567] r4:e0dff008
[ 9.260586] [<c0a0c6b4>] (device_register) from [<c025f2a8>] (workqueue_sysfs_register+0xe4/0x1f0)
[ 9.260609] r5:e0dff008 r4:e0dff000
[ 9.260627] [<c025f1c4>] (workqueue_sysfs_register) from [<c025f7fc>] (alloc_workqueue+0x448/0x6ac)
[ 9.260655] r10:e0dfecc0 r9:e0dfec08 r8:e0dfec0c r7:e0dfec04 r6:e0dfecc0 r5:e0dfec00
[ 9.260675] r4:00000001
[ 9.260698] [<c025f3b4>] (alloc_workqueue) from [<c0d8b58c>] (scmi_notification_init+0x78/0x1dc)
[ 9.260720] r3:c0a1b488 r2:00000000
[ 9.260739] r10:e0de1040 r9:00800004 r8:e0de10d8 r7:e0de10f4 r6:e0de1074 r5:e0e54740
[ 9.260759] r4:e0e50d00
[ 9.260778] [<c0d8b514>] (scmi_notification_init) from [<c0d87c00>] (scmi_probe+0x268/0x4fc)
[ 9.260805] r9:00800004 r8:e0de10d8 r7:e0de10f4 r6:e0de1074 r5:e0de10f0 r4:e472d810
[ 9.260833] [<c0d87998>] (scmi_probe) from [<c0a17d74>] (platform_drv_probe+0x70/0xc8)
[ 9.260859] r10:e472d844 r9:c2ab02b4 r8:c2b5e298 r7:00000000 r6:c2ab02b4 r5:00000000
[ 9.260879] r4:e472d810
[ 9.260898] [<c0a17d04>] (platform_drv_probe) from [<c0a13e80>] (really_probe+0x184/0x728)
[ 9.260922] r7:00000000 r6:c2fc82a0 r5:c2fc8220 r4:e472d810
[ 9.260946] [<c0a13cfc>] (really_probe) from [<c0a146b4>] (driver_probe_device+0xa4/0x278)
[ 9.260973] r10:e0cef438 r9:c0a14a04 r8:c2b771c0 r7:c2b5e298 r6:c2ab02b4 r5:e472d834
[ 9.260993] r4:e472d810
[ 9.261014] [<c0a14610>] (driver_probe_device) from [<c0a14aec>] (__device_attach_driver+0xe8/0x148)
[ 9.261042] r10:e0cef438 r9:c0a14a04 r8:e466bdc0 r7:e472d810 r6:00000000 r5:c2ab02b4
[ 9.261062] r4:e466bdc0
[ 9.261082] [<c0a14a04>] (__device_attach_driver) from [<c0a10f6c>] (bus_for_each_drv+0x108/0x158)
[ 9.261107] r7:c2806d48 r6:00000000 r5:bb8cd79c r4:e466bd00
[ 9.261131] [<c0a10e64>] (bus_for_each_drv) from [<c0a13c38>] (__device_attach+0x190/0x234)
[ 9.261158] r10:e46fde00 r9:e472d848 r8:e472d854 r7:e466be00 r6:c2806d48 r5:bb8cd7b4
[ 9.261178] r4:e472d810
[ 9.261199] [<c0a13aa8>] (__device_attach) from [<c0a14b68>] (device_initial_probe+0x1c/0x20)
[ 9.261226] r10:e46fde54 r9:c29d6380 r8:c29d7910 r7:e472d810 r6:c29d78c0 r5:c29d6340
[ 9.261246] r4:e44efe00
[ 9.261266] [<c0a14b4c>] (device_initial_probe) from [<c0a125fc>] (bus_probe_device+0xdc/0xec)
[ 9.261296] [<c0a12520>] (bus_probe_device) from [<c0a12e24>] (deferred_probe_work_func+0xd4/0x11c)
[ 9.261324] r9:c29d6380 r8:e4672e54 r7:c29d6380 r6:c29d6340 r5:c29d6340 r4:e472d810
[ 9.261356] [<c0a12d50>] (deferred_probe_work_func) from [<c025a534>] (process_one_work+0x420/0x8f0)
[ 9.261384] r10:e452fd84 r9:c2b53750 r8:00000000 r7:e452ae0c r6:e8a1e240 r5:e452ae00
[ 9.261404] r4:c29d6440
[ 9.261424] [<c025a114>] (process_one_work) from [<c025af00>] (worker_thread+0x4fc/0x91c)
[ 9.261450] r10:e8a1e258 r9:e452ae00 r8:e8a1e254 r7:e8a1e240 r6:e452fd84 r5:e8a1e254
[ 9.261470] r4:e452ae14
[ 9.261492] [<c025aa04>] (worker_thread) from [<c026591c>] (kthread+0x21c/0x22c)
[ 9.261517] r10:e455bce0 r9:e45b8d80 r8:e452ae00 r7:c025aa04 r6:e45b8d80 r5:00000000
[ 9.261537] r4:e45c0400
[ 9.261555] [<c0265700>] (kthread) from [<c0200114>] (ret_from_fork+0x14/0x20)
[ 9.261575] Exception stack(0xe466bfb0 to 0xe466bff8)
[ 9.261593] bfa0: 00000000 00000000 00000000 00000000
[ 9.261618] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 9.261641] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 9.261663] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0265700
[ 9.261683] r4:e4622080
[ 9.261728] kobject_add_internal failed for scmi_notify with -EEXIST, don't try to register things with the same name in the same directory.
[ 9.261805] arm-scmi brcm_scmi@1: SCMI Notifications - Initialization Failed.
[ 9.261847] arm-scmi brcm_scmi@1: SCMI Notifications NOT available.
[ 9.262026] arm-scmi brcm_scmi@1: SCMI Protocol v1.0 'brcm-scmi:' Firmware version 0x1
Fix this by using dev_name(handle->dev) which guarantees that the name is
unique and this also helps correlate which notification workqueue corresponds
to which SCMI device instance.
Fixes: bd31b249692e ("firmware: arm_scmi: Add notification dispatch and delivery")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/firmware/arm_scmi/notify.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
index 4731daaacd19..24c9ef232f48 100644
--- a/drivers/firmware/arm_scmi/notify.c
+++ b/drivers/firmware/arm_scmi/notify.c
@@ -1468,7 +1468,7 @@ int scmi_notification_init(struct scmi_handle *handle)
ni->gid = gid;
ni->handle = handle;
- ni->notify_wq = alloc_workqueue("scmi_notify",
+ ni->notify_wq = alloc_workqueue(dev_name(handle->dev),
WQ_UNBOUND | WQ_FREEZABLE | WQ_SYSFS,
0);
if (!ni->notify_wq)
--
2.25.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Fix duplicate workqueue name
2020-10-14 2:17 [PATCH] firmware: arm_scmi: Fix duplicate workqueue name Florian Fainelli
@ 2020-10-14 9:18 ` Sudeep Holla
2020-10-14 13:05 ` Sudeep Holla
2020-10-14 17:08 ` Florian Fainelli
2020-10-14 20:29 ` Sudeep Holla
1 sibling, 2 replies; 12+ messages in thread
From: Sudeep Holla @ 2020-10-14 9:18 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-arm-kernel, Jonathan Cameron, Cristian Marussi, linux-kernel
Hi Florian,
Thanks for the patch, it shows someone else is also using this and
testing 😉.
On Tue, Oct 13, 2020 at 07:17:37PM -0700, Florian Fainelli wrote:
> When more than a single SCMI device are present in the system, the
> creation of the notification workqueue with the WQ_SYSFS flag will lead
> to the following sysfs duplicate node warning:
>
Please trim the calltrace next time without timestamp and register raw
hex values.
You using this on 32-bit platform ? If so, thanks for additional test
coverage.
> [ 9.259990] sysfs: cannot create duplicate filename '/devices/virtual/workqueue/scmi_notify'
> [ 9.260024] CPU: 0 PID: 20 Comm: kworker/0:1 Not tainted 5.9.0-gdf4dd84a3f7d #29
> [ 9.260045] Hardware name: Broadcom STB (Flattened Device Tree)
> [ 9.260083] Workqueue: events deferred_probe_work_func
> [ 9.260099] Backtrace:
> [ 9.260127] [<c02120b4>] (dump_backtrace) from [<c02123d8>] (show_stack+0x20/0x24)
> [ 9.260155] r9:ffffffff r8:00000000 r7:c298e3c0 r6:60000013 r5:00000000 r4:c298e3c0
> [ 9.260186] [<c02123b8>] (show_stack) from [<c08852a0>] (dump_stack+0xbc/0xe0)
> [ 9.260216] [<c08851e4>] (dump_stack) from [<c054442c>] (sysfs_warn_dup+0x70/0x80)
> [ 9.260243] r10:e472d814 r9:ffffffef r8:e4614a50 r7:c2806d48 r6:e0e50f80 r5:e4614a50
> [ 9.260265] r4:e0e82000 r3:c5b17c34
> [ 9.260285] [<c05443bc>] (sysfs_warn_dup) from [<c05445fc>] (sysfs_create_dir_ns+0x15c/0x1a4)
> [ 9.260310] r7:c2806d48 r6:ffffffef r5:e0dff008 r4:bb8cd6e4
> [ 9.260335] [<c05444a0>] (sysfs_create_dir_ns) from [<c088ebac>] (kobject_add_internal+0x140/0x4d0)
> [ 9.260363] r9:e451fa80 r8:e0dff01c r7:e0dff014 r6:e45b1400 r5:e0dff018 r4:e0dff008
> [ 9.260390] [<c088ea6c>] (kobject_add_internal) from [<c088f004>] (kobject_add+0xc8/0x138)
> [ 9.260416] r10:e0dff030 r9:00000000 r8:e45b1400 r7:00000000 r6:c2806d48 r5:e0dff008
> [ 9.260436] r4:bb8cd708
> [ 9.260455] [<c088ef3c>] (kobject_add) from [<c0a0bc70>] (device_add+0x1dc/0xc20)
> [ 9.260476] r3:e0dff014 r2:00000000
> [ 9.260495] r9:00000000 r8:e466b980 r7:e45b1400 r6:c2806d48 r5:bb8cd724 r4:e0dff008
> [ 9.260521] [<c0a0ba94>] (device_add) from [<c0a0c6d8>] (device_register+0x24/0x28)
> [ 9.260547] r10:c2831340 r9:e0dfec08 r8:e0dff028 r7:e0dfecc0 r6:e0dfec00 r5:e0dff008
> [ 9.260567] r4:e0dff008
> [ 9.260586] [<c0a0c6b4>] (device_register) from [<c025f2a8>] (workqueue_sysfs_register+0xe4/0x1f0)
> [ 9.260609] r5:e0dff008 r4:e0dff000
> [ 9.260627] [<c025f1c4>] (workqueue_sysfs_register) from [<c025f7fc>] (alloc_workqueue+0x448/0x6ac)
> [ 9.260655] r10:e0dfecc0 r9:e0dfec08 r8:e0dfec0c r7:e0dfec04 r6:e0dfecc0 r5:e0dfec00
> [ 9.260675] r4:00000001
> [ 9.260698] [<c025f3b4>] (alloc_workqueue) from [<c0d8b58c>] (scmi_notification_init+0x78/0x1dc)
> [ 9.260720] r3:c0a1b488 r2:00000000
> [ 9.260739] r10:e0de1040 r9:00800004 r8:e0de10d8 r7:e0de10f4 r6:e0de1074 r5:e0e54740
> [ 9.260759] r4:e0e50d00
> [ 9.260778] [<c0d8b514>] (scmi_notification_init) from [<c0d87c00>] (scmi_probe+0x268/0x4fc)
> [ 9.260805] r9:00800004 r8:e0de10d8 r7:e0de10f4 r6:e0de1074 r5:e0de10f0 r4:e472d810
> [ 9.260833] [<c0d87998>] (scmi_probe) from [<c0a17d74>] (platform_drv_probe+0x70/0xc8)
> [ 9.260859] r10:e472d844 r9:c2ab02b4 r8:c2b5e298 r7:00000000 r6:c2ab02b4 r5:00000000
> [ 9.260879] r4:e472d810
> [ 9.260898] [<c0a17d04>] (platform_drv_probe) from [<c0a13e80>] (really_probe+0x184/0x728)
> [ 9.260922] r7:00000000 r6:c2fc82a0 r5:c2fc8220 r4:e472d810
> [ 9.260946] [<c0a13cfc>] (really_probe) from [<c0a146b4>] (driver_probe_device+0xa4/0x278)
> [ 9.260973] r10:e0cef438 r9:c0a14a04 r8:c2b771c0 r7:c2b5e298 r6:c2ab02b4 r5:e472d834
> [ 9.260993] r4:e472d810
> [ 9.261014] [<c0a14610>] (driver_probe_device) from [<c0a14aec>] (__device_attach_driver+0xe8/0x148)
> [ 9.261042] r10:e0cef438 r9:c0a14a04 r8:e466bdc0 r7:e472d810 r6:00000000 r5:c2ab02b4
> [ 9.261062] r4:e466bdc0
> [ 9.261082] [<c0a14a04>] (__device_attach_driver) from [<c0a10f6c>] (bus_for_each_drv+0x108/0x158)
> [ 9.261107] r7:c2806d48 r6:00000000 r5:bb8cd79c r4:e466bd00
> [ 9.261131] [<c0a10e64>] (bus_for_each_drv) from [<c0a13c38>] (__device_attach+0x190/0x234)
> [ 9.261158] r10:e46fde00 r9:e472d848 r8:e472d854 r7:e466be00 r6:c2806d48 r5:bb8cd7b4
> [ 9.261178] r4:e472d810
> [ 9.261199] [<c0a13aa8>] (__device_attach) from [<c0a14b68>] (device_initial_probe+0x1c/0x20)
> [ 9.261226] r10:e46fde54 r9:c29d6380 r8:c29d7910 r7:e472d810 r6:c29d78c0 r5:c29d6340
> [ 9.261246] r4:e44efe00
> [ 9.261266] [<c0a14b4c>] (device_initial_probe) from [<c0a125fc>] (bus_probe_device+0xdc/0xec)
> [ 9.261296] [<c0a12520>] (bus_probe_device) from [<c0a12e24>] (deferred_probe_work_func+0xd4/0x11c)
> [ 9.261324] r9:c29d6380 r8:e4672e54 r7:c29d6380 r6:c29d6340 r5:c29d6340 r4:e472d810
> [ 9.261356] [<c0a12d50>] (deferred_probe_work_func) from [<c025a534>] (process_one_work+0x420/0x8f0)
> [ 9.261384] r10:e452fd84 r9:c2b53750 r8:00000000 r7:e452ae0c r6:e8a1e240 r5:e452ae00
> [ 9.261404] r4:c29d6440
> [ 9.261424] [<c025a114>] (process_one_work) from [<c025af00>] (worker_thread+0x4fc/0x91c)
> [ 9.261450] r10:e8a1e258 r9:e452ae00 r8:e8a1e254 r7:e8a1e240 r6:e452fd84 r5:e8a1e254
> [ 9.261470] r4:e452ae14
> [ 9.261492] [<c025aa04>] (worker_thread) from [<c026591c>] (kthread+0x21c/0x22c)
> [ 9.261517] r10:e455bce0 r9:e45b8d80 r8:e452ae00 r7:c025aa04 r6:e45b8d80 r5:00000000
> [ 9.261537] r4:e45c0400
> [ 9.261555] [<c0265700>] (kthread) from [<c0200114>] (ret_from_fork+0x14/0x20)
> [ 9.261575] Exception stack(0xe466bfb0 to 0xe466bff8)
> [ 9.261593] bfa0: 00000000 00000000 00000000 00000000
> [ 9.261618] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [ 9.261641] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [ 9.261663] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0265700
> [ 9.261683] r4:e4622080
> [ 9.261728] kobject_add_internal failed for scmi_notify with -EEXIST, don't try to register things with the same name in the same directory.
> [ 9.261805] arm-scmi brcm_scmi@1: SCMI Notifications - Initialization Failed.
> [ 9.261847] arm-scmi brcm_scmi@1: SCMI Notifications NOT available.
> [ 9.262026] arm-scmi brcm_scmi@1: SCMI Protocol v1.0 'brcm-scmi:' Firmware version 0x1
>
> Fix this by using dev_name(handle->dev) which guarantees that the name is
> unique and this also helps correlate which notification workqueue corresponds
> to which SCMI device instance.
>
I am curious as how multiple SCMI instances are used. We know few limitations
in the code to handle that yet, so interested to know if you are carrying
more patches/fixes.
> Fixes: bd31b249692e ("firmware: arm_scmi: Add notification dispatch and delivery")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> drivers/firmware/arm_scmi/notify.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
> index 4731daaacd19..24c9ef232f48 100644
> --- a/drivers/firmware/arm_scmi/notify.c
> +++ b/drivers/firmware/arm_scmi/notify.c
> @@ -1468,7 +1468,7 @@ int scmi_notification_init(struct scmi_handle *handle)
> ni->gid = gid;
> ni->handle = handle;
>
> - ni->notify_wq = alloc_workqueue("scmi_notify",
> + ni->notify_wq = alloc_workqueue(dev_name(handle->dev),
Not sure if it makes sense to add "_notify" to clearly identify the workqueue.
> WQ_UNBOUND | WQ_FREEZABLE | WQ_SYSFS,
> 0);
> if (!ni->notify_wq)
> --
> 2.25.1
>
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Fix duplicate workqueue name
2020-10-14 9:18 ` Sudeep Holla
@ 2020-10-14 13:05 ` Sudeep Holla
2020-10-14 13:48 ` Cristian Marussi
2020-10-14 17:08 ` Florian Fainelli
1 sibling, 1 reply; 12+ messages in thread
From: Sudeep Holla @ 2020-10-14 13:05 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-kernel, Cristian Marussi, linux-arm-kernel, Sudeep Holla,
Jonathan Cameron
On Wed, Oct 14, 2020 at 10:18:06AM +0100, Sudeep Holla wrote:
> Hi Florian,
>
> Thanks for the patch, it shows someone else is also using this and
> testing 😉.
>
> On Tue, Oct 13, 2020 at 07:17:37PM -0700, Florian Fainelli wrote:
> > When more than a single SCMI device are present in the system, the
> > creation of the notification workqueue with the WQ_SYSFS flag will lead
> > to the following sysfs duplicate node warning:
> >
>
> Please trim the calltrace next time without timestamp and register raw
> hex values.
>
> You using this on 32-bit platform ? If so, thanks for additional test
> coverage.
>
> > [ 9.259990] sysfs: cannot create duplicate filename '/devices/virtual/workqueue/scmi_notify'
> > [ 9.260024] CPU: 0 PID: 20 Comm: kworker/0:1 Not tainted 5.9.0-gdf4dd84a3f7d #29
> > [ 9.260045] Hardware name: Broadcom STB (Flattened Device Tree)
> > [ 9.260083] Workqueue: events deferred_probe_work_func
> > [ 9.260099] Backtrace:
> > [ 9.260127] [<c02120b4>] (dump_backtrace) from [<c02123d8>] (show_stack+0x20/0x24)
> > [ 9.260155] r9:ffffffff r8:00000000 r7:c298e3c0 r6:60000013 r5:00000000 r4:c298e3c0
> > [ 9.260186] [<c02123b8>] (show_stack) from [<c08852a0>] (dump_stack+0xbc/0xe0)
> > [ 9.260216] [<c08851e4>] (dump_stack) from [<c054442c>] (sysfs_warn_dup+0x70/0x80)
> > [ 9.260243] r10:e472d814 r9:ffffffef r8:e4614a50 r7:c2806d48 r6:e0e50f80 r5:e4614a50
> > [ 9.260265] r4:e0e82000 r3:c5b17c34
> > [ 9.260285] [<c05443bc>] (sysfs_warn_dup) from [<c05445fc>] (sysfs_create_dir_ns+0x15c/0x1a4)
> > [ 9.260310] r7:c2806d48 r6:ffffffef r5:e0dff008 r4:bb8cd6e4
> > [ 9.260335] [<c05444a0>] (sysfs_create_dir_ns) from [<c088ebac>] (kobject_add_internal+0x140/0x4d0)
> > [ 9.260363] r9:e451fa80 r8:e0dff01c r7:e0dff014 r6:e45b1400 r5:e0dff018 r4:e0dff008
> > [ 9.260390] [<c088ea6c>] (kobject_add_internal) from [<c088f004>] (kobject_add+0xc8/0x138)
> > [ 9.260416] r10:e0dff030 r9:00000000 r8:e45b1400 r7:00000000 r6:c2806d48 r5:e0dff008
> > [ 9.260436] r4:bb8cd708
> > [ 9.260455] [<c088ef3c>] (kobject_add) from [<c0a0bc70>] (device_add+0x1dc/0xc20)
> > [ 9.260476] r3:e0dff014 r2:00000000
> > [ 9.260495] r9:00000000 r8:e466b980 r7:e45b1400 r6:c2806d48 r5:bb8cd724 r4:e0dff008
> > [ 9.260521] [<c0a0ba94>] (device_add) from [<c0a0c6d8>] (device_register+0x24/0x28)
> > [ 9.260547] r10:c2831340 r9:e0dfec08 r8:e0dff028 r7:e0dfecc0 r6:e0dfec00 r5:e0dff008
> > [ 9.260567] r4:e0dff008
> > [ 9.260586] [<c0a0c6b4>] (device_register) from [<c025f2a8>] (workqueue_sysfs_register+0xe4/0x1f0)
> > [ 9.260609] r5:e0dff008 r4:e0dff000
> > [ 9.260627] [<c025f1c4>] (workqueue_sysfs_register) from [<c025f7fc>] (alloc_workqueue+0x448/0x6ac)
> > [ 9.260655] r10:e0dfecc0 r9:e0dfec08 r8:e0dfec0c r7:e0dfec04 r6:e0dfecc0 r5:e0dfec00
> > [ 9.260675] r4:00000001
> > [ 9.260698] [<c025f3b4>] (alloc_workqueue) from [<c0d8b58c>] (scmi_notification_init+0x78/0x1dc)
> > [ 9.260720] r3:c0a1b488 r2:00000000
> > [ 9.260739] r10:e0de1040 r9:00800004 r8:e0de10d8 r7:e0de10f4 r6:e0de1074 r5:e0e54740
> > [ 9.260759] r4:e0e50d00
> > [ 9.260778] [<c0d8b514>] (scmi_notification_init) from [<c0d87c00>] (scmi_probe+0x268/0x4fc)
> > [ 9.260805] r9:00800004 r8:e0de10d8 r7:e0de10f4 r6:e0de1074 r5:e0de10f0 r4:e472d810
> > [ 9.260833] [<c0d87998>] (scmi_probe) from [<c0a17d74>] (platform_drv_probe+0x70/0xc8)
> > [ 9.260859] r10:e472d844 r9:c2ab02b4 r8:c2b5e298 r7:00000000 r6:c2ab02b4 r5:00000000
> > [ 9.260879] r4:e472d810
> > [ 9.260898] [<c0a17d04>] (platform_drv_probe) from [<c0a13e80>] (really_probe+0x184/0x728)
> > [ 9.260922] r7:00000000 r6:c2fc82a0 r5:c2fc8220 r4:e472d810
> > [ 9.260946] [<c0a13cfc>] (really_probe) from [<c0a146b4>] (driver_probe_device+0xa4/0x278)
> > [ 9.260973] r10:e0cef438 r9:c0a14a04 r8:c2b771c0 r7:c2b5e298 r6:c2ab02b4 r5:e472d834
> > [ 9.260993] r4:e472d810
> > [ 9.261014] [<c0a14610>] (driver_probe_device) from [<c0a14aec>] (__device_attach_driver+0xe8/0x148)
> > [ 9.261042] r10:e0cef438 r9:c0a14a04 r8:e466bdc0 r7:e472d810 r6:00000000 r5:c2ab02b4
> > [ 9.261062] r4:e466bdc0
> > [ 9.261082] [<c0a14a04>] (__device_attach_driver) from [<c0a10f6c>] (bus_for_each_drv+0x108/0x158)
> > [ 9.261107] r7:c2806d48 r6:00000000 r5:bb8cd79c r4:e466bd00
> > [ 9.261131] [<c0a10e64>] (bus_for_each_drv) from [<c0a13c38>] (__device_attach+0x190/0x234)
> > [ 9.261158] r10:e46fde00 r9:e472d848 r8:e472d854 r7:e466be00 r6:c2806d48 r5:bb8cd7b4
> > [ 9.261178] r4:e472d810
> > [ 9.261199] [<c0a13aa8>] (__device_attach) from [<c0a14b68>] (device_initial_probe+0x1c/0x20)
> > [ 9.261226] r10:e46fde54 r9:c29d6380 r8:c29d7910 r7:e472d810 r6:c29d78c0 r5:c29d6340
> > [ 9.261246] r4:e44efe00
> > [ 9.261266] [<c0a14b4c>] (device_initial_probe) from [<c0a125fc>] (bus_probe_device+0xdc/0xec)
> > [ 9.261296] [<c0a12520>] (bus_probe_device) from [<c0a12e24>] (deferred_probe_work_func+0xd4/0x11c)
> > [ 9.261324] r9:c29d6380 r8:e4672e54 r7:c29d6380 r6:c29d6340 r5:c29d6340 r4:e472d810
> > [ 9.261356] [<c0a12d50>] (deferred_probe_work_func) from [<c025a534>] (process_one_work+0x420/0x8f0)
> > [ 9.261384] r10:e452fd84 r9:c2b53750 r8:00000000 r7:e452ae0c r6:e8a1e240 r5:e452ae00
> > [ 9.261404] r4:c29d6440
> > [ 9.261424] [<c025a114>] (process_one_work) from [<c025af00>] (worker_thread+0x4fc/0x91c)
> > [ 9.261450] r10:e8a1e258 r9:e452ae00 r8:e8a1e254 r7:e8a1e240 r6:e452fd84 r5:e8a1e254
> > [ 9.261470] r4:e452ae14
> > [ 9.261492] [<c025aa04>] (worker_thread) from [<c026591c>] (kthread+0x21c/0x22c)
> > [ 9.261517] r10:e455bce0 r9:e45b8d80 r8:e452ae00 r7:c025aa04 r6:e45b8d80 r5:00000000
> > [ 9.261537] r4:e45c0400
> > [ 9.261555] [<c0265700>] (kthread) from [<c0200114>] (ret_from_fork+0x14/0x20)
> > [ 9.261575] Exception stack(0xe466bfb0 to 0xe466bff8)
> > [ 9.261593] bfa0: 00000000 00000000 00000000 00000000
> > [ 9.261618] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> > [ 9.261641] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > [ 9.261663] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0265700
> > [ 9.261683] r4:e4622080
> > [ 9.261728] kobject_add_internal failed for scmi_notify with -EEXIST, don't try to register things with the same name in the same directory.
> > [ 9.261805] arm-scmi brcm_scmi@1: SCMI Notifications - Initialization Failed.
> > [ 9.261847] arm-scmi brcm_scmi@1: SCMI Notifications NOT available.
> > [ 9.262026] arm-scmi brcm_scmi@1: SCMI Protocol v1.0 'brcm-scmi:' Firmware version 0x1
> >
> > Fix this by using dev_name(handle->dev) which guarantees that the name is
> > unique and this also helps correlate which notification workqueue corresponds
> > to which SCMI device instance.
> >
>
> I am curious as how multiple SCMI instances are used. We know few limitations
> in the code to handle that yet, so interested to know if you are carrying
> more patches/fixes.
>
> > Fixes: bd31b249692e ("firmware: arm_scmi: Add notification dispatch and delivery")
> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > ---
> > drivers/firmware/arm_scmi/notify.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
> > index 4731daaacd19..24c9ef232f48 100644
> > --- a/drivers/firmware/arm_scmi/notify.c
> > +++ b/drivers/firmware/arm_scmi/notify.c
> > @@ -1468,7 +1468,7 @@ int scmi_notification_init(struct scmi_handle *handle)
> > ni->gid = gid;
> > ni->handle = handle;
> >
> > - ni->notify_wq = alloc_workqueue("scmi_notify",
> > + ni->notify_wq = alloc_workqueue(dev_name(handle->dev),
>
> Not sure if it makes sense to add "_notify" to clearly identify the workqueue.
>
I have pushed a version with above change [1], please check if you are
happy with that ?
--
Regards,
Sudeep
[1] https://git.kernel.org/sudeep.holla/linux/c/b2cd15549b
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Fix duplicate workqueue name
2020-10-14 13:05 ` Sudeep Holla
@ 2020-10-14 13:48 ` Cristian Marussi
2020-10-14 16:18 ` Sudeep Holla
0 siblings, 1 reply; 12+ messages in thread
From: Cristian Marussi @ 2020-10-14 13:48 UTC (permalink / raw)
To: Sudeep Holla
Cc: Florian Fainelli, linux-kernel, linux-arm-kernel, Jonathan Cameron
Hi
On Wed, Oct 14, 2020 at 02:05:24PM +0100, Sudeep Holla wrote:
> On Wed, Oct 14, 2020 at 10:18:06AM +0100, Sudeep Holla wrote:
> > Hi Florian,
> >
> > Thanks for the patch, it shows someone else is also using this and
> > testing 😉.
> >
> > On Tue, Oct 13, 2020 at 07:17:37PM -0700, Florian Fainelli wrote:
> > > When more than a single SCMI device are present in the system, the
> > > creation of the notification workqueue with the WQ_SYSFS flag will lead
> > > to the following sysfs duplicate node warning:
> > >
> >
> > Please trim the calltrace next time without timestamp and register raw
> > hex values.
> >
> > You using this on 32-bit platform ? If so, thanks for additional test
> > coverage.
> >
> > > [ 9.259990] sysfs: cannot create duplicate filename '/devices/virtual/workqueue/scmi_notify'
> > > [ 9.260024] CPU: 0 PID: 20 Comm: kworker/0:1 Not tainted 5.9.0-gdf4dd84a3f7d #29
> > > [ 9.260045] Hardware name: Broadcom STB (Flattened Device Tree)
> > > [ 9.260083] Workqueue: events deferred_probe_work_func
> > > [ 9.260099] Backtrace:
> > > [ 9.260127] [<c02120b4>] (dump_backtrace) from [<c02123d8>] (show_stack+0x20/0x24)
> > > [ 9.260155] r9:ffffffff r8:00000000 r7:c298e3c0 r6:60000013 r5:00000000 r4:c298e3c0
> > > [ 9.260186] [<c02123b8>] (show_stack) from [<c08852a0>] (dump_stack+0xbc/0xe0)
> > > [ 9.260216] [<c08851e4>] (dump_stack) from [<c054442c>] (sysfs_warn_dup+0x70/0x80)
> > > [ 9.260243] r10:e472d814 r9:ffffffef r8:e4614a50 r7:c2806d48 r6:e0e50f80 r5:e4614a50
> > > [ 9.260265] r4:e0e82000 r3:c5b17c34
> > > [ 9.260285] [<c05443bc>] (sysfs_warn_dup) from [<c05445fc>] (sysfs_create_dir_ns+0x15c/0x1a4)
> > > [ 9.260310] r7:c2806d48 r6:ffffffef r5:e0dff008 r4:bb8cd6e4
> > > [ 9.260335] [<c05444a0>] (sysfs_create_dir_ns) from [<c088ebac>] (kobject_add_internal+0x140/0x4d0)
> > > [ 9.260363] r9:e451fa80 r8:e0dff01c r7:e0dff014 r6:e45b1400 r5:e0dff018 r4:e0dff008
> > > [ 9.260390] [<c088ea6c>] (kobject_add_internal) from [<c088f004>] (kobject_add+0xc8/0x138)
> > > [ 9.260416] r10:e0dff030 r9:00000000 r8:e45b1400 r7:00000000 r6:c2806d48 r5:e0dff008
> > > [ 9.260436] r4:bb8cd708
> > > [ 9.260455] [<c088ef3c>] (kobject_add) from [<c0a0bc70>] (device_add+0x1dc/0xc20)
> > > [ 9.260476] r3:e0dff014 r2:00000000
> > > [ 9.260495] r9:00000000 r8:e466b980 r7:e45b1400 r6:c2806d48 r5:bb8cd724 r4:e0dff008
> > > [ 9.260521] [<c0a0ba94>] (device_add) from [<c0a0c6d8>] (device_register+0x24/0x28)
> > > [ 9.260547] r10:c2831340 r9:e0dfec08 r8:e0dff028 r7:e0dfecc0 r6:e0dfec00 r5:e0dff008
> > > [ 9.260567] r4:e0dff008
> > > [ 9.260586] [<c0a0c6b4>] (device_register) from [<c025f2a8>] (workqueue_sysfs_register+0xe4/0x1f0)
> > > [ 9.260609] r5:e0dff008 r4:e0dff000
> > > [ 9.260627] [<c025f1c4>] (workqueue_sysfs_register) from [<c025f7fc>] (alloc_workqueue+0x448/0x6ac)
> > > [ 9.260655] r10:e0dfecc0 r9:e0dfec08 r8:e0dfec0c r7:e0dfec04 r6:e0dfecc0 r5:e0dfec00
> > > [ 9.260675] r4:00000001
> > > [ 9.260698] [<c025f3b4>] (alloc_workqueue) from [<c0d8b58c>] (scmi_notification_init+0x78/0x1dc)
> > > [ 9.260720] r3:c0a1b488 r2:00000000
> > > [ 9.260739] r10:e0de1040 r9:00800004 r8:e0de10d8 r7:e0de10f4 r6:e0de1074 r5:e0e54740
> > > [ 9.260759] r4:e0e50d00
> > > [ 9.260778] [<c0d8b514>] (scmi_notification_init) from [<c0d87c00>] (scmi_probe+0x268/0x4fc)
> > > [ 9.260805] r9:00800004 r8:e0de10d8 r7:e0de10f4 r6:e0de1074 r5:e0de10f0 r4:e472d810
> > > [ 9.260833] [<c0d87998>] (scmi_probe) from [<c0a17d74>] (platform_drv_probe+0x70/0xc8)
> > > [ 9.260859] r10:e472d844 r9:c2ab02b4 r8:c2b5e298 r7:00000000 r6:c2ab02b4 r5:00000000
> > > [ 9.260879] r4:e472d810
> > > [ 9.260898] [<c0a17d04>] (platform_drv_probe) from [<c0a13e80>] (really_probe+0x184/0x728)
> > > [ 9.260922] r7:00000000 r6:c2fc82a0 r5:c2fc8220 r4:e472d810
> > > [ 9.260946] [<c0a13cfc>] (really_probe) from [<c0a146b4>] (driver_probe_device+0xa4/0x278)
> > > [ 9.260973] r10:e0cef438 r9:c0a14a04 r8:c2b771c0 r7:c2b5e298 r6:c2ab02b4 r5:e472d834
> > > [ 9.260993] r4:e472d810
> > > [ 9.261014] [<c0a14610>] (driver_probe_device) from [<c0a14aec>] (__device_attach_driver+0xe8/0x148)
> > > [ 9.261042] r10:e0cef438 r9:c0a14a04 r8:e466bdc0 r7:e472d810 r6:00000000 r5:c2ab02b4
> > > [ 9.261062] r4:e466bdc0
> > > [ 9.261082] [<c0a14a04>] (__device_attach_driver) from [<c0a10f6c>] (bus_for_each_drv+0x108/0x158)
> > > [ 9.261107] r7:c2806d48 r6:00000000 r5:bb8cd79c r4:e466bd00
> > > [ 9.261131] [<c0a10e64>] (bus_for_each_drv) from [<c0a13c38>] (__device_attach+0x190/0x234)
> > > [ 9.261158] r10:e46fde00 r9:e472d848 r8:e472d854 r7:e466be00 r6:c2806d48 r5:bb8cd7b4
> > > [ 9.261178] r4:e472d810
> > > [ 9.261199] [<c0a13aa8>] (__device_attach) from [<c0a14b68>] (device_initial_probe+0x1c/0x20)
> > > [ 9.261226] r10:e46fde54 r9:c29d6380 r8:c29d7910 r7:e472d810 r6:c29d78c0 r5:c29d6340
> > > [ 9.261246] r4:e44efe00
> > > [ 9.261266] [<c0a14b4c>] (device_initial_probe) from [<c0a125fc>] (bus_probe_device+0xdc/0xec)
> > > [ 9.261296] [<c0a12520>] (bus_probe_device) from [<c0a12e24>] (deferred_probe_work_func+0xd4/0x11c)
> > > [ 9.261324] r9:c29d6380 r8:e4672e54 r7:c29d6380 r6:c29d6340 r5:c29d6340 r4:e472d810
> > > [ 9.261356] [<c0a12d50>] (deferred_probe_work_func) from [<c025a534>] (process_one_work+0x420/0x8f0)
> > > [ 9.261384] r10:e452fd84 r9:c2b53750 r8:00000000 r7:e452ae0c r6:e8a1e240 r5:e452ae00
> > > [ 9.261404] r4:c29d6440
> > > [ 9.261424] [<c025a114>] (process_one_work) from [<c025af00>] (worker_thread+0x4fc/0x91c)
> > > [ 9.261450] r10:e8a1e258 r9:e452ae00 r8:e8a1e254 r7:e8a1e240 r6:e452fd84 r5:e8a1e254
> > > [ 9.261470] r4:e452ae14
> > > [ 9.261492] [<c025aa04>] (worker_thread) from [<c026591c>] (kthread+0x21c/0x22c)
> > > [ 9.261517] r10:e455bce0 r9:e45b8d80 r8:e452ae00 r7:c025aa04 r6:e45b8d80 r5:00000000
> > > [ 9.261537] r4:e45c0400
> > > [ 9.261555] [<c0265700>] (kthread) from [<c0200114>] (ret_from_fork+0x14/0x20)
> > > [ 9.261575] Exception stack(0xe466bfb0 to 0xe466bff8)
> > > [ 9.261593] bfa0: 00000000 00000000 00000000 00000000
> > > [ 9.261618] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> > > [ 9.261641] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > > [ 9.261663] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0265700
> > > [ 9.261683] r4:e4622080
> > > [ 9.261728] kobject_add_internal failed for scmi_notify with -EEXIST, don't try to register things with the same name in the same directory.
> > > [ 9.261805] arm-scmi brcm_scmi@1: SCMI Notifications - Initialization Failed.
> > > [ 9.261847] arm-scmi brcm_scmi@1: SCMI Notifications NOT available.
> > > [ 9.262026] arm-scmi brcm_scmi@1: SCMI Protocol v1.0 'brcm-scmi:' Firmware version 0x1
> > >
> > > Fix this by using dev_name(handle->dev) which guarantees that the name is
> > > unique and this also helps correlate which notification workqueue corresponds
> > > to which SCMI device instance.
> > >
> >
> > I am curious as how multiple SCMI instances are used. We know few limitations
> > in the code to handle that yet, so interested to know if you are carrying
> > more patches/fixes.
> >
> > > Fixes: bd31b249692e ("firmware: arm_scmi: Add notification dispatch and delivery")
> > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > > ---
> > > drivers/firmware/arm_scmi/notify.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
> > > index 4731daaacd19..24c9ef232f48 100644
> > > --- a/drivers/firmware/arm_scmi/notify.c
> > > +++ b/drivers/firmware/arm_scmi/notify.c
> > > @@ -1468,7 +1468,7 @@ int scmi_notification_init(struct scmi_handle *handle)
> > > ni->gid = gid;
> > > ni->handle = handle;
> > >
> > > - ni->notify_wq = alloc_workqueue("scmi_notify",
> > > + ni->notify_wq = alloc_workqueue(dev_name(handle->dev),
> >
> > Not sure if it makes sense to add "_notify" to clearly identify the workqueue.
> >
>
> I have pushed a version with above change [1], please check if you are
> happy with that ?
>
> --
> Regards,
> Sudeep
>
> [1] https://git.kernel.org/sudeep.holla/linux/c/b2cd15549b
I agree with the need to retain _notify name, but I'm not so sure about
the above patch...which is:
diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
index c24e427dce0d7..f60fa630dd98f 100644
--- a/drivers/firmware/arm_scmi/notify.c
+++ b/drivers/firmware/arm_scmi/notify.c
@@ -1461,6 +1461,7 @@ static const struct scmi_notify_ops notify_ops = {
int scmi_notification_init(struct scmi_handle *handle)
{
void *gid;
+ char scmi_wq_name[32];
struct scmi_notify_instance *ni;
gid = devres_open_group(handle->dev, NULL, GFP_KERNEL);
@@ -1474,7 +1475,8 @@ int scmi_notification_init(struct scmi_handle *handle)
ni->gid = gid;
ni->handle = handle;
- ni->notify_wq = alloc_workqueue("scmi_notify",
+ sprintf(scmi_wq_name, "%s_notify", dev_name(handle->dev));
+ ni->notify_wq = alloc_workqueue(scmi_wq_name,
WQ_UNBOUND | WQ_FREEZABLE | WQ_SYSFS,
0);
if (!ni->notify_wq)
...does not expose a potential buffer overflow given that the dev_name now comes
from the DT chosen name for this SCMI server instance ?
I'd use snprintf and enlarge a bit the static scmi_wq_name[] to fit a max
device bane plus "_notify".
Regards
Cristian
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Fix duplicate workqueue name
2020-10-14 13:48 ` Cristian Marussi
@ 2020-10-14 16:18 ` Sudeep Holla
2020-10-14 17:13 ` Florian Fainelli
0 siblings, 1 reply; 12+ messages in thread
From: Sudeep Holla @ 2020-10-14 16:18 UTC (permalink / raw)
To: Cristian Marussi
Cc: Florian Fainelli, linux-kernel, linux-arm-kernel, Jonathan Cameron
On Wed, Oct 14, 2020 at 02:48:19PM +0100, Cristian Marussi wrote:
[...]
> >
> > I have pushed a version with above change [1], please check if you are
> > happy with that ?
> >
> > [1] https://git.kernel.org/sudeep.holla/linux/c/b2cd15549b
>
> I agree with the need to retain _notify name, but I'm not so sure about
> the above patch...which is:
>
I agree, I thought about it and just cooked up this as a quick solution.
I will move to that, even I wasn't happy with this TBH.
> diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
> index c24e427dce0d7..f60fa630dd98f 100644
> --- a/drivers/firmware/arm_scmi/notify.c
> +++ b/drivers/firmware/arm_scmi/notify.c
> @@ -1461,6 +1461,7 @@ static const struct scmi_notify_ops notify_ops = {
> int scmi_notification_init(struct scmi_handle *handle)
> {
> void *gid;
> + char scmi_wq_name[32];
> struct scmi_notify_instance *ni;
>
> gid = devres_open_group(handle->dev, NULL, GFP_KERNEL);
> @@ -1474,7 +1475,8 @@ int scmi_notification_init(struct scmi_handle *handle)
> ni->gid = gid;
> ni->handle = handle;
>
> - ni->notify_wq = alloc_workqueue("scmi_notify",
> + sprintf(scmi_wq_name, "%s_notify", dev_name(handle->dev));
> + ni->notify_wq = alloc_workqueue(scmi_wq_name,
> WQ_UNBOUND | WQ_FREEZABLE | WQ_SYSFS,
> 0);
> if (!ni->notify_wq)
>
> ...does not expose a potential buffer overflow given that the dev_name now comes
> from the DT chosen name for this SCMI server instance ?
>
> I'd use snprintf and enlarge a bit the static scmi_wq_name[] to fit a max
> device bane plus "_notify".
>
> Regards
> Cristian
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Fix duplicate workqueue name
2020-10-14 9:18 ` Sudeep Holla
2020-10-14 13:05 ` Sudeep Holla
@ 2020-10-14 17:08 ` Florian Fainelli
2020-10-14 17:32 ` Sudeep Holla
1 sibling, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2020-10-14 17:08 UTC (permalink / raw)
To: Sudeep Holla
Cc: linux-arm-kernel, Jonathan Cameron, Cristian Marussi, linux-kernel
On 10/14/20 2:18 AM, Sudeep Holla wrote:
> Hi Florian,
>
> Thanks for the patch, it shows someone else is also using this and
> testing 😉.
>
> On Tue, Oct 13, 2020 at 07:17:37PM -0700, Florian Fainelli wrote:
>> When more than a single SCMI device are present in the system, the
>> creation of the notification workqueue with the WQ_SYSFS flag will lead
>> to the following sysfs duplicate node warning:
>>
>
> Please trim the calltrace next time without timestamp and register raw
> hex values.
Will do, thanks!
>
> You using this on 32-bit platform ? If so, thanks for additional test
> coverage.
We have a mix of ARMv7/LPAE (Brahma-B15) and ARMv8 (Brahma-B53,
Cortex-A72) devices that we regularly test with 32-bit and 64-bit kernels.
>
>> [ 9.259990] sysfs: cannot create duplicate filename '/devices/virtual/workqueue/scmi_notify'
>> [ 9.260024] CPU: 0 PID: 20 Comm: kworker/0:1 Not tainted 5.9.0-gdf4dd84a3f7d #29
>> [ 9.260045] Hardware name: Broadcom STB (Flattened Device Tree)
>> [ 9.260083] Workqueue: events deferred_probe_work_func
>> [ 9.260099] Backtrace:
>> [ 9.260127] [<c02120b4>] (dump_backtrace) from [<c02123d8>] (show_stack+0x20/0x24)
>> [ 9.260155] r9:ffffffff r8:00000000 r7:c298e3c0 r6:60000013 r5:00000000 r4:c298e3c0
>> [ 9.260186] [<c02123b8>] (show_stack) from [<c08852a0>] (dump_stack+0xbc/0xe0)
>> [ 9.260216] [<c08851e4>] (dump_stack) from [<c054442c>] (sysfs_warn_dup+0x70/0x80)
>> [ 9.260243] r10:e472d814 r9:ffffffef r8:e4614a50 r7:c2806d48 r6:e0e50f80 r5:e4614a50
>> [ 9.260265] r4:e0e82000 r3:c5b17c34
>> [ 9.260285] [<c05443bc>] (sysfs_warn_dup) from [<c05445fc>] (sysfs_create_dir_ns+0x15c/0x1a4)
>> [ 9.260310] r7:c2806d48 r6:ffffffef r5:e0dff008 r4:bb8cd6e4
>> [ 9.260335] [<c05444a0>] (sysfs_create_dir_ns) from [<c088ebac>] (kobject_add_internal+0x140/0x4d0)
>> [ 9.260363] r9:e451fa80 r8:e0dff01c r7:e0dff014 r6:e45b1400 r5:e0dff018 r4:e0dff008
>> [ 9.260390] [<c088ea6c>] (kobject_add_internal) from [<c088f004>] (kobject_add+0xc8/0x138)
>> [ 9.260416] r10:e0dff030 r9:00000000 r8:e45b1400 r7:00000000 r6:c2806d48 r5:e0dff008
>> [ 9.260436] r4:bb8cd708
>> [ 9.260455] [<c088ef3c>] (kobject_add) from [<c0a0bc70>] (device_add+0x1dc/0xc20)
>> [ 9.260476] r3:e0dff014 r2:00000000
>> [ 9.260495] r9:00000000 r8:e466b980 r7:e45b1400 r6:c2806d48 r5:bb8cd724 r4:e0dff008
>> [ 9.260521] [<c0a0ba94>] (device_add) from [<c0a0c6d8>] (device_register+0x24/0x28)
>> [ 9.260547] r10:c2831340 r9:e0dfec08 r8:e0dff028 r7:e0dfecc0 r6:e0dfec00 r5:e0dff008
>> [ 9.260567] r4:e0dff008
>> [ 9.260586] [<c0a0c6b4>] (device_register) from [<c025f2a8>] (workqueue_sysfs_register+0xe4/0x1f0)
>> [ 9.260609] r5:e0dff008 r4:e0dff000
>> [ 9.260627] [<c025f1c4>] (workqueue_sysfs_register) from [<c025f7fc>] (alloc_workqueue+0x448/0x6ac)
>> [ 9.260655] r10:e0dfecc0 r9:e0dfec08 r8:e0dfec0c r7:e0dfec04 r6:e0dfecc0 r5:e0dfec00
>> [ 9.260675] r4:00000001
>> [ 9.260698] [<c025f3b4>] (alloc_workqueue) from [<c0d8b58c>] (scmi_notification_init+0x78/0x1dc)
>> [ 9.260720] r3:c0a1b488 r2:00000000
>> [ 9.260739] r10:e0de1040 r9:00800004 r8:e0de10d8 r7:e0de10f4 r6:e0de1074 r5:e0e54740
>> [ 9.260759] r4:e0e50d00
>> [ 9.260778] [<c0d8b514>] (scmi_notification_init) from [<c0d87c00>] (scmi_probe+0x268/0x4fc)
>> [ 9.260805] r9:00800004 r8:e0de10d8 r7:e0de10f4 r6:e0de1074 r5:e0de10f0 r4:e472d810
>> [ 9.260833] [<c0d87998>] (scmi_probe) from [<c0a17d74>] (platform_drv_probe+0x70/0xc8)
>> [ 9.260859] r10:e472d844 r9:c2ab02b4 r8:c2b5e298 r7:00000000 r6:c2ab02b4 r5:00000000
>> [ 9.260879] r4:e472d810
>> [ 9.260898] [<c0a17d04>] (platform_drv_probe) from [<c0a13e80>] (really_probe+0x184/0x728)
>> [ 9.260922] r7:00000000 r6:c2fc82a0 r5:c2fc8220 r4:e472d810
>> [ 9.260946] [<c0a13cfc>] (really_probe) from [<c0a146b4>] (driver_probe_device+0xa4/0x278)
>> [ 9.260973] r10:e0cef438 r9:c0a14a04 r8:c2b771c0 r7:c2b5e298 r6:c2ab02b4 r5:e472d834
>> [ 9.260993] r4:e472d810
>> [ 9.261014] [<c0a14610>] (driver_probe_device) from [<c0a14aec>] (__device_attach_driver+0xe8/0x148)
>> [ 9.261042] r10:e0cef438 r9:c0a14a04 r8:e466bdc0 r7:e472d810 r6:00000000 r5:c2ab02b4
>> [ 9.261062] r4:e466bdc0
>> [ 9.261082] [<c0a14a04>] (__device_attach_driver) from [<c0a10f6c>] (bus_for_each_drv+0x108/0x158)
>> [ 9.261107] r7:c2806d48 r6:00000000 r5:bb8cd79c r4:e466bd00
>> [ 9.261131] [<c0a10e64>] (bus_for_each_drv) from [<c0a13c38>] (__device_attach+0x190/0x234)
>> [ 9.261158] r10:e46fde00 r9:e472d848 r8:e472d854 r7:e466be00 r6:c2806d48 r5:bb8cd7b4
>> [ 9.261178] r4:e472d810
>> [ 9.261199] [<c0a13aa8>] (__device_attach) from [<c0a14b68>] (device_initial_probe+0x1c/0x20)
>> [ 9.261226] r10:e46fde54 r9:c29d6380 r8:c29d7910 r7:e472d810 r6:c29d78c0 r5:c29d6340
>> [ 9.261246] r4:e44efe00
>> [ 9.261266] [<c0a14b4c>] (device_initial_probe) from [<c0a125fc>] (bus_probe_device+0xdc/0xec)
>> [ 9.261296] [<c0a12520>] (bus_probe_device) from [<c0a12e24>] (deferred_probe_work_func+0xd4/0x11c)
>> [ 9.261324] r9:c29d6380 r8:e4672e54 r7:c29d6380 r6:c29d6340 r5:c29d6340 r4:e472d810
>> [ 9.261356] [<c0a12d50>] (deferred_probe_work_func) from [<c025a534>] (process_one_work+0x420/0x8f0)
>> [ 9.261384] r10:e452fd84 r9:c2b53750 r8:00000000 r7:e452ae0c r6:e8a1e240 r5:e452ae00
>> [ 9.261404] r4:c29d6440
>> [ 9.261424] [<c025a114>] (process_one_work) from [<c025af00>] (worker_thread+0x4fc/0x91c)
>> [ 9.261450] r10:e8a1e258 r9:e452ae00 r8:e8a1e254 r7:e8a1e240 r6:e452fd84 r5:e8a1e254
>> [ 9.261470] r4:e452ae14
>> [ 9.261492] [<c025aa04>] (worker_thread) from [<c026591c>] (kthread+0x21c/0x22c)
>> [ 9.261517] r10:e455bce0 r9:e45b8d80 r8:e452ae00 r7:c025aa04 r6:e45b8d80 r5:00000000
>> [ 9.261537] r4:e45c0400
>> [ 9.261555] [<c0265700>] (kthread) from [<c0200114>] (ret_from_fork+0x14/0x20)
>> [ 9.261575] Exception stack(0xe466bfb0 to 0xe466bff8)
>> [ 9.261593] bfa0: 00000000 00000000 00000000 00000000
>> [ 9.261618] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> [ 9.261641] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
>> [ 9.261663] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0265700
>> [ 9.261683] r4:e4622080
>> [ 9.261728] kobject_add_internal failed for scmi_notify with -EEXIST, don't try to register things with the same name in the same directory.
>> [ 9.261805] arm-scmi brcm_scmi@1: SCMI Notifications - Initialization Failed.
>> [ 9.261847] arm-scmi brcm_scmi@1: SCMI Notifications NOT available.
>> [ 9.262026] arm-scmi brcm_scmi@1: SCMI Protocol v1.0 'brcm-scmi:' Firmware version 0x1
>>
>> Fix this by using dev_name(handle->dev) which guarantees that the name is
>> unique and this also helps correlate which notification workqueue corresponds
>> to which SCMI device instance.
>>
>
> I am curious as how multiple SCMI instances are used. We know few limitations
> in the code to handle that yet, so interested to know if you are carrying
> more patches/fixes.
We currently have two SCMI device nodes in Device Tree:
- the first one is responsible for all of the base, performance, sensors
protocols and is present on all of the chips listed above
- the second one is responsible for a proprietary protocol through which
we encapsulate a variety of operations towards a secure agent in the
system, this is only present in a subset of devices.
The reason for that split was because the nature of the SCMI traffic was
different, the first one is comprised of small messages dealing with
power management, most messages can be completed synchronously (or
quasi, if you remember our mailbox implementation) whereas the second is
comprised of large and asynchronously completed messages. This is not
probably a common set-up, and we have since eliminated that second SCMI
interface and went with a direct Linux to agent interface that does not
"speak" SCMI.
We may still have multiple SCMI devices in the future since we are
trying to separate out the CPU performance domain which involves writing
to registers that are either ARM system control registers, or only on an
ARM CPU local bus. The other clock/performance controls can go directly
to the agent implementing SCMI.
>
>> Fixes: bd31b249692e ("firmware: arm_scmi: Add notification dispatch and delivery")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> drivers/firmware/arm_scmi/notify.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
>> index 4731daaacd19..24c9ef232f48 100644
>> --- a/drivers/firmware/arm_scmi/notify.c
>> +++ b/drivers/firmware/arm_scmi/notify.c
>> @@ -1468,7 +1468,7 @@ int scmi_notification_init(struct scmi_handle *handle)
>> ni->gid = gid;
>> ni->handle = handle;
>>
>> - ni->notify_wq = alloc_workqueue("scmi_notify",
>> + ni->notify_wq = alloc_workqueue(dev_name(handle->dev),
>
> Not sure if it makes sense to add "_notify" to clearly identify the workqueue.
I had intentionally not done it because:
- it would create a different name than "dev_name" and I liked having
the same name throughout the entire sysfs hierarchy to easily identify
the device
- it would require doing a sprintf() variant to format the name which
require a temporary buffer
No strong preference either way though.
--
Florian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Fix duplicate workqueue name
2020-10-14 16:18 ` Sudeep Holla
@ 2020-10-14 17:13 ` Florian Fainelli
2020-10-14 17:39 ` Sudeep Holla
0 siblings, 1 reply; 12+ messages in thread
From: Florian Fainelli @ 2020-10-14 17:13 UTC (permalink / raw)
To: Sudeep Holla, Cristian Marussi
Cc: linux-kernel, linux-arm-kernel, Jonathan Cameron
On 10/14/20 9:18 AM, Sudeep Holla wrote:
> On Wed, Oct 14, 2020 at 02:48:19PM +0100, Cristian Marussi wrote:
>
> [...]
>
>>>
>>> I have pushed a version with above change [1], please check if you are
>>> happy with that ?
>>>
>>> [1] https://git.kernel.org/sudeep.holla/linux/c/b2cd15549b
>>
>> I agree with the need to retain _notify name, but I'm not so sure about
>> the above patch...which is:
>>
>
> I agree, I thought about it and just cooked up this as a quick solution.
> I will move to that, even I wasn't happy with this TBH.
The reason why I went with just dev_name() was such that the workqueue
name and the device nodes under /sys would strictly match, which helps
as an user, and also it avoided the temporary buffer and its size
limitations. However I don't have a strong feeling about this. Sudeep, I
can confirm that:
https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/patch/?id=b2cd15549b1fad6b0725c7a5a89ec3d41723fe8f
works as well for me:
# ls -l /sys/bus/workqueue/devices/
total 0
lrwxrwxrwx 1 root root 0 Jan 1 00:01
brcm_scmi@0_notify -> ../../../devices/virtual/workqueue/brcm_scmi@0_notify
lrwxrwxrwx 1 root root 0 Jan 1 00:01
brcm_scmi@1_notify -> ../../../devices/virtual/workqueue/brcm_scmi@1_notify
lrwxrwxrwx 1 root root 0 Jan 1 00:01 scsi_tmf_0 ->
../../../devices/virtual/workqueue/scsi_tmf_0
lrwxrwxrwx 1 root root 0 Jan 1 00:01 scsi_tmf_1 ->
../../../devices/virtual/workqueue/scsi_tmf_1
lrwxrwxrwx 1 root root 0 Jan 1 00:01 writeback ->
../../../devices/virtual/workqueue/writeback
--
Florian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Fix duplicate workqueue name
2020-10-14 17:08 ` Florian Fainelli
@ 2020-10-14 17:32 ` Sudeep Holla
2020-10-14 17:35 ` Sudeep Holla
0 siblings, 1 reply; 12+ messages in thread
From: Sudeep Holla @ 2020-10-14 17:32 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-arm-kernel, Jonathan Cameron, Sudeep Holla,
Cristian Marussi, linux-kernel
On Wed, Oct 14, 2020 at 10:08:32AM -0700, Florian Fainelli wrote:
> On 10/14/20 2:18 AM, Sudeep Holla wrote:
> > Hi Florian,
> >
> > Thanks for the patch, it shows someone else is also using this and
> > testing 😉.
> >
> > On Tue, Oct 13, 2020 at 07:17:37PM -0700, Florian Fainelli wrote:
> >> When more than a single SCMI device are present in the system, the
> >> creation of the notification workqueue with the WQ_SYSFS flag will lead
> >> to the following sysfs duplicate node warning:
> >>
> >
> > Please trim the calltrace next time without timestamp and register raw
> > hex values.
>
> Will do, thanks!
>
Thanks!
> >
> > You using this on 32-bit platform ? If so, thanks for additional test
> > coverage.
>
> We have a mix of ARMv7/LPAE (Brahma-B15) and ARMv8 (Brahma-B53,
> Cortex-A72) devices that we regularly test with 32-bit and 64-bit kernels.
>
Ah OK, good to know.
[...]
> >> Fix this by using dev_name(handle->dev) which guarantees that the name is
> >> unique and this also helps correlate which notification workqueue corresponds
> >> to which SCMI device instance.
> >>
> >
> > I am curious as how multiple SCMI instances are used. We know few limitations
> > in the code to handle that yet, so interested to know if you are carrying
> > more patches/fixes.
>
> We currently have two SCMI device nodes in Device Tree:
>
> - the first one is responsible for all of the base, performance, sensors
> protocols and is present on all of the chips listed above
>
> - the second one is responsible for a proprietary protocol through which
> we encapsulate a variety of operations towards a secure agent in the
> system, this is only present in a subset of devices.
>
And any particular reasons it can't exist in the same node. And also are
they talking to different SCMI firmware implementation meaning different
location in the system. The reason I ask is we have notion of one platform
with agent id = 0 as per the specification. It can be split in terms
of implementation and can have some side band communication amongst
themselves but can't have agent ID other than 0. It violates specification.
I don't have issues split it into 2 or more SCMI device as long as it
doesn't provide notion of existence of multiple SCMI platform firmware
implementations with different agent ID.
Also Cristian has posted patches to support custom protocols[1]. It would
be good if you can take a look/review/test/comment...
> The reason for that split was because the nature of the SCMI traffic was
> different, the first one is comprised of small messages dealing with
> power management, most messages can be completed synchronously (or
> quasi, if you remember our mailbox implementation) whereas the second is
> comprised of large and asynchronously completed messages. This is not
> probably a common set-up, and we have since eliminated that second SCMI
> interface and went with a direct Linux to agent interface that does not
> "speak" SCMI.
>
I still don't see strong reasons for splitting it. Also by above I assume
you still have one SCMI server/platform implementation and just different
types of transport for std vs custom protocol and all sorts of such combo.
I think we deal with that as long as they are of same transport type like
mailbox or smc.
> We may still have multiple SCMI devices in the future since we are
> trying to separate out the CPU performance domain which involves writing
> to registers that are either ARM system control registers, or only on an
> ARM CPU local bus. The other clock/performance controls can go directly
> to the agent implementing SCMI.
>
We have something called fastchannels for that. Your scmi perf protocol
can present those registers in those fastchannels if they are MMIO.
If they are system registers, then you need not use scmi at all for CPU
DVFS and still need one SCMI device for all other needs.
> >
> >> Fixes: bd31b249692e ("firmware: arm_scmi: Add notification dispatch and delivery")
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >> ---
> >> drivers/firmware/arm_scmi/notify.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
> >> index 4731daaacd19..24c9ef232f48 100644
> >> --- a/drivers/firmware/arm_scmi/notify.c
> >> +++ b/drivers/firmware/arm_scmi/notify.c
> >> @@ -1468,7 +1468,7 @@ int scmi_notification_init(struct scmi_handle *handle)
> >> ni->gid = gid;
> >> ni->handle = handle;
> >>
> >> - ni->notify_wq = alloc_workqueue("scmi_notify",
> >> + ni->notify_wq = alloc_workqueue(dev_name(handle->dev),
> >
> > Not sure if it makes sense to add "_notify" to clearly identify the workqueue.
>
> I had intentionally not done it because:
>
OK fair enough.
> - it would create a different name than "dev_name" and I liked having
> the same name throughout the entire sysfs hierarchy to easily identify
> the device
>
Good point. I was worried if we need to create another workqueue for
something else in the future.
> - it would require doing a sprintf() variant to format the name which
> require a temporary buffer
>
Indeed, which I tried but don't like.
> No strong preference either way though.
For now, we can have as is in your patch. We can address this if and
when we create another workqueue in scmi
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Fix duplicate workqueue name
2020-10-14 17:32 ` Sudeep Holla
@ 2020-10-14 17:35 ` Sudeep Holla
0 siblings, 0 replies; 12+ messages in thread
From: Sudeep Holla @ 2020-10-14 17:35 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-arm-kernel, Jonathan Cameron, Cristian Marussi,
Sudeep Holla, linux-kernel
On Wed, Oct 14, 2020 at 06:32:42PM +0100, Sudeep Holla wrote:
> On Wed, Oct 14, 2020 at 10:08:32AM -0700, Florian Fainelli wrote:
> > On 10/14/20 2:18 AM, Sudeep Holla wrote:
> > > Hi Florian,
> > >
> > > Thanks for the patch, it shows someone else is also using this and
> > > testing 😉.
> > >
> > > On Tue, Oct 13, 2020 at 07:17:37PM -0700, Florian Fainelli wrote:
> > >> When more than a single SCMI device are present in the system, the
> > >> creation of the notification workqueue with the WQ_SYSFS flag will lead
> > >> to the following sysfs duplicate node warning:
> > >>
> > >
> > > Please trim the calltrace next time without timestamp and register raw
> > > hex values.
> >
> > Will do, thanks!
> >
>
> Thanks!
>
> > >
> > > You using this on 32-bit platform ? If so, thanks for additional test
> > > coverage.
> >
> > We have a mix of ARMv7/LPAE (Brahma-B15) and ARMv8 (Brahma-B53,
> > Cortex-A72) devices that we regularly test with 32-bit and 64-bit kernels.
> >
>
> Ah OK, good to know.
>
> [...]
>
> > >> Fix this by using dev_name(handle->dev) which guarantees that the name is
> > >> unique and this also helps correlate which notification workqueue corresponds
> > >> to which SCMI device instance.
> > >>
> > >
> > > I am curious as how multiple SCMI instances are used. We know few limitations
> > > in the code to handle that yet, so interested to know if you are carrying
> > > more patches/fixes.
> >
> > We currently have two SCMI device nodes in Device Tree:
> >
> > - the first one is responsible for all of the base, performance, sensors
> > protocols and is present on all of the chips listed above
> >
> > - the second one is responsible for a proprietary protocol through which
> > we encapsulate a variety of operations towards a secure agent in the
> > system, this is only present in a subset of devices.
> >
>
> And any particular reasons it can't exist in the same node. And also are
> they talking to different SCMI firmware implementation meaning different
> location in the system. The reason I ask is we have notion of one platform
> with agent id = 0 as per the specification. It can be split in terms
> of implementation and can have some side band communication amongst
> themselves but can't have agent ID other than 0. It violates specification.
>
> I don't have issues split it into 2 or more SCMI device as long as it
> doesn't provide notion of existence of multiple SCMI platform firmware
> implementations with different agent ID.
>
> Also Cristian has posted patches to support custom protocols[1]. It would
> be good if you can take a look/review/test/comment...
>
Pressed enter too early, link added now.
--
Regards,
Sudeep
[1] https://lore.kernel.org/lkml/20201014150545.44807-1-cristian.marussi@arm.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Fix duplicate workqueue name
2020-10-14 17:13 ` Florian Fainelli
@ 2020-10-14 17:39 ` Sudeep Holla
2020-10-14 18:11 ` Florian Fainelli
0 siblings, 1 reply; 12+ messages in thread
From: Sudeep Holla @ 2020-10-14 17:39 UTC (permalink / raw)
To: Florian Fainelli
Cc: Cristian Marussi, linux-kernel, linux-arm-kernel, Sudeep Holla,
Jonathan Cameron
On Wed, Oct 14, 2020 at 10:13:04AM -0700, Florian Fainelli wrote:
> On 10/14/20 9:18 AM, Sudeep Holla wrote:
> > On Wed, Oct 14, 2020 at 02:48:19PM +0100, Cristian Marussi wrote:
> >
> > [...]
> >
> >>>
> >>> I have pushed a version with above change [1], please check if you are
> >>> happy with that ?
> >>>
> >>> [1] https://git.kernel.org/sudeep.holla/linux/c/b2cd15549b
> >>
> >> I agree with the need to retain _notify name, but I'm not so sure about
> >> the above patch...which is:
> >>
> >
> > I agree, I thought about it and just cooked up this as a quick solution.
> > I will move to that, even I wasn't happy with this TBH.
>
> The reason why I went with just dev_name() was such that the workqueue
> name and the device nodes under /sys would strictly match, which helps
> as an user, and also it avoided the temporary buffer and its size
> limitations.
Agreed. I just showed that as example and was hoping to use some nice
kstr* APIs to achieve what I wanted but soon realised there exists none.
So as replied earlier, I will take this change as it for now. Let us
address this in future if it becomes an issue.
Thanks for quick test, we now know whom to bother if we need more testing
😉 as out internal platforms are not that great to cover all the aspects
we add in the spec and even in the kernel.
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Fix duplicate workqueue name
2020-10-14 17:39 ` Sudeep Holla
@ 2020-10-14 18:11 ` Florian Fainelli
0 siblings, 0 replies; 12+ messages in thread
From: Florian Fainelli @ 2020-10-14 18:11 UTC (permalink / raw)
To: Sudeep Holla
Cc: Cristian Marussi, linux-kernel, linux-arm-kernel, Jonathan Cameron
On 10/14/20 10:39 AM, Sudeep Holla wrote:
> On Wed, Oct 14, 2020 at 10:13:04AM -0700, Florian Fainelli wrote:
>> On 10/14/20 9:18 AM, Sudeep Holla wrote:
>>> On Wed, Oct 14, 2020 at 02:48:19PM +0100, Cristian Marussi wrote:
>>>
>>> [...]
>>>
>>>>>
>>>>> I have pushed a version with above change [1], please check if you are
>>>>> happy with that ?
>>>>>
>>>>> [1] https://git.kernel.org/sudeep.holla/linux/c/b2cd15549b
>>>>
>>>> I agree with the need to retain _notify name, but I'm not so sure about
>>>> the above patch...which is:
>>>>
>>>
>>> I agree, I thought about it and just cooked up this as a quick solution.
>>> I will move to that, even I wasn't happy with this TBH.
>>
>> The reason why I went with just dev_name() was such that the workqueue
>> name and the device nodes under /sys would strictly match, which helps
>> as an user, and also it avoided the temporary buffer and its size
>> limitations.
>
> Agreed. I just showed that as example and was hoping to use some nice
> kstr* APIs to achieve what I wanted but soon realised there exists none.
> So as replied earlier, I will take this change as it for now. Let us
> address this in future if it becomes an issue.
>
> Thanks for quick test, we now know whom to bother if we need more testing
> 😉 as out internal platforms are not that great to cover all the aspects
> we add in the spec and even in the kernel.
No problem! I still need to find the time to upgrade the ATF equivalent
implementation to support SCMI 3.0, have not done that just yet.
--
Florian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] firmware: arm_scmi: Fix duplicate workqueue name
2020-10-14 2:17 [PATCH] firmware: arm_scmi: Fix duplicate workqueue name Florian Fainelli
2020-10-14 9:18 ` Sudeep Holla
@ 2020-10-14 20:29 ` Sudeep Holla
1 sibling, 0 replies; 12+ messages in thread
From: Sudeep Holla @ 2020-10-14 20:29 UTC (permalink / raw)
To: Florian Fainelli, linux-arm-kernel
Cc: Sudeep Holla, linux-kernel, Cristian Marussi, Jonathan Cameron
On Tue, 13 Oct 2020 19:17:37 -0700, Florian Fainelli wrote:
> When more than a single SCMI device are present in the system, the
> creation of the notification workqueue with the WQ_SYSFS flag will lead
> to the following sysfs duplicate node warning:
>
> [ 9.259990] sysfs: cannot create duplicate filename '/devices/virtual/workqueue/scmi_notify'
> [ 9.260024] CPU: 0 PID: 20 Comm: kworker/0:1 Not tainted 5.9.0-gdf4dd84a3f7d #29
> [ 9.260045] Hardware name: Broadcom STB (Flattened Device Tree)
> [ 9.260083] Workqueue: events deferred_probe_work_func
> [ 9.260099] Backtrace:
> [ 9.260127] [<c02120b4>] (dump_backtrace) from [<c02123d8>] (show_stack+0x20/0x24)
> [ 9.260155] r9:ffffffff r8:00000000 r7:c298e3c0 r6:60000013 r5:00000000 r4:c298e3c0
> [ 9.260186] [<c02123b8>] (show_stack) from [<c08852a0>] (dump_stack+0xbc/0xe0)
> [ 9.260216] [<c08851e4>] (dump_stack) from [<c054442c>] (sysfs_warn_dup+0x70/0x80)
> [ 9.260243] r10:e472d814 r9:ffffffef r8:e4614a50 r7:c2806d48 r6:e0e50f80 r5:e4614a50
> [ 9.260265] r4:e0e82000 r3:c5b17c34
> [ 9.260285] [<c05443bc>] (sysfs_warn_dup) from [<c05445fc>] (sysfs_create_dir_ns+0x15c/0x1a4)
> [ 9.260310] r7:c2806d48 r6:ffffffef r5:e0dff008 r4:bb8cd6e4
> [ 9.260335] [<c05444a0>] (sysfs_create_dir_ns) from [<c088ebac>] (kobject_add_internal+0x140/0x4d0)
> [ 9.260363] r9:e451fa80 r8:e0dff01c r7:e0dff014 r6:e45b1400 r5:e0dff018 r4:e0dff008
> [ 9.260390] [<c088ea6c>] (kobject_add_internal) from [<c088f004>] (kobject_add+0xc8/0x138)
> [ 9.260416] r10:e0dff030 r9:00000000 r8:e45b1400 r7:00000000 r6:c2806d48 r5:e0dff008
> [ 9.260436] r4:bb8cd708
> [ 9.260455] [<c088ef3c>] (kobject_add) from [<c0a0bc70>] (device_add+0x1dc/0xc20)
> [ 9.260476] r3:e0dff014 r2:00000000
> [ 9.260495] r9:00000000 r8:e466b980 r7:e45b1400 r6:c2806d48 r5:bb8cd724 r4:e0dff008
> [ 9.260521] [<c0a0ba94>] (device_add) from [<c0a0c6d8>] (device_register+0x24/0x28)
> [ 9.260547] r10:c2831340 r9:e0dfec08 r8:e0dff028 r7:e0dfecc0 r6:e0dfec00 r5:e0dff008
> [ 9.260567] r4:e0dff008
> [ 9.260586] [<c0a0c6b4>] (device_register) from [<c025f2a8>] (workqueue_sysfs_register+0xe4/0x1f0)
> [ 9.260609] r5:e0dff008 r4:e0dff000
> [ 9.260627] [<c025f1c4>] (workqueue_sysfs_register) from [<c025f7fc>] (alloc_workqueue+0x448/0x6ac)
> [ 9.260655] r10:e0dfecc0 r9:e0dfec08 r8:e0dfec0c r7:e0dfec04 r6:e0dfecc0 r5:e0dfec00
> [ 9.260675] r4:00000001
> [ 9.260698] [<c025f3b4>] (alloc_workqueue) from [<c0d8b58c>] (scmi_notification_init+0x78/0x1dc)
> [ 9.260720] r3:c0a1b488 r2:00000000
> [ 9.260739] r10:e0de1040 r9:00800004 r8:e0de10d8 r7:e0de10f4 r6:e0de1074 r5:e0e54740
> [ 9.260759] r4:e0e50d00
> [ 9.260778] [<c0d8b514>] (scmi_notification_init) from [<c0d87c00>] (scmi_probe+0x268/0x4fc)
> [ 9.260805] r9:00800004 r8:e0de10d8 r7:e0de10f4 r6:e0de1074 r5:e0de10f0 r4:e472d810
> [ 9.260833] [<c0d87998>] (scmi_probe) from [<c0a17d74>] (platform_drv_probe+0x70/0xc8)
> [ 9.260859] r10:e472d844 r9:c2ab02b4 r8:c2b5e298 r7:00000000 r6:c2ab02b4 r5:00000000
> [ 9.260879] r4:e472d810
> [ 9.260898] [<c0a17d04>] (platform_drv_probe) from [<c0a13e80>] (really_probe+0x184/0x728)
> [ 9.260922] r7:00000000 r6:c2fc82a0 r5:c2fc8220 r4:e472d810
> [ 9.260946] [<c0a13cfc>] (really_probe) from [<c0a146b4>] (driver_probe_device+0xa4/0x278)
> [ 9.260973] r10:e0cef438 r9:c0a14a04 r8:c2b771c0 r7:c2b5e298 r6:c2ab02b4 r5:e472d834
> [ 9.260993] r4:e472d810
> [ 9.261014] [<c0a14610>] (driver_probe_device) from [<c0a14aec>] (__device_attach_driver+0xe8/0x148)
> [ 9.261042] r10:e0cef438 r9:c0a14a04 r8:e466bdc0 r7:e472d810 r6:00000000 r5:c2ab02b4
> [ 9.261062] r4:e466bdc0
> [ 9.261082] [<c0a14a04>] (__device_attach_driver) from [<c0a10f6c>] (bus_for_each_drv+0x108/0x158)
> [ 9.261107] r7:c2806d48 r6:00000000 r5:bb8cd79c r4:e466bd00
> [ 9.261131] [<c0a10e64>] (bus_for_each_drv) from [<c0a13c38>] (__device_attach+0x190/0x234)
> [ 9.261158] r10:e46fde00 r9:e472d848 r8:e472d854 r7:e466be00 r6:c2806d48 r5:bb8cd7b4
> [ 9.261178] r4:e472d810
> [ 9.261199] [<c0a13aa8>] (__device_attach) from [<c0a14b68>] (device_initial_probe+0x1c/0x20)
> [ 9.261226] r10:e46fde54 r9:c29d6380 r8:c29d7910 r7:e472d810 r6:c29d78c0 r5:c29d6340
> [ 9.261246] r4:e44efe00
> [ 9.261266] [<c0a14b4c>] (device_initial_probe) from [<c0a125fc>] (bus_probe_device+0xdc/0xec)
> [ 9.261296] [<c0a12520>] (bus_probe_device) from [<c0a12e24>] (deferred_probe_work_func+0xd4/0x11c)
> [ 9.261324] r9:c29d6380 r8:e4672e54 r7:c29d6380 r6:c29d6340 r5:c29d6340 r4:e472d810
> [ 9.261356] [<c0a12d50>] (deferred_probe_work_func) from [<c025a534>] (process_one_work+0x420/0x8f0)
> [ 9.261384] r10:e452fd84 r9:c2b53750 r8:00000000 r7:e452ae0c r6:e8a1e240 r5:e452ae00
> [ 9.261404] r4:c29d6440
> [ 9.261424] [<c025a114>] (process_one_work) from [<c025af00>] (worker_thread+0x4fc/0x91c)
> [ 9.261450] r10:e8a1e258 r9:e452ae00 r8:e8a1e254 r7:e8a1e240 r6:e452fd84 r5:e8a1e254
> [ 9.261470] r4:e452ae14
> [ 9.261492] [<c025aa04>] (worker_thread) from [<c026591c>] (kthread+0x21c/0x22c)
> [ 9.261517] r10:e455bce0 r9:e45b8d80 r8:e452ae00 r7:c025aa04 r6:e45b8d80 r5:00000000
> [ 9.261537] r4:e45c0400
> [ 9.261555] [<c0265700>] (kthread) from [<c0200114>] (ret_from_fork+0x14/0x20)
> [ 9.261575] Exception stack(0xe466bfb0 to 0xe466bff8)
> [ 9.261593] bfa0: 00000000 00000000 00000000 00000000
> [ 9.261618] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [ 9.261641] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [ 9.261663] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0265700
> [ 9.261683] r4:e4622080
> [ 9.261728] kobject_add_internal failed for scmi_notify with -EEXIST, don't try to register things with the same name in the same directory.
> [ 9.261805] arm-scmi brcm_scmi@1: SCMI Notifications - Initialization Failed.
> [ 9.261847] arm-scmi brcm_scmi@1: SCMI Notifications NOT available.
> [ 9.262026] arm-scmi brcm_scmi@1: SCMI Protocol v1.0 'brcm-scmi:' Firmware version 0x1
>
> [...]
Applied to sudeep.holla/linux (for-next/scmi), thanks!
[1/1] firmware: arm_scmi: Fix duplicate workqueue name
https://git.kernel.org/sudeep.holla/c/b9ceca6be4
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-10-14 20:29 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-14 2:17 [PATCH] firmware: arm_scmi: Fix duplicate workqueue name Florian Fainelli
2020-10-14 9:18 ` Sudeep Holla
2020-10-14 13:05 ` Sudeep Holla
2020-10-14 13:48 ` Cristian Marussi
2020-10-14 16:18 ` Sudeep Holla
2020-10-14 17:13 ` Florian Fainelli
2020-10-14 17:39 ` Sudeep Holla
2020-10-14 18:11 ` Florian Fainelli
2020-10-14 17:08 ` Florian Fainelli
2020-10-14 17:32 ` Sudeep Holla
2020-10-14 17:35 ` Sudeep Holla
2020-10-14 20:29 ` Sudeep Holla
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).