[net-next PATCH v9 1/4] net: dsa: add devm_dsa_register_switch()
Vladimir Oltean
olteanv at gmail.com
Thu Dec 5 08:03:44 PST 2024
On Thu, Dec 05, 2024 at 03:51:31PM +0100, Christian Marangi wrote:
> Some DSA driver can be simplified if devres takes care of unregistering
> the DSA switch. This permits to effectively drop the remove OP from
> driver that just execute the dsa_unregister_switch() and nothing else.
>
> Signed-off-by: Christian Marangi <ansuelsmth at gmail.com>
> ---
The premise is false. *No* DSA drivers can safely be simplified if
devres is let to take care of calling dsa_unregister_switch().
See, no remove() method of a DSA driver calls dsa_unregister_switch()
directly, but instead they all test dev_get_drvdata() against NULL first.
See this patch set for a full explanation:
https://lore.kernel.org/netdev/20210917133436.553995-1-vladimir.oltean@nxp.com/
but the short explanation is that the parent bus can implement its own
.shutdown() as .remove(), which for the DSA switch device means that
during shutdown/reboot, both .shutdown() *and* .remove() will be called.
The DSA framework is only prepared for either dsa_unregister_switch()
*or* dsa_switch_shutdown() to be called. It doesn't work if *both* are
called, so we have this mechanism where .shutdown() will set the device
drvdata to NULL, so that .remove() will become a no-op. But that mechanism
will become void if we start to drop the driver's remove() and rely on
devres to call dsa_unregister_switch().
Demo for sja1105 driver with the spi-fsl-dspi.c controller driver as parent.
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index f8454f3b6f9c..b9c92a5e5f5f 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -3404,17 +3404,7 @@ static int sja1105_probe(struct spi_device *spi)
return -ENOMEM;
}
- return dsa_register_switch(priv->ds);
-}
-
-static void sja1105_remove(struct spi_device *spi)
-{
- struct sja1105_private *priv = spi_get_drvdata(spi);
-
- if (!priv)
- return;
-
- dsa_unregister_switch(priv->ds);
+ return devm_dsa_register_switch(dev, priv->ds);
}
static void sja1105_shutdown(struct spi_device *spi)
@@ -3466,7 +3456,6 @@ static struct spi_driver sja1105_driver = {
},
.id_table = sja1105_spi_ids,
.probe = sja1105_probe,
- .remove = sja1105_remove,
.shutdown = sja1105_shutdown,
};
root at debian:~# reboot
[ 52.421866] watchdog: watchdog0: watchdog did not stop!
[ 52.515700] systemd-shutdown[1]: Using hardware watchdog 'sp805-wdt', version 0, device /dev/watchdog0
[ 52.525256] systemd-shutdown[1]: Watchdog running with a timeout of 5min 44s.
[ 52.977392] systemd-shutdown[1]: Syncing filesystems and block devices.
[ 53.041107] systemd-shutdown[1]: Sending SIGTERM to remaining processes...
[ 53.070259] systemd-journald[277]: Received SIGTERM from PID 1 (systemd-shutdow).
[ 53.123590] systemd-shutdown[1]: Sending SIGKILL to remaining processes...
[ 53.156518] systemd-shutdown[1]: Unmounting file systems.
[ 53.170735] (sd-remount)[632]: Remounting '/' read-only with options ''.
[ 53.229253] EXT4-fs (mmcblk0p2): re-mounted e092e674-ed6c-4216-b216-58d8390ae85d ro. Quota mode: none.
[ 53.313634] systemd-shutdown[1]: All filesystems unmounted.
[ 53.319334] systemd-shutdown[1]: Deactivating swaps.
[ 53.324625] systemd-shutdown[1]: All swaps deactivated.
[ 53.329943] systemd-shutdown[1]: Detaching loop devices.
[ 53.342596] systemd-shutdown[1]: All loop devices detached.
[ 53.348263] systemd-shutdown[1]: Stopping MD devices.
[ 53.354180] systemd-shutdown[1]: All MD devices stopped.
[ 53.359633] systemd-shutdown[1]: Detaching DM devices.
[ 53.365699] systemd-shutdown[1]: All DM devices detached.
[ 53.371178] systemd-shutdown[1]: All filesystems, swaps, loop devices, MD devices and DM devices detached.
[ 53.381033] watchdog: watchdog0: watchdog did not stop!
[ 53.424144] systemd-shutdown[1]: Syncing filesystems and block devices.
[ 53.431313] systemd-shutdown[1]: Rebooting.
[ 53.458710] sja1105 spi2.0 sw0p0: Link is Down
[ 53.477004] mscc_felix 0000:00:00.5 swp0: Link is Down
[ 53.486054] fsl_enetc 0000:00:00.2 eno2: Link is Down
[ 53.518865] Unable to handle kernel NULL pointer dereference at virtual address 000000000000002c
[ 53.527921] Mem abort info:
[ 53.530776] ESR = 0x0000000096000004
[ 53.534612] EC = 0x25: DABT (current EL), IL = 32 bits
[ 53.539988] SET = 0, FnV = 0
[ 53.543124] EA = 0, S1PTW = 0
[ 53.546315] FSC = 0x04: level 0 translation fault
[ 53.551282] Data abort info:
[ 53.554211] ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[ 53.559792] CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[ 53.564904] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[ 53.570476] user pgtable: 4k pages, 48-bit VAs, pgdp=0000002083f73000
[ 53.577029] [000000000000002c] pgd=0000000000000000, p4d=0000000000000000
[ 53.584079] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
[ 53.590374] Modules linked in:
[ 53.593442] CPU: 0 UID: 0 PID: 1 Comm: systemd-shutdow Tainted: G N 6.12.0-10714-gc118f6e3b41e-dirty #2585
[ 53.604532] Tainted: [N]=TEST
[ 53.613010] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 53.619999] pc : dsa_tree_conduit_admin_state_change+0x44/0xf8
[ 53.625864] lr : dsa_unregister_switch+0x194/0x2a8
[ 53.630674] sp : ffff80008002b940
[ 53.633997] x29: ffff80008002b960 x28: ffff330a40938000 x27: ffff330a40cff818
[ 53.641168] x26: ffffba4f5bb05000 x25: 0000000000000001 x24: ffff330a42e19400
[ 53.648338] x23: ffff330a42e13668 x22: ffff330a435a5890 x21: ffff330a42dc7000
[ 53.655507] x20: ffff330a42e5e080 x19: ffff330a435a5880 x18: 0000000000000000
[ 53.662676] x17: ffffba4f5be23118 x16: ffffba4f5bb0d088 x15: 0000000000000108
[ 53.669845] x14: ffffba4f5c02b550 x13: 0000000000000004 x12: ffff330a40938908
[ 53.677014] x11: ffffba4f5b2ae418 x10: 0000000000000000 x9 : 0000000000000000
[ 53.684183] x8 : 0000000000000000 x7 : ffffba4f5917fbe0 x6 : 0000000000000000
[ 53.691352] x5 : 0000000000000020 x4 : ffff80008002b620 x3 : 0000000000000000
[ 53.698521] x2 : 0000000000000000 x1 : ffff330a42dc7000 x0 : ffff330a435a5880
[ 53.705691] Call trace:
[ 53.708142] dsa_tree_conduit_admin_state_change+0x44/0xf8 (P)
[ 53.714001] dsa_unregister_switch+0x194/0x2a8 (L)
[ 53.718811] dsa_unregister_switch+0x194/0x2a8
[ 53.723272] devm_dsa_unregister_switch+0x1c/0x30
[ 53.727994] devm_action_release+0x20/0x38
[ 53.732107] devres_release_all+0xc4/0x130
[ 53.736217] device_release_driver_internal+0x1d0/0x280
[ 53.741464] device_release_driver+0x24/0x38
[ 53.745751] bus_remove_device+0x154/0x170
[ 53.749862] device_del+0x1f8/0x3e8
[ 53.753361] spi_unregister_device+0x90/0xe8
[ 53.757646] __unregister+0x1c/0x38
[ 53.761147] device_for_each_child+0x6c/0xc8
[ 53.765432] spi_unregister_controller+0x50/0x158
[ 53.770153] dspi_remove+0x28/0x98
[ 53.773567] dspi_shutdown+0x1c/0x30
[ 53.777154] platform_shutdown+0x30/0x48
[ 53.781089] device_shutdown+0x174/0x238
[ 53.785025] kernel_restart+0x4c/0x128
[ 53.788788] __arm64_sys_reboot+0x200/0x2e8
[ 53.792987] invoke_syscall+0x4c/0x110
[ 53.796752] el0_svc_common+0xb8/0xf0
[ 53.800429] do_el0_svc+0x28/0x40
[ 53.803757] el0_svc+0x4c/0xc0
[ 53.806823] el0t_64_sync_handler+0x84/0x108
[ 53.811109] el0t_64_sync+0x198/0x1a0
[ 53.814786] Code: 0a490949 37000489 37b00468 f9421828 (b9402d09)
[ 53.820901] ---[ end trace 0000000000000000 ]---
[ 53.825600] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
[ 53.833306] Kernel Offset: 0x3a4ed7800000 from 0xffff800080000000
[ 53.839420] PHYS_OFFSET: 0xfff0cd1640000000
[ 53.843615] CPU features: 0x080,0002012c,00800000,8200421b
[ 53.849120] Memory Limit: none
[ 53.852184] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---
This will need a lot more thought before it makes its appearance as a
tool in the DSA toolbox. Otherwise it is just an avoidable source of
problems.
More information about the linux-arm-kernel
mailing list