All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] 8250_lpss: check null return when calling pci_ioremap_bar
@ 2019-07-19  2:54 ` Navid Emamdoost
  0 siblings, 0 replies; 13+ messages in thread
From: Navid Emamdoost @ 2019-07-19  2:54 UTC (permalink / raw)
  Cc: emamd001, kjlu, smccaman, Navid Emamdoost, Greg Kroah-Hartman,
	Jiri Slaby, Andy Shevchenko, Vinod Koul, linux-serial,
	linux-kernel

pci_ioremap_bar may return null. This is eventually de-referenced at 
drivers/dma/dw/core.c:1154 and drivers/dma/dw/core.c:1168. A null check is
 needed to prevent null de-reference. I am adding the check and in case of
 failure returning -ENOMEM (I am not sure this is the best errno, you may 
consider it as a placeholder), and subsequently changing the caller’s 
return type, and propagating the error.

Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
 drivers/tty/serial/8250/8250_lpss.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index 53ca9ba6ab4b..5954b2e09b76 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -161,7 +161,7 @@ static const struct dw_dma_platform_data qrk_serial_dma_pdata = {
 	.multi_block = {0},
 };
 
-static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
+static int qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
 {
 	struct uart_8250_dma *dma = &lpss->dma;
 	struct dw_dma_chip *chip = &lpss->dma_chip;
@@ -172,12 +172,14 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
 	chip->dev = &pdev->dev;
 	chip->irq = pci_irq_vector(pdev, 0);
 	chip->regs = pci_ioremap_bar(pdev, 1);
+	if (!chip->regs)
+		return -ENOMEM;
 	chip->pdata = &qrk_serial_dma_pdata;
 
 	/* Falling back to PIO mode if DMA probing fails */
 	ret = dw_dma_probe(chip);
 	if (ret)
-		return;
+		return 0;
 
 	pci_try_set_mwi(pdev);
 
@@ -191,6 +193,7 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
 	param->hs_polarity = true;
 
 	lpss->dma_maxburst = 8;
+	return 0;
 }
 
 static void qrk_serial_exit_dma(struct lpss8250 *lpss)
@@ -219,7 +222,9 @@ static int qrk_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
 
 	port->irq = pci_irq_vector(pdev, 0);
 
-	qrk_serial_setup_dma(lpss, port);
+	ret = qrk_serial_setup_dma(lpss, port);
+	if (ret < 0)
+		return ret;
 	return 0;
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH] 8250_lpss: check null return when calling pci_ioremap_bar
@ 2019-07-19  2:54 ` Navid Emamdoost
  0 siblings, 0 replies; 13+ messages in thread
From: Navid Emamdoost @ 2019-07-19  2:54 UTC (permalink / raw)
  Cc: emamd001, kjlu, smccaman, Navid Emamdoost, Greg Kroah-Hartman,
	Jiri Slaby, Andy Shevchenko, Vinod Koul, linux-serial,
	linux-kernel

pci_ioremap_bar may return null. This is eventually de-referenced at 
drivers/dma/dw/core.c:1154 and drivers/dma/dw/core.c:1168. A null check is
 needed to prevent null de-reference. I am adding the check and in case of
 failure returning -ENOMEM (I am not sure this is the best errno, you may 
consider it as a placeholder), and subsequently changing the caller’s 
return type, and propagating the error.

Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
 drivers/tty/serial/8250/8250_lpss.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index 53ca9ba6ab4b..5954b2e09b76 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -161,7 +161,7 @@ static const struct dw_dma_platform_data qrk_serial_dma_pdata = {
 	.multi_block = {0},
 };
 
