From: Aya Levin <[email protected]>
[ Upstream commit bd37c2888ccaa5ceb9895718f6909b247cc372e0 ]
Check return value of mlx5_fw_tracer_start(), set error path and fix
return value of mlx5_fw_tracer_init() accordingly.
Fixes: c71ad41ccb0c ("net/mlx5: FW tracer, events handling")
Signed-off-by: Aya Levin <[email protected]>
Reviewed-by: Moshe Shemesh <[email protected]>
Reviewed-by: Tariq Toukan <[email protected]>
Signed-off-by: Saeed Mahameed <[email protected]>
Signed-off-by: Sasha Levin <[email protected]>
---
.../net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c b/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
index 2eb022ad7fd0..3dfcb20e97c6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
@@ -1019,12 +1019,19 @@ int mlx5_fw_tracer_init(struct mlx5_fw_tracer *tracer)
MLX5_NB_INIT(&tracer->nb, fw_tracer_event, DEVICE_TRACER);
mlx5_eq_notifier_register(dev, &tracer->nb);
- mlx5_fw_tracer_start(tracer);
-
+ err = mlx5_fw_tracer_start(tracer);
+ if (err) {
+ mlx5_core_warn(dev, "FWTracer: Failed to start tracer %d\n", err);
+ goto err_notifier_unregister;
+ }
return 0;
+err_notifier_unregister:
+ mlx5_eq_notifier_unregister(dev, &tracer->nb);
+ mlx5_core_destroy_mkey(dev, &tracer->buff.mkey);
err_dealloc_pd:
mlx5_core_dealloc_pd(dev, tracer->buff.pdn);
+ cancel_work_sync(&tracer->read_fw_strings_work);
return err;
}
--
2.30.2
Hi!
> [ Upstream commit bd37c2888ccaa5ceb9895718f6909b247cc372e0 ]
>
> Check return value of mlx5_fw_tracer_start(), set error path and fix
> return value of mlx5_fw_tracer_init() accordingly.
This is actually two fixes in one: There's cancel_work_sync() added to
the error path, but there's additional error that needs fixing.
Could someone familiar with the code verify it after me?
Best regards,
Pavel
Signed-off-by: Pavel Machek (CIP) <[email protected]>
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c b/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
index 3dfcb20e97c6..857be86b4a11 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
@@ -1007,7 +1007,7 @@ int mlx5_fw_tracer_init(struct mlx5_fw_tracer *tracer)
err = mlx5_core_alloc_pd(dev, &tracer->buff.pdn);
if (err) {
mlx5_core_warn(dev, "FWTracer: Failed to allocate PD %d\n", err);
- return err;
+ goto err_cancel_work;
}
err = mlx5_fw_tracer_create_mkey(tracer);
@@ -1031,6 +1031,7 @@ int mlx5_fw_tracer_init(struct mlx5_fw_tracer *tracer)
mlx5_core_destroy_mkey(dev, &tracer->buff.mkey);
err_dealloc_pd:
mlx5_core_dealloc_pd(dev, tracer->buff.pdn);
+err_cancel_work:
cancel_work_sync(&tracer->read_fw_strings_work);
return err;
}
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> @@ -1019,12 +1019,19 @@ int mlx5_fw_tracer_init(struct mlx5_fw_tracer *tracer)
...
> err_dealloc_pd:
> mlx5_core_dealloc_pd(dev, tracer->buff.pdn);
> + cancel_work_sync(&tracer->read_fw_strings_work);
> return err;
> }
>
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
On Tue, 2021-08-17 at 19:51 +0200, Pavel Machek wrote:
> Hi!
>
> > [ Upstream commit bd37c2888ccaa5ceb9895718f6909b247cc372e0 ]
> >
> > Check return value of mlx5_fw_tracer_start(), set error path and
> > fix
> > return value of mlx5_fw_tracer_init() accordingly.
>
> This is actually two fixes in one: There's cancel_work_sync() added
> to
Yes, the reasoning was that the patch is fixing the whole error path of
the function in one-shot since we can blame it on a single commit.
> the error path, but there's additional error that needs fixing.
Yes.
>
> Could someone familiar with the code verify it after me?
>
> Best regards,
> Pavel
>
> Signed-off-by: Pavel Machek (CIP) <[email protected]>
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> b/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> index 3dfcb20e97c6..857be86b4a11 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> @@ -1007,7 +1007,7 @@ int mlx5_fw_tracer_init(struct mlx5_fw_tracer
> *tracer)
> err = mlx5_core_alloc_pd(dev, &tracer->buff.pdn);
> if (err) {
> mlx5_core_warn(dev, "FWTracer: Failed to allocate PD
> %d\n", err);
> - return err;
> + goto err_cancel_work;
> }
>
> err = mlx5_fw_tracer_create_mkey(tracer);
> @@ -1031,6 +1031,7 @@ int mlx5_fw_tracer_init(struct mlx5_fw_tracer
> *tracer)
> mlx5_core_destroy_mkey(dev, &tracer->buff.mkey);
> err_dealloc_pd:
> mlx5_core_dealloc_pd(dev, tracer->buff.pdn);
> +err_cancel_work:
> cancel_work_sync(&tracer->read_fw_strings_work);
> return err;
this is correct, do you want to submit this patch or do you want us to
handle ?
maybe it is better if we delayed queue_work() to after all the fragile
code behind it, to reduce the error path handling ..
thanks for pointing this out.
Hi!
> > Could someone familiar with the code verify it after me?
> > Signed-off-by: Pavel Machek (CIP) <[email protected]>
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> > index 3dfcb20e97c6..857be86b4a11 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/diag/fw_tracer.c
> > @@ -1007,7 +1007,7 @@ int mlx5_fw_tracer_init(struct mlx5_fw_tracer
> > *tracer)
> > ????????err = mlx5_core_alloc_pd(dev, &tracer->buff.pdn);
> > ????????if (err) {
> > ????????????????mlx5_core_warn(dev, "FWTracer: Failed to allocate PD
> > %d\n", err);
> > -???????????????return err;
> > +???????????????goto err_cancel_work;
> > ????????}
> > ?
> > ????????err = mlx5_fw_tracer_create_mkey(tracer);
> > @@ -1031,6 +1031,7 @@ int mlx5_fw_tracer_init(struct mlx5_fw_tracer
> > *tracer)
> > ????????mlx5_core_destroy_mkey(dev, &tracer->buff.mkey);
> > ?err_dealloc_pd:
> > ????????mlx5_core_dealloc_pd(dev, tracer->buff.pdn);
> > +err_cancel_work:
> > ????????cancel_work_sync(&tracer->read_fw_strings_work);
> > ????????return err;
>
> this is correct, do you want to submit this patch or do you want us to
> handle ?
> maybe it is better if we delayed queue_work() to after all the fragile
> code behind it, to reduce the error path handling ..
I'd prefer you to handle it.
Thank you,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany