| Message ID | 20210204163351.2929670-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8fd54a73b7cda11548154451bdb4bde6d8ff74c7 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: dsa: call teardown method on probe failure | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: olteanv@gmail.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 25 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Thu, Feb 04, 2021 at 06:33:51PM +0200, Vladimir Oltean wrote: > Since teardown is supposed to undo the effects of the setup method, it > should be called in the error path for dsa_switch_setup, not just in > dsa_switch_teardown. I disagree with this. If setup failed, it should of cleaned itself up. That is the generally accepted way of doing things. If a function is going to exit with an error, it should first undo whatever it did before exiting. You are adding extra semantics to the teardown op. It can no longer assume setup was successful. So it needs to be very careful about what it tears down, it cannot assume everything has been setup. I doubt the existing implementations actually do that. Andrew
On Thu, Feb 04, 2021 at 06:00:26PM +0100, Andrew Lunn wrote: > On Thu, Feb 04, 2021 at 06:33:51PM +0200, Vladimir Oltean wrote: > > Since teardown is supposed to undo the effects of the setup method, it > > should be called in the error path for dsa_switch_setup, not just in > > dsa_switch_teardown. > > I disagree with this. If setup failed, it should of cleaned itself up. > That is the generally accepted way of doing things. If a function is > going to exit with an error, it should first undo whatever it did > before exiting. > > You are adding extra semantics to the teardown op. It can no longer > assume setup was successful. So it needs to be very careful about what > it tears down, it cannot assume everything has been setup. I doubt the > existing implementations actually do that. I'm sorry, I don't understand. I write a driver, I implement .setup(). I allocate some memory, I expect that I can deallocate it in .teardown(). Now dsa_switch_setup comes, calls my .setup() which succedes. But then mdiobus_register(ds->slave_mii_bus) which comes right after .setup() fails. Are you saying we shouldn't call the driver's .teardown()? Why not?
On Thu, Feb 04, 2021 at 05:06:15PM +0000, Vladimir Oltean wrote: > On Thu, Feb 04, 2021 at 06:00:26PM +0100, Andrew Lunn wrote: > > On Thu, Feb 04, 2021 at 06:33:51PM +0200, Vladimir Oltean wrote: > > > Since teardown is supposed to undo the effects of the setup method, it > > > should be called in the error path for dsa_switch_setup, not just in > > > dsa_switch_teardown. > > > > I disagree with this. If setup failed, it should of cleaned itself up. > > That is the generally accepted way of doing things. If a function is > > going to exit with an error, it should first undo whatever it did > > before exiting. > > > > You are adding extra semantics to the teardown op. It can no longer > > assume setup was successful. So it needs to be very careful about what > > it tears down, it cannot assume everything has been setup. I doubt the > > existing implementations actually do that. > > I'm sorry, I don't understand. > I write a driver, I implement .setup(). I allocate some memory, I expect > that I can deallocate it in .teardown(). > Now dsa_switch_setup comes, calls my .setup() which succedes. But then > mdiobus_register(ds->slave_mii_bus) which comes right after .setup() > fails. Are you saying we shouldn't call the driver's .teardown()? > Why not? Hi Vladimir Ah, sorry. Read you commit message wrongly. I though you were calling teardown if setup failed. But that is not what the patch does. It calls it if things after setup fail. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On 2/4/21 8:33 AM, Vladimir Oltean wrote: > Since teardown is supposed to undo the effects of the setup method, it > should be called in the error path for dsa_switch_setup, not just in > dsa_switch_teardown. > > Fixes: 5e3f847a02aa ("net: dsa: Add teardown callback for drivers") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Thu, 4 Feb 2021 18:33:51 +0200 you wrote: > Since teardown is supposed to undo the effects of the setup method, it > should be called in the error path for dsa_switch_setup, not just in > dsa_switch_teardown. > > Fixes: 5e3f847a02aa ("net: dsa: Add teardown callback for drivers") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > [...] Here is the summary with links: - [net] net: dsa: call teardown method on probe failure https://git.kernel.org/netdev/net/c/8fd54a73b7cd You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index 96249c4ad5f2..4d4956ed303b 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -724,20 +724,23 @@ static int dsa_switch_setup(struct dsa_switch *ds) ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev); if (!ds->slave_mii_bus) { err = -ENOMEM; - goto unregister_notifier; + goto teardown; } dsa_slave_mii_bus_init(ds); err = mdiobus_register(ds->slave_mii_bus); if (err < 0) - goto unregister_notifier; + goto teardown; } ds->setup = true; return 0; +teardown: + if (ds->ops->teardown) + ds->ops->teardown(ds); unregister_notifier: dsa_switch_unregister_notifier(ds); unregister_devlink_ports:
Since teardown is supposed to undo the effects of the setup method, it should be called in the error path for dsa_switch_setup, not just in dsa_switch_teardown. Fixes: 5e3f847a02aa ("net: dsa: Add teardown callback for drivers") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- net/dsa/dsa2.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)