-static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
+static int qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
 {
 	struct uart_8250_dma *dma = &lpss->dma;
 	struct dw_dma_chip *chip = &lpss->dma_chip;
@@ -172,12 +172,14 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
 	chip->dev = &pdev->dev;
 	chip->irq = pci_irq_vector(pdev, 0);
 	chip->regs = pci_ioremap_bar(pdev, 1);
+	if (!chip->regs)
+		return -ENOMEM;
 	chip->pdata = &qrk_serial_dma_pdata;
 
 	/* Falling back to PIO mode if DMA probing fails */
 	ret = dw_dma_probe(chip);
 	if (ret)
-		return;
+		return 0;
 
 	pci_try_set_mwi(pdev);
 
@@ -191,6 +193,7 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
 	param->hs_polarity = true;
 
 	lpss->dma_maxburst = 8;
+	return 0;
 }
 
 static void qrk_serial_exit_dma(struct lpss8250 *lpss)
@@ -219,7 +222,9 @@ static int qrk_serial_setup(struct lpss8250 *lpss, struct uart_port *port)
 
 	port->irq = pci_irq_vector(pdev, 0);
 
-	qrk_serial_setup_dma(lpss, port);
+	ret = qrk_serial_setup_dma(lpss, port);
+	if (ret < 0)
+		return ret;
 	return 0;
 }
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] 8250_lpss: check null return when calling pci_ioremap_bar
  2019-07-19  2:54 ` Navid Emamdoost
  (?)
@ 2019-07-19 13:37 ` Andy Shevchenko
  2019-07-19 15:15   ` Andy Shevchenko
  -1 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2019-07-19 13:37 UTC (permalink / raw)
  To: Navid Emamdoost, Kangjie Lu, Aditya Pakki
  Cc: emamd001, smccaman, Greg Kroah-Hartman, Jiri Slaby, Vinod Koul,
	linux-serial, linux-kernel

On Thu, Jul 18, 2019 at 09:54:42PM -0500, Navid Emamdoost wrote:
> pci_ioremap_bar may return null. This is eventually de-referenced at 
> drivers/dma/dw/core.c:1154 and drivers/dma/dw/core.c:1168. A null check is
>  needed to prevent null de-reference. I am adding the check and in case of
>  failure returning -ENOMEM (I am not sure this is the best errno, you may 
> consider it as a placeholder), and subsequently changing the caller’s 
> return type, and propagating the error.

Thanks for the patch, my comments below.

>  	chip->irq = pci_irq_vector(pdev, 0);
>  	chip->regs = pci_ioremap_bar(pdev, 1);
> +	if (!chip->regs)
> +		return -ENOMEM;

This is the same case as below, it's fine to go on without DMA support.

>  	chip->pdata = &qrk_serial_dma_pdata;

So, I would rather to put like this...

Hold on, I remember someone already tried to fix this [1].

I dunno why it wasn't v5, due to [2].

Also, similar to yours, but wrong [3].

Thus, please, collaborate guys, and send one compiling solution  based on [1].

>  	/* Falling back to PIO mode if DMA probing fails */
>  	ret = dw_dma_probe(chip);
>  	if (ret)
> -		return;
> +		return 0;

[1]: https://www.spinics.net/lists/linux-serial/msg33965.html
[2]: https://lists.01.org/pipermail/kbuild-all/2019-March/059215.html
[3]: https://lore.kernel.org/patchwork/patch/1051000/

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] 8250_lpss: check null return when calling pci_ioremap_bar
  2019-07-19  2:54 ` Navid Emamdoost
@ 2019-07-19 13:58   ` kbuild test robot
  -1 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2019-07-19 13:58 UTC (permalink / raw)
  To: Navid Emamdoost
  Cc: kbuild-all, emamd001, kjlu, smccaman, Navid Emamdoost,
	Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko, Vinod Koul,
	linux-serial, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

Hi Navid,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.2 next-20190718]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Navid-Emamdoost/8250_lpss-check-null-return-when-calling-pci_ioremap_bar/20190719-120441
config: x86_64-randconfig-h002-201928 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/tty/serial/8250/8250_lpss.c: In function 'qrk_serial_setup':
>> drivers/tty/serial/8250/8250_lpss.c:225:6: error: void value not ignored as it ought to be
     ret = qrk_serial_setup_dma(lpss, port);
         ^

vim +225 drivers/tty/serial/8250/8250_lpss.c

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31667 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] 8250_lpss: check null return when calling pci_ioremap_bar
@ 2019-07-19 13:58   ` kbuild test robot
  0 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2019-07-19 13:58 UTC (permalink / raw)
  Cc: kbuild-all, emamd001, kjlu, smccaman, Navid Emamdoost,
	Greg Kroah-Hartman, Jiri Slaby, Andy Shevchenko, Vinod Koul,
	linux-serial, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]

