Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 12 Jun 2019 10:37:39 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Simon =?iso-8859-1?Q?Sandstr=F6m?= <simon@nikanor.nu>
Cc:     gregkh@linuxfoundation.org, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] staging: kpc2000: improve label names in
 kp2000_pcie_probe
Message-ID: <20190612073739.GC1915@kadam>
References: <20190610200535.31820-1-simon@nikanor.nu>
 <20190610200535.31820-2-simon@nikanor.nu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190610200535.31820-2-simon@nikanor.nu>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thanks!

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

Not related to your patch (IOW ignore if you want to) the error handling
is slightly more complicated than required:

drivers/staging/kpc2000/kpc2000/core.c
   356           * Step 4: Setup the Register BAR
   357           */
   358          reg_bar_phys_addr = pci_resource_start(pcard->pdev, REG_BAR);
   359          reg_bar_phys_len = pci_resource_len(pcard->pdev, REG_BAR);
   360  
   361          pcard->regs_bar_base = ioremap_nocache(reg_bar_phys_addr, PAGE_SIZE);
   362          if (!pcard->regs_bar_base) {
   363                  dev_err(&pcard->pdev->dev,
   364                          "probe: REG_BAR could not remap memory to virtual space\n");
   365                  err = -ENODEV;
   366                  goto err_disable_device;
   367          }
   368          dev_dbg(&pcard->pdev->dev,
   369                  "probe: REG_BAR virt hardware address start [%p]\n",
   370                  pcard->regs_bar_base);
   371  
   372          err = pci_request_region(pcard->pdev, REG_BAR, KP_DRIVER_NAME_KP2000);
   373          if (err) {
   374                  iounmap(pcard->regs_bar_base);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
We could move this to the bottom of the function.  We would need to
re-order it slightly to free things in the mirror of how they are
allocated.  (Always just free the most recent allocation).

   375                  dev_err(&pcard->pdev->dev,
   376                          "probe: failed to acquire PCI region (%d)\n",
   377                          err);
   378                  err = -ENODEV;
   379                  goto err_disable_device;
   380          }
   381  
   382          pcard->regs_base_resource.start = reg_bar_phys_addr;
   383          pcard->regs_base_resource.end   = reg_bar_phys_addr +
   384                                            reg_bar_phys_len - 1;
   385          pcard->regs_base_resource.flags = IORESOURCE_MEM;
   386  
   387          /*
   388           * Step 5: Setup the DMA BAR
   389           */
   390          dma_bar_phys_addr = pci_resource_start(pcard->pdev, DMA_BAR);
   391          dma_bar_phys_len = pci_resource_len(pcard->pdev, DMA_BAR);
   392  
   393          pcard->dma_bar_base = ioremap_nocache(dma_bar_phys_addr,
   394                                                dma_bar_phys_len);
   395          if (!pcard->dma_bar_base) {
   396                  dev_err(&pcard->pdev->dev,
   397                          "probe: DMA_BAR could not remap memory to virtual space\n");
   398                  err = -ENODEV;
   399                  goto err_unmap_regs;
   400          }
   401          dev_dbg(&pcard->pdev->dev,
   402                  "probe: DMA_BAR virt hardware address start [%p]\n",
   403                  pcard->dma_bar_base);
   404  
   405          pcard->dma_common_regs = pcard->dma_bar_base + KPC_DMA_COMMON_OFFSET;
   406  
   407          err = pci_request_region(pcard->pdev, DMA_BAR, "kp2000_pcie");
   408          if (err) {
   409                  iounmap(pcard->dma_bar_base);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Same.

   410                  dev_err(&pcard->pdev->dev,
   411                          "probe: failed to acquire PCI region (%d)\n", err);
   412                  err = -ENODEV;
   413                  goto err_unmap_regs;
   414          }
   415  
   416          pcard->dma_base_resource.start = dma_bar_phys_addr;

[ snip ]

   509          dev_dbg(&pcard->pdev->dev, "%s() complete!\n", __func__);
   510          mutex_unlock(&pcard->sem);
   511          return 0;
   512  
   513  err_remove_sysfs:
   514          sysfs_remove_files(&pdev->dev.kobj, kp_attr_list);
   515  err_free_irq:
   516          free_irq(pcard->pdev->irq, pcard);
   517  err_disable_msi:
   518          pci_disable_msi(pcard->pdev);
   519  err_unmap_dma:
   520          iounmap(pcard->dma_bar_base);
   521          pci_release_region(pdev, DMA_BAR);
   522          pcard->dma_bar_base = NULL;
   523  err_unmap_regs:
   524          iounmap(pcard->regs_bar_base);
   525          pci_release_region(pdev, REG_BAR);
   526          pcard->regs_bar_base = NULL;

err_release_dma:
		pci_release_region(pdev, DMA_BAR);
err_unmap_dma:
		iounmap(pcard->dma_bar_base);
err_release_reg:
		pci_release_region(pdev, REG_BAR);
err_unmap_reg:
		iounmap(pcard->regs_bar_base);

I moved swapped the pci_release_region() and the iounmap() order.  There
is no need to set "pcard->regs_bar_base = NULL;" so just remove that.

   527  err_disable_device:
   528          pci_disable_device(pcard->pdev);
   529  err_remove_ida:
   530          mutex_unlock(&pcard->sem);
   531          ida_simple_remove(&card_num_ida, pcard->card_num);
   532  err_free_pcard:
   533          kfree(pcard);
   534          return err;
   535  }

regards,
dan carpenter