Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1423137 > unrolled thread

[PATCH 1/2] mlx5: only register devlink when ethernet is available

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] mlx5: only register devlink when ethernet is available Arnd Bergmann <arnd@arndb.de> - 2016-06-15 17:30 +0200
    Re: [PATCH 1/2] mlx5: only register devlink when ethernet is available Saeed Mahameed <saeedm@dev.mellanox.co.il> - 2016-06-15 18:10 +0200
      Re: [PATCH 1/2] mlx5: only register devlink when ethernet is available Arnd Bergmann <arnd@arndb.de> - 2016-06-15 22:50 +0200
        Re: [PATCH 1/2] mlx5: only register devlink when ethernet is available Saeed Mahameed <saeedm@dev.mellanox.co.il> - 2016-06-17 17:00 +0200
          Re: [PATCH 1/2] mlx5: only register devlink when ethernet is available Arnd Bergmann <arnd@arndb.de> - 2016-06-17 17:10 +0200
            Re: [PATCH 1/2] mlx5: only register devlink when ethernet is  available Leon Romanovsky <leon@kernel.org> - 2016-06-17 18:20 +0200

#1423137 — [PATCH 1/2] mlx5: only register devlink when ethernet is available

We get a build error with the mlx5 driver when the ethernet
support (CONFIG_MLX5_CORE_EN) is disabled:

drivers/net/ethernet/mellanox/mlx5/core/main.c:1320:22: error: 'mlx5_devlink_eswitch_mode_set' undeclared here (not in a function)
drivers/net/ethernet/mellanox/mlx5/core/main.c:1321:22: error: 'mlx5_devlink_eswitch_mode_get' undeclared here (not in a function)
drivers/net/built-in.o:(.rodata+0x25a68): undefined reference to `mlx5_devlink_eswitch_mode_get'
drivers/net/built-in.o:(.rodata+0x25a6c): undefined reference to `mlx5_devlink_eswitch_mode_set'

There are actually two problems here, but they are closely related,
so I'm addressing them both:

- The header is included under an #ifdef, which is usually a bad idea
  as it hides the function declarations, so we fail to compile even
  if we don't actually use the functions in the end.
- The references to the functions are kept in the object file because
  we don't check whether they are built-in or not.

As we don't want to add any useless #ifdef here, this uses an
IS_ENABLED() check to drop the mlx5_devlink_ops structure when we don't
need it, and to skip the register/unregister step.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: f7856daf57b9 ("net/mlx5: Add devlink interface")
---
 drivers/net/ethernet/mellanox/mlx5/core/main.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index dc568096b87c..d238e312b123 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -54,9 +54,7 @@
 #include <net/devlink.h>
 #include "mlx5_core.h"
 #include "fs_core.h"
-#ifdef CONFIG_MLX5_CORE_EN
 #include "eswitch.h"
-#endif
 
 MODULE_AUTHOR("Eli Cohen <eli@mellanox.com>");
 MODULE_DESCRIPTION("Mellanox Connect-IB, ConnectX-4 core driver");
