• Vladimir Oltean's avatar
    net: dsa: tear down devlink port regions when tearing down the devlink port on error · fd292c18
    Vladimir Oltean authored
    Commit 86f8b1c0 ("net: dsa: Do not make user port errors fatal")
    decided it was fine to ignore errors on certain ports that fail to
    probe, and go on with the ports that do probe fine.
    
    Commit fb6ec87f ("net: dsa: Fix type was not set for devlink port")
    noticed that devlink_port_type_eth_set(dlp, dp->slave); does not get
    called, and devlink notices after a timeout of 3600 seconds and prints a
    WARN_ON. So it went ahead to unregister the devlink port. And because
    there exists an UNUSED port flavour, we actually re-register the devlink
    port as UNUSED.
    
    Commit 08156ba4 ("net: dsa: Add devlink port regions support to
    DSA") added devlink port regions, which are set up by the driver and not
    by DSA.
    
    When we trigger the devlink port deregistration and reregistration as
    unused, devlink now prints another WARN_ON, from here:
    
    devlink_port_unregister:
    	WARN_ON(!list_empty(&devlink_port->region_list));
    
    So the port still has regions, which makes sense, because they were set
    up by the driver, and the driver doesn't know we're unregistering the
    devlink port.
    
    Somebody needs to tear them down, and optionally (actually it would be
    nice, to be consistent) set them up again for the new devlink port.
    
    But DSA's layering stays in our way quite badly here.
    
    The options I've considered are:
    
    1. Introduce a function in devlink to just change a port's type and
       flavour. No dice, devlink keeps a lot of state, it really wants the
       port to not be registered when you set its parameters, so changing
       anything can only be done by destroying what we currently have and
       recreating it.
    
    2. Make DSA cache the parameters passed to dsa_devlink_port_region_create,
       and the region returned, keep those in a list, then when the devlink
       port unregister needs to take place, the existing devlink regions are
       destroyed by DSA, and we replay the creation of new regions using the
       cached parameters. Problem: mv88e6xxx keeps the region pointers in
       chip->ports[port].region, and these will remain stale after DSA frees
       them. There are many things DSA can do, but updating mv88e6xxx's
       private pointers is not one of them.
    
    3. Just let the driver do it (i.e. introduce a very specific method
       called ds->ops->port_reinit_as_unused, which unregisters its devlink
       port devlink regions, then the old devlink port, then registers the
       new one, then the devlink port regions for it). While it does work,
       as opposed to the others, it's pretty horrible from an API
       perspective and we can do better.
    
    4. Introduce a new pair of methods, ->port_setup and ->port_teardown,
       which in the case of mv88e6xxx must register and unregister the
       devlink port regions. Call these 2 methods when the port must be
       reinitialized as unused.
    
    Naturally, I went for the 4th approach.
    
    Fixes: 08156ba4 ("net: dsa: Add devlink port regions support to DSA")
    Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
    Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
    fd292c18
devlink.c 17.1 KB