On Sat, Apr 23, 2016 at 7:00 PM, Ido Schimmel <ido...@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.