Hi Navid,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.2 next-20190718]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Navid-Emamdoost/8250_lpss-check-null-return-when-calling-pci_ioremap_bar/20190719-120441
config: x86_64-randconfig-h002-201928 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/tty/serial/8250/8250_lpss.c: In function 'qrk_serial_setup':
>> drivers/tty/serial/8250/8250_lpss.c:225:6: error: void value not ignored as it ought to be
     ret = qrk_serial_setup_dma(lpss, port);
         ^

vim +225 drivers/tty/serial/8250/8250_lpss.c

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31667 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] 8250_lpss: check null return when calling pci_ioremap_bar
  2019-07-19 13:37 ` Andy Shevchenko
@ 2019-07-19 15:15   ` Andy Shevchenko
  2019-07-19 17:48     ` [PATCH v2] " Navid Emamdoost
  0 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2019-07-19 15:15 UTC (permalink / raw)
  To: Navid Emamdoost, Kangjie Lu, Aditya Pakki
  Cc: emamd001, smccaman, Greg Kroah-Hartman, Jiri Slaby, Vinod Koul,
	linux-serial, linux-kernel

On Fri, Jul 19, 2019 at 04:37:35PM +0300, Andy Shevchenko wrote:
> On Thu, Jul 18, 2019 at 09:54:42PM -0500, Navid Emamdoost wrote:

> Thus, please, collaborate guys, and send one compiling solution  based on [1].

> [1]: https://www.spinics.net/lists/linux-serial/msg33965.html

For your convenience (what has to be applied to v4):

diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index 53ca9ba6ab4b..6214da67e9b3 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -195,11 +195,15 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
 
 static void qrk_serial_exit_dma(struct lpss8250 *lpss)
 {
+	struct dw_dma_chip *chip = &lpss->dma_chip;
 	struct dw_dma_slave *param = &lpss->dma_param;
 
 	if (!param->dma_dev)
 		return;
-	dw_dma_remove(&lpss->dma_chip);
+
+	dw_dma_remove(chip);
+
+	pci_iounmap(to_pci_dev(chip->dev), chip->regs);
 }
 #else	/* CONFIG_SERIAL_8250_DMA */
 static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port) {}

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2] 8250_lpss: check null return when calling pci_ioremap_bar
  2019-07-19 15:15   ` Andy Shevchenko
