| Message ID | 20230109183120.649825-2-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | devlink: features, linecard and reporters locking cleanup | expand |
On Mon, 9 Jan 2023 19:31:10 +0100 Jiri Pirko wrote: > Note that mlx5 used this to enable devlink reload conditionally only > when device didn't act as multi port slave. Move the multi port check > into mlx5_devlink_reload_down() callback alongside with the other > checks preventing the device from reload in certain states. Right, but this is not 100% equivalent because we generate the notifications _before_ we try to reload_down: devlink_ns_change_notify(devlink, dest_net, curr_net, false); err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack); if (err) return err; IDK why, I haven't investigated.
Tue, Jan 10, 2023 at 01:55:00AM CET, kuba@kernel.org wrote: >On Mon, 9 Jan 2023 19:31:10 +0100 Jiri Pirko wrote: >> Note that mlx5 used this to enable devlink reload conditionally only >> when device didn't act as multi port slave. Move the multi port check >> into mlx5_devlink_reload_down() callback alongside with the other >> checks preventing the device from reload in certain states. > >Right, but this is not 100% equivalent because we generate the >notifications _before_ we try to reload_down: > > devlink_ns_change_notify(devlink, dest_net, curr_net, false); > err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack); > if (err) > return err; > >IDK why, I haven't investigated. Right, but that is done even in other cases where down can't be done. I I think there's a bug here, down DEL notification is sent before calling down which can potentially fail. I think the notification call should be moved after reload_down() call. Then the bahaviour would stay the same for the features case and will get fixed for the reload_down() reject cases. What do you think?
On Tue, 10 Jan 2023 08:12:10 +0100 Jiri Pirko wrote: >> Right, but this is not 100% equivalent because we generate the >> notifications _before_ we try to reload_down: >> >> devlink_ns_change_notify(devlink, dest_net, curr_net, false); >> err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack); >> if (err) >> return err; >> >> IDK why, I haven't investigated. > > Right, but that is done even in other cases where down can't be done. I > I think there's a bug here, down DEL notification is sent before calling > down which can potentially fail. I think the notification call should be > moved after reload_down() call. Then the bahaviour would stay the same > for the features case and will get fixed for the reload_down() reject > cases. What do you think? I was gonna say that it sounds reasonable, and that maybe we should be in fact using devlink_notify_register() instead of the custom instance-and-params-only devlink_ns_change_notify(). But then I looked at who added this counter-intuitive code and found out it's for a reason - see 05a7f4a8dff19. So you gotta check if mlx5 still has this problem...
On Tue, Jan 10, 2023 at 12:59:15PM -0800, Jakub Kicinski wrote: > On Tue, 10 Jan 2023 08:12:10 +0100 Jiri Pirko wrote: > >> Right, but this is not 100% equivalent because we generate the > >> notifications _before_ we try to reload_down: > >> > >> devlink_ns_change_notify(devlink, dest_net, curr_net, false); > >> err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack); > >> if (err) > >> return err; > >> > >> IDK why, I haven't investigated. > > > > Right, but that is done even in other cases where down can't be done. I > > I think there's a bug here, down DEL notification is sent before calling > > down which can potentially fail. I think the notification call should be > > moved after reload_down() call. Then the bahaviour would stay the same > > for the features case and will get fixed for the reload_down() reject > > cases. What do you think? > > I was gonna say that it sounds reasonable, and that maybe we should > be in fact using devlink_notify_register() instead of the custom > instance-and-params-only devlink_ns_change_notify(). > > But then I looked at who added this counter-intuitive code > and found out it's for a reason - see 05a7f4a8dff19. > > So you gotta check if mlx5 still has this problem... I don't see anything in the tree what will prevent the issue which I wrote in 05a7f4a8dff19. Thanks
Wed, Jan 11, 2023 at 08:36:36AM CET, leon@kernel.org wrote: >On Tue, Jan 10, 2023 at 12:59:15PM -0800, Jakub Kicinski wrote: >> On Tue, 10 Jan 2023 08:12:10 +0100 Jiri Pirko wrote: >> >> Right, but this is not 100% equivalent because we generate the >> >> notifications _before_ we try to reload_down: >> >> >> >> devlink_ns_change_notify(devlink, dest_net, curr_net, false); >> >> err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack); >> >> if (err) >> >> return err; >> >> >> >> IDK why, I haven't investigated. >> > >> > Right, but that is done even in other cases where down can't be done. I >> > I think there's a bug here, down DEL notification is sent before calling >> > down which can potentially fail. I think the notification call should be >> > moved after reload_down() call. Then the bahaviour would stay the same >> > for the features case and will get fixed for the reload_down() reject >> > cases. What do you think? >> >> I was gonna say that it sounds reasonable, and that maybe we should >> be in fact using devlink_notify_register() instead of the custom >> instance-and-params-only devlink_ns_change_notify(). >> >> But then I looked at who added this counter-intuitive code >> and found out it's for a reason - see 05a7f4a8dff19. >> >> So you gotta check if mlx5 still has this problem... > >I don't see anything in the tree what will prevent the issue >which I wrote in 05a7f4a8dff19. Okay. I will remove this patch from the set and address this in a separate patchset. Thanks! > >Thanks
On Wed, Jan 11, 2023 at 09:23:23AM +0100, Jiri Pirko wrote: > Wed, Jan 11, 2023 at 08:36:36AM CET, leon@kernel.org wrote: > >On Tue, Jan 10, 2023 at 12:59:15PM -0800, Jakub Kicinski wrote: > >> On Tue, 10 Jan 2023 08:12:10 +0100 Jiri Pirko wrote: > >> >> Right, but this is not 100% equivalent because we generate the > >> >> notifications _before_ we try to reload_down: > >> >> > >> >> devlink_ns_change_notify(devlink, dest_net, curr_net, false); > >> >> err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack); > >> >> if (err) > >> >> return err; > >> >> > >> >> IDK why, I haven't investigated. > >> > > >> > Right, but that is done even in other cases where down can't be done. I > >> > I think there's a bug here, down DEL notification is sent before calling > >> > down which can potentially fail. I think the notification call should be > >> > moved after reload_down() call. Then the bahaviour would stay the same > >> > for the features case and will get fixed for the reload_down() reject > >> > cases. What do you think? > >> > >> I was gonna say that it sounds reasonable, and that maybe we should > >> be in fact using devlink_notify_register() instead of the custom > >> instance-and-params-only devlink_ns_change_notify(). > >> > >> But then I looked at who added this counter-intuitive code > >> and found out it's for a reason - see 05a7f4a8dff19. > >> > >> So you gotta check if mlx5 still has this problem... > > > >I don't see anything in the tree what will prevent the issue > >which I wrote in 05a7f4a8dff19. > > Okay. I will remove this patch from the set and address this in a > separate patchset. Thanks! BTW, I tried your v3 series and it caused to tests which changes switchdev mode to stuck. I'll try v4 now. Thanks > > > > >Thanks
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c index 26913dc816d3..8b3e7697390f 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c @@ -1303,7 +1303,6 @@ int bnxt_dl_register(struct bnxt *bp) if (rc) goto err_dl_port_unreg; - devlink_set_features(dl, DEVLINK_F_RELOAD); out: devlink_register(dl); return 0; diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_devlink.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_devlink.c index 3d3b69605423..9a939c0b217f 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_devlink.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hclge_devlink.c @@ -114,7 +114,6 @@ int hclge_devlink_init(struct hclge_dev *hdev) priv->hdev = hdev; hdev->devlink = devlink; - devlink_set_features(devlink, DEVLINK_F_RELOAD); devlink_register(devlink); return 0; } diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_devlink.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_devlink.c index a6c3c5e8f0ab..1b535142c65a 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_devlink.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_devlink.c @@ -116,7 +116,6 @@ int hclgevf_devlink_init(struct hclgevf_dev *hdev) priv->hdev = hdev; hdev->devlink = devlink; - devlink_set_features(devlink, DEVLINK_F_RELOAD); devlink_register(devlink); return 0; } diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c index 8286e47b4bae..026f65a4e4ca 100644 --- a/drivers/net/ethernet/intel/ice/ice_devlink.c +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c @@ -1376,7 +1376,6 @@ void ice_devlink_register(struct ice_pf *pf) { struct devlink *devlink = priv_to_devlink(pf); - devlink_set_features(devlink, DEVLINK_F_RELOAD); devlink_register(devlink); } diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c index 3ae246391549..5e7736be2091 100644 --- a/drivers/net/ethernet/mellanox/mlx4/main.c +++ b/drivers/net/ethernet/mellanox/mlx4/main.c @@ -4031,7 +4031,6 @@ static int mlx4_init_one(struct pci_dev *pdev, const struct pci_device_id *id) goto err_params_unregister; pci_save_state(pdev); - devlink_set_features(devlink, DEVLINK_F_RELOAD); devl_unlock(devlink); devlink_register(devlink); return 0; diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c index 5bd83c0275f8..67bc3ade273a 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c @@ -156,6 +156,11 @@ static int mlx5_devlink_reload_down(struct devlink *devlink, bool netns_change, return -EOPNOTSUPP; } + if (mlx5_core_is_mp_slave(dev)) { + NL_SET_ERR_MSG_MOD(extack, "reload is unsupported for multi port slave"); + return -EOPNOTSUPP; + } + if (pci_num_vf(pdev)) { NL_SET_ERR_MSG_MOD(extack, "reload while VFs are present is unfavorable"); } @@ -871,7 +876,6 @@ void mlx5_devlink_traps_unregister(struct devlink *devlink) int mlx5_devlink_register(struct devlink *devlink) { - struct mlx5_core_dev *dev = devlink_priv(devlink); int err; err = devlink_params_register(devlink, mlx5_devlink_params, @@ -889,9 +893,6 @@ int mlx5_devlink_register(struct devlink *devlink) if (err) goto max_uc_list_err; - if (!mlx5_core_is_mp_slave(dev)) - devlink_set_features(devlink, DEVLINK_F_RELOAD); - return 0; max_uc_list_err: diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c index a0a06e2eff82..0b791706a9c1 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/core.c +++ b/drivers/net/ethernet/mellanox/mlxsw/core.c @@ -2211,7 +2211,6 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info, } if (!reload) { - devlink_set_features(devlink, DEVLINK_F_RELOAD); devl_unlock(devlink); devlink_register(devlink); } diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c index 738784fda117..35a0fb683801 100644 --- a/drivers/net/netdevsim/dev.c +++ b/drivers/net/netdevsim/dev.c @@ -1609,7 +1609,6 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) goto err_hwstats_exit; nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY; - devlink_set_features(devlink, DEVLINK_F_RELOAD); devl_unlock(devlink); return 0; diff --git a/include/net/devlink.h b/include/net/devlink.h index 425ecef431b7..8460b53bb2f6 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -1646,7 +1646,7 @@ static inline struct devlink *devlink_alloc(const struct devlink_ops *ops, { return devlink_alloc_ns(ops, priv_size, &init_net, dev); } -void devlink_set_features(struct devlink *devlink, u64 features); + int devl_register(struct devlink *devlink); void devl_unregister(struct devlink *devlink); void devlink_register(struct devlink *devlink); diff --git a/net/devlink/core.c b/net/devlink/core.c index a31a317626d7..4014a01c8f3d 100644 --- a/net/devlink/core.c +++ b/net/devlink/core.c @@ -114,23 +114,6 @@ struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp) goto retry; } -/** - * devlink_set_features - Set devlink supported features - * - * @devlink: devlink - * @features: devlink support features - * - * This interface allows us to set reload ops separatelly from - * the devlink_alloc. - */ -void devlink_set_features(struct devlink *devlink, u64 features) -{ - WARN_ON(features & DEVLINK_F_RELOAD && - !devlink_reload_supported(devlink->ops)); - devlink->features = features; -} -EXPORT_SYMBOL_GPL(devlink_set_features); - /** * devl_register - Register devlink instance * @devlink: devlink @@ -297,7 +280,6 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net) * all devlink instances from this namespace into init_net. */ devlinks_xa_for_each_registered_get(net, index, devlink) { - WARN_ON(!(devlink->features & DEVLINK_F_RELOAD)); devl_lock(devlink); err = 0; if (devl_is_registered(devlink)) @@ -307,7 +289,6 @@ static void __net_exit devlink_pernet_pre_exit(struct net *net) &actions_performed, NULL); devl_unlock(devlink); devlink_put(devlink); - if (err && err != -EOPNOTSUPP) pr_warn("Failed to reload devlink instance into init_net\n"); } diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h index 5d2bbe295659..82268c4579a3 100644 --- a/net/devlink/devl_internal.h +++ b/net/devlink/devl_internal.h @@ -39,7 +39,6 @@ struct devlink { struct list_head linecard_list; struct mutex linecards_lock; /* protects linecard_list */ const struct devlink_ops *ops; - u64 features; struct xarray snapshot_ids; struct devlink_dev_stats stats; struct device *dev; diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c index 1e23b2da78cc..478b81b85f03 100644 --- a/net/devlink/leftover.c +++ b/net/devlink/leftover.c @@ -4429,9 +4429,6 @@ static int devlink_nl_cmd_reload(struct sk_buff *skb, struct genl_info *info) u32 actions_performed; int err; - if (!(devlink->features & DEVLINK_F_RELOAD)) - return -EOPNOTSUPP; - err = devlink_resources_validate(devlink, NULL, info); if (err) { NL_SET_ERR_MSG_MOD(info->extack, "resources size validation failed");