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

[Vadim Pasternak]

> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> Sent: Wednesday, 4 October 2023 11:52
> To: Vadim Pasternak <vadimp@xxxxxxxxxx>
> Cc: Christophe JAILLET <christophe.jaillet@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 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.

Hi Ilpo,

Thank you very much for your inputs.

I sent one bugfix and two amendment patches.

I just wanted to note, that in case of some comments for these
patches, I'll be able to make modifications only after 15 October.

Thanks,
Vadim.

> 
> 
> --
>  i.