| Message ID | 8bd0a7944f0f4f1342333eaf8d92d8e9d5623110.1696141233.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform: mellanox: Fix a resource leak in an error handling path in mlxplat_probe() | expand |
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.
Hi Ilpo, Thank you very much for review. > -----Original Message----- > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Sent: Tuesday, 3 October 2023 15:06 > 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() > > 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 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: > >
On Tue, 3 Oct 2023, Vadim Pasternak wrote: > Hi Ilpo, > Thank you very much for review. > > > -----Original Message----- > > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Sent: Tuesday, 3 October 2023 15:06 > > 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() > > > > 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 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.
> -----Original Message----- > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Sent: Wednesday, 4 October 2023 11:52 > To: Vadim Pasternak <vadimp@nvidia.com> > Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>; 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() > > On Tue, 3 Oct 2023, Vadim Pasternak wrote: > > > Hi Ilpo, > > Thank you very much for review. > > > > > -----Original Message----- > > > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > Sent: Tuesday, 3 October 2023 15:06 > > > 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() > > > > > > 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 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.
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:
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. So Review with care! --- drivers/platform/x86/mlx-platform.c | 1 + 1 file changed, 1 insertion(+)