[PATCH net] net: dsa: call teardown method on probe failure

[PATCH net] net: dsa: call teardown method on probe failure

[Vladimir Oltean]

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

Re: [PATCH net] net: dsa: call teardown method on probe failure

[Andrew Lunn]

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

Re: [PATCH net] net: dsa: call teardown method on probe failure

[Vladimir Oltean]

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?

Re: [PATCH net] net: dsa: call teardown method on probe failure

[Andrew Lunn]

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

Re: [PATCH net] net: dsa: call teardown method on probe failure

[Florian Fainelli]

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

Re: [PATCH net] net: dsa: call teardown method on probe failure

[patchwork-bot+netdevbpf]

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