@ 2019-07-19 17:48     ` Navid Emamdoost
  2019-07-19 22:36       ` Andy Shevchenko
       [not found]       ` <CGME20190726113223eucas1p287f8f2df03f66658bd492c592fd426e6@eucas1p2.samsung.com>
  0 siblings, 2 replies; 13+ messages in thread
From: Navid Emamdoost @ 2019-07-19 17:48 UTC (permalink / raw)
  To: andriy.shevchenko
  Cc: emamd001, kjlu, smccaman, Navid Emamdoost, Greg Kroah-Hartman,
	Jiri Slaby, Vinod Koul, linux-serial, linux-kernel

pci_ioremap_bar may return null. This is eventually de-referenced at 
drivers/dma/dw/core.c:1154 and drivers/dma/dw/core.c:1168. A null check 
is needed to prevent null de-reference. I am adding the check and in case
 of failure. Thanks to Andy Shevchenko for the hint on the necessity of 
pci_iounmap when exiting.

Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
 drivers/tty/serial/8250/8250_lpss.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index 53ca9ba6ab4b..d07e431110d9 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -169,10 +169,12 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
 	struct pci_dev *pdev = to_pci_dev(port->dev);
 	int ret;
 
+	chip->pdata = &qrk_serial_dma_pdata;
 	chip->dev = &pdev->dev;
 	chip->irq = pci_irq_vector(pdev, 0);
 	chip->regs = pci_ioremap_bar(pdev, 1);
-	chip->pdata = &qrk_serial_dma_pdata;
+	if (!chip->regs)
+		return;
 
 	/* Falling back to PIO mode if DMA probing fails */
 	ret = dw_dma_probe(chip);
@@ -195,11 +197,15 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
 
 static void qrk_serial_exit_dma(struct lpss8250 *lpss)
 {
+	struct dw_dma_chip *chip = &lpss->dma_chip;
 	struct dw_dma_slave *param = &lpss->dma_param;
 
 	if (!param->dma_dev)
 		return;
-	dw_dma_remove(&lpss->dma_chip);
+
+	dw_dma_remove(chip);
+
+	pci_iounmap(to_pci_dev(chip->dev), chip->regs);
 }
 #else	/* CONFIG_SERIAL_8250_DMA */
 static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port) {}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] 8250_lpss: check null return when calling pci_ioremap_bar
  2019-07-19 17:48     ` [PATCH v2] " Navid Emamdoost
@ 2019-07-19 22:36       ` Andy Shevchenko
       [not found]       ` <CGME20190726113223eucas1p287f8f2df03f66658bd492c592fd426e6@eucas1p2.samsung.com>
  1 sibling, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2019-07-19 22:36 UTC (permalink / raw)
  To: Navid Emamdoost
  Cc: Andy Shevchenko, emamd001, Kangjie Lu, smccaman,
	Greg Kroah-Hartman, Jiri Slaby, Vinod Koul,
	open list:SERIAL DRIVERS, Linux Kernel Mailing List

On Sat, Jul 20, 2019 at 12:45 AM Navid Emamdoost
<navid.emamdoost@gmail.com> wrote:
>
> pci_ioremap_bar may return null. This is eventually de-referenced at
> drivers/dma/dw/core.c:1154 and drivers/dma/dw/core.c:1168. A null check
> is needed to prevent null de-reference. I am adding the check and in case
>  of failure. Thanks to Andy Shevchenko for the hint on the necessity of
> pci_iounmap when exiting.
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
>  drivers/tty/serial/8250/8250_lpss.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
> index 53ca9ba6ab4b..d07e431110d9 100644
> --- a/drivers/tty/serial/8250/8250_lpss.c
> +++ b/drivers/tty/serial/8250/8250_lpss.c
> @@ -169,10 +169,12 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
>         struct pci_dev *pdev = to_pci_dev(port->dev);
>         int ret;
>
> +       chip->pdata = &qrk_serial_dma_pdata;
>         chip->dev = &pdev->dev;
>         chip->irq = pci_irq_vector(pdev, 0);
>         chip->regs = pci_ioremap_bar(pdev, 1);
> -       chip->pdata = &qrk_serial_dma_pdata;
> +       if (!chip->regs)
> +               return;
>
>         /* Falling back to PIO mode if DMA probing fails */
>         ret = dw_dma_probe(chip);
> @@ -195,11 +197,15 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
>
>  static void qrk_serial_exit_dma(struct lpss8250 *lpss)
>  {
> +       struct dw_dma_chip *chip = &lpss->dma_chip;
>         struct dw_dma_slave *param = &lpss->dma_param;
>
>         if (!param->dma_dev)
>                 return;
> -       dw_dma_remove(&lpss->dma_chip);
> +
> +       dw_dma_remove(chip);
> +
> +       pci_iounmap(to_pci_dev(chip->dev), chip->regs);
>  }
>  #else  /* CONFIG_SERIAL_8250_DMA */
>  static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port) {}
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] 8250_lpss: check null return when calling pci_ioremap_bar
       [not found]       ` <CGME20190726113223eucas1p287f8f2df03f66658bd492c592fd426e6@eucas1p2.samsung.com>
@ 2019-07-26 11:32         ` Bartlomiej Zolnierkiewicz
  2019-07-26 11:57           ` Andy Shevchenko
  0 siblings, 1 reply; 13+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2019-07-26 11:32 UTC (permalink / raw)
  To: Navid Emamdoost
  Cc: andriy.shevchenko, emamd001, kjlu, smccaman, Greg Kroah-Hartman,
	Jiri Slaby, Vinod Koul, linux-serial, linux-kernel


Hi,

On 7/19/19 7:48 PM, Navid Emamdoost wrote:
> pci_ioremap_bar may return null. This is eventually de-referenced at 
> drivers/dma/dw/core.c:1154 and drivers/dma/dw/core.c:1168. A null check 
> is needed to prevent null de-reference. I am adding the check and in case
>  of failure. Thanks to Andy Shevchenko for the hint on the necessity of 
> pci_iounmap when exiting.
> 
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
>  drivers/tty/serial/8250/8250_lpss.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
> index 53ca9ba6ab4b..d07e431110d9 100644
> --- a/drivers/tty/serial/8250/8250_lpss.c
> +++ b/drivers/tty/serial/8250/8250_lpss.c
> @@ -169,10 +169,12 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
>  	struct pci_dev *pdev = to_pci_dev(port->dev);
>  	int ret;
>  
> +	chip->pdata = &qrk_serial_dma_pdata;
>  	chip->dev = &pdev->dev;
>  	chip->irq = pci_irq_vector(pdev, 0);
>  	chip->regs = pci_ioremap_bar(pdev, 1);
> -	chip->pdata = &qrk_serial_dma_pdata;
> +	if (!chip->regs)
> +		return;
>  
>  	/* Falling back to PIO mode if DMA probing fails */
>  	ret = dw_dma_probe(chip);

pci_iounmap() should also be called on dw_dma_probe() failure (in such
case param->dma_dev is NULL so pci_iounmap() in qrk_serial_exit_dma()
won't be called during exit).

> @@ -195,11 +197,15 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
>  pci_iounmap
>  static void qrk_serial_exit_dma(struct lpss8250 *lpss)
>  {
> +	struct dw_dma_chip *chip = &lpss->dma_chip;
>  	struct dw_dma_slave *param = &lpss->dma_param;
>  
>  	if (!param->dma_dev)
>  		return;
> -	dw_dma_remove(&lpss->dma_chip);
> +
> +	dw_dma_remove(chip);
> +
> +	pci_iounmap(to_pci_dev(chip->dev), chip->regs);
>  }
>  #else	/* CONFIG_SERIAL_8250_DMA */
>  static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port) {}

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] 8250_lpss: check null return when calling pci_ioremap_bar
  2019-07-26 11:32         ` Bartlomiej Zolnierkiewicz
@ 2019-07-26 11:57           ` Andy Shevchenko
  2019-07-26 19:53             ` [PATCH v3] " Navid Emamdoost
  0 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2019-07-26 11:57 UTC (permalink / raw)
  To: Bartlomiej Zolnierkiewicz
  Cc: Navid Emamdoost, emamd001, kjlu, smccaman, Greg Kroah-Hartman,
	Jiri Slaby, Vinod Koul, linux-serial, linux-kernel

On Fri, Jul 26, 2019 at 01:32:21PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On 7/19/19 7:48 PM, Navid Emamdoost wrote:
> > pci_ioremap_bar may return null. This is eventually de-referenced at 
> > drivers/dma/dw/core.c:1154 and drivers/dma/dw/core.c:1168. A null check 
> > is needed to prevent null de-reference. I am adding the check and in case
> >  of failure. Thanks to Andy Shevchenko for the hint on the necessity of 
> > pci_iounmap when exiting.

> > @@ -169,10 +169,12 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
> >  	struct pci_dev *pdev = to_pci_dev(port->dev);
> >  	int ret;
> >  
> > +	chip->pdata = &qrk_serial_dma_pdata;
> >  	chip->dev = &pdev->dev;
> >  	chip->irq = pci_irq_vector(pdev, 0);
> >  	chip->regs = pci_ioremap_bar(pdev, 1);
> > -	chip->pdata = &qrk_serial_dma_pdata;
> > +	if (!chip->regs)
> > +		return;
> >  
> >  	/* Falling back to PIO mode if DMA probing fails */
> >  	ret = dw_dma_probe(chip);
> 
> pci_iounmap() should also be called on dw_dma_probe() failure (in such
> case param->dma_dev is NULL so pci_iounmap() in qrk_serial_exit_dma()
> won't be called during exit).

Oh, yes, good catch!
Navid, can you send a follow up fix?

> > @@ -195,11 +197,15 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
> >  pci_iounmap
> >  static void qrk_serial_exit_dma(struct lpss8250 *lpss)
> >  {
> > +	struct dw_dma_chip *chip = &lpss->dma_chip;
> >  	struct dw_dma_slave *param = &lpss->dma_param;
> >  
> >  	if (!param->dma_dev)
> >  		return;
> > -	dw_dma_remove(&lpss->dma_chip);
> > +
> > +	dw_dma_remove(chip);
> > +
> > +	pci_iounmap(to_pci_dev(chip->dev), chip->regs);
> >  }

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3] 8250_lpss: check null return when calling pci_ioremap_bar
  2019-07-26 11:57           ` Andy Shevchenko
@ 2019-07-26 19:53             ` Navid Emamdoost
       [not found]               ` <CAEkB2ERhxLj7ogoy1E3j8d4MyEZqroWS1tPRxyJXR2oLhNz+LQ@mail.gmail.com>
  0 siblings, 1 reply; 13+ messages in thread
From: Navid Emamdoost @ 2019-07-26 19:53 UTC (permalink / raw)
  To: andriy.shevchenko
  Cc: emamd001, kjlu, b.zolnierkie, gregkh, smccaman, secalert,
	Navid Emamdoost, Jiri Slaby, Vinod Koul, linux-serial,
	linux-kernel

pci_ioremap_bar may return null. This is eventually de-referenced at
drivers/dma/dw/core.c:1154 and drivers/dma/dw/core.c:1168. A null check
is needed to prevent null de-reference. I am adding the check and in case
 of failure. Thanks to Andy Shevchenko for the hint on the necessity of
pci_iounmap when exiting.
Update: also covered the dw_dma_probe() failure.

Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
 drivers/tty/serial/8250/8250_lpss.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_lpss.c b/drivers/tty/serial/8250/8250_lpss.c
index 53ca9ba6ab4b..19b356119ef6 100644
--- a/drivers/tty/serial/8250/8250_lpss.c
+++ b/drivers/tty/serial/8250/8250_lpss.c
@@ -169,15 +169,20 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
 	struct pci_dev *pdev = to_pci_dev(port->dev);
 	int ret;
 
+	chip->pdata = &qrk_serial_dma_pdata;
 	chip->dev = &pdev->dev;
 	chip->irq = pci_irq_vector(pdev, 0);
 	chip->regs = pci_ioremap_bar(pdev, 1);
-	chip->pdata = &qrk_serial_dma_pdata;
+	if (!chip->regs)
+		return;
 
 	/* Falling back to PIO mode if DMA probing fails */
 	ret = dw_dma_probe(chip);
-	if (ret)
+	if (ret) {
+		dw_dma_remove(chip);
+		pci_iounmap(to_pci_dev(chip->dev), chip->regs);
 		return;
+	}
 
 	pci_try_set_mwi(pdev);
 
@@ -195,11 +200,15 @@ static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port)
 
 static void qrk_serial_exit_dma(struct lpss8250 *lpss)
 {
+	struct dw_dma_chip *chip = &lpss->dma_chip;
 	struct dw_dma_slave *param = &lpss->dma_param;
 
 	if (!param->dma_dev)
 		return;
-	dw_dma_remove(&lpss->dma_chip);
+
+	dw_dma_remove(chip);
+
+	pci_iounmap(to_pci_dev(chip->dev), chip->regs);
 }
 #else	/* CONFIG_SERIAL_8250_DMA */
 static void qrk_serial_setup_dma(struct lpss8250 *lpss, struct uart_port *port) {}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v3] 8250_lpss: check null return when calling pci_ioremap_bar
       [not found]               ` <CAEkB2ERhxLj7ogoy1E3j8d4MyEZqroWS1tPRxyJXR2oLhNz+LQ@mail.gmail.com>
@ 2019-09-07 15:53                 ` Andy Shevchenko
       [not found]                   ` <CAEkB2ET-SQFiBbroxDFEVrPxto6a2wLJf0NM7R=ERcPargr66Q@mail.gmail.com>
  0 siblings, 1 reply; 13+ messages in thread
From: Andy Shevchenko @ 2019-09-07 15:53 UTC (permalink / raw)
  To: Navid Emamdoost
  Cc: Navid Emamdoost, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
	Jiri Slaby, Vinod Koul, linux-serial, linux-kernel

On Fri, Sep 06, 2019 at 05:32:39PM -0500, Navid Emamdoost wrote:

> I was wondering is anyone reviewing this patch?
> https://lore.kernel.org/patchwork/patch/1106267/

Why?

The one below is a part of upstraem

commit f5d6aadf3b6434f11393e33be9fd25a56d0bc872
Author: Navid Emamdoost <navid.emamdoost@gmail.com>
Date:   Fri Jul 19 12:48:45 2019 -0500

    8250_lpss: check null return when calling pci_ioremap_bar

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3] 8250_lpss: check null return when calling pci_ioremap_bar
       [not found]                   ` <CAEkB2ET-SQFiBbroxDFEVrPxto6a2wLJf0NM7R=ERcPargr66Q@mail.gmail.com>
@ 2019-09-07 16:16                     ` Andy Shevchenko
  0 siblings, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2019-09-07 16:16 UTC (permalink / raw)
  To: Navid Emamdoost
  Cc: Navid Emamdoost, Bartlomiej Zolnierkiewicz, Greg Kroah-Hartman,
	Jiri Slaby, Vinod Koul, linux-serial, linux-kernel

On Sat, Sep 07, 2019 at 11:06:29AM -0500, Navid Emamdoost wrote:
> Cool! I thought it is forgotten, as there were no response to the v3 of the
> patch.

Nevertheless, you may send a follow up, since one call to pci_iounmap() is
missed when dw_dma_probe() fails.

> Thanks for letting me know.

You're welcome.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-09-07 16:16 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-19  2:54 [PATCH] 8250_lpss: check null return when calling pci_ioremap_bar Navid Emamdoost
2019-07-19  2:54 ` Navid Emamdoost
2019-07-19 13:37 ` Andy Shevchenko
2019-07-19 15:15   ` Andy Shevchenko
2019-07-19 17:48     ` [PATCH v2] " Navid Emamdoost
2019-07-19 22:36       ` Andy Shevchenko
     [not found]       ` <CGME20190726113223eucas1p287f8f2df03f66658bd492c592fd426e6@eucas1p2.samsung.com>
2019-07-26 11:32         ` Bartlomiej Zolnierkiewicz
2019-07-26 11:57           ` Andy Shevchenko
2019-07-26 19:53             ` [PATCH v3] " Navid Emamdoost
     [not found]               ` <CAEkB2ERhxLj7ogoy1E3j8d4MyEZqroWS1tPRxyJXR2oLhNz+LQ@mail.gmail.com>
2019-09-07 15:53                 ` Andy Shevchenko
     [not found]                   ` <CAEkB2ET-SQFiBbroxDFEVrPxto6a2wLJf0NM7R=ERcPargr66Q@mail.gmail.com>
2019-09-07 16:16                     ` Andy Shevchenko
2019-07-19 13:58 ` [PATCH] " kbuild test robot
2019-07-19 13:58   ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.