* [PATCH 0/3] misc: microchip: pci1xxxx: Fix the error handling path of gp_aux_bus_probe()
@ 2022-09-18 6:27 Christophe JAILLET
2022-09-18 6:27 ` [PATCH 1/3] misc: microchip: pci1xxxx: Do not disable the pci device twice in gp_aux_bus_remove() Christophe JAILLET
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Christophe JAILLET @ 2022-09-18 6:27 UTC (permalink / raw)
Cc: linux-kernel, kernel-janitors, Christophe JAILLET
The 3 patches are all related to error handling in the gp_aux_bus_probe() probe.
They have been splitted to ease review.
Christophe JAILLET (3):
misc: microchip: pci1xxxx: Do not disable the pci device twice in
gp_aux_bus_remove()
misc: microchip: pci1xxxx: Fix a memory leak in the error handling of
gp_aux_bus_probe()
misc: microchip: pci1xxxx: Fix the error handling paths of
gp_aux_bus_probe()
drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c | 78 ++++++++++---------
1 file changed, 40 insertions(+), 38 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] misc: microchip: pci1xxxx: Do not disable the pci device twice in gp_aux_bus_remove()
2022-09-18 6:27 [PATCH 0/3] misc: microchip: pci1xxxx: Fix the error handling path of gp_aux_bus_probe() Christophe JAILLET
@ 2022-09-18 6:27 ` Christophe JAILLET
2022-09-18 6:27 ` [PATCH 2/3] misc: microchip: pci1xxxx: Fix a memory leak in the error handling of gp_aux_bus_probe() Christophe JAILLET
2022-09-18 6:27 ` [PATCH 3/3] misc: microchip: pci1xxxx: Fix the error handling paths " Christophe JAILLET
2 siblings, 0 replies; 6+ messages in thread
From: Christophe JAILLET @ 2022-09-18 6:27 UTC (permalink / raw)
To: Kumaravel Thiagarajan, Arnd Bergmann, Greg Kroah-Hartman
Cc: linux-kernel, kernel-janitors, Christophe JAILLET, linux-gpio
gp_aux_bus_probe() uses pcim_enable_device(), so there is no point in
calling pci_disable_device() explicitly in the remove function.
Fixes: 393fc2f5948f ("misc: microchip: pci1xxxx: load auxiliary bus driver for the PIO function in the multi-function endpoint of pci1xxxx device.")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
index edff3ee73f6f..6c4f8384aa09 100644
--- a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
+++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
@@ -139,7 +139,6 @@ static void gp_aux_bus_remove(struct pci_dev *pdev)
auxiliary_device_delete(&aux_bus->aux_device_wrapper[1]->aux_dev);
auxiliary_device_uninit(&aux_bus->aux_device_wrapper[1]->aux_dev);
kfree(aux_bus);
- pci_disable_device(pdev);
}
static const struct pci_device_id pci1xxxx_tbl[] = {
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] misc: microchip: pci1xxxx: Fix a memory leak in the error handling of gp_aux_bus_probe()
2022-09-18 6:27 [PATCH 0/3] misc: microchip: pci1xxxx: Fix the error handling path of gp_aux_bus_probe() Christophe JAILLET
2022-09-18 6:27 ` [PATCH 1/3] misc: microchip: pci1xxxx: Do not disable the pci device twice in gp_aux_bus_remove() Christophe JAILLET
@ 2022-09-18 6:27 ` Christophe JAILLET
2022-09-18 6:27 ` [PATCH 3/3] misc: microchip: pci1xxxx: Fix the error handling paths " Christophe JAILLET
2 siblings, 0 replies; 6+ messages in thread
From: Christophe JAILLET @ 2022-09-18 6:27 UTC (permalink / raw)
To: Kumaravel Thiagarajan, Arnd Bergmann, Greg Kroah-Hartman
Cc: linux-kernel, kernel-janitors, Christophe JAILLET, linux-gpio
'aux_bus' is freed in the remove function but not in the error handling
path of the probe.
Use devm_kzalloc() to simplify the remove function and fix the leak in the
probe.
Fixes: 393fc2f5948f ("misc: microchip: pci1xxxx: load auxiliary bus driver for the PIO function in the multi-function endpoint of pci1xxxx device.")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
index 6c4f8384aa09..32af2b14ff34 100644
--- a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
+++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
@@ -38,7 +38,7 @@ static int gp_aux_bus_probe(struct pci_dev *pdev, const struct pci_device_id *id
if (retval)
return retval;
- aux_bus = kzalloc(sizeof(*aux_bus), GFP_KERNEL);
+ aux_bus = devm_kzalloc(&pdev->dev, sizeof(*aux_bus), GFP_KERNEL);
if (!aux_bus)
return -ENOMEM;
@@ -138,7 +138,6 @@ static void gp_aux_bus_remove(struct pci_dev *pdev)
auxiliary_device_uninit(&aux_bus->aux_device_wrapper[0]->aux_dev);
auxiliary_device_delete(&aux_bus->aux_device_wrapper[1]->aux_dev);
auxiliary_device_uninit(&aux_bus->aux_device_wrapper[1]->aux_dev);
- kfree(aux_bus);
}
static const struct pci_device_id pci1xxxx_tbl[] = {
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] misc: microchip: pci1xxxx: Fix the error handling paths of gp_aux_bus_probe()
2022-09-18 6:27 [PATCH 0/3] misc: microchip: pci1xxxx: Fix the error handling path of gp_aux_bus_probe() Christophe JAILLET
2022-09-18 6:27 ` [PATCH 1/3] misc: microchip: pci1xxxx: Do not disable the pci device twice in gp_aux_bus_remove() Christophe JAILLET
2022-09-18 6:27 ` [PATCH 2/3] misc: microchip: pci1xxxx: Fix a memory leak in the error handling of gp_aux_bus_probe() Christophe JAILLET
@ 2022-09-18 6:27 ` Christophe JAILLET
[not found] ` <202209181527.7wmi1NBN-lkp@intel.com>
2022-09-19 17:47 ` Kumaravel.Thiagarajan
2 siblings, 2 replies; 6+ messages in thread
From: Christophe JAILLET @ 2022-09-18 6:27 UTC (permalink / raw)
To: Kumaravel Thiagarajan, Arnd Bergmann, Greg Kroah-Hartman
Cc: linux-kernel, kernel-janitors, Christophe JAILLET, linux-gpio
There are several issues related to the error handling paths of
gp_aux_bus_probe():
- some resources may be released twice. Once explicitly in the error
handling path, and once via the release() function
- auxiliary_device_delete() should be called after the first successful
auxiliary_device_add()
To fix them, reorder the code:
- move the place where we get the irq for the 2nd wrapper.
- call kfree() and ida_free() after error checks, rather then in the
error handling path.
- have the error handling path look like the remove function
Fixes: 393fc2f5948f ("misc: microchip: pci1xxxx: load auxiliary bus driver for the PIO function in the multi-function endpoint of pci1xxxx device.")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is speculative and untested, review with care.
Other solutions are possible.
For example, we could use devm_kzalloc() to simplify the error handling
path and the release function.
---
drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c | 74 ++++++++++---------
1 file changed, 39 insertions(+), 35 deletions(-)
diff --git a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
index 32af2b14ff34..d3253e98f2ec 100644
--- a/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
+++ b/drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
@@ -32,7 +32,7 @@ static void gp_auxiliary_device_release(struct device *dev)
static int gp_aux_bus_probe(struct pci_dev *pdev, const struct pci_device_id *id)
{
struct aux_bus_device *aux_bus;
- int retval;
+ int irq, retval;
retval = pcim_enable_device(pdev);
if (retval)
@@ -48,8 +48,10 @@ static int gp_aux_bus_probe(struct pci_dev *pdev, const struct pci_device_id *id
return -ENOMEM;
retval = ida_alloc(&gp_client_ida, GFP_KERNEL);
- if (retval < 0)
- goto err_ida_alloc_0;
+ if (retval < 0) {
+ kfree(aux_bus->aux_device_wrapper[0]);
+ return retval;
+ }
aux_bus->aux_device_wrapper[0]->aux_dev.name = aux_dev_otp_e2p_name;
aux_bus->aux_device_wrapper[0]->aux_dev.dev.parent = &pdev->dev;
@@ -60,21 +62,38 @@ static int gp_aux_bus_probe(struct pci_dev *pdev, const struct pci_device_id *id
aux_bus->aux_device_wrapper[0]->gp_aux_data.region_length = pci_resource_end(pdev, 0);
retval = auxiliary_device_init(&aux_bus->aux_device_wrapper[0]->aux_dev);
- if (retval < 0)
- goto err_aux_dev_init_0;
+ if (retval < 0) {
+ ida_free(&gp_client_ida, aux_bus->aux_device_wrapper[0]->aux_dev.id);
+ kfree(aux_bus->aux_device_wrapper[0]);
+ goto err_aux_dev_uninit_0;
+ }
retval = auxiliary_device_add(&aux_bus->aux_device_wrapper[0]->aux_dev);
if (retval)
- goto err_aux_dev_add_0;
+ goto err_aux_dev_uninit_0;
+
+
+ retval = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
+ if (retval < 0)
+ goto err_aux_dev_del_0;
+
+ retval = pci_irq_vector(pdev, 0);
+ if (retval < 0)
+ goto err_aux_dev_del_0;
+ irq = retval;
aux_bus->aux_device_wrapper[1] = kzalloc(sizeof(*aux_bus->aux_device_wrapper[1]),
GFP_KERNEL);
- if (!aux_bus->aux_device_wrapper[1])
- return -ENOMEM;
+ if (!aux_bus->aux_device_wrapper[1]) {
+ retval = -ENOMEM;
+ goto err_aux_dev_del_0;
+ }
retval = ida_alloc(&gp_client_ida, GFP_KERNEL);
- if (retval < 0)
- goto err_ida_alloc_1;
+ if (retval < 0) {
+ kfree(aux_bus->aux_device_wrapper[1]);
+ goto err_aux_dev_del_0;
+ }
aux_bus->aux_device_wrapper[1]->aux_dev.name = aux_dev_gpio_name;
aux_bus->aux_device_wrapper[1]->aux_dev.dev.parent = &pdev->dev;
@@ -84,49 +103,34 @@ static int gp_aux_bus_probe(struct pci_dev *pdev, const struct pci_device_id *id
aux_bus->aux_device_wrapper[1]->gp_aux_data.region_start = pci_resource_start(pdev, 0);
aux_bus->aux_device_wrapper[1]->gp_aux_data.region_length = pci_resource_end(pdev, 0);
- retval = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
-
- if (retval < 0)
- goto err_aux_dev_init_1;
-
- retval = pci_irq_vector(pdev, 0);
- if (retval < 0)
- goto err_aux_dev_init_1;
-
pdev->irq = retval;
aux_bus->aux_device_wrapper[1]->gp_aux_data.irq_num = pdev->irq;
retval = auxiliary_device_init(&aux_bus->aux_device_wrapper[1]->aux_dev);
- if (retval < 0)
- goto err_aux_dev_init_1;
+ if (retval < 0) {
+ ida_free(&gp_client_ida, aux_bus->aux_device_wrapper[1]->aux_dev.id);
+ kfree(aux_bus->aux_device_wrapper[1]);
+ goto err_aux_dev_del_0;
+ }
retval = auxiliary_device_add(&aux_bus->aux_device_wrapper[1]->aux_dev);
if (retval)
- goto err_aux_dev_add_1;
+ goto err_aux_dev_uninit_1;
pci_set_drvdata(pdev, aux_bus);
pci_set_master(pdev);
return 0;
-err_aux_dev_add_1:
+err_aux_dev_uninit_1:
auxiliary_device_uninit(&aux_bus->aux_device_wrapper[1]->aux_dev);
-err_aux_dev_init_1:
- ida_free(&gp_client_ida, aux_bus->aux_device_wrapper[1]->aux_dev.id);
-
-err_ida_alloc_1:
- kfree(aux_bus->aux_device_wrapper[1]);
+err_aux_dev_del_0:
+ auxiliary_device_delete(&aux_bus->aux_device_wrapper[0]->aux_dev);
-err_aux_dev_add_0:
+err_aux_dev_uninit_0:
auxiliary_device_uninit(&aux_bus->aux_device_wrapper[0]->aux_dev);
-err_aux_dev_init_0:
- ida_free(&gp_client_ida, aux_bus->aux_device_wrapper[0]->aux_dev.id);
-
-err_ida_alloc_0:
- kfree(aux_bus->aux_device_wrapper[0]);
-
return retval;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] misc: microchip: pci1xxxx: Fix the error handling paths of gp_aux_bus_probe()
[not found] ` <202209181527.7wmi1NBN-lkp@intel.com>
@ 2022-09-18 9:25 ` Christophe JAILLET
0 siblings, 0 replies; 6+ messages in thread
From: Christophe JAILLET @ 2022-09-18 9:25 UTC (permalink / raw)
To: kernel test robot, Kumaravel Thiagarajan, Arnd Bergmann,
Greg Kroah-Hartman
Cc: kbuild-all, linux-kernel, kernel-janitors, linux-gpio
Le 18/09/2022 à 10:03, kernel test robot a écrit :
> Hi Christophe,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on char-misc/char-misc-testing]
> [also build test WARNING on next-20220916]
> [cannot apply to linus/master v6.0-rc5]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Christophe-JAILLET/misc-microchip-pci1xxxx-Fix-the-error-handling-path-of-gp_aux_bus_probe/20220918-143022
> base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git ceecbbddbf549fe0b7ffa3804a6e255b3360030f
> config: xtensa-randconfig-r022-20220918
> compiler: xtensa-linux-gcc (GCC) 12.1.0
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # https://github.com/intel-lab-lkp/linux/commit/089c1639fdebdad9be8de56c1546308eac15747d
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Christophe-JAILLET/misc-microchip-pci1xxxx-Fix-the-error-handling-path-of-gp_aux_bus_probe/20220918-143022
> git checkout 089c1639fdebdad9be8de56c1546308eac15747d
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=xtensa SHELL=/bin/bash drivers/misc/mchp_pci1xxxx/
>
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>):
>
> drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c: In function 'gp_aux_bus_probe':
>>> drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c:35:13: warning: variable 'irq' set but not used [-Wunused-but-set-variable]
> 35 | int irq, retval;
> | ^~~
>
>
> vim +/irq +35 drivers/misc/mchp_pci1xxxx/mchp_pci1xxxx_gp.c
>
[...]
> 75
> 76 retval = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
> 77 if (retval < 0)
> 78 goto err_aux_dev_del_0;
> 79
> 80 retval = pci_irq_vector(pdev, 0);
> 81 if (retval < 0)
> 82 goto err_aux_dev_del_0;
> 83 irq = retval;
We save the irq number here...
> 84
> 85 aux_bus->aux_device_wrapper[1] = kzalloc(sizeof(*aux_bus->aux_device_wrapper[1]),
> 86 GFP_KERNEL);
> 87 if (!aux_bus->aux_device_wrapper[1]) {
> 88 retval = -ENOMEM;
> 89 goto err_aux_dev_del_0;
> 90 }
> 91
> 92 retval = ida_alloc(&gp_client_ida, GFP_KERNEL);
> 93 if (retval < 0) {
> 94 kfree(aux_bus->aux_device_wrapper[1]);
> 95 goto err_aux_dev_del_0;
> 96 }
> 97
> 98 aux_bus->aux_device_wrapper[1]->aux_dev.name = aux_dev_gpio_name;
> 99 aux_bus->aux_device_wrapper[1]->aux_dev.dev.parent = &pdev->dev;
> 100 aux_bus->aux_device_wrapper[1]->aux_dev.dev.release = gp_auxiliary_device_release;
> 101 aux_bus->aux_device_wrapper[1]->aux_dev.id = retval;
> 102
> 103 aux_bus->aux_device_wrapper[1]->gp_aux_data.region_start = pci_resource_start(pdev, 0);
> 104 aux_bus->aux_device_wrapper[1]->gp_aux_data.region_length = pci_resource_end(pdev, 0);
> 105
> 106 pdev->irq = retval;
... then this should be:
pdev->irq = irq;
here.
I'll send a v2 in a few days.
Let see first if we get some other feedback.
CJ
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 3/3] misc: microchip: pci1xxxx: Fix the error handling paths of gp_aux_bus_probe()
2022-09-18 6:27 ` [PATCH 3/3] misc: microchip: pci1xxxx: Fix the error handling paths " Christophe JAILLET
[not found] ` <202209181527.7wmi1NBN-lkp@intel.com>
@ 2022-09-19 17:47 ` Kumaravel.Thiagarajan
1 sibling, 0 replies; 6+ messages in thread
From: Kumaravel.Thiagarajan @ 2022-09-19 17:47 UTC (permalink / raw)
To: christophe.jaillet, arnd, gregkh
Cc: linux-kernel, kernel-janitors, linux-gpio
> -----Original Message-----
> From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Sent: Sunday, September 18, 2022 11:58 AM
> To: Kumaravel Thiagarajan - I21417
> <Kumaravel.Thiagarajan@microchip.com>; Arnd Bergmann
> <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org;
> Christophe JAILLET <christophe.jaillet@wanadoo.fr>; linux-
> gpio@vger.kernel.org
> Subject: [PATCH 3/3] misc: microchip: pci1xxxx: Fix the error handling paths
> of gp_aux_bus_probe()
>
> There are several issues related to the error handling paths of
> gp_aux_bus_probe():
> - some resources may be released twice. Once explicitly in the error
> handling path, and once via the release() function
> - auxiliary_device_delete() should be called after the first successful
> auxiliary_device_add()
Thanks for your patch. I had noticed this after one of the reviewers had only partially fixed them
up. I need some time to review and test your patch with the hardware before approving them.
>
> To fix them, reorder the code:
> - move the place where we get the irq for the 2nd wrapper.
> - call kfree() and ida_free() after error checks, rather then in the
> error handling path.
> - have the error handling path look like the remove function
>
> Fixes: 393fc2f5948f ("misc: microchip: pci1xxxx: load auxiliary bus driver for
> the PIO function in the multi-function endpoint of pci1xxxx device.")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This patch is speculative and untested, review with care.
>
> Other solutions are possible.
> For example, we could use devm_kzalloc() to simplify the error handling path
> and the release function.
I thought about this but had some doubts whether it will work with the auxiliary bus
architecture as the memory allocated will get tagged with the same device pointer.
I need to do some experiments before taking up this path.
Thank You.
Regards,
Kumar
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-19 17:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-18 6:27 [PATCH 0/3] misc: microchip: pci1xxxx: Fix the error handling path of gp_aux_bus_probe() Christophe JAILLET
2022-09-18 6:27 ` [PATCH 1/3] misc: microchip: pci1xxxx: Do not disable the pci device twice in gp_aux_bus_remove() Christophe JAILLET
2022-09-18 6:27 ` [PATCH 2/3] misc: microchip: pci1xxxx: Fix a memory leak in the error handling of gp_aux_bus_probe() Christophe JAILLET
2022-09-18 6:27 ` [PATCH 3/3] misc: microchip: pci1xxxx: Fix the error handling paths " Christophe JAILLET
[not found] ` <202209181527.7wmi1NBN-lkp@intel.com>
2022-09-18 9:25 ` Christophe JAILLET
2022-09-19 17:47 ` Kumaravel.Thiagarajan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).