Message ID | 20200602122837.161519-1-leon@kernel.org |
---|---|
State | Awaiting Upstream |
Delegated to: | David Miller |
Headers | show |
Series | [net] net/mlx5: Don't fail driver on failure to create debugfs | expand |
On Tue, Jun 02, 2020 at 03:28:37PM +0300, Leon Romanovsky wrote: > From: Leon Romanovsky <leonro@mellanox.com> > > Clang warns: > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1278:6: warning: variable > 'err' is used uninitialized whenever 'if' condition is true > [-Wsometimes-uninitialized] > if (!priv->dbg_root) { > ^~~~~~~~~~~~~~~ > drivers/net/ethernet/mellanox/mlx5/core/main.c:1303:9: note: > uninitialized use occurs here > return err; > ^~~ > drivers/net/ethernet/mellanox/mlx5/core/main.c:1278:2: note: remove the > 'if' if its condition is always false > if (!priv->dbg_root) { > ^~~~~~~~~~~~~~~~~~~~~~ > drivers/net/ethernet/mellanox/mlx5/core/main.c:1259:9: note: initialize > the variable 'err' to silence this warning > int err; > ^ > = 0 > 1 warning generated. > > The check of returned value of debugfs_create_dir() is wrong because > by the design debugfs failures should never fail the driver and the > check itself was wrong too. The kernel compiled without CONFIG_DEBUG_FS > will return ERR_PTR(-ENODEV) and not NULL as expected. > > Fixes: 11f3b84d7068 ("net/mlx5: Split mdev init and pci init") > Link: https://github.com/ClangBuiltLinux/linux/issues/1042 > Reported-by: Nathan Chancellor <natechancellor@gmail.com> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> Thanks! That's what I figured it should be. Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > --- > Original discussion: > https://lore.kernel.org/lkml/20200530055447.1028004-1-natechancellor@gmail.com > --- > drivers/net/ethernet/mellanox/mlx5/core/main.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c > index df46b1fce3a7..110e8d277d15 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c > @@ -1275,11 +1275,6 @@ static int mlx5_mdev_init(struct mlx5_core_dev *dev, int profile_idx) > > priv->dbg_root = debugfs_create_dir(dev_name(dev->device), > mlx5_debugfs_root); > - if (!priv->dbg_root) { > - dev_err(dev->device, "mlx5_core: error, Cannot create debugfs dir, aborting\n"); > - goto err_dbg_root; > - } > - > err = mlx5_health_init(dev); > if (err) > goto err_health_init; > -- > 2.26.2 >
Saeed, please pick this up.
On Tue, Jun 02, 2020 at 12:27:24PM -0700, Nathan Chancellor wrote: > On Tue, Jun 02, 2020 at 03:28:37PM +0300, Leon Romanovsky wrote: > > From: Leon Romanovsky <leonro@mellanox.com> > > > > Clang warns: > > > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1278:6: warning: variable > > 'err' is used uninitialized whenever 'if' condition is true > > [-Wsometimes-uninitialized] > > if (!priv->dbg_root) { > > ^~~~~~~~~~~~~~~ > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1303:9: note: > > uninitialized use occurs here > > return err; > > ^~~ > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1278:2: note: remove the > > 'if' if its condition is always false > > if (!priv->dbg_root) { > > ^~~~~~~~~~~~~~~~~~~~~~ > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1259:9: note: initialize > > the variable 'err' to silence this warning > > int err; > > ^ > > = 0 > > 1 warning generated. > > > > The check of returned value of debugfs_create_dir() is wrong because > > by the design debugfs failures should never fail the driver and the > > check itself was wrong too. The kernel compiled without CONFIG_DEBUG_FS > > will return ERR_PTR(-ENODEV) and not NULL as expected. > > > > Fixes: 11f3b84d7068 ("net/mlx5: Split mdev init and pci init") > > Link: https://github.com/ClangBuiltLinux/linux/issues/1042 > > Reported-by: Nathan Chancellor <natechancellor@gmail.com> > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > > Thanks! That's what I figured it should be. > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > > > --- > > Original discussion: > > https://lore.kernel.org/lkml/20200530055447.1028004-1-natechancellor@gmail.com > > --- > > drivers/net/ethernet/mellanox/mlx5/core/main.c | 5 ----- > > 1 file changed, 5 deletions(-) > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c > > index df46b1fce3a7..110e8d277d15 100644 > > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c > > @@ -1275,11 +1275,6 @@ static int mlx5_mdev_init(struct mlx5_core_dev *dev, int profile_idx) > > > > priv->dbg_root = debugfs_create_dir(dev_name(dev->device), > > mlx5_debugfs_root); > > - if (!priv->dbg_root) { > > - dev_err(dev->device, "mlx5_core: error, Cannot create debugfs dir, aborting\n"); > > - goto err_dbg_root; Actually, this removes the only use of err_dbg_root, so that should be removed at the same time. Cheers, Nathan > > - } > > - > > err = mlx5_health_init(dev); > > if (err) > > goto err_health_init; > > -- > > 2.26.2 > >
On Wed, 2020-06-03 at 11:34 -0700, Nathan Chancellor wrote: > On Tue, Jun 02, 2020 at 12:27:24PM -0700, Nathan Chancellor wrote: > > On Tue, Jun 02, 2020 at 03:28:37PM +0300, Leon Romanovsky wrote: > > > From: Leon Romanovsky <leonro@mellanox.com> > > > > > > Clang warns: > > > > > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1278:6: warning: > > > variable > > > 'err' is used uninitialized whenever 'if' condition is true > > > [-Wsometimes-uninitialized] > > > if (!priv->dbg_root) { > > > ^~~~~~~~~~~~~~~ > > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1303:9: note: > > > uninitialized use occurs here > > > return err; > > > ^~~ > > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1278:2: note: > > > remove the > > > 'if' if its condition is always false > > > if (!priv->dbg_root) { > > > ^~~~~~~~~~~~~~~~~~~~~~ > > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1259:9: note: > > > initialize > > > the variable 'err' to silence this warning > > > int err; > > > ^ > > > = 0 > > > 1 warning generated. > > > > > > The check of returned value of debugfs_create_dir() is wrong > > > because > > > by the design debugfs failures should never fail the driver and > > > the > > > check itself was wrong too. The kernel compiled without > > > CONFIG_DEBUG_FS > > > will return ERR_PTR(-ENODEV) and not NULL as expected. > > > > > > Fixes: 11f3b84d7068 ("net/mlx5: Split mdev init and pci init") > > > Link: https://github.com/ClangBuiltLinux/linux/issues/1042 > > > Reported-by: Nathan Chancellor <natechancellor@gmail.com> > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > > > > Thanks! That's what I figured it should be. > > > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > > > > > --- > > > Original discussion: > > > https://lore.kernel.org/lkml/20200530055447.1028004-1-natechancellor@gmail.com > > > --- > > > drivers/net/ethernet/mellanox/mlx5/core/main.c | 5 ----- > > > 1 file changed, 5 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > b/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > index df46b1fce3a7..110e8d277d15 100644 > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > @@ -1275,11 +1275,6 @@ static int mlx5_mdev_init(struct > > > mlx5_core_dev *dev, int profile_idx) > > > > > > priv->dbg_root = debugfs_create_dir(dev_name(dev->device), > > > mlx5_debugfs_root); > > > - if (!priv->dbg_root) { > > > - dev_err(dev->device, "mlx5_core: error, Cannot create > > > debugfs dir, aborting\n"); > > > - goto err_dbg_root; > > Actually, this removes the only use of err_dbg_root, so that should > be > removed at the same time. > Fixed this up and applied to net-next-mlx5, Thanks!
On Thu, Jun 04, 2020 at 04:44:00AM +0000, Saeed Mahameed wrote: > On Wed, 2020-06-03 at 11:34 -0700, Nathan Chancellor wrote: > > On Tue, Jun 02, 2020 at 12:27:24PM -0700, Nathan Chancellor wrote: > > > On Tue, Jun 02, 2020 at 03:28:37PM +0300, Leon Romanovsky wrote: > > > > From: Leon Romanovsky <leonro@mellanox.com> > > > > > > > > Clang warns: > > > > > > > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1278:6: warning: > > > > variable > > > > 'err' is used uninitialized whenever 'if' condition is true > > > > [-Wsometimes-uninitialized] > > > > if (!priv->dbg_root) { > > > > ^~~~~~~~~~~~~~~ > > > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1303:9: note: > > > > uninitialized use occurs here > > > > return err; > > > > ^~~ > > > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1278:2: note: > > > > remove the > > > > 'if' if its condition is always false > > > > if (!priv->dbg_root) { > > > > ^~~~~~~~~~~~~~~~~~~~~~ > > > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1259:9: note: > > > > initialize > > > > the variable 'err' to silence this warning > > > > int err; > > > > ^ > > > > = 0 > > > > 1 warning generated. > > > > > > > > The check of returned value of debugfs_create_dir() is wrong > > > > because > > > > by the design debugfs failures should never fail the driver and > > > > the > > > > check itself was wrong too. The kernel compiled without > > > > CONFIG_DEBUG_FS > > > > will return ERR_PTR(-ENODEV) and not NULL as expected. > > > > > > > > Fixes: 11f3b84d7068 ("net/mlx5: Split mdev init and pci init") > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/1042 > > > > Reported-by: Nathan Chancellor <natechancellor@gmail.com> > > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > > > > > > Thanks! That's what I figured it should be. > > > > > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > > > > > > > --- > > > > Original discussion: > > > > https://lore.kernel.org/lkml/20200530055447.1028004-1-natechancellor@gmail.com > > > > --- > > > > drivers/net/ethernet/mellanox/mlx5/core/main.c | 5 ----- > > > > 1 file changed, 5 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > > b/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > > index df46b1fce3a7..110e8d277d15 100644 > > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > > @@ -1275,11 +1275,6 @@ static int mlx5_mdev_init(struct > > > > mlx5_core_dev *dev, int profile_idx) > > > > > > > > priv->dbg_root = debugfs_create_dir(dev_name(dev->device), > > > > mlx5_debugfs_root); > > > > - if (!priv->dbg_root) { > > > > - dev_err(dev->device, "mlx5_core: error, Cannot create > > > > debugfs dir, aborting\n"); > > > > - goto err_dbg_root; > > > > Actually, this removes the only use of err_dbg_root, so that should > > be > > removed at the same time. > > > > Fixed this up and applied to net-next-mlx5, > Thanks! > Hi Saeed, I see this warning in mainline now, is this something you were planning to have merged this cycle or next? I see it in several configs so it would be nice if it could be resolved this one, since it was introduced by a patch in this cycle even though the core issue has been around for a few months. Cheers, Nathan
On Mon, 2020-06-08 at 14:22 -0700, Nathan Chancellor wrote: > On Thu, Jun 04, 2020 at 04:44:00AM +0000, Saeed Mahameed wrote: > > On Wed, 2020-06-03 at 11:34 -0700, Nathan Chancellor wrote: > > > On Tue, Jun 02, 2020 at 12:27:24PM -0700, Nathan Chancellor > > > wrote: > > > > On Tue, Jun 02, 2020 at 03:28:37PM +0300, Leon Romanovsky > > > > wrote: > > > > > From: Leon Romanovsky <leonro@mellanox.com> > > > > > > > > > > Clang warns: > > > > > > > > > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1278:6: > > > > > warning: > > > > > variable > > > > > 'err' is used uninitialized whenever 'if' condition is true > > > > > [-Wsometimes-uninitialized] > > > > > if (!priv->dbg_root) { > > > > > ^~~~~~~~~~~~~~~ > > > > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1303:9: note: > > > > > uninitialized use occurs here > > > > > return err; > > > > > ^~~ > > > > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1278:2: note: > > > > > remove the > > > > > 'if' if its condition is always false > > > > > if (!priv->dbg_root) { > > > > > ^~~~~~~~~~~~~~~~~~~~~~ > > > > > drivers/net/ethernet/mellanox/mlx5/core/main.c:1259:9: note: > > > > > initialize > > > > > the variable 'err' to silence this warning > > > > > int err; > > > > > ^ > > > > > = 0 > > > > > 1 warning generated. > > > > > > > > > > The check of returned value of debugfs_create_dir() is wrong > > > > > because > > > > > by the design debugfs failures should never fail the driver > > > > > and > > > > > the > > > > > check itself was wrong too. The kernel compiled without > > > > > CONFIG_DEBUG_FS > > > > > will return ERR_PTR(-ENODEV) and not NULL as expected. > > > > > > > > > > Fixes: 11f3b84d7068 ("net/mlx5: Split mdev init and pci > > > > > init") > > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/1042 > > > > > Reported-by: Nathan Chancellor <natechancellor@gmail.com> > > > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > > > > > > > > Thanks! That's what I figured it should be. > > > > > > > > Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > > > > > > > > > --- > > > > > Original discussion: > > > > > https://lore.kernel.org/lkml/20200530055447.1028004-1-natechancellor@gmail.com > > > > > --- > > > > > drivers/net/ethernet/mellanox/mlx5/core/main.c | 5 ----- > > > > > 1 file changed, 5 deletions(-) > > > > > > > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > > > b/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > > > index df46b1fce3a7..110e8d277d15 100644 > > > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > > > @@ -1275,11 +1275,6 @@ static int mlx5_mdev_init(struct > > > > > mlx5_core_dev *dev, int profile_idx) > > > > > > > > > > priv->dbg_root = debugfs_create_dir(dev_name(dev- > > > > > >device), > > > > > mlx5_debugfs_root); > > > > > - if (!priv->dbg_root) { > > > > > - dev_err(dev->device, "mlx5_core: error, Cannot > > > > > create > > > > > debugfs dir, aborting\n"); > > > > > - goto err_dbg_root; > > > > > > Actually, this removes the only use of err_dbg_root, so that > > > should > > > be > > > removed at the same time. > > > > > > > Fixed this up and applied to net-next-mlx5, > > Thanks! > > > > Hi Saeed, > > I see this warning in mainline now, is this something you were > planning > to have merged this cycle or next? I see it in several configs so it > would be nice if it could be resolved this one, since it was > introduced > by a patch in this cycle even though the core issue has been around > for > a few months. > Hi Nathan, I just pulled this patch into net-mlx5, and will send it out in my net PR today. Thanks, Saeed. > Cheers, > Nathan
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c index df46b1fce3a7..110e8d277d15 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c @@ -1275,11 +1275,6 @@ static int mlx5_mdev_init(struct mlx5_core_dev *dev, int profile_idx) priv->dbg_root = debugfs_create_dir(dev_name(dev->device), mlx5_debugfs_root); - if (!priv->dbg_root) { - dev_err(dev->device, "mlx5_core: error, Cannot create debugfs dir, aborting\n"); - goto err_dbg_root; - } - err = mlx5_health_init(dev); if (err) goto err_health_init;