Message-ID: <BN9PR12MB53810BC39ADE210092559CADAFC4A@BN9PR12MB5381.namprd12.prod.outlook.com>
Date:   Tue, 3 Oct 2023 14:44:37 +0000
From:   Vadim Pasternak <vadimp@...dia.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>
CC:     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" <kernel-janitors@...r.kernel.org>,
        "platform-driver-x86@...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()

Hi Ilpo,
Thank you very much for review.

> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> Sent: Tuesday, 3 October 2023 15:06
> 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 would prefer to keep mlxplat_i2c_mux_complition_notify() as separate
function. I am going to use it as a callback. 

I suggest I'll prepare the next patches:

1.
As a bugfix, fix provided by Christophe and additional cleanup in
mlxplat_i2c_main_init():

@@ -6514,6 +6514,10 @@ static int mlxplat_i2c_main_init(struct mlxplat_priv *priv)
        return 0;
 
 fail_mlxplat_i2c_mux_topology_init:
+       if (priv->pdev_i2c) {
+               platform_device_unregister(priv->pdev_i2c);
+               priv->pdev_i2c = NULL;
+       }
 fail_platform_i2c_register:
 fail_mlxplat_mlxcpld_verify_bus_topology:
        return err;
@@ -6598,6 +6602,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:

2.
Move mlxplat_pre_exit() inside mlxplat_i2c_main_exit()

3.
Fix of ' complition' misspelling:
s/mlxplat_i2c_main_complition_notify/ mlxplat_i2c_main_completion_notify

4.

Renaming:
mlxplat_pre_init()/mlxplat_post_exit() to
	mlxplat_logicdev_init()/mlxplat_logicdev_exit()

mlxplat_i2c_main_init()/mlxplat_i2c_main_exit() keep as is.

mlxplat_post_init()/mlxplat_pre_exit() to
	mlxplat_platdevs_init()/mlxplat_platdevs_exit()

What do you think?

Thanks,
Vadim.

> --
>  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