Re: [PATCH] net: sungem: disable PCI device on remove

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The patch only balances the probe-time pci_enable_device(),
  but the driver takes a *second* PCI enable reference per interface-up…

--

net: sungem: disable PCI device on remove

The patch adds a pci_disable_device() call at the end of gem_remove_one()
so that the pci_enable_device() done in gem_init_one() is balanced after a
successful probe. Previously only the probe failure path disabled the
device.

> diff --git a/drivers/net/ethernet/sun/sungem.c b/drivers/net/ethernet/sun/sungem.c
> index dc5638d105dbb..6abf4d292802c 100644
> --- a/drivers/net/ethernet/sun/sungem.c
> +++ b/drivers/net/ethernet/sun/sungem.c
> @@ -2806,6 +2806,7 @@ static void gem_remove_one(struct pci_dev *pdev)
>  		iounmap(gp->regs);
>  		pci_release_regions(pdev);
>  		free_netdev(dev);
> +		pci_disable_device(pdev);
>  	}
>  }

[Severity: Medium]
Is one unconditional pci_disable_device() here enough to guarantee the
device is actually disabled at unbind?

sungem takes a second enable reference every time the interface is brought
up, in gem_open():

	/* Make sure PCI access and bus master are enabled */
	rc = pci_enable_device(gp->pdev);
	if (rc) {
		...
		gem_put_cell(gp);
		return -ENXIO;
	}
	return gem_do_start(dev);

If gem_do_start() fails, that reference does not appear to be released.
The request_irq() failure path there unwinds only the chip state and the
cell:

	rc = request_irq(gp->pdev->irq, gem_interrupt,
			 IRQF_SHARED, dev->name, (void *)dev);
	if (rc) {
		netdev_err(dev, "failed to request irq !\n");

		gem_reset(gp);
		gem_clean_rings(gp);
		gem_put_cell(gp);
		return rc;
	}

and gem_open() returns that error verbatim.

__dev_open() in net/core/dev.c does not call ndo_stop when ndo_open fails:

	if (!ret && ops->ndo_open)
		ret = ops->ndo_open(dev);

	netpoll_poll_enable(dev);

	if (ret)
		clear_bit(__LINK_STATE_START, &dev->state);

so gem_close(), which holds the matching pci_disable_device(gp->pdev), is
never reached and pdev->enable_cnt stays at 2 for the rest of the binding.

In that case, does the new call in gem_remove_one() only drop the count
from 2 to 1?  pci_disable_device() in drivers/pci/pci.c is reference
counted:

	if (atomic_dec_return(&dev->enable_cnt) != 0)
		return;

	pci_host_bridge_disable_device(dev);
	do_pci_disable_device(dev);
	dev->is_busmaster = 0;

which would leave memory decoding and bus mastering enabled on a device
with no driver bound, and no warning, since the dev_WARN_ONCE there only
fires for enable_cnt <= 0.

Should the gem_open()/gem_do_start() failure path also drop its enable
reference, for example a pci_disable_device() in the request_irq() error
block of gem_do_start(), or in gem_open() when gem_do_start() returns an
error?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916055913.2910349-1-lgs201920130244%40gmail.com