Re: [RESEND PATCH v3 1/3] PCI/MSI: Add Devres managed IRQ vectors allocation

Shawn Lin
2026-04-24T07:23:50Z

在 2026/04/22 星期三 21:07, Philipp Stanner 写道:
> On Wed, 2026-04-22 at 16:36 +0800, Shawn Lin wrote:
>> Hi Philipp
>>
>> 在 2026/04/22 星期三 15:32, Philipp Stanner 写道:
>>> On Wed, 2026-04-22 at 10:38 +0800, Shawn Lin wrote:
>>>> The PCI/MSI subsystem suffers from a long-standing design issue where

...

>
> The biggest issue I see is that you add another hybrid function, even
> if it's temporary. pcim_alloc_irq_vectors() will sometimes be managed
> and sometimes not – until you have removed all users of the
> aforementioned functions, and finally performed step №4 from above.
>
> Note that even in the past, with the oldest, most broken version of the
> PCI devres API, pcim_ functions were *always* managed, completely
> independently from pci(m)_enable_device().
>
> What pcim_enable_device() did was implicitly switch some pci_
> functions, such as pci_request_regions(), into managed mode.
>
> The solution presented / intended by you would rely on:
> 1. New users of pcim_ being aware (through documentation for
> example) that this one pcim_ function is special and not always
> managed (for now).

Agreed. That could be limited by code check instead of document, though.

> 2. New users being nice and really using pcim_enable_device().
> 3. You being able to fully see it all through to the end, i.e., you
> not being switched to a different project, switching jobs and
> opening a bar in Hawaii etc. etc.
>

I've been contributing for over 10 years purely out of personal
interest, for non-employees' work like this series， no one's ever paid
me for it. So it shouldn't be an issue at all for me. :))

> My experience has, unfortunately, been that people often don't read
> documentation, especially if they are already used to interfaces like
> that working a certain way.

+1

> How many drivers are there to be ported, do you have a list and maybe a
> time horizon? I took almost a year to remove my helper functions again.
>

I will add it to my v4.

> That's not a hard "NACK" from my side, it's just I wouldn't do it like
> that. I suppose it is for the maintainers to pick one of the two
> solution pathways:
>
> a) have another hybrid function temporarily or
> b) have duplicated code temporarily
>
> (where "temporarily" means until the contributor finishes the job :)
>

I fully understand your concern. The more straight way is to adopt
option b for sure. I will sent out my v4 soon.

Thanks for your suggestions, really appreciate!

>
> BTW, please keep in mind that in my opinion, once PCI/MSI is solved, I
> believe that pcim_enable_device() should do nothing anymore but disable
> the device on driver detach, without any other side effects.
>
>
> Regards
> P.
>
>>
>>
>>> The reason was that the hybrid behavior was burried so deep in the
>>> subsystem.
>>>
>>> See here:
>>>
>>> Here I had to reimplement some functions to be used by my new pcim_
>>> functions:
>>> https://lore.kernel.org/linux-pci/20240610093149.20640-4-pstanner@redhat.com/
>>>
>>>
>>> And here I could finally remove them later:
>>> https://lore.kernel.org/linux-pci/20250519112959.25487-7-phasta@kernel.org/
>>>
>>>
>>> I hope this helps. And again sorry for potentially causing you
>>> unnecessary work :(
>>>
>>>
>>> Regards
>>> Philipp
>>>
>>>
>>>> +}
>>>> +EXPORT_SYMBOL(pcim_alloc_irq_vectors_affinity);
>>>> +
>>>> +/**
>>>>    * pci_irq_vector() - Get Linux IRQ number of a device interrupt vector
>>>>    * @dev: the PCI device to operate on
>>>>    * @nr:  device-relative interrupt vector index (0-based); has different
>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>> index 2c44545..3716c67 100644
>>>> --- a/include/linux/pci.h
>>>> +++ b/include/linux/pci.h
>>>> @@ -1770,6 +1770,12 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
>>>>       unsigned int max_vecs, unsigned int flags,
>>>>       struct irq_affinity *affd);
>>>>
>>>> +int pcim_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
>>>> +   unsigned int max_vecs, unsigned int flags);
>>>> +int pcim_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
>>>> +    unsigned int max_vecs, unsigned int flags,
>>>> +    struct irq_affinity *affd);
>>>> +
>>>>   bool pci_msix_can_alloc_dyn(struct pci_dev *dev);
>>>>   struct msi_map pci_msix_alloc_irq_at(struct pci_dev *dev, unsigned int index,
>>>>         const struct irq_affinity_desc *affdesc);
>>>> @@ -1812,6 +1818,22 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
>>>>          flags, NULL);
>>>>   }
>>>>
>>>> +static inline int
>>>> +pcim_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
>>>> +        unsigned int max_vecs, unsigned int flags,
>>>> +        struct irq_affinity *aff_desc)
>>>> +{
>>>> + return pci_alloc_irq_vectors_affinity(dev, min_vecs, max_vecs,
>>>> +       flags, aff_desc);
>>>> +}
>>>> +static inline int
>>>> +pcim_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
>>>> +       unsigned int max_vecs, unsigned int flags)
>>>> +{
>>>> + return pcim_alloc_irq_vectors_affinity(dev, min_vecs, max_vecs,
>>>> +       flags, NULL);
>>>> +}
>>>> +
>>>>   static inline bool pci_msix_can_alloc_dyn(struct pci_dev *dev)
>>>>   { return false; }
>>>>   static inline struct msi_map pci_msix_alloc_irq_at(struct pci_dev *dev, unsigned int index,
>>>
>>>
>
>