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

On Thu, 2025-12-11 at 14:23 +0000, Yao Zi wrote:

On Mon, Dec 08, 2025 at 10:54:36AM +0100, Philipp Stanner wrote:

quoted

On Fri, 2025-12-05 at 16:16 -0600, Bjorn Helgaas wrote:

quoted

[+to Philipp, Thomas for MSI devres question]

On Fri, Dec 05, 2025 at 09:34:54AM +0000, Russell King (Oracle) wrote:

quoted

On Fri, Dec 05, 2025 at 05:31:34AM +0000, Yao Zi wrote:

quoted

On Mon, Nov 24, 2025 at 07:06:12PM +0000, Russell King (Oracle) wrote:

quoted

On Mon, Nov 24, 2025 at 04:32:10PM +0000, Yao Zi wrote:

...

quoted

quoted

quoted

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."

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.

Thanks for the clarification, would you mind me sending a patch to fix
the description, and also mention the automatic clean-up behavior
shouldn't be relied anymore in new code?

If I would mind if *you* send such a patch? I for sure wouldn't mind,
that's the entire idea of open source development ^^

I can of course review it if you +Cc me.

...

quoted

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…

Do you think adding an implementation of pcim_alloc_irq_vectors(), that
always call pci_free_irq_vectors() regardless whether the PCI device is
managed, will help the conversion?

This will make it more trival to rewrite drivers depending on the
automatic clean-up behavior: since calling pci_free_irq_vectors()
several times is okay, we could simply change pci_alloc_irq_vectors() to
pcim_alloc_irq_vectors(), without considering where to call
pci_free_irq_vectors().

Introducing pcim_alloc_irq_vectors() will also help newly-introduced
drivers to reduce duplicated code to handle resource clean-up.

That's in fact how I have cleaned up the hybrid nature that was present
until this year in pci_request_region() et al.:

https://lore.kernel.org/linux-pci/20250519112959.25487-2-phasta@kernel.org/ (local)

It's one way to do it. First port everyone who relies on managed
behavior to a pcim_ function, and once they're all ported, remove the
hybrid nature from the pci_ function.

So that works, yes. The real question is just whether one wants a pcim_
function for the irq vectors. My personal impression is that this looks
like a useful feature; but my expertise with MSI is a bit limited.
There's also this strange kernel-wide msi_enabled global bool.. 

I guess the best way to find out is to try implementing it.

In case of doubt, the boring unmanaged pci_ version is the safe choice.
One contributor around here has once called the managed versions "the
crazy devres voodoo" :p

(BTW, just to be sure, pcim_ functions must not be interconnected with
pcim_enable() in the future anymore, nor shall they use global state.
All PCI devres functionality should purely work on the device file
through pure devres. The pcim_enable() interconnection cam only from
the hybrid feature.)


P.

quoted

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

quoted

Regards
Philipp

Regards,
Yao Zi