@@ -1329,7 +1327,8 @@ static int init_one(struct pci_dev *pdev,
 	struct mlx5_priv *priv;
 	int err;
 
-	devlink = devlink_alloc(&mlx5_devlink_ops, sizeof(*dev));
+	devlink = devlink_alloc(IS_ENABLED(CONFIG_MLX5_CORE_EN) ?
+				&mlx5_devlink_ops : NULL, sizeof(*dev));
 	if (!devlink) {
 		dev_err(&pdev->dev, "kzalloc failed\n");
 		return -ENOMEM;
@@ -1372,7 +1371,8 @@ static int init_one(struct pci_dev *pdev,
 		goto clean_health;
 	}
 
-	err = devlink_register(devlink, &pdev->dev);
+	if (IS_ENABLED(CONFIG_MLX5_CORE_EN))
+		err = devlink_register(devlink, &pdev->dev);
 	if (err)
 		goto clean_load;
 
@@ -1397,7 +1397,8 @@ static void remove_one(struct pci_dev *pdev)
 	struct devlink *devlink = priv_to_devlink(dev);
 	struct mlx5_priv *priv = &dev->priv;
 
-	devlink_unregister(devlink);
+	if (IS_ENABLED(CONFIG_MLX5_CORE_EN))
+		devlink_unregister(devlink);
 	if (mlx5_unload_one(dev, priv)) {
 		dev_err(&dev->pdev->dev, "mlx5_unload_one failed\n");
 		mlx5_health_cleanup(dev);
-- 
2.9.0

[toc] | [next] | [standalone]

#1423194

On Wed, Jun 15, 2016 at 6:27 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> We get a build error with the mlx5 driver when the ethernet
> support (CONFIG_MLX5_CORE_EN) is disabled:
>
> drivers/net/ethernet/mellanox/mlx5/core/main.c:1320:22: error: 'mlx5_devlink_eswitch_mode_set' undeclared here (not in a function)
> drivers/net/ethernet/mellanox/mlx5/core/main.c:1321:22: error: 'mlx5_devlink_eswitch_mode_get' undeclared here (not in a function)
> drivers/net/built-in.o:(.rodata+0x25a68): undefined reference to `mlx5_devlink_eswitch_mode_get'
> drivers/net/built-in.o:(.rodata+0x25a6c): undefined reference to `mlx5_devlink_eswitch_mode_set'
>
> There are actually two problems here, but they are closely related,
> so I'm addressing them both:
>
> - The header is included under an #ifdef, which is usually a bad idea
>   as it hides the function declarations, so we fail to compile even
>   if we don't actually use the functions in the end.
> - The references to the functions are kept in the object file because
>   we don't check whether they are built-in or not.
>
> As we don't want to add any useless #ifdef here, this uses an
> IS_ENABLED() check to drop the mlx5_devlink_ops structure when we don't
> need it, and to skip the register/unregister step.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: f7856daf57b9 ("net/mlx5: Add devlink interface")

Hi Arnd,

We already took care of those issues, they only apply to Leon's tree
https://git.kernel.org/cgit/linux/kernel/git/leon/linux-rdma.git/,
this tree is meant to maintain MLX5 Shared code between netdev and
linux-rdma trees prior to submission to both trees.

This patch is a non-shared code and it only exists in
https://git.kernel.org/cgit/linux/kernel/git/leon/linux-rdma.git/log/?h=topic/net-next-mlx5.
It is yet to be submitted to Dave's net/net-next tree. later on, this
patch and all the others will go through the normal submission
process.

For the future I don't see any reason to CC the whole netdev, rdma and
kernel folks.
Unless you, Dave and Doug think otherwise.

Thanks
Saeed.

[toc] | [prev] | [next] | [standalone]

#1423434

On Wednesday, June 15, 2016 7:04:54 PM CEST Saeed Mahameed wrote:
> 
> Hi Arnd,
> 
> We already took care of those issues, they only apply to Leon's tree
> https://git.kernel.org/cgit/linux/kernel/git/leon/linux-rdma.git/,
> this tree is meant to maintain MLX5 Shared code between netdev and
> linux-rdma trees prior to submission to both trees.
> 
> This patch is a non-shared code and it only exists in
> https://git.kernel.org/cgit/linux/kernel/git/leon/linux-rdma.git/log/?h=topic/net-next-mlx5.
> It is yet to be submitted to Dave's net/net-next tree. later on, this
> patch and all the others will go through the normal submission
> process.

Ok, I see. It would be nice if the process had a way to avoid build regressions
in linux-next, in particular if you already have a fix by the time a patch
that introduces a problem gets added.

Can you check if the fix for the second problem correctly removes the
unnecessary 64-bit division (as opposed to adding a call to div_s64()
or do_div()), and if it removes all traces of 'struct timespec' again?

	Arnd

[toc] | [prev] | [next] | [standalone]

#1425181

On Wed, Jun 15, 2016 at 11:50 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday, June 15, 2016 7:04:54 PM CEST Saeed Mahameed wrote:
> Ok, I see. It would be nice if the process had a way to avoid build regressions
> in linux-next, in particular if you already have a fix by the time a patch
> that introduces a problem gets added.
>

The reason we added this tree is to get 0-day testing but currently it
makes some unwanted noise
so we will remove it until we figure it out.

>
> Can you check if the fix for the second problem correctly removes the
> unnecessary 64-bit division (as opposed to adding a call to div_s64()
> or do_div()), and if it removes all traces of 'struct timespec' again?
>

Yes, same thing, already fixed, will reply to that thread.

[toc] | [prev] | [next] | [standalone]

#1425190

On Friday, June 17, 2016 5:50:14 PM CEST Saeed Mahameed wrote:
> On Wed, Jun 15, 2016 at 11:50 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday, June 15, 2016 7:04:54 PM CEST Saeed Mahameed wrote:
> > Ok, I see. It would be nice if the process had a way to avoid build regressions
> > in linux-next, in particular if you already have a fix by the time a patch
> > that introduces a problem gets added.
> >
> 
> The reason we added this tree is to get 0-day testing but currently it
> makes some unwanted noise
> so we will remove it until we figure it out.

I think you can simply ask Fengguang Wu to add your git tree to the list
of trees he pulls from for the 0-day test bot.

> > Can you check if the fix for the second problem correctly removes the
> > unnecessary 64-bit division (as opposed to adding a call to div_s64()
> > or do_div()), and if it removes all traces of 'struct timespec' again?
> >
> 
> Yes, same thing, already fixed, will reply to that thread.

Ok, thanks for the confirmation!

	Arnd

[toc] | [prev] | [next] | [standalone]

#1425298 — Re: [PATCH 1/2] mlx5: only register devlink when ethernet is available

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jun 17, 2016 at 05:02:33PM +0200, Arnd Bergmann wrote:
> On Friday, June 17, 2016 5:50:14 PM CEST Saeed Mahameed wrote:
> > On Wed, Jun 15, 2016 at 11:50 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Wednesday, June 15, 2016 7:04:54 PM CEST Saeed Mahameed wrote:
> > > Ok, I see. It would be nice if the process had a way to avoid build regressions
> > > in linux-next, in particular if you already have a fix by the time a patch
> > > that introduces a problem gets added.
> > >
> > 
> > The reason we added this tree is to get 0-day testing but currently it
> > makes some unwanted noise
> > so we will remove it until we figure it out.
> 
> I think you can simply ask Fengguang Wu to add your git tree to the list
> of trees he pulls from for the 0-day test bot.

It is not 0-day only, but linux-next too. It works flawlessly for RDMA
topics and Doug receives cleaned and fully tested patches. Sadly enough,
it didn't work well for mlx5 net part.

Till further notice, I removed mlx5 net part (submission queue) from my
tree and from linux-next.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web