* [PATCH v2] tty: mxser: improve error handling in mxser_probe() and mxser_module_init() [not found] <51228a02.BjrxclRZRRerxqem%fengguang.wu@intel.com> @ 2013-02-19 5:57 ` Alexey Khoroshilov 2013-02-19 13:02 ` Jiri Slaby 2013-03-28 15:16 ` Jiri Slaby 0 siblings, 2 replies; 5+ messages in thread From: Alexey Khoroshilov @ 2013-02-19 5:57 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Alexey Khoroshilov, Jiri Slaby, linux-kernel, ldv-project 1. Currently mxser_probe() and mxser_module_init() ignore errors that can happen in tty_port_register_device(). 2. mxser_module_init() does not deallocate resources allocated in mxser_get_ISA_conf() if mxser_initbrd() failed. The patch adds proper error handling in all the cases. Also it moves free_irq() from mxser_release_ISA_res() to mxser_board_remove(), since it makes mxser_release_ISA_res() a counterpart for mxser_get_ISA_conf(), while free_irq() is relevant to both ISA and PCI boards. Found by Linux Driver Verification project (linuxtesting.org). Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> --- drivers/tty/mxser.c | 42 +++++++++++++++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c index 4011386..deeb5ad 100644 --- a/drivers/tty/mxser.c +++ b/drivers/tty/mxser.c @@ -2364,7 +2364,6 @@ static void mxser_release_vector(struct mxser_board *brd) static void mxser_release_ISA_res(struct mxser_board *brd) { - free_irq(brd->irq, brd); release_region(brd->ports[0].ioaddr, 8 * brd->info->nports); mxser_release_vector(brd); } @@ -2430,6 +2429,7 @@ static void mxser_board_remove(struct mxser_board *brd) tty_unregister_device(mxvar_sdriver, brd->idx + i); tty_port_destroy(&brd->ports[i].port); } + free_irq(brd->irq, brd); } static int __init mxser_get_ISA_conf(int cap, struct mxser_board *brd) @@ -2554,6 +2554,7 @@ static int mxser_probe(struct pci_dev *pdev, struct mxser_board *brd; unsigned int i, j; unsigned long ioaddress; + struct device *tty_dev; int retval = -EINVAL; for (i = 0; i < MXSER_BOARDS; i++) @@ -2637,13 +2638,25 @@ static int mxser_probe(struct pci_dev *pdev, if (retval) goto err_rel3; - for (i = 0; i < brd->info->nports; i++) - tty_port_register_device(&brd->ports[i].port, mxvar_sdriver, - brd->idx + i, &pdev->dev); + for (i = 0; i < brd->info->nports; i++) { + tty_dev = tty_port_register_device(&brd->ports[i].port, + mxvar_sdriver, brd->idx + i, &pdev->dev); + if (IS_ERR(tty_dev)) { + retval = PTR_ERR(tty_dev); + for (; i > 0; i--) + tty_unregister_device(mxvar_sdriver, + brd->idx + i - 1); + goto err_relbrd; + } + } pci_set_drvdata(pdev, brd); return 0; +err_relbrd: + for (i = 0; i < brd->info->nports; i++) + tty_port_destroy(&brd->ports[i].port); + free_irq(brd->irq, brd); err_rel3: pci_release_region(pdev, 3); err_zero: @@ -2665,7 +2678,6 @@ static void mxser_remove(struct pci_dev *pdev) mxser_board_remove(brd); - free_irq(pdev->irq, brd); pci_release_region(pdev, 2); pci_release_region(pdev, 3); pci_disable_device(pdev); @@ -2683,6 +2695,7 @@ static struct pci_driver mxser_driver = { static int __init mxser_module_init(void) { struct mxser_board *brd; + struct device *tty_dev; unsigned int b, i, m; int retval; @@ -2728,14 +2741,29 @@ static int __init mxser_module_init(void) /* mxser_initbrd will hook ISR. */ if (mxser_initbrd(brd, NULL) < 0) { + mxser_release_ISA_res(brd); brd->info = NULL; continue; } brd->idx = m * MXSER_PORTS_PER_BOARD; - for (i = 0; i < brd->info->nports; i++) - tty_port_register_device(&brd->ports[i].port, + for (i = 0; i < brd->info->nports; i++) { + tty_dev = tty_port_register_device(&brd->ports[i].port, mxvar_sdriver, brd->idx + i, NULL); + if (IS_ERR(tty_dev)) { + for (; i > 0; i--) + tty_unregister_device(mxvar_sdriver, + brd->idx + i - 1); + for (i = 0; i < brd->info->nports; i++) + tty_port_destroy(&brd->ports[i].port); + free_irq(brd->irq, brd); + mxser_release_ISA_res(brd); + brd->info = NULL; + break; + } + } + if (brd->info == NULL) + continue; m++; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] tty: mxser: improve error handling in mxser_probe() and mxser_module_init() 2013-02-19 5:57 ` [PATCH v2] tty: mxser: improve error handling in mxser_probe() and mxser_module_init() Alexey Khoroshilov @ 2013-02-19 13:02 ` Jiri Slaby 2013-03-28 15:16 ` Jiri Slaby 1 sibling, 0 replies; 5+ messages in thread From: Jiri Slaby @ 2013-02-19 13:02 UTC (permalink / raw) To: Alexey Khoroshilov, Greg Kroah-Hartman; +Cc: linux-kernel, ldv-project On 02/19/2013 06:57 AM, Alexey Khoroshilov wrote: > 1. Currently mxser_probe() and mxser_module_init() ignore errors > that can happen in tty_port_register_device(). > 2. mxser_module_init() does not deallocate resources allocated in mxser_get_ISA_conf() > if mxser_initbrd() failed. > > The patch adds proper error handling in all the cases. > Also it moves free_irq() from mxser_release_ISA_res() to mxser_board_remove(), > since it makes mxser_release_ISA_res() a counterpart for mxser_get_ISA_conf(), > while free_irq() is relevant to both ISA and PCI boards. > > Found by Linux Driver Verification project (linuxtesting.org). Looks good to me, thanks. > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> > --- > drivers/tty/mxser.c | 42 +++++++++++++++++++++++++++++++++++------- > 1 file changed, 35 insertions(+), 7 deletions(-) > > diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c > index 4011386..deeb5ad 100644 > --- a/drivers/tty/mxser.c > +++ b/drivers/tty/mxser.c > @@ -2364,7 +2364,6 @@ static void mxser_release_vector(struct mxser_board *brd) > > static void mxser_release_ISA_res(struct mxser_board *brd) > { > - free_irq(brd->irq, brd); > release_region(brd->ports[0].ioaddr, 8 * brd->info->nports); > mxser_release_vector(brd); > } > @@ -2430,6 +2429,7 @@ static void mxser_board_remove(struct mxser_board *brd) > tty_unregister_device(mxvar_sdriver, brd->idx + i); > tty_port_destroy(&brd->ports[i].port); > } > + free_irq(brd->irq, brd); > } > > static int __init mxser_get_ISA_conf(int cap, struct mxser_board *brd) > @@ -2554,6 +2554,7 @@ static int mxser_probe(struct pci_dev *pdev, > struct mxser_board *brd; > unsigned int i, j; > unsigned long ioaddress; > + struct device *tty_dev; > int retval = -EINVAL; > > for (i = 0; i < MXSER_BOARDS; i++) > @@ -2637,13 +2638,25 @@ static int mxser_probe(struct pci_dev *pdev, > if (retval) > goto err_rel3; > > - for (i = 0; i < brd->info->nports; i++) > - tty_port_register_device(&brd->ports[i].port, mxvar_sdriver, > - brd->idx + i, &pdev->dev); > + for (i = 0; i < brd->info->nports; i++) { > + tty_dev = tty_port_register_device(&brd->ports[i].port, > + mxvar_sdriver, brd->idx + i, &pdev->dev); > + if (IS_ERR(tty_dev)) { > + retval = PTR_ERR(tty_dev); > + for (; i > 0; i--) > + tty_unregister_device(mxvar_sdriver, > + brd->idx + i - 1); > + goto err_relbrd; > + } > + } > > pci_set_drvdata(pdev, brd); > > return 0; > +err_relbrd: > + for (i = 0; i < brd->info->nports; i++) > + tty_port_destroy(&brd->ports[i].port); > + free_irq(brd->irq, brd); > err_rel3: > pci_release_region(pdev, 3); > err_zero: > @@ -2665,7 +2678,6 @@ static void mxser_remove(struct pci_dev *pdev) > > mxser_board_remove(brd); > > - free_irq(pdev->irq, brd); > pci_release_region(pdev, 2); > pci_release_region(pdev, 3); > pci_disable_device(pdev); > @@ -2683,6 +2695,7 @@ static struct pci_driver mxser_driver = { > static int __init mxser_module_init(void) > { > struct mxser_board *brd; > + struct device *tty_dev; > unsigned int b, i, m; > int retval; > > @@ -2728,14 +2741,29 @@ static int __init mxser_module_init(void) > > /* mxser_initbrd will hook ISR. */ > if (mxser_initbrd(brd, NULL) < 0) { > + mxser_release_ISA_res(brd); > brd->info = NULL; > continue; > } > > brd->idx = m * MXSER_PORTS_PER_BOARD; > - for (i = 0; i < brd->info->nports; i++) > - tty_port_register_device(&brd->ports[i].port, > + for (i = 0; i < brd->info->nports; i++) { > + tty_dev = tty_port_register_device(&brd->ports[i].port, > mxvar_sdriver, brd->idx + i, NULL); > + if (IS_ERR(tty_dev)) { > + for (; i > 0; i--) > + tty_unregister_device(mxvar_sdriver, > + brd->idx + i - 1); > + for (i = 0; i < brd->info->nports; i++) > + tty_port_destroy(&brd->ports[i].port); > + free_irq(brd->irq, brd); > + mxser_release_ISA_res(brd); > + brd->info = NULL; > + break; > + } > + } > + if (brd->info == NULL) > + continue; > > m++; > } > -- js ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] tty: mxser: improve error handling in mxser_probe() and mxser_module_init() 2013-02-19 5:57 ` [PATCH v2] tty: mxser: improve error handling in mxser_probe() and mxser_module_init() Alexey Khoroshilov 2013-02-19 13:02 ` Jiri Slaby @ 2013-03-28 15:16 ` Jiri Slaby 2013-04-07 19:46 ` [PATCH] tty: mxser: fix cycle termination condition " Alexey Khoroshilov 1 sibling, 1 reply; 5+ messages in thread From: Jiri Slaby @ 2013-03-28 15:16 UTC (permalink / raw) To: Alexey Khoroshilov, Greg Kroah-Hartman; +Cc: linux-kernel, ldv-project Hi, back from Rome, I suppose ;). On 02/19/2013 06:57 AM, Alexey Khoroshilov wrote: > 1. Currently mxser_probe() and mxser_module_init() ignore errors > that can happen in tty_port_register_device(). > 2. mxser_module_init() does not deallocate resources allocated in mxser_get_ISA_conf() > if mxser_initbrd() failed. > > The patch adds proper error handling in all the cases. > Also it moves free_irq() from mxser_release_ISA_res() to mxser_board_remove(), > since it makes mxser_release_ISA_res() a counterpart for mxser_get_ISA_conf(), > while free_irq() is relevant to both ISA and PCI boards. > > Found by Linux Driver Verification project (linuxtesting.org). > > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> > --- > drivers/tty/mxser.c | 42 +++++++++++++++++++++++++++++++++++------- > 1 file changed, 35 insertions(+), 7 deletions(-) > > diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c > index 4011386..deeb5ad 100644 > --- a/drivers/tty/mxser.c > +++ b/drivers/tty/mxser.c > @@ -2364,7 +2364,6 @@ static void mxser_release_vector(struct mxser_board *brd) > > static void mxser_release_ISA_res(struct mxser_board *brd) > { > - free_irq(brd->irq, brd); > release_region(brd->ports[0].ioaddr, 8 * brd->info->nports); > mxser_release_vector(brd); > } > @@ -2430,6 +2429,7 @@ static void mxser_board_remove(struct mxser_board *brd) > tty_unregister_device(mxvar_sdriver, brd->idx + i); > tty_port_destroy(&brd->ports[i].port); > } > + free_irq(brd->irq, brd); > } > > static int __init mxser_get_ISA_conf(int cap, struct mxser_board *brd) > @@ -2554,6 +2554,7 @@ static int mxser_probe(struct pci_dev *pdev, > struct mxser_board *brd; > unsigned int i, j; > unsigned long ioaddress; > + struct device *tty_dev; > int retval = -EINVAL; > > for (i = 0; i < MXSER_BOARDS; i++) > @@ -2637,13 +2638,25 @@ static int mxser_probe(struct pci_dev *pdev, > if (retval) > goto err_rel3; > > - for (i = 0; i < brd->info->nports; i++) > - tty_port_register_device(&brd->ports[i].port, mxvar_sdriver, > - brd->idx + i, &pdev->dev); > + for (i = 0; i < brd->info->nports; i++) { > + tty_dev = tty_port_register_device(&brd->ports[i].port, > + mxvar_sdriver, brd->idx + i, &pdev->dev); > + if (IS_ERR(tty_dev)) { > + retval = PTR_ERR(tty_dev); > + for (; i > 0; i--) To me, it seems that v1 of the patch was merged. Could you fix this up? [Coverity revealed this.] thanks, -- js suse labs ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] tty: mxser: fix cycle termination condition in mxser_probe() and mxser_module_init() 2013-03-28 15:16 ` Jiri Slaby @ 2013-04-07 19:46 ` Alexey Khoroshilov 2013-04-08 7:18 ` Jiri Slaby 0 siblings, 1 reply; 5+ messages in thread From: Alexey Khoroshilov @ 2013-04-07 19:46 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Alexey Khoroshilov, Jiri Slaby, linux-kernel, ldv-project There is a bug in resources deallocation code in mxser_probe() and mxser_module_init(). As soon as variable 'i' is unsigned int, cycle termination condition i >= 0 is always true. The patch fixes the issue. Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> --- drivers/tty/mxser.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c index 484b6a3..302909c 100644 --- a/drivers/tty/mxser.c +++ b/drivers/tty/mxser.c @@ -2643,9 +2643,9 @@ static int mxser_probe(struct pci_dev *pdev, mxvar_sdriver, brd->idx + i, &pdev->dev); if (IS_ERR(tty_dev)) { retval = PTR_ERR(tty_dev); - for (i--; i >= 0; i--) + for (; i > 0; i--) tty_unregister_device(mxvar_sdriver, - brd->idx + i); + brd->idx + i - 1); goto err_relbrd; } } @@ -2751,9 +2751,9 @@ static int __init mxser_module_init(void) tty_dev = tty_port_register_device(&brd->ports[i].port, mxvar_sdriver, brd->idx + i, NULL); if (IS_ERR(tty_dev)) { - for (i--; i >= 0; i--) + for (; i > 0; i--) tty_unregister_device(mxvar_sdriver, - brd->idx + i); + brd->idx + i - 1); for (i = 0; i < brd->info->nports; i++) tty_port_destroy(&brd->ports[i].port); free_irq(brd->irq, brd); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] tty: mxser: fix cycle termination condition in mxser_probe() and mxser_module_init() 2013-04-07 19:46 ` [PATCH] tty: mxser: fix cycle termination condition " Alexey Khoroshilov @ 2013-04-08 7:18 ` Jiri Slaby 0 siblings, 0 replies; 5+ messages in thread From: Jiri Slaby @ 2013-04-08 7:18 UTC (permalink / raw) To: Alexey Khoroshilov, Greg Kroah-Hartman; +Cc: linux-kernel, ldv-project On 04/07/2013 09:46 PM, Alexey Khoroshilov wrote: > There is a bug in resources deallocation code in mxser_probe() and mxser_module_init(). > As soon as variable 'i' is unsigned int, cycle termination condition i >= 0 is always true. > The patch fixes the issue. > > Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> Yes, thanks. Acked-by: Jiri Slaby <jslaby@suse.cz> > --- > drivers/tty/mxser.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c > index 484b6a3..302909c 100644 > --- a/drivers/tty/mxser.c > +++ b/drivers/tty/mxser.c > @@ -2643,9 +2643,9 @@ static int mxser_probe(struct pci_dev *pdev, > mxvar_sdriver, brd->idx + i, &pdev->dev); > if (IS_ERR(tty_dev)) { > retval = PTR_ERR(tty_dev); > - for (i--; i >= 0; i--) > + for (; i > 0; i--) > tty_unregister_device(mxvar_sdriver, > - brd->idx + i); > + brd->idx + i - 1); > goto err_relbrd; > } > } > @@ -2751,9 +2751,9 @@ static int __init mxser_module_init(void) > tty_dev = tty_port_register_device(&brd->ports[i].port, > mxvar_sdriver, brd->idx + i, NULL); > if (IS_ERR(tty_dev)) { > - for (i--; i >= 0; i--) > + for (; i > 0; i--) > tty_unregister_device(mxvar_sdriver, > - brd->idx + i); > + brd->idx + i - 1); > for (i = 0; i < brd->info->nports; i++) > tty_port_destroy(&brd->ports[i].port); > free_irq(brd->irq, brd); > -- js suse labs ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-04-08 7:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <51228a02.BjrxclRZRRerxqem%fengguang.wu@intel.com>
2013-02-19 5:57 ` [PATCH v2] tty: mxser: improve error handling in mxser_probe() and mxser_module_init() Alexey Khoroshilov
2013-02-19 13:02 ` Jiri Slaby
2013-03-28 15:16 ` Jiri Slaby
2013-04-07 19:46 ` [PATCH] tty: mxser: fix cycle termination condition " Alexey Khoroshilov
2013-04-08 7:18 ` Jiri Slaby
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox