diff mbox series

[net-next,5/5] devlink: hold the instance lock during eswitch_mode callbacks

Message ID 20220317042023.1470039-6-kuba@kernel.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series devlink: hold the instance lock in eswitch callbacks | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 2 this patch: 2
netdev/cc_maintainers warning 5 maintainers not CCed: idosch@nvidia.com jiri@nvidia.com leon@kernel.org linux-rdma@vger.kernel.org oss-drivers@corigine.com
netdev/build_clang success Errors and warnings before: 18 this patch: 18
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 7 this patch: 7
netdev/checkpatch warning WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Jakub Kicinski March 17, 2022, 4:20 a.m. UTC 
Make the devlink core hold the instance lock during eswitch_mode
callbacks. Cheat in case of mlx5 (see the cover letter).

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 22 +++++--------------
 .../mellanox/mlx5/core/eswitch_offloads.c     | 15 ++++++++++++-
 .../net/ethernet/netronome/nfp/nfp_devlink.c  |  7 +-----
 drivers/net/netdevsim/dev.c                   | 16 +++++---------
 net/core/devlink.c                            |  6 -----
 5 files changed, 26 insertions(+), 40 deletions(-)

Comments

Leon Romanovsky March 17, 2022, 7:52 a.m. UTC | #1 
On Wed, Mar 16, 2022 at 09:20:23PM -0700, Jakub Kicinski wrote:
> Make the devlink core hold the instance lock during eswitch_mode
> callbacks. Cheat in case of mlx5 (see the cover letter).

And this is one the main difference between your and mine proposals/solutions.
I didn't want to cheat as it doesn't help to the end goal - remove devlink_mutex.

Can you please change the comments in mlx5 to be more direct?
"/* TODO: convert the driver to devl_* */"
->
"/* FIXME: devl_unlock() followed by devl_lock() inside driver callback
  * never correct and prone to races. Never repeat this pattern.
  *
  * This code MUST be fixed before removing devlink_mutex as it is safe
  * to do only because of that mutex.
  */"

Something like that.

The code is correct, but like I said before, I don't like the direction.
I expect that this anti-pattern is going to be copy/pasted very soon.

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
Jiri Pirko March 17, 2022, 8:37 a.m. UTC | #2 
Thu, Mar 17, 2022 at 05:20:23AM CET, kuba@kernel.org wrote:
>Make the devlink core hold the instance lock during eswitch_mode
>callbacks. Cheat in case of mlx5 (see the cover letter).
>
>Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Looks fine.

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

I agree with Leon that it would be better to have the "TODO" comment
elsewhere.
Jakub Kicinski March 17, 2022, 3:41 p.m. UTC | #3 
On Thu, 17 Mar 2022 09:52:10 +0200 Leon Romanovsky wrote:
> On Wed, Mar 16, 2022 at 09:20:23PM -0700, Jakub Kicinski wrote:
> > Make the devlink core hold the instance lock during eswitch_mode
> > callbacks. Cheat in case of mlx5 (see the cover letter).  
> 
> And this is one the main difference between your and mine proposals/solutions.
> I didn't want to cheat as it doesn't help to the end goal - remove devlink_mutex.
> 
> Can you please change the comments in mlx5 to be more direct?
> "/* TODO: convert the driver to devl_* */"
> ->  
> "/* FIXME: devl_unlock() followed by devl_lock() inside driver callback
>   * never correct and prone to races. Never repeat this pattern.
>   *
>   * This code MUST be fixed before removing devlink_mutex as it is safe
>   * to do only because of that mutex.
>   */"
> 
> Something like that.

Should I add this comment in all the spots I'm adding the re-locking?
Does it make sense to add a wrapper:

/* FIXME: devl_unlock() followed by devl_lock() inside driver callback
 * never correct and prone to races. Never repeat this pattern.
 *
 * This code MUST be fixed before removing devlink_mutex as it is safe
 * to do only because of that mutex.
 */
static void mlx5_eswtich_mode_callback_enter(...)
{
	devl_unlock(devlink);
 	down_write(&esw->mode_lock);
}

and respective helper for "leave" ?

> The code is correct, but like I said before, I don't like the direction.
> I expect that this anti-pattern is going to be copy/pasted very soon.

Yeah, this is most certainly a very temporary hack.

BTW is there a chance for me to access a fully featured mlx5 system
somehow to test the future conversion patches? Or perhaps you or
someone on the team would be interested in doing the conversion?

