| Message ID | 20210816125436.588162993@linuxfoundation.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
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) <pavel@denx.de> 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) <pavel@denx.de> > > 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) <pavel@denx.de> > > > > 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
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; }