lkml.org 
[lkml]   [2021]   [Feb]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v1 01/12] gna: add driver module
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

\
 
 \ /
  Last update: 2021-02-19 15:38    [W:0.193 / U:5.932 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site