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

[Philipp Stanner <phasta@mailbox.org>]

On Fri, 2025-12-05 at 16:16 -0600, Bjorn Helgaas wrote:
> [+to Philipp, Thomas for MSI devres question]
> 
> On Fri, Dec 05, 2025 at 09:34:54AM +0000, Russell King (Oracle) wrote:
> > On Fri, Dec 05, 2025 at 05:31:34AM +0000, Yao Zi wrote:
> > > On Mon, Nov 24, 2025 at 07:06:12PM +0000, Russell King (Oracle) wrote:
> > > > On Mon, Nov 24, 2025 at 04:32:10PM +0000, Yao Zi wrote:
> > > > > +static int motorcomm_setup_irq(struct pci_dev *pdev,
> > > > > +			       struct stmmac_resources *res,
> > > > > +			       struct plat_stmmacenet_data *plat)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +	ret = pci_alloc_irq_vectors(pdev, 6, 6, PCI_IRQ_MSIX);
> > > > > +	if (ret > 0) {
> > > > > +		res->rx_irq[0]	= pci_irq_vector(pdev, 0);
> > > > > +		res->tx_irq[0]	= pci_irq_vector(pdev, 4);
> > > > > +		res->irq	= pci_irq_vector(pdev, 5);
> > > > > +
> > > > > +		plat->flags |= STMMAC_FLAG_MULTI_MSI_EN;
> > > > > +
> > > > > +		return 0;
> > > > > +	}
> > > > > +
> > > > > +	dev_info(&pdev->dev, "failed to allocate MSI-X vector: %d\n", ret);
> > > > > +	dev_info(&pdev->dev, "try MSI instead\n");
> > > > > +
> > > > > +	ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> > > > > +	if (ret < 0)
> > > > > +		return dev_err_probe(&pdev->dev, ret,
> > > > > +				     "failed to allocate MSI\n");
> > > > > +
> > > > > +	res->irq = pci_irq_vector(pdev, 0);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int motorcomm_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > > > +{
> > > > ...
> > > > > +	ret = motorcomm_setup_irq(pdev, &res, plat);
> > > > > +	if (ret)
> > > > > +		return dev_err_probe(&pdev->dev, ret, "failed to setup IRQ\n");
> > > > > +
> > > > > +	motorcomm_init(priv);
> > > > > +
> > > > > +	res.addr = priv->base + GMAC_OFFSET;
> > > > > +
> > > > > +	return stmmac_dvr_probe(&pdev->dev, plat, &res);
> > > > 
> > > > If stmmac_dvr_probe() fails, then it will return an error code. This
> > > > leaves the PCI MSI interrupt allocated...
> > > 
> > > This isn't true. MSI API is a little magical: when the device is enabled

s/magical/confusing and explosive

;)

> > > through pcim_enable_device(), the device becomes devres-managed, and
> > > a plain call to pci_alloc_irq_vectors() becomes managed, too, even if
> > > its name doesn't indicate it's a devres-managed API.

Just to be clear: Callers of pci_setup_msi_context() are the last users
in PCI which express this strange behavior. All the other APIs behave
as one could expect from their name (pcim_ vs pci_).

> > > 
> > > pci_free_irq_vectors() will be automatically called on driver deattach.
> > > See pcim_setup_msi_release() in drivers/pci/msi/msi.c, which is invoked
> > > by pci_alloc_irq_vectors() internally.

Yes, this is correct.

> > 
> > This looks very non-intuitive, and the documentation for
> > pci_alloc_irq_vectors() doesn't help:
> > 
> >  * Upon a successful allocation, the caller should use pci_irq_vector()
> >  * to get the Linux IRQ number to be passed to request_threaded_irq().
> >  * The driver must call pci_free_irq_vectors() on cleanup.
> >    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > because if what you say is correct (and it looks like it is) then this
> > line is blatently incorrect.

True, this line is false. It should probably state "If you didn't
enable your PCI device with pcim_enable_device(), you must call
pci_free_irq_vectors() on cleanup."

I don't know whether calling pci_free_irq_vectors() is a bug (once
manually, once hidden by devres), though.

If it's not a bug, one could keep the docu that way or at least phrase
it in a way so that no additional users start relying on that hybrid
mechanism.

There is actually no reason anymore to call pcim_enable_device() at
all, other than that you get automatic device disablement.

> > 
> > Bjorn?

BTW, if PCI has a TODO list somewhere, removing that hybrid devres
feature for MSI should be on it, for obvious reasons.

The good news is that it's the last remainder of PCI hybrid devres and
getting rid of it would allow for removal of some additional code, too
(e.g., is_enabled bit and pcim_pin_device()).

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…

If you give me a pointer I can provide a TODO entry. In any case, feel
free to set me as a reviewer!

Regards
Philipp