Commit 7cbee87c authored by Jason Gunthorpe's avatar Jason Gunthorpe

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: default avatarJason Gunthorpe <jgg@mellanox.com>
Signed-off-by: default avatarLeon Romanovsky <leonro@mellanox.com>
parent cda8daf1
...@@ -509,7 +509,6 @@ int ipoib_ib_dev_stop_default(struct net_device *dev); ...@@ -509,7 +509,6 @@ int ipoib_ib_dev_stop_default(struct net_device *dev);
void ipoib_pkey_dev_check_presence(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); 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_join_task(struct work_struct *work);
void ipoib_mcast_carrier_on_task(struct work_struct *work); void ipoib_mcast_carrier_on_task(struct work_struct *work);
......
...@@ -215,11 +215,6 @@ static int ipoib_stop(struct net_device *dev) ...@@ -215,11 +215,6 @@ static int ipoib_stop(struct net_device *dev)
return 0; 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) static netdev_features_t ipoib_fix_features(struct net_device *dev, netdev_features_t features)
{ {
struct ipoib_dev_priv *priv = ipoib_priv(dev); struct ipoib_dev_priv *priv = ipoib_priv(dev);
...@@ -1816,7 +1811,33 @@ int ipoib_dev_init(struct net_device *dev, struct ib_device *ca, int port) ...@@ -1816,7 +1811,33 @@ int ipoib_dev_init(struct net_device *dev, struct ib_device *ca, int port)
return ret; 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; struct ipoib_dev_priv *priv = ipoib_priv(dev), *cpriv, *tcpriv;
LIST_HEAD(head); LIST_HEAD(head);
...@@ -1888,7 +1909,7 @@ static const struct header_ops ipoib_header_ops = { ...@@ -1888,7 +1909,7 @@ static const struct header_ops ipoib_header_ops = {
}; };
static const struct net_device_ops ipoib_netdev_ops_pf = { static const struct net_device_ops ipoib_netdev_ops_pf = {
.ndo_uninit = ipoib_uninit, .ndo_uninit = ipoib_ndo_uninit,
.ndo_open = ipoib_open, .ndo_open = ipoib_open,
.ndo_stop = ipoib_stop, .ndo_stop = ipoib_stop,
.ndo_change_mtu = ipoib_change_mtu, .ndo_change_mtu = ipoib_change_mtu,
...@@ -1907,7 +1928,7 @@ static const struct net_device_ops ipoib_netdev_ops_pf = { ...@@ -1907,7 +1928,7 @@ static const struct net_device_ops ipoib_netdev_ops_pf = {
}; };
static const struct net_device_ops ipoib_netdev_ops_vf = { static const struct net_device_ops ipoib_netdev_ops_vf = {
.ndo_uninit = ipoib_uninit, .ndo_uninit = ipoib_ndo_uninit,
.ndo_open = ipoib_open, .ndo_open = ipoib_open,
.ndo_stop = ipoib_stop, .ndo_stop = ipoib_stop,
.ndo_change_mtu = ipoib_change_mtu, .ndo_change_mtu = ipoib_change_mtu,
...@@ -2310,7 +2331,8 @@ static struct net_device *ipoib_add_port(const char *format, ...@@ -2310,7 +2331,8 @@ static struct net_device *ipoib_add_port(const char *format,
if (result) { if (result) {
pr_warn("%s: couldn't register ipoib port %d; error %d\n", pr_warn("%s: couldn't register ipoib port %d; error %d\n",
hca->name, port, result); hca->name, port, result);
goto register_failed; ipoib_parent_unregister_pre(priv->dev);
goto device_init_failed;
} }
result = -ENOMEM; result = -ENOMEM;
...@@ -2328,16 +2350,9 @@ static struct net_device *ipoib_add_port(const char *format, ...@@ -2328,16 +2350,9 @@ static struct net_device *ipoib_add_port(const char *format,
return priv->dev; return priv->dev;
sysfs_failed: sysfs_failed:
ipoib_parent_unregister_pre(priv->dev);
unregister_netdev(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: device_init_failed:
rn = netdev_priv(priv->dev); rn = netdev_priv(priv->dev);
rn->free_rdma_netdev(priv->dev); rn->free_rdma_netdev(priv->dev);
...@@ -2391,16 +2406,7 @@ static void ipoib_remove_one(struct ib_device *device, void *client_data) ...@@ -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) { list_for_each_entry_safe(priv, tmp, dev_list, list) {
struct rdma_netdev *parent_rn = netdev_priv(priv->dev); struct rdma_netdev *parent_rn = netdev_priv(priv->dev);
ib_unregister_event_handler(&priv->event_handler); ipoib_parent_unregister_pre(priv->dev);
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);
/* Wrap rtnl_lock/unlock with mutex to protect sysfs calls */ /* Wrap rtnl_lock/unlock with mutex to protect sysfs calls */
mutex_lock(&priv->sysfs_mutex); mutex_lock(&priv->sysfs_mutex);
......
...@@ -83,7 +83,7 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv, ...@@ -83,7 +83,7 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
result = register_netdevice(priv->dev); result = register_netdevice(priv->dev);
if (result) { if (result) {
ipoib_warn(priv, "failed to initialize; error %i", result); ipoib_warn(priv, "failed to initialize; error %i", result);
goto register_failed; goto err;
} }
/* RTNL childs don't need proprietary sysfs entries */ /* RTNL childs don't need proprietary sysfs entries */
...@@ -108,9 +108,6 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv, ...@@ -108,9 +108,6 @@ int __ipoib_vlan_add(struct ipoib_dev_priv *ppriv, struct ipoib_dev_priv *priv,
result = -ENOMEM; result = -ENOMEM;
unregister_netdevice(priv->dev); unregister_netdevice(priv->dev);
register_failed:
ipoib_dev_cleanup(priv->dev);
err: err:
return result; return result;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment