2016-11-02 17:16:52

by Masahiro Yamada

Subject: [PATCH] mtd: nand: denali_pci: add missing pci_release_regions() calls

The probe function calls pci_request_regions(), but I do not see
corresponding pci_release_regions() calls.

While we are here, rename the jump labels to follow the guideline
"Choose label names which say what the goto does" suggested by
Documentation/CodingStyle.

Signed-off-by: Masahiro Yamada <[email protected]>
---

drivers/mtd/nand/denali_pci.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/denali_pci.c b/drivers/mtd/nand/denali_pci.c
index de31514..be8152f 100644
--- a/drivers/mtd/nand/denali_pci.c
+++ b/drivers/mtd/nand/denali_pci.c
@@ -76,28 +76,31 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
denali->flash_reg = ioremap_nocache(csr_base, csr_len);
if (!denali->flash_reg) {
dev_err(&dev->dev, "Spectra: Unable to remap memory region\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto release_regions;
}

denali->flash_mem = ioremap_nocache(mem_base, mem_len);
if (!denali->flash_mem) {
dev_err(&dev->dev, "Spectra: ioremap_nocache failed!");
ret = -ENOMEM;
- goto failed_remap_reg;
+ goto unmap_reg;
}

ret = denali_init(denali);
if (ret)
- goto failed_remap_mem;
+ goto unmap_mem;

pci_set_drvdata(dev, denali);

return 0;

-failed_remap_mem:
+unmap_mem:
iounmap(denali->flash_mem);
-failed_remap_reg:
+unmap_reg:
iounmap(denali->flash_reg);
+release_regions:
+ pci_release_regions(dev);
return ret;
}

@@ -109,6 +112,7 @@ static void denali_pci_remove(struct pci_dev *dev)
denali_remove(denali);
iounmap(denali->flash_reg);
iounmap(denali->flash_mem);
+ pci_release_regions(dev);
}

