Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Date:   Tue, 3 Oct 2023 15:05:45 +0300 (EEST)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>
cc:     Vadim Pasternak <vadimp@nvidia.com>,
        Hans de Goede <hdegoede@redhat.com>,
        Mark Gross <markgross@kernel.org>,
        Michael Shych <michaelsh@nvidia.com>,
        LKML <linux-kernel@vger.kernel.org>,
        kernel-janitors@vger.kernel.org,
        platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH] platform: mellanox: Fix a resource leak in an error
 handling path in mlxplat_probe()
In-Reply-To: <8bd0a7944f0f4f1342333eaf8d92d8e9d5623110.1696141233.git.christophe.jaillet@wanadoo.fr>
Message-ID: <7d966897-56b5-4a53-3b75-dd90072e17@linux.intel.com>
References: <8bd0a7944f0f4f1342333eaf8d92d8e9d5623110.1696141233.git.christophe.jaillet@wanadoo.fr>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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@wanadoo.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:
>