* [PATCH rdma-next] RDMA/mlx5: Init dest_type when create flow
@ 2020-07-07 11:02 Leon Romanovsky
2020-07-10 19:46 ` Jason Gunthorpe
2020-07-16 17:12 ` Jason Gunthorpe
0 siblings, 2 replies; 6+ messages in thread
From: Leon Romanovsky @ 2020-07-07 11:02 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe; +Cc: Daria Velikovsky, linux-rdma, Maor Gottlieb
From: Daria Velikovsky <daria@mellanox.com>
When using action drop dest_type was never assigned to any value.
Add initialization of dest_type to -1 since 0 is valid.
Fixes: f29de9eee782 ("RDMA/mlx5: Add support for drop action in DV steering")
Signed-off-by: Daria Velikovsky <daria@mellanox.com>
Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
Based on
https://lore.kernel.org/lkml/20200702081809.423482-1-leon@kernel.org
---
drivers/infiniband/hw/mlx5/fs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/infiniband/hw/mlx5/fs.c b/drivers/infiniband/hw/mlx5/fs.c
index 0d8abb7c3cdf..1a7e6226f11a 100644
--- a/drivers/infiniband/hw/mlx5/fs.c
+++ b/drivers/infiniband/hw/mlx5/fs.c
@@ -1903,7 +1903,7 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)(
struct mlx5_flow_context flow_context = {.flow_tag =
MLX5_FS_DEFAULT_FLOW_TAG};
u32 *offset_attr, offset = 0, counter_id = 0;
- int dest_id, dest_type, inlen, len, ret, i;
+ int dest_id, dest_type = -1, inlen, len, ret, i;
struct mlx5_ib_flow_handler *flow_handler;
struct mlx5_ib_flow_matcher *fs_matcher;
struct ib_uobject **arr_flow_actions;
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH rdma-next] RDMA/mlx5: Init dest_type when create flow
2020-07-07 11:02 [PATCH rdma-next] RDMA/mlx5: Init dest_type when create flow Leon Romanovsky
@ 2020-07-10 19:46 ` Jason Gunthorpe
2020-07-12 17:40 ` Leon Romanovsky
2020-07-16 17:12 ` Jason Gunthorpe
1 sibling, 1 reply; 6+ messages in thread
From: Jason Gunthorpe @ 2020-07-10 19:46 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Doug Ledford, Daria Velikovsky, linux-rdma, Maor Gottlieb
On Tue, Jul 07, 2020 at 02:02:59PM +0300, Leon Romanovsky wrote:
> From: Daria Velikovsky <daria@mellanox.com>
>
> When using action drop dest_type was never assigned to any value.
> Add initialization of dest_type to -1 since 0 is valid.
>
> Fixes: f29de9eee782 ("RDMA/mlx5: Add support for drop action in DV steering")
> Signed-off-by: Daria Velikovsky <daria@mellanox.com>
> Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> Based on
> https://lore.kernel.org/lkml/20200702081809.423482-1-leon@kernel.org
> drivers/infiniband/hw/mlx5/fs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/hw/mlx5/fs.c b/drivers/infiniband/hw/mlx5/fs.c
> index 0d8abb7c3cdf..1a7e6226f11a 100644
> +++ b/drivers/infiniband/hw/mlx5/fs.c
> @@ -1903,7 +1903,7 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)(
> struct mlx5_flow_context flow_context = {.flow_tag =
> MLX5_FS_DEFAULT_FLOW_TAG};
> u32 *offset_attr, offset = 0, counter_id = 0;
> - int dest_id, dest_type, inlen, len, ret, i;
> + int dest_id, dest_type = -1, inlen, len, ret, i;
I think this should be done inside get_dests() - it is pretty ugly to
have an function with an output pointer that is only filled sometimes
on success.
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH rdma-next] RDMA/mlx5: Init dest_type when create flow
2020-07-10 19:46 ` Jason Gunthorpe
@ 2020-07-12 17:40 ` Leon Romanovsky
2020-07-13 11:53 ` Jason Gunthorpe
0 siblings, 1 reply; 6+ messages in thread
From: Leon Romanovsky @ 2020-07-12 17:40 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: Doug Ledford, Daria Velikovsky, linux-rdma, Maor Gottlieb
On Fri, Jul 10, 2020 at 04:46:44PM -0300, Jason Gunthorpe wrote:
> On Tue, Jul 07, 2020 at 02:02:59PM +0300, Leon Romanovsky wrote:
> > From: Daria Velikovsky <daria@mellanox.com>
> >
> > When using action drop dest_type was never assigned to any value.
> > Add initialization of dest_type to -1 since 0 is valid.
> >
> > Fixes: f29de9eee782 ("RDMA/mlx5: Add support for drop action in DV steering")
> > Signed-off-by: Daria Velikovsky <daria@mellanox.com>
> > Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > Based on
> > https://lore.kernel.org/lkml/20200702081809.423482-1-leon@kernel.org
> > drivers/infiniband/hw/mlx5/fs.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/infiniband/hw/mlx5/fs.c b/drivers/infiniband/hw/mlx5/fs.c
> > index 0d8abb7c3cdf..1a7e6226f11a 100644
> > +++ b/drivers/infiniband/hw/mlx5/fs.c
> > @@ -1903,7 +1903,7 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)(
> > struct mlx5_flow_context flow_context = {.flow_tag =
> > MLX5_FS_DEFAULT_FLOW_TAG};
> > u32 *offset_attr, offset = 0, counter_id = 0;
> > - int dest_id, dest_type, inlen, len, ret, i;
> > + int dest_id, dest_type = -1, inlen, len, ret, i;
>
> I think this should be done inside get_dests() - it is pretty ugly to
> have an function with an output pointer that is only filled sometimes
> on success.
This was original patch which I rewrote because don't like the approach
when function changes fields when it doesn't need to change. I prefer
the current approach where caller has explicitly decided which default
value he wants.
Thanks
>
> Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH rdma-next] RDMA/mlx5: Init dest_type when create flow
2020-07-12 17:40 ` Leon Romanovsky
@ 2020-07-13 11:53 ` Jason Gunthorpe
2020-07-14 6:10 ` Leon Romanovsky
0 siblings, 1 reply; 6+ messages in thread
From: Jason Gunthorpe @ 2020-07-13 11:53 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Doug Ledford, Daria Velikovsky, linux-rdma, Maor Gottlieb
On Sun, Jul 12, 2020 at 08:40:16PM +0300, Leon Romanovsky wrote:
> On Fri, Jul 10, 2020 at 04:46:44PM -0300, Jason Gunthorpe wrote:
> > On Tue, Jul 07, 2020 at 02:02:59PM +0300, Leon Romanovsky wrote:
> > > From: Daria Velikovsky <daria@mellanox.com>
> > >
> > > When using action drop dest_type was never assigned to any value.
> > > Add initialization of dest_type to -1 since 0 is valid.
> > >
> > > Fixes: f29de9eee782 ("RDMA/mlx5: Add support for drop action in DV steering")
> > > Signed-off-by: Daria Velikovsky <daria@mellanox.com>
> > > Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
> > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > > Based on
> > > https://lore.kernel.org/lkml/20200702081809.423482-1-leon@kernel.org
> > > drivers/infiniband/hw/mlx5/fs.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/infiniband/hw/mlx5/fs.c b/drivers/infiniband/hw/mlx5/fs.c
> > > index 0d8abb7c3cdf..1a7e6226f11a 100644
> > > +++ b/drivers/infiniband/hw/mlx5/fs.c
> > > @@ -1903,7 +1903,7 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)(
> > > struct mlx5_flow_context flow_context = {.flow_tag =
> > > MLX5_FS_DEFAULT_FLOW_TAG};
> > > u32 *offset_attr, offset = 0, counter_id = 0;
> > > - int dest_id, dest_type, inlen, len, ret, i;
> > > + int dest_id, dest_type = -1, inlen, len, ret, i;
> >
> > I think this should be done inside get_dests() - it is pretty ugly to
> > have an function with an output pointer that is only filled sometimes
> > on success.
>
> This was original patch which I rewrote because don't like the approach
> when function changes fields when it doesn't need to change. I prefer
> the current approach where caller has explicitly decided which default
> value he wants.
How is it a "default value" ? The function's job is to fill dest_type,
it should fill it correctly or fail
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH rdma-next] RDMA/mlx5: Init dest_type when create flow
2020-07-13 11:53 ` Jason Gunthorpe
@ 2020-07-14 6:10 ` Leon Romanovsky
0 siblings, 0 replies; 6+ messages in thread
From: Leon Romanovsky @ 2020-07-14 6:10 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: Doug Ledford, Daria Velikovsky, linux-rdma, Maor Gottlieb
On Mon, Jul 13, 2020 at 08:53:13AM -0300, Jason Gunthorpe wrote:
> On Sun, Jul 12, 2020 at 08:40:16PM +0300, Leon Romanovsky wrote:
> > On Fri, Jul 10, 2020 at 04:46:44PM -0300, Jason Gunthorpe wrote:
> > > On Tue, Jul 07, 2020 at 02:02:59PM +0300, Leon Romanovsky wrote:
> > > > From: Daria Velikovsky <daria@mellanox.com>
> > > >
> > > > When using action drop dest_type was never assigned to any value.
> > > > Add initialization of dest_type to -1 since 0 is valid.
> > > >
> > > > Fixes: f29de9eee782 ("RDMA/mlx5: Add support for drop action in DV steering")
> > > > Signed-off-by: Daria Velikovsky <daria@mellanox.com>
> > > > Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
> > > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > > > Based on
> > > > https://lore.kernel.org/lkml/20200702081809.423482-1-leon@kernel.org
> > > > drivers/infiniband/hw/mlx5/fs.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/infiniband/hw/mlx5/fs.c b/drivers/infiniband/hw/mlx5/fs.c
> > > > index 0d8abb7c3cdf..1a7e6226f11a 100644
> > > > +++ b/drivers/infiniband/hw/mlx5/fs.c
> > > > @@ -1903,7 +1903,7 @@ static int UVERBS_HANDLER(MLX5_IB_METHOD_CREATE_FLOW)(
> > > > struct mlx5_flow_context flow_context = {.flow_tag =
> > > > MLX5_FS_DEFAULT_FLOW_TAG};
> > > > u32 *offset_attr, offset = 0, counter_id = 0;
> > > > - int dest_id, dest_type, inlen, len, ret, i;
> > > > + int dest_id, dest_type = -1, inlen, len, ret, i;
> > >
> > > I think this should be done inside get_dests() - it is pretty ugly to
> > > have an function with an output pointer that is only filled sometimes
> > > on success.
> >
> > This was original patch which I rewrote because don't like the approach
> > when function changes fields when it doesn't need to change. I prefer
> > the current approach where caller has explicitly decided which default
> > value he wants.
>
> How is it a "default value" ? The function's job is to fill dest_type,
> it should fill it correctly or fail
It is one of the possible implementations to fix the bug, I presented
different way that is preferrable by me. Do you think that it is so
important that we MUST do your variant?
Thanks
>
> Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH rdma-next] RDMA/mlx5: Init dest_type when create flow
2020-07-07 11:02 [PATCH rdma-next] RDMA/mlx5: Init dest_type when create flow Leon Romanovsky
2020-07-10 19:46 ` Jason Gunthorpe
@ 2020-07-16 17:12 ` Jason Gunthorpe
1 sibling, 0 replies; 6+ messages in thread
From: Jason Gunthorpe @ 2020-07-16 17:12 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Doug Ledford, Daria Velikovsky, linux-rdma, Maor Gottlieb
On Tue, Jul 07, 2020 at 02:02:59PM +0300, Leon Romanovsky wrote:
> From: Daria Velikovsky <daria@mellanox.com>
>
> When using action drop dest_type was never assigned to any value.
> Add initialization of dest_type to -1 since 0 is valid.
>
> Fixes: f29de9eee782 ("RDMA/mlx5: Add support for drop action in DV steering")
> Signed-off-by: Daria Velikovsky <daria@mellanox.com>
> Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
> Based on
> https://lore.kernel.org/lkml/20200702081809.423482-1-leon@kernel.org
> ---
> drivers/infiniband/hw/mlx5/fs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied to for-next, thanks
Jason
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-16 17:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-07 11:02 [PATCH rdma-next] RDMA/mlx5: Init dest_type when create flow Leon Romanovsky
2020-07-10 19:46 ` Jason Gunthorpe
2020-07-12 17:40 ` Leon Romanovsky
2020-07-13 11:53 ` Jason Gunthorpe
2020-07-14 6:10 ` Leon Romanovsky
2020-07-16 17:12 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).