Message ID | 20190211115608.22677-3-leon@kernel.org |
---|---|
State | Awaiting Upstream |
Delegated to: | David Miller |
Headers | show |
Series | Followup changes to mlx5-next branch | expand |
On Mon, Feb 11, 2019 at 01:56:08PM +0200, Leon Romanovsky wrote: > From: Leon Romanovsky <leonro@mellanox.com> > > Combine all HCA capabilities setters under one function > and compile out the ODP related function in case kernel > was compiled without ODP support. > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > .../net/ethernet/mellanox/mlx5/core/main.c | 47 +++++++++++++------ > 1 file changed, 33 insertions(+), 14 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c > index 6d45518edbdc..d7145ab6105d 100644 > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c > @@ -459,6 +459,7 @@ static int handle_hca_cap_atomic(struct mlx5_core_dev *dev) > return err; > } > > +#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING > static int handle_hca_cap_odp(struct mlx5_core_dev *dev) > { > void *set_hca_cap; > @@ -502,6 +503,7 @@ static int handle_hca_cap_odp(struct mlx5_core_dev *dev) > kfree(set_ctx); > return err; > } > +#endif > > static int handle_hca_cap(struct mlx5_core_dev *dev) > { > @@ -576,6 +578,35 @@ static int handle_hca_cap(struct mlx5_core_dev *dev) > return err; > } > > +static int set_hca_cap(struct mlx5_core_dev *dev) > +{ > + struct pci_dev *pdev = dev->pdev; > + int err; > + > + err = handle_hca_cap(dev); > + if (err) { > + dev_err(&pdev->dev, "handle_hca_cap failed\n"); > + goto out; > + } > + > + err = handle_hca_cap_atomic(dev); > + if (err) { > + dev_err(&pdev->dev, "handle_hca_cap_atomic failed\n"); > + goto out; > + } > + > +#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING > + err = handle_hca_cap_odp(dev); > + if (err) { > + dev_err(&pdev->dev, "handle_hca_cap_odp failed\n"); > + goto out; > + } > +#endif Adding if (IS_ENABLED..) return 0; To the top of handle_hca_cap_odp is alot better. Jason
On Mon, Feb 11, 2019 at 07:50:55PM +0000, Jason Gunthorpe wrote: > On Mon, Feb 11, 2019 at 01:56:08PM +0200, Leon Romanovsky wrote: > > From: Leon Romanovsky <leonro@mellanox.com> > > > > Combine all HCA capabilities setters under one function > > and compile out the ODP related function in case kernel > > was compiled without ODP support. > > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > > .../net/ethernet/mellanox/mlx5/core/main.c | 47 +++++++++++++------ > > 1 file changed, 33 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c > > index 6d45518edbdc..d7145ab6105d 100644 > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c > > @@ -459,6 +459,7 @@ static int handle_hca_cap_atomic(struct mlx5_core_dev *dev) > > return err; > > } > > > > +#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING > > static int handle_hca_cap_odp(struct mlx5_core_dev *dev) > > { > > void *set_hca_cap; > > @@ -502,6 +503,7 @@ static int handle_hca_cap_odp(struct mlx5_core_dev *dev) > > kfree(set_ctx); > > return err; > > } > > +#endif > > > > static int handle_hca_cap(struct mlx5_core_dev *dev) > > { > > @@ -576,6 +578,35 @@ static int handle_hca_cap(struct mlx5_core_dev *dev) > > return err; > > } > > > > +static int set_hca_cap(struct mlx5_core_dev *dev) > > +{ > > + struct pci_dev *pdev = dev->pdev; > > + int err; > > + > > + err = handle_hca_cap(dev); > > + if (err) { > > + dev_err(&pdev->dev, "handle_hca_cap failed\n"); > > + goto out; > > + } > > + > > + err = handle_hca_cap_atomic(dev); > > + if (err) { > > + dev_err(&pdev->dev, "handle_hca_cap_atomic failed\n"); > > + goto out; > > + } > > + > > +#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING > > + err = handle_hca_cap_odp(dev); > > + if (err) { > > + dev_err(&pdev->dev, "handle_hca_cap_odp failed\n"); > > + goto out; > > + } > > +#endif > > Adding > if (IS_ENABLED..) > return 0; > > To the top of handle_hca_cap_odp is alot better. Saeed gave comment that he prefers code to be compiled-out in case config is not set. In you suggestion, the code will exist and only with some optimizations enabled it will be thrown. Thanks > > Jason
On Mon, Feb 11, 2019 at 10:02:07PM +0200, Leon Romanovsky wrote: > On Mon, Feb 11, 2019 at 07:50:55PM +0000, Jason Gunthorpe wrote: > > On Mon, Feb 11, 2019 at 01:56:08PM +0200, Leon Romanovsky wrote: > > > From: Leon Romanovsky <leonro@mellanox.com> > > > > > > Combine all HCA capabilities setters under one function > > > and compile out the ODP related function in case kernel > > > was compiled without ODP support. > > > > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > > > .../net/ethernet/mellanox/mlx5/core/main.c | 47 +++++++++++++------ > > > 1 file changed, 33 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > index 6d45518edbdc..d7145ab6105d 100644 > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > @@ -459,6 +459,7 @@ static int handle_hca_cap_atomic(struct mlx5_core_dev *dev) > > > return err; > > > } > > > > > > +#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING > > > static int handle_hca_cap_odp(struct mlx5_core_dev *dev) > > > { > > > void *set_hca_cap; > > > @@ -502,6 +503,7 @@ static int handle_hca_cap_odp(struct mlx5_core_dev *dev) > > > kfree(set_ctx); > > > return err; > > > } > > > +#endif > > > > > > static int handle_hca_cap(struct mlx5_core_dev *dev) > > > { > > > @@ -576,6 +578,35 @@ static int handle_hca_cap(struct mlx5_core_dev *dev) > > > return err; > > > } > > > > > > +static int set_hca_cap(struct mlx5_core_dev *dev) > > > +{ > > > + struct pci_dev *pdev = dev->pdev; > > > + int err; > > > + > > > + err = handle_hca_cap(dev); > > > + if (err) { > > > + dev_err(&pdev->dev, "handle_hca_cap failed\n"); > > > + goto out; > > > + } > > > + > > > + err = handle_hca_cap_atomic(dev); > > > + if (err) { > > > + dev_err(&pdev->dev, "handle_hca_cap_atomic failed\n"); > > > + goto out; > > > + } > > > + > > > +#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING > > > + err = handle_hca_cap_odp(dev); > > > + if (err) { > > > + dev_err(&pdev->dev, "handle_hca_cap_odp failed\n"); > > > + goto out; > > > + } > > > +#endif > > > > Adding > > if (IS_ENABLED..) > > return 0; > > > > To the top of handle_hca_cap_odp is alot better. > > Saeed gave comment that he prefers code to be compiled-out in case > config is not set. In you suggestion, the code will exist and only > with some optimizations enabled it will be thrown. the kernel always compiles with optimizations that will throw away this code, it is a widely used pattern. #ifdef creates a compilation test matrix that is very undesired. Jason
On Mon, 2019-02-11 at 20:32 +0000, Jason Gunthorpe wrote: > On Mon, Feb 11, 2019 at 10:02:07PM +0200, Leon Romanovsky wrote: > > On Mon, Feb 11, 2019 at 07:50:55PM +0000, Jason Gunthorpe wrote: > > > On Mon, Feb 11, 2019 at 01:56:08PM +0200, Leon Romanovsky wrote: > > > > From: Leon Romanovsky <leonro@mellanox.com> > > > > > > > > Combine all HCA capabilities setters under one function > > > > and compile out the ODP related function in case kernel > > > > was compiled without ODP support. > > > > > > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > > > > .../net/ethernet/mellanox/mlx5/core/main.c | 47 > > > > +++++++++++++------ > > > > 1 file changed, 33 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > > b/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > > index 6d45518edbdc..d7145ab6105d 100644 > > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c > > > > @@ -459,6 +459,7 @@ static int handle_hca_cap_atomic(struct > > > > mlx5_core_dev *dev) > > > > return err; > > > > } > > > > > > > > +#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING > > > > static int handle_hca_cap_odp(struct mlx5_core_dev *dev) > > > > { > > > > void *set_hca_cap; > > > > @@ -502,6 +503,7 @@ static int handle_hca_cap_odp(struct > > > > mlx5_core_dev *dev) > > > > kfree(set_ctx); > > > > return err; > > > > } > > > > +#endif > > > > > > > > static int handle_hca_cap(struct mlx5_core_dev *dev) > > > > { > > > > @@ -576,6 +578,35 @@ static int handle_hca_cap(struct > > > > mlx5_core_dev *dev) > > > > return err; > > > > } > > > > > > > > +static int set_hca_cap(struct mlx5_core_dev *dev) > > > > +{ > > > > + struct pci_dev *pdev = dev->pdev; > > > > + int err; > > > > + > > > > + err = handle_hca_cap(dev); > > > > + if (err) { > > > > + dev_err(&pdev->dev, "handle_hca_cap failed\n"); > > > > + goto out; > > > > + } > > > > + > > > > + err = handle_hca_cap_atomic(dev); > > > > + if (err) { > > > > + dev_err(&pdev->dev, "handle_hca_cap_atomic > > > > failed\n"); > > > > + goto out; > > > > + } > > > > + > > > > +#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING > > > > + err = handle_hca_cap_odp(dev); > > > > + if (err) { > > > > + dev_err(&pdev->dev, "handle_hca_cap_odp > > > > failed\n"); > > > > + goto out; > > > > + } > > > > +#endif > > > > > > Adding > > > if (IS_ENABLED..) > > > return 0; > > > > > > To the top of handle_hca_cap_odp is alot better. > > > > Saeed gave comment that he prefers code to be compiled-out in case > > config is not set. In you suggestion, the code will exist and only > > with some optimizations enabled it will be thrown. > > the kernel always compiles with optimizations that will throw away > this code, it is a widely used pattern. > > #ifdef creates a compilation test matrix that is very undesired. > It is more about code separation and readability rather than compiler optimizations, optimally i want to have a file(s) that will include all IB stuff and compile out directly from Makefile, of course we still need stub functions in a header file which will yield the same compilation test matrix, but code readability and separation will become more clear. -Saeed. > Jason
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c index 6d45518edbdc..d7145ab6105d 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c @@ -459,6 +459,7 @@ static int handle_hca_cap_atomic(struct mlx5_core_dev *dev) return err; } +#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING static int handle_hca_cap_odp(struct mlx5_core_dev *dev) { void *set_hca_cap; @@ -502,6 +503,7 @@ static int handle_hca_cap_odp(struct mlx5_core_dev *dev) kfree(set_ctx); return err; } +#endif static int handle_hca_cap(struct mlx5_core_dev *dev) { @@ -576,6 +578,35 @@ static int handle_hca_cap(struct mlx5_core_dev *dev) return err; } +static int set_hca_cap(struct mlx5_core_dev *dev) +{ + struct pci_dev *pdev = dev->pdev; + int err; + + err = handle_hca_cap(dev); + if (err) { + dev_err(&pdev->dev, "handle_hca_cap failed\n"); + goto out; + } + + err = handle_hca_cap_atomic(dev); + if (err) { + dev_err(&pdev->dev, "handle_hca_cap_atomic failed\n"); + goto out; + } + +#ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING + err = handle_hca_cap_odp(dev); + if (err) { + dev_err(&pdev->dev, "handle_hca_cap_odp failed\n"); + goto out; + } +#endif + +out: + return err; +} + static int set_hca_ctrl(struct mlx5_core_dev *dev) { struct mlx5_reg_host_endianness he_in; @@ -963,21 +994,9 @@ static int mlx5_load_one(struct mlx5_core_dev *dev, struct mlx5_priv *priv, goto reclaim_boot_pages; } - err = handle_hca_cap(dev); + err = set_hca_cap(dev); if (err) { - dev_err(&pdev->dev, "handle_hca_cap failed\n"); - goto reclaim_boot_pages; - } - - err = handle_hca_cap_atomic(dev); - if (err) { - dev_err(&pdev->dev, "handle_hca_cap_atomic failed\n"); - goto reclaim_boot_pages; - } - - err = handle_hca_cap_odp(dev); - if (err) { - dev_err(&pdev->dev, "handle_hca_cap_odp failed\n"); + dev_err(&pdev->dev, "set_hca_cap failed\n"); goto reclaim_boot_pages; }