static struct pci_driver denali_pci_driver = {
--
1.9.1

2016-11-05 07:35:58

by Marek Vasut

Subject: Re: [PATCH] mtd: nand: denali_pci: add missing pci_release_regions() calls

On 11/02/2016 06:13 PM, Masahiro Yamada wrote:
> The probe function calls pci_request_regions(), but I do not see
> corresponding pci_release_regions() calls.
>
> While we are here, rename the jump labels to follow the guideline
> "Choose label names which say what the goto does" suggested by
> Documentation/CodingStyle.
>
> Signed-off-by: Masahiro Yamada <[email protected]>

Reviewed-by: Marek Vasut <[email protected]>

> ---
>
> drivers/mtd/nand/denali_pci.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali_pci.c b/drivers/mtd/nand/denali_pci.c
> index de31514..be8152f 100644
> --- a/drivers/mtd/nand/denali_pci.c
> +++ b/drivers/mtd/nand/denali_pci.c
> @@ -76,28 +76,31 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> denali->flash_reg = ioremap_nocache(csr_base, csr_len);
> if (!denali->flash_reg) {
> dev_err(&dev->dev, "Spectra: Unable to remap memory region\n");
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto release_regions;
> }
>
> denali->flash_mem = ioremap_nocache(mem_base, mem_len);
> if (!denali->flash_mem) {
> dev_err(&dev->dev, "Spectra: ioremap_nocache failed!");
> ret = -ENOMEM;
> - goto failed_remap_reg;
> + goto unmap_reg;
> }
>
> ret = denali_init(denali);
> if (ret)
> - goto failed_remap_mem;
> + goto unmap_mem;
>
> pci_set_drvdata(dev, denali);
>
> return 0;
>
> -failed_remap_mem:
> +unmap_mem:
> iounmap(denali->flash_mem);
> -failed_remap_reg:
> +unmap_reg:
> iounmap(denali->flash_reg);
> +release_regions:
> + pci_release_regions(dev);
> return ret;
> }
>
> @@ -109,6 +112,7 @@ static void denali_pci_remove(struct pci_dev *dev)
> denali_remove(denali);
> iounmap(denali->flash_reg);
> iounmap(denali->flash_mem);
> + pci_release_regions(dev);
> }
>
> static struct pci_driver denali_pci_driver = {
>


--
Best regards,
Marek Vasut

2016-11-06 22:55:54

by Boris Brezillon

Subject: Re: [PATCH] mtd: nand: denali_pci: add missing pci_release_regions() calls

On Thu, 3 Nov 2016 02:13:10 +0900
Masahiro Yamada <[email protected]> wrote:

> The probe function calls pci_request_regions(), but I do not see
> corresponding pci_release_regions() calls.
>
> While we are here, rename the jump labels to follow the guideline
> "Choose label names which say what the goto does" suggested by
> Documentation/CodingStyle.
>
> Signed-off-by: Masahiro Yamada <[email protected]>

Applied.

Thanks,

Boris

> ---
>
> drivers/mtd/nand/denali_pci.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali_pci.c b/drivers/mtd/nand/denali_pci.c
> index de31514..be8152f 100644
> --- a/drivers/mtd/nand/denali_pci.c
> +++ b/drivers/mtd/nand/denali_pci.c
> @@ -76,28 +76,31 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> denali->flash_reg = ioremap_nocache(csr_base, csr_len);
> if (!denali->flash_reg) {
> dev_err(&dev->dev, "Spectra: Unable to remap memory region\n");
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto release_regions;
> }
>
> denali->flash_mem = ioremap_nocache(mem_base, mem_len);
> if (!denali->flash_mem) {
> dev_err(&dev->dev, "Spectra: ioremap_nocache failed!");
> ret = -ENOMEM;
> - goto failed_remap_reg;
> + goto unmap_reg;
> }
>
> ret = denali_init(denali);
> if (ret)
> - goto failed_remap_mem;
> + goto unmap_mem;
>
> pci_set_drvdata(dev, denali);
>
> return 0;
>
> -failed_remap_mem:
> +unmap_mem:
> iounmap(denali->flash_mem);
> -failed_remap_reg:
> +unmap_reg:
> iounmap(denali->flash_reg);
> +release_regions:
> + pci_release_regions(dev);
> return ret;
> }
>
> @@ -109,6 +112,7 @@ static void denali_pci_remove(struct pci_dev *dev)
> denali_remove(denali);
> iounmap(denali->flash_reg);
> iounmap(denali->flash_mem);
> + pci_release_regions(dev);
> }
>
> static struct pci_driver denali_pci_driver = {

2016-11-07 13:36:28

by Andy Shevchenko

Subject: Re: [PATCH] mtd: nand: denali_pci: add missing pci_release_regions() calls

On Thu, 2016-11-03 at 02:13 +0900, Masahiro Yamada wrote:
> The probe function calls pci_request_regions(), but I do not see
> corresponding pci_release_regions() calls.
>
> While we are here, rename the jump labels to follow the guideline
> "Choose label names which say what the goto does" suggested by
> Documentation/CodingStyle.

NACK!

Please, remove or revert this patch.
Everything is done in pcim_release() function.

>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
>
>  drivers/mtd/nand/denali_pci.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali_pci.c
> b/drivers/mtd/nand/denali_pci.c
> index de31514..be8152f 100644
> --- a/drivers/mtd/nand/denali_pci.c
> +++ b/drivers/mtd/nand/denali_pci.c
> @@ -76,28 +76,31 @@ static int denali_pci_probe(struct pci_dev *dev,
> const struct pci_device_id *id)
>   denali->flash_reg = ioremap_nocache(csr_base, csr_len);
>   if (!denali->flash_reg) {
>   dev_err(&dev->dev, "Spectra: Unable to remap memory
> region\n");
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto release_regions;
>   }
>  
>   denali->flash_mem = ioremap_nocache(mem_base, mem_len);
>   if (!denali->flash_mem) {
>   dev_err(&dev->dev, "Spectra: ioremap_nocache
> failed!");
>   ret = -ENOMEM;
> - goto failed_remap_reg;
> + goto unmap_reg;
>   }
>  
>   ret = denali_init(denali);
>   if (ret)
> - goto failed_remap_mem;
> + goto unmap_mem;
>  
>   pci_set_drvdata(dev, denali);
>  
>   return 0;
>  
> -failed_remap_mem:
> +unmap_mem:
>   iounmap(denali->flash_mem);
> -failed_remap_reg:
> +unmap_reg:
>   iounmap(denali->flash_reg);
> +release_regions:
> + pci_release_regions(dev);
>   return ret;
>  }
>  
> @@ -109,6 +112,7 @@ static void denali_pci_remove(struct pci_dev *dev)
>   denali_remove(denali);
>   iounmap(denali->flash_reg);
>   iounmap(denali->flash_mem);
> + pci_release_regions(dev);
>  }
>  
>  static struct pci_driver denali_pci_driver = {

--
Andy Shevchenko <[email protected]>
Intel Finland Oy

2016-11-07 13:52:45

by Boris Brezillon

Subject: Re: [PATCH] mtd: nand: denali_pci: add missing pci_release_regions() calls

On Mon, 07 Nov 2016 15:33:04 +0200
Andy Shevchenko <[email protected]> wrote:

> On Thu, 2016-11-03 at 02:13 +0900, Masahiro Yamada wrote:
> > The probe function calls pci_request_regions(), but I do not see
> > corresponding pci_release_regions() calls.
> >
> > While we are here, rename the jump labels to follow the guideline
> > "Choose label names which say what the goto does" suggested by
> > Documentation/CodingStyle.
>
> NACK!
>
> Please, remove or revert this patch.
> Everything is done in pcim_release() function.

Indeed. I'll drop the patch.

Thanks,

Boris

>
> >
> > Signed-off-by: Masahiro Yamada <[email protected]>
> > ---
> >
> >  drivers/mtd/nand/denali_pci.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/denali_pci.c
> > b/drivers/mtd/nand/denali_pci.c
> > index de31514..be8152f 100644
> > --- a/drivers/mtd/nand/denali_pci.c
> > +++ b/drivers/mtd/nand/denali_pci.c
> > @@ -76,28 +76,31 @@ static int denali_pci_probe(struct pci_dev *dev,
> > const struct pci_device_id *id)
> >   denali->flash_reg = ioremap_nocache(csr_base, csr_len);
> >   if (!denali->flash_reg) {
> >   dev_err(&dev->dev, "Spectra: Unable to remap memory
> > region\n");
> > - return -ENOMEM;
> > + ret = -ENOMEM;
> > + goto release_regions;
> >   }
> >  
> >   denali->flash_mem = ioremap_nocache(mem_base, mem_len);
> >   if (!denali->flash_mem) {
> >   dev_err(&dev->dev, "Spectra: ioremap_nocache
> > failed!");
> >   ret = -ENOMEM;
> > - goto failed_remap_reg;
> > + goto unmap_reg;
> >   }
> >  
> >   ret = denali_init(denali);
> >   if (ret)
> > - goto failed_remap_mem;
> > + goto unmap_mem;
> >  
> >   pci_set_drvdata(dev, denali);
> >  
> >   return 0;
> >  
> > -failed_remap_mem:
> > +unmap_mem:
> >   iounmap(denali->flash_mem);
> > -failed_remap_reg:
> > +unmap_reg:
> >   iounmap(denali->flash_reg);
> > +release_regions:
> > + pci_release_regions(dev);
> >   return ret;
> >  }
> >  
> > @@ -109,6 +112,7 @@ static void denali_pci_remove(struct pci_dev *dev)
> >   denali_remove(denali);
> >   iounmap(denali->flash_reg);
> >   iounmap(denali->flash_mem);
> > + pci_release_regions(dev);
> >  }
> >  
> >  static struct pci_driver denali_pci_driver = {
>