Re: [PATCH net-next v3 2/3] net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller

[Philipp Stanner <phasta@mailbox.org>]

On Mon, 2025-12-08 at 10:15 +0000, Russell King (Oracle) wrote:
> On Mon, Dec 08, 2025 at 10:54:36AM +0100, Philipp Stanner wrote:
> > The bad news is that it's not super trivial to remove. I looked into it
> > about two times and decided I can't invest that time currently. You
> > need to go over all drivers again to see who uses pcim_enable_device(),
> > then add free_irq_vecs() for them all and so on…
> 
> So that I can confirm, you're saying that all drivers that call
> pci_alloc_irq_vectors() should call pci_free_irq_vectors() in their
> ->remove() method and not rely on the devres behaviour that
> pcim_enable_device() will permit.

"permit" is kind of a generous word. This behavior is dangerous and
there were bugs because of that in the past, because it confused
programmers. See:

f00059b4c1b0 drm/vboxvideo: fix mapping leaks


pcim_enable_device() used to switch all sorts of functions into managed
mode. As far as I could figure out through git, back in 2009 it was
intended that ALL pci functions are switched into managed mode that
way. That's also how it was documented.

The ecosystem then fractured, however. Some functions were always
managed (pcim_), some never, and some sometimes.

I removed all "sometimes managed" functions since 2024. The last
remainder is MSI.

If we want to remove that, we need to:
   1. Find all drivers that rely on pci_free_irq_vectors() being run
      automatically. IOW those that use pcim_enable_device() + wrappers
      around pci_setup_msi_context().
   2. Port those drivers to do the free_irq_vecs manually, if it's not
      a problem if it's called twice. If that were a problem, those
      drivers would also need to replace pcim_enable_device() with
      pci_enable_device().
   3. Once all drivers are ported, remove the devres code from msi.c
   4. Do associated cleanup work in PCI.

> 
> In terms of whether it's safe to call this twice, pci_free_irq_vectors()
> calls pci_disable_msix() and pci_disable_msi().
> 
> pci_disable_msix() checks:
> 
>         if (!pci_msi_enabled() || !dev || !dev->msix_enabled)
>                 return;
> 
> which will set dev->msix_enabled to 0 via pci_msix_shutdown().
> 
> pci_disable_msi() does a similar check:
> 
>         if (!pci_msi_enabled() || !dev || !dev->msi_enabled)
>                 return;
> 
> and similarly pci_msi_shutdown() sets dev->msi_enabled to 0.
> 
> So my conclusion is it's safe to call pci_free_irq_vectors() twice for
> the same device.
> 

Hm. Looks good.


P.