[PATCH] Documentation: PCI: Clarify pci_free_irq_vectors() usage for managed devices

[PATCH] Documentation: PCI: Clarify pci_free_irq_vectors() usage for managed devices

[Shawn Lin]

Update the msi-howto.rst documentation to clarify that drivers using the
resource-managed function pcim_enable_device() must not call pci_free_irq_vectors().
This is because such devices have automatic IRQ vector management via pcim_msi_release(),
which is registered by pci_setup_msi_context() when pdev->is_managed is true.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 Documentation/PCI/msi-howto.rst | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/PCI/msi-howto.rst b/Documentation/PCI/msi-howto.rst
index 667ebe2..dc85f1e 100644
--- a/Documentation/PCI/msi-howto.rst
+++ b/Documentation/PCI/msi-howto.rst
@@ -113,8 +113,11 @@ vectors, use the following function::
 
   int pci_irq_vector(struct pci_dev *dev, unsigned int nr);
 
-Any allocated resources should be freed before removing the device using
-the following function::
+If the driver enables the device using the resource-managed function
+pcim_enable_device(), the driver shouldn't call pci_free_irq_vectors()
+because the IRQ vectors are automatically managed. Otherwise, the driver
+should free any allocated IRQ vectors before removing the device using the
+following function::
 
   void pci_free_irq_vectors(struct pci_dev *dev);
 
-- 
2.7.4

Re: [PATCH] Documentation: PCI: Clarify pci_free_irq_vectors() usage for managed devices

[Bjorn Helgaas]

[+cc Philipp, any comment? would like your ack if possible]

On Mon, Jan 26, 2026 at 10:57:13AM +0800, Shawn Lin wrote:
> Update the msi-howto.rst documentation to clarify that drivers using the
> resource-managed function pcim_enable_device() must not call pci_free_irq_vectors().
> This is because such devices have automatic IRQ vector management via pcim_msi_release(),
> which is registered by pci_setup_msi_context() when pdev->is_managed is true.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
> 
>  Documentation/PCI/msi-howto.rst | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/PCI/msi-howto.rst b/Documentation/PCI/msi-howto.rst
> index 667ebe2..dc85f1e 100644
> --- a/Documentation/PCI/msi-howto.rst
> +++ b/Documentation/PCI/msi-howto.rst
> @@ -113,8 +113,11 @@ vectors, use the following function::
>  
>    int pci_irq_vector(struct pci_dev *dev, unsigned int nr);
>  
> -Any allocated resources should be freed before removing the device using
> -the following function::
> +If the driver enables the device using the resource-managed function
> +pcim_enable_device(), the driver shouldn't call pci_free_irq_vectors()
> +because the IRQ vectors are automatically managed. Otherwise, the driver
> +should free any allocated IRQ vectors before removing the device using the
> +following function::
>  
>    void pci_free_irq_vectors(struct pci_dev *dev);
>  
> -- 
> 2.7.4
> 

Re: [PATCH] Documentation: PCI: Clarify pci_free_irq_vectors() usage for managed devices

[Philipp Stanner]

On Fri, 2026-02-06 at 16:33 -0600, Bjorn Helgaas wrote:
> [+cc Philipp, any comment? would like your ack if possible]

The patch is a good idea and desirable. I think, however, that it
should be a series with a second patch informing about this behavior in
pci_free_irq_vectors() docu, too. I think that more people read
function API documentation than the generated full docs, especially
when hacking something together quickly.

> 
> On Mon, Jan 26, 2026 at 10:57:13AM +0800, Shawn Lin wrote:
> > Update the msi-howto.rst documentation to clarify that drivers using the
> > resource-managed function pcim_enable_device()
> > 

pcim_enable_device() can be called a "resource-managed function"
because itself is managed in the sense of it setting up a consequence
for its own actions, which is automatic device-enablement. I.e., after
calling it, calling pci_disable_device() becomes obsolete.

That's, however, not decisive here. What is decisive is that it also
switches those MSI functions into "hybrid mode" so that they suddenly
have side-effects. So pcim_enable_device() does *two* things.

Depending on what you want to achieve with your detailed commit
message, you might want to point that out. Some inspiration might come
from my commits in various drivers. To make it bullet proof, I would
say sth like:

"For legacy reasons, pcim_enable_device() switched several, normally
un-managed, functions into managed mode. Since various cleanups, the
only function affected in such a way by pcim_enable_device() today are
callers of pcim_setup_msi_release(). This behavior is dangerous and
confusing and should be removed from the kernel."

depending on how verbose you want to be. This could be merged with the
below, of course.

> >  must not call pci_free_irq_vectors().
> > This is because such devices have automatic IRQ vector management via pcim_msi_release(),
> > which is registered by pci_setup_msi_context() when pdev->is_managed is true.

There is unfortunately also pdev->is_msi_managed in addition to
is_managed.

I think you don't need to hint at the internal implementation in the
commit message – but a separate patch with a code TODO in pci/msi/msi.c
where you point out that this is broken and should be removed sounds
like a good idea to me.

Could also be put on a PCI TODO list if there is one (Bjorn?)

