Hi Ilpo,
Thank you very much for review.
> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> Sent: Tuesday, 3 October 2023 15:06
> To: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
> Cc: Vadim Pasternak <vadimp@xxxxxxxxxx>; Hans de Goede
> <hdegoede@xxxxxxxxxx>; Mark Gross <markgross@xxxxxxxxxx>; Michael
> Shych <michaelsh@xxxxxxxxxx>; LKML <linux-kernel@xxxxxxxxxxxxxxx>; kernel-
> janitors@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx
> 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@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 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:
> >
