| Message ID | 20241205145142.29278-2-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net: dsa: Add Airoha AN8855 support | expand |
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@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@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.
diff --git a/include/net/dsa.h b/include/net/dsa.h index 72ae65e7246a..c703d5dc3fb0 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -1355,6 +1355,7 @@ static inline void dsa_tag_generic_flow_dissect(const struct sk_buff *skb, void dsa_unregister_switch(struct dsa_switch *ds); int dsa_register_switch(struct dsa_switch *ds); +int devm_dsa_register_switch(struct device *dev, struct dsa_switch *ds); void dsa_switch_shutdown(struct dsa_switch *ds); struct dsa_switch *dsa_switch_find(int tree_index, int sw_index); void dsa_flush_workqueue(void); diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index 5a7c0e565a89..aca6aee68248 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -1544,6 +1544,25 @@ int dsa_register_switch(struct dsa_switch *ds) } EXPORT_SYMBOL_GPL(dsa_register_switch); +static void devm_dsa_unregister_switch(void *data) +{ + struct dsa_switch *ds = data; + + dsa_unregister_switch(ds); +} + +int devm_dsa_register_switch(struct device *dev, struct dsa_switch *ds) +{ + int err; + + err = dsa_register_switch(ds); + if (err) + return err; + + return devm_add_action_or_reset(dev, devm_dsa_unregister_switch, ds); +} +EXPORT_SYMBOL_GPL(devm_dsa_register_switch); + static void dsa_switch_remove(struct dsa_switch *ds) { struct dsa_switch_tree *dst = ds->dst;
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@gmail.com> --- include/net/dsa.h | 1 + net/dsa/dsa.c | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+)