> Thanks,
> Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Thanks!
diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
index b2a9528b456b..eb4803b11c0e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
@@ -559,44 +559,34 @@  int bnxt_dl_eswitch_mode_set(struct devlink *devlink, u16 mode,
 			     struct netlink_ext_ack *extack)
 {
 	struct bnxt *bp = bnxt_get_bp_from_dl(devlink);
-	int rc = 0;
 
-	devl_lock(devlink);
 	if (bp->eswitch_mode == mode) {
 		netdev_info(bp->dev, "already in %s eswitch mode\n",
 			    mode == DEVLINK_ESWITCH_MODE_LEGACY ?
 			    "legacy" : "switchdev");
-		rc = -EINVAL;
-		goto done;
+		return -EINVAL;
 	}
 
 	switch (mode) {
 	case DEVLINK_ESWITCH_MODE_LEGACY:
 		bnxt_vf_reps_destroy(bp);
-		break;
+		return 0;
 
 	case DEVLINK_ESWITCH_MODE_SWITCHDEV:
 		if (bp->hwrm_spec_code < 0x10803) {
 			netdev_warn(bp->dev, "FW does not support SRIOV E-Switch SWITCHDEV mode\n");
-			rc = -ENOTSUPP;
-			goto done;
+			return -ENOTSUPP;
 		}
 
 		if (pci_num_vf(bp->pdev) == 0) {
 			netdev_info(bp->dev, "Enable VFs before setting switchdev mode\n");
-			rc = -EPERM;
-			goto done;
+			return -EPERM;
 		}
-		rc = bnxt_vf_reps_create(bp);
-		break;
+		return bnxt_vf_reps_create(bp);
 
 	default:
-		rc = -EINVAL;
-		goto done;
+		return -EINVAL;
 	}
-done:
-	devl_unlock(devlink);
-	return rc;
 }
 
 #endif
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 35cf4cb3098e..590171a03e4e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -3351,6 +3351,8 @@  int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode,
 	if (esw_mode_from_devlink(mode, &mlx5_mode))
 		return -EINVAL;
 
+	devl_unlock(devlink); /* TODO: convert the driver to devl_* */
+
 	mlx5_lag_disable_change(esw->dev);
 	err = mlx5_esw_try_lock(esw);
 	if (err < 0) {
@@ -3381,6 +3383,7 @@  int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode,
 	mlx5_esw_unlock(esw);
 enable_lag:
 	mlx5_lag_enable_change(esw->dev);
+	devl_lock(devlink);
 	return err;
 }
 
@@ -3393,6 +3396,7 @@  int mlx5_devlink_eswitch_mode_get(struct devlink *devlink, u16 *mode)
 	if (IS_ERR(esw))
 		return PTR_ERR(esw);
 
+	devl_unlock(devlink); /* TODO: convert the driver to devl_* */
 	down_write(&esw->mode_lock);
 	err = eswitch_devlink_esw_mode_check(esw);
 	if (err)
@@ -3401,6 +3405,7 @@  int mlx5_devlink_eswitch_mode_get(struct devlink *devlink, u16 *mode)
 	err = esw_mode_to_devlink(esw->mode, mode);
 unlock:
 	up_write(&esw->mode_lock);
+	devl_lock(devlink);
 	return err;
 }
 
@@ -3447,6 +3452,7 @@  int mlx5_devlink_eswitch_inline_mode_set(struct devlink *devlink, u8 mode,
 	if (IS_ERR(esw))
 		return PTR_ERR(esw);
 
+	devl_unlock(devlink); /* TODO: convert the driver to devl_* */
 	down_write(&esw->mode_lock);
 	err = eswitch_devlink_esw_mode_check(esw);
 	if (err)
@@ -3485,10 +3491,12 @@  int mlx5_devlink_eswitch_inline_mode_set(struct devlink *devlink, u8 mode,
 
 	esw->offloads.inline_mode = mlx5_mode;
 	up_write(&esw->mode_lock);
+	devl_lock(devlink);
 	return 0;
 
 out:
 	up_write(&esw->mode_lock);
+	devl_lock(devlink);
 	return err;
 }
 
@@ -3501,6 +3509,7 @@  int mlx5_devlink_eswitch_inline_mode_get(struct devlink *devlink, u8 *mode)
 	if (IS_ERR(esw))
 		return PTR_ERR(esw);
 
+	devl_unlock(devlink); /* TODO: convert the driver to devl_* */
 	down_write(&esw->mode_lock);
 	err = eswitch_devlink_esw_mode_check(esw);
 	if (err)
@@ -3509,6 +3518,7 @@  int mlx5_devlink_eswitch_inline_mode_get(struct devlink *devlink, u8 *mode)
 	err = esw_inline_mode_to_devlink(esw->offloads.inline_mode, mode);
 unlock:
 	up_write(&esw->mode_lock);
+	devl_lock(devlink);
 	return err;
 }
 