> > 
> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> > ---
> > 
> >  Documentation/PCI/msi-howto.rst | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/PCI/msi-howto.rst b/Documentation/PCI/msi-howto.rst
> > index 667ebe2..dc85f1e 100644
> > --- a/Documentation/PCI/msi-howto.rst
> > +++ b/Documentation/PCI/msi-howto.rst
> > @@ -113,8 +113,11 @@ vectors, use the following function::
> >  
> >    int pci_irq_vector(struct pci_dev *dev, unsigned int nr);
> >  
> > -Any allocated resources should be freed before removing the device using
> > -the following function::
> > +If the driver enables the device using the resource-managed function
> > +pcim_enable_device(),

Same, "resource-managed" is not the decisive criterium. I would simply
state "pcim_enable_device() activates automatic management for IRQ
vectors".

(I'm a bit nitty; here it's not that important, but in the commit
message I would address this).


You can keep me on Cc, I can do a review for the next revision.

And thanks for picking this up!

Philipp

> >  the driver shouldn't call pci_free_irq_vectors()
> > +because the IRQ vectors are automatically managed. Otherwise, the driver
> > +should free any allocated IRQ vectors before removing the device using the
> > +following function::
> >  
> >    void pci_free_irq_vectors(struct pci_dev *dev);
> >  
> > -- 
> > 2.7.4
> > 

Re: [PATCH] Documentation: PCI: Clarify pci_free_irq_vectors() usage for managed devices

[Shawn Lin]

在 2026/02/09 星期一 16:05, Philipp Stanner 写道:
> On Fri, 2026-02-06 at 16:33 -0600, Bjorn Helgaas wrote:
>> [+cc Philipp, any comment? would like your ack if possible]
> 
> The patch is a good idea and desirable. I think, however, that it
> should be a series with a second patch informing about this behavior in
> pci_free_irq_vectors() docu, too. I think that more people read
> function API documentation than the generated full docs, especially
> when hacking something together quickly.
> 
>>
>> On Mon, Jan 26, 2026 at 10:57:13AM +0800, Shawn Lin wrote:
>>> Update the msi-howto.rst documentation to clarify that drivers using the
>>> resource-managed function pcim_enable_device()
>>>
> 
> pcim_enable_device() can be called a "resource-managed function"
> because itself is managed in the sense of it setting up a consequence
> for its own actions, which is automatic device-enablement. I.e., after
> calling it, calling pci_disable_device() becomes obsolete.
> 
> That's, however, not decisive here. What is decisive is that it also
> switches those MSI functions into "hybrid mode" so that they suddenly
> have side-effects. So pcim_enable_device() does *two* things.
> 
> Depending on what you want to achieve with your detailed commit
> message, you might want to point that out. Some inspiration might come
> from my commits in various drivers. To make it bullet proof, I would
> say sth like:
> 
> "For legacy reasons, pcim_enable_device() switched several, normally
> un-managed, functions into managed mode. Since various cleanups, the
> only function affected in such a way by pcim_enable_device() today are
> callers of pcim_setup_msi_release(). This behavior is dangerous and
> confusing and should be removed from the kernel."
> 
> depending on how verbose you want to be. This could be merged with the
> below, of course.
> 
>>>   must not call pci_free_irq_vectors().
>>> This is because such devices have automatic IRQ vector management via pcim_msi_release(),
>>> which is registered by pci_setup_msi_context() when pdev->is_managed is true.
> 
> There is unfortunately also pdev->is_msi_managed in addition to
> is_managed.
> 
> I think you don't need to hint at the internal implementation in the
> commit message – but a separate patch with a code TODO in pci/msi/msi.c
> where you point out that this is broken and should be removed sounds
> like a good idea to me.
> 
> Could also be put on a PCI TODO list if there is one (Bjorn?)
> 
>>>
>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>> ---
>>>
>>>   Documentation/PCI/msi-howto.rst | 7 +++++--
>>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/PCI/msi-howto.rst b/Documentation/PCI/msi-howto.rst
>>> index 667ebe2..dc85f1e 100644
>>> --- a/Documentation/PCI/msi-howto.rst
>>> +++ b/Documentation/PCI/msi-howto.rst
>>> @@ -113,8 +113,11 @@ vectors, use the following function::
>>>   
>>>     int pci_irq_vector(struct pci_dev *dev, unsigned int nr);
>>>   
>>> -Any allocated resources should be freed before removing the device using
>>> -the following function::
>>> +If the driver enables the device using the resource-managed function
>>> +pcim_enable_device(),
> 
> Same, "resource-managed" is not the decisive criterium. I would simply
> state "pcim_enable_device() activates automatic management for IRQ
> vectors".
> 
> (I'm a bit nitty; here it's not that important, but in the commit
> message I would address this).
> 
> 
> You can keep me on Cc, I can do a review for the next revision.
> 

Thanks for these helpful review, will work on v2.

> And thanks for picking this up!
> 
> Philipp
> 
>>>   the driver shouldn't call pci_free_irq_vectors()
>>> +because the IRQ vectors are automatically managed. Otherwise, the driver
>>> +should free any allocated IRQ vectors before removing the device using the
>>> +following function::
>>>   
>>>     void pci_free_irq_vectors(struct pci_dev *dev);
>>>   
>>> -- 
>>> 2.7.4
>>>
> 
> 
>