RE: [PATCH] platform: mellanox: Fix a resource leak in an error handling path in mlxplat_probe()

[Ilpo Järvinen]

On Tue, 3 Oct 2023, Vadim Pasternak wrote:

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

It's okay for it to remain as long as the init/deinit pairs properly in 
the end.

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

This can be and should be combined with step 1 (where .probe()'s rollback 
and .remove() would call it and not mlxplat_pre_exit() at all). It already 
makes the pairing okay on the logic level so only name pairing needs to be 
done after that.

You can do separate patch both with Fixes tags since these are kinda 
independent issues.

These 1+2 are what I suggested in 2).

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

Yes to 3 & 4.


-- 
 i.