@@ -3524,6 +3534,7 @@  int mlx5_devlink_eswitch_encap_mode_set(struct devlink *devlink,
 	if (IS_ERR(esw))
 		return PTR_ERR(esw);
 
+	devl_unlock(devlink); /* TODO: convert the driver to devl_* */
 	down_write(&esw->mode_lock);
 	err = eswitch_devlink_esw_mode_check(esw);
 	if (err)
@@ -3571,6 +3582,7 @@  int mlx5_devlink_eswitch_encap_mode_set(struct devlink *devlink,
 
 unlock:
 	up_write(&esw->mode_lock);
+	devl_lock(devlink);
 	return err;
 }
 
@@ -3584,7 +3596,7 @@  int mlx5_devlink_eswitch_encap_mode_get(struct devlink *devlink,
 	if (IS_ERR(esw))
 		return PTR_ERR(esw);
 
-
+	devl_unlock(devlink); /* TODO: convert the driver to devl_* */
 	down_write(&esw->mode_lock);
 	err = eswitch_devlink_esw_mode_check(esw);
 	if (err)
@@ -3593,6 +3605,7 @@  int mlx5_devlink_eswitch_encap_mode_get(struct devlink *devlink,
 	*encap = esw->offloads.encap;
 unlock:
 	up_write(&esw->mode_lock);
+	devl_lock(devlink);
 	return err;
 }
 
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
index 48b95566b52b..405786c00334 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
@@ -144,13 +144,8 @@  static int nfp_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode,
 					struct netlink_ext_ack *extack)
 {
 	struct nfp_pf *pf = devlink_priv(devlink);
-	int ret;
-
-	devl_lock(devlink);
-	ret = nfp_app_eswitch_mode_set(pf->app, mode);
-	devl_unlock(devlink);
 
-	return ret;
+	return nfp_app_eswitch_mode_set(pf->app, mode);
 }
 
 static const struct nfp_devlink_versions_simple {
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 68cd1defe990..57a3ac893792 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -615,22 +615,16 @@  static int nsim_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode,
 					 struct netlink_ext_ack *extack)
 {
 	struct nsim_dev *nsim_dev = devlink_priv(devlink);
-	int err = 0;
 
-	devl_lock(devlink);
 	if (mode == nsim_dev->esw_mode)
-		goto unlock;
+		return 0;
 
 	if (mode == DEVLINK_ESWITCH_MODE_LEGACY)
-		err = nsim_esw_legacy_enable(nsim_dev, extack);
-	else if (mode == DEVLINK_ESWITCH_MODE_SWITCHDEV)
-		err = nsim_esw_switchdev_enable(nsim_dev, extack);
-	else
-		err = -EINVAL;
+		return nsim_esw_legacy_enable(nsim_dev, extack);
+	if (mode == DEVLINK_ESWITCH_MODE_SWITCHDEV)
+		return nsim_esw_switchdev_enable(nsim_dev, extack);
 
-unlock:
-	devl_unlock(devlink);
-	return err;
+	return -EINVAL;
 }
 
 static int nsim_devlink_eswitch_mode_get(struct devlink *devlink, u16 *mode)
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 5aac5370c136..aeca13b6e57b 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2868,15 +2868,11 @@  static int devlink_rate_nodes_check(struct devlink *devlink, u16 mode,
 {
 	struct devlink_rate *devlink_rate;
 
-	/* Take the lock to sync with destroy */
-	mutex_lock(&devlink->lock);
 	list_for_each_entry(devlink_rate, &devlink->rate_list, list)
 		if (devlink_rate_is_node(devlink_rate)) {
-			mutex_unlock(&devlink->lock);
 			NL_SET_ERR_MSG_MOD(extack, "Rate node(s) exists.");
 			return -EBUSY;
 		}
-	mutex_unlock(&devlink->lock);
 	return 0;
 }
 
@@ -8735,14 +8731,12 @@  static const struct genl_small_ops devlink_nl_ops[] = {
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
 		.doit = devlink_nl_cmd_eswitch_get_doit,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NO_LOCK,
 	},
 	{
 		.cmd = DEVLINK_CMD_ESWITCH_SET,
 		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
 		.doit = devlink_nl_cmd_eswitch_set_doit,
 		.flags = GENL_ADMIN_PERM,
-		.internal_flags = DEVLINK_NL_FLAG_NO_LOCK,
 	},
 	{
 		.cmd = DEVLINK_CMD_DPIPE_TABLE_GET,