On Sun, 1 Oct 2023, Christophe JAILLET wrote:
> If an error occurs after a successful mlxplat_i2c_main_init(),
> mlxplat_i2c_main_exit() should be called to free some resources.
>
> Add the missing call, as already done in the remove function.
>
> Fixes: 158cd8320776 ("platform: mellanox: Split logic in init and exit flow")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
> ---
> This patch is based on comparison between functions called in the remove
> function and the error handling path of the probe.
>
> For some reason, the way the code is written and function names are
> puzzling to me.
Indeed, pre/post are mixed up for init/exit variants which makes
everything very confusing and the call to mlxplat_post_init() is buried
deep into the call chain.
These would seem better names for the init/deinit with proper pairing:
- ..._logicdev_init/deinit for FPGA/CPLD init.
- ..._mainbus_init/deinit
- perhaps the rest could be just ..._platdevs_reg/unreg
Those alone would make it much easier to follow.
In addition,
- mlxplat_i2c_mux_complition_notify() looks just useless call chain level
and should be removed (it also has a typo in its name).
- Maybe ..._platdev_reg() (currently mlxplat_post_init()) should be called
directly from mainbus_init() or even from .probe() and not from the
mux topo init.
> So Review with care!
It does not look complete as also mlxplat_i2c_main_init() lacks rollback
it should do it mlxplat_i2c_mux_topology_init() failed.
Since currently mlxplat_i2c_main_init() is what ultimately calls
mlxplat_post_init() deep down in the call chain (unless the call to it
gets moved out from there), it would be appropriate for
mlxplat_i2c_main_exit() to do/call the cleanup. And neither .probe() nor
.remove() should call mlxplat_pre_exit() directly in that case.
So two alternative ways forward for the fix before all the renaming:
1) Move mlxplat_post_init() call out of its current place into .probe()
and make the rollback path there to match that.
2) Do cleanup properly in mlxplat_i2c_main_init() and
mlxplat_i2c_main_exit().
I'd prefer 1) because it's much simpler to follow the init logic when the
init components are not hidden deep into the call chain.
--
i.
> ---
> drivers/platform/x86/mlx-platform.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/platform/x86/mlx-platform.c b/drivers/platform/x86/mlx-platform.c
> index 3d96dbf79a72..64701b63336e 100644
> --- a/drivers/platform/x86/mlx-platform.c
> +++ b/drivers/platform/x86/mlx-platform.c
> @@ -6598,6 +6598,7 @@ static int mlxplat_probe(struct platform_device *pdev)
> fail_register_reboot_notifier:
> fail_regcache_sync:
> mlxplat_pre_exit(priv);
> + mlxplat_i2c_main_exit(priv);
> fail_mlxplat_i2c_main_init:
> fail_regmap_write:
> fail_alloc:
>
