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

[Andy Shevchenko]

On Fri, Feb 19, 2021 at 3:21 PM Maciej Kwapulinski
<maciej.kwapulinski@xxxxxxxxxxxxxxx> wrote:
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> writes:
> > On Tue, Feb 16, 2021 at 6:11 PM Maciej Kwapulinski
> > <maciej.kwapulinski@xxxxxxxxxxxxxxx> 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