IB/ipoib: Move all uninit code into ndo_uninit

Currently uninit is sometimes done twice in error flows, and is sprinkled
a bit all over the place.

Improve the clarity of the design by moving all uninit only into
ndo_uinit.

Some duplication is removed:
 - Sometimes IPOIB_STOP_NEIGH_GC was done before unregister, but
   this duplicates the process in ipoib_neigh_hash_init
 - Flushing priv->wq was sometimes done before unregister,
   but that duplicates what has been done in ndo_uninit

Uniniting the IB event queue must remain before unregister_netdev as it
requires the RTNL lock to be dropped, this is moved to a helper to make
that flow really clear and remove some duplication in error flows.

If register_netdev fails (and ndo_init is NULL) then it almost always
calls ndo_uninit, which lets us remove all the extra code from the error
unwinds. The next patch in the series will close the 'almost always' hole
by pairing a proper ndo_init with ndo_uninit.

Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>

diff --git a/drivers/infiniband/ulp/ipoib/ipoib.h b/drivers/infiniband/ulp/ipoib/ipoib.h
index c619c0098ba69225135ec07c0dc70bb41ee74d99..04fc5ad1b69f5ca62421d5039e81dc1efce3ce1a 100644 (file)
--- a/drivers/infiniband/ulp/ipoib/ipoib.h
+++ b/drivers/infiniband/ulp/ipoib/ipoib.h
@@ -509,7 +509,6 @@ int ipoib_ib_dev_stop_default(struct net_device *dev);
 void ipoib_pkey_dev_check_presence(struct net_device *dev);
 
 int ipoib_dev_init(struct net_device *dev, struct ib_device *ca, int port);
-void ipoib_dev_cleanup(struct net_device *dev);
 
 void ipoib_mcast_join_task(struct work_struct *work);
 void ipoib_mcast_carrier_on_task(struct work_struct *work);

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 84bee915105970e86a62fe74cf7614093dca325d..d4e9951dc5392aa933beaf891bf04f53d88ebb4b 100644 (file)
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -215,11 +215,6 @@ static int ipoib_stop(struct net_device *dev)
        return 0;
 }
 
-static void ipoib_uninit(struct net_device *dev)
-{
-       ipoib_dev_cleanup(dev);
-}
-
 static netdev_features_t ipoib_fix_features(struct net_device *dev, netdev_features_t features)
 {
        struct ipoib_dev_priv *priv = ipoib_priv(dev);
@@ -1816,7 +1811,33 @@ out:
        return ret;
 }
 
-void ipoib_dev_cleanup(struct net_device *dev)
+/*
+ * This must be called before doing an unregister_netdev on a parent device to
+ * shutdown the IB event handler.
+ */
+static void ipoib_parent_unregister_pre(struct net_device *ndev)
+{
+       struct ipoib_dev_priv *priv = ipoib_priv(ndev);
+
+       /*
+        * ipoib_set_mac checks netif_running before pushing work, clearing
+        * running ensures the it will not add more work.
+        */
+       rtnl_lock();
+       dev_change_flags(priv->dev, priv->dev->flags & ~IFF_UP);
+       rtnl_unlock();
+
+       /* ipoib_event() cannot be running once this returns */
+       ib_unregister_event_handler(&priv->event_handler);
+
+       /*
+        * Work on the queue grabs the rtnl lock, so this cannot be done while
+        * also holding it.
+        */
+       flush_workqueue(ipoib_workqueue);
+}
+
+static void ipoib_ndo_uninit(struct net_device *dev)
 {
        struct ipoib_dev_priv *priv = ipoib_priv(dev), *cpriv, *tcpriv;
        LIST_HEAD(head);
@@ -1888,7 +1909,7 @@ static const struct header_ops ipoib_header_ops = {
 };
 
 static const struct net_device_ops ipoib_netdev_ops_pf = {
-       .ndo_uninit              = ipoib_uninit,
+       .ndo_uninit              = ipoib_ndo_uninit,
        .ndo_open                = ipoib_open,
        .ndo_stop                = ipoib_stop,
        .ndo_change_mtu          = ipoib_change_mtu,
@@ -1907,7 +1928,7 @@ static const struct net_device_ops ipoib_netdev_ops_pf = {
 };
 
 static const struct net_device_ops ipoib_netdev_ops_vf = {
-       .ndo_uninit              = ipoib_uninit,
+       .ndo_uninit              = ipoib_ndo_uninit,
        .ndo_open                = ipoib_open,
        .ndo_stop                = ipoib_stop,
        .ndo_change_mtu          = ipoib_change_mtu,
@@ -2310,7 +2331,8 @@ static struct net_device *ipoib_add_port(const char *format,
        if (result) {
                pr_warn("%s: couldn't register ipoib port %d; error %d\n",
                        hca->name, port, result);
-               goto register_failed;
+               ipoib_parent_unregister_pre(priv->dev);
+               goto device_init_failed;
        }
 
        result = -ENOMEM;
@@ -2328,16 +2350,9 @@ static struct net_device *ipoib_add_port(const char *format,
        return priv->dev;
 
 sysfs_failed:
+       ipoib_parent_unregister_pre(priv->dev);
        unregister_netdev(priv->dev);
 
-register_failed:
-       ib_unregister_event_handler(&priv->event_handler);
-       flush_workqueue(ipoib_workqueue);
-       /* Stop GC if started before flush */
-       cancel_delayed_work_sync(&priv->neigh_reap_task);
-       flush_workqueue(priv->wq);
-       ipoib_dev_cleanup(priv->dev);
-
 device_init_failed:
        rn = netdev_priv(priv->dev);
        rn->free_rdma_netdev(priv->dev);
@@ -2391,16 +2406,7 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data)
        list_for_each_entry_safe(priv, tmp, dev_list, list) {
                struct rdma_netdev *parent_rn = netdev_priv(priv->dev);
 
-               ib_unregister_event_handler(&priv->event_handler);
-               flush_workqueue(ipoib_workqueue);
-
-               rtnl_lock();
-               dev_change_flags(priv->dev, priv->dev->flags & ~IFF_UP);
-               rtnl_unlock();
-
-               /* Stop GC */
-               cancel_delayed_work_sync(&priv->neigh_reap_task);
-               flush_workqueue(priv->wq);
+               ipoib_parent_unregister_pre(priv->dev);
 
                /* Wrap rtnl_lock/unlock with mutex to protect sysfs calls */
                mutex_lock(&priv->sysfs_mutex);

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
index 1b7bfd500893785491728d328e819bb6e15a170d..b62ab85c8eadc2f2dc4518806fecc46f44355cb7 100644 (file)
--- a/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_vlan.c
@@ -83,7 +83,7 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
        result = register_netdevice(priv->dev);
        if (result) {
                ipoib_warn(priv, "failed to initialize; error %i", result);
-               goto register_failed;
+               goto err;
        }
 
        /* RTNL childs don't need proprietary sysfs entries */
@@ -108,9 +108,6 @@ sysfs_failed:
        result = -ENOMEM;
        unregister_netdevice(priv->dev);
 
-register_failed:
-       ipoib_dev_cleanup(priv->dev);
-
 err:
        return result;
 }