* [PATCH net] net: dsa: call teardown method on probe failure @ 2021-02-04 16:33 Vladimir Oltean 2021-02-04 17:00 ` Andrew Lunn ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Vladimir Oltean @ 2021-02-04 16:33 UTC (permalink / raw) To: David S . Miller, Jakub Kicinski Cc: netdev, Andrew Lunn, Florian Fainelli, Vivien Didelot 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(-) 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: -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: dsa: call teardown method on probe failure 2021-02-04 16:33 [PATCH net] net: dsa: call teardown method on probe failure Vladimir Oltean @ 2021-02-04 17:00 ` Andrew Lunn 2021-02-04 17:06 ` Vladimir Oltean 2021-02-04 19:33 ` Florian Fainelli 2021-02-05 4:40 ` patchwork-bot+netdevbpf 2 siblings, 1 reply; 6+ messages in thread From: Andrew Lunn @ 2021-02-04 17:00 UTC (permalink / raw) To: Vladimir Oltean Cc: David S . Miller, Jakub Kicinski, netdev, Florian Fainelli, Vivien Didelot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: dsa: call teardown method on probe failure 2021-02-04 17:00 ` Andrew Lunn @ 2021-02-04 17:06 ` Vladimir Oltean 2021-02-04 17:18 ` Andrew Lunn 0 siblings, 1 reply; 6+ messages in thread From: Vladimir Oltean @ 2021-02-04 17:06 UTC (permalink / raw) To: Andrew Lunn Cc: David S . Miller, Jakub Kicinski, netdev, Florian Fainelli, Vivien Didelot 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: dsa: call teardown method on probe failure 2021-02-04 17:06 ` Vladimir Oltean @ 2021-02-04 17:18 ` Andrew Lunn 0 siblings, 0 replies; 6+ messages in thread From: Andrew Lunn @ 2021-02-04 17:18 UTC (permalink / raw) To: Vladimir Oltean Cc: David S . Miller, Jakub Kicinski, netdev, Florian Fainelli, Vivien Didelot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: dsa: call teardown method on probe failure 2021-02-04 16:33 [PATCH net] net: dsa: call teardown method on probe failure Vladimir Oltean 2021-02-04 17:00 ` Andrew Lunn @ 2021-02-04 19:33 ` Florian Fainelli 2021-02-05 4:40 ` patchwork-bot+netdevbpf 2 siblings, 0 replies; 6+ messages in thread From: Florian Fainelli @ 2021-02-04 19:33 UTC (permalink / raw) To: Vladimir Oltean, David S . Miller, Jakub Kicinski Cc: netdev, Andrew Lunn, Vivien Didelot 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> -- Florian ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: dsa: call teardown method on probe failure 2021-02-04 16:33 [PATCH net] net: dsa: call teardown method on probe failure Vladimir Oltean 2021-02-04 17:00 ` Andrew Lunn 2021-02-04 19:33 ` Florian Fainelli @ 2021-02-05 4:40 ` patchwork-bot+netdevbpf 2 siblings, 0 replies; 6+ messages in thread From: patchwork-bot+netdevbpf @ 2021-02-05 4:40 UTC (permalink / raw) To: Vladimir Oltean; +Cc: davem, kuba, netdev, andrew, f.fainelli, vivien.didelot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-05 4:40 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-04 16:33 [PATCH net] net: dsa: call teardown method on probe failure Vladimir Oltean 2021-02-04 17:00 ` Andrew Lunn 2021-02-04 17:06 ` Vladimir Oltean 2021-02-04 17:18 ` Andrew Lunn 2021-02-04 19:33 ` Florian Fainelli 2021-02-05 4:40 ` patchwork-bot+netdevbpf
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).