[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7d966897-56b5-4a53-3b75-dd90072e17@linux.intel.com>
Date: Tue, 3 Oct 2023 15:05:45 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
cc: Vadim Pasternak <vadimp@...dia.com>,
Hans de Goede <hdegoede@...hat.com>,
Mark Gross <markgross@...nel.org>,
Michael Shych <michaelsh@...dia.com>,
LKML <linux-kernel@...r.kernel.org>,
kernel-janitors@...r.kernel.org,
platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH] platform: mellanox: Fix a resource leak in an error
handling path in mlxplat_probe()
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@...adoo.fr>
> ---
> 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:
>
Powered by blists - more mailing lists