Hi Jason,
> From: Parav Pandit <parav@xxxxxxxxxx>
> Sent: Tuesday, October 20, 2020 12:57 AM
>
>
>
> > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Sent: Tuesday, October 20, 2020 12:31 AM
> >
> > On Mon, Oct 19, 2020 at 01:23:23PM +0000, Parav Pandit wrote:
> > > > > - err = register_netdevice_notifier(&dev-
> >port[port_num].roce.nb);
> > > > > + err = register_netdevice_notifier_net(mlx5_core_net(dev-
> >mdev),
> > > > > + &dev-
> >port[port_num].roce.nb);
> > > >
> > > > This looks racy, what lock needs to be held to keep
> > > > *mlx5_core_net()
> > stable?
> > >
> > > mlx5_core_net() cannot be accessed outside of mlx5 driver's load,
> > > unload,
> > reload path.
> > >
> > > When this is getting executed, devlink cannot be executing reload.
> > > This is guarded by devlink_reload_enable/disable calls done by mlx5 core.
> >
> > A comment that devlink_reload_enable/disable() must be held would be
> > helpful
> >
> Yes. will add.
>
> > > >
> > > > > if (err) {
> > > > > dev->port[port_num].roce.nb.notifier_call = NULL;
> > > > > return err;
> > > > > @@ -3335,7 +3336,8 @@ static int mlx5_add_netdev_notifier(struct
> > > > >mlx5_ib_dev *dev, u8 port_num) static void
> > > > >mlx5_remove_netdev_notifier(struct mlx5_ib_dev *dev, u8
> port_num)
> > {
> > > > > if (dev->port[port_num].roce.nb.notifier_call) {
> > > > > - unregister_netdevice_notifier(&dev-
> > > > >port[port_num].roce.nb);
> > > > > +
> unregister_netdevice_notifier_net(mlx5_core_net(dev-
> > > > >mdev),
> > > > > + &dev-
> > > > >port[port_num].roce.nb);
> > > >
> > > > This seems dangerous too, what if the mlx5_core_net changed before
> > > > we get here?
> > >
> > > When I inspected driver, code, I am not aware of any code flow where
> > > this can change before reaching here, because registration and
> > > unregistration is done only in driver load, unload and reload path.
> > > Reload can happen only after devlink_reload_enable() is done.
> >
> > But we enable reload right after init_one
> >
> > > > What are the rules for when devlink_net() changes?
> > > >
> > > devlink_net() changes only after unload() callback is completed in driver.
> >
> > You mean mlx5_devlink_reload_down ?
> >
> Right.
> > That seems OK then
> Ok. will work with Leon to add the comment.
Is below fix up ok?
commit 33cf8a09e735849f622e8084a7b08d421f11a4e1 (HEAD -> netns-del-fix)
Author: Parav Pandit <parav@xxxxxxxxxx>
Date: Tue Oct 20 12:26:08 2020 +0300
fixup: for RDMA/mlx5: Fix devlink deadlock on net namespace deletion
Changelog:
v0->v1:
- Added kdoc comment description for the API usage and allowed context
issue: 2230150
Change-Id: Ibd233f771682c27565f48c54cd48fd87b0a7790f
Signed-off-by: Parav Pandit <parav@xxxxxxxxxx>
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index 560b551d5ff8..3382855b7ef1 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -1209,6 +1209,19 @@ static inline bool mlx5_is_roce_enabled(struct mlx5_core_dev *dev)
return val.vbool;
}
+/**
+ * mlx5_core_net - Provide net namespace of the mlx5_core_dev
+ * @dev: mlx5 core device
+ *
+ * mlx5_core_net() returns the net namespace of mlx5 core device.
+ * This can be called only in below described limited context.
+ * (a) When a devlink instance for mlx5_core is registered and
+ * when devlink reload operation is disabled.
+ * or
+ * (b) during devlink reload reload_down() and reload_up callbacks
+ * where it is ensured that devlink instance's net namespace is
+ * stable.
+ */
static inline struct net *mlx5_core_net(struct mlx5_core_dev *dev)
{
return devlink_net(priv_to_devlink(dev));
