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
