| Message ID | 1461351647-27412-13-git-send-email-saeedm@mellanox.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi, Fri, Apr 22, 2016 at 10:00:47PM IDT, saeedm@mellanox.com wrote: >From: Eran Ben Elisha <eranbe@mellanox.com> > >Disable link up when initializing the HCA. Link up/down will be changed >using (Ports Administrative and Operational Status Register) PAOS >commands. > >If link layer is Ethernet, up/down the link in ndo_open/stop. If link >layer is IB, up/down the link as part of the mlx5 IB add/remove flow. > >Before this patch the link was up when the HCA was initialized, now the >driver will manage the link. > >Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com> >Signed-off-by: Eugenia Emantayev <eugenia@mellanox.com> >Signed-off-by: Saeed Mahameed <saeedm@mellanox.com> >--- mutt suggests I sent you the following patches a while ago: net/mlx5e: Set port administrative status in ndo_{open, close} net/mlx5e: Initialize port administrative status to down So I'm curious as to why you didn't follow up on them and instead came up with this patch. When I initially debugged this I pointed you to the fact that the firmware also needs to be patched, as setting the administrative status down via PAOS doesn't really do anything. The operational status will remain up. I just tested this patchset with the latest release firmware (12.14.2036) and I'm bumping into the same problem. Until this is properly fixed the mlx5 driver is blacklisted in our test: https://github.com/jpirko/lnst/blob/master/recipes/switchdev/basic-001-links.py#L19 Also, why is this directed at net-next? > drivers/infiniband/hw/mlx5/main.c | 11 +++++++++++ > drivers/infiniband/hw/mlx5/mlx5_ib.h | 5 +++++ > drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c | 4 ++-- > .../net/ethernet/mellanox/mlx5/core/en_ethtool.c | 4 ++-- > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 4 ++++ > drivers/net/ethernet/mellanox/mlx5/core/main.c | 4 ++++ > drivers/net/ethernet/mellanox/mlx5/core/port.c | 5 +++-- > include/linux/mlx5/port.h | 3 ++- > 8 files changed, 33 insertions(+), 7 deletions(-) > [...] >diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c >index b2db180..f083797 100644 >--- a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c >+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c >@@ -202,12 +202,12 @@ static int mlx5e_dcbnl_ieee_setpfc(struct net_device *dev, > > mlx5_query_port_admin_status(mdev, &ps); > if (ps == MLX5_PORT_UP) >- mlx5_set_port_admin_status(mdev, MLX5_PORT_DOWN); >+ mlx5_set_port_admin_status(mdev, MLX5_PORT_DOWN, 1); Why is this needed? The link doesn't need to go through training when setting PFC. It's only needed for PAUSE frames when auto-negotiation is on, but you don't support that anyway. Thanks. > > ret = mlx5_set_port_pfc(mdev, pfc->pfc_en, pfc->pfc_en); > > if (ps == MLX5_PORT_UP) >- mlx5_set_port_admin_status(mdev, MLX5_PORT_UP); >+ mlx5_set_port_admin_status(mdev, MLX5_PORT_UP, 1); > > return ret; > }
On Sat, Apr 23, 2016 at 7:00 PM, Ido Schimmel <idosch@mellanox.com> wrote: > mutt suggests I sent you the following patches a while ago: > > net/mlx5e: Set port administrative status in ndo_{open, close} > net/mlx5e: Initialize port administrative status to down > > So I'm curious as to why you didn't follow up on them and instead came > up with this patch. > > When I initially debugged this I pointed you to the fact that the > firmware also needs to be patched, as setting the administrative status > down via PAOS doesn't really do anything. The operational status will > remain up. I just tested this patchset with the latest release firmware > (12.14.2036) and I'm bumping into the same problem. > Hi Ido, I do remember that patch, and back then i told you that your patch is not cooked enough and that we were working on a solution: 1. you need to negotiate with the FW on if the FW can give up port ownership to SW, you can find that in handle_hca_cap, MLX5_CAP_GEN_MAX(dev, disable_link_up). 2. you need to configure the FW to give up port ownership via NVconfig (mlxconfig tool) i think it is PORT_OWNER bit, Eran can elaborate more if needed. 3.1 and 2 are required for mlx5_set_port_admin_status to take effect, otherwise it is a NOOP. Eran tested this patch and it works for him, maybe you missed step 2. Step 2 will not be required in future FW (it will be the FW default). Other than this patch, nothing more is required for driver to assume ownership on port's link status. > Until this is properly fixed the mlx5 driver is blacklisted in our test: > https://github.com/jpirko/lnst/blob/master/recipes/switchdev/basic-001-links.py#L19 > > Also, why is this directed at net-next? > it is kind of new behavior, and not a bug fix. >> drivers/infiniband/hw/mlx5/main.c | 11 +++++++++++ >> drivers/infiniband/hw/mlx5/mlx5_ib.h | 5 +++++ >> drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c | 4 ++-- >> .../net/ethernet/mellanox/mlx5/core/en_ethtool.c | 4 ++-- >> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 4 ++++ >> drivers/net/ethernet/mellanox/mlx5/core/main.c | 4 ++++ >> drivers/net/ethernet/mellanox/mlx5/core/port.c | 5 +++-- >> include/linux/mlx5/port.h | 3 ++- >> 8 files changed, 33 insertions(+), 7 deletions(-) >> > > [...] > >>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c >>index b2db180..f083797 100644 >>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c >>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c >>@@ -202,12 +202,12 @@ static int mlx5e_dcbnl_ieee_setpfc(struct net_device *dev, >> >> mlx5_query_port_admin_status(mdev, &ps); >> if (ps == MLX5_PORT_UP) >>- mlx5_set_port_admin_status(mdev, MLX5_PORT_DOWN); >>+ mlx5_set_port_admin_status(mdev, MLX5_PORT_DOWN, 1); > > Why is this needed? The link doesn't need to go through training when > setting PFC. It's only needed for PAUSE frames when auto-negotiation is > on, but you don't support that anyway. > > Thanks. > The code was there before this patch, only the function prototype was changed. Maybe you have a valid point here, but this need to be fixed in a different patch, it has nothing to do with this one. Eran and Team please check this on Sunday, I found nothing re this matter in PRM.
On Sat, Apr 23, 2016 at 11:21 PM, Saeed Mahameed <saeedm@dev.mellanox.co.il> wrote: > On Sat, Apr 23, 2016 at 7:00 PM, Ido Schimmel <idosch@mellanox.com> wrote: >> Also, why is this directed at net-next? > it is kind of new behavior, and not a bug fix. Saeed, We should be aiming to patch the driver around this issue with something that goes all the way back through -stable to the very 1st upstream kernel supporting mlx5 EN --- the operational link doesn't go down when instructed by the admin and this is an entry level requirement from Ethernet of things (such as NIC and Switch ports).
On Sat, Apr 23, 2016 at 11:28 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote: > On Sat, Apr 23, 2016 at 11:21 PM, Saeed Mahameed > <saeedm@dev.mellanox.co.il> wrote: >> On Sat, Apr 23, 2016 at 7:00 PM, Ido Schimmel <idosch@mellanox.com> wrote: > >>> Also, why is this directed at net-next? > >> it is kind of new behavior, and not a bug fix. > > Saeed, > > We should be aiming to patch the driver around this issue with > something that goes all the way back through -stable to the very 1st > upstream kernel supporting mlx5 EN --- the operational link doesn't go > down when instructed by the admin and this is an entry level > requirement from Ethernet of things (such as NIC and Switch ports). Well, this feature requires updating mlx5_ifc, which was done in earlier patch which added disable_link_up[0x1] bit, I think it is doable, but it will introduce a conflict that Dave might need to solve by not taking this bit from the patch I will send to net if we decide to do so, are you saying that i need to drop this patch and submit it to net ?
On Sun, Apr 24, 2016 at 12:24 AM, Saeed Mahameed <saeedm@dev.mellanox.co.il> wrote: > On Sat, Apr 23, 2016 at 11:28 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote: >> On Sat, Apr 23, 2016 at 11:21 PM, Saeed Mahameed >> <saeedm@dev.mellanox.co.il> wrote: >>> On Sat, Apr 23, 2016 at 7:00 PM, Ido Schimmel <idosch@mellanox.com> wrote: >> >>>> Also, why is this directed at net-next? >> >>> it is kind of new behavior, and not a bug fix. >> >> Saeed, >> >> We should be aiming to patch the driver around this issue with >> something that goes all the way back through -stable to the very 1st >> upstream kernel supporting mlx5 EN --- the operational link doesn't go >> down when instructed by the admin and this is an entry level >> requirement from Ethernet of things (such as NIC and Switch ports). > > Well, this feature requires updating mlx5_ifc, which was done in > earlier patch which added disable_link_up[0x1] bit, > I think it is doable, but it will introduce a conflict that Dave might > need to solve by not taking this bit from the patch I will send to net > if we decide to do so, > are you saying that i need to drop this patch and submit it to net ? net-next is rebased over net and when needed conflicts are solved. Do the right patch for net and we will take it from there. Please make sure your patch/es meet the -stable criteria so they can be pushed there as well. Or.
Sat, Apr 23, 2016 at 11:21:45PM IDT, saeedm@dev.mellanox.co.il wrote: >On Sat, Apr 23, 2016 at 7:00 PM, Ido Schimmel <idosch@mellanox.com> wrote: >> mutt suggests I sent you the following patches a while ago: >> >> net/mlx5e: Set port administrative status in ndo_{open, close} >> net/mlx5e: Initialize port administrative status to down >> >> So I'm curious as to why you didn't follow up on them and instead came >> up with this patch. >> >> When I initially debugged this I pointed you to the fact that the >> firmware also needs to be patched, as setting the administrative status >> down via PAOS doesn't really do anything. The operational status will >> remain up. I just tested this patchset with the latest release firmware >> (12.14.2036) and I'm bumping into the same problem. >> > >Hi Ido, > >I do remember that patch, and back then i told you that your patch is >not cooked enough and that we were working on a solution: > >1. you need to negotiate with the FW on if the FW can give up port >ownership to SW, you can find that in handle_hca_cap, >MLX5_CAP_GEN_MAX(dev, disable_link_up). Yea, I don't think that part was available back then? I just directly set a value in firmware memory hoping it would be the default in the next version following my report. >2. you need to configure the FW to give up port ownership via NVconfig >(mlxconfig tool) i think it is PORT_OWNER bit, Eran can elaborate >more if needed. I'll check that tomorrow morning and report. >3.1 and 2 are required for mlx5_set_port_admin_status to take effect, >otherwise it is a NOOP. > >Eran tested this patch and it works for him, maybe you missed step 2. >Step 2 will not be required in future FW (it will be the FW default). Great! >Other than this patch, nothing more is required for driver to assume >ownership on port's link status. > >> Until this is properly fixed the mlx5 driver is blacklisted in our test: >> https://github.com/jpirko/lnst/blob/master/recipes/switchdev/basic-001-links.py#L19 >> >> Also, why is this directed at net-next? >> > >it is kind of new behavior, and not a bug fix. > >>> drivers/infiniband/hw/mlx5/main.c | 11 +++++++++++ >>> drivers/infiniband/hw/mlx5/mlx5_ib.h | 5 +++++ >>> drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c | 4 ++-- >>> .../net/ethernet/mellanox/mlx5/core/en_ethtool.c | 4 ++-- >>> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 4 ++++ >>> drivers/net/ethernet/mellanox/mlx5/core/main.c | 4 ++++ >>> drivers/net/ethernet/mellanox/mlx5/core/port.c | 5 +++-- >>> include/linux/mlx5/port.h | 3 ++- >>> 8 files changed, 33 insertions(+), 7 deletions(-) >>> >> >> [...] >> >>>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c >>>index b2db180..f083797 100644 >>>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c >>>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c >>>@@ -202,12 +202,12 @@ static int mlx5e_dcbnl_ieee_setpfc(struct net_device *dev, >>> >>> mlx5_query_port_admin_status(mdev, &ps); >>> if (ps == MLX5_PORT_UP) >>>- mlx5_set_port_admin_status(mdev, MLX5_PORT_DOWN); >>>+ mlx5_set_port_admin_status(mdev, MLX5_PORT_DOWN, 1); >> >> Why is this needed? The link doesn't need to go through training when >> setting PFC. It's only needed for PAUSE frames when auto-negotiation is >> on, but you don't support that anyway. >> >> Thanks. >> > >The code was there before this patch, only the function prototype was changed. >Maybe you have a valid point here, but this need to be fixed in a >different patch, it has nothing to do with this one. >Eran and Team please check this on Sunday, I found nothing re this >matter in PRM. See PFCC register: "Setting pfctx / pfcrx when the link is up takes effect immediately." Thanks.
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c index 5acf346..6fd365f 100644 --- a/drivers/infiniband/hw/mlx5/main.c +++ b/drivers/infiniband/hw/mlx5/main.c @@ -2261,6 +2261,9 @@ static void *mlx5_ib_add(struct mlx5_core_dev *mdev) if (err) goto err_dealloc; + mlx5_foreach_port(dev, i, IB_LINK_LAYER_INFINIBAND) + mlx5_set_port_admin_status(dev->mdev, MLX5_PORT_UP, i + 1); + if (mlx5_use_mad_ifc(dev)) get_ext_port_caps(dev); @@ -2443,6 +2446,9 @@ err_disable_roce: if (ll == IB_LINK_LAYER_ETHERNET) mlx5_disable_roce(dev); + mlx5_foreach_port(dev, i, IB_LINK_LAYER_INFINIBAND) + mlx5_set_port_admin_status(mdev, MLX5_PORT_DOWN, i + 1); + err_dealloc: ib_dealloc_device((struct ib_device *)dev); @@ -2453,6 +2459,7 @@ static void mlx5_ib_remove(struct mlx5_core_dev *mdev, void *context) { struct mlx5_ib_dev *dev = context; enum rdma_link_layer ll = mlx5_ib_port_link_layer(&dev->ib_dev, 1); + int i; ib_unregister_device(&dev->ib_dev); destroy_umrc_res(dev); @@ -2460,6 +2467,10 @@ static void mlx5_ib_remove(struct mlx5_core_dev *mdev, void *context) destroy_dev_resources(&dev->devr); if (ll == IB_LINK_LAYER_ETHERNET) mlx5_disable_roce(dev); + + mlx5_foreach_port(dev, i, IB_LINK_LAYER_INFINIBAND) + mlx5_set_port_admin_status(mdev, MLX5_PORT_DOWN, i + 1); + ib_dealloc_device(&dev->ib_dev); } diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h index b46c255..4585fca 100644 --- a/drivers/infiniband/hw/mlx5/mlx5_ib.h +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h @@ -62,6 +62,11 @@ pr_warn("%s:%s:%d:(pid %d): " format, (dev)->ib_dev.name, __func__, \ #define MLX5_IB_DEFAULT_UIDX 0xffffff #define MLX5_USER_ASSIGNED_UIDX_MASK __mlx5_mask(qpc, user_index) +#define mlx5_foreach_port(dev, p, type) \ + for ((p) = 0; (p) < MLX5_CAP_GEN(dev->mdev, num_ports); (p)++) \ + if (mlx5_ib_port_link_layer(&dev->ib_dev, (p) + 1) == \ + (type)) + enum { MLX5_IB_MMAP_CMD_SHIFT = 8, MLX5_IB_MMAP_CMD_MASK = 0xff, diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c index b2db180..f083797 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c @@ -202,12 +202,12 @@ static int mlx5e_dcbnl_ieee_setpfc(struct net_device *dev, mlx5_query_port_admin_status(mdev, &ps); if (ps == MLX5_PORT_UP) - mlx5_set_port_admin_status(mdev, MLX5_PORT_DOWN); + mlx5_set_port_admin_status(mdev, MLX5_PORT_DOWN, 1); ret = mlx5_set_port_pfc(mdev, pfc->pfc_en, pfc->pfc_en); if (ps == MLX5_PORT_UP) - mlx5_set_port_admin_status(mdev, MLX5_PORT_UP); + mlx5_set_port_admin_status(mdev, MLX5_PORT_UP, 1); return ret; } diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c index 0518c86..2fed67f 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c @@ -777,10 +777,10 @@ static int mlx5e_set_settings(struct net_device *netdev, mlx5_query_port_admin_status(mdev, &ps); if (ps == MLX5_PORT_UP) - mlx5_set_port_admin_status(mdev, MLX5_PORT_DOWN); + mlx5_set_port_admin_status(mdev, MLX5_PORT_DOWN, 1); mlx5_set_port_proto(mdev, link_modes, MLX5_PTYS_EN); if (ps == MLX5_PORT_UP) - mlx5_set_port_admin_status(mdev, MLX5_PORT_UP); + mlx5_set_port_admin_status(mdev, MLX5_PORT_UP, 1); out: return err; diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c index 5bad17d..6c9fa9f 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c @@ -1632,9 +1632,12 @@ static int mlx5e_open(struct net_device *netdev) { struct mlx5e_priv *priv = netdev_priv(netdev); int err; + u8 state; mutex_lock(&priv->state_lock); err = mlx5e_open_locked(netdev); + state = err ? MLX5_PORT_DOWN : MLX5_PORT_UP; + mlx5_set_port_admin_status(priv->mdev, state, 1); mutex_unlock(&priv->state_lock); return err; @@ -1666,6 +1669,7 @@ static int mlx5e_close(struct net_device *netdev) int err; mutex_lock(&priv->state_lock); + mlx5_set_port_admin_status(priv->mdev, MLX5_PORT_DOWN, 1); err = mlx5e_close_locked(netdev); mutex_unlock(&priv->state_lock); diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c index 3f3b2fa..6211cc3 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c @@ -497,6 +497,10 @@ static int handle_hca_cap(struct mlx5_core_dev *dev) MLX5_SET(cmd_hca_cap, set_hca_cap, log_uar_page_sz, PAGE_SHIFT - 12); + /* disable link up by INIT_HCA */ + if (MLX5_CAP_GEN_MAX(dev, disable_link_up)) + MLX5_SET(cmd_hca_cap, set_hca_cap, disable_link_up, 1); + err = set_caps(dev, set_ctx, set_sz, MLX5_SET_HCA_CAP_OP_MOD_GENERAL_DEVICE); diff --git a/drivers/net/ethernet/mellanox/mlx5/core/port.c b/drivers/net/ethernet/mellanox/mlx5/core/port.c index 4cb2a44..5036e14 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/port.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/port.c @@ -223,14 +223,15 @@ int mlx5_set_port_proto(struct mlx5_core_dev *dev, u32 proto_admin, EXPORT_SYMBOL_GPL(mlx5_set_port_proto); int mlx5_set_port_admin_status(struct mlx5_core_dev *dev, - enum mlx5_port_status status) + enum mlx5_port_status status, + u8 port) { u32 in[MLX5_ST_SZ_DW(paos_reg)]; u32 out[MLX5_ST_SZ_DW(paos_reg)]; memset(in, 0, sizeof(in)); - MLX5_SET(paos_reg, in, local_port, 1); + MLX5_SET(paos_reg, in, local_port, port); MLX5_SET(paos_reg, in, admin_status, status); MLX5_SET(paos_reg, in, ase, 1); diff --git a/include/linux/mlx5/port.h b/include/linux/mlx5/port.h index 7391eb8..0656118 100644 --- a/include/linux/mlx5/port.h +++ b/include/linux/mlx5/port.h @@ -68,7 +68,8 @@ int mlx5_query_port_proto_oper(struct mlx5_core_dev *dev, int mlx5_set_port_proto(struct mlx5_core_dev *dev, u32 proto_admin, int proto_mask); int mlx5_set_port_admin_status(struct mlx5_core_dev *dev, - enum mlx5_port_status status); + enum mlx5_port_status status, + u8 port); int mlx5_query_port_admin_status(struct mlx5_core_dev *dev, enum mlx5_port_status *status); int mlx5_set_port_beacon(struct mlx5_core_dev *dev, u16 beacon_duration);