| Message ID | 20200904200621.2407839-1-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [net-next] mlx4: make sure to always set the port type | expand |
On Fri, Sep 04, 2020 at 01:06:21PM -0700, Jakub Kicinski wrote: > Even tho mlx4_core registers the devlink ports, it's mlx4_en > and mlx4_ib which set their type. In situations where one of > the two is not built yet the machine has ports of given type > we see the devlink warning from devlink_port_type_warn() trigger. > > Having ports of a type not supported by the kernel may seem > surprising, but it does occur in practice - when the unsupported > port is not plugged in to a switch anyway users are more than happy > not to see it (and potentially allocate any resources to it). > > Set the type in mlx4_core if type-specific driver is not built. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > drivers/net/ethernet/mellanox/mlx4/main.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c > index 258c7a96f269..70cf24ba71e4 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/main.c > +++ b/drivers/net/ethernet/mellanox/mlx4/main.c > @@ -3031,6 +3031,17 @@ static int mlx4_init_port_info(struct mlx4_dev *dev, int port) > if (err) > return err; > > + /* Ethernet and IB drivers will normally set the port type, > + * but if they are not built set the type now to prevent > + * devlink_port_type_warn() from firing. > + */ > + if (!IS_ENABLED(CONFIG_MLX4_EN) && > + dev->caps.port_type[port] == MLX4_PORT_TYPE_ETH) > + devlink_port_type_eth_set(&info->devlink_port, NULL); ^^^^^ Won't it crash in devlink_port_type_eth_set()? The first line there dereferences pointer. 7612 const struct net_device_ops *ops = netdev->netdev_ops; And can we call to devlink_port_type_*_set() without IS_ENABLED() check? Thanks > + else if (!IS_ENABLED(CONFIG_MLX4_INFINIBAND) && > + dev->caps.port_type[port] == MLX4_PORT_TYPE_IB) > + devlink_port_type_ib_set(&info->devlink_port, NULL); > + > info->dev = dev; > info->port = port; > if (!mlx4_is_slave(dev)) { > -- > 2.26.2 >
On Sun, 6 Sep 2020 10:27:59 +0300 Leon Romanovsky wrote: > On Fri, Sep 04, 2020 at 01:06:21PM -0700, Jakub Kicinski wrote: > > Even tho mlx4_core registers the devlink ports, it's mlx4_en > > and mlx4_ib which set their type. In situations where one of > > the two is not built yet the machine has ports of given type > > we see the devlink warning from devlink_port_type_warn() trigger. > > > > Having ports of a type not supported by the kernel may seem > > surprising, but it does occur in practice - when the unsupported > > port is not plugged in to a switch anyway users are more than happy > > not to see it (and potentially allocate any resources to it). > > > > Set the type in mlx4_core if type-specific driver is not built. > > > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > > --- > > drivers/net/ethernet/mellanox/mlx4/main.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c > > index 258c7a96f269..70cf24ba71e4 100644 > > --- a/drivers/net/ethernet/mellanox/mlx4/main.c > > +++ b/drivers/net/ethernet/mellanox/mlx4/main.c > > @@ -3031,6 +3031,17 @@ static int mlx4_init_port_info(struct mlx4_dev *dev, int port) > > if (err) > > return err; > > > > + /* Ethernet and IB drivers will normally set the port type, > > + * but if they are not built set the type now to prevent > > + * devlink_port_type_warn() from firing. > > + */ > > + if (!IS_ENABLED(CONFIG_MLX4_EN) && > > + dev->caps.port_type[port] == MLX4_PORT_TYPE_ETH) > > + devlink_port_type_eth_set(&info->devlink_port, NULL); > ^^^^^ > > Won't it crash in devlink_port_type_eth_set()? > The first line there dereferences pointer. > 7612 const struct net_device_ops *ops = netdev->netdev_ops; Damn, good catch. It's not supposed to be required. I'll patch devlink. > And can we call to devlink_port_type_*_set() without IS_ENABLED() check? It'll generate two netlink notifications - not the end of the world but also doesn't feel super clean.
Sun, Sep 06, 2020 at 06:33:05PM CEST, kuba@kernel.org wrote: >On Sun, 6 Sep 2020 10:27:59 +0300 Leon Romanovsky wrote: >> On Fri, Sep 04, 2020 at 01:06:21PM -0700, Jakub Kicinski wrote: >> > Even tho mlx4_core registers the devlink ports, it's mlx4_en >> > and mlx4_ib which set their type. In situations where one of >> > the two is not built yet the machine has ports of given type >> > we see the devlink warning from devlink_port_type_warn() trigger. >> > >> > Having ports of a type not supported by the kernel may seem >> > surprising, but it does occur in practice - when the unsupported >> > port is not plugged in to a switch anyway users are more than happy >> > not to see it (and potentially allocate any resources to it). >> > >> > Set the type in mlx4_core if type-specific driver is not built. >> > >> > Signed-off-by: Jakub Kicinski <kuba@kernel.org> >> > --- >> > drivers/net/ethernet/mellanox/mlx4/main.c | 11 +++++++++++ >> > 1 file changed, 11 insertions(+) >> > >> > diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c >> > index 258c7a96f269..70cf24ba71e4 100644 >> > --- a/drivers/net/ethernet/mellanox/mlx4/main.c >> > +++ b/drivers/net/ethernet/mellanox/mlx4/main.c >> > @@ -3031,6 +3031,17 @@ static int mlx4_init_port_info(struct mlx4_dev *dev, int port) >> > if (err) >> > return err; >> > >> > + /* Ethernet and IB drivers will normally set the port type, >> > + * but if they are not built set the type now to prevent >> > + * devlink_port_type_warn() from firing. >> > + */ >> > + if (!IS_ENABLED(CONFIG_MLX4_EN) && >> > + dev->caps.port_type[port] == MLX4_PORT_TYPE_ETH) >> > + devlink_port_type_eth_set(&info->devlink_port, NULL); >> ^^^^^ >> >> Won't it crash in devlink_port_type_eth_set()? >> The first line there dereferences pointer. >> 7612 const struct net_device_ops *ops = netdev->netdev_ops; > >Damn, good catch. It's not supposed to be required. I'll patch devlink. When you set the port type to ethernet, you should have the net_device instance. Why wouldn't you? > >> And can we call to devlink_port_type_*_set() without IS_ENABLED() check? > >It'll generate two netlink notifications - not the end of the world but >also doesn't feel super clean.
On Mon, Sep 07, 2020 at 08:21:35AM +0200, Jiri Pirko wrote: > Sun, Sep 06, 2020 at 06:33:05PM CEST, kuba@kernel.org wrote: > >On Sun, 6 Sep 2020 10:27:59 +0300 Leon Romanovsky wrote: > >> On Fri, Sep 04, 2020 at 01:06:21PM -0700, Jakub Kicinski wrote: > >> > Even tho mlx4_core registers the devlink ports, it's mlx4_en > >> > and mlx4_ib which set their type. In situations where one of > >> > the two is not built yet the machine has ports of given type > >> > we see the devlink warning from devlink_port_type_warn() trigger. > >> > > >> > Having ports of a type not supported by the kernel may seem > >> > surprising, but it does occur in practice - when the unsupported > >> > port is not plugged in to a switch anyway users are more than happy > >> > not to see it (and potentially allocate any resources to it). > >> > > >> > Set the type in mlx4_core if type-specific driver is not built. > >> > > >> > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > >> > --- > >> > drivers/net/ethernet/mellanox/mlx4/main.c | 11 +++++++++++ > >> > 1 file changed, 11 insertions(+) > >> > > >> > diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c > >> > index 258c7a96f269..70cf24ba71e4 100644 > >> > --- a/drivers/net/ethernet/mellanox/mlx4/main.c > >> > +++ b/drivers/net/ethernet/mellanox/mlx4/main.c > >> > @@ -3031,6 +3031,17 @@ static int mlx4_init_port_info(struct mlx4_dev *dev, int port) > >> > if (err) > >> > return err; > >> > > >> > + /* Ethernet and IB drivers will normally set the port type, > >> > + * but if they are not built set the type now to prevent > >> > + * devlink_port_type_warn() from firing. > >> > + */ > >> > + if (!IS_ENABLED(CONFIG_MLX4_EN) && > >> > + dev->caps.port_type[port] == MLX4_PORT_TYPE_ETH) > >> > + devlink_port_type_eth_set(&info->devlink_port, NULL); > >> ^^^^^ > >> > >> Won't it crash in devlink_port_type_eth_set()? > >> The first line there dereferences pointer. > >> 7612 const struct net_device_ops *ops = netdev->netdev_ops; > > > >Damn, good catch. It's not supposed to be required. I'll patch devlink. > > When you set the port type to ethernet, you should have the net_device > instance. Why wouldn't you? It is how mlx4 is implemented, see mlx4_dev_cap() function: 588 for (i = 1; i <= dev->caps.num_ports; ++i) { 589 dev->caps.port_type[i] = MLX4_PORT_TYPE_NONE; .... The port type is being set to IB or ETH without relation to net_device, fixing it will require very major code rewrite for the stable driver that in maintenance mode. > > > > > >> And can we call to devlink_port_type_*_set() without IS_ENABLED() check? > > > >It'll generate two netlink notifications - not the end of the world but > >also doesn't feel super clean. I would say that such a situation is corner case during the driver init and not an end of the world to see double netlink message. Thanks
Mon, Sep 07, 2020 at 08:48:30AM CEST, leon@kernel.org wrote: >On Mon, Sep 07, 2020 at 08:21:35AM +0200, Jiri Pirko wrote: >> Sun, Sep 06, 2020 at 06:33:05PM CEST, kuba@kernel.org wrote: >> >On Sun, 6 Sep 2020 10:27:59 +0300 Leon Romanovsky wrote: >> >> On Fri, Sep 04, 2020 at 01:06:21PM -0700, Jakub Kicinski wrote: >> >> > Even tho mlx4_core registers the devlink ports, it's mlx4_en >> >> > and mlx4_ib which set their type. In situations where one of >> >> > the two is not built yet the machine has ports of given type >> >> > we see the devlink warning from devlink_port_type_warn() trigger. >> >> > >> >> > Having ports of a type not supported by the kernel may seem >> >> > surprising, but it does occur in practice - when the unsupported >> >> > port is not plugged in to a switch anyway users are more than happy >> >> > not to see it (and potentially allocate any resources to it). >> >> > >> >> > Set the type in mlx4_core if type-specific driver is not built. >> >> > >> >> > Signed-off-by: Jakub Kicinski <kuba@kernel.org> >> >> > --- >> >> > drivers/net/ethernet/mellanox/mlx4/main.c | 11 +++++++++++ >> >> > 1 file changed, 11 insertions(+) >> >> > >> >> > diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c >> >> > index 258c7a96f269..70cf24ba71e4 100644 >> >> > --- a/drivers/net/ethernet/mellanox/mlx4/main.c >> >> > +++ b/drivers/net/ethernet/mellanox/mlx4/main.c >> >> > @@ -3031,6 +3031,17 @@ static int mlx4_init_port_info(struct mlx4_dev *dev, int port) >> >> > if (err) >> >> > return err; >> >> > >> >> > + /* Ethernet and IB drivers will normally set the port type, >> >> > + * but if they are not built set the type now to prevent >> >> > + * devlink_port_type_warn() from firing. >> >> > + */ >> >> > + if (!IS_ENABLED(CONFIG_MLX4_EN) && >> >> > + dev->caps.port_type[port] == MLX4_PORT_TYPE_ETH) >> >> > + devlink_port_type_eth_set(&info->devlink_port, NULL); >> >> ^^^^^ >> >> >> >> Won't it crash in devlink_port_type_eth_set()? >> >> The first line there dereferences pointer. >> >> 7612 const struct net_device_ops *ops = netdev->netdev_ops; >> > >> >Damn, good catch. It's not supposed to be required. I'll patch devlink. >> >> When you set the port type to ethernet, you should have the net_device >> instance. Why wouldn't you? > >It is how mlx4 is implemented, see mlx4_dev_cap() function: >588 for (i = 1; i <= dev->caps.num_ports; ++i) { >589 dev->caps.port_type[i] = MLX4_PORT_TYPE_NONE; >.... > >The port type is being set to IB or ETH without relation to net_device, >fixing it will require very major code rewrite for the stable driver >that in maintenance mode. Because the eth driver is not loaded, I see. The purpose of the WARN in devlink_port_type_eth_set is to prevent drivers from registering particular port without netdev/ibdev. That is what was repeatedly happening in the past as the driver developers didn't know they need to do it or were just lazy to do so. I wonder if there is any possibility to do both... > >> >> >> > >> >> And can we call to devlink_port_type_*_set() without IS_ENABLED() check? >> > >> >It'll generate two netlink notifications - not the end of the world but >> >also doesn't feel super clean. > >I would say that such a situation is corner case during the driver init and >not an end of the world to see double netlink message. > >Thanks
On Mon, Sep 07, 2020 at 09:19:39AM +0200, Jiri Pirko wrote: > Mon, Sep 07, 2020 at 08:48:30AM CEST, leon@kernel.org wrote: > >On Mon, Sep 07, 2020 at 08:21:35AM +0200, Jiri Pirko wrote: > >> Sun, Sep 06, 2020 at 06:33:05PM CEST, kuba@kernel.org wrote: > >> >On Sun, 6 Sep 2020 10:27:59 +0300 Leon Romanovsky wrote: > >> >> On Fri, Sep 04, 2020 at 01:06:21PM -0700, Jakub Kicinski wrote: > >> >> > Even tho mlx4_core registers the devlink ports, it's mlx4_en > >> >> > and mlx4_ib which set their type. In situations where one of > >> >> > the two is not built yet the machine has ports of given type > >> >> > we see the devlink warning from devlink_port_type_warn() trigger. > >> >> > > >> >> > Having ports of a type not supported by the kernel may seem > >> >> > surprising, but it does occur in practice - when the unsupported > >> >> > port is not plugged in to a switch anyway users are more than happy > >> >> > not to see it (and potentially allocate any resources to it). > >> >> > > >> >> > Set the type in mlx4_core if type-specific driver is not built. > >> >> > > >> >> > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > >> >> > --- > >> >> > drivers/net/ethernet/mellanox/mlx4/main.c | 11 +++++++++++ > >> >> > 1 file changed, 11 insertions(+) > >> >> > > >> >> > diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c > >> >> > index 258c7a96f269..70cf24ba71e4 100644 > >> >> > --- a/drivers/net/ethernet/mellanox/mlx4/main.c > >> >> > +++ b/drivers/net/ethernet/mellanox/mlx4/main.c > >> >> > @@ -3031,6 +3031,17 @@ static int mlx4_init_port_info(struct mlx4_dev *dev, int port) > >> >> > if (err) > >> >> > return err; > >> >> > > >> >> > + /* Ethernet and IB drivers will normally set the port type, > >> >> > + * but if they are not built set the type now to prevent > >> >> > + * devlink_port_type_warn() from firing. > >> >> > + */ > >> >> > + if (!IS_ENABLED(CONFIG_MLX4_EN) && > >> >> > + dev->caps.port_type[port] == MLX4_PORT_TYPE_ETH) > >> >> > + devlink_port_type_eth_set(&info->devlink_port, NULL); > >> >> ^^^^^ > >> >> > >> >> Won't it crash in devlink_port_type_eth_set()? > >> >> The first line there dereferences pointer. > >> >> 7612 const struct net_device_ops *ops = netdev->netdev_ops; > >> > > >> >Damn, good catch. It's not supposed to be required. I'll patch devlink. > >> > >> When you set the port type to ethernet, you should have the net_device > >> instance. Why wouldn't you? > > > >It is how mlx4 is implemented, see mlx4_dev_cap() function: > >588 for (i = 1; i <= dev->caps.num_ports; ++i) { > >589 dev->caps.port_type[i] = MLX4_PORT_TYPE_NONE; > >.... > > > >The port type is being set to IB or ETH without relation to net_device, > >fixing it will require very major code rewrite for the stable driver > >that in maintenance mode. > > Because the eth driver is not loaded, I see. The purpose of the > WARN in devlink_port_type_eth_set is to prevent drivers from registering > particular port without netdev/ibdev. That is what was repeatedly > happening in the past as the driver developers didn't know they need to > do it or were just lazy to do so. > > I wonder if there is any possibility to do both... It is hard to say, hope that Jakub will take a look. Thanks > > > > >> > >> > >> > > >> >> And can we call to devlink_port_type_*_set() without IS_ENABLED() check? > >> > > >> >It'll generate two netlink notifications - not the end of the world but > >> >also doesn't feel super clean. > > > >I would say that such a situation is corner case during the driver init and > >not an end of the world to see double netlink message. > > > >Thanks
On Mon, 7 Sep 2020 09:19:39 +0200 Jiri Pirko wrote: > >The port type is being set to IB or ETH without relation to net_device, > >fixing it will require very major code rewrite for the stable driver > >that in maintenance mode. > > Because the eth driver is not loaded, I see. The purpose of the > WARN in devlink_port_type_eth_set is to prevent drivers from registering > particular port without netdev/ibdev. That is what was repeatedly > happening in the past as the driver developers didn't know they need to > do it or were just lazy to do so. > > I wonder if there is any possibility to do both... I think we have two options in this case: - set type to eth without the netdev - selectively mute the warning I think the former is better, because we still want to see what the port type is. Perhaps we should add a: dev_warn("devlink port type set without software interface reference, device type not supported by the kernel?"); That way people won't just pass NULL out of laziness, hopefully. WDYT?
On Mon, 7 Sep 2020 09:48:30 +0300 Leon Romanovsky wrote: >>>> And can we call to devlink_port_type_*_set() without IS_ENABLED() check? >>> >>> It'll generate two netlink notifications - not the end of the world but >>> also doesn't feel super clean. > > I would say that such a situation is corner case during the driver init and > not an end of the world to see double netlink message. Could you spell out your reasoning here? Are you concerned about out-of-tree drivers? I don't see how adding IS_ENABLED() to the condition outweighs the benefit of not having duplicated netlink notifications.
Mon, Sep 07, 2020 at 06:34:01PM CEST, kuba@kernel.org wrote: >On Mon, 7 Sep 2020 09:19:39 +0200 Jiri Pirko wrote: >> >The port type is being set to IB or ETH without relation to net_device, >> >fixing it will require very major code rewrite for the stable driver >> >that in maintenance mode. >> >> Because the eth driver is not loaded, I see. The purpose of the >> WARN in devlink_port_type_eth_set is to prevent drivers from registering >> particular port without netdev/ibdev. That is what was repeatedly >> happening in the past as the driver developers didn't know they need to >> do it or were just lazy to do so. >> >> I wonder if there is any possibility to do both... > >I think we have two options in this case: > - set type to eth without the netdev > - selectively mute the warning > >I think the former is better, because we still want to see what the >port type is. Perhaps we should add a: > > dev_warn("devlink port type set without software interface > reference, device type not supported by the kernel?"); > >That way people won't just pass NULL out of laziness, hopefully. > >WDYT? Okay. That sounds probably like the best option we have.
On Mon, Sep 07, 2020 at 09:36:14AM -0700, Jakub Kicinski wrote: > On Mon, 7 Sep 2020 09:48:30 +0300 Leon Romanovsky wrote: > >>>> And can we call to devlink_port_type_*_set() without IS_ENABLED() check? > >>> > >>> It'll generate two netlink notifications - not the end of the world but > >>> also doesn't feel super clean. > > > > I would say that such a situation is corner case during the driver init and > > not an end of the world to see double netlink message. > > Could you spell out your reasoning here? Are you concerned about > out-of-tree drivers? Nothing fancy, I just didn't see users who compiled mlx4_core and used it without eth/ib. The corner case is because this double netlink can be seen only during driver reload and only if port type wasn't set. > > I don't see how adding IS_ENABLED() to the condition outweighs > the benefit of not having duplicated netlink notifications. Readability? Anyway, it doesn't matter. Thanks
diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c index 258c7a96f269..70cf24ba71e4 100644 --- a/drivers/net/ethernet/mellanox/mlx4/main.c +++ b/drivers/net/ethernet/mellanox/mlx4/main.c @@ -3031,6 +3031,17 @@ static int mlx4_init_port_info(struct mlx4_dev *dev, int port) if (err) return err; + /* Ethernet and IB drivers will normally set the port type, + * but if they are not built set the type now to prevent + * devlink_port_type_warn() from firing. + */ + if (!IS_ENABLED(CONFIG_MLX4_EN) && + dev->caps.port_type[port] == MLX4_PORT_TYPE_ETH) + devlink_port_type_eth_set(&info->devlink_port, NULL); + else if (!IS_ENABLED(CONFIG_MLX4_INFINIBAND) && + dev->caps.port_type[port] == MLX4_PORT_TYPE_IB) + devlink_port_type_ib_set(&info->devlink_port, NULL); + info->dev = dev; info->port = port; if (!mlx4_is_slave(dev)) {
Even tho mlx4_core registers the devlink ports, it's mlx4_en and mlx4_ib which set their type. In situations where one of the two is not built yet the machine has ports of given type we see the devlink warning from devlink_port_type_warn() trigger. Having ports of a type not supported by the kernel may seem surprising, but it does occur in practice - when the unsupported port is not plugged in to a switch anyway users are more than happy not to see it (and potentially allocate any resources to it). Set the type in mlx4_core if type-specific driver is not built. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- drivers/net/ethernet/mellanox/mlx4/main.c | 11 +++++++++++ 1 file changed, 11 insertions(+)