Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210216160525.5028-1-maciej.kwapulinski@linux.intel.com>
 <20210216160525.5028-2-maciej.kwapulinski@linux.intel.com>
 <CAHp75Vep0Fm1k_7gJcozk4t316QmUgt5Qe3PauwDg=py5VnHfQ@mail.gmail.com> <85blcgw5v6.fsf@linux.intel.com>
In-Reply-To: <85blcgw5v6.fsf@linux.intel.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Fri, 19 Feb 2021 16:36:18 +0200
Message-ID: <CAHp75VfvHGPC8bvDUDt0ge9dh-YEnH8ob5xV6iPQwODjWETqPg@mail.gmail.com>
Subject: Re: [PATCH v1 01/12] gna: add driver module
To:     Maciej Kwapulinski <maciej.kwapulinski@linux.intel.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Jonathan Corbet <corbet@lwn.net>,
        Derek Kiernan <derek.kiernan@xilinx.com>,
        Dragan Cvetic <dragan.cvetic@xilinx.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux Documentation List <linux-doc@vger.kernel.org>,
        Tomasz Jankowski <tomasz1.jankowski@intel.com>,
        Savo Novakovic <savox.novakovic@intel.com>,
        Jianxun Zhang <jianxun.zhang@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Feb 19, 2021 at 3:21 PM Maciej Kwapulinski
<maciej.kwapulinski@linux.intel.com> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> > On Tue, Feb 16, 2021 at 6:11 PM Maciej Kwapulinski
> > <maciej.kwapulinski@linux.intel.com> wrote:
> >>
> ....
> >> +err_clear_master:
> >> +       pci_clear_master(pcidev);
> >> +err_release_regions:
> >> +       pci_release_regions(pcidev);
> >> +end:
> >> +       dev_err(&pcidev->dev, "gna probe failed with %d\n", ret);
> >> +       return ret;
> >
> > These are all completely redundant.
> >
>
> following is refactor of gna_probe(), but without pci_release_regions(),
> smatch (v7fcfe259) produces warning:
>   drivers/misc/gna/gna_device.c:78 gna_probe() warn: 'pcidev' not
>   released on lines: 56,65.

Then the tool is mistaken.

> here's the code refactored:
>
> int gna_probe(struct pci_dev *pcidev, const struct pci_device_id *pci_id)
> {
>         struct gna_private *gna_priv;
>         int ret;
>
>         ret = pcim_enable_device(pcidev);
>         if (ret) {
>                 dev_err(&pcidev->dev, "pci device can't be enabled\n");
>                 return ret;
>         }
>
>         ret = pci_request_regions(pcidev, GNA_DRV_NAME);
>         if (ret)
>                 return ret;
>
>         ret = pci_set_dma_mask(pcidev, DMA_BIT_MASK(64));
>         if (ret) {
>                 dev_err(&pcidev->dev, "pci_set_dma_mask returned error %d\n", ret);
>                 return ret;
>         }
>
>         pci_set_master(pcidev);
>
>         /* init gna device */
>         gna_priv = devm_kzalloc(&pcidev->dev, sizeof(*gna_priv), GFP_KERNEL);
>         if (!gna_priv) {
>                 //pci_release_regions(pcidev);
>                 return -ENOMEM;                 // line 56
>         }
>         /* Map BAR0 */
>         gna_priv->bar0.iostart = pci_resource_start(pcidev, 0);
>         gna_priv->bar0.iosize = pci_resource_len(pcidev, 0);
>         gna_priv->bar0.mem_addr = pcim_iomap(pcidev, 0, 0);
>         if (!gna_priv->bar0.mem_addr) {
>                 //pci_release_regions(pcidev);
>                 dev_err(&pcidev->dev, "could not map BAR 0\n");
>                 return -EINVAL;               // line 65
>         }
>
>         dev_dbg(&pcidev->dev, "bar0 io start: 0x%llx\n", (unsigned long long)gna_priv->bar0.iostart);
>         dev_dbg(&pcidev->dev, "bar0 io size: %llu\n", (unsigned long long)gna_priv->bar0.iosize);
>         dev_dbg(&pcidev->dev, "bar0 memory address: %p\n", gna_priv->bar0.mem_addr);
>
>         ret = gna_dev_init(gna_priv, pcidev, pci_id);
>         if (ret) {
>                 dev_err(&pcidev->dev, "could not initialize gna private structure\n");
>                 return ret;
>         }
>
>         return 0;
> }
>
> I've also added 'noinline' directive to pci_release_regions(), to see if
> it is called by the core code on "rmmod gna", but can't see the call.
>
> Is the smatch tool that causes problems here?
> Do You suggest other way to handle the problem?

File a bug against that tool.

Before you are doing v2, I would suggest you to go thru all comments
and address them. Above "refactored" code doesn't even have a half of
the comments I gave being addressed. If you have concerns, please,
reply to the review email with a specific excerpt as a context.

-- 
With Best Regards,
Andy Shevchenko