Re: [PATCH v1 01/12] gna: add driver module

From: Maciej Kwapulinski <maciej.kwapulinski@linux.intel.com>
To: Andy Shevchenko <andy.shevchenko@gmail.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>
Subject: Re: [PATCH v1 01/12] gna: add driver module
Date: Fri, 19 Feb 2021 14:21:17 +0100	[thread overview]
Message-ID: <85blcgw5v6.fsf@linux.intel.com> (raw)
In-Reply-To: <CAHp75Vep0Fm1k_7gJcozk4t316QmUgt5Qe3PauwDg=py5VnHfQ@mail.gmail.com>